Changeset 103681 in vbox

Timestamp:

Mar 5, 2024 1:55:44 PM (13 months ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

162046

Message:

DevXHCI: Fixed a rare race condition which could miss a ringing doorbell. Also added more debugging code (not compiled by default) and print doorbell state in VM debugger (see bugref:10113).

File:

• trunk/src/VBox/Devices/USB/DevXHCI.cpp (modified) (10 diffs)

trunk/src/VBox/Devices/USB/DevXHCI.cpp

@@ -1604 +1604 @@
 #endif
 
-    /** Flag indicating a sleeping worker thread. */
-    volatile bool                   fWrkThreadSleeping;
+    /** Flag indicating a pending worker thread notification. */
+    volatile bool                   fNotificationSent;
     volatile bool                   afPadding[3];
 
@@ -2166 +2166 @@
 
     /* Tell the worker thread there's something to do. */
-    if (ASMAtomicReadBool(&pThis->fWrkThreadSleeping))
+    if (!ASMAtomicXchgBool(&pThis->fNotificationSent, true))
     {
         LogFlowFunc(("Signal event semaphore\n"));
@@ -4353 +4353 @@
 
         /* Abort the endpoint, i.e. cancel any outstanding URBs. This needs to be done after
-         * writing back the EP state so that the completion callback can operate.
+         * writing back the EP state so that the completion callback can operate. Note that
+         * the completion callback will not modify the TR when it sees that the EP is not in
+         * the 'running' state.
+         * NB: If a URB is canceled before it completed, we have no way to tell if any data
+         * was already (partially) transferred.
          */
         if (RT_SUCCESS(xhciR3FindRhDevBySlot(pDevIns, pThis, pThisCC, uSlotID, &pRh, &uPort)))
@@ -4364 +4368 @@
         }
 
-        /// @todo The completion callbacks should do more work for canceled URBs.
         /* Once the completion callbacks had a chance to run, we have to adjust
          * the endpoint state.
@@ -4372 +4375 @@
         PDMDevHlpPCIPhysReadMeta(pDevIns, GCPhysEndp, &endp_ctx, sizeof(endp_ctx));
 
+        /* If the enqueue and dequeue pointers are different, a transfer was
+         * in progress.
+         */
         bool fXferWasInProgress = endp_ctx.trep != endp_ctx.trdp;
 
@@ -5253 +5259 @@
     while (pThread->enmState == PDMTHREADSTATE_RUNNING)
     {
-        uint32_t    u32Tasks = 0;
         uint8_t     uSlotID;
 
-        ASMAtomicWriteBool(&pThis->fWrkThreadSleeping, true);
-        u32Tasks = ASMAtomicXchgU32(&pThis->u32TasksNew, 0);
-        if (!u32Tasks)
-        {
-            Assert(ASMAtomicReadBool(&pThis->fWrkThreadSleeping));
+        bool fNotificationSent = ASMAtomicXchgBool(&pThis->fNotificationSent, false);
+        if (!fNotificationSent)
+        {
             rc = PDMDevHlpSUPSemEventWaitNoResume(pDevIns, pThis->hEvtProcess, RT_INDEFINITE_WAIT);
             AssertLogRelMsgReturn(RT_SUCCESS(rc) || rc == VERR_INTERRUPTED, ("%Rrc\n", rc), rc);
@@ -5266 +5269 @@
                 break;
             LogFlowFunc(("Woken up with rc=%Rrc\n", rc));
-            u32Tasks = ASMAtomicXchgU32(&pThis->u32TasksNew, 0);
+            ASMAtomicWriteBool(&pThis->fNotificationSent, false);
         }
 
@@ -5306 +5309 @@
 
         RTCritSectLeave(&pThisCC->CritSectThrd);
-
-        ASMAtomicWriteBool(&pThis->fWrkThreadSleeping, false);
     } /* While running */
 
@@ -7566 +7567 @@
         return;
     }
+
+    if (pszArgs && strstr(pszArgs, "genintrhw"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating hardware interrupt (external)...\n");
+        int iIntr = 0;
+        PXHCIINTRPTR pIntr = &pThis->aInterrupters[iIntr & XHCI_INTR_MASK];
+        xhciSetIntr(pDevIns, pThis, pIntr);
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "genintrint"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating hardware interrupt (internal)...\n");
+        int iIntr = 0;
+        PXHCIINTRPTR pIntr = &pThis->aInterrupters[iIntr & XHCI_INTR_MASK];
+        xhciR3SetIntrPending(pDevIns, pThis, pIntr);
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "genintrhw"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating hardware interrupt (external)...\n");
+        int iIntr = 0;
+        PXHCIINTRPTR pIntr = &pThis->aInterrupters[iIntr & XHCI_INTR_MASK];
+        xhciSetIntr(pDevIns, pThis, pIntr);
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "genintrint"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating hardware interrupt (internal)...\n");
+        int iIntr = 0;
+        PXHCIINTRPTR pIntr = &pThis->aInterrupters[iIntr & XHCI_INTR_MASK];
+        xhciR3SetIntrPending(pDevIns, pThis, pIntr);
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "genportchgevt"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating port change event...\n");
+        int iPort = 0;
+        xhciR3GenPortChgEvent(pDevIns, pThis, IDX_TO_ID(iPort));
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "genmfiwrapevt"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating MF Index wrap event...\n");
+        XHCI_EVENT_TRB  ed;
+        RT_ZERO(ed);
+        ed.mwe.cc   = XHCI_TCC_SUCCESS;
+        ed.mwe.type = XHCI_TRB_MFIDX_WRAP;
+        xhciR3WriteEvent(pDevIns, pThis, &ed, XHCI_PRIMARY_INTERRUPTER, false);
+        return;
+    }
+
+    if (pszArgs && strstr(pszArgs, "gendoorbell"))
+    {
+        pHlp->pfnPrintf(pHlp, "Generating doorbell ring..\n");
+        xhciKickWorker(pDevIns, pThis, XHCI_JOB_DOORBELL, 0);
+        return;
+    }
 #endif
 
@@ -7732 +7795 @@
                 pHlp->pfnPrintf(pHlp, "  Speed:%u Entries:%u RhPort:%u", ctxSlot.speed, ctxSlot.ctx_ent, ctxSlot.rh_port);
                 pHlp->pfnPrintf(pHlp, " Address:%u State:%s \n", ctxSlot.dev_addr, pcszDesc);
+                pHlp->pfnPrintf(pHlp, "  Doorbells:%08X\n", pThis->aBellsRung[ID_TO_IDX(uSlotID)]);
 
                 /* Endpoint contexts. */