Changeset 70781 in vbox for trunk/src/VBox/VMM

Timestamp:

Jan 29, 2018 5:24:06 AM (7 years ago)

Author:

vboxsync

Message:

VMM: Nested Hw.virt: Clean up interrupt injection for nested-guests.

Location:

trunk/src/VBox/VMM

Files:

• VMMAll/CPUMAllRegs.cpp (modified) (1 diff)
• VMMAll/HMSVMAll.cpp (modified) (1 diff)
• VMMR0/HMSVMR0.cpp (modified) (3 diffs)
• VMMR3/EM.cpp (modified) (3 diffs)

trunk/src/VBox/VMM/VMMAll/CPUMAllRegs.cpp

@@ -2585 +2585 @@
 
 /**
- * Checks whether the SVM nested-guest is in a state to receive virtual
- * (injected by VMRUN) interrupts.
+ * Checks whether the SVM nested-guest is in a state to receive virtual (setup
+ * for injection by VMRUN instruction) interrupts.
  *
  * @returns VBox status code.
  * @retval  true if it's ready, false otherwise.
  *
- * @param   pCtx        The guest-CPU context.
- */
-VMM_INT_DECL(bool) CPUMCanSvmNstGstTakeVirtIntr(PCCPUMCTX pCtx)
+ * @param   pVCpu   The cross context virtual CPU structure of the calling EMT.
+ * @param   pCtx    The guest-CPU context.
+ */
+VMM_INT_DECL(bool) CPUMCanSvmNstGstTakeVirtIntr(PVMCPU pVCpu, PCCPUMCTX pCtx)
 {
 #ifdef IN_RC
-    RT_NOREF(pCtx);
+    RT_NOREF2(pVCpu, pCtx);
     AssertReleaseFailedReturn(false);
 #else
     Assert(CPUMIsGuestInSvmNestedHwVirtMode(pCtx));
     Assert(pCtx->hwvirt.fGif);
-
-    PCSVMVMCBCTRL pVmcbCtrl = &pCtx->hwvirt.svm.CTX_SUFF(pVmcb)->ctrl;
-    if (   !pVmcbCtrl->IntCtrl.n.u1IgnoreTPR
-        &&  pVmcbCtrl->IntCtrl.n.u4VIntrPrio <= pVmcbCtrl->IntCtrl.n.u8VTPR)
-        return false;
-
-    if (!pCtx->rflags.Bits.u1IF)
-        return false;
-
-    return true;
+    Assert(pCtx->hwvirt.fGif);
+
+    /*
+     * Although at present, the V_TPR and V_INTR_PRIO fields are not modified
+     * by SVM R0 code and we could inspect them directly here, we play it
+     * safe and ask HM if it has cached the VMCB.
+     */
+    if (!pCtx->hwvirt.svm.fHMCachedVmcb)
+    {
+        PCSVMVMCBCTRL pVmcbCtrl = &pCtx->hwvirt.svm.CTX_SUFF(pVmcb)->ctrl;
+        if (   !pVmcbCtrl->IntCtrl.n.u1IgnoreTPR
+            &&  pVmcbCtrl->IntCtrl.n.u4VIntrPrio <= pVmcbCtrl->IntCtrl.n.u8VTPR)
+            return false;
+
+        X86EFLAGS fEFlags;
+        if (pVmcbCtrl->IntCtrl.n.u1VIntrMasking)
+            fEFlags.u = pCtx->eflags.u;
+        else
+            fEFlags.u = pCtx->hwvirt.svm.HostState.rflags.u;
+
+        return fEFlags.Bits.u1IF;
+    }
+
+    return HMCanSvmNstGstTakeVirtIntr(pVCpu, pCtx);
 #endif
 }

trunk/src/VBox/VMM/VMMAll/HMSVMAll.cpp

@@ -535 +535 @@
     else
         fEFlags.u = pCtx->eflags.u;
-
     return fEFlags.Bits.u1IF;
 }
 
+
+/**
+ * Checks whether the SVM nested-guest is in a state to receive virtual (setup
+ * for injection by VMRUN instruction) interrupts.
+ *
+ * @returns true if it's ready, false otherwise.
+ * @param   pVCpu       The cross context virtual CPU structure of the calling EMT.
+ * @param   pCtx        The guest-CPU context.
+ *
+ * @remarks This function looks at the VMCB cache rather than directly at the
+ *          nested-guest VMCB. The latter may have been modified for executing
+ *          using hardware-assisted SVM.
+ *
+ * @sa      CPUMCanSvmNstGstTakeVirtIntr.
+ */
+VMM_INT_DECL(bool) HMCanSvmNstGstTakeVirtIntr(PVMCPU pVCpu, PCCPUMCTX pCtx)
+{
+#ifdef IN_RC
+    RT_NOREF2(pVCpu, pCtx);
+    AssertReleaseFailedReturn(false);
+#else
+    Assert(pCtx->hwvirt.svm.fHMCachedVmcb);
+    PCSVMNESTEDVMCBCACHE pVmcbNstGstCache = &pVCpu->hm.s.svm.NstGstVmcbCache;
+
+    PCSVMVMCBCTRL pVmcbCtrl = &pCtx->hwvirt.svm.CTX_SUFF(pVmcb)->ctrl;
+    if (   !pVmcbCtrl->IntCtrl.n.u1IgnoreTPR
+        &&  pVmcbCtrl->IntCtrl.n.u4VIntrPrio <= pVmcbCtrl->IntCtrl.n.u8VTPR)
+        return false;
+
+    X86EFLAGS fEFlags;
+    if (pVmcbNstGstCache->fVIntrMasking)
+        fEFlags.u = pCtx->eflags.u;
+    else
+        fEFlags.u = pCtx->hwvirt.svm.HostState.rflags.u;
+    return fEFlags.Bits.u1IF;
+#endif
+}
+

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

@@ -3505 +3505 @@
              * by the physical CPU.
              */
+            /** @todo later explore this for performance reasons. Right now the hardware
+             *        takes care of virtual interrupt injection for nested-guest. */
 #if 0
             if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST)
                 && (pVmcbNstGstCache->u64InterceptCtrl & SVM_CTRL_INTERCEPT_VINTR)
-                && CPUMCanSvmNstGstTakeVirtIntr(pCtx))
+                && CPUMCanSvmNstGstTakeVirtIntr(pVCpu, pCtx))
             {
                 Log4(("Intercepting virtual interrupt -> #VMEXIT\n"));
@@ -3641 +3643 @@
     if (fGif)
     {
+        /*
+         * For nested-guests we have no way to determine if we're injecting a physical or virtual
+         * interrupt at this point. Hence the partial verification below.
+         */
         if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
-            fAllowInt = CPUMCanSvmNstGstTakePhysIntr(pVCpu, pCtx);
+            fAllowInt = CPUMCanSvmNstGstTakePhysIntr(pVCpu, pCtx) || CPUMCanSvmNstGstTakeVirtIntr(pVCpu, pCtx);
         else
             fAllowInt = RT_BOOL(pCtx->eflags.u32 & X86_EFL_IF);
@@ -3681 +3687 @@
     }
     else
-    {
-#ifdef VBOX_WITH_NESTED_HWVIRT
-        /*
-         * If IEM emulated VMRUN and injected an event, it would not clear the EVENTINJ::Valid bit
-         * as a physical CPU clears it in the VMCB as part of the #VMEXIT (if the AMD spec. is to
-         * believed, real behavior might differ). Regardless, IEM does it only on #VMEXIT for now
-         * and since we are continuing nested-guest execution using hardware-assisted SVM, we need
-         * to clear this field otherwise we will inject the event twice, see @bugref{7243#78}.
-         */
-        if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
-            pVmcb->ctrl.EventInject.n.u1Valid = 0;
-#endif
         Assert(pVmcb->ctrl.EventInject.n.u1Valid == 0);
-    }
 
     /*

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

@@ -1641 +1641 @@
 }
 
+#ifdef VBOX_WITH_NESTED_HWVIRT
+/**
+ * Helper for emR3ForcedActions() for injecting interrupts into the
+ * nested-guest.
+ *
+ * @returns VBox status code.
+ * @param   pVCpu       The cross context virtual CPU structure.
+ * @param   pCtx        Pointer to the nested-guest CPU context.
+ * @param   pfResched   Where to store whether a reschedule is required.
+ * @param   pfInject    Where to store whether an interrupt was injected (and if
+ *                      a wake up is pending).
+ */
+static int emR3NstGstInjectIntr(PVMCPU pVCpu, PCPUMCTX pCtx, bool *pfResched, bool *pfInject)
+{
+    *pfResched = false;
+    *pfInject  = false;
+    if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
+    {
+        PVM  pVM  = pVCpu->CTX_SUFF(pVM);
+        bool fGif = pCtx->hwvirt.fGif;
+#ifdef VBOX_WITH_RAW_MODE
+        fGif &= !PATMIsPatchGCAddr(pVM, pCtx->eip);
+#endif
+        if (fGif)
+        {
+            if (CPUMCanSvmNstGstTakePhysIntr(pVCpu, pCtx))
+            {
+                Assert(pVCpu->em.s.enmState != EMSTATE_WAIT_SIPI);
+                if (VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_APIC | VMCPU_FF_INTERRUPT_PIC))
+                {
+                    if (CPUMIsGuestSvmCtrlInterceptSet(pVCpu, pCtx, SVM_CTRL_INTERCEPT_INTR))
+                    {
+                        VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_INTR, 0, 0);
+                        if (RT_SUCCESS(rcStrict))
+                        {
+                            /** @todo r=ramshankar: Do we need to signal a wakeup here? If a nested-guest
+                             *        doesn't intercept HLT but intercepts INTR? */
+                            *pfResched = true;
+                            return VINF_EM_RESCHEDULE;
+                        }
+
+                        AssertMsgFailed(("INTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
+                        return VINF_EM_TRIPLE_FAULT;
+                    }
+
+                    /* Note: it's important to make sure the return code from TRPMR3InjectEvent isn't ignored! */
+                    /** @todo this really isn't nice, should properly handle this */
+                    int rc = TRPMR3InjectEvent(pVM, pVCpu, TRPM_HARDWARE_INT);
+                    if (pVM->em.s.fIemExecutesAll && (   rc == VINF_EM_RESCHEDULE_REM
+                                                      || rc == VINF_EM_RESCHEDULE_HM
+                                                      || rc == VINF_EM_RESCHEDULE_RAW))
+                    {
+                        rc = VINF_EM_RESCHEDULE;
+                    }
+
+                    *pfResched = true;
+                    *pfInject  = true;
+                    return rc;
+                }
+            }
+
+            if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST)
+                && CPUMCanSvmNstGstTakeVirtIntr(pVCpu, pCtx))
+            {
+                if (CPUMIsGuestSvmCtrlInterceptSet(pVCpu, pCtx, SVM_CTRL_INTERCEPT_VINTR))
+                {
+                    VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_VINTR, 0, 0);
+                    if (RT_SUCCESS(rcStrict))
+                    {
+                        /** @todo r=ramshankar: Do we need to signal a wakeup here? If a nested-guest
+                         *        doesn't intercept HLT but intercepts VINTR? */
+                        *pfResched = true;
+                        return VINF_EM_RESCHEDULE;
+                    }
+
+                    AssertMsgFailed(("VINTR #VMEXIT failed! rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
+                    return VINF_EM_TRIPLE_FAULT;
+                }
+
+                VMCPU_FF_CLEAR(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST);
+                uint8_t const uNstGstVector = CPUMGetSvmNstGstInterrupt(pCtx);
+                AssertMsg(uNstGstVector > 0 && uNstGstVector <= X86_XCPT_LAST, ("Invalid VINTR vector %#x\n", uNstGstVector));
+                TRPMAssertTrap(pVCpu, uNstGstVector, TRPM_HARDWARE_INT);
+                Log(("EM: Asserting nested-guest virt. hardware intr: %#x\n", uNstGstVector));
+
+                *pfResched = true;
+                *pfInject  = true;
+                return VINF_EM_RESCHEDULE;
+            }
+        }
+        return VINF_SUCCESS;
+    }
+
+    if (CPUMIsGuestInVmxNestedHwVirtMode(pCtx))
+    { /** @todo Nested VMX. */ }
+
+    /* Shouldn't really get here. */
+    AssertMsgFailed(("Unrecognized nested hwvirt. arch!\n"));
+    return VERR_EM_INTERNAL_ERROR;
+}
+#endif
 
 /**
@@ -1951 +2052 @@
          * Interrupts.
          */
-        /** @todo this can be optimized a bit. later.   */
         bool fWakeupPending = false;
         if (    !VM_FF_IS_PENDING(pVM, VM_FF_PGM_NO_MEMORY)
@@ -1959 +2059 @@
                 && !TRPMHasTrap(pVCpu)) /* an interrupt could already be scheduled for dispatching in the recompiler. */
             {
+                Assert(!HMR3IsEventPending(pVCpu));
                 PCPUMCTX pCtx = pVCpu->em.s.pCtx;
-                bool fGif     = pCtx->hwvirt.fGif;
+#ifdef VBOX_WITH_NESTED_HWVIRT
+                if (CPUMIsGuestInNestedHwVirtMode(pCtx))
+                {
+                    bool fResched, fInject;
+                    rc2 = emR3NstGstInjectIntr(pVCpu, pCtx, &fResched, &fInject);
+                    if (fInject)
+                    {
+                        fWakeupPending = true;
+#ifdef VBOX_STRICT
+                        rcIrq = rc2;
+#endif
+                    }
+                    if (fResched)
+                        UPDATE_RC();
+                }
+                else
+#endif
+                {
+                    if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_APIC | VMCPU_FF_INTERRUPT_PIC)
+#ifdef VBOX_WITH_NESTED_HWVIRT
+                        && pCtx->hwvirt.fGif
+#endif
 #ifdef VBOX_WITH_RAW_MODE
-                /* We cannot just inspect EFLAGS when nested hw.virt is enabled (see e.g. CPUMCanSvmNstGstTakePhysIntr). */
-                fGif &= !PATMIsPatchGCAddr(pVM, pCtx->eip);
-#endif
-                if (fGif)
-                {
-                    bool fIntrEnabled;
-#ifdef VBOX_WITH_NESTED_HWVIRT
-                    if (CPUMIsGuestInSvmNestedHwVirtMode(pCtx))
-                        fIntrEnabled = CPUMCanSvmNstGstTakePhysIntr(pVCpu, pCtx);
-                    else
-                        fIntrEnabled = pCtx->eflags.Bits.u1IF;
-#else
-                    fIntrEnabled = pCtx->eflags.Bits.u1IF;
-#endif
-                    if (fIntrEnabled)
+                        && !PATMIsPatchGCAddr(pVM, pCtx->eip)
+#endif
+                        && pCtx->eflags.Bits.u1IF)
                     {
-                        Assert(!HMR3IsEventPending(pVCpu));
                         Assert(pVCpu->em.s.enmState != EMSTATE_WAIT_SIPI);
-                        if (VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_APIC | VMCPU_FF_INTERRUPT_PIC))
+                        /* Note: it's important to make sure the return code from TRPMR3InjectEvent isn't ignored! */
+                        /** @todo this really isn't nice, should properly handle this */
+                        rc2 = TRPMR3InjectEvent(pVM, pVCpu, TRPM_HARDWARE_INT);
+                        if (pVM->em.s.fIemExecutesAll && (   rc2 == VINF_EM_RESCHEDULE_REM
+                                                          || rc2 == VINF_EM_RESCHEDULE_HM
+                                                          || rc2 == VINF_EM_RESCHEDULE_RAW))
                         {
-#ifdef VBOX_WITH_NESTED_HWVIRT
-                            if (CPUMIsGuestSvmCtrlInterceptSet(pVCpu, pCtx, SVM_CTRL_INTERCEPT_INTR))
-                            {
-                                VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_INTR, 0, 0);
-                                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
-                                     *        result in trapping triple-fault intercepts? */
-                                    rc2 = VINF_EM_TRIPLE_FAULT;
-                                }
-                            }
-                            else
-#endif
-                            {
-                                /* Note: it's important to make sure the return code from TRPMR3InjectEvent isn't ignored! */
-                                /** @todo this really isn't nice, should properly handle this */
-                                rc2 = TRPMR3InjectEvent(pVM, pVCpu, TRPM_HARDWARE_INT);
-                                if (pVM->em.s.fIemExecutesAll && (   rc2 == VINF_EM_RESCHEDULE_REM
-                                                                  || rc2 == VINF_EM_RESCHEDULE_HM
-                                                                  || rc2 == VINF_EM_RESCHEDULE_RAW))
-                                    rc2 = VINF_EM_RESCHEDULE;
-#ifdef VBOX_STRICT
-                                rcIrq = rc2;
-#endif
-                            }
-                            UPDATE_RC();
-                            /* Reschedule required: We must not miss the wakeup below! */
-                            fWakeupPending = true;
-                        }
-                    }
-#ifdef VBOX_WITH_NESTED_HWVIRT
-                    /*
-                     * Check nested-guest virtual interrupts.
-                     */
-                    else if (   VMCPU_FF_IS_PENDING(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST)
-                             && CPUMCanSvmNstGstTakeVirtIntr(pCtx))
-                    {
-                        if (CPUMIsGuestSvmCtrlInterceptSet(pVCpu, pCtx, SVM_CTRL_INTERCEPT_VINTR))
-                        {
-                            VBOXSTRICTRC rcStrict = IEMExecSvmVmexit(pVCpu, SVM_EXIT_VINTR, 0, 0);
-                            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
-                                 *        result in trapping triple-fault intercepts? */
-                                rc2 = VINF_EM_TRIPLE_FAULT;
-                            }
-                        }
-                        else
-                        {
-                            /*
-                             * Prepare the nested-guest interrupt for injection.
-                             */
-                            VMCPU_FF_CLEAR(pVCpu, VMCPU_FF_INTERRUPT_NESTED_GUEST);
-                            uint8_t uNstGstVector = CPUMGetSvmNstGstInterrupt(pCtx);
-                            TRPMAssertTrap(pVCpu, uNstGstVector, TRPM_HARDWARE_INT);
-                            Log(("EM: Asserting nested-guest virt. hardware intr: %#x\n", uNstGstVector));
-                            /** @todo reschedule to HM/REM later, when the HMR0 nested-guest execution is
-                             *  done. For now just reschedule to IEM. */
                             rc2 = VINF_EM_RESCHEDULE;
                         }
+#ifdef VBOX_STRICT
+                        rcIrq = rc2;
+#endif
                         UPDATE_RC();
                         /* Reschedule required: We must not miss the wakeup below! */
                         fWakeupPending = true;
                     }
-#endif  /* VBOX_WITH_NESTED_HWVIRT */
                 }
             }