Changeset 82183 in vbox

Timestamp:

Nov 25, 2019 4:14:33 PM (5 years ago)

Author:

vboxsync

Message:

Storage/DevVirtioSCSI.cpp: Fixed sizeof() calc in virtioScsiR3ReqErr() that probably caused problems with response code. After fix still works booting Linux with non-bootable virtio SCSI disks, but now failes in BIOS boot with bad INQUIRY, which should fail if testing 16 SCSI disks and failing to boot if any of them fail

Location:

trunk/src/VBox/Devices

Files:

• Storage/DevVirtioSCSI.cpp (modified) (9 diffs)
• VirtIO/Virtio_1_0.cpp (modified) (1 diff)

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

@@ -775 +775 @@
     AssertReturn(pabSenseBuf, VERR_NO_MEMORY);
 
-    Log2Func(("   status: %s    response: %s\n", SCSIStatusText(pRespHdr->uStatus), virtioGetReqRespText(pRespHdr->uResponse)));
+    Log2Func(("   status: %s    response: %s\n",
+        SCSIStatusText(pRespHdr->uStatus), virtioGetReqRespText(pRespHdr->uResponse)));
 
     PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
@@ -783 +784 @@
     AssertReturn(paReqSegs, VERR_NO_MEMORY);
 
-    paReqSegs[0].cbSeg = sizeof(pRespHdr);
+    paReqSegs[0].cbSeg = sizeof(*pRespHdr);
     paReqSegs[0].pvSeg = pRespHdr;
     paReqSegs[1].cbSeg = pThis->virtioScsiConfig.uSenseSize;
@@ -842 +843 @@
             break;
         case SCSI_SENSE_NOT_READY:
-            respHdr->uResponse = VIRTIOSCSI_S_BUSY; /* e.g. re-tryable */
+            /* Not sure what to return for this. See choices at VirtIO 1.0,  5.6.6.1.1 */
+            respHdr->uResponse = VIRTIOSCSI_S_FAILURE;
+            /* respHdr->uResponse = VIRTIOSCSI_S_BUSY; */ /* BUSY is VirtIO's 'retryable' response */
             break;
         default:
@@ -871 +874 @@
     AssertRC(rc);
 
-    /* Masking used to deal with datatype size differences between APIs (Windows complains otherwise) */
+    /* Masking deals with data type size discrepancies between
+     * The APIs (virtio and VBox). Windows C-compiler complains otherwise */
     Assert(!(cbXfer & 0xffffffff00000000));
     uint32_t cbXfer32 = cbXfer & 0xffffffff;
@@ -1126 +1130 @@
     {
         LogRel(("* * * REPORT LUNS LU ACCESSED * * * "));
-        uScsiLun = 0xff; /* Force rejection. todo: figure out right way to handle, r=paul */
+        /* Force rejection. todo: figure out right way to handle. Note this is a very
+         * vague and confusing part of the VirtIO spec which deviates from the SCSI standard
+         * as Klaus has pointed out on numerous occasions. I have not been able to determine
+         * how to implement this properly. Nor do I see any of the guest drivers at this
+         * point making use of it, hence the loud log message so we can catch it if/when a guest
+         * does access it and can resume the investigation at that point.  r=paul */
+        uScsiLun = 0xff;
     }
     else
@@ -1166 +1176 @@
     }
 
-    if (RT_LIKELY(uTarget < pThis->cTargets
-                  &&  pThisCC->paTargetInstances[uTarget].fPresent
-                  &&  pThisCC->paTargetInstances[uTarget].pDrvMediaEx))
+    if (RT_LIKELY(   uTarget < pThis->cTargets
+                  && pThisCC->paTargetInstances[uTarget].fPresent
+                  && pThisCC->paTargetInstances[uTarget].pDrvMediaEx))
     { /*  likely */ }
     else
@@ -1551 +1561 @@
                 if (RT_UNLIKELY(pThread->enmState != PDMTHREADSTATE_RUNNING))
                     return VINF_SUCCESS;
+                if (rc == VERR_INTERRUPTED)
+                    continue;
                 Log6Func(("%s worker woken\n", VIRTQNAME(qIdx)));
                 ASMAtomicWriteBool(&pWorkerR3->fNotified, false);
@@ -1998 +2010 @@
     pHlp->pfnSSMGetU32(pSSM,  &pThis->fResetting);
 
-    /** @todo Ask aeichner about BIOS-related changes */
-
     pHlp->pfnSSMGetU32(pSSM, &pThis->cTargets);
 
@@ -2078 +2088 @@
     pHlp->pfnSSMPutU32(pSSM,  pThis->fHasLunChange);
     pHlp->pfnSSMPutU32(pSSM,  pThis->fResetting);
-
-    /** @todo Ask aeichner about BIOS-related changes */
 
     AssertMsg(!pThis->cActiveReqs, ("There are still outstanding requests on this device\n"));

trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp

@@ -170 +170 @@
 DECLINLINE(bool) virtqIsEmpty(PPDMDEVINS pDevIns, PVIRTIOCORE pVirtio, uint16_t idxQueue)
 {
-    return virtioReadAvailRingIdx(pDevIns, pVirtio, idxQueue) == pVirtio->virtqState[idxQueue].uAvailIdx;
+    uint16_t uAvailGst = virtioReadAvailRingIdx(pDevIns, pVirtio, idxQueue);
+    bool fEmpty = uAvailGst == pVirtio->virtqState[idxQueue].uAvailIdx;
+
+    LogFlow(("Q<%u>: uAvailGst=%u uAvailIdx=%u -> fEmpty=%RTbool\n",
+             idxQueue, uAvailGst, pVirtio->virtqState[idxQueue].uAvailIdx, fEmpty));
+    return fEmpty;
 }