Changeset 98661 in vbox for trunk/src/VBox/Devices/Storage

Timestamp:

Feb 20, 2023 4:31:14 PM (2 years ago)

Author:

vboxsync

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

155961

Message:

Devices/DevVirtioSCSI: Possible fix for bugref:9440#c164, the queue head can get out of sync if two threads complete I/O requests concurrently

File:

• trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (modified) (8 diffs)

trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp

@@ -368 +368 @@
 {
     R3PTRTYPE(PPDMTHREAD)           pThread;                    /**< pointer to worker thread's handle                 */
-    uint16_t                        auRedoDescs[VIRTQ_SIZE];/**< List of previously suspended reqs to re-submit    */
+    RTCRITSECT                      CritSectVirtq;              /**< Protecting the virtq against concurrent thread access. */
+    uint16_t                        auRedoDescs[VIRTQ_SIZE];    /**< List of previously suspended reqs to re-submit    */
     uint16_t                        cRedoDescs;                 /**< Number of redo desc chain head desc idxes in list */
 } VIRTIOSCSIWORKERR3;
@@ -715 +716 @@
 
 #endif /* LOG_ENABLED */
+
+
+/**
+ * Wrapper around virtioCoreR3VirtqUsedBufPut() and virtioCoreVirtqUsedRingSync() doing some device locking.
+ *
+ * @returns nothing.
+ * @param   pDevIns         The PDM device instance.
+ * @param   pVirtio         Pointer to the shared virtio core structure.
+ * @param   uVirtqNbr       The virtq number.
+ * @param   pSgVirtReturn   The S/G buffer to return data from.
+ * @param   pVirtqBuf       The virtq buffer.
+ *
+ * @note This is a temporary fix, see @bugref{9440#c164} on why it is necessary. Should be moved to VirtioCore or
+ *       VirtioCore should be changed to not require locking.
+ */
+DECLINLINE(void) virtioScsiR3VirtqUsedBufPutAndSync(PPDMDEVINS pDevIns, PVIRTIOCORE pVirtio, uint16_t uVirtqNbr, PRTSGBUF pSgVirtReturn,
+                                                    PVIRTQBUF pVirtqBuf)
+{
+    PVIRTIOSCSICC pThisCC = PDMDEVINS_2_DATA_CC(pDevIns, PVIRTIOSCSICC);
+
+    RTCritSectEnter(&pThisCC->aWorkers[uVirtqNbr].CritSectVirtq);
+    virtioCoreR3VirtqUsedBufPut(pThisCC->pDevIns, pVirtio, uVirtqNbr, pSgVirtReturn, pVirtqBuf, true /* fFence */);
+    virtioCoreVirtqUsedRingSync(pThisCC->pDevIns, pVirtio, uVirtqNbr);
+    RTCritSectLeave(&pThisCC->aWorkers[uVirtqNbr].CritSectVirtq);
+}
 
 
@@ -793 +819 @@
     RTSgBufInit(&ReqSgBuf, aReqSegs, RT_ELEMENTS(aReqSegs));
 
-    rc = virtioCoreR3VirtqUsedBufPut(pDevIns, &pThis->Virtio, EVENTQ_IDX, &ReqSgBuf, pVirtqBuf, true /*fFence*/);
-    if (rc == VINF_SUCCESS)
-        virtioCoreVirtqUsedRingSync(pDevIns, &pThis->Virtio, EVENTQ_IDX, false);
-    else
-        LogRel(("Error writing control message to guest\n"));
+    virtioScsiR3VirtqUsedBufPutAndSync(pDevIns, &pThis->Virtio, EVENTQ_IDX, &ReqSgBuf, pVirtqBuf);
     virtioCoreR3VirtqBufRelease(&pThis->Virtio, pVirtqBuf);
 
@@ -889 +911 @@
         pRespHdr->uResponse = VIRTIOSCSI_S_RESET;
 
-    virtioCoreR3VirtqUsedBufPut(pDevIns, &pThis->Virtio, uVirtqNbr, &ReqSgBuf, pVirtqBuf, true /* fFence */);
-    virtioCoreVirtqUsedRingSync(pDevIns, &pThis->Virtio, uVirtqNbr);
-
+    virtioScsiR3VirtqUsedBufPutAndSync(pDevIns, &pThis->Virtio, uVirtqNbr, &ReqSgBuf, pVirtqBuf);
     Log2(("---------------------------------------------------------------------------------\n"));
 
@@ -1074 +1094 @@
                         VERR_BUFFER_OVERFLOW);
 
-        virtioCoreR3VirtqUsedBufPut(pDevIns, &pThis->Virtio, pReq->uVirtqNbr, &ReqSgBuf, pReq->pVirtqBuf, true /* fFence TBD */);
-        virtioCoreVirtqUsedRingSync(pDevIns, &pThis->Virtio, pReq->uVirtqNbr);
-
+        virtioScsiR3VirtqUsedBufPutAndSync(pDevIns, &pThis->Virtio, pReq->uVirtqNbr, &ReqSgBuf, pReq->pVirtqBuf);
         Log2(("-----------------------------------------------------------------------------------------\n"));
     }
@@ -1560 +1578 @@
     RTSgBufInit(&ReqSgBuf, aReqSegs, cSegs);
 
-    virtioCoreR3VirtqUsedBufPut(pDevIns, &pThis->Virtio, uVirtqNbr, &ReqSgBuf, pVirtqBuf, true /*fFence*/);
-    virtioCoreVirtqUsedRingSync(pDevIns, &pThis->Virtio, uVirtqNbr);
-
+    virtioScsiR3VirtqUsedBufPutAndSync(pDevIns, &pThis->Virtio, uVirtqNbr, &ReqSgBuf, pVirtqBuf);
     return VINF_SUCCESS;
 }
@@ -2460 +2476 @@
            pThisCC->aWorkers[uVirtqNbr].pThread = NULL;
         }
+
+        if (RTCritSectIsInitialized(&pThisCC->aWorkers[uVirtqNbr].CritSectVirtq))
+            RTCritSectDelete(&pThisCC->aWorkers[uVirtqNbr].CritSectVirtq);
     }
 
@@ -2574 +2593 @@
         }
         pThis->afVirtqAttached[uVirtqNbr] = true;
+        rc = RTCritSectInit(&pThisCC->aWorkers[uVirtqNbr].CritSectVirtq);
+        if (RT_FAILURE(rc))
+            return PDMDevHlpVMSetError(pDevIns, rc, RT_SRC_POS,
+                                       N_("DevVirtioSCSI: Failed to create worker critical section"));
     }