Changeset 82401 in vbox for trunk/src/VBox/Devices/Audio

Timestamp:

Dec 4, 2019 11:33:29 PM (5 years ago)

Author:

vboxsync

Message:

DevHDA: Handle some partial/multiple register accesses (both reads and writes) in ring-0. In the read case IOM may upon our request translate a byte or word access that exactly matches the desired register into a dword access spanning multiple registers. We can only safely handle these if we know there won't be any VINF_IOM_R3_MMIO_READ statuses showing up. Fortunately we can tell that from the pfnRead functions. In the write case, the access may be a byte write to a 16-bit, 24-bit or 32-bit register. We just supply the missing bits and do it. Mind we only support doing this if it's the first byte in the register that's being accessed, but that seems to do the trick almost all of the time. bugref:9218

Location:

trunk/src/VBox/Devices/Audio

Files:

• DevHDA.cpp (modified) (16 diffs)
• DevHDA.h (modified) (1 diff)

trunk/src/VBox/Devices/Audio/DevHDA.cpp

@@ -532 +532 @@
 };
 
+#endif /* IN_RING3 */
+
 /**
  * 32-bit size indexed masks, i.e. g_afMasks[2 bytes] = 0xffff.
@@ -540 +542 @@
 };
 
-#endif /* IN_RING3 */
 
 
@@ -2981 +2982 @@
 }
 
-#endif /* IN_RING3 */
+#else   /* !IN_RING3 */
+
+/**
+ * Checks if a dword read starting with @a idxRegDsc is safe.
+ *
+ * We can guarentee it only standard reader callbacks are used.
+ * @returns true if it will always succeed, false if it may return back to
+ *          ring-3 or we're just not sure.
+ * @param   idxRegDsc       The first register descriptor in the DWORD being read.
+ */
+DECLINLINE(bool) hdaIsMultiReadSafeInRZ(unsigned idxRegDsc)
+{
+    int32_t cbLeft = 4; /* signed on purpose */
+    do
+    {
+        if (   g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU24
+            || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU16
+            || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU8
+            || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadUnimpl)
+        { /* okay */ }
+        else
+        {
+            Log4(("hdaIsMultiReadSafeInRZ: idxRegDsc=%u %s\n", idxRegDsc, g_aHdaRegMap[idxRegDsc].abbrev));
+            return false;
+        }
+
+        idxRegDsc++;
+        if (idxRegDsc < RT_ELEMENTS(g_aHdaRegMap))
+            cbLeft -= g_aHdaRegMap[idxRegDsc].offset - g_aHdaRegMap[idxRegDsc - 1].offset;
+        else
+            break;
+    } while (cbLeft > 0);
+    return true;
+}
+
+
+#endif /* !IN_RING3 */
+
 
 /* MMIO callbacks */
@@ -3029 +3067 @@
                 STAM_COUNTER_INC(&pThis->aStatRegReads[idxRegDsc]);
             }
+#ifndef IN_RING3
+            else if (!hdaIsMultiReadSafeInRZ(idxRegDsc))
+
+            {
+                STAM_COUNTER_INC(&pThis->aStatRegReadsToR3[idxRegDsc]);
+                rc = VINF_IOM_R3_MMIO_READ;
+            }
+#endif
             else
             {
@@ -3035 +3081 @@
                  * ASSUMES that only DWORD reads have sideeffects.
                  */
-#ifdef IN_RING3
-                STAM_COUNTER_INC(&pThis->StatRegMultiReads);
+                STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiReads));
+                Log4(("hdaMmioRead: multi read: %#x LB %#x %s\n", off, cb, g_aHdaRegMap[idxRegDsc].abbrev));
                 uint32_t u32Value = 0;
                 unsigned cbLeft   = 4;
@@ -3045 +3091 @@
 
                     rc = g_aHdaRegMap[idxRegDsc].pfnRead(pDevIns, pThis, idxRegDsc, &u32Tmp);
-                    Log3Func(("\tRead %s[%db] => %x (%Rrc)*\n", g_aHdaRegMap[idxRegDsc].abbrev, cbReg, u32Tmp, VBOXSTRICTRC_VAL(rc)));
+                    Log4Func(("\tRead %s[%db] => %x (%Rrc)*\n", g_aHdaRegMap[idxRegDsc].abbrev, cbReg, u32Tmp, VBOXSTRICTRC_VAL(rc)));
                     STAM_COUNTER_INC(&pThis->aStatRegReads[idxRegDsc]);
+#ifdef IN_RING3
                     if (rc != VINF_SUCCESS)
                         break;
+#else
+                    AssertMsgBreak(rc == VINF_SUCCESS, ("rc=%Rrc - impossible, we sanitized the readers!\n", VBOXSTRICTRC_VAL(rc)));
+#endif
                     u32Value |= (u32Tmp & g_afMasks[cbReg]) << ((4 - cbLeft) * 8);
 
@@ -3060 +3110 @@
                 else
                     Assert(!IOM_SUCCESS(rc));
-#else  /* !IN_RING3 */
-                /* Take the easy way out. */
-                STAM_COUNTER_INC(&pThis->aStatRegReadsToR3[idxRegDsc]);
-                rc = VINF_IOM_R3_MMIO_READ;
-#endif /* !IN_RING3 */
             }
         }
@@ -3197 +3242 @@
 #ifdef LOG_ENABLED
     uint32_t const u32LogOldValue = idxRegDsc >= 0 ? pThis->au32Regs[idxRegMem] : UINT32_MAX;
-    if (idxRegDsc == -1)
-        Log3Func(("@%#05x u32=%#010x cb=%d\n", (uint32_t)off, *(uint32_t const *)pv, cb));
-    else
-        Log3Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev));
 #endif
 
@@ -3209 +3250 @@
     if (idxRegDsc >= 0 && g_aHdaRegMap[idxRegDsc].size == cb)
     {
+        Log3Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev));
         rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, "");
         Log3Func(("\t%#x -> %#x\n", u32LogOldValue, idxRegMem != UINT32_MAX ? pThis->au32Regs[idxRegMem] : UINT32_MAX));
+    }
+    /*
+     * Sub-register access.  Supply missing bits as needed.
+     */
+    else if (   idxRegDsc >= 0
+             && cb < g_aHdaRegMap[idxRegDsc].size)
+    {
+        u64Value |=   pThis->au32Regs[g_aHdaRegMap[idxRegDsc].mem_idx]
+                    & g_afMasks[g_aHdaRegMap[idxRegDsc].size]
+                    & ~g_afMasks[cb];
+        Log4Func(("@%#05x u%u=%#0*RX64 cb=%#x cbReg=%x %s\n"
+                  "\tSupplying missing bits (%#x): %#llx -> %#llx ...\n",
+                  (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, cb, g_aHdaRegMap[idxRegDsc].size, g_aHdaRegMap[idxRegDsc].abbrev,
+                  g_afMasks[g_aHdaRegMap[idxRegDsc].size] & ~g_afMasks[cb], u64Value & g_afMasks[cb], u64Value));
+        rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, "");
+        Log4Func(("\t%#x -> %#x\n", u32LogOldValue, idxRegMem != UINT32_MAX ? pThis->au32Regs[idxRegMem] : UINT32_MAX));
+        STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegSubWrite));
     }
     /*
@@ -3218 +3277 @@
     {
 #ifdef IN_RING3
-        if (idxRegDsc >= 0 && g_aHdaRegMap[idxRegDsc].size != cb)
-            Log3Func(("\tSize mismatch: %RU32 (reg) vs %u (access)!!\n", g_aHdaRegMap[idxRegDsc].size, cb));
+        if (idxRegDsc == -1)
+            Log4Func(("@%#05x u32=%#010x cb=%d\n", (uint32_t)off, *(uint32_t const *)pv, cb));
+        else if (g_aHdaRegMap[idxRegDsc].size == cb)
+            Log4Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev));
+        else
+            Log4Func(("@%#05x u%u=%#0*RX64 %s - mismatch cbReg=%u\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value,
+                      g_aHdaRegMap[idxRegDsc].abbrev, g_aHdaRegMap[idxRegDsc].size));
 
         /*
@@ -3238 +3302 @@
                 u64Value <<= cbBefore * 8;
                 u64Value  |= pThis->au32Regs[idxRegMem] & g_afMasks[cbBefore];
-                Log3Func(("\tWithin register, supplied %u leading bits: %#llx -> %#llx ...\n",
+                Log4Func(("\tWithin register, supplied %u leading bits: %#llx -> %#llx ...\n",
                           cbBefore * 8, ~g_afMasks[cbBefore] & u64Value, u64Value));
-                STAM_COUNTER_INC(&pThis->StatRegMultiWrites);
+                STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiWrites));
             }
             else
@@ -3246 +3310 @@
         }
         else
-            STAM_COUNTER_INC(&pThis->StatRegMultiWrites);
-
+        {
+            Log4(("hdaMmioWrite: multi write: %s\n", g_aHdaRegMap[idxRegDsc].abbrev));
+            STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiWrites));
+        }
 
         /* Loop thru the write area, it may cover multiple registers. */
@@ -3261 +3327 @@
                 {
                     u64Value |= pThis->au32Regs[idxRegMem] & g_afMasks[cbReg] & ~g_afMasks[cb];
-                    Log3Func(("\tSupplying missing bits (%#x): %#llx -> %#llx ...\n",
+                    Log4Func(("\tSupplying missing bits (%#x): %#llx -> %#llx ...\n",
                               g_afMasks[cbReg] & ~g_afMasks[cb], u64Value & g_afMasks[cb], u64Value));
                 }
@@ -3268 +3334 @@
 # endif
                 rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, "*");
-                Log3Func(("\t%#x -> %#x\n", uLogOldVal, pThis->au32Regs[idxRegMem]));
+                Log4Func(("\t%#x -> %#x\n", uLogOldVal, pThis->au32Regs[idxRegMem]));
             }
             else
@@ -4717 +4783 @@
     AssertRCReturn(rc, rc);
 
+    /** @todo r=bird: The IOMMMIO_FLAGS_READ_DWORD flag isn't entirely optimal,
+     * as several frequently used registers aren't dword sized.  6.0 and earlier
+     * will go to ring-3 to handle accesses to any such register, where-as 6.1 and
+     * later will do trivial register reads in ring-0.   Real optimal code would use
+     * IOMMMIO_FLAGS_READ_PASSTHRU and do the necessary extra work to deal with
+     * anything the guest may throw at us. */
     rc = PDMDevHlpPCIIORegionCreateMmio(pDevIns, 0, 0x4000, PCI_ADDRESS_SPACE_MEM, hdaMmioWrite, hdaMmioRead, NULL /*pvUser*/,
                                         IOMMMIO_FLAGS_READ_DWORD | IOMMMIO_FLAGS_WRITE_PASSTHRU, "HDA", &pThis->hMmio);
@@ -5077 +5149 @@
                                g_aHdaRegMap[i].desc, "Regs/%03x-%s-Writes-ToR3", g_aHdaRegMap[i].offset, g_aHdaRegMap[i].abbrev);
     }
-    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReads,     STAMTYPE_COUNTER, "RegMultiReads",      STAMUNIT_OCCURENCES, "Register read not targeting just one register");
-    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWrites,    STAMTYPE_COUNTER, "RegMultiWrites",     STAMUNIT_OCCURENCES, "Register writes not targeting just one register");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReadsR3,   STAMTYPE_COUNTER, "RegMultiReadsR3",    STAMUNIT_OCCURENCES, "Register read not targeting just one register, handled in ring-3");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReadsRZ,   STAMTYPE_COUNTER, "RegMultiReadsRZ",    STAMUNIT_OCCURENCES, "Register read not targeting just one register, handled in ring-0");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWritesR3,  STAMTYPE_COUNTER, "RegMultiWritesR3",   STAMUNIT_OCCURENCES, "Register writes not targeting just one register, handled in ring-3");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWritesRZ,  STAMTYPE_COUNTER, "RegMultiWritesRZ",   STAMUNIT_OCCURENCES, "Register writes not targeting just one register, handled in ring-0");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegSubWriteR3,     STAMTYPE_COUNTER, "RegSubWritesR3",     STAMUNIT_OCCURENCES, "Trucated register writes, handled in ring-3");
+    PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegSubWriteRZ,     STAMTYPE_COUNTER, "RegSubWritesRZ",     STAMUNIT_OCCURENCES, "Trucated register writes, handled in ring-0");
     PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegUnknownReads,   STAMTYPE_COUNTER, "RegUnknownReads",    STAMUNIT_OCCURENCES, "Reads of unknown registers.");
     PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegUnknownWrites,  STAMTYPE_COUNTER, "RegUnknownWrites",   STAMUNIT_OCCURENCES, "Writes to unknown registers.");

trunk/src/VBox/Devices/Audio/DevHDA.h

@@ -209 +209 @@
     STAMCOUNTER             aStatRegWrites[HDA_NUM_REGS];
     STAMCOUNTER             aStatRegWritesToR3[HDA_NUM_REGS];
-    STAMCOUNTER             StatRegMultiReads;
-    STAMCOUNTER             StatRegMultiWrites;
+    STAMCOUNTER             StatRegMultiReadsRZ;
+    STAMCOUNTER             StatRegMultiReadsR3;
+    STAMCOUNTER             StatRegMultiWritesRZ;
+    STAMCOUNTER             StatRegMultiWritesR3;
+    STAMCOUNTER             StatRegSubWriteRZ;
+    STAMCOUNTER             StatRegSubWriteR3;
     STAMCOUNTER             StatRegUnknownReads;
     STAMCOUNTER             StatRegUnknownWrites;