Index: /trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp =================================================================== --- /trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp (revision 83586) +++ /trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp (revision 83587) @@ -1427,5 +1427,9 @@ uint16_t cSegsAllocated = VIRTIONET_PREALLOCATE_RX_SEG_COUNT; - PRTSGBUF pVirtSegBufToGuest = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF)); + /** @todo r=bird: error codepaths below are almost all leaky! Maybe keep + * allocations and cleanup here and put the code doing the complicated + * work into a helper that can AssertReturn at will without needing to + * care about cleaning stuff up. */ + PRTSGBUF pVirtSegBufToGuest = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF)); /** @todo r=bird: Missing check. */ PRTSGSEG paVirtSegsToGuest = (PRTSGSEG)RTMemAllocZ(sizeof(RTSGSEG) * cSegsAllocated); AssertReturn(paVirtSegsToGuest, VERR_NO_MEMORY); @@ -1438,20 +1442,22 @@ for (cDescs = uOffset = 0; uOffset < cb; ) { - PVIRTIO_DESC_CHAIN_T pDescChain; + PVIRTIO_DESC_CHAIN_T pDescChain = NULL; int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, RXQIDX_QPAIR(idxQueue), &pDescChain, true); - AssertRC(rc == VINF_SUCCESS || rc == VERR_NOT_AVAILABLE); + Assert(rc == VINF_SUCCESS || rc == VERR_NOT_AVAILABLE, ("%Rrc\n", rc)); /** @todo Find a better way to deal with this */ - AssertMsgReturn(rc == VINF_SUCCESS && pDescChain->cbPhysReturn, - ("Not enough Rx buffers in queue to accomodate ethernet packet\n"), - VERR_INTERNAL_ERROR); + AssertMsgReturnStmt(rc == VINF_SUCCESS && pDescChain->cbPhysReturn, + ("Not enough Rx buffers in queue to accomodate ethernet packet\n"), + virtioCoreR3DescChainRelease(pDescChain), + VERR_INTERNAL_ERROR); /* Unlikely that len of 1st seg of guest Rx (IN) buf is less than sizeof(virtio_net_pkt_hdr) == 12. * Assert it to reduce complexity. Robust solution would entail finding seg idx and offset of * virtio_net_header.num_buffers (to update field *after* hdr & pkts copied to gcPhys) */ - AssertMsgReturn(pDescChain->pSgPhysReturn->paSegs[0].cbSeg >= sizeof(VIRTIONET_PKT_HDR_T), - ("Desc chain's first seg has insufficient space for pkt header!\n"), - VERR_INTERNAL_ERROR); + AssertMsgReturnStmt(pDescChain->pSgPhysReturn->paSegs[0].cbSeg >= sizeof(VIRTIONET_PKT_HDR_T), + ("Desc chain's first seg has insufficient space for pkt header!\n"), + virtioCoreR3DescChainRelease(pDescChain), + VERR_INTERNAL_ERROR); uint32_t cbDescChainLeft = pDescChain->cbPhysReturn; @@ -1500,4 +1506,6 @@ break; } + + virtioCoreR3DescChainRelease(pDescChain); } @@ -2063,12 +2071,13 @@ int rc; - PVIRTIO_DESC_CHAIN_T pDescChain; + PVIRTIO_DESC_CHAIN_T pDescChain = NULL; while ((rc = virtioCoreR3QueuePeek(pVirtio->pDevIns, pVirtio, idxQueue, &pDescChain)) == VINF_SUCCESS) { - if (RT_SUCCESS(rc)) + if (RT_SUCCESS(rc)) /** @todo r=bird: pointless, see loop condition. */ Log10Func(("%s fetched descriptor chain from %s\n", INSTANCE(pThis), VIRTQNAME(idxQueue))); else { LogFunc(("%s failed to find expected data on %s, rc = %Rrc\n", INSTANCE(pThis), VIRTQNAME(idxQueue), rc)); + virtioCoreR3DescChainRelease(pDescChain); break; } @@ -2096,5 +2105,6 @@ if (pThisCC->pDrv) { - PDMNETWORKGSO Gso, *pGso = virtioNetR3SetupGsoCtx(&Gso, &PktHdr); + PDMNETWORKGSO Gso; + PPDMNETWORKGSO pGso = virtioNetR3SetupGsoCtx(&Gso, &PktHdr); /** @todo Optimize away the extra copying! (lazy bird) */ @@ -2141,4 +2151,5 @@ Log4Func(("Failed to allocate S/G buffer: size=%u rc=%Rrc\n", uSize, rc)); /* Stop trying to fetch TX descriptors until we get more bandwidth. */ + virtioCoreR3DescChainRelease(pDescChain); break; } @@ -2152,4 +2163,7 @@ virtioCoreQueueSync(pVirtio->pDevIns, pVirtio, idxQueue); } + + virtioCoreR3DescChainRelease(pDescChain); + pDescChain = NULL; } virtioNetR3SetWriteLed(pThisCC, false); @@ -2266,5 +2280,5 @@ { Log10Func(("%s fetching next descriptor chain from %s\n", INSTANCE(pThis), VIRTQNAME(idxQueue))); - PVIRTIO_DESC_CHAIN_T pDescChain; + PVIRTIO_DESC_CHAIN_T pDescChain = NULL; int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, idxQueue, &pDescChain, true); if (rc == VERR_NOT_AVAILABLE) @@ -2274,4 +2288,5 @@ } virtioNetR3Ctrl(pDevIns, pThis, pThisCC, pDescChain); + virtioCoreR3DescChainRelease(pDescChain); } else if (IS_TX_QUEUE(idxQueue)) Index: /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp =================================================================== --- /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 83586) +++ /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 83587) @@ -717,5 +717,5 @@ } - PVIRTIO_DESC_CHAIN_T pDescChain; + PVIRTIO_DESC_CHAIN_T pDescChain = NULL; int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, EVENTQ_IDX, &pDescChain, true); AssertRCReturn(rc, rc); @@ -742,4 +742,5 @@ virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, EVENTQ_IDX, &ReqSgBuf, pDescChain, true /*fFence*/); virtioCoreQueueSync(pDevIns, &pThis->Virtio, EVENTQ_IDX); + virtioCoreR3DescChainRelease(pDescChain); return VINF_SUCCESS; @@ -751,4 +752,6 @@ RTMemFree(pReq->pbSense); pReq->pbSense = NULL; + virtioCoreR3DescChainRelease(pReq->pDescChain); + pReq->pDescChain = NULL; pTarget->pDrvMediaEx->pfnIoReqFree(pTarget->pDrvMediaEx, pReq->hIoReq); } @@ -1257,4 +1260,5 @@ pReq->cbDataOut = cbDataOut; pReq->pDescChain = pDescChain; + virtioCoreR3DescChainRetain(pDescChain); /* (For pReq->pDescChain. Released by virtioScsiR3FreeReq.) */ pReq->uDataInOff = offDataIn; pReq->uDataOutOff = offDataOut; @@ -1550,19 +1554,21 @@ pWorkerR3->auRedoDescs[i], &pDescChain); if (RT_FAILURE(rc)) - LogRel(("Error fetching desc chain to redo, %Rrc", rc)); + LogRel(("Error fetching desc chain to redo, %Rrc", rc)); rc = virtioScsiR3ReqSubmit(pDevIns, pThis, pThisCC, qIdx, pDescChain); if (RT_FAILURE(rc)) - LogRel(("Error submitting req packet, resetting %Rrc", rc)); + LogRel(("Error submitting req packet, resetting %Rrc", rc)); + + virtioCoreR3DescChainRelease(pDescChain); } pWorkerR3->cRedoDescs = 0; Log6Func(("fetching next descriptor chain from %s\n", VIRTQNAME(qIdx))); - PVIRTIO_DESC_CHAIN_T pDescChain; + PVIRTIO_DESC_CHAIN_T pDescChain = NULL; int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, qIdx, &pDescChain, true); if (rc == VERR_NOT_AVAILABLE) { - Log6Func(("Nothing found in %s\n", VIRTQNAME(qIdx))); - continue; + Log6Func(("Nothing found in %s\n", VIRTQNAME(qIdx))); + continue; } @@ -1572,8 +1578,10 @@ else /* request queue index */ { - rc = virtioScsiR3ReqSubmit(pDevIns, pThis, pThisCC, qIdx, pDescChain); - if (RT_FAILURE(rc)) - LogRel(("Error submitting req packet, resetting %Rrc", rc)); + rc = virtioScsiR3ReqSubmit(pDevIns, pThis, pThisCC, qIdx, pDescChain); + if (RT_FAILURE(rc)) + LogRel(("Error submitting req packet, resetting %Rrc", rc)); } + + virtioCoreR3DescChainRelease(pDescChain); } } Index: /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp =================================================================== --- /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp (revision 83586) +++ /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp (revision 83587) @@ -602,15 +602,10 @@ uint16_t uHeadIdx, PPVIRTIO_DESC_CHAIN_T ppDescChain) { - AssertReturn(ppDescChain, VERR_INVALID_PARAMETER); + AssertReturn(ppDescChain, VERR_INVALID_POINTER); + *ppDescChain = NULL; Assert(idxQueue < RT_ELEMENTS(pVirtio->virtqState)); - PVIRTQSTATE pVirtq = &pVirtio->virtqState[idxQueue]; - - PVIRTIOSGSEG paSegsIn = (PVIRTIOSGSEG)RTMemAlloc(VIRTQ_MAX_SIZE * sizeof(VIRTIOSGSEG)); - AssertReturn(paSegsIn, VERR_NO_MEMORY); - - PVIRTIOSGSEG paSegsOut = (PVIRTIOSGSEG)RTMemAlloc(VIRTQ_MAX_SIZE * sizeof(VIRTIOSGSEG)); - AssertReturn(paSegsOut, VERR_NO_MEMORY); + PVIRTQSTATE pVirtq = &pVirtio->virtqState[idxQueue]; AssertMsgReturn(IS_DRIVER_OK(pVirtio) && pVirtio->uQueueEnable[idxQueue], @@ -622,7 +617,25 @@ RT_NOREF(pVirtq); + /* + * Allocate and initialize the descriptor chain structure. + */ + PVIRTIO_DESC_CHAIN_T pDescChain = (PVIRTIO_DESC_CHAIN_T)RTMemAllocZ(sizeof(VIRTIO_DESC_CHAIN_T)); + AssertReturn(pDescChain, VERR_NO_MEMORY); + pDescChain->u32Magic = VIRTIO_DESC_CHAIN_MAGIC; + pDescChain->cRefs = 1; + pDescChain->uHeadIdx = uHeadIdx; + *ppDescChain = pDescChain; + + /* + * Gather segments. + */ VIRTQ_DESC_T desc; - uint32_t cbIn = 0, cbOut = 0, cSegsIn = 0, cSegsOut = 0; + uint32_t cbIn = 0; + uint32_t cbOut = 0; + uint32_t cSegsIn = 0; + uint32_t cSegsOut = 0; + PVIRTIOSGSEG paSegsIn = pDescChain->aSegsIn; + PVIRTIOSGSEG paSegsOut = pDescChain->aSegsOut; do @@ -657,5 +670,5 @@ Log6Func(("%s IN desc_idx=%u seg=%u addr=%RGp cb=%u\n", VIRTQNAME(pVirtio, idxQueue), uDescIdx, cSegsIn, desc.GCPhysBuf, desc.cb)); cbIn += desc.cb; - pSeg = &(paSegsIn[cSegsIn++]); + pSeg = &paSegsIn[cSegsIn++]; } else @@ -663,5 +676,5 @@ Log6Func(("%s OUT desc_idx=%u seg=%u addr=%RGp cb=%u\n", VIRTQNAME(pVirtio, idxQueue), uDescIdx, cSegsOut, desc.GCPhysBuf, desc.cb)); cbOut += desc.cb; - pSeg = &(paSegsOut[cSegsOut++]); + pSeg = &paSegsOut[cSegsOut++]; } @@ -672,17 +685,11 @@ } while (desc.fFlags & VIRTQ_DESC_F_NEXT); - PVIRTIO_DESC_CHAIN_T pDescChain = (PVIRTIO_DESC_CHAIN_T)RTMemAllocZ(sizeof(VIRTIO_DESC_CHAIN_T)); - AssertReturn(pDescChain, VERR_NO_MEMORY); - - pDescChain->uHeadIdx = uHeadIdx; - *ppDescChain = pDescChain; - + /* + * Add segments to the descriptor chain structure. + */ if (cSegsIn) { - PVIRTIOSGBUF pSgPhysIn = (PVIRTIOSGBUF)RTMemAllocZ(sizeof(VIRTIOSGBUF)); - AssertReturn(pSgPhysIn, VERR_NO_MEMORY); - - virtioCoreSgBufInit(pSgPhysIn, paSegsIn, cSegsIn); - pDescChain->pSgPhysReturn = pSgPhysIn; + virtioCoreSgBufInit(&pDescChain->SgBufIn, paSegsIn, cSegsIn); + pDescChain->pSgPhysReturn = &pDescChain->SgBufIn; pDescChain->cbPhysReturn = cbIn; } @@ -690,17 +697,57 @@ if (cSegsOut) { - PVIRTIOSGBUF pSgPhysOut = (PVIRTIOSGBUF)RTMemAllocZ(sizeof(VIRTIOSGBUF)); - AssertReturn(pSgPhysOut, VERR_NO_MEMORY); - - virtioCoreSgBufInit(pSgPhysOut, paSegsOut, cSegsOut); - pDescChain->pSgPhysSend = pSgPhysOut; + virtioCoreSgBufInit(&pDescChain->SgBufOut, paSegsOut, cSegsOut); + pDescChain->pSgPhysSend = &pDescChain->SgBufOut; pDescChain->cbPhysSend = cbOut; } - Log6Func(("%s -- segs OUT: %u (%u bytes) IN: %u (%u bytes) --\n", pVirtq->szVirtqName, cSegsOut, cbOut, cSegsIn, cbIn)); return VINF_SUCCESS; } + + +/** + * Retains a reference to the given descriptor chain. + * + * @returns New reference count. + * @retval UINT32_MAX on invalid parameter. + * @param pDescChain The descriptor chain to reference. + */ +uint32_t virtioCoreR3DescChainRetain(PVIRTIO_DESC_CHAIN_T pDescChain) +{ + AssertReturn(pDescChain, UINT32_MAX); + AssertReturn(pDescChain->u32Magic == VIRTIO_DESC_CHAIN_MAGIC, UINT32_MAX); + uint32_t cRefs = ASMAtomicIncU32(&pDescChain->cRefs); + Assert(cRefs > 1); + Assert(cRefs < 16); + return cRefs; +} + + +/** + * Releases a reference to the given descriptor chain. + * + * @returns New reference count. + * @retval 0 if freed or invalid parameter. + * @param pDescChain The descriptor chain to reference. NULL is quietly + * ignored (returns 0). + */ +uint32_t virtioCoreR3DescChainRelease(PVIRTIO_DESC_CHAIN_T pDescChain) +{ + if (!pDescChain) + return 0; + AssertReturn(pDescChain, 0); + AssertReturn(pDescChain->u32Magic == VIRTIO_DESC_CHAIN_MAGIC, 0); + uint32_t cRefs = ASMAtomicDecU32(&pDescChain->cRefs); + Assert(cRefs < 16); + if (cRefs == 0) + { + pDescChain->u32Magic = ~VIRTIO_DESC_CHAIN_MAGIC; + RTMemFree(pDescChain); + } + return cRefs; +} + /* @@ -839,4 +886,6 @@ * @param ppDescChain Address to store pointer to descriptor chain that contains the * pre-processed transaction information pulled from the virtq. + * Returned reference must be released by calling + * virtioCoreR3DescChainRelease(). * @param fRemove flags whether to remove desc chain from queue (false = peek) * @@ -893,4 +942,7 @@ * @retval VERR_INVALID_STATE VirtIO not in ready state * @retval VERR_NOT_AVAILABLE Queue is empty + * + * @note This function will not release any reference to pDescChain. The + * caller must take care of that. */ int virtioCoreR3QueuePut(PPDMDEVINS pDevIns, PVIRTIOCORE pVirtio, uint16_t idxQueue, PRTSGBUF pSgVirtReturn, @@ -900,4 +952,7 @@ PVIRTQSTATE pVirtq = &pVirtio->virtqState[idxQueue]; PVIRTIOSGBUF pSgPhysReturn = pDescChain->pSgPhysReturn; + + Assert(pDescChain->u32Magic == VIRTIO_DESC_CHAIN_MAGIC); + Assert(pDescChain->cRefs > 0); AssertMsgReturn(IS_DRIVER_OK(pVirtio) /*&& pVirtio->uQueueEnable[idxQueue]*/, @@ -951,16 +1006,4 @@ Log6Func(("Write ahead used_idx=%u, %s used_idx=%u\n", pVirtq->uUsedIdx, VIRTQNAME(pVirtio, idxQueue), virtioReadUsedRingIdx(pDevIns, pVirtio, idxQueue))); - - if (pDescChain->pSgPhysSend) - { - RTMemFree(pDescChain->pSgPhysSend->paSegs); - RTMemFree(pDescChain->pSgPhysSend); - } - if (pDescChain->pSgPhysReturn) - { - RTMemFree(pSgPhysReturn->paSegs); - RTMemFree(pSgPhysReturn); - } - RTMemFree(pDescChain); return VINF_SUCCESS; Index: /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h =================================================================== --- /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h (revision 83586) +++ /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h (revision 83587) @@ -93,12 +93,31 @@ typedef PVIRTIOSGBUF *PPVIRTIOSGBUF; +/** + * Virtio descriptor chain representation. + */ typedef struct VIRTIO_DESC_CHAIN { - uint32_t uHeadIdx; /**< Head idx of associated desc chain */ - uint32_t cbPhysSend; /**< Total size of src buffer */ - PVIRTIOSGBUF pSgPhysSend; /**< Phys S/G/ buf for data from guest */ - uint32_t cbPhysReturn; /**< Total size of dst buffer */ - PVIRTIOSGBUF pSgPhysReturn; /**< Phys S/G buf to store result for guest */ -} VIRTIO_DESC_CHAIN_T, *PVIRTIO_DESC_CHAIN_T, **PPVIRTIO_DESC_CHAIN_T; + uint32_t u32Magic; /**< Magic value, VIRTIO_DESC_CHAIN_MAGIC. */ + uint32_t volatile cRefs; /**< Reference counter. */ + uint32_t uHeadIdx; /**< Head idx of associated desc chain */ + uint32_t cbPhysSend; /**< Total size of src buffer */ + PVIRTIOSGBUF pSgPhysSend; /**< Phys S/G/ buf for data from guest */ + uint32_t cbPhysReturn; /**< Total size of dst buffer */ + PVIRTIOSGBUF pSgPhysReturn; /**< Phys S/G buf to store result for guest */ + + /** @name Internal (bird combined 5 allocations into a single), fingers off. + * @{ */ + VIRTIOSGBUF SgBufIn; + VIRTIOSGBUF SgBufOut; + VIRTIOSGSEG aSegsIn[VIRTQ_MAX_SIZE]; + VIRTIOSGSEG aSegsOut[VIRTQ_MAX_SIZE]; + /** @} */ +} VIRTIO_DESC_CHAIN_T; +/** Pointer to a Virtio descriptor chain. */ +typedef VIRTIO_DESC_CHAIN_T *PVIRTIO_DESC_CHAIN_T; +/** Pointer to a Virtio descriptor chain pointer. */ +typedef VIRTIO_DESC_CHAIN_T **PPVIRTIO_DESC_CHAIN_T; +/** Magic value for VIRTIO_DESC_CHAIN_T::u32Magic. */ +#define VIRTIO_DESC_CHAIN_MAGIC UINT32_C(0x19600219) typedef struct VIRTIOPCIPARAMS @@ -377,4 +396,6 @@ int virtioCoreR3DescChainGet(PPDMDEVINS pDevIns, PVIRTIOCORE pVirtio, uint16_t idxQueue, uint16_t uHeadIdx, PPVIRTIO_DESC_CHAIN_T ppDescChain); +uint32_t virtioCoreR3DescChainRetain(PVIRTIO_DESC_CHAIN_T pDescChain); +uint32_t virtioCoreR3DescChainRelease(PVIRTIO_DESC_CHAIN_T pDescChain); int virtioCoreR3QueuePeek(PPDMDEVINS pDevIns, PVIRTIOCORE pVirtio, uint16_t idxQueue,