Changeset 87127 in vbox for trunk/src/VBox/Devices/PC

Timestamp:

Dec 25, 2020 1:10:15 PM (4 years ago)

Author:

vboxsync

Message:

DMA,PDM: Pass the pDevIns of the caller to PDMDMAREG::pfnRegister. DMA controller now locks all registered DMA devices before doing callbacks, as it is in the best position to deal with the lock order inversion issue. bugref:9888

File:

• trunk/src/VBox/Devices/PC/DevDMA.cpp (modified) (7 diffs)

trunk/src/VBox/Devices/PC/DevDMA.cpp

@@ -103 +103 @@
 /* State information for a single DMA channel. */
 typedef struct {
+    PPDMDEVINS                          pDevInsHandler; /**< The device instance the channel is associated with. */
     RTR3PTR                             pvUser;         /* User specific context. */
     R3PTRTYPE(PFNDMATRANSFERHANDLER)    pfnXferHandler; /* Transfer handler for channel. */
@@ -591 +592 @@
 
     Log3(("DMA address %screment, mode %d\n", IS_MODE_DEC(ch->u8Mode) ? "de" : "in", ch->u8Mode >> 6));
+    AssertReturnVoid(ch->pfnXferHandler);
 
     /* Addresses and counts are shifted for 16-bit channels. */
@@ -597 +599 @@
      * transferring more than it should if auto-init is not in use.
      */
-    end_cnt = ch->pfnXferHandler(pThis->pDevIns, ch->pvUser, (ctlidx * 4) + chidx,
+    end_cnt = ch->pfnXferHandler(ch->pDevInsHandler, ch->pvUser, (ctlidx * 4) + chidx,
                                  start_cnt, (ch->u16BaseCount + 1) << dc->is16bit);
     ch->u16CurCount = end_cnt >> dc->is16bit;
@@ -624 +626 @@
     DMAControl  *dc;
     int          chidx, mask;
+
     STAM_PROFILE_START(&pThis->StatRun, a);
+
+    /* We must first lock all the devices then the DMAC or we end up with a
+       lock order validation when the callback helpers (PDMDMACREG) are being
+       invoked from I/O port and MMIO callbacks in channel devices.  While this
+       may sound a little brutish, it's actually in line with the bus locking
+       the original DMAC did back in the days. Besides, we've only got the FDC
+       and SB16 as potential customers here at present, so hardly a problem. */
+    for (unsigned idxCtl = 0; idxCtl < RT_ELEMENTS(pThis->DMAC); idxCtl++)
+        for (unsigned idxCh = 0; idxCh < RT_ELEMENTS(pThis->DMAC[idxCtl].ChState); idxCh++)
+            if (pThis->DMAC[idxCtl].ChState[idxCh].pDevInsHandler)
+                PDMDevHlpCritSectEnter(pDevIns, pThis->DMAC[idxCtl].ChState[idxCh].pDevInsHandler->pCritSectRoR3, VERR_IGNORED);
     PDMDevHlpCritSectEnter(pDevIns, pDevIns->pCritSectRoR3, VERR_IGNORED);
 
@@ -644 +658 @@
     }
 
+    /* Unlock everything (order is mostly irrelevant). */
+    for (unsigned idxCtl = 0; idxCtl < RT_ELEMENTS(pThis->DMAC); idxCtl++)
+        for (unsigned idxCh = 0; idxCh < RT_ELEMENTS(pThis->DMAC[idxCtl].ChState); idxCh++)
+            if (pThis->DMAC[idxCtl].ChState[idxCh].pDevInsHandler)
+                PDMDevHlpCritSectLeave(pDevIns, pThis->DMAC[idxCtl].ChState[idxCh].pDevInsHandler->pCritSectRoR3);
     PDMDevHlpCritSectLeave(pDevIns, pDevIns->pCritSectRoR3);
+
     STAM_PROFILE_STOP(&pThis->StatRun, a);
     return 0;
@@ -652 +672 @@
  * @interface_method_impl{PDMDMAREG,pfnRegister}
  */
-static DECLCALLBACK(void) dmaR3Register(PPDMDEVINS pDevIns, unsigned uChannel,
+static DECLCALLBACK(void) dmaR3Register(PPDMDEVINS pDevIns, unsigned uChannel, PPDMDEVINS pDevInsHandler,
                                         PFNDMATRANSFERHANDLER pfnTransferHandler, void *pvUser)
 {
@@ -661 +681 @@
 
     PDMDevHlpCritSectEnter(pDevIns, pDevIns->pCritSectRoR3, VERR_IGNORED);
+    ch->pDevInsHandler = pDevInsHandler;
     ch->pfnXferHandler = pfnTransferHandler;
     ch->pvUser = pvUser;