Changeset 96761 in vbox

Timestamp:

Sep 16, 2022 5:40:42 AM (2 years ago)

Author:

vboxsync

Message:

VMM/IEM: Nested VMX: bugref:10092 Comments, assertions. Remove call to PGMShwMakePageNotPresent for the APIC-access page to try force shadow table entry to be not-present, this won't do. It must be instead be handled conditionally in (SyncHandlerPte).

File:

• trunk/src/VBox/VMM/VMMAll/IEMAllCImplVmxInstr.cpp (modified) (17 diffs)

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

@@ -1201 +1201 @@
      *     instruction. Interrupt inhibition for any nested-guest instruction
      *     is supplied by the guest-interruptibility state VMCS field and will
-     *     be set up as part of loading the guest state.
+     *     be set up as part of loading the guest state. Technically
+     *     blocking-by-STI is possible with VMLAUNCH/VMRESUME but we currently
+     *     disallow it since we can't distinguish it from blocking-by-MovSS
+     *     and no nested-hypervisor we care about uses STI immediately
+     *     followed by VMLAUNCH/VMRESUME.
      *
      *   - VMCPU_FF_BLOCK_NMIS needs to be cached as VM-exits caused before
@@ -1247 +1251 @@
     int rc = PGMChangeMode(pVCpu, pVCpu->cpum.GstCtx.cr0 | X86_CR0_PE, pVCpu->cpum.GstCtx.cr4, pVCpu->cpum.GstCtx.msrEFER,
                            true /* fForce */);
-    AssertRCReturn(rc, rc);
+    if (RT_SUCCESS(rc))
+    { /* likely */ }
+    else
+        return rc;
 
     /* Invalidate IEM TLBs now that we've forced a PGM mode change. */
@@ -1467 +1474 @@
          *        currently. */
         pVmcs->u32GuestIntrState |= VMX_VMCS_GUEST_INT_STATE_BLOCK_STI;
+
+        /* Clear inhibition unconditionally since we've ensured it isn't set prior to executing VMLAUNCH/VMRESUME. */
+        VMCPU_FF_CLEAR(pVCpu, VMCPU_FF_INHIBIT_INTERRUPTS);
     }
     /* Nothing to do for SMI/enclave. We don't support enclaves or SMM yet. */
@@ -2452 +2462 @@
     pVmcs->u64RoExitQual.u = u64ExitQual;
 
-    LogFlow(("vmexit: reason=%u qual=%#RX64 cs:rip=%04x:%#RX64 cr0=%#RX64 cr3=%#RX64 cr4=%#RX64\n", uExitReason,
+    LogFlow(("vmexit: reason=%u qual=%#RX64 cs:rip=%04x:%#RX64 cr0=%#RX64 cr3=%#RX64 cr4=%#RX64 eflags=%#RX32\n", uExitReason,
              pVmcs->u64RoExitQual.u, pVCpu->cpum.GstCtx.cs.Sel, pVCpu->cpum.GstCtx.rip, pVCpu->cpum.GstCtx.cr0,
-             pVCpu->cpum.GstCtx.cr3, pVCpu->cpum.GstCtx.cr4));
+             pVCpu->cpum.GstCtx.cr3, pVCpu->cpum.GstCtx.cr4, pVCpu->cpum.GstCtx.eflags.u32));
 
     /*
@@ -4622 +4632 @@
          */
         iemVmxVirtApicSetPendingWrite(pVCpu, offAccess);
+
+        LogFlowFunc(("Write access at offset %#x not intercepted -> Wrote %#RX32\n", offAccess, u32Data));
     }
     else
@@ -4640 +4652 @@
         u32Data &= s_auAccessSizeMasks[cbAccess];
         *(uint32_t *)pvData = u32Data;
+
+        LogFlowFunc(("Read access at offset %#x not intercepted -> Read %#RX32\n", offAccess, u32Data));
     }
 
@@ -7030 +7044 @@
         && (pVmcs->u32GuestIntrState & (VMX_VMCS_GUEST_INT_STATE_BLOCK_STI | VMX_VMCS_GUEST_INT_STATE_BLOCK_MOVSS)))
         EMSetInhibitInterruptsPC(pVCpu, pVmcs->u64GuestRip.u);
-    else if (VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INHIBIT_INTERRUPTS))
-        VMCPU_FF_CLEAR(pVCpu, VMCPU_FF_INHIBIT_INTERRUPTS);
+    else
+        Assert(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INHIBIT_INTERRUPTS));
 
     /* NMI blocking. */
@@ -7146 +7160 @@
                                                 pVM->iem.s.hVmxApicAccessPage, 0 /*uUser*/, NULL /*pszDesc*/);
             if (RT_SUCCESS(rc))
-            {
-                /*
-                 * This to make double sure we trigger EPT violations (rather than EPT misconfigs)
-                 * in case we somehow managed to sync the page when CPUMIsGuestVmxApicAccessPageAddr
-                 * returned false while sycing its PTE in (SyncHandlerPte).
-                 */
-                PGMShwMakePageNotPresent(pVCpu, GCPhysApicAccess, 0 /* fOpFlags */);
-            }
+            { /* likely */ }
             else
                 IEM_VMX_VMENTRY_FAILED_RET(pVCpu, pszInstr, pszFailure, kVmxVDiag_Vmentry_AddrApicAccessHandlerReg);
@@ -9886 +9893 @@
         uint32_t const fAccess   = enmAccessType == PGMACCESSTYPE_WRITE ? IEM_ACCESS_DATA_W : IEM_ACCESS_DATA_R;
         uint16_t const offAccess = GCPhysFault & GUEST_PAGE_OFFSET_MASK;
+
+        LogFlowFunc(("Fault at %#RGp (cbBuf=%u fAccess=%#x)\n", GCPhysFault, cbBuf, fAccess));
         VBOXSTRICTRC rcStrict = iemVmxVirtApicAccessMem(pVCpu, offAccess, cbBuf, pvBuf, fAccess);
         if (RT_FAILURE(rcStrict))
@@ -9918 +9927 @@
      * Handle the VMX APIC-access page only when the guest is in VMX non-root mode.
      * Otherwise we must deregister the page and allow regular RAM access.
-     * Failing to do so lands us with endless EPT misconfiguration VM-exits.
+     * Failing to do so lands us with endless EPT VM-exits.
      */
     RTGCPHYS const GCPhysPage = GCPhysFault & ~(RTGCPHYS)GUEST_PAGE_OFFSET_MASK;
@@ -9940 +9949 @@
         uint16_t const offAccess         = GCPhysNestedFault & GUEST_PAGE_OFFSET_MASK;
         bool const fIntercept = iemVmxVirtApicIsMemAccessIntercepted(pVCpu, offAccess, 1 /* cbAccess */, fAccess);
+        LogFlowFunc(("#PF at %#RGp (GCPhysNestedFault=%#RGp offAccess=%#x)\n", GCPhysFault, GCPhysNestedFault, offAccess));
         if (fIntercept)
         {
@@ -9946 +9956 @@
              * within the APIC-access page. Currently only HM is supported.
              */
-            AssertMsg(VM_IS_HM_ENABLED(pVM), ("VM-exit auxiliary info. fetching not supported for execution engine %d\n",
-                                              pVM->bMainExecutionEngine));
+            AssertMsg(VM_IS_HM_ENABLED(pVM),
+                      ("VM-exit auxiliary info. fetching not supported for execution engine %d\n", pVM->bMainExecutionEngine));
+
             HMEXITAUX HmExitAux;
             RT_ZERO(HmExitAux);
@@ -9961 +9972 @@
              * Refer to @bugref{10092#c33s} for a more detailed explanation.
              */
-            AssertMsg(HmExitAux.Vmx.uReason == VMX_EXIT_EPT_VIOLATION,
-                      ("Unexpected call to APIC-access page #PF handler for %#RGp off=%u uErr=%#RGx uReason=%u\n",
-                       GCPhysPage, offAccess, uErr, HmExitAux.Vmx.uReason));
+            AssertMsgReturn(HmExitAux.Vmx.uReason == VMX_EXIT_EPT_VIOLATION,
+                            ("Unexpected call to APIC-access page #PF handler for %#RGp offAcesss=%u uErr=%#RGx uReason=%u\n",
+                             GCPhysPage, offAccess, uErr, HmExitAux.Vmx.uReason), VERR_IEM_IPE_7);
 
             /*
@@ -9979 +9990 @@
                 else
                     enmAccess = VMXAPICACCESS_LINEAR_READ;
+
+                /* For linear-address accesss the instruction length must be valid. */
+                AssertMsg(HmExitAux.Vmx.cbInstr > 0,
+                          ("Invalid APIC-access VM-exit instruction length. cbInstr=%u\n", HmExitAux.Vmx.cbInstr));
             }
             else
@@ -9990 +10005 @@
                     enmAccess = VMXAPICACCESS_PHYSICAL_INSTR;
                 }
+
+                /* For physical accesses the instruction length is undefined, we zero it for safety and consistency. */
+                HmExitAux.Vmx.cbInstr = 0;
             }
 
@@ -10007 +10025 @@
              * Raise the APIC-access VM-exit.
              */
+            LogFlowFunc(("Raising APIC-access VM-exit from #PF handler at offset %#x\n", offAccess));
             VBOXSTRICTRC rcStrict = iemVmxVmexitApicAccessWithInfo(pVCpu, &ExitInfo, &ExitEventInfo);
             return iemExecStatusCodeFiddling(pVCpu, rcStrict);
@@ -10018 +10037 @@
          * the APIC-access page (which we derive from the faulting address).
          */
+        LogFlowFunc(("Access at offset %#x not intercepted -> VINF_EM_RAW_EMULATE_INSTR\n", offAccess));
         return VINF_EM_RAW_EMULATE_INSTR;
     }