Changeset 75510 in vbox for trunk/src/VBox/VMM/VMMAll

Timestamp:

Nov 16, 2018 8:36:57 AM (6 years ago)

Author:

vboxsync

Message:

VMM/IEM: Nested VMX: bugref:9180 Try to unify signalling of TPR/EOI/Self-IPI virtualization/APIC-write emulation operations. Also remove
the duplicate handling of virtualization of x2APIC MSR accesses (as distinct from MSR intercepts). Currently we handle it in CPUM,
and IEM calls into CPUM so it should be handled fine, no need to handle it in IEM too.

Location:

trunk/src/VBox/VMM/VMMAll

Files:

• IEMAllCImpl.cpp.h (modified) (8 diffs)
• IEMAllCImplVmxInstr.cpp.h (modified) (6 diffs)

trunk/src/VBox/VMM/VMMAll/IEMAllCImpl.cpp.h

@@ -5909 +5909 @@
                 uint32_t const uVTpr = (uNewCrX & 0xf) << 4;
                 iemVmxVirtApicWriteRaw32(pVCpu, XAPIC_OFF_TPR, uVTpr);
-                rcStrict = iemVmxVmexitTprVirtualization(pVCpu, cbInstr);
-                if (rcStrict != VINF_VMX_INTERCEPT_NOT_ACTIVE)
-                    return rcStrict;
-                rcStrict = VINF_SUCCESS;
+                iemVmxVirtApicSignalAction(pVCpu, XAPIC_OFF_TPR);
                 break;
             }
@@ -6693 +6690 @@
         if (iemVmxIsRdmsrWrmsrInterceptSet(pVCpu, VMX_EXIT_RDMSR, pVCpu->cpum.GstCtx.ecx))
             IEM_VMX_VMEXIT_INSTR_RET(pVCpu, VMX_EXIT_RDMSR, cbInstr);
-
-        /** @todo NSTVMX: Handle other x2APIC MSRs in VMX non-root mode. Perhaps having a
-         *        dedicated virtual-APIC device might be better... */
-        if (   pVCpu->cpum.GstCtx.ecx == MSR_IA32_X2APIC_TPR
-            && IEM_VMX_IS_PROCCTLS2_SET(pVCpu, VMX_PROC_CTLS2_VIRT_X2APIC_MODE))
-        {
-            uint32_t const uVTpr = iemVmxVirtApicReadRaw32(pVCpu, XAPIC_OFF_TPR);
-            pVCpu->cpum.GstCtx.rax = uVTpr;
-            pVCpu->cpum.GstCtx.rdx = 0;
-            iemRegAddToRipAndClearRF(pVCpu, cbInstr);
-            return VINF_SUCCESS;
-        }
     }
 #endif
@@ -6779 +6764 @@
     uValue.s.Hi = pVCpu->cpum.GstCtx.edx;
 
+    uint32_t const idMsr = pVCpu->cpum.GstCtx.ecx;
+
     /** @todo make CPUMAllMsrs.cpp import the necessary MSR state. */
     IEM_CTX_IMPORT_RET(pVCpu, CPUMCTX_EXTRN_ALL_MSRS);
@@ -6788 +6775 @@
     if (IEM_VMX_IS_NON_ROOT_MODE(pVCpu))
     {
-        if (iemVmxIsRdmsrWrmsrInterceptSet(pVCpu, VMX_EXIT_WRMSR, pVCpu->cpum.GstCtx.ecx))
+        if (iemVmxIsRdmsrWrmsrInterceptSet(pVCpu, VMX_EXIT_WRMSR, idMsr))
             IEM_VMX_VMEXIT_INSTR_RET(pVCpu, VMX_EXIT_WRMSR, cbInstr);
-
-        /* Check x2APIC MSRs first. */
-        if (IEM_VMX_IS_PROCCTLS2_SET(pVCpu, VMX_PROC_CTLS2_VIRT_X2APIC_MODE))
-        {
-            switch (pVCpu->cpum.GstCtx.ecx)
-            {
-                case MSR_IA32_X2APIC_TPR:
-                {
-                    if (   !uValue.s.Hi
-                        && !(uValue.s.Lo & UINT32_C(0xffffff00)))
-                    {
-                        uint32_t const uVTpr = uValue.s.Lo;
-                        iemVmxVirtApicWriteRaw32(pVCpu, XAPIC_OFF_TPR, uVTpr);
-                        VBOXSTRICTRC rcStrict = iemVmxVmexitTprVirtualization(pVCpu, cbInstr);
-                        if (rcStrict != VINF_VMX_INTERCEPT_NOT_ACTIVE)
-                            return rcStrict;
-                        return VINF_SUCCESS;
-                    }
-                    Log(("IEM: Invalid TPR MSR write (%#x,%#x) -> #GP(0)\n", uValue.s.Hi, uValue.s.Lo));
-                    return iemRaiseGeneralProtectionFault0(pVCpu);
-                }
-
-                case MSR_IA32_X2APIC_EOI:
-                case MSR_IA32_X2APIC_SELF_IPI:
-                {
-                    /** @todo NSTVMX: EOI and Self-IPI virtualization. */
-                    break;
-                }
-            }
-        }
-        else if (pVCpu->cpum.GstCtx.ecx == MSR_IA32_BIOS_UPDT_TRIG)
-        {
-            /** @todo NSTVMX: We must not allow any microcode updates in VMX non-root mode.
-             *        Since we don't implement this MSR anyway it's currently not a problem.
-             *        If we do, we should probably move this check to the MSR handler.  */
-        }
-        else if (pVCpu->cpum.GstCtx.ecx == MSR_IA32_RTIT_CTL)
-        {
-            /** @todo NSTVMX: We don't support Intel PT yet. When we do, this MSR must #GP
-             *        when IntelPT is not supported in VMX. */
-        }
     }
 #endif
@@ -6837 +6783 @@
     if (IEM_SVM_IS_CTRL_INTERCEPT_SET(pVCpu, SVM_CTRL_INTERCEPT_MSR_PROT))
     {
-        VBOXSTRICTRC rcStrict = iemSvmHandleMsrIntercept(pVCpu, pVCpu->cpum.GstCtx.ecx, true /* fWrite */);
+        VBOXSTRICTRC rcStrict = iemSvmHandleMsrIntercept(pVCpu, idMsr, true /* fWrite */);
         if (rcStrict == VINF_SVM_VMEXIT)
             return VINF_SUCCESS;
         if (rcStrict != VINF_SVM_INTERCEPT_NOT_ACTIVE)
         {
-            Log(("IEM: SVM intercepted rdmsr(%#x) failed. rc=%Rrc\n", pVCpu->cpum.GstCtx.ecx, VBOXSTRICTRC_VAL(rcStrict)));
+            Log(("IEM: SVM intercepted rdmsr(%#x) failed. rc=%Rrc\n", idMsr, VBOXSTRICTRC_VAL(rcStrict)));
             return rcStrict;
         }
@@ -6851 +6797 @@
      * Do the job.
      */
-    VBOXSTRICTRC rcStrict = CPUMSetGuestMsr(pVCpu, pVCpu->cpum.GstCtx.ecx, uValue.u);
+    VBOXSTRICTRC rcStrict = CPUMSetGuestMsr(pVCpu, idMsr, uValue.u);
     if (rcStrict == VINF_SUCCESS)
     {
@@ -6862 +6808 @@
     if (rcStrict == VINF_CPUM_R3_MSR_WRITE)
     {
-        Log(("IEM: wrmsr(%#x) -> ring-3\n", pVCpu->cpum.GstCtx.ecx));
+        Log(("IEM: wrmsr(%#x) -> ring-3\n", idMsr));
         return rcStrict;
     }
@@ -6871 +6817 @@
     {
         pVCpu->iem.s.cLogRelWrMsr++;
-        LogRel(("IEM: wrmsr(%#x,%#x`%08x) -> #GP(0)\n", pVCpu->cpum.GstCtx.ecx, uValue.s.Hi, uValue.s.Lo));
+        LogRel(("IEM: wrmsr(%#x,%#x`%08x) -> #GP(0)\n", idMsr, uValue.s.Hi, uValue.s.Lo));
     }
     else
-        Log((   "IEM: wrmsr(%#x,%#x`%08x) -> #GP(0)\n", pVCpu->cpum.GstCtx.ecx, uValue.s.Hi, uValue.s.Lo));
+        Log((   "IEM: wrmsr(%#x,%#x`%08x) -> #GP(0)\n", idMsr, uValue.s.Hi, uValue.s.Lo));
     AssertMsgReturn(rcStrict == VERR_CPUM_RAISE_GP_0, ("%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)), VERR_IPE_UNEXPECTED_STATUS);
     return iemRaiseGeneralProtectionFault0(pVCpu);

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

@@ -911 +911 @@
 
 /**
+ * Signal that a virtual-APIC action needs to be performed at a later time (post
+ * instruction execution).
+ *
+ * @param   pVCpu       The cross context virtual CPU structure.
+ * @param   offApic     The virtual-APIC page offset that was updated pertaining to
+ *                      the event.
+ */
+DECLINLINE(void) iemVmxVirtApicSignalAction(PVMCPU pVCpu, uint16_t offApic)
+{
+    Assert(offApic < XAPIC_OFF_END + 4);
+
+    /*
+     * Record the currently updated APIC offset, as we need this later for figuring
+     * out whether to perform TPR, EOI or self-IPI virtualization as well as well
+     * as for supplying the exit qualification when causing an APIC-write VM-exit.
+     */
+    pVCpu->cpum.GstCtx.hwvirt.vmx.offVirtApicWrite = offApic;
+
+    /*
+     * Signal that we need to perform a virtual-APIC action (TPR/PPR/EOI/Self-IPI
+     * virtualization or APIC-write emulation).
+     */
+    if (!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_VMX_UPDATE_VAPIC))
+        VMCPU_FF_SET(pVCpu, VMCPU_FF_VMX_UPDATE_VAPIC);
+}
+
+
+/**
  * Masks the nested-guest CR0/CR4 mask subjected to the corresponding guest/host
  * mask and the read-shadow (CR0/CR4 read).
@@ -4314 +4342 @@
     {
         /*
-         * Record the currently updated APIC offset, as we need this later for figuring
-         * out whether to perform TPR, EOI or self-IPI virtualization as well as well
-         * as for supplying the exit qualification when causing an APIC-write VM-exit.
-         */
-        pVCpu->cpum.GstCtx.hwvirt.vmx.offVirtApicWrite = offAccess;
-
-        /*
          * A write access to the APIC-access page that is virtualized (rather than
          * causing a VM-exit) writes data to the virtual-APIC page.
@@ -4328 +4349 @@
 
         /*
+         * Record the currently updated APIC offset, as we need this later for figuring
+         * out whether to perform TPR, EOI or self-IPI virtualization as well as well
+         * as for supplying the exit qualification when causing an APIC-write VM-exit.
+         *
          * After completion of the current operation, we need to perform TPR virtualization,
          * EOI virtualization or APIC-write VM-exit depending on which register was written.
@@ -4339 +4364 @@
          * See Intel spec. 29.4.3.2 "APIC-Write Emulation".
          */
-        VMCPU_FF_SET(pVCpu, VMCPU_FF_VMX_UPDATE_VAPIC);
+        iemVmxVirtApicSignalAction(pVCpu, offAccess);
     }
     else
@@ -4423 +4448 @@
  * @retval  VERR_OUT_RANGE if the MSR read was supposed to be virtualized but was
  *          not within the range of valid MSRs, caller must raise \#GP(0).
+ * @retval  VINF_VMX_INTERCEPT_NOT_ACTIVE if the MSR must be written normally.
  *
  * @param   pVCpu       The cross context virtual CPU structure.
@@ -4469 +4495 @@
          * as for supplying the exit qualification when causing an APIC-write VM-exit.
          */
-        pVCpu->cpum.GstCtx.hwvirt.vmx.offVirtApicWrite = offReg;
-        VMCPU_FF_SET(pVCpu, VMCPU_FF_VMX_UPDATE_VAPIC);
+        iemVmxVirtApicSignalAction(pVCpu, offReg);
 
         return VINF_VMX_MODIFIES_BEHAVIOR;