Changeset 90110 in vbox

Timestamp:

Jul 9, 2021 10:13:34 AM (4 years ago)

Author:

vboxsync

Message:

AMD IOMMU: bugref:9654 Fixed more issues in address translation resulting in non-contiguous physical addresses.
The IOTLB cache still has issues with this and thus remains disabled.

File:

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

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

@@ -801 +801 @@
 
 
-/**
- * Checks whether two consecutive I/O page lookup results translates to a physically
- * contiguous region.
- *
- * @returns @c true if they are contiguous, @c false otherwise.
- * @param   pPageLookupPrev     The I/O page lookup result of the previous page.
- * @param   pPageLookup         The I/O page lookup result of the current page.
- */
-static bool iommuAmdLookupIsAccessContig(PCIOPAGELOOKUP pPageLookupPrev, PCIOPAGELOOKUP pPageLookup)
-{
-    size_t const   cbPrev      = RT_BIT_64(pPageLookupPrev->cShift);
-    RTGCPHYS const GCPhysPrev  = pPageLookupPrev->GCPhysSpa;
-    RTGCPHYS const GCPhys      = pPageLookup->GCPhysSpa;
-
-    /* Paranoia: Ensure offset bits are 0. */
-    Assert(!(GCPhysPrev & X86_GET_PAGE_OFFSET_MASK(pPageLookupPrev->cShift)));
-    Assert(!(GCPhys     & X86_GET_PAGE_OFFSET_MASK(pPageLookup->cShift)));
-
-    /* Paranoia: Ensure permissions are identical. */
-    Assert(pPageLookupPrev->fPerm  == pPageLookup->fPerm);
-
-    return GCPhysPrev + cbPrev == GCPhys;
-}
-
-
 #ifdef IOMMU_WITH_DTE_CACHE
 /**
@@ -3535 +3510 @@
         int rc = PDMDevHlpPCIPhysRead(pDevIns, GCPhysDte, pDte, sizeof(*pDte));
         if (RT_SUCCESS(rc))
-            return rc;
+            return VINF_SUCCESS;
 
         /* Raise a device table hardware error. */
@@ -3606 +3581 @@
      * Note: This MUST be checked prior to checking the root page table level below!
      */
-    uint8_t const fDtePerm  = (pDte->au64[0] >> IOMMU_IO_PERM_SHIFT) & IOMMU_IO_PERM_MASK;
+    uint8_t const fDtePerm = (pDte->au64[0] >> IOMMU_IO_PERM_SHIFT) & IOMMU_IO_PERM_MASK;
     if ((fPerm & fDtePerm) == fPerm)
     { /* likely */ }
@@ -3898 +3873 @@
                                      PCIOMMUOPAUX pAux, PIOADDRRANGE pAddrOut, size_t *pcbPages)
 {
-    AssertPtr(pfnIoPageLookup);
-    AssertPtr(pAddrIn);
-    AssertPtr(pAddrOut);
-
     int            rc;
     size_t const   cbIova      = pAddrIn->cb;
@@ -3910 +3881 @@
     uint64_t       uIovaPage   = pAddrIn->uAddr & X86_PAGE_4K_BASE_MASK;
     uint64_t       offIova     = pAddrIn->uAddr & X86_PAGE_4K_OFFSET_MASK;
-    uint64_t       cbPages     = 0;
+    size_t         cbPages     = 0;
+    size_t const   cbPage      = X86_PAGE_4K_SIZE;
 
     IOPAGELOOKUP PageLookupPrev;
@@ -3916 +3888 @@
     for (;;)
     {
+        /* Lookup the physical page corresponding to the I/O virtual address. */
         IOPAGELOOKUP PageLookup;
         rc = pfnIoPageLookup(pDevIns, uIovaPage, fPerm, pAux, &PageLookup);
         if (RT_SUCCESS(rc))
         {
-            Assert(PageLookup.cShift >= X86_PAGE_4K_SHIFT);
+            /* Validate results of the translation. */
+            Assert(PageLookup.cShift >= X86_PAGE_4K_SHIFT && PageLookup.cShift <= 51);
+            Assert(!(PageLookup.GCPhysSpa & X86_GET_PAGE_OFFSET_MASK(PageLookup.cShift)));
+            Assert((PageLookup.fPerm & fPerm) == fPerm);
 
             /* Store the translated address before continuing to access more pages. */
@@ -3927 +3903 @@
                 uint64_t const offMask = X86_GET_PAGE_OFFSET_MASK(PageLookup.cShift);
                 uint64_t const offSpa  = uIova & offMask;
-                AssertMsg(!(PageLookup.GCPhysSpa & offMask), ("GCPhysSpa=%#RX64 offMask=%#RX64\n",
-                                                              PageLookup.GCPhysSpa, offMask));
                 GCPhysSpa = PageLookup.GCPhysSpa | offSpa;
             }
-            /* Check if addresses translated so far result in a physically contiguous region. */
-            else if (!iommuAmdLookupIsAccessContig(&PageLookupPrev, &PageLookup))
+            /*
+             * Check if translated address results in a physically contiguous region.
+             * Also ensure that the permissions for all pages in this range are identical
+             * because we specify a common permission while adding pages in this range
+             * to the IOTLB cache.
+             */
+            else if (   PageLookupPrev.GCPhysSpa + cbPage == PageLookup.GCPhysSpa
+                     && PageLookupPrev.fPerm              == PageLookup.fPerm)
+            { /* likely */ }
+            else
             {
+                Assert(cbRemaining > 0);
                 rc = VERR_OUT_OF_RANGE;
                 break;
@@ -3942 +3925 @@
 
             /* Update size of all pages read thus far. */
-            uint64_t const cbPage = RT_BIT_64(PageLookup.cShift);
             cbPages += cbPage;
 
@@ -3948 +3930 @@
             if (cbRemaining > cbPage - offIova)
             {
-                cbRemaining -= (cbPage - offIova);  /* Calculate how much more we need to access. */
-                uIovaPage   += cbPage;              /* Update address of the next access. */
-                offIova      = 0;                   /* After first page, remaining pages are accessed from offset 0. */
+                cbRemaining -= (cbPage - offIova); /* Calculate how much more we need to access. */
+                uIovaPage   += cbPage;             /* Update address of the next access. */
+                offIova      = 0;                  /* After the first page, remaining pages are accessed from offset 0. */
             }
             else
@@ -3963 +3945 @@
     }
 
-    pAddrOut->uAddr = GCPhysSpa;                /* Update the translated address. */
-    pAddrOut->cb    = cbIova - cbRemaining;     /* Update the size of the contiguous memory region. */
-    pAddrOut->fPerm = PageLookupPrev.fPerm;     /* Update the allowed permissions for this access. */
+    pAddrOut->uAddr = GCPhysSpa;                   /* Update the translated address. */
+    pAddrOut->cb    = cbIova - cbRemaining;        /* Update the size of the contiguous memory region. */
+    pAddrOut->fPerm = PageLookupPrev.fPerm;        /* Update the allowed permissions for this access. */
     if (pcbPages)
-        *pcbPages = cbPages;                    /* Update the size (in bytes) of the pages accessed. */
+        *pcbPages = cbPages;                       /* Update the size (in bytes) of the pages accessed. */
     return rc;
 }
@@ -4027 +4009 @@
                     Aux.idDomain = Dte.n.u16DomainId;
 
+                    size_t cbPages;
                     IOADDRRANGE AddrOut;
 
                     /* Lookup the address from the DTE and I/O page tables.*/
-                    size_t cbPages = 0;
                     rc = iommuAmdLookupIoAddrRange(pDevIns, iommuAmdDteLookupPage, &AddrIn, &Aux, &AddrOut, &cbPages);
                     GCPhysSpa    = AddrOut.uAddr;
                     cbContiguous = AddrOut.cb;
 
-                    /* If we stopped since translation resulted in non-contiguous physical addresses,
-                       what we translated so far is still valid. */
+                    /*
+                     * If we stopped since translation resulted in non-contiguous physical addresses
+                     * or permissions aren't identical for all pages in the access, what we translated
+                     * thus far is still valid.
+                     */
                     if (rc == VERR_OUT_OF_RANGE)
                     {
@@ -4043 +4028 @@
                         STAM_COUNTER_INC(&pThis->StatAccessDteNonContig); NOREF(pThis);
                     }
-
-                    if (rc == VERR_IOMMU_ADDR_ACCESS_DENIED)
+                    else if (rc == VERR_IOMMU_ADDR_ACCESS_DENIED)
                         STAM_COUNTER_INC(&pThis->StatAccessDtePermDenied);
 
@@ -4051 +4035 @@
                     {
                         /* Update that addresses requires translation (cumulative permissions of DTE and I/O page tables). */
-                        iommuAmdDteCacheAdd(pDevIns, idDevice, &Dte, IOMMU_DTE_CACHE_F_ADDR_TRANSLATE);
+                        iommuAmdDteCacheAdd(pDevIns, Aux.idDevice, &Dte, IOMMU_DTE_CACHE_F_ADDR_TRANSLATE);
                         /* Update IOTLB for the contiguous range of I/O virtual addresses. */
-                        iommuAmdIotlbAddRange(pDevIns, Dte.n.u16DomainId, uIova & X86_PAGE_4K_BASE_MASK, cbPages,
-                                              GCPhysSpa & X86_PAGE_4K_BASE_MASK, AddrOut.fPerm);
+                        iommuAmdIotlbAddRange(pDevIns, Aux.idDomain, AddrIn.uAddr & X86_PAGE_4K_BASE_MASK, cbPages,
+                                              AddrOut.uAddr & X86_PAGE_4K_BASE_MASK, AddrOut.fPerm);
                     }
 #endif