Changeset 64036 in vbox for trunk/src

Timestamp:

Sep 27, 2016 10:40:37 AM (8 years ago)

Author:

vboxsync

Message:

AHCI: Free the request before notifying the guest about the completion or it might submit a new request with the same tag while it is still allocated causing errors

File:

• trunk/src/VBox/Devices/Storage/DevAHCI.cpp (modified) (13 diffs)

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

@@ -5208 +5208 @@
  * @returns nothing.
  * @param   pAhciPort          The port of the SATA controller.
- * @param   pAhciReq           The state of the task.
+ * @param   cbTransfer         Transfer size of the request.
  * @param   pCmdFis            Pointer to the command FIS from the guest.
+ * @param   fRead              Flag whether this is a read request.
  * @param   fInterrupt         If an interrupt should be send to the guest.
  */
-static void ahciSendPioSetupFis(PAHCIPort pAhciPort, PAHCIREQ pAhciReq, uint8_t *pCmdFis,
-                                bool fInterrupt)
+static void ahciSendPioSetupFis(PAHCIPort pAhciPort, size_t cbTransfer, uint8_t *pCmdFis,
+                                bool fRead, bool fInterrupt)
 {
     uint8_t abPioSetupFis[20];
@@ -5221 +5222 @@
     ahciLog(("%s: building PIO setup Fis\n", __FUNCTION__));
 
-    AssertMsg(   pAhciReq->cbTransfer > 0
-              && pAhciReq->cbTransfer <= 65534,
+    AssertMsg(   cbTransfer > 0
+              && cbTransfer <= 65534,
               ("Can't send PIO setup FIS for requests with 0 bytes to transfer or greater than 65534\n"));
 
@@ -5230 +5231 @@
         abPioSetupFis[AHCI_CMDFIS_TYPE]  = AHCI_CMDFIS_TYPE_PIOSETUP;
         abPioSetupFis[AHCI_CMDFIS_BITS]  = (fInterrupt ? AHCI_CMDFIS_I : 0);
-        if (pAhciReq->enmType == PDMMEDIAEXIOREQTYPE_READ)
+        if (fRead)
             abPioSetupFis[AHCI_CMDFIS_BITS] |= AHCI_CMDFIS_D;
         abPioSetupFis[AHCI_CMDFIS_STS]   = pCmdFis[AHCI_CMDFIS_STS];
@@ -5245 +5246 @@
 
         /* Set transfer count. */
-        abPioSetupFis[16] = (pAhciReq->cbTransfer >> 8) & 0xff;
-        abPioSetupFis[17] = pAhciReq->cbTransfer & 0xff;
+        abPioSetupFis[16] = (cbTransfer >> 8) & 0xff;
+        abPioSetupFis[17] = cbTransfer & 0xff;
 
         /* Update registers. */
@@ -5274 +5275 @@
  * @returns Nothing
  * @param   pAhciPort          The port of the SATA controller.
- * @param   pAhciReq           The state of the task.
+ * @param   uTag               The tag of the request.
  * @param   pCmdFis            Pointer to the command FIS from the guest.
  * @param   fInterrupt         If an interrupt should be send to the guest.
  */
-static void ahciSendD2HFis(PAHCIPort pAhciPort, PAHCIREQ pAhciReq, uint8_t *pCmdFis, bool fInterrupt)
+static void ahciSendD2HFis(PAHCIPort pAhciPort, uint32_t uTag, uint8_t *pCmdFis, bool fInterrupt)
 {
     uint8_t d2hFis[20];
@@ -5327 +5328 @@
 
             /* Mark command as completed. */
-            ASMAtomicOrU32(&pAhciPort->u32TasksFinished, (1 << pAhciReq->uTag));
+            ASMAtomicOrU32(&pAhciPort->u32TasksFinished, RT_BIT_32(uTag));
         }
 
@@ -5857 +5858 @@
 static void ahciR3ReqFree(PAHCIPort pAhciPort, PAHCIREQ pAhciReq)
 {
-    int rc = pAhciPort->pDrvMediaEx->pfnIoReqFree(pAhciPort->pDrvMediaEx, pAhciReq->hIoReq);
-    AssertRC(rc);
+    if (   pAhciReq
+        && !(pAhciReq->fFlags & AHCI_REQ_IS_ON_STACK))
+    {
+        int rc = pAhciPort->pDrvMediaEx->pfnIoReqFree(pAhciPort->pDrvMediaEx, pAhciReq->hIoReq);
+        AssertRC(rc);
+    }
 }
 
@@ -5957 +5962 @@
         }
 
+        /*
+         * Make a copy of the required data now and free the request. Otherwise the guest
+         * might issue a new request with the same tag and we run into a conflict when allocating
+         * a new request with the same tag later on.
+         */
+        uint32_t fFlags = pAhciReq->fFlags;
+        uint32_t uTag   = pAhciReq->uTag;
+        size_t   cbTransfer = pAhciReq->cbTransfer;
+        bool     fRead      = pAhciReq->enmType == PDMMEDIAEXIOREQTYPE_READ;
+        uint8_t  cmdFis[AHCI_CMDFIS_TYPE_H2D_SIZE];
+        memcpy(&cmdFis[0], &pAhciReq->cmdFis[0], sizeof(cmdFis));
+
+        ahciR3ReqFree(pAhciPort, pAhciReq);
         if (!fRedo)
         {
 
             /* Post a PIO setup FIS first if this is a PIO command which transfers data. */
-            if (pAhciReq->fFlags & AHCI_REQ_PIO_DATA)
-                ahciSendPioSetupFis(pAhciPort, pAhciReq, pAhciReq->cmdFis, false /* fInterrupt */);
-
-            if (pAhciReq->fFlags & AHCI_REQ_CLEAR_SACT)
+            if (fFlags & AHCI_REQ_PIO_DATA)
+                ahciSendPioSetupFis(pAhciPort, cbTransfer, &cmdFis[0], fRead, false /* fInterrupt */);
+
+            if (fFlags & AHCI_REQ_CLEAR_SACT)
             {
                 if (RT_SUCCESS(rcReq) && !ASMAtomicReadPtrT(&pAhciPort->pTaskErr, PAHCIREQ))
-                    ASMAtomicOrU32(&pAhciPort->u32QueuedTasksFinished, RT_BIT_32(pAhciReq->uTag));
+                    ASMAtomicOrU32(&pAhciPort->u32QueuedTasksFinished, RT_BIT_32(uTag));
             }
 
-            if (pAhciReq->fFlags & AHCI_REQ_IS_QUEUED)
+            if (fFlags & AHCI_REQ_IS_QUEUED)
             {
                 /*
@@ -5980 +5998 @@
             }
             else
-                ahciSendD2HFis(pAhciPort, pAhciReq, pAhciReq->cmdFis, true);
+                ahciSendD2HFis(pAhciPort, uTag, &cmdFis[0], true);
         }
     }
@@ -6009 +6027 @@
                         pAhciReq->cbTransfer, rcReq));
          }
+
+         ahciR3ReqFree(pAhciPort, pAhciReq);
     }
 
@@ -6024 +6044 @@
         PDMDevHlpAsyncNotificationCompleted(pAhciPort->pDevInsR3);
 
-    /* Don't free the request if it is on the stack. */
-    if (   pAhciReq
-        && !(pAhciReq->fFlags & AHCI_REQ_IS_ON_STACK))
-        ahciR3ReqFree(pAhciPort, pAhciReq);
     return fCanceled;
 }
@@ -6456 +6472 @@
          * but this FIS does not assert an interrupt
          */
-        ahciSendD2HFis(pAhciPort, pAhciReq, pAhciReq->cmdFis, false);
+        ahciSendD2HFis(pAhciPort, pAhciReq->uTag, pAhciReq->cmdFis, false);
         pAhciPort->regTFD &= ~AHCI_PORT_TFD_BSY;
     }
@@ -6586 +6602 @@
                 ahciLog(("%s: Setting device into reset state\n", __FUNCTION__));
                 pAhciPort->fResetDevice = true;
-                ahciSendD2HFis(pAhciPort, pAhciReq, pAhciReq->cmdFis, true);
+                ahciSendD2HFis(pAhciPort, pAhciReq->uTag, pAhciReq->cmdFis, true);
             }
             else if (pAhciPort->fResetDevice) /* The bit is not set and we are in a reset state. */