Changeset 82218 in vbox for trunk/src

Timestamp:

Nov 26, 2019 11:16:13 AM (5 years ago)

Author:

vboxsync

Message:

MsixCommon: Refactored the MMIO bits, changing the registration flags and code to make more sense (don't know it is correct). bugref:9218

Location:

trunk/src/VBox/Devices/Bus

Files:

• DevPciIch9.cpp (modified) (1 diff)
• MsixCommon.cpp (modified) (5 diffs)

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

@@ -980 +980 @@
     pPciDev->Int.s.pfnConfigRead    = NULL;
     pPciDev->Int.s.pfnConfigWrite   = NULL;
+    pPciDev->Int.s.hMmioMsix        = NIL_IOMMMIOHANDLE;
     if (pBus->fTypePiix3 && pPciDev->cbConfig > 256)
         pPciDev->cbConfig = 256;

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

@@ -24 +24 @@
 #include <VBox/log.h>
 #include <VBox/vmm/mm.h>
+#include <VBox/AssertGuest.h>
 
 #include <iprt/assert.h>
@@ -117 +118 @@
 }
 
-
-PDMBOTHCBDECL(int) msixR3MMIORead(PPDMDEVINS pDevIns, void *pvUser, RTGCPHYS GCPhysAddr, void *pv, unsigned cb)
+/**
+ * @callback_method_impl{FNIOMMMIONEWREAD}
+ */
+static DECLCALLBACK(VBOXSTRICTRC) msixR3MMIORead(PPDMDEVINS pDevIns, void *pvUser, RTGCPHYS off, void *pv, unsigned cb)
 {
     PPDMPCIDEV pPciDev = (PPDMPCIDEV)pvUser;
-    uint32_t   off     = (uint32_t)(GCPhysAddr & 0xffff);
-    LogFlowFunc(("GCPhysAddr=%RGp cb=%d\n", GCPhysAddr, cb));
-
-    /// @todo qword accesses?
     RT_NOREF(pDevIns);
-    AssertMsgReturn(cb == 4, ("MSI-X must be accessed with 4-byte reads\n"), VERR_INTERNAL_ERROR);
-    AssertMsgReturn(off + cb <= pPciDev->Int.s.cbMsixRegion, ("Out of bounds access for the MSI-X region\n"), VINF_IOM_MMIO_UNUSED_FF);
+
+    /* Validate IOM behaviour. */
+    Assert(cb == 4);
+    Assert((off & 3) == 0);
+
+    /* Do the read if it's within the MSI state. */
+    ASSERT_GUEST_MSG_RETURN(off + cb <= pPciDev->Int.s.cbMsixRegion, ("Out of bounds access for the MSI-X region\n"),
+                            VINF_IOM_MMIO_UNUSED_FF);
     *(uint32_t *)pv = *(uint32_t *)&pPciDev->abMsixState[off];
 
+    LogFlowFunc(("off=%RGp cb=%d -> %#010RX32\n", off, cb, *(uint32_t *)pv));
     return VINF_SUCCESS;
 }
 
-PDMBOTHCBDECL(int) msixR3MMIOWrite(PPDMDEVINS pDevIns, void *pvUser, RTGCPHYS GCPhysAddr, void const *pv, unsigned cb)
+/**
+ * @callback_method_impl{FNIOMMMIONEWWRITE}
+ */
+static DECLCALLBACK(VBOXSTRICTRC) msixR3MMIOWrite(PPDMDEVINS pDevIns, void *pvUser, RTGCPHYS off, void const *pv, unsigned cb)
 {
     PPDMPCIDEV pPciDev = (PPDMPCIDEV)pvUser;
-    uint32_t   off     = (uint32_t)(GCPhysAddr & 0xffff);
-    LogFlowFunc(("GCPhysAddr=%RGp cb=%d\n", GCPhysAddr, cb));
-
-    /// @todo qword accesses?
-    AssertMsgReturn(cb == 4, ("MSI-X must be accessed with 4-byte reads\n"), VERR_INTERNAL_ERROR);
-    AssertMsgReturn(off + cb <= pPciDev->Int.s.offMsixPba, ("Trying to write to PBA\n"), VINF_SUCCESS);
+    LogFlowFunc(("off=%RGp cb=%d %#010RX32\n", off, cb, *(uint32_t *)pv));
+
+    /* Validate IOM behaviour. */
+    Assert(cb == 4);
+    Assert((off & 3) == 0);
+
+    /* Do the write if it's within the MSI state. */
+    ASSERT_GUEST_MSG_RETURN(off + cb <= pPciDev->Int.s.offMsixPba, ("Trying to write to PBA\n"),
+                            VINF_SUCCESS);
     *(uint32_t *)&pPciDev->abMsixState[off] = *(uint32_t *)pv;
 
@@ -147 +159 @@
     msixR3CheckPendingVector(pDevIns, (PCPDMPCIHLP)pPciDev->Int.s.pvPciBusPtrR3, pPciDev, off / VBOX_MSIX_ENTRY_SIZE);
     return VINF_SUCCESS;
-}
-
-/**
- * @callback_method_impl{FNPCIIOREGIONMAP}
- */
-static DECLCALLBACK(int) msixR3Map(PPDMDEVINS pDevIns, PPDMPCIDEV pPciDev, uint32_t iRegion,
-                                   RTGCPHYS GCPhysAddress, RTGCPHYS cb, PCIADDRESSSPACE enmType)
-{
-    Assert(enmType == PCI_ADDRESS_SPACE_MEM);
-    NOREF(iRegion); NOREF(enmType);
-
-    return PDMDevHlpMMIORegister(pDevIns, GCPhysAddress, cb, pPciDev,
-                                 IOMMMIO_FLAGS_READ_PASSTHRU | IOMMMIO_FLAGS_WRITE_PASSTHRU,
-                                 msixR3MMIOWrite, msixR3MMIORead, "MSI-X tables");
 }
 
@@ -179 +177 @@
     uint8_t    iBar        = pMsiReg->iMsixBar;
 
-    AssertMsgReturn(cVectors <= VBOX_MSIX_MAX_ENTRIES,
-                    ("Too many MSI-X vectors: %d\n", cVectors),
-                    VERR_TOO_MUCH_DATA);
-    AssertMsgReturn(iBar <= 5,
-                    ("Using wrong BAR for MSI-X: %d\n", iBar),
-                    VERR_INVALID_PARAMETER);
-
+    AssertMsgReturn(cVectors <= VBOX_MSIX_MAX_ENTRIES, ("Too many MSI-X vectors: %d\n", cVectors), VERR_TOO_MUCH_DATA);
+    AssertMsgReturn(iBar <= 5, ("Using wrong BAR for MSI-X: %d\n", iBar), VERR_INVALID_PARAMETER);
     Assert(iCapOffset != 0 && iCapOffset < 0xff && iNextOffset < 0xff);
 
-    int rc = VINF_SUCCESS;
     uint16_t cbPba = cVectors / 8;
     if (cVectors % 8)
@@ -200 +192 @@
     if (!pciDevIsPassthrough(pDev))
     {
-        rc = PDMDevHlpPCIIORegionRegister(pDev->Int.s.CTX_SUFF(pDevIns), iBar, cbMsixRegion, PCI_ADDRESS_SPACE_MEM, msixR3Map);
-        if (RT_FAILURE (rc))
-            return rc;
+        /** @todo r=bird: This used to be IOMMMIO_FLAGS_READ_PASSTHRU |
+         * IOMMMIO_FLAGS_WRITE_PASSTHRU with the callbacks asserting and
+         * returning VERR_INTERNAL_ERROR on non-dword reads.  That is of
+         * course certifiable insane behaviour.  So, instead I've changed it
+         * so the callbacks only see dword reads and writes.  I'm not at all
+         * sure about the read-missing behaviour, but it seems like a good
+         * idea for now. */
+        int rc = PDMDevHlpPCIIORegionCreateMmio(pDev->Int.s.CTX_SUFF(pDevIns), iBar, cbMsixRegion, PCI_ADDRESS_SPACE_MEM,
+                                                msixR3MMIOWrite, msixR3MMIORead, pDev,
+                                                IOMMMIO_FLAGS_READ_DWORD | IOMMMIO_FLAGS_WRITE_DWORD_READ_MISSING,
+                                                "MSI-X tables", &pDev->Int.s.hMmioMsix);
+        AssertRCReturn(rc, rc);
     }