Xen devel - Mar 2011 - [PATCH 12/12] Nested Virtualization: hap-on-hap

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, 

Looks like you''ve sorted out shooting down old users of a p2m table. 
hap_write_p2m_entry still isn''t right, though:
> @@ -834,38 +864,81 @@ static void
>  hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p,
>                      mfn_t table_mfn, l1_pgentry_t new, unsigned int level)
>  {
> -    uint32_t old_flags;
> +    struct domain *d = v->domain;
> +    uint32_t old_flags = l1e_get_flags(*p);
You have moved this read outside the hap_lock.  Please put it back. 
> +    p2m_type_t op2mt = p2m_flags_to_type(old_flags);
>  
> -    hap_lock(v->domain);
> +    /* We know always use the host p2m here, regardless if the vcpu
> +     * is in host or guest mode. The vcpu can be in guest mode by
> +     * a hypercall which passes a domain and chooses mostly the first
> +     * vcpu.
> +     * XXX This is the reason why this function can not be used re-used
> +     * for updating the nestedp2m. Otherwise, hypercalls would randomly
> +     * operate on host p2m and nested p2m.
> +     */
> +    if ( nestedhvm_enabled(d)
> +      && p2m_is_valid(op2mt) )
> +    {
> +        if ( l1e_get_intpte(new) != l1e_get_intpte(*p) ) {
> +            p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new));
>  
> -    old_flags = l1e_get_flags(*p);
> +            /* Skip flush on vram tracking or XP mode in Win7 hang
> +             * very early in the virtual BIOS (long before the bootloader
> +             * runs), otherwise. VRAM tracking happens so often that
> +             * flushing and fixing the nestedp2m doesn''t let XP
mode
> +             * proceed to boot.
> +             */
> +            if ( !((op2mt == p2m_ram_rw && np2mt ==
p2m_ram_logdirty)
> +                || (op2mt == p2m_ram_logdirty && np2mt ==
p2m_ram_rw)) )
That''s not safe.  If the MFN has changed, you _have_ to flush, even if
you''re replacing a logdirty entry with a r/w one.   And if
you''re
replacing a r/w entry with a logdirty one, you _have_ to flush or
logdirty doesn''t work correctly.  If that case is too slow then you
should batch the flushes somehow, not just skip them.

Cheers,

Tim.
> +            {
> +                /* This GFN -> MFN is going to get removed. */
> +                /* XXX There is a more efficient way to do that
> +                 * but it works for now.
> +                 * Note, p2m_flush_nestedp2m calls hap_lock() internally.
> +                 */
> +                p2m_flush_nestedp2m(d);
> +            }
> +        }
> +    }
> +
> +    hap_lock(d);
> +
>      safe_write_pte(p, new);
>      if ( (old_flags & _PAGE_PRESENT)
>           && (level == 1 || (level == 2 && (old_flags &
_PAGE_PSE))) )
> -             flush_tlb_mask(&v->domain->domain_dirty_cpumask);
> +             flush_tlb_mask(&d->domain_dirty_cpumask);
>  
>  #if CONFIG_PAGING_LEVELS == 3
>      /* install P2M in monitor table for PAE Xen */
>      if ( level == 3 )
>          /* We have written to the p2m l3: need to sync the per-vcpu
>           * copies of it in the monitor tables */
> -        p2m_install_entry_in_monitors(v->domain, (l3_pgentry_t *)p);
> +        p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
>  #endif
>  
> -    hap_unlock(v->domain);
> +    hap_unlock(d);
>  }
-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

This is the new version. I fixed the open items from Tim''s last review.

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 03/31/11 17:25, Christoph Egger wrote:
>
> This is the new version. I fixed the open items from Tim''s last
review.
Sorry, I mistakenly resent an older version and noticed it just now.
This time this is the latest version.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

At 16:48 +0100 on 05 Apr (1302022090), Christoph Egger
wrote:
> On 03/31/11 17:25, Christoph Egger wrote:
> >
> > This is the new version. I fixed the open items from Tim''s
last review.
> 
> Sorry, I mistakenly resent an older version and noticed it just now.
> This time this is the latest version.
Thank you.  I have applied the full series, which should appear as
23157--23168 in the staging tree soon.  I had to forward-port a few
small things, and I also fixed up one last race condition in the remote
shootdowns, as 23170:86f87da1445a, so please check that I havent broken
anything in your tests.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/06/11 12:29, Tim Deegan wrote:
> At 16:48 +0100 on 05 Apr (1302022090), Christoph Egger wrote:
>> On 03/31/11 17:25, Christoph Egger wrote:
>>>
>>> This is the new version. I fixed the open items from Tim''s
last review.
>>
>> Sorry, I mistakenly resent an older version and noticed it just now.
>> This time this is the latest version.
>
> Thank you.  I have applied the full series, which should appear as
> 23157--23168 in the staging tree soon.  I had to forward-port a few
> small things, and I also fixed up one last race condition in the remote
> shootdowns, as 23170:86f87da1445a, so please check that I havent broken
> anything in your tests.
Thank you for applying the patch series. xentrace was broken again but 
that wasn''t you. I already submitted a fix for this.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 05.04.11 at 17:48, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>--- a/xen/include/asm-x86/domain.h
>+++ b/xen/include/asm-x86/domain.h
>...
>@@ -225,6 +227,7 @@ struct paging_vcpu {
> #define MAX_CPUID_INPUT 40
> typedef xen_domctl_cpuid_t cpuid_input_t;
> 
>+#define MAX_NESTEDP2M 10
> struct p2m_domain;
> struct time_scale {
>     int shift;
>@@ -258,6 +261,12 @@ struct arch_domain
>     struct paging_domain paging;
>     struct p2m_domain *p2m;
> 
>+    /* nestedhvm: translate l2 guest physical to host physical */
>+    struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>+    spinlock_t nested_p2m_lock;
>+    int nested_p2m_locker;
>+    const char *nested_p2m_function;
>+
>     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
>     int *irq_pirq;
>     int *pirq_irq;
Was there a specific reason to add this to struct arch_domain
instead of struct hvm_domain? I.e. can any pf these fields be
used on pv (or idle) domains?

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/11 11:03, Jan Beulich wrote:
>>>> On 05.04.11 at 17:48, Christoph
Egger<Christoph.Egger@amd.com>  wrote:
>> diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> ...
>> @@ -225,6 +227,7 @@ struct paging_vcpu {
>> #define MAX_CPUID_INPUT 40
>> typedef xen_domctl_cpuid_t cpuid_input_t;
>>
>> +#define MAX_NESTEDP2M 10
>> struct p2m_domain;
>> struct time_scale {
>>      int shift;
>> @@ -258,6 +261,12 @@ struct arch_domain
>>      struct paging_domain paging;
>>      struct p2m_domain *p2m;
>>
>> +    /* nestedhvm: translate l2 guest physical to host physical */
>> +    struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>> +    spinlock_t nested_p2m_lock;
>> +    int nested_p2m_locker;
>> +    const char *nested_p2m_function;
>> +
>>      /* NB. protected by d->event_lock and by irq_desc[irq].lock */
>>      int *irq_pirq;
>>      int *pirq_irq;
>
> Was there a specific reason to add this to struct arch_domain
> instead of struct hvm_domain? I.e. can any pf these fields be
> used on pv (or idle) domains?
The reason is that there is already a ''struct p2m_domain *p2m''
field.
If that can be moved to struct hvm_domain then  nested_p2m can
definitely move over to there, too.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 29.04.11 at 11:09, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> On 04/29/11 11:03, Jan Beulich wrote:
>>>>> On 05.04.11 at 17:48, Christoph
Egger<Christoph.Egger@amd.com>  wrote:
>>> diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> ...
>>> @@ -225,6 +227,7 @@ struct paging_vcpu {
>>> #define MAX_CPUID_INPUT 40
>>> typedef xen_domctl_cpuid_t cpuid_input_t;
>>>
>>> +#define MAX_NESTEDP2M 10
>>> struct p2m_domain;
>>> struct time_scale {
>>>      int shift;
>>> @@ -258,6 +261,12 @@ struct arch_domain
>>>      struct paging_domain paging;
>>>      struct p2m_domain *p2m;
>>>
>>> +    /* nestedhvm: translate l2 guest physical to host physical */
>>> +    struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>>> +    spinlock_t nested_p2m_lock;
>>> +    int nested_p2m_locker;
>>> +    const char *nested_p2m_function;
>>> +
>>>      /* NB. protected by d->event_lock and by irq_desc[irq].lock
*/
>>>      int *irq_pirq;
>>>      int *pirq_irq;
>>
>> Was there a specific reason to add this to struct arch_domain
>> instead of struct hvm_domain? I.e. can any pf these fields be
>> used on pv (or idle) domains?
> 
> The reason is that there is already a ''struct p2m_domain
*p2m'' field.
> If that can be moved to struct hvm_domain then  nested_p2m can
> definitely move over to there, too.
No, I don''t think these are connected - a pv domain can still require
a p2m (e.g. for the iommu), but I would have thought that the
nesting stuff doesn''t apply there.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel