[PATCH v2 09/13] x86/svm: Fix VMLOAD/VMSAVE state handling when using nested virt

[chunjie.zhu@xxxxxxxxxx]

From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>

Currently, the SVM VMLOAD/VMSAVE state handling always uses the vCPU's
active VMCB to load/store the state. The active VMCB changes depending
on whether or not the vCPU is in guest mode yet the state is not
properly copied between VMCB(0-1) and VMCB(0-2) nor is the
vmcb_sync_state updated when switching active VMCB.

The most common way this fails is when context switching to a vCPU in
guest mode and immediately taking a VMEXIT (e.g. due to an interrupt for
L1 having arrived in the meantime). The code issues an unconditional
VMSAVE into VMCB(0-1) but the state has not yet been loaded since
context switching which results in garbage in VMCB(0-1). The garbage
state is then (re-)loaded from VMCB(0-1) shortly before VMRUN. This
results in frequent, random crashes in L2.

Since the state covered by VMLOAD/VMSAVE is a property of the vCPU and
is not affected by switches to and from guest mode, always use VMCB(0-1)
to store and retrieve it. This avoids the need to synchronize state
between VMCBs and fixes the random crashes. L1 itself is responsible for
using VMLOAD/VMSAVE to load/save the state from hardware into its own
VMCB(1-2) and this change does not affect that.

Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
---
 xen/arch/x86/hvm/svm/nestedsvm.c |   7 --
 xen/arch/x86/hvm/svm/svm.c       | 107 +++++++++++++++++--------------
 xen/arch/x86/hvm/svm/vmcb.c      |  26 ++++----
 xen/arch/x86/hvm/svm/vmcb.h      |   3 +-
 4 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index d47e57c8a1d7..e1cb10e1de11 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -198,11 +198,6 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct 
cpu_user_regs *regs)
     ASSERT(n1vmcb != NULL);
     ASSERT(n2vmcb != NULL);
 
-    /*
-     * nsvm_vmcb_prepare4vmexit() already saved register values
-     * handled by VMSAVE/VMLOAD into n1vmcb directly.
-     */
-
     /* switch vmcb to l1 guest's vmcb */
     v->arch.hvm.svm.vmcb = n1vmcb;
     v->arch.hvm.svm.vmcb_pa = nv->nv_n1vmcx_pa;
@@ -977,8 +972,6 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct 
cpu_user_regs *regs)
     struct vmcb_struct *ns_vmcb = nv->nv_vvmcx;
     struct vmcb_struct *n2vmcb = nv->nv_n2vmcx;
 
-    svm_vmsave_pa(nv->nv_n1vmcx_pa);
-
     /* Cache guest physical address of virtual vmcb
      * for VMCB Cleanbit emulation.
      */
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ea32dcead218..5aeffd5e416f 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -443,30 +443,30 @@ static int svm_vmcb_restore(struct vcpu *v, struct 
hvm_hw_cpu *c)
 
 static void svm_save_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
 {
-    struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
 
-    data->sysenter_cs      = vmcb->sysenter_cs;
-    data->sysenter_esp     = vmcb->sysenter_esp;
-    data->sysenter_eip     = vmcb->sysenter_eip;
-    data->shadow_gs        = vmcb->kerngsbase;
-    data->msr_lstar        = vmcb->lstar;
-    data->msr_star         = vmcb->star;
-    data->msr_cstar        = vmcb->cstar;
-    data->msr_syscall_mask = vmcb->sfmask;
+    data->sysenter_cs      = n1_vmcb->sysenter_cs;
+    data->sysenter_esp     = n1_vmcb->sysenter_esp;
+    data->sysenter_eip     = n1_vmcb->sysenter_eip;
+    data->shadow_gs        = n1_vmcb->kerngsbase;
+    data->msr_lstar        = n1_vmcb->lstar;
+    data->msr_star         = n1_vmcb->star;
+    data->msr_cstar        = n1_vmcb->cstar;
+    data->msr_syscall_mask = n1_vmcb->sfmask;
 }
 
 static void svm_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
 {
-    struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
 
-    vmcb->lstar        = data->msr_lstar;
-    vmcb->star         = data->msr_star;
-    vmcb->cstar        = data->msr_cstar;
-    vmcb->sfmask       = data->msr_syscall_mask;
-    vmcb->kerngsbase   = data->shadow_gs;
-    vmcb->sysenter_cs  = data->sysenter_cs;
-    vmcb->sysenter_esp = data->sysenter_esp;
-    vmcb->sysenter_eip = data->sysenter_eip;
+    n1_vmcb->lstar        = data->msr_lstar;
+    n1_vmcb->star         = data->msr_star;
+    n1_vmcb->cstar        = data->msr_cstar;
+    n1_vmcb->sfmask       = data->msr_syscall_mask;
+    n1_vmcb->kerngsbase   = data->shadow_gs;
+    n1_vmcb->sysenter_cs  = data->sysenter_cs;
+    n1_vmcb->sysenter_esp = data->sysenter_esp;
+    n1_vmcb->sysenter_eip = data->sysenter_eip;
     v->arch.hvm.guest_efer = data->msr_efer;
     svm_update_guest_efer(v);
 }
@@ -578,18 +578,19 @@ static void cf_check svm_cpuid_policy_changed(struct vcpu 
*v)
 void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
 {
     struct svm_vcpu *svm = &v->arch.hvm.svm;
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
 
     if ( new_state == vmcb_needs_vmsave )
     {
         if ( svm->vmcb_sync_state == vmcb_needs_vmload )
-            svm_vmload_pa(svm->vmcb_pa);
+            svm_vmload_pa(nv->nv_n1vmcx_pa);
 
         svm->vmcb_sync_state = new_state;
     }
     else
     {
         if ( svm->vmcb_sync_state == vmcb_needs_vmsave )
-            svm_vmsave_pa(svm->vmcb_pa);
+            svm_vmsave_pa(nv->nv_n1vmcx_pa);
 
         if ( svm->vmcb_sync_state != vmcb_needs_vmload )
             svm->vmcb_sync_state = new_state;
@@ -605,6 +606,7 @@ static void cf_check svm_get_segment_register(
     struct vcpu *v, enum x86_segment seg, struct segment_register *reg)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
 
     ASSERT((v == current) || !vcpu_runnable(v));
 
@@ -612,8 +614,9 @@ static void cf_check svm_get_segment_register(
     {
     case x86_seg_fs ... x86_seg_gs:
         svm_sync_vmcb(v, vmcb_in_sync);
+        *reg = n1_vmcb->sreg[seg];
+        break;
 
-        /* Fallthrough. */
     case x86_seg_es ... x86_seg_ds:
         *reg = vmcb->sreg[seg];
 
@@ -623,7 +626,7 @@ static void cf_check svm_get_segment_register(
 
     case x86_seg_tss:
         svm_sync_vmcb(v, vmcb_in_sync);
-        *reg = vmcb->tr;
+        *reg = n1_vmcb->tr;
         break;
 
     case x86_seg_gdt:
@@ -636,7 +639,7 @@ static void cf_check svm_get_segment_register(
 
     case x86_seg_ldt:
         svm_sync_vmcb(v, vmcb_in_sync);
-        *reg = vmcb->ldtr;
+        *reg = n1_vmcb->ldtr;
         break;
 
     default:
@@ -651,6 +654,7 @@ static void cf_check svm_set_segment_register(
     struct vcpu *v, enum x86_segment seg, struct segment_register *reg)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
 
     ASSERT((v == current) || !vcpu_runnable(v));
 
@@ -689,12 +693,16 @@ static void cf_check svm_set_segment_register(
 
         /* Fallthrough */
     case x86_seg_es ... x86_seg_cs:
-    case x86_seg_ds ... x86_seg_gs:
+    case x86_seg_ds:
         vmcb->sreg[seg] = *reg;
         break;
 
+    case x86_seg_fs ... x86_seg_gs:
+        n1_vmcb->sreg[seg] = *reg;
+        break;
+
     case x86_seg_tss:
-        vmcb->tr = *reg;
+        n1_vmcb->tr = *reg;
         break;
 
     case x86_seg_gdt:
@@ -708,7 +716,7 @@ static void cf_check svm_set_segment_register(
         break;
 
     case x86_seg_ldt:
-        vmcb->ldtr = *reg;
+        n1_vmcb->ldtr = *reg;
         break;
 
     case x86_seg_sys:
@@ -1667,6 +1675,7 @@ static int cf_check svm_msr_read_intercept(
     struct vcpu *v = current;
     const struct domain *d = v->domain;
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
     const struct nestedsvm *nsvm = &vcpu_nestedsvm(v);
     uint64_t tmp;
 
@@ -1689,43 +1698,43 @@ static int cf_check svm_msr_read_intercept(
     switch ( msr )
     {
     case MSR_IA32_SYSENTER_CS:
-        *msr_content = vmcb->sysenter_cs;
+        *msr_content = n1_vmcb->sysenter_cs;
         break;
 
     case MSR_IA32_SYSENTER_ESP:
-        *msr_content = vmcb->sysenter_esp;
+        *msr_content = n1_vmcb->sysenter_esp;
         break;
 
     case MSR_IA32_SYSENTER_EIP:
-        *msr_content = vmcb->sysenter_eip;
+        *msr_content = n1_vmcb->sysenter_eip;
         break;
 
     case MSR_STAR:
-        *msr_content = vmcb->star;
+        *msr_content = n1_vmcb->star;
         break;
 
     case MSR_LSTAR:
-        *msr_content = vmcb->lstar;
+        *msr_content = n1_vmcb->lstar;
         break;
 
     case MSR_CSTAR:
-        *msr_content = vmcb->cstar;
+        *msr_content = n1_vmcb->cstar;
         break;
 
     case MSR_SYSCALL_MASK:
-        *msr_content = vmcb->sfmask;
+        *msr_content = n1_vmcb->sfmask;
         break;
 
     case MSR_FS_BASE:
-        *msr_content = vmcb->fs.base;
+        *msr_content = n1_vmcb->fs.base;
         break;
 
     case MSR_GS_BASE:
-        *msr_content = vmcb->gs.base;
+        *msr_content = n1_vmcb->gs.base;
         break;
 
     case MSR_SHADOW_GS_BASE:
-        *msr_content = vmcb->kerngsbase;
+        *msr_content = n1_vmcb->kerngsbase;
         break;
 
     case MSR_IA32_MCx_MISC(4): /* Threshold register */
@@ -1858,6 +1867,7 @@ static int cf_check svm_msr_write_intercept(
     struct vcpu *v = current;
     struct domain *d = v->domain;
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
     struct nestedsvm *nsvm = &vcpu_nestedsvm(v);
 
     switch ( msr )
@@ -1891,45 +1901,45 @@ static int cf_check svm_msr_write_intercept(
         switch ( msr )
         {
         case MSR_IA32_SYSENTER_ESP:
-            vmcb->sysenter_esp = msr_content;
+            n1_vmcb->sysenter_esp = msr_content;
             break;
 
         case MSR_IA32_SYSENTER_EIP:
-            vmcb->sysenter_eip = msr_content;
+            n1_vmcb->sysenter_eip = msr_content;
             break;
 
         case MSR_LSTAR:
-            vmcb->lstar = msr_content;
+            n1_vmcb->lstar = msr_content;
             break;
 
         case MSR_CSTAR:
-            vmcb->cstar = msr_content;
+            n1_vmcb->cstar = msr_content;
             break;
 
         case MSR_FS_BASE:
-            vmcb->fs.base = msr_content;
+            n1_vmcb->fs.base = msr_content;
             break;
 
         case MSR_GS_BASE:
-            vmcb->gs.base = msr_content;
+            n1_vmcb->gs.base = msr_content;
             break;
 
         case MSR_SHADOW_GS_BASE:
-            vmcb->kerngsbase = msr_content;
+            n1_vmcb->kerngsbase = msr_content;
             break;
         }
         break;
 
     case MSR_IA32_SYSENTER_CS:
-        vmcb->sysenter_cs = msr_content;
+        n1_vmcb->sysenter_cs = msr_content;
         break;
 
     case MSR_STAR:
-        vmcb->star = msr_content;
+        n1_vmcb->star = msr_content;
         break;
 
     case MSR_SYSCALL_MASK:
-        vmcb->sfmask = msr_content;
+        n1_vmcb->sfmask = msr_content;
         break;
 
     case MSR_IA32_DEBUGCTLMSR:
@@ -2257,6 +2267,7 @@ static bool cf_check svm_get_pending_event(
 static uint64_t cf_check svm_get_reg(struct vcpu *v, unsigned int reg)
 {
     struct vcpu *curr = current;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
     const struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
     struct domain *d = v->domain;
 
@@ -2268,7 +2279,7 @@ static uint64_t cf_check svm_get_reg(struct vcpu *v, 
unsigned int reg)
     case MSR_SHADOW_GS_BASE:
         if ( v == curr )
             svm_sync_vmcb(v, vmcb_in_sync);
-        return vmcb->kerngsbase;
+        return n1_vmcb->kerngsbase;
 
     default:
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x) Bad register\n",
@@ -2544,7 +2555,7 @@ void asmlinkage svm_vmexit_handler(void)
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
         gdprintk(XENLOG_ERR, "invalid VMCB state:\n");
-        svm_vmcb_dump(__func__, vmcb);
+        svm_vmcb_dump(__func__, v, vmcb);
         domain_crash(v->domain);
         goto out;
     }
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 39b9e6bf2068..fc3ba7c44815 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -250,16 +250,17 @@ static void svm_dump_sel(const char *name, const struct 
segment_register *s)
            name, s->sel, s->attr, s->limit, s->base);
 }
 
-void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
+void svm_vmcb_dump(const char *from, struct vcpu *v,
+                   const struct vmcb_struct *vmcb)
 {
-    struct vcpu *curr = current;
+    struct vmcb_struct *n1_vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
 
     /*
      * If we are dumping the VMCB currently in context, some guest state may
      * still be cached in hardware.  Retrieve it.
      */
-    if ( vmcb == curr->arch.hvm.svm.vmcb )
-        svm_sync_vmcb(curr, vmcb_in_sync);
+    if ( v == current )
+        svm_sync_vmcb(v, vmcb_in_sync);
 
     printk("Dumping guest's current state at %s...\n", from);
     printk("Size of VMCB = %zu, paddr = %"PRIpaddr", vaddr = %p\n",
@@ -293,7 +294,8 @@ void svm_vmcb_dump(const char *from, const struct 
vmcb_struct *vmcb)
     printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n",
            vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes);
     printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = %#"PRIx64"\n",
-           vmcb_get_cpl(vmcb), vmcb_get_efer(vmcb), vmcb->star, vmcb->lstar);
+           vmcb_get_cpl(vmcb), vmcb_get_efer(vmcb), n1_vmcb->star,
+           n1_vmcb->lstar);
     printk("CR0 = 0x%016"PRIx64" CR2 = 0x%016"PRIx64"\n",
            vmcb_get_cr0(vmcb), vmcb_get_cr2(vmcb));
     printk("CR3 = 0x%016"PRIx64" CR4 = 0x%016"PRIx64"\n",
@@ -305,9 +307,9 @@ void svm_vmcb_dump(const char *from, const struct 
vmcb_struct *vmcb)
     printk("DR6 = 0x%016"PRIx64", DR7 = 0x%016"PRIx64"\n",
            vmcb_get_dr6(vmcb), vmcb_get_dr7(vmcb));
     printk("CSTAR = 0x%016"PRIx64" SFMask = 0x%016"PRIx64"\n",
-           vmcb->cstar, vmcb->sfmask);
+           n1_vmcb->cstar, n1_vmcb->sfmask);
     printk("KernGSBase = 0x%016"PRIx64" PAT = 0x%016"PRIx64"\n",
-           vmcb->kerngsbase, vmcb_get_g_pat(vmcb));
+           n1_vmcb->kerngsbase, vmcb_get_g_pat(vmcb));
     printk("SSP = 0x%016"PRIx64" S_CET = 0x%016"PRIx64" ISST = 
0x%016"PRIx64"\n",
            vmcb->_ssp, vmcb->_msr_s_cet, vmcb->_msr_isst);
     printk("H_CR3 = 0x%016"PRIx64" CleanBits = %#x\n",
@@ -319,12 +321,12 @@ void svm_vmcb_dump(const char *from, const struct 
vmcb_struct *vmcb)
     svm_dump_sel("  DS", &vmcb->ds);
     svm_dump_sel("  SS", &vmcb->ss);
     svm_dump_sel("  ES", &vmcb->es);
-    svm_dump_sel("  FS", &vmcb->fs);
-    svm_dump_sel("  GS", &vmcb->gs);
+    svm_dump_sel("  FS", &n1_vmcb->fs);
+    svm_dump_sel("  GS", &n1_vmcb->gs);
     svm_dump_sel("GDTR", &vmcb->gdtr);
-    svm_dump_sel("LDTR", &vmcb->ldtr);
+    svm_dump_sel("LDTR", &n1_vmcb->ldtr);
     svm_dump_sel("IDTR", &vmcb->idtr);
-    svm_dump_sel("  TR", &vmcb->tr);
+    svm_dump_sel("  TR", &n1_vmcb->tr);
 }
 
 bool svm_vmcb_isvalid(
@@ -423,7 +425,7 @@ static void cf_check vmcb_dump(unsigned char ch)
                 continue;
             }
             printk("\tVCPU %d\n", v->vcpu_id);
-            svm_vmcb_dump("key_handler", v->arch.hvm.svm.vmcb);
+            svm_vmcb_dump("key_handler", v, v->arch.hvm.svm.vmcb);
 
             process_pending_softirqs();
         }
diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
index 28dae325ced0..650465b090cf 100644
--- a/xen/arch/x86/hvm/svm/vmcb.h
+++ b/xen/arch/x86/hvm/svm/vmcb.h
@@ -563,7 +563,8 @@ int  svm_create_vmcb(struct vcpu *v);
 void svm_destroy_vmcb(struct vcpu *v);
 
 void setup_vmcb_dump(void);
-void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb);
+void svm_vmcb_dump(const char *from, struct vcpu *v,
+                   const struct vmcb_struct *vmcb);
 bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
                       const struct vcpu *v, bool verbose);
 
-- 
2.34.1