Changeset 69142 in vbox

Timestamp:

Oct 20, 2017 9:59:27 AM (7 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

118451

Message:

VMM: Nested Hw.virt: SVM fixes.

Location:

trunk/src/VBox/VMM

Files:

• VMMR0/HMSVMR0.cpp (modified) (19 diffs)
• VMMR3/EM.cpp (modified) (2 diffs)
• include/HMInternal.h (modified) (1 diff)

trunk/src/VBox/VMM/VMMR0/HMSVMR0.cpp

@@ -1756 +1756 @@
         hmR0SvmLoadGuestXcptIntercepts(pVCpu, pVmcb);
 
+        /** @todo Optimization: we don't need to intercept VMMCALL when the
+         *        nested-guest isn't intercepting them, and possibly others. */
+
         /* Next, merge the intercepts into the nested-guest VMCB. */
         pVmcbNstGst->ctrl.u16InterceptRdCRx |= pVmcb->ctrl.u16InterceptRdCRx;
@@ -1982 +1985 @@
                           | HM_CHANGED_GUEST_SYSENTER_ESP_MSR
                           | HM_CHANGED_GUEST_LAZY_MSRS            /* Unused. */
-                          | HM_CHANGED_SVM_NESTED_GUEST
                           | HM_CHANGED_SVM_RESERVED1              /* Reserved. */
                           | HM_CHANGED_SVM_RESERVED2
-                          | HM_CHANGED_SVM_RESERVED3);
+                          | HM_CHANGED_SVM_RESERVED3
+                          | HM_CHANGED_SVM_RESERVED4);
 
     /* All the guest state bits should be loaded except maybe the host context and/or shared host/guest bits. */
@@ -2015 +2018 @@
     PSVMNESTEDVMCBCACHE pNstGstVmcbCache = &pVCpu->hm.s.svm.NstGstVmcbCache;
 
-    pNstGstVmcbCache->u16InterceptRdCRx = pVmcbNstGstCtrl->u16InterceptRdCRx;
-    pNstGstVmcbCache->u16InterceptWrCRx = pVmcbNstGstCtrl->u16InterceptWrCRx;
-    pNstGstVmcbCache->u16InterceptRdDRx = pVmcbNstGstCtrl->u16InterceptRdDRx;
-    pNstGstVmcbCache->u16InterceptWrCRx = pVmcbNstGstCtrl->u16InterceptWrDRx;
-    pNstGstVmcbCache->u32InterceptXcpt  = pVmcbNstGstCtrl->u32InterceptXcpt;
-    pNstGstVmcbCache->u64InterceptCtrl  = pVmcbNstGstCtrl->u64InterceptCtrl;
-    pNstGstVmcbCache->u64CR3            = pVmcbNstGstState->u64CR3;
-    pNstGstVmcbCache->u64CR4            = pVmcbNstGstState->u64CR4;
-    pNstGstVmcbCache->u64IOPMPhysAddr   = pVmcbNstGstCtrl->u64IOPMPhysAddr;
-    pNstGstVmcbCache->u64MSRPMPhysAddr  = pVmcbNstGstCtrl->u64MSRPMPhysAddr;
-    pNstGstVmcbCache->u64VmcbCleanBits  = pVmcbNstGstCtrl->u64VmcbCleanBits;
-    pNstGstVmcbCache->fVIntrMasking     = pVmcbNstGstCtrl->IntCtrl.n.u1VIntrMasking;
-    pNstGstVmcbCache->TLBCtrl           = pVmcbNstGstCtrl->TLBCtrl;
-    pNstGstVmcbCache->NestedPagingCtrl  = pVmcbNstGstCtrl->NestedPaging;
-    pNstGstVmcbCache->fValid            = true;
+    /*
+     * Cache the nested-guest programmed VMCB fields if we have not cached it yet.
+     * Otherwise we risk re-caching the values we may have modified, see @bugref{7243#c44}.
+     */
+    if (!pNstGstVmcbCache->fValid)
+    {
+        pNstGstVmcbCache->u16InterceptRdCRx = pVmcbNstGstCtrl->u16InterceptRdCRx;
+        pNstGstVmcbCache->u16InterceptWrCRx = pVmcbNstGstCtrl->u16InterceptWrCRx;
+        pNstGstVmcbCache->u16InterceptRdDRx = pVmcbNstGstCtrl->u16InterceptRdDRx;
+        pNstGstVmcbCache->u16InterceptWrDRx = pVmcbNstGstCtrl->u16InterceptWrDRx;
+        pNstGstVmcbCache->u32InterceptXcpt  = pVmcbNstGstCtrl->u32InterceptXcpt;
+        pNstGstVmcbCache->u64InterceptCtrl  = pVmcbNstGstCtrl->u64InterceptCtrl;
+        pNstGstVmcbCache->u64CR3            = pVmcbNstGstState->u64CR3;
+        pNstGstVmcbCache->u64CR4            = pVmcbNstGstState->u64CR4;
+        pNstGstVmcbCache->u64IOPMPhysAddr   = pVmcbNstGstCtrl->u64IOPMPhysAddr;
+        pNstGstVmcbCache->u64MSRPMPhysAddr  = pVmcbNstGstCtrl->u64MSRPMPhysAddr;
+        pNstGstVmcbCache->u64VmcbCleanBits  = pVmcbNstGstCtrl->u64VmcbCleanBits;
+        pNstGstVmcbCache->fVIntrMasking     = pVmcbNstGstCtrl->IntCtrl.n.u1VIntrMasking;
+        pNstGstVmcbCache->TLBCtrl           = pVmcbNstGstCtrl->TLBCtrl;
+        pNstGstVmcbCache->NestedPagingCtrl  = pVmcbNstGstCtrl->NestedPaging;
+        pNstGstVmcbCache->fValid            = true;
+        Log4(("hmR0SvmVmRunCacheVmcb: Cached VMCB fields\n"));
+    }
 }
 
@@ -2073 +2084 @@
 
 /**
- * Sets up the nested-guest for hardware-assisted SVM execution.
- *
- * @param   pVCpu           The cross context virtual CPU structure.
- * @param   pCtx            Pointer to the guest-CPU context.
- */
-static void hmR0SvmLoadGuestVmcbNested(PVMCPU pVCpu, PCPUMCTX pCtx)
-{
-    if (HMCPU_CF_IS_PENDING(pVCpu, HM_CHANGED_SVM_NESTED_GUEST))
-    {
-        hmR0SvmVmRunSetupVmcb(pVCpu, pCtx);
-        HMCPU_CF_CLEAR(pVCpu, HM_CHANGED_SVM_NESTED_GUEST);
-    }
-}
-
-
-/**
  * Loads the nested-guest state into the VMCB.
  *
@@ -2104 +2099 @@
     Assert(pVmcbNstGst);
 
-    /* First, we need to setup the nested-guest VMCB for hardware-assisted SVM execution. */
-    hmR0SvmLoadGuestVmcbNested(pVCpu, pCtx);
+    hmR0SvmVmRunSetupVmcb(pVCpu, pCtx);
 
     hmR0SvmLoadGuestSegmentRegs(pVCpu, pVmcbNstGst, pCtx);
@@ -2134 +2128 @@
                           | HM_CHANGED_SVM_RESERVED1              /* Reserved. */
                           | HM_CHANGED_SVM_RESERVED2
-                          | HM_CHANGED_SVM_RESERVED3);
+                          | HM_CHANGED_SVM_RESERVED3
+                          | HM_CHANGED_SVM_RESERVED4);
 
     /* All the guest state bits should be loaded except maybe the host context and/or shared host/guest bits. */
@@ -3041 +3036 @@
     Assert(!pVCpu->hm.s.Event.fPending);
 
-    bool const fIntrEnabled = pCtx->hwvirt.svm.fGif && CPUMCanSvmNstGstTakePhysIntr(pCtx);
-    if (fIntrEnabled)
+    bool const fGif = pCtx->hwvirt.svm.fGif;
+    if (fGif)
     {
         PSVMVMCB pVmcbNstGst = pCtx->hwvirt.svm.CTX_SUFF(pVmcb);
-        SVMEVENT Event;
-        Event.u = 0;
 
         bool const fIntShadow = hmR0SvmIsIntrShadowActive(pVCpu, pCtx);
@@ -3070 +3063 @@
                 Log4(("Pending NMI\n"));
 
+                SVMEVENT Event;
+                Event.u = 0;
                 Event.n.u1Valid  = 1;
                 Event.n.u8Vector = X86_XCPT_NMI;
@@ -3082 +3077 @@
 
         /*
-         * Check if the nested-guest can receive external interrupts (PIC/APIC).
+         * Check if the nested-guest can receive external interrupts (generated by
+         * the guest's PIC/APIC).
          *
-         * Physical (from the nested-guest's point of view) intercepts are -always-
-         * intercepted, see HMSVM_MANDATORY_NESTED_GUEST_CTRL_INTERCEPTS.
+         * External intercepts from the physical CPU are -always- intercepted when
+         * executing using hardware-assisted SVM, see HMSVM_MANDATORY_NESTED_GUEST_CTRL_INTERCEPTS.
          *
-         * Physical interrupts take priority over virtual interrupts,
+         * External interrupts that are generated for the outer guest may be intercepted
+         * depending on how the nested-guest VMCB was programmed by guest software.
+         *
+         * Physical interrupts always take priority over virtual interrupts,
          * see AMD spec. 15.21.4 "Injecting Virtual (INTR) Interrupts".
-         *
-         * We must be careful that the call to CPUMCanSvmNstGstTakePhysIntr below
-         * happens -before- modifying the nested-guests's V_INTR_MASKING bit,
-         * which is currently set later in hmR0SvmLoadGuestApicStateNested.
          */
         if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_APIC | VMCPU_FF_INTERRUPT_PIC)
@@ -3099 +3094 @@
             && CPUMCanSvmNstGstTakePhysIntr(pCtx))
         {
-            return IEMExecSvmVmexit(pVCpu, SVM_EXIT_INTR, 0, 0);
+            if (CPUMIsGuestSvmCtrlInterceptSet(pCtx, SVM_CTRL_INTERCEPT_INTR))
+            {
+                Log4(("Intercepting external interrupt -> #VMEXIT\n"));
+                return IEMExecSvmVmexit(pVCpu, SVM_EXIT_INTR, 0, 0);
+            }
+
+            uint8_t u8Interrupt;
+            int rc = PDMGetInterrupt(pVCpu, &u8Interrupt);
+            if (RT_SUCCESS(rc))
+            {
+                Log4(("Injecting external interrupt u8Interrupt=%#x\n", u8Interrupt));
+
+                SVMEVENT Event;
+                Event.u = 0;
+                Event.n.u1Valid  = 1;
+                Event.n.u8Vector = u8Interrupt;
+                Event.n.u3Type   = SVM_EVENT_EXTERNAL_IRQ;
+
+                hmR0SvmSetPendingEvent(pVCpu, &Event, 0 /* GCPtrFaultAddress */);
+            }
+            else if (rc == VERR_APIC_INTR_MASKED_BY_TPR)
+            {
+                /*
+                 * AMD-V has no TPR thresholding feature. We just avoid posting the interrupt.
+                 * We just avoid delivering the TPR-masked interrupt here. TPR will be updated
+                 * always via hmR0SvmLoadGuestState() -> hmR0SvmLoadGuestApicState().
+                 */
+                STAM_COUNTER_INC(&pVCpu->hm.s.StatSwitchTprMaskedIrq);
+            }
+            else
+                STAM_COUNTER_INC(&pVCpu->hm.s.StatSwitchGuestIrq);
         }
 
         /*
-         * Check if the nested-guest can receive virtual interrupts.
+         * Check if the nested-guest can receive virtual (injected by VMRUN) interrupts.
+         * We can call CPUMCanSvmNstGstTakeVirtIntr here as we don't cache/modify any
+         * nested-guest VMCB interrupt control fields besides V_INTR_MASKING, see hmR0SvmVmRunCacheVmcb.
          */
         if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST)
             && CPUMCanSvmNstGstTakeVirtIntr(pCtx))
         {
-            uint8_t const u8Interrupt = CPUMGetSvmNstGstInterrupt(pCtx);
-            Log4(("Injecting virtual interrupt u8Interrupt=%#x\n", u8Interrupt));
-
-            Event.n.u1Valid  = 1;
-            Event.n.u8Vector = u8Interrupt;
-            Event.n.u3Type   = SVM_EVENT_EXTERNAL_IRQ;
-
-            VMCPU_FF_CLEAR(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST);
-            hmR0SvmSetPendingEvent(pVCpu, &Event, 0 /* GCPtrFaultAddress */);
-            return VINF_SUCCESS;
+            Log4(("Intercepting external interrupt -> #VMEXIT\n"));
+            return IEMExecSvmVmexit(pVCpu, SVM_EXIT_VINTR, 0, 0);
         }
     }
@@ -3153 +3172 @@
         bool const fBlockNmi  = VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_BLOCK_NMIS);
         PSVMVMCB pVmcb        = pVCpu->hm.s.svm.pVmcb;
-
-        SVMEVENT Event;
-        Event.u = 0;
 
         Log4Func(("fGif=%RTbool fBlockInt=%RTbool fIntShadow=%RTbool APIC/PIC_Pending=%RTbool\n", fGif, fBlockInt, fIntShadow,
@@ -3171 +3187 @@
                 Log4(("Pending NMI\n"));
 
+                SVMEVENT Event;
+                Event.u = 0;
                 Event.n.u1Valid  = 1;
                 Event.n.u8Vector = X86_XCPT_NMI;
@@ -3197 +3215 @@
                     Log4(("Injecting external interrupt u8Interrupt=%#x\n", u8Interrupt));
 
+                    SVMEVENT Event;
+                    Event.u = 0;
                     Event.n.u1Valid  = 1;
                     Event.n.u8Vector = u8Interrupt;
@@ -3532 +3552 @@
     HMSVM_ASSERT_PREEMPT_SAFE();
 
+    if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
+    {
 #ifdef VBOX_WITH_NESTED_HWVIRT_ONLY_IN_IEM
-    if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
-    {
         Log2(("hmR0SvmPreRunGuest: Rescheduling to IEM due to nested-hwvirt or forced IEM exec -> VINF_EM_RESCHEDULE_REM\n"));
         return VINF_EM_RESCHEDULE_REM;
-    }
 #endif
+    }
+    else
+        return VINF_SVM_VMEXIT;
 
     /* Check force flag actions that might require us to go back to ring-3. */
@@ -3573 +3595 @@
     /** @todo Get new STAM counter for this? */
     STAM_COUNTER_INC(&pVCpu->hm.s.StatLoadFull);
+
+    PCSVMNESTEDVMCBCACHE pNstGstVmcbCache = &pVCpu->hm.s.svm.NstGstVmcbCache;
+    Assert(pNstGstVmcbCache->fValid);
 
     /*
@@ -4106 +4131 @@
     hmR0SvmSaveGuestState(pVCpu, pMixedCtx, pVmcbNstGst);       /* Save the nested-guest state from the VMCB to the
                                                                    guest-CPU context. */
+
+    /*
+     * Currently, reload the entire nested-guest VMCB due to code that directly inspects
+     * the nested-guest VMCB instead of the cache, e.g. hmR0SvmEvaluatePendingEventNested.
+     */
+    HMSvmNstGstVmExitNotify(pVCpu, pVmcbNstGst);
+    HMCPU_CF_SET(pVCpu, HM_CHANGED_ALL_GUEST);
 }
 #endif
@@ -4714 +4746 @@
         case SVM_EXIT_WRITE_CR8:   /** @todo Shouldn't writes to CR8 go to V_TPR instead since we run with V_INTR_MASKING set?? */
         {
+            Log4(("hmR0SvmHandleExitNested: Write CRx: u16InterceptWrCRx=%#x u64ExitCode=%#RX64 %#x\n",
+                  pVmcbNstGstCache->u16InterceptWrCRx, pSvmTransient->u64ExitCode,
+                  (1U << (uint16_t)(pSvmTransient->u64ExitCode - SVM_EXIT_WRITE_CR0))));
             if (pVmcbNstGstCache->u16InterceptWrCRx & (1U << (uint16_t)(pSvmTransient->u64ExitCode - SVM_EXIT_WRITE_CR0)))
                 HM_SVM_RET_VMEXIT_NESTED(pVCpu, uExitCode, uExitInfo1, uExitInfo2);
@@ -6092 +6127 @@
             uint8_t const cbInstr = pVmcb->ctrl.u64NextRIP - pCtx->rip;
             uint8_t const iGReg   = pVmcb->ctrl.u64ExitInfo1 & SVM_EXIT1_MOV_CRX_GPR_NUMBER;
+            Log4(("hmR0SvmExitWriteCRx: Mov CR%u w/ iGReg=%#x\n", iCrReg, iGReg));
             rcStrict = IEMExecDecodedMovCRxWrite(pVCpu, cbInstr, iCrReg, iGReg);
             fDecodedInstr = true;
@@ -6100 +6136 @@
     if (!fDecodedInstr)
     {
+        Log4(("hmR0SvmExitWriteCRx: iCrReg=%#x\n", iCrReg));
         rcStrict = IEMExecOneBypassEx(pVCpu, CPUMCTX2CORE(pCtx), NULL);
         if (RT_UNLIKELY(   rcStrict == VERR_IEM_ASPECT_NOT_IMPLEMENTED

trunk/src/VBox/VMM/VMMR3/EM.cpp

@@ -1990 +1990 @@
                         {
                             VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_INTR, 0, 0);
-                            if (rcStrict == VINF_SVM_VMEXIT)
+                            if (RT_SUCCESS(rcStrict))
                                 rc2 = VINF_EM_RESCHEDULE;
                             else
                             {
+                                AssertMsgFailed(("INTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
                                 Log(("EM: SVM Nested-guest INTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
                                 /** @todo should we call iemInitiateCpuShutdown? Should this
@@ -2029 +2030 @@
                             {
                                 VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_VINTR, 0, 0);
-                                if (rcStrict == VINF_SVM_VMEXIT)
+                                if (RT_SUCCESS(rcStrict))
                                     rc2 = VINF_EM_RESCHEDULE;
                                 else
                                 {
+                                    AssertMsgFailed(("VINTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
                                     Log(("EM: SVM Nested-guest VINTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
                                     /** @todo should we call iemInitiateCpuShutdown? Should this

trunk/src/VBox/VMM/include/HMInternal.h

@@ -186 +186 @@
 #define HM_CHANGED_SVM_RESERVED2                 RT_BIT(21)
 #define HM_CHANGED_SVM_RESERVED3                 RT_BIT(22)
-#define HM_CHANGED_SVM_NESTED_GUEST              RT_BIT(23)
+#define HM_CHANGED_SVM_RESERVED4                 RT_BIT(23)
 
 #define HM_CHANGED_ALL_GUEST                     (  HM_CHANGED_GUEST_CR0                \