Index: /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp =================================================================== --- /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 83570) +++ /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 83571) @@ -737,5 +737,5 @@ RTSgBufInit(pReqSegBuf, paReqSegs, 1); - virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, EVENTQ_IDX, pReqSegBuf, pDescChain, true); + virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, EVENTQ_IDX, pReqSegBuf, pDescChain, true /*fFence*/); virtioCoreQueueSync(pDevIns, &pThis->Virtio, EVENTQ_IDX); @@ -1319,7 +1319,4 @@ uint16_t qIdx, PVIRTIO_DESC_CHAIN_T pDescChain) { - uint8_t bResponse = VIRTIOSCSI_S_OK; - uint8_t cSegs = 0; - AssertReturn(pDescChain->cbPhysSend >= RT_MIN(sizeof(VIRTIOSCSI_CTRL_AN_T), sizeof(VIRTIOSCSI_CTRL_TMF_T)), 0); @@ -1328,39 +1325,39 @@ * Allocate buffer and read in the control command */ - PVIRTIO_SCSI_CTRL_UNION_T pScsiCtrlUnion = (PVIRTIO_SCSI_CTRL_UNION_T)RTMemAllocZ(sizeof(VIRTIO_SCSI_CTRL_UNION_T)); - AssertPtrReturn(pScsiCtrlUnion, VERR_NO_MEMORY /*ignored*/); - - uint8_t *pb = pScsiCtrlUnion->ab; - for (size_t cb = RT_MIN(pDescChain->cbPhysSend, sizeof(VIRTIO_SCSI_CTRL_UNION_T)); cb; ) - { - size_t cbSeg = cb; + VIRTIO_SCSI_CTRL_UNION_T ScsiCtrlUnion; + RT_ZERO(ScsiCtrlUnion); + + size_t const cbCtrl = RT_MIN(pDescChain->cbPhysSend, sizeof(VIRTIO_SCSI_CTRL_UNION_T)); + for (size_t offCtrl = 0; offCtrl < cbCtrl; ) + { + size_t cbSeg = cbCtrl - offCtrl; RTGCPHYS GCPhys = virtioCoreSgBufGetNextSegment(pDescChain->pSgPhysSend, &cbSeg); - PDMDevHlpPCIPhysRead(pDevIns, GCPhys, pb, cbSeg); - pb += cbSeg; - cb -= cbSeg; - } - - /** @todo r=bird: Memory leak here. */ - AssertReturn( (pScsiCtrlUnion->scsiCtrl.uType == VIRTIOSCSI_T_TMF + PDMDevHlpPCIPhysRead(pDevIns, GCPhys, &ScsiCtrlUnion.ab[offCtrl], cbSeg); + offCtrl += cbSeg; + } + + AssertReturn( (ScsiCtrlUnion.scsiCtrl.uType == VIRTIOSCSI_T_TMF && pDescChain->cbPhysSend >= sizeof(VIRTIOSCSI_CTRL_TMF_T)) - || ( ( pScsiCtrlUnion->scsiCtrl.uType == VIRTIOSCSI_T_AN_QUERY - || pScsiCtrlUnion->scsiCtrl.uType == VIRTIOSCSI_T_AN_SUBSCRIBE) - && pDescChain->cbPhysSend >= sizeof(VIRTIOSCSI_CTRL_AN_T)), 0); - - /** @todo r=bird: This segment handling is unnecessary. A stack array should - * suffice. The stack variable w/ initializer + memcpy into paReqSegs - * done in the switch below is also weird + suboptimal. */ - PRTSGSEG paReqSegs = (PRTSGSEG)RTMemAllocZ(sizeof(RTSGSEG) * 2); - AssertReturn(paReqSegs, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here. */ - - switch (pScsiCtrlUnion->scsiCtrl.uType) + || ( ( ScsiCtrlUnion.scsiCtrl.uType == VIRTIOSCSI_T_AN_QUERY + || ScsiCtrlUnion.scsiCtrl.uType == VIRTIOSCSI_T_AN_SUBSCRIBE) + && pDescChain->cbPhysSend >= sizeof(VIRTIOSCSI_CTRL_AN_T)), + 0 /** @todo r=bird: what kind of status is '0' here? */); + + union + { + uint32_t fSupportedEvents; + } uData; + uint8_t bResponse = VIRTIOSCSI_S_OK; + uint8_t cSegs; + RTSGSEG aReqSegs[2]; + switch (ScsiCtrlUnion.scsiCtrl.uType) { case VIRTIOSCSI_T_TMF: /* Task Management Functions */ { - uint8_t uTarget = pScsiCtrlUnion->scsiCtrlTmf.abScsiLun[1]; - uint32_t uScsiLun = (pScsiCtrlUnion->scsiCtrlTmf.abScsiLun[2] << 8 - | pScsiCtrlUnion->scsiCtrlTmf.abScsiLun[3]) & 0x3fff; + uint8_t uTarget = ScsiCtrlUnion.scsiCtrlTmf.abScsiLun[1]; + uint32_t uScsiLun = RT_MAKE_U16(ScsiCtrlUnion.scsiCtrlTmf.abScsiLun[3], + ScsiCtrlUnion.scsiCtrlTmf.abScsiLun[2]) & 0x3fff; Log2Func(("[%s] (Target: %d LUN: %d) Task Mgt Function: %s\n", - VIRTQNAME(qIdx), uTarget, uScsiLun, virtioGetTMFTypeText(pScsiCtrlUnion->scsiCtrlTmf.uSubtype))); + VIRTQNAME(qIdx), uTarget, uScsiLun, virtioGetTMFTypeText(ScsiCtrlUnion.scsiCtrlTmf.uSubtype))); if (uTarget >= pThis->cTargets || !pThisCC->paTargetInstances[uTarget].fPresent) @@ -1370,5 +1367,5 @@ bResponse = VIRTIOSCSI_S_INCORRECT_LUN; else - switch (pScsiCtrlUnion->scsiCtrlTmf.uSubtype) + switch (ScsiCtrlUnion.scsiCtrlTmf.uSubtype) { case VIRTIOSCSI_T_TMF_ABORT_TASK: @@ -1400,17 +1397,13 @@ bResponse = VIRTIOSCSI_S_FAILURE; } - - RTSGSEG aSegs[] = { { &bResponse, sizeof(bResponse) } }; - memcpy(paReqSegs, aSegs, sizeof(aSegs)); - cSegs = RT_ELEMENTS(aSegs); + cSegs = 0; /* only bResponse */ break; } case VIRTIOSCSI_T_AN_QUERY: /* Guest SCSI driver is querying supported async event notifications */ { - - PVIRTIOSCSI_CTRL_AN_T pScsiCtrlAnQuery = &pScsiCtrlUnion->scsiCtrlAsyncNotify; + PVIRTIOSCSI_CTRL_AN_T const pScsiCtrlAnQuery = &ScsiCtrlUnion.scsiCtrlAsyncNotify; uint8_t uTarget = pScsiCtrlAnQuery->abScsiLun[1]; - uint32_t uScsiLun = (pScsiCtrlAnQuery->abScsiLun[2] << 8 | pScsiCtrlAnQuery->abScsiLun[3]) & 0x3fff; + uint32_t uScsiLun = RT_MAKE_U16(pScsiCtrlAnQuery->abScsiLun[3], pScsiCtrlAnQuery->abScsiLun[2]) & 0x3fff; if (uTarget >= pThis->cTargets || !pThisCC->paTargetInstances[uTarget].fPresent) @@ -1431,14 +1424,13 @@ } #endif - uint32_t fSupportedEvents = SUPPORTED_EVENTS; - RTSGSEG aSegs[] = { { &fSupportedEvents, sizeof(fSupportedEvents) }, - { &bResponse, sizeof(bResponse) } }; - memcpy(paReqSegs, aSegs, sizeof(aSegs)); - cSegs = RT_ELEMENTS(aSegs); + uData.fSupportedEvents = SUPPORTED_EVENTS; + aReqSegs[0].pvSeg = &uData.fSupportedEvents; + aReqSegs[0].cbSeg = sizeof(uData.fSupportedEvents); + cSegs = 1; break; } case VIRTIOSCSI_T_AN_SUBSCRIBE: /* Guest SCSI driver is subscribing to async event notification(s) */ { - PVIRTIOSCSI_CTRL_AN_T pScsiCtrlAnSubscribe = &pScsiCtrlUnion->scsiCtrlAsyncNotify; + PVIRTIOSCSI_CTRL_AN_T const pScsiCtrlAnSubscribe = &ScsiCtrlUnion.scsiCtrlAsyncNotify; if (pScsiCtrlAnSubscribe->fEventsRequested & ~SUBSCRIBABLE_EVENTS) @@ -1447,6 +1439,5 @@ uint8_t uTarget = pScsiCtrlAnSubscribe->abScsiLun[1]; - uint32_t uScsiLun = (pScsiCtrlAnSubscribe->abScsiLun[2] << 8 - | pScsiCtrlAnSubscribe->abScsiLun[3]) & 0x3fff; + uint32_t uScsiLun = RT_MAKE_U16(pScsiCtrlAnSubscribe->abScsiLun[3], pScsiCtrlAnSubscribe->abScsiLun[2]) & 0x3fff; #ifdef LOG_ENABLED @@ -1470,47 +1461,32 @@ } - RTSGSEG aSegs[] = { { &pThis->fAsyncEvtsEnabled, sizeof(pThis->fAsyncEvtsEnabled) }, - { &bResponse, sizeof(bResponse) } }; - memcpy(paReqSegs, aSegs, sizeof(aSegs)); - cSegs = RT_ELEMENTS(aSegs); + aReqSegs[0].pvSeg = &pThis->fAsyncEvtsEnabled; + aReqSegs[0].cbSeg = sizeof(pThis->fAsyncEvtsEnabled); + cSegs = 1; break; } default: { - LogFunc(("Unknown control type extracted from %s: %u\n", VIRTQNAME(qIdx), pScsiCtrlUnion->scsiCtrl.uType)); + LogFunc(("Unknown control type extracted from %s: %u\n", VIRTQNAME(qIdx), ScsiCtrlUnion.scsiCtrl.uType)); bResponse = VIRTIOSCSI_S_FAILURE; - RTSGSEG aSegs[] = { { &bResponse, sizeof(bResponse) } }; - memcpy(paReqSegs, aSegs, sizeof(aSegs)); - cSegs = RT_ELEMENTS(aSegs); + cSegs = 0; /* only bResponse */ + break; } } + + /* Add the response code: */ + aReqSegs[cSegs].pvSeg = &bResponse; + aReqSegs[cSegs].cbSeg = sizeof(bResponse); + cSegs++; + Assert(cSegs <= RT_ELEMENTS(aReqSegs)); + LogFunc(("Response code: %s\n", virtioGetReqRespText(bResponse))); - PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF)); - AssertReturn(pReqSegBuf, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here. */ - - /* Copy segment data to malloc'd memory to avoid stack out-of-scope errors sanitizer doesn't detect */ - /** @todo r=bird: The above comment makes zero sense as the memory is freed - * before we return, so there cannot be any trouble with out-of-scope - * stuff here. */ - for (int i = 0; i < cSegs; i++) - { - void *pv = paReqSegs[i].pvSeg; - paReqSegs[i].pvSeg = RTMemDup(pv, paReqSegs[i].cbSeg); - AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here. */ - } - - RTSgBufInit(pReqSegBuf, paReqSegs, cSegs); - - virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, qIdx, pReqSegBuf, pDescChain, true); + RTSGBUF ReqSgBuf; + RTSgBufInit(&ReqSgBuf, aReqSegs, cSegs); + + virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, qIdx, &ReqSgBuf, pDescChain, true /*fFence*/); virtioCoreQueueSync(pDevIns, &pThis->Virtio, qIdx); - - for (int i = 0; i < cSegs; i++) - RTMemFree(paReqSegs[i].pvSeg); - - RTMemFree(paReqSegs); - RTMemFree(pReqSegBuf); - /** @todo r=bird: Leaking pScsiCtrlUnion here! */ return VINF_SUCCESS;