Changeset 86145 in vbox for trunk/src/VBox

Timestamp:

Sep 17, 2020 10:31:47 AM (4 years ago)

Author:

vboxsync

Message:

AMD IOMMU: bugref:9654 Fix unmapping MMIO region when the region address changes, resetting IOMMU base address register to 0.
Also fix setting the PCI Bus master bit on VM reset. The VM now seems to behave identically on VM reset wrt to the IOMMU.

File:

• trunk/src/VBox/Devices/Bus/DevIommuAmd.cpp (modified) (14 diffs)

trunk/src/VBox/Devices/Bus/DevIommuAmd.cpp

@@ -926 +926 @@
     iommuAmdCmdThreadWakeUpIfNeeded(pDevIns);
 
-    LogFlowFunc(("Set CmdBufHeadPtr to %#RX32\n", offBuf));
+    Log5Func(("Set CmdBufHeadPtr to %#RX32\n", offBuf));
     return VINF_SUCCESS;
 }
@@ -967 +967 @@
     iommuAmdCmdThreadWakeUpIfNeeded(pDevIns);
 
-    LogFlowFunc(("Set CmdBufTailPtr to %#RX32\n", offBuf));
+    Log5Func(("Set CmdBufTailPtr to %#RX32\n", offBuf));
     return VINF_SUCCESS;
 }
@@ -1104 +1104 @@
     Assert(!(off & (cb - 1)));
 
-    LogFlowFunc(("off=%#x cb=%u uValue=%#RX64\n", off, cb, uValue));
+    Log5Func(("off=%#x cb=%u uValue=%#RX64\n", off, cb, uValue));
 
     PIOMMU pThis = PDMDEVINS_2_DATA(pDevIns, PIOMMU);
@@ -1251 +1251 @@
     PDMPCIDEV_ASSERT_VALID(pDevIns, pPciDev);
 
-    LogFlowFunc(("off=%#x\n", off));
+    Log5Func(("off=%#x\n", off));
 
     /** @todo IOMMU: fine-grained locking? */
@@ -1410 +1410 @@
 static void iommuAmdRaiseMsiInterrupt(PPDMDEVINS pDevIns)
 {
+    LogFlowFunc(("\n"));
     if (iommuAmdIsMsiEnabled(pDevIns))
+    {
+        LogFunc(("Raising MSI\n"));
         PDMDevHlpPCISetIrq(pDevIns, 0, PDM_IRQ_LEVEL_HIGH);
+    }
 }
 
@@ -3042 +3046 @@
                         {
                             Assert(EvtError.n.u4EvtCode == IOMMU_EVT_COMMAND_HW_ERROR);
+                            LogFunc(("Raising command hardware error. Cmd=%#x -> COMMAND_HW_ERROR\n", Cmd.n.u4Opcode));
                             iommuAmdRaiseCmdHwErrorEvent(pDevIns, (PCEVT_CMD_HW_ERR_T)&EvtError);
                         }
@@ -3049 +3054 @@
                 else
                 {
+                    LogFunc(("Failed to read command at %#RGp. rc=%Rrc -> COMMAND_HW_ERROR\n", GCPhysCmd, rc));
                     EVT_CMD_HW_ERR_T EvtCmdHwErr;
                     iommuAmdInitCmdHwErrorEvent(GCPhysCmd, &EvtCmdHwErr);
@@ -3089 +3095 @@
     /** @todo IOMMU: PCI config read stat counter. */
     VBOXSTRICTRC rcStrict = PDMDevHlpPCIConfigRead(pDevIns, pPciDev, uAddress, cb, pu32Value);
-    Log3Func(("Reading PCI config register %#x (cb=%u) -> %#x %Rrc\n", uAddress, cb, *pu32Value,
-              VBOXSTRICTRC_VAL(rcStrict)));
+    Log3Func(("uAddress=%#x (cb=%u) -> %#x. rc=%Rrc\n", uAddress, cb, *pu32Value, VBOXSTRICTRC_VAL(rcStrict)));
     return rcStrict;
 }
@@ -3122 +3127 @@
     IOMMU_LOCK(pDevIns);
 
-    VBOXSTRICTRC rcStrict = VERR_INVALID_FUNCTION;
+    VBOXSTRICTRC rcStrict = VERR_IOMMU_IPE_3;
     switch (uAddress)
     {
@@ -3137 +3142 @@
             if (pThis->IommuBar.n.u1Enable)
             {
-                Assert(pThis->hMmio != NIL_IOMMMIOHANDLE);
-                Assert(PDMDevHlpMmioGetMappingAddress(pDevIns, pThis->hMmio) == NIL_RTGCPHYS);
+                Assert(pThis->hMmio != NIL_IOMMMIOHANDLE);  /* Paranoia. Ensure we have a valid IOM MMIO handle. */
                 Assert(!pThis->ExtFeat.n.u1PerfCounterSup); /* Base is 16K aligned when performance counters aren't supported. */
-                RTGCPHYS const GCPhysMmioBase = RT_MAKE_U64(pThis->IommuBar.au32[0] & 0xffffc000, pThis->IommuBar.au32[1]);
+                RTGCPHYS const GCPhysMmioBase     = RT_MAKE_U64(pThis->IommuBar.au32[0] & 0xffffc000, pThis->IommuBar.au32[1]);
+                RTGCPHYS const GCPhysMmioBasePrev = PDMDevHlpMmioGetMappingAddress(pDevIns, pThis->hMmio);
+
+                /* If the MMIO region is already mapped at the specified address, we're done. */
+                Assert(GCPhysMmioBase != NIL_RTGCPHYS);
+                if (GCPhysMmioBasePrev == GCPhysMmioBase)
+                {
+                    rcStrict = VINF_SUCCESS;
+                    break;
+                }
+
+                /* Unmap the previous MMIO region (which is at a different address). */
+                if (GCPhysMmioBasePrev != NIL_RTGCPHYS)
+                {
+                    LogFlowFunc(("Unmapping previous MMIO region at %#RGp\n", GCPhysMmioBasePrev));
+                    rcStrict = PDMDevHlpMmioUnmap(pDevIns, pThis->hMmio);
+                    if (RT_FAILURE(rcStrict))
+                    {
+                        LogFunc(("Failed to unmap MMIO region at %#RGp. rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
+                        break;
+                    }
+                }
+
+                /* Map the newly specified MMIO region. */
+                LogFlowFunc(("Mapping MMIO region at %#RGp\n", GCPhysMmioBase));
                 rcStrict = PDMDevHlpMmioMap(pDevIns, pThis->hMmio, GCPhysMmioBase);
                 if (RT_FAILURE(rcStrict))
-                    LogFunc(("Failed to map IOMMU MMIO region at %#RGp. rc=%Rrc\n", GCPhysMmioBase, rcStrict));
+                {
+                    LogFunc(("Failed to unmap MMIO region at %#RGp. rc=%Rrc\n", VBOXSTRICTRC_VAL(rcStrict)));
+                    break;
+                }
             }
+            else
+                rcStrict = VINF_SUCCESS;
             break;
         }
@@ -3174 +3207 @@
     IOMMU_UNLOCK(pDevIns);
 
-    Log3Func(("PCI config write: %#x -> To %#x (%u) %Rrc\n", u32Value, uAddress, cb, VBOXSTRICTRC_VAL(rcStrict)));
+    Log3Func(("uAddress=%#x (cb=%u) with %#x. rc=%Rrc\n", uAddress, cb, u32Value, VBOXSTRICTRC_VAL(rcStrict)));
     return rcStrict;
 }
@@ -3950 +3983 @@
     PDMPCIDEV_ASSERT_VALID(pDevIns, pPciDev);
 
+    IOMMU_LOCK_NORET(pDevIns);
+
     LogFlowFunc(("\n"));
 
@@ -4015 +4050 @@
     pThis->PprLogBOverflowEarly.u64  = 0;
 
+    pThis->IommuBar.u64              = 0;
     PDMPciDevSetDWord(pPciDev, IOMMU_PCI_OFF_BASE_ADDR_REG_LO, 0);
     PDMPciDevSetDWord(pPciDev, IOMMU_PCI_OFF_BASE_ADDR_REG_HI, 0);
 
-    /*
-     * I ASSUME all MMIO regions mapped by a PDM device are automatically unmapped
-     * on VM reset. If not, we need to enable the following...
-     */
-#if 0
-    /* Unmap the MMIO region on reset if it has been mapped previously. */
-    Assert(pThis->hMmio != NIL_IOMMMIOHANDLE);
-    if (PDMDevHlpMmioGetMappingAddress(pDevIns, pThis->hMmio) != NIL_RTGCPHYS)
-        PDMDevHlpMmioUnmap(pDevIns, pThis->hMmio);
-#endif
+    PDMPciDevSetCommand(pPciDev, VBOX_PCI_COMMAND_MASTER);
+
+    IOMMU_UNLOCK(pDevIns);
 }
 
@@ -4095 +4124 @@
     PDMPciDevSetVendorId(pPciDev,          IOMMU_PCI_VENDOR_ID);       /* AMD */
     PDMPciDevSetDeviceId(pPciDev,          IOMMU_PCI_DEVICE_ID);       /* VirtualBox IOMMU device */
-    PDMPciDevSetCommand(pPciDev,           VBOX_PCI_COMMAND_MASTER);   /* Enable bus master (as we write to main memory) */
+    PDMPciDevSetCommand(pPciDev,           VBOX_PCI_COMMAND_MASTER);   /* Enable bus master (as we directly access main memory) */
     PDMPciDevSetStatus(pPciDev,            VBOX_PCI_STATUS_CAP_LIST);  /* Capability list supported */
     PDMPciDevSetRevisionId(pPciDev,        IOMMU_PCI_REVISION_ID);     /* VirtualBox specific device implementation revision */