Skip to contentSkip to conversation

nestedhvm: ASID emulation

PATCH In review Christoph Egger 3 participants 20 messages Apr 13, 2011 – Apr 15, 2011

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 nestedhvm: ASID emulation

Implement ASID emulation.
This allows the l1 guest to run the l2 guest using hw ASID.

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---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/arch/x86/hvm/asid.c +64 -18
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/asid.c
@@ -102,10 +102,14 @@ void hvm_asid_flush_core(void)
     data->disabled = 1;
 }
 
-bool_t hvm_asid_handle_vmenter(void)
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest)
 {
+    int need_flush = 0;
     struct vcpu *v = current;
     struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
+    struct nestedvcpu *nv;
+
+    nv = &vcpu_nestedhvm(v);
 
     /* On erratum #170 systems we must flush the TLB. 
      * Generation overruns are taken here, too. */

@@ -113,27 +117,69 @@ bool_t hvm_asid_handle_vmenter(void)
         goto disabled;
 
     /* Test if VCPU has valid ASID. */
-    if ( v->arch.hvm_vcpu.asid_generation == data->core_asid_generation )
-        return 0;
-
-    /* If there are no free ASIDs, need to go to a new generation */
-    if ( unlikely(data->next_asid > data->max_asid) )
-    {
-        hvm_asid_flush_core();
-        data->next_asid = 1;
-        if ( data->disabled )
-            goto disabled;
+    if ( v->arch.hvm_vcpu.asid_generation == data->core_asid_generation ) {
+        if ( run_n2guest ) {
+            if ( !nv->nv_new_vasid && data->next_asid > nv->nv_n2asid ) {
+                /* l1 guest doesn't request a new asid */
+                /* When asid generation changed last time when we were
+                 * were going to run l1 guest then
+                 * next_asid <= nv->nv_n2asid.
+                 */
+                ASSERT(nv->nv_n2asid != 0);
+                ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+                v->arch.hvm_vcpu.asid = nv->nv_n2asid;
+                return 0;
+            }
+        } else if ( data->next_asid > nv->nv_n1asid ) {
+            /* When asid generation changed last time when we were going to
+             * run l2 guest then next_asid <= nv->nv_n1asid.
+             */
+            ASSERT(nv->nv_n1asid != 0);
+            ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+            v->arch.hvm_vcpu.asid = nv->nv_n1asid;
+            return 0;
+        }
     }
 
-    /* Now guaranteed to be a free ASID. */
-    v->arch.hvm_vcpu.asid = data->next_asid++;
+    do {
+        /* If there are no free ASIDs, need to go to a new generation */
+        if ( unlikely(data->next_asid > data->max_asid) )
+        {
+            hvm_asid_flush_core();
+            data->next_asid = 1;
+            if ( data->disabled )
+                goto disabled;
+        }
+
+        if ( data->next_asid == 1 ) {
+            /* We start a new generation, so all old ASID allocations are
+             * stale now. Ensure we flush the TLB also in case of another
+             * iteration.
+             */
+            need_flush = 1;
+        }
+
+        /* Now guaranteed to be a free ASID. */
+        if ( run_n2guest )
+            /* nv_n1asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n2asid = data->next_asid++;
+        else
+            /* nv_n2asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n1asid = data->next_asid++;
+
+        /* Make sure an asid isn't used twice */
+    } while (nv->nv_n2asid == nv->nv_n1asid);
+
+    ASSERT(nv->nv_n1asid != 0);
+
+    v->arch.hvm_vcpu.asid = (run_n2guest) ? nv->nv_n2asid : nv->nv_n1asid;
     v->arch.hvm_vcpu.asid_generation = data->core_asid_generation;
 
-    /*
-     * When we assign ASID 1, flush all TLB entries as we are starting a new
-     * generation, and all old ASID allocations are now stale. 
-     */
-    return (v->arch.hvm_vcpu.asid == 1);
+    return need_flush;
 
  disabled:
     v->arch.hvm_vcpu.asid = 0;
xen/arch/x86/hvm/nestedhvm.c +3 -0
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/nestedhvm.c
@@ -61,6 +61,9 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = VMCX_EADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    nv->nv_new_vasid = 0;
+    nv->nv_n1asid = 0;
+    nv->nv_n2asid = 0;
 
     if ( hvm_funcs.nhvm_vcpu_reset )
         hvm_funcs.nhvm_vcpu_reset(v);
xen/arch/x86/hvm/svm/asid.c +8 -1
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/svm/asid.c
@@ -22,6 +22,7 @@
 #include <xen/perfc.h>
 #include <asm/hvm/svm/asid.h>
 #include <asm/amd.h>
+#include <asm/hvm/nestedhvm.h>
 
 void svm_asid_init(struct cpuinfo_x86 *c)
 {

@@ -42,7 +43,13 @@ asmlinkage void svm_asid_handle_vmrun(vo
 {
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
-    bool_t need_flush = hvm_asid_handle_vmenter();
+    bool_t need_flush;
+    bool_t vcpu_guestmode = 0;
+
+    if ( nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr) )
+        vcpu_guestmode = 1;
+
+    need_flush = hvm_asid_handle_vmenter(vcpu_guestmode);
 
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( curr->arch.hvm_vcpu.asid == 0 )
xen/arch/x86/hvm/svm/nestedsvm.c +3 -7
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/svm/nestedsvm.c
@@ -261,8 +261,6 @@ int nsvm_vcpu_hostrestore(struct vcpu *v
     /* Cleanbits */
     n1vmcb->cleanbits.bytes = 0;
 
-    hvm_asid_flush_vcpu(v);
-
     return 0;
 }
 

@@ -408,9 +406,7 @@ static int nsvm_vmcb_prepare4vmrun(struc
     if (rc)
         return rc;
 
-    /* ASID */
-    hvm_asid_flush_vcpu(v);
-    /* n2vmcb->_guest_asid = ns_vmcb->_guest_asid; */
+    /* ASID - Emulation handled in hvm_asid_handle_vmenter() */
 
     /* TLB control */
     n2vmcb->tlb_control = n1vmcb->tlb_control | ns_vmcb->tlb_control;

@@ -605,8 +601,8 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct
     svm->ns_vmcb_guestcr3 = ns_vmcb->_cr3;
     svm->ns_vmcb_hostcr3 = ns_vmcb->_h_cr3;
 
-    nv->nv_flushp2m = (ns_vmcb->tlb_control
-        || (svm->ns_guest_asid != ns_vmcb->_guest_asid));
+    nv->nv_new_vasid = (svm->ns_guest_asid != ns_vmcb->_guest_asid);
+    nv->nv_flushp2m = (ns_vmcb->tlb_control || nv->nv_new_vasid);
     svm->ns_guest_asid = ns_vmcb->_guest_asid;
 
     /* nested paging for the guest */
xen/arch/x86/hvm/svm/svm.c +14 -1
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/svm/svm.c
@@ -1580,6 +1580,17 @@ static void svm_vmexit_do_invalidate_cac
     __update_guest_eip(regs, inst_len);
 }
 
+static void svm_invlpga_intercept(struct vcpu *v,
+    unsigned long vaddr, uint32_t asid)
+{
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
+    if (asid == 0)
+       asid = nv->nv_n1asid; /* remap to l1 guest asid */
+    else
+       asid = nv->nv_n2asid; /* remap to l2 guest asid */
+    svm_invlpga(vaddr, asid);
+}
+
 static void svm_invlpg_intercept(unsigned long vaddr)
 {
     struct vcpu *curr = current;

@@ -1892,10 +1903,12 @@ asmlinkage void svm_vmexit_handler(struc
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
     case VMEXIT_INVLPG:
-    case VMEXIT_INVLPGA:
         if ( !handle_mmio() )
             hvm_inject_exception(TRAP_gp_fault, 0, 0);
         break;
+    case VMEXIT_INVLPGA:
+        svm_invlpga_intercept(v, regs->rax, regs->ecx);
+        break;
 
     case VMEXIT_VMMCALL:
         if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
xen/arch/x86/hvm/vmx/vmx.c +1 -1
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/arch/x86/hvm/vmx/vmx.c
@@ -2673,7 +2673,7 @@ asmlinkage void vmx_vmenter_helper(void)
         goto out;
 
     old_asid = curr->arch.hvm_vcpu.asid;
-    need_flush = hvm_asid_handle_vmenter();
+    need_flush = hvm_asid_handle_vmenter(0 /* false */);
     new_asid = curr->arch.hvm_vcpu.asid;
 
     if ( unlikely(new_asid != old_asid) )
xen/include/asm-x86/hvm/asid.h +1 -1
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/include/asm-x86/hvm/asid.h
@@ -35,7 +35,7 @@ void hvm_asid_flush_core(void);
 
 /* Called before entry to guest context. Checks ASID allocation, returns a
  * boolean indicating whether all ASIDs must be flushed. */
-bool_t hvm_asid_handle_vmenter(void);
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest);
 
 #endif /* __ASM_X86_HVM_ASID_H__ */
xen/include/asm-x86/hvm/vcpu.h +5 -0
diff -r 5cd1060fb63e -r cb2f8bc62a7e xen/include/asm-x86/hvm/vcpu.h
@@ -49,6 +49,11 @@ struct nestedvcpu {
     uint64_t nv_n1vmcx_pa; /* host physical address of nv_n1vmcx */
     uint64_t nv_n2vmcx_pa; /* host physical address of nv_n2vmcx */
 
+    /* ASID emulation */
+    bool_t nv_new_vasid;   /* true when l1 guest requests new virtual asid */
+    uint32_t nv_n1asid;    /* hw ASID number used to run l1 guest */
+    uint32_t nv_n2asid;    /* hw ASID number used to run l2 guest */
+
     /* SVM/VMX arch specific */
     union {
         struct nestedsvm nsvm;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 5 lines · Christoph Egger

Implement ASID emulation.
This allows the l1 guest to run the l2 guest using hw ASID.

Signed-off-by Christoph Egger <[email protected]>

First, how much of a win is this compared with what we do currently?

Second, the two-asids-per-vcpu allocation scheme in hvm_asid_handle_vmenter() looks broken. I mean, consider this comment:
/* When asid generation changed last time when we were
* were going to run l1 guest then next_asid <= nv->nv_n2asid. */
I don't see how you can assert this to be true. Arbitrary generations can have passed, and next_asid incremented to an arbitrary value, since the last time you allocated nv_n2asid.

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

-- Keir

author
#18 lore raw
In reply to Christoph Egger

Rebased due to change in previous patch.

Quoted 6 lines · Christoph Egger

Implement ASID emulation.
This allows the l1 guest to run the l2 guest using hw ASID.

Signed-off-by Christoph Egger<[email protected]>
Signature · 6 lines
---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/arch/x86/hvm/asid.c +64 -18
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/asid.c
@@ -102,10 +102,14 @@ void hvm_asid_flush_core(void)
     data->disabled = 1;
 }
 
-bool_t hvm_asid_handle_vmenter(void)
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest)
 {
+    int need_flush = 0;
     struct vcpu *curr = current;
     struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
+    struct nestedvcpu *nv;
+
+    nv = &vcpu_nestedhvm(v);
 
     /* On erratum #170 systems we must flush the TLB. 
      * Generation overruns are taken here, too. */

@@ -113,27 +117,69 @@ bool_t hvm_asid_handle_vmenter(void)
         goto disabled;
 
     /* Test if VCPU has valid ASID. */
-    if ( curr->arch.hvm_vcpu.asid_generation == data->core_asid_generation )
-        return 0;
-
-    /* If there are no free ASIDs, need to go to a new generation */
-    if ( unlikely(data->next_asid > data->max_asid) )
-    {
-        hvm_asid_flush_core();
-        data->next_asid = 1;
-        if ( data->disabled )
-            goto disabled;
+    if ( curr->arch.hvm_vcpu.asid_generation == data->core_asid_generation ) {
+        if ( run_n2guest ) {
+            if ( !nv->nv_new_vasid && data->next_asid > nv->nv_n2asid ) {
+                /* l1 guest doesn't request a new asid */
+                /* When asid generation changed last time when we were
+                 * were going to run l1 guest then
+                 * next_asid <= nv->nv_n2asid.
+                 */
+                ASSERT(nv->nv_n2asid != 0);
+                ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+                curr->arch.hvm_vcpu.asid = nv->nv_n2asid;
+                return 0;
+            }
+        } else if ( data->next_asid > nv->nv_n1asid ) {
+            /* When asid generation changed last time when we were going to
+             * run l2 guest then next_asid <= nv->nv_n1asid.
+             */
+            ASSERT(nv->nv_n1asid != 0);
+            ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+            curr->arch.hvm_vcpu.asid = nv->nv_n1asid;
+            return 0;
+        }
     }
 
-    /* Now guaranteed to be a free ASID. */
-    curr->arch.hvm_vcpu.asid = data->next_asid++;
+    do {
+        /* If there are no free ASIDs, need to go to a new generation */
+        if ( unlikely(data->next_asid > data->max_asid) )
+        {
+            hvm_asid_flush_core();
+            data->next_asid = 1;
+            if ( data->disabled )
+                goto disabled;
+        }
+
+        if ( data->next_asid == 1 ) {
+            /* We start a new generation, so all old ASID allocations are
+             * stale now. Ensure we flush the TLB also in case of another
+             * iteration.
+             */
+            need_flush = 1;
+        }
+
+        /* Now guaranteed to be a free ASID. */
+        if ( run_n2guest )
+            /* nv_n1asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n2asid = data->next_asid++;
+        else
+            /* nv_n2asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n1asid = data->next_asid++;
+
+        /* Make sure an asid isn't used twice */
+    } while (nv->nv_n2asid == nv->nv_n1asid);
+
+    ASSERT(nv->nv_n1asid != 0);
+
+    curr->arch.hvm_vcpu.asid = (run_n2guest) ? nv->nv_n2asid : nv->nv_n1asid;
     curr->arch.hvm_vcpu.asid_generation = data->core_asid_generation;
 
-    /*
-     * When we assign ASID 1, flush all TLB entries as we are starting a new
-     * generation, and all old ASID allocations are now stale. 
-     */
-    return (curr->arch.hvm_vcpu.asid == 1);
+    return need_flush;
 
  disabled:
     curr->arch.hvm_vcpu.asid = 0;
xen/arch/x86/hvm/nestedhvm.c +3 -0
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/nestedhvm.c
@@ -61,6 +61,9 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = VMCX_EADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    nv->nv_new_vasid = 0;
+    nv->nv_n1asid = 0;
+    nv->nv_n2asid = 0;
 
     if ( hvm_funcs.nhvm_vcpu_reset )
         hvm_funcs.nhvm_vcpu_reset(v);
xen/arch/x86/hvm/svm/asid.c +8 -1
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/svm/asid.c
@@ -22,6 +22,7 @@
 #include <xen/perfc.h>
 #include <asm/hvm/svm/asid.h>
 #include <asm/amd.h>
+#include <asm/hvm/nestedhvm.h>
 
 void svm_asid_init(struct cpuinfo_x86 *c)
 {

@@ -42,7 +43,13 @@ asmlinkage void svm_asid_handle_vmrun(vo
 {
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
-    bool_t need_flush = hvm_asid_handle_vmenter();
+    bool_t need_flush;
+    bool_t vcpu_guestmode = 0;
+
+    if ( nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr) )
+        vcpu_guestmode = 1;
+
+    need_flush = hvm_asid_handle_vmenter(vcpu_guestmode);
 
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( curr->arch.hvm_vcpu.asid == 0 )
xen/arch/x86/hvm/svm/nestedsvm.c +3 -7
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/svm/nestedsvm.c
@@ -261,8 +261,6 @@ int nsvm_vcpu_hostrestore(struct vcpu *v
     /* Cleanbits */
     n1vmcb->cleanbits.bytes = 0;
 
-    hvm_asid_flush_vcpu(v);
-
     return 0;
 }
 

@@ -408,9 +406,7 @@ static int nsvm_vmcb_prepare4vmrun(struc
     if (rc)
         return rc;
 
-    /* ASID */
-    hvm_asid_flush_vcpu(v);
-    /* n2vmcb->_guest_asid = ns_vmcb->_guest_asid; */
+    /* ASID - Emulation handled in hvm_asid_handle_vmenter() */
 
     /* TLB control */
     n2vmcb->tlb_control = n1vmcb->tlb_control | ns_vmcb->tlb_control;

@@ -605,8 +601,8 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct
     svm->ns_vmcb_guestcr3 = ns_vmcb->_cr3;
     svm->ns_vmcb_hostcr3 = ns_vmcb->_h_cr3;
 
-    nv->nv_flushp2m = (ns_vmcb->tlb_control
-        || (svm->ns_guest_asid != ns_vmcb->_guest_asid));
+    nv->nv_new_vasid = (svm->ns_guest_asid != ns_vmcb->_guest_asid);
+    nv->nv_flushp2m = (ns_vmcb->tlb_control || nv->nv_new_vasid);
     svm->ns_guest_asid = ns_vmcb->_guest_asid;
 
     /* nested paging for the guest */
xen/arch/x86/hvm/svm/svm.c +14 -1
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/svm/svm.c
@@ -1580,6 +1580,17 @@ static void svm_vmexit_do_invalidate_cac
     __update_guest_eip(regs, inst_len);
 }
 
+static void svm_invlpga_intercept(struct vcpu *v,
+    unsigned long vaddr, uint32_t asid)
+{
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
+    if (asid == 0)
+       asid = nv->nv_n1asid; /* remap to l1 guest asid */
+    else
+       asid = nv->nv_n2asid; /* remap to l2 guest asid */
+    svm_invlpga(vaddr, asid);
+}
+
 static void svm_invlpg_intercept(unsigned long vaddr)
 {
     struct vcpu *curr = current;

@@ -1892,10 +1903,12 @@ asmlinkage void svm_vmexit_handler(struc
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
     case VMEXIT_INVLPG:
-    case VMEXIT_INVLPGA:
         if ( !handle_mmio() )
             hvm_inject_exception(TRAP_gp_fault, 0, 0);
         break;
+    case VMEXIT_INVLPGA:
+        svm_invlpga_intercept(v, regs->rax, regs->ecx);
+        break;
 
     case VMEXIT_VMMCALL:
         if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
xen/arch/x86/hvm/vmx/vmx.c +1 -1
diff -r 5458a9862db2 -r c217f85d7d0a xen/arch/x86/hvm/vmx/vmx.c
@@ -2673,7 +2673,7 @@ asmlinkage void vmx_vmenter_helper(void)
         goto out;
 
     old_asid = curr->arch.hvm_vcpu.asid;
-    need_flush = hvm_asid_handle_vmenter();
+    need_flush = hvm_asid_handle_vmenter(0 /* false */);
     new_asid = curr->arch.hvm_vcpu.asid;
 
     if ( unlikely(new_asid != old_asid) )
xen/include/asm-x86/hvm/asid.h +1 -1
diff -r 5458a9862db2 -r c217f85d7d0a xen/include/asm-x86/hvm/asid.h
@@ -35,7 +35,7 @@ void hvm_asid_flush_core(void);
 
 /* Called before entry to guest context. Checks ASID allocation, returns a
  * boolean indicating whether all ASIDs must be flushed. */
-bool_t hvm_asid_handle_vmenter(void);
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest);
 
 #endif /* __ASM_X86_HVM_ASID_H__ */
xen/include/asm-x86/hvm/vcpu.h +5 -0
diff -r 5458a9862db2 -r c217f85d7d0a xen/include/asm-x86/hvm/vcpu.h
@@ -49,6 +49,11 @@ struct nestedvcpu {
     uint64_t nv_n1vmcx_pa; /* host physical address of nv_n1vmcx */
     uint64_t nv_n2vmcx_pa; /* host physical address of nv_n2vmcx */
 
+    /* ASID emulation */
+    bool_t nv_new_vasid;   /* true when l1 guest requests new virtual asid */
+    uint32_t nv_n1asid;    /* hw ASID number used to run l1 guest */
+    uint32_t nv_n2asid;    /* hw ASID number used to run l2 guest */
+
     /* SVM/VMX arch specific */
     union {
         struct nestedsvm nsvm;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#2 lore raw
In reply to Keir Fraser
Quoted 9 lines · Keir Fraser
Quoted 5 lines · Christoph Egger

Implement ASID emulation.
This allows the l1 guest to run the l2 guest using hw ASID.

Signed-off-by Christoph Egger<[email protected]>

First, how much of a win is this compared with what we do currently?

We talk about a win of about 1000 cycles per VMRUN and another 1000 cycles per VMEXIT emulation.

That's a speedup of about 10% for each VMRUN and about 20% for each VMEXIT emulation.

Quoted 4 lines

Second, the two-asids-per-vcpu allocation scheme in hvm_asid_handle_vmenter() looks broken. I mean, consider this comment:
/* When asid generation changed last time when we were
* were going to run l1 guest then next_asid<= nv->nv_n2asid. */

Oh, the 2nd 'were' should be removed.

Quoted 3 lines

I don't see how you can assert this to be true. Arbitrary generations can have passed, and next_asid incremented to an arbitrary value, since the last time you allocated nv_n2asid.

There are different cases to handle:

1. nestedhvm is disabled.

In this case, 'run_n2guest' is always false then the function should have the old behaviour across multiple VMRUNs.

2. nestedhvm is enabled and we are going to run l1 guest

We run the l2 guest in the last call. The asid generation may have changed by then. In this case the current nv_n1asid number is stale and the value of data->next_asid is <= of nv->nv_n1asid. The the value of nv->nv_n1asid is valid data->next_asid is larger than nv->nv_n1asid. In this case just reuse the same hw ASID that has been used from the last VMEXIT emulation.

3. nestedhvm is enabled and we are going to run l1 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example. But the hw ASID number may never match the hw ASID used to run the l2 guest.

4. nestedhvm is enabled and we are going to run l2 guest

We run the l1 guest in the last call. The asid generation may have changed by then. In this case the current nv_n2asid number is stale and the value of data->next_asid is <= of nv->nv_n2asid. The the value of nv->nv_n2asid is valid if l1 guest doesn't change the virtual asid (= asid number in the virtual vmcb) and data->next_asid is larger than nv->nv_n2asid. In this case just reuse the same hw ASID that has been used from the last VMRUN emulation.

5. nestedhvm is enabled and we are going to run l2 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example. But the hw ASID number may never match the hw ASID used to run the l1 guest.

In all cases we have to verify that the

Quoted 3 lines

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

-- Keir

Signature · 6 lines
---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
author
#19 lore raw
In reply to Christoph Egger
Quoted 1 line · Christoph Egger

Rebased due to change in previous patch.

Fixed build fallout from rebasing.

Quoted 6 lines · Christoph Egger
Quoted 5 lines

Implement ASID emulation.
This allows the l1 guest to run the l2 guest using hw ASID.

Signed-off-by Christoph Egger<[email protected]>
Signature · 6 lines
---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

# HG changeset patch
# User cegger
# Date 1302705903 -7200
Implement ASID implementation.
This allows the guest to run nested guest using hw ASID.

Signed-off-by Christoph Egger <[email protected]>
xen/arch/x86/hvm/asid.c +64 -18
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/asid.c
@@ -102,10 +102,14 @@ void hvm_asid_flush_core(void)
     data->disabled = 1;
 }
 
-bool_t hvm_asid_handle_vmenter(void)
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest)
 {
+    int need_flush = 0;
     struct vcpu *curr = current;
     struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
+    struct nestedvcpu *nv;
+
+    nv = &vcpu_nestedhvm(curr);
 
     /* On erratum #170 systems we must flush the TLB. 
      * Generation overruns are taken here, too. */

@@ -113,27 +117,69 @@ bool_t hvm_asid_handle_vmenter(void)
         goto disabled;
 
     /* Test if VCPU has valid ASID. */
-    if ( curr->arch.hvm_vcpu.asid_generation == data->core_asid_generation )
-        return 0;
-
-    /* If there are no free ASIDs, need to go to a new generation */
-    if ( unlikely(data->next_asid > data->max_asid) )
-    {
-        hvm_asid_flush_core();
-        data->next_asid = 1;
-        if ( data->disabled )
-            goto disabled;
+    if ( curr->arch.hvm_vcpu.asid_generation == data->core_asid_generation ) {
+        if ( run_n2guest ) {
+            if ( !nv->nv_new_vasid && data->next_asid > nv->nv_n2asid ) {
+                /* l1 guest doesn't request a new asid */
+                /* When asid generation changed last time when we were
+                 * were going to run l1 guest then
+                 * next_asid <= nv->nv_n2asid.
+                 */
+                ASSERT(nv->nv_n2asid != 0);
+                ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+                curr->arch.hvm_vcpu.asid = nv->nv_n2asid;
+                return 0;
+            }
+        } else if ( data->next_asid > nv->nv_n1asid ) {
+            /* When asid generation changed last time when we were going to
+             * run l2 guest then next_asid <= nv->nv_n1asid.
+             */
+            ASSERT(nv->nv_n1asid != 0);
+            ASSERT(nv->nv_n1asid != nv->nv_n2asid);
+            curr->arch.hvm_vcpu.asid = nv->nv_n1asid;
+            return 0;
+        }
     }
 
-    /* Now guaranteed to be a free ASID. */
-    curr->arch.hvm_vcpu.asid = data->next_asid++;
+    do {
+        /* If there are no free ASIDs, need to go to a new generation */
+        if ( unlikely(data->next_asid > data->max_asid) )
+        {
+            hvm_asid_flush_core();
+            data->next_asid = 1;
+            if ( data->disabled )
+                goto disabled;
+        }
+
+        if ( data->next_asid == 1 ) {
+            /* We start a new generation, so all old ASID allocations are
+             * stale now. Ensure we flush the TLB also in case of another
+             * iteration.
+             */
+            need_flush = 1;
+        }
+
+        /* Now guaranteed to be a free ASID. */
+        if ( run_n2guest )
+            /* nv_n1asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n2asid = data->next_asid++;
+        else
+            /* nv_n2asid might have an asid from an old generation.
+             * We handle this on next vmenter.
+             */
+            nv->nv_n1asid = data->next_asid++;
+
+        /* Make sure an asid isn't used twice */
+    } while (nv->nv_n2asid == nv->nv_n1asid);
+
+    ASSERT(nv->nv_n1asid != 0);
+
+    curr->arch.hvm_vcpu.asid = (run_n2guest) ? nv->nv_n2asid : nv->nv_n1asid;
     curr->arch.hvm_vcpu.asid_generation = data->core_asid_generation;
 
-    /*
-     * When we assign ASID 1, flush all TLB entries as we are starting a new
-     * generation, and all old ASID allocations are now stale. 
-     */
-    return (curr->arch.hvm_vcpu.asid == 1);
+    return need_flush;
 
  disabled:
     curr->arch.hvm_vcpu.asid = 0;
xen/arch/x86/hvm/nestedhvm.c +3 -0
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/nestedhvm.c
@@ -61,6 +61,9 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = VMCX_EADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    nv->nv_new_vasid = 0;
+    nv->nv_n1asid = 0;
+    nv->nv_n2asid = 0;
 
     if ( hvm_funcs.nhvm_vcpu_reset )
         hvm_funcs.nhvm_vcpu_reset(v);
xen/arch/x86/hvm/svm/asid.c +8 -1
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/svm/asid.c
@@ -22,6 +22,7 @@
 #include <xen/perfc.h>
 #include <asm/hvm/svm/asid.h>
 #include <asm/amd.h>
+#include <asm/hvm/nestedhvm.h>
 
 void svm_asid_init(struct cpuinfo_x86 *c)
 {

@@ -42,7 +43,13 @@ asmlinkage void svm_asid_handle_vmrun(vo
 {
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
-    bool_t need_flush = hvm_asid_handle_vmenter();
+    bool_t need_flush;
+    bool_t vcpu_guestmode = 0;
+
+    if ( nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr) )
+        vcpu_guestmode = 1;
+
+    need_flush = hvm_asid_handle_vmenter(vcpu_guestmode);
 
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( curr->arch.hvm_vcpu.asid == 0 )
xen/arch/x86/hvm/svm/nestedsvm.c +3 -7
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/svm/nestedsvm.c
@@ -261,8 +261,6 @@ int nsvm_vcpu_hostrestore(struct vcpu *v
     /* Cleanbits */
     n1vmcb->cleanbits.bytes = 0;
 
-    hvm_asid_flush_vcpu(v);
-
     return 0;
 }
 

@@ -408,9 +406,7 @@ static int nsvm_vmcb_prepare4vmrun(struc
     if (rc)
         return rc;
 
-    /* ASID */
-    hvm_asid_flush_vcpu(v);
-    /* n2vmcb->_guest_asid = ns_vmcb->_guest_asid; */
+    /* ASID - Emulation handled in hvm_asid_handle_vmenter() */
 
     /* TLB control */
     n2vmcb->tlb_control = n1vmcb->tlb_control | ns_vmcb->tlb_control;

@@ -605,8 +601,8 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct
     svm->ns_vmcb_guestcr3 = ns_vmcb->_cr3;
     svm->ns_vmcb_hostcr3 = ns_vmcb->_h_cr3;
 
-    nv->nv_flushp2m = (ns_vmcb->tlb_control
-        || (svm->ns_guest_asid != ns_vmcb->_guest_asid));
+    nv->nv_new_vasid = (svm->ns_guest_asid != ns_vmcb->_guest_asid);
+    nv->nv_flushp2m = (ns_vmcb->tlb_control || nv->nv_new_vasid);
     svm->ns_guest_asid = ns_vmcb->_guest_asid;
 
     /* nested paging for the guest */
xen/arch/x86/hvm/svm/svm.c +14 -1
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/svm/svm.c
@@ -1580,6 +1580,17 @@ static void svm_vmexit_do_invalidate_cac
     __update_guest_eip(regs, inst_len);
 }
 
+static void svm_invlpga_intercept(struct vcpu *v,
+    unsigned long vaddr, uint32_t asid)
+{
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
+    if (asid == 0)
+       asid = nv->nv_n1asid; /* remap to l1 guest asid */
+    else
+       asid = nv->nv_n2asid; /* remap to l2 guest asid */
+    svm_invlpga(vaddr, asid);
+}
+
 static void svm_invlpg_intercept(unsigned long vaddr)
 {
     struct vcpu *curr = current;

@@ -1892,10 +1903,12 @@ asmlinkage void svm_vmexit_handler(struc
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
     case VMEXIT_INVLPG:
-    case VMEXIT_INVLPGA:
         if ( !handle_mmio() )
             hvm_inject_exception(TRAP_gp_fault, 0, 0);
         break;
+    case VMEXIT_INVLPGA:
+        svm_invlpga_intercept(v, regs->rax, regs->ecx);
+        break;
 
     case VMEXIT_VMMCALL:
         if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
xen/arch/x86/hvm/vmx/vmx.c +1 -1
diff -r e1895b172170 -r 07ce1a6ecad4 xen/arch/x86/hvm/vmx/vmx.c
@@ -2673,7 +2673,7 @@ asmlinkage void vmx_vmenter_helper(void)
         goto out;
 
     old_asid = curr->arch.hvm_vcpu.asid;
-    need_flush = hvm_asid_handle_vmenter();
+    need_flush = hvm_asid_handle_vmenter(0 /* false */);
     new_asid = curr->arch.hvm_vcpu.asid;
 
     if ( unlikely(new_asid != old_asid) )
xen/include/asm-x86/hvm/asid.h +1 -1
diff -r e1895b172170 -r 07ce1a6ecad4 xen/include/asm-x86/hvm/asid.h
@@ -35,7 +35,7 @@ void hvm_asid_flush_core(void);
 
 /* Called before entry to guest context. Checks ASID allocation, returns a
  * boolean indicating whether all ASIDs must be flushed. */
-bool_t hvm_asid_handle_vmenter(void);
+bool_t hvm_asid_handle_vmenter(bool_t run_n2guest);
 
 #endif /* __ASM_X86_HVM_ASID_H__ */
xen/include/asm-x86/hvm/vcpu.h +5 -0
diff -r e1895b172170 -r 07ce1a6ecad4 xen/include/asm-x86/hvm/vcpu.h
@@ -49,6 +49,11 @@ struct nestedvcpu {
     uint64_t nv_n1vmcx_pa; /* host physical address of nv_n1vmcx */
     uint64_t nv_n2vmcx_pa; /* host physical address of nv_n2vmcx */
 
+    /* ASID emulation */
+    bool_t nv_new_vasid;   /* true when l1 guest requests new virtual asid */
+    uint32_t nv_n1asid;    /* hw ASID number used to run l1 guest */
+    uint32_t nv_n2asid;    /* hw ASID number used to run l2 guest */
+
     /* SVM/VMX arch specific */
     union {
         struct nestedsvm nsvm;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw
In reply to Christoph Egger
Quoted 8 lines · Christoph Egger
Quoted 2 lines · Keir Fraser

On 13/04/2011 11:37, "Christoph Egger"<[email protected]> wrote:

We talk about a win of about 1000 cycles per VMRUN and another 1000 cycles per VMEXIT emulation.

That's a speedup of about 10% for each VMRUN and about 20% for each VMEXIT emulation.

Is this measurable on a macro benchmark?

I mean this looks like a micro-optimisation on a feature that noone is going to use for serious performance work anyway.

Quoted 5 lines

4. nestedhvm is enabled and we are going to run l2 guest

We run the l1 guest in the last call. The asid generation may have changed by then. In this case the current nv_n2asid number is stale and the value of data->next_asid is <= of nv->nv_n2asid.

How do you know for sure that next_asid will be <= nv_n2asid in this case? What if other VCPUs have run meanwhile, and next_asid has been incremented multiple times until it is greather than nv_n2asid?

-- Keir

Quoted 20 lines

The the value of nv->nv_n2asid is valid if l1 guest doesn't change the virtual asid (= asid number in the virtual vmcb) and data->next_asid is larger than nv->nv_n2asid. In this case just reuse the same hw ASID that has been used from the last VMRUN emulation.

5. nestedhvm is enabled and we are going to run l2 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example.
But the hw ASID number may never match the hw ASID used to run the l1 guest.

In all cases we have to verify that the

Quoted 3 lines

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

-- Keir

author
#4 lore raw
Quoted 5 lines · Keir Fraser
Quoted 3 lines · Christoph Egger
Quoted 2 lines · Keir Fraser

On 13/04/2011 11:37, "Christoph Egger"<[email protected]> wrote:

Quoted 7 lines
Quoted 5 lines

We talk about a win of about 1000 cycles per VMRUN and another 1000 cycles per VMEXIT emulation.

That's a speedup of about 10% for each VMRUN and about 20% for each VMEXIT emulation.

Is this measurable on a macro benchmark?

I measured this with xentrace while l2 guest is booting.

The speedup is noticable on the end-user side just by the feeling on how fast the l2 guest reacts on user input.

Quoted 5 lines

I mean this looks like a micro-optimisation on a feature that noone is going to use for serious performance work anyway.

Quoted 2 lines

4. nestedhvm is enabled and we are going to run l2 guest

Quoted 7 lines
Quoted 3 lines

We run the l1 guest in the last call. The asid generation may have changed by then. In this case the current nv_n2asid number is stale and the value of data->next_asid is<= of nv->nv_n2asid.

How do you know for sure that next_asid will be<= nv_n2asid in this case? What if other VCPUs have run meanwhile, and next_asid has been incremented multiple times until it is greather than nv_n2asid?

In that case curr->arch.hvm_vcpu.asid_generation is not equal to data->core_asid_generation and a new hw ASID number is assigned regardless of the values of data->next_asid and nv_n2asid.

Christoph

Quoted 26 lines

-- Keir

Quoted 20 lines

The the value of nv->nv_n2asid is valid if l1 guest doesn't change the virtual asid (= asid number in the virtual vmcb) and data->next_asid is larger than nv->nv_n2asid. In this case just reuse the same hw ASID that has been used from the last VMRUN emulation.

5. nestedhvm is enabled and we are going to run l2 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example.
But the hw ASID number may never match the hw ASID used to run the l1 guest.

In all cases we have to verify that the

Quoted 3 lines

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

   -- Keir
Signature · 6 lines
---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
#5 lore raw
Quoted 9 lines · Christoph Egger
Quoted 3 lines · Keir Fraser

On 13/04/2011 15:26, "Christoph Egger"<[email protected]> wrote:

Is this measurable on a macro benchmark?

I measured this with xentrace while l2 guest is booting.

The speedup is noticable on the end-user side just by the feeling on how fast the l2 guest reacts on user input.

Yikes. Does nestedhvm suck really badly then? I can't believe this patch alone gets you from sucky to good performance. Is it an improvement from sucky to not-quite-as-sucky?

Quoted 9 lines
Quoted 9 lines

I mean this looks like a micro-optimisation on a feature that noone is going to use for serious performance work anyway.

Quoted 5 lines

4. nestedhvm is enabled and we are going to run l2 guest

We run the l1 guest in the last call. The asid generation may have changed by then. In this case the current nv_n2asid number is stale and the value of data->next_asid is<= of nv->nv_n2asid.

Quoted 7 lines
Quoted 3 lines

How do you know for sure that next_asid will be<= nv_n2asid in this case? What if other VCPUs have run meanwhile, and next_asid has been incremented multiple times until it is greather than nv_n2asid?

In that case curr->arch.hvm_vcpu.asid_generation is not equal to data->core_asid_generation and a new hw ASID number is assigned regardless of the values of data->next_asid and nv_n2asid.

What if nv_n2asid wast last assigned on a previous generation, but nv_n1asid was assigned from the current generation? Then you'd have next_asid > nv_n2asid, but nv_n2asid is stale.

-- Keir

Quoted 29 lines

Christoph

Quoted 26 lines

-- Keir

Quoted 20 lines

The the value of nv->nv_n2asid is valid if l1 guest doesn't change the virtual asid (= asid number in the virtual vmcb) and data->next_asid is larger than nv->nv_n2asid. In this case just reuse the same hw ASID that has been used from the last VMRUN emulation.

5. nestedhvm is enabled and we are going to run l2 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example.
But the hw ASID number may never match the hw ASID used to run the l1 guest.

In all cases we have to verify that the

Quoted 3 lines

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

   -- Keir
author
#6 lore raw
Quoted 7 lines · Keir Fraser
Quoted 5 lines · Christoph Egger
Quoted 3 lines · Keir Fraser

On 13/04/2011 15:26, "Christoph Egger"<[email protected]> wrote:

Is this measurable on a macro benchmark?

Quoted 8 lines
Quoted 4 lines

I measured this with xentrace while l2 guest is booting.

The speedup is noticable on the end-user side just by the feeling on how fast the l2 guest reacts on user input.

Yikes. Does nestedhvm suck really badly then? I can't believe this patch alone gets you from sucky to good performance. Is it an improvement from sucky to not-quite-as-sucky?

The very first patch series I sent to xen-devel, the cost of a VMRUN emulation was about 25000 cycles with Xen-on-Xen while hvmloader was coming up as l2 guest. With every new patch series there were also performance optimizations done. Now the cost of a VMRUN emulation is about 11000 cycles and with this ASID emulation patch the cost of a VMRUN emulation goes down to about 10000 cycles.

There is still room for more performance, I am working on.

A cost of 15000 cycles for VMRUN emulation is fast enough that Solitair in XP mode of Windows 7 guest recognizes a double click ;-)

Solitair in XP mode runs faster than Solitair in Windows 7 ;-)

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines

I mean this looks like a micro-optimisation on a feature that noone is going to use for serious performance work anyway.

Quoted 5 lines

4. nestedhvm is enabled and we are going to run l2 guest

We run the l1 guest in the last call. The asid generation may have changed by then. In this case the current nv_n2asid number is stale and the value of data->next_asid is<= of nv->nv_n2asid.

How do you know for sure that next_asid will be<= nv_n2asid in this case? What if other VCPUs have run meanwhile, and next_asid has been incremented multiple times until it is greather than nv_n2asid?

Quoted 7 lines
Quoted 3 lines

In that case curr->arch.hvm_vcpu.asid_generation is not equal to data->core_asid_generation and a new hw ASID number is assigned regardless of the values of data->next_asid and nv_n2asid.

What if nv_n2asid wast last assigned on a previous generation, but nv_n1asid was assigned from the current generation? Then you'd have next_asid> nv_n2asid, but nv_n2asid is stale.

When the generation changes, next_asid is set back to 1 and the hw TLB flushed.

If next_asid is larger than nv_n2asid then it is reused and this is ok since the hw TLB got flushed. It is just important that nv_n1asid never gets the same hw ASID assigned and that is what the do { } while loop ensures.

The hardware doesn't enforce an incremental use of the hw ASID numbers.

The point of ASID emulation is to reduce the number of TLB invalidations by switching forth and back between two hw ASID numbers when switching between l1 and l2 guest. This reduces pagetable walks of the hw MMU so much that the performance win is noticable by the end-user.

Without this patch the ASID is invalidated on every VMRUN and VMEXIT emulation.

The Flush-by-ASID feature even let me reuse the same hw ASID number when the l2 guest wants to flush its TLB.

Christoph

Quoted 24 lines

-- Keir

Quoted 22 lines

Christoph

Quoted 20 lines
   -- Keir
Quoted 17 lines

The the value of nv->nv_n2asid is valid if l1 guest doesn't change the virtual asid (= asid number in the virtual vmcb) and data->next_asid is larger than nv->nv_n2asid. In this case just reuse the same hw ASID that has been used from the last VMRUN emulation.

5. nestedhvm is enabled and we are going to run l2 guest again

The same hw ASID should be reused unless the generation changed because the nestedp2m got flushed or the vcpu moved to a different physical cpu, for example.
But the hw ASID number may never match the hw ASID used to run the l1 guest.

In all cases we have to verify that the

Quoted 1 line

I wouldn't bother fixing #2 unless there's a convincing answer for #1.

Signature · 6 lines
---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
#7 lore raw
Quoted 11 lines · Christoph Egger
Quoted 3 lines

What if nv_n2asid wast last assigned on a previous generation, but nv_n1asid was assigned from the current generation? Then you'd have next_asid> nv_n2asid, but nv_n2asid is stale.

When the generation changes, next_asid is set back to 1 and the hw TLB flushed.

If next_asid is larger than nv_n2asid then it is reused and this is ok since the hw TLB got flushed. It is just important that nv_n1asid never gets the same hw ASID assigned and that is what the do { } while loop ensures.

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

-- Keir

author
#8 lore raw
Quoted 14 lines · Keir Fraser
Quoted 11 lines · Christoph Egger
Quoted 3 lines

What if nv_n2asid wast last assigned on a previous generation, but nv_n1asid was assigned from the current generation? Then you'd have next_asid> nv_n2asid, but nv_n2asid is stale.

When the generation changes, next_asid is set back to 1 and the hw TLB flushed.

If next_asid is larger than nv_n2asid then it is reused and this is ok since the hw TLB got flushed. It is just important that nv_n1asid never gets the same hw ASID assigned and that is what the do { } while loop ensures.

Quoted 4 lines

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

I am not sure if I got you right. You mean what if two vcpus run on one
physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() before so that asid_generation and core_asid_generation do not match and a new asid is always assigned.

Christoph

Signature · 6 lines
---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
#9 lore raw
Quoted 9 lines · Christoph Egger
Quoted 4 lines

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

I am not sure if I got you right. You mean what if two vcpus run on one physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() before so that asid_generation and core_asid_generation do not match and a new asid is always assigned.

No, it only does that if a given VCPU gets scheduled onto a *different* PCPU than last time it ran.

I've attached a mostly rewritten version of your patch that is about half the size and I believe has a fighting chance of being correct (however it is only build tested). Give it a look and a spin.

-- Keir

Quoted 1 line

Christoph

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#10 lore raw
Quoted 12 lines · Keir Fraser
Quoted 9 lines · Christoph Egger
Quoted 4 lines

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

I am not sure if I got you right. You mean what if two vcpus run on one physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() before so that asid_generation and core_asid_generation do not match and a new asid is always assigned.

Quoted 6 lines

No, it only does that if a given VCPU gets scheduled onto a *different* PCPU than last time it ran.

I've attached a mostly rewritten version of your patch that is about half the size and I believe has a fighting chance of being correct (however it is only build tested). Give it a look and a spin.

Yes, it is correct. I like the idea to maintain a generation per asid. Please apply it. Thanks for this work.

Christoph

Signature · 6 lines
---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
#11 lore raw
Quoted 16 lines · Christoph Egger
Quoted 15 lines · Keir Fraser
Quoted 9 lines · Christoph Egger
Quoted 4 lines

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

I am not sure if I got you right. You mean what if two vcpus run on one physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() before so that asid_generation and core_asid_generation do not match and a new asid is always assigned.

No, it only does that if a given VCPU gets scheduled onto a *different* PCPU than last time it ran.

Quoted 6 lines
Quoted 3 lines

I've attached a mostly rewritten version of your patch that is about half the size and I believe has a fighting chance of being correct (however it is only build tested). Give it a look and a spin.

Yes, it is correct. I like the idea to maintain a generation per asid. Please apply it. Thanks for this work.

You didn't notice my subtle error in switching n1 and n2 asid selection in svm_asid_handle_vmrun()? ;-) Actually I bet it would take a lot of testing to pick up on that error, since the transposition doesn't much matter except that we are then switched round relative to which ASID gets flushed on guest-initiated INVLPGA, and also we'd flush the 'wrong' ASID when guest requests a new L2 guest ASID. Anyway, glad I spotted it before I checked the patch in! Stale TLB bugs are no fun.

-- Keir

Quoted 2 lines

Christoph

author
#12 lore raw
Quoted 25 lines · Keir Fraser
Quoted 20 lines · Christoph Egger
Quoted 18 lines · Keir Fraser
Quoted 9 lines · Christoph Egger
Quoted 4 lines

What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW asid in this generation as this vcpu's (now stale, as it's from a previous generation's) nv_n2asid? This PCPU can be interleaving execution of other HVM VCPUs after all.

I am not sure if I got you right. You mean what if two vcpus run on one physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() before so that asid_generation and core_asid_generation do not match and a new asid is always assigned.

No, it only does that if a given VCPU gets scheduled onto a *different* PCPU than last time it ran.

I've attached a mostly rewritten version of your patch that is about half the size and I believe has a fighting chance of being correct (however it is only build tested). Give it a look and a spin.

Quoted 5 lines
Quoted 2 lines

Yes, it is correct. I like the idea to maintain a generation per asid. Please apply it. Thanks for this work.

You didn't notice my subtle error in switching n1 and n2 asid selection in svm_asid_handle_vmrun()? ;-)

Oh, now that you mention it...

A l2 smp guest actually boots up. Maybe I need to use more vcpus than I have physical cpus to hit that bug.

I think, this snippet should be like this:

struct vcpu *curr = current;
struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb; struct hvm_vcpu_asid *curr_asid;
bool_t need_flush;
bool_t vcpu_guestmode = 0;

if (nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr))

    vcpu_guestmode = 1;

curr_asid = vcpu_guestmode ?

    &curr->arch.hvm_vcpu.n2_asid : &curr->arch.hvm_vcpu.n1_asid;

need_flush = hvm_asid_handle_vmenter(curr_asid);

Christoph

Quoted 14 lines

Actually I bet it would take a lot of testing to pick up on that error, since the transposition doesn't much matter except
that we are then switched round relative to which ASID gets flushed on guest-initiated INVLPGA, and also we'd flush the 'wrong' ASID when guest requests a new L2 guest ASID. Anyway, glad I spotted it before I checked the patch in! Stale TLB bugs are no fun.

-- Keir

Quoted 2 lines

Christoph

Signature · 6 lines
---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
#13 lore raw
Quoted 4 lines · Christoph Egger
Quoted 2 lines

You didn't notice my subtle error in switching n1 and n2 asid selection in svm_asid_handle_vmrun()? ;-)

Oh, now that you mention it...

Yeah, I fixed it before I checked it in (xen-unstable:23229).

Quoted 2 lines

A l2 smp guest actually boots up. Maybe I need to use more vcpus than I have physical cpus to hit that bug.

I think it would have been a source of very subtle quite hard-to-repro bugs. Not something you'd pick up in a simple does-it-boot smoke test. So a pretty nasty bug all round!

-- Keir

author
#14 lore raw
Quoted 7 lines · Keir Fraser
Quoted 4 lines · Christoph Egger
Quoted 2 lines

You didn't notice my subtle error in switching n1 and n2 asid selection in svm_asid_handle_vmrun()? ;-)

Oh, now that you mention it...

Quoted 8 lines

Yeah, I fixed it before I checked it in (xen-unstable:23229).

Quoted 2 lines

A l2 smp guest actually boots up. Maybe I need to use more vcpus than I have physical cpus to hit that bug.

I think it would have been a source of very subtle quite hard-to-repro bugs. Not something you'd pick up in a simple does-it-boot smoke test. So a pretty nasty bug all round!

Just found another fallout that got lost from my original patch.

After shutting down XP mode in Windows 7, Win7 turns off SVM in EFER after about 30 seconds. When starting XP mode again, Win 7 turns SVM on again.

Then nv_n2asid can be stale. Attached patch fixes this.

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---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/arch/x86/hvm/nestedhvm.c +1 -0
diff -r 9a0ed35421da -r ad5960696d68 xen/arch/x86/hvm/nestedhvm.c
@@ -61,6 +61,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = VMCX_EADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    memset(&nv->nv_n2asid, 0, sizeof(struct hvm_vcpu_asid));
 
     if ( hvm_funcs.nhvm_vcpu_reset )
         hvm_funcs.nhvm_vcpu_reset(v);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#16 lore raw
In reply to Keir Fraser
Quoted 7 lines · Keir Fraser
Quoted 5 lines · Christoph Egger
Quoted 1 line · Keir Fraser

On 15/04/2011 10:08, "Christoph Egger"<[email protected]> wrote:

Just found another fallout that got lost from my original patch.

Quoted 8 lines
Quoted 5 lines

After shutting down XP mode in Windows 7, Win7 turns off SVM in EFER after about 30 seconds. When starting XP mode again, Win 7 turns SVM on again.

Then nv_n2asid can be stale. Attached patch fixes this.

Should be using hvm_asid_flush_vcpu_asid(), so I'll fix the patch to use that and then check it in.

Yes, right. Thank you.

Christoph

Quoted 8 lines

Thanks,
Keir

Quoted 2 lines
Signed-off-by Christoph Egger<[email protected]>
Signature · 6 lines
---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
#15 lore raw
In reply to Christoph Egger
Quoted 10 lines · Christoph Egger
Quoted 1 line · Keir Fraser

On 15/04/2011 10:08, "Christoph Egger"<[email protected]> wrote:

Just found another fallout that got lost from my original patch.

After shutting down XP mode in Windows 7, Win7 turns off SVM in EFER after about 30 seconds. When starting XP mode again, Win 7 turns SVM on again.

Then nv_n2asid can be stale. Attached patch fixes this.

Should be using hvm_asid_flush_vcpu_asid(), so I'll fix the patch to use that and then check it in.

Thanks,
Keir

Quoted 2 lines
Signed-off-by Christoph Egger <[email protected]>
author
#17 lore raw
Quoted 7 lines · Keir Fraser
Quoted 5 lines · Christoph Egger
Quoted 1 line · Keir Fraser

On 15/04/2011 10:08, "Christoph Egger"<[email protected]> wrote:

Just found another fallout that got lost from my original patch.

Quoted 8 lines
Quoted 5 lines

After shutting down XP mode in Windows 7, Win7 turns off SVM in EFER after about 30 seconds. When starting XP mode again, Win 7 turns SVM on again.

Then nv_n2asid can be stale. Attached patch fixes this.

Should be using hvm_asid_flush_vcpu_asid(), so I'll fix the patch to use that and then check it in.

There are a lot of regressions now... I assume the non-nested virtualization case got broken.

Attached patch covers the non-nested case (and contains the above discussed hvm_asid_flush_vcpu_asid() as it didn't appear yet upstream).

Signed-off-by Christoph Egger<[email protected]>
Signature · 6 lines
---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/arch/x86/hvm/nestedhvm.c +1 -0
diff -r ad5960696d68 xen/arch/x86/hvm/nestedhvm.c
@@ -61,7 +61,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = VMCX_EADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
 
     if ( hvm_funcs.nhvm_vcpu_reset )
         hvm_funcs.nhvm_vcpu_reset(v);
xen/arch/x86/hvm/svm/asid.c +10 -4
diff -r ad5960696d68 xen/arch/x86/hvm/svm/asid.c
@@ -43,10 +43,16 @@ asmlinkage void svm_asid_handle_vmrun(vo
 {
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
-    struct hvm_vcpu_asid *p_asid =
-        nestedhvm_vcpu_in_guestmode(curr)
-        ? &vcpu_nestedhvm(curr).nv_n2asid : &curr->arch.hvm_vcpu.n1asid;
-    bool_t need_flush = hvm_asid_handle_vmenter(p_asid);
+    struct hvm_vcpu_asid *p_asid;
+    bool_t need_flush;
+    bool_t vcpu_guestmode = 0;
+
+    if (nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr))
+        vcpu_guestmode = 1;
+
+    p_asid = vcpu_guestmode ?
+        &vcpu_nestedhvm(curr).nv_n2asid : &curr->arch.hvm_vcpu.n1asid;
+    need_flush = hvm_asid_handle_vmenter(p_asid);
 
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( p_asid->asid == 0 )

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel