Changeset 81181 in vbox

Timestamp:

Oct 9, 2019 12:09:24 PM (5 years ago)

Author:

vboxsync

Message:

Audio/HDA: Added more SD register checks (bugref:9569).

Location:

trunk/src/VBox/Devices/Audio

Files:

• DevHDA.cpp (modified) (5 diffs)
• HDAStream.cpp (modified) (5 diffs)

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

@@ -402 +402 @@
     /* Offset 0x88 (SD0) */ \
     { offset + 0x8,  0x00004, 0xFFFFFFFF, 0xFFFFFFFF, HDA_RD_FLAG_NONE,         hdaRegReadU32 , hdaRegWriteSDCBL  , HDA_REG_IDX_STRM(name, CBL)  , #name " Cyclic Buffer Length" }, \
-    /* Offset 0x8C (SD0) */ \
-    { offset + 0xC,  0x00002, 0x0000FFFF, 0x0000FFFF, HDA_RD_FLAG_NONE,         hdaRegReadU16 , hdaRegWriteSDLVI  , HDA_REG_IDX_STRM(name, LVI)  , #name " Last Valid Index" }, \
+    /* Offset 0x8C (SD0) -- upper 8 bits are reserved */ \
+    { offset + 0xC,  0x00002, 0x0000FFFF, 0x000000FF, HDA_RD_FLAG_NONE,         hdaRegReadU16 , hdaRegWriteSDLVI  , HDA_REG_IDX_STRM(name, LVI)  , #name " Last Valid Index" }, \
     /* Reserved: FIFO Watermark. ** @todo Document this! */ \
     { offset + 0xE,  0x00002, 0x00000007, 0x00000007, HDA_RD_FLAG_NONE,         hdaRegReadU16 , hdaRegWriteSDFIFOW, HDA_REG_IDX_STRM(name, FIFOW), #name " FIFO Watermark" }, \
@@ -1368 +1368 @@
             hdaR3StreamLock(pStream);
 
-            int rc2;
+            int rc2 = VINF_SUCCESS;
 
 # ifdef VBOX_WITH_AUDIO_HDA_ASYNC_IO
@@ -1420 +1420 @@
             }
 
-            /* Enable/disable the stream. */
-            rc2 = hdaR3StreamEnable(pStream, fRun /* fEnable */);
-            AssertRC(rc2);
-
-            if (fRun)
+            if (RT_SUCCESS(rc2))
             {
-                /* Keep track of running streams. */
-                pThis->cStreamsActive++;
-
-                /* (Re-)init the stream's period. */
-                hdaR3StreamPeriodInit(&pStream->State.Period,
-                                      pStream->u8SD, pStream->u16LVI, pStream->u32CBL, &pStream->State.Cfg);
-
-                /* Begin a new period for this stream. */
-                rc2 = hdaR3StreamPeriodBegin(&pStream->State.Period, hdaWalClkGetCurrent(pThis)/* Use current wall clock time */);
+                /* Enable/disable the stream. */
+                rc2 = hdaR3StreamEnable(pStream, fRun /* fEnable */);
                 AssertRC(rc2);
 
-                rc2 = hdaR3TimerSet(pThis, pStream, TMTimerGet(pThis->pTimer[pStream->u8SD]) + pStream->State.cTransferTicks, false /* fForce */);
-                AssertRC(rc2);
-            }
-            else
-            {
-                /* Keep track of running streams. */
-                Assert(pThis->cStreamsActive);
-                if (pThis->cStreamsActive)
-                    pThis->cStreamsActive--;
-
-                /* Make sure to (re-)schedule outstanding (delayed) interrupts. */
-                hdaR3ReschedulePendingInterrupts(pThis);
-
-                /* Reset the period. */
-                hdaR3StreamPeriodReset(&pStream->State.Period);
+                if (fRun)
+                {
+                    /* Keep track of running streams. */
+                    pThis->cStreamsActive++;
+
+                    /* (Re-)init the stream's period. */
+                    hdaR3StreamPeriodInit(&pStream->State.Period,
+                                          pStream->u8SD, pStream->u16LVI, pStream->u32CBL, &pStream->State.Cfg);
+
+                    /* Begin a new period for this stream. */
+                    rc2 = hdaR3StreamPeriodBegin(&pStream->State.Period, hdaWalClkGetCurrent(pThis)/* Use current wall clock time */);
+                    AssertRC(rc2);
+
+                    rc2 = hdaR3TimerSet(pThis, pStream, TMTimerGet(pThis->pTimer[pStream->u8SD]) + pStream->State.cTransferTicks,
+                                        false /* fForce */);
+                    AssertRC(rc2);
+                }
+                else
+                {
+                    /* Keep track of running streams. */
+                    Assert(pThis->cStreamsActive);
+                    if (pThis->cStreamsActive)
+                        pThis->cStreamsActive--;
+
+                    /* Make sure to (re-)schedule outstanding (delayed) interrupts. */
+                    hdaR3ReschedulePendingInterrupts(pThis);
+
+                    /* Reset the period. */
+                    hdaR3StreamPeriodReset(&pStream->State.Period);
+                }
             }
 
@@ -1597 +1601 @@
     DEVHDA_LOCK_RETURN(pDevIns, pThis, VINF_IOM_R3_MMIO_WRITE);
 
+    const uint8_t uSD = HDA_SD_NUM_FROM_REG(pThis, LVI, iReg);
+
 #ifdef HDA_USE_DMA_ACCESS_HANDLER
-    uint8_t uSD = HDA_SD_NUM_FROM_REG(pThis, LVI, iReg);
-
     if (hdaGetDirFromSD(uSD) == PDMAUDIODIR_OUT)
     {
@@ -1613 +1617 @@
     }
 #endif
+
+    ASSERT_GUEST_LOGREL_MSG(u32Value <= UINT8_MAX, /* Should be covered by the register write mask, but just to make sure. */
+                            ("LVI for stream #%RU8 must not be bigger than %RU8\n", uSD, UINT8_MAX - 1));
 
     int rc2 = hdaRegWriteU16(pDevIns, pThis, iReg, u32Value);

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

@@ -26 +26 @@
 #include <iprt/semaphore.h>
 
+#include <VBox/AssertGuest.h>
 #include <VBox/vmm/pdmdev.h>
 #include <VBox/vmm/pdmaudioifs.h>
@@ -239 +240 @@
     AssertRCReturn(rc, rc);
 
+    ASSERT_GUEST_LOGREL_MSG_RETURN(u32CBL % pStream->State.Mapping.cbFrameSize == 0,
+                                   ("CBL for stream #%RU8 does not align to frame size\n", pStream->u8SD),
+                                   VERR_INVALID_PARAMETER);
+
     /*
      * Set the stream's timer Hz rate, based on the stream channel count.
@@ -344 +349 @@
              pStream->u8SD, pStream->u64BDLBase, pStream->u32CBL, pStream->u16LVI, pStream->u16FIFOS));
 
-    /* Make sure that mandatory parameters are set up correctly. */
-    AssertStmt(pStream->u32CBL %  pStream->State.Mapping.cbFrameSize == 0, rc = VERR_INVALID_PARAMETER);
-    AssertStmt(pStream->u16LVI >= 1,                                       rc = VERR_INVALID_PARAMETER);
-
     if (RT_SUCCESS(rc))
     {
@@ -357 +358 @@
         /* Figure out how many transfer fragments we're going to use for this stream. */
         /** @todo Use a more dynamic fragment size? */
-        Assert(pStream->u16LVI <= UINT8_MAX - 1);
         uint8_t cFragments = pStream->u16LVI + 1;
         if (cFragments <= 1)
@@ -446 +446 @@
         Assert(pStream->State.cbTransferSize);
         Assert(pStream->State.cbTransferSize % pStream->State.Mapping.cbFrameSize == 0);
-
-        /* Calculate the bytes we need to transfer to / from the stream's DMA per iteration.
-         * This is bound to the device's Hz rate and thus to the (virtual) timing the device expects. */
-        pStream->State.cbTransferChunk = (pStream->State.Cfg.Props.uHz / pStream->State.uTimerHz) * pStream->State.Mapping.cbFrameSize;
-        Assert(pStream->State.cbTransferChunk);
-        Assert(pStream->State.cbTransferChunk % pStream->State.Mapping.cbFrameSize == 0);
-
-        /* Make sure that the transfer chunk does not exceed the overall transfer size. */
-        if (pStream->State.cbTransferChunk > pStream->State.cbTransferSize)
-            pStream->State.cbTransferChunk = pStream->State.cbTransferSize;
-
-        const uint64_t cTicksPerHz = TMTimerGetFreq(pStream->pTimer) / pStream->State.uTimerHz;
-
-        /* Calculate the timer ticks per byte for this stream. */
-        pStream->State.cTicksPerByte = cTicksPerHz / pStream->State.cbTransferChunk;
-        Assert(pStream->State.cTicksPerByte);
-
-        /* Calculate timer ticks per transfer. */
-        pStream->State.cTransferTicks     = pStream->State.cbTransferChunk * pStream->State.cTicksPerByte;
-        Assert(pStream->State.cTransferTicks);
-
-        LogFunc(("[SD%RU8] Timer %uHz (%RU64 ticks per Hz), cTicksPerByte=%RU64, cbTransferChunk=%RU32, cTransferTicks=%RU64, " \
-                 "cbTransferSize=%RU32\n",
-                 pStream->u8SD, pStream->State.uTimerHz, cTicksPerHz, pStream->State.cTicksPerByte,
-                 pStream->State.cbTransferChunk, pStream->State.cTransferTicks, pStream->State.cbTransferSize));
-
-        /* Make sure to also update the stream's DMA counter (based on its current LPIB value). */
-        hdaR3StreamSetPosition(pStream, HDA_STREAM_REG(pThis, LPIB, pStream->u8SD));
+        ASSERT_GUEST_LOGREL_MSG_STMT(pStream->State.cbTransferSize,
+                                     ("Transfer size for stream #%RU8 is invalid\n", pStream->u8SD), rc = VERR_INVALID_PARAMETER);
+        if (RT_SUCCESS(rc))
+        {
+            /* Calculate the bytes we need to transfer to / from the stream's DMA per iteration.
+             * This is bound to the device's Hz rate and thus to the (virtual) timing the device expects. */
+            pStream->State.cbTransferChunk = (pStream->State.Cfg.Props.uHz / pStream->State.uTimerHz) * pStream->State.Mapping.cbFrameSize;
+            Assert(pStream->State.cbTransferChunk);
+            Assert(pStream->State.cbTransferChunk % pStream->State.Mapping.cbFrameSize == 0);
+            ASSERT_GUEST_LOGREL_MSG_STMT(pStream->State.cbTransferChunk,
+                                         ("Transfer chunk for stream #%RU8 is invalid\n", pStream->u8SD),
+                                         rc = VERR_INVALID_PARAMETER);
+            if (RT_SUCCESS(rc))
+            {
+                /* Make sure that the transfer chunk does not exceed the overall transfer size. */
+                if (pStream->State.cbTransferChunk > pStream->State.cbTransferSize)
+                    pStream->State.cbTransferChunk = pStream->State.cbTransferSize;
+
+                const uint64_t cTicksPerHz = TMTimerGetFreq(pStream->pTimer) / pStream->State.uTimerHz;
+
+                /* Calculate the timer ticks per byte for this stream. */
+                pStream->State.cTicksPerByte = cTicksPerHz / pStream->State.cbTransferChunk;
+                Assert(pStream->State.cTicksPerByte);
+
+                /* Calculate timer ticks per transfer. */
+                pStream->State.cTransferTicks = pStream->State.cbTransferChunk * pStream->State.cTicksPerByte;
+                Assert(pStream->State.cTransferTicks);
+
+                LogFunc(("[SD%RU8] Timer %uHz (%RU64 ticks per Hz), cTicksPerByte=%RU64, cbTransferChunk=%RU32, " \
+                         "cTransferTicks=%RU64, cbTransferSize=%RU32\n",
+                         pStream->u8SD, pStream->State.uTimerHz, cTicksPerHz, pStream->State.cTicksPerByte,
+                         pStream->State.cbTransferChunk, pStream->State.cTransferTicks, pStream->State.cbTransferSize));
+
+                /* Make sure to also update the stream's DMA counter (based on its current LPIB value). */
+                hdaR3StreamSetPosition(pStream, HDA_STREAM_REG(pThis, LPIB, pStream->u8SD));
 
 #ifdef LOG_ENABLED
-        hdaR3BDLEDumpAll(pThis, pStream->u64BDLBase, pStream->u16LVI + 1);
+                hdaR3BDLEDumpAll(pThis, pStream->u64BDLBase, pStream->u16LVI + 1);
 #endif
+            }
+        }
     }