Changeset 88668 in vbox

Timestamp:

Apr 23, 2021 4:17:04 AM (4 years ago)

Author:

vboxsync

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

143982

Message:

AMD IOMMU: bugref:9654 Fix locking that got broken when the PDM lock was used instead of the device's own lock. We need to lock MMIO reads and writes.

File:

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

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

@@ -173 +173 @@
 
 /** Acquires the cache lock. */
-# define IOMMU_LOCK_CACHE(a_pDevIns, a_pThis) \
+#define IOMMU_LOCK_CACHE(a_pDevIns, a_pThis) \
     do { \
         int const rcLock = PDMDevHlpCritSectEnter((a_pDevIns), &(a_pThis)->CritSectCache, VERR_SEM_BUSY); \
@@ -200 +200 @@
 
 /** Acquires the PDM lock. */
-#define IOMMU_LOCK(a_pDevIns, a_pThisCC)  \
+#define IOMMU_LOCK(a_pDevIns, a_pThisCC, a_rcBusy)  \
     do { \
-        int const rcLock = (a_pThisCC)->CTX_SUFF(pIommuHlp)->pfnLock((a_pDevIns), VERR_SEM_BUSY); \
+        int const rcLock = (a_pThisCC)->CTX_SUFF(pIommuHlp)->pfnLock((a_pDevIns), (a_rcBusy)); \
         if (RT_LIKELY(rcLock == VINF_SUCCESS)) \
         { /* likely */ } \
@@ -2716 +2716 @@
     Log4Func(("off=%#x cb=%u uValue=%#RX64\n", off, cb, uValue));
 
-    PIOMMU        pThis = PDMDEVINS_2_DATA(pDevIns, PIOMMU);
-    PCIOMMUREGACC pReg  = iommuAmdGetRegAccess(off);
+    PIOMMU        pThis   = PDMDEVINS_2_DATA(pDevIns, PIOMMU);
+    PIOMMUCC      pThisCC = PDMDEVINS_2_DATA_CC(pDevIns, PIOMMUCC);
+    PCIOMMUREGACC pReg    = iommuAmdGetRegAccess(off);
     if (pReg)
     { /* likely */ }
@@ -2743 +2744 @@
     {
         if (!(off & 7))
-            return pReg->pfnWrite(pDevIns, pThis, off, uValue);
+        {
+            IOMMU_LOCK(pDevIns, pThisCC, VINF_IOM_R3_MMIO_WRITE);
+            VBOXSTRICTRC rcStrict = pReg->pfnWrite(pDevIns, pThis, off, uValue);
+            IOMMU_UNLOCK(pDevIns, pThisCC);
+            return rcStrict;
+        }
 
         LogFunc(("Misaligned access while writing register at off=%#x (cb=%u) with %#RX64 -> Ignored\n", off, cb, uValue));
@@ -2753 +2759 @@
     if (!(off & 7))
     {
+        VBOXSTRICTRC rcStrict;
+        IOMMU_LOCK(pDevIns, pThisCC, VINF_IOM_R3_MMIO_WRITE);
+
         /*
          * Lower 32 bits of a 64-bit register or a 32-bit register is being written.
@@ -2759 +2768 @@
         uint64_t u64Read;
         if (pReg->pfnRead)
-        {
-            VBOXSTRICTRC rcStrict = pReg->pfnRead(pDevIns, pThis, off, &u64Read);
-            if (RT_FAILURE(rcStrict))
-            {
-                LogFunc(("Reading off %#x during split write failed! rc=%Rrc\n -> Ignored", off, VBOXSTRICTRC_VAL(rcStrict)));
-                return rcStrict;
-            }
-        }
+            rcStrict = pReg->pfnRead(pDevIns, pThis, off, &u64Read);
         else
+        {
+            rcStrict = VINF_SUCCESS;
             u64Read = 0;
-
-        uValue = (u64Read & UINT64_C(0xffffffff00000000)) | uValue;
-        return pReg->pfnWrite(pDevIns, pThis, off, uValue);
+        }
+
+        if (RT_SUCCESS(rcStrict))
+        {
+            uValue = (u64Read & UINT64_C(0xffffffff00000000)) | uValue;
+            rcStrict = pReg->pfnWrite(pDevIns, pThis, off, uValue);
+        }
+        else
+            LogFunc(("Reading off %#x during split write failed! rc=%Rrc\n -> Ignored", off, VBOXSTRICTRC_VAL(rcStrict)));
+
+        IOMMU_UNLOCK(pDevIns, pThisCC);
+        return rcStrict;
     }
 
@@ -2778 +2791 @@
      * Merge with lower 32 bits (after reading the full 64-bits) and perform a 64-bit write.
      */
+    VBOXSTRICTRC rcStrict;
     Assert(!(off & 3));
     Assert(off & 7);
@@ -2783 +2797 @@
     uint64_t u64Read;
     if (pReg->pfnRead)
-    {
-        VBOXSTRICTRC rcStrict = pReg->pfnRead(pDevIns, pThis, off - 4, &u64Read);
-        if (RT_FAILURE(rcStrict))
-        {
-            LogFunc(("Reading off %#x during split write failed! rc=%Rrc\n -> Ignored", off, VBOXSTRICTRC_VAL(rcStrict)));
-            return rcStrict;
-        }
-    }
+        rcStrict = pReg->pfnRead(pDevIns, pThis, off - 4, &u64Read);
     else
+    {
+        rcStrict = VINF_SUCCESS;
         u64Read = 0;
-
-    uValue = (uValue << 32) | (u64Read & UINT64_C(0xffffffff));
-    return pReg->pfnWrite(pDevIns, pThis, off - 4, uValue);
+    }
+
+    if (RT_SUCCESS(rcStrict))
+    {
+        uValue = (uValue << 32) | (u64Read & UINT64_C(0xffffffff));
+        rcStrict = pReg->pfnWrite(pDevIns, pThis, off - 4, uValue);
+    }
+    else
+        LogFunc(("Reading off %#x during split write failed! rc=%Rrc\n -> Ignored", off, VBOXSTRICTRC_VAL(rcStrict)));
+
+    IOMMU_UNLOCK(pDevIns, pThisCC);
+    return rcStrict;
 }
 
@@ -2824 +2842 @@
     Log4Func(("off=%#x\n", off));
 
-    PIOMMU      pThis = PDMDEVINS_2_DATA(pDevIns, PIOMMU);
+    PIOMMU      pThis   = PDMDEVINS_2_DATA(pDevIns, PIOMMU);
+    PIOMMUCC    pThisCC = PDMDEVINS_2_DATA_CC(pDevIns, PIOMMUCC);
     PCPDMPCIDEV pPciDev = pDevIns->apPciDevs[0];
     PDMPCIDEV_ASSERT_VALID(pDevIns, pPciDev); NOREF(pPciDev);
@@ -2851 +2870 @@
      */
     if (!(off & 7))
-        return pReg->pfnRead(pDevIns, pThis, off, puResult);
+    {
+        IOMMU_LOCK(pDevIns, pThisCC, VINF_IOM_R3_MMIO_READ);
+        VBOXSTRICTRC rcStrict = pReg->pfnRead(pDevIns, pThis, off, puResult);
+        IOMMU_UNLOCK(pDevIns, pThisCC);
+        return rcStrict;
+    }
 
     /*
@@ -2860 +2884 @@
     Assert(off & 7);
     Assert(off >= 4);
+    IOMMU_LOCK(pDevIns, pThisCC, VINF_IOM_R3_MMIO_READ);
     VBOXSTRICTRC rcStrict = pReg->pfnRead(pDevIns, pThis, off - 4, puResult);
+    IOMMU_UNLOCK(pDevIns, pThisCC);
     if (RT_SUCCESS(rcStrict))
         *puResult >>= 32;
@@ -3458 +3484 @@
     PIOMMUCC pThisCC = PDMDEVINS_2_DATA_CC(pDevIns, PIOMMUCC);
 
-    IOMMU_LOCK(pDevIns, pThisCC);
+    IOMMU_LOCK(pDevIns, pThisCC, VERR_SEM_BUSY);
 
     /* Figure out which device table segment is being accessed. */
@@ -4852 +4878 @@
                 if (pCmdComWait->n.u1Interrupt)
                 {
-                    IOMMU_LOCK(pDevIns, pThisR3);
+                    IOMMU_LOCK(pDevIns, pThisR3, VERR_IGNORED);
                     ASMAtomicOrU64(&pThis->Status.u64, IOMMU_STATUS_COMPLETION_WAIT_INTR);
                     bool const fRaiseInt = pThis->Ctrl.n.u1CompWaitIntrEn;
                     IOMMU_UNLOCK(pDevIns, pThisR3);
-
                     if (fRaiseInt)
                         iommuAmdMsiInterruptRaise(pDevIns);
@@ -5083 +5108 @@
          *        save on host memory a bit, we could (once PGM has the necessary APIs)
          *        lock the page mappings page mappings and access them directly. */
-        IOMMU_LOCK(pDevIns, pThisR3);
+        IOMMU_LOCK(pDevIns, pThisR3, VERR_IGNORED);
 
         if (pThis->Status.n.u1CmdBufRunning)
@@ -5104 +5129 @@
                 IOMMU_UNLOCK(pDevIns, pThisR3);
                 int rc = PDMDevHlpPCIPhysRead(pDevIns, GCPhysCmdBufBase, pvCmds, cbCmdBuf);
-                IOMMU_LOCK(pDevIns, pThisR3);
+                IOMMU_LOCK(pDevIns, pThisR3, VERR_IGNORED);
 
                 if (RT_SUCCESS(rc))
@@ -5267 +5292 @@
 
     PIOMMUR3 pThisR3 = PDMDEVINS_2_DATA_CC(pDevIns, PIOMMUR3);
-    IOMMU_LOCK(pDevIns, pThisR3);
+    IOMMU_LOCK(pDevIns, pThisR3, VERR_IGNORED);
 
     VBOXSTRICTRC rcStrict;
@@ -6611 +6636 @@
 
     int rc;
-    IOMMU_LOCK(pDevIns, pThisR3);
+    IOMMU_LOCK(pDevIns, pThisR3, VERR_IGNORED);
 
     /* Map MMIO regions if the IOMMU BAR is enabled. */