Changeset 50018 in vbox

Timestamp:

Jan 2, 2014 3:50:08 PM (11 years ago)

Author:

vboxsync

Message:

Storage/AHCI: Make sure all threads are idling before finishing the controller reset, don't process garbage FIS structures

Location:

trunk/src/VBox/Devices

Files:

• Storage/DevAHCI.cpp (modified) (12 diffs)
• testcase/tstDeviceStructSizeRC.cpp (modified) (1 diff)

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

@@ -675 +675 @@
     /** Bitmask of ports which asserted an interrupt. */
     volatile uint32_t               u32PortsInterrupted;
+    /** Number of I/O threads currently active - used for async controller reset handling. */
+    volatile uint32_t               cThreadsActive;
     /** Device is in a reset state. */
     bool                            fReset;
@@ -698 +700 @@
     /** Flag whether we have written the first 4bytes in an 8byte MMIO write successfully. */
     volatile bool                   f8ByteMMIO4BytesWrittenSuccessfully;
+
+#if HC_ARCH_BITS == 64
+    uint32_t                        Alignment7;
+#endif
 
     /** The support driver session handle. */
@@ -1735 +1741 @@
     return VINF_IOM_R3_MMIO_WRITE;
 #else
-    ahci->regHbaCtrl = (u32Value & AHCI_HBA_CTRL_RW_MASK) | AHCI_HBA_CTRL_AE;
-    if (ahci->regHbaCtrl & AHCI_HBA_CTRL_HR)
+    /*
+     * Increase the active thread counter because we might set the host controller
+     * reset bit.
+     */
+    ASMAtomicIncU32(&ahci->cThreadsActive);
+    ASMAtomicWriteU32(&ahci->regHbaCtrl, (u32Value & AHCI_HBA_CTRL_RW_MASK) | AHCI_HBA_CTRL_AE);
+
+    /*
+     * Do the HBA reset if requested and there is no other active thread at the moment,
+     * the work is deferred to the last active thread otherwise.
+     */
+    uint32_t cThreadsActive = ASMAtomicDecU32(&ahci->cThreadsActive);
+    if (   (u32Value & AHCI_HBA_CTRL_HR)
+        && !cThreadsActive)
         ahciHBAReset(ahci);
+
     return VINF_SUCCESS;
 #endif
@@ -6321 +6340 @@
  * Retrieve a command FIS from guest memory.
  *
- * @returns nothing
+ * @returns whether the H2D FIS was successfully read from the guest memory.
  * @param pAhciReq The state of the actual task.
  */
-static void ahciPortTaskGetCommandFis(PAHCIPort pAhciPort, PAHCIREQ pAhciReq)
+static bool ahciPortTaskGetCommandFis(PAHCIPort pAhciPort, PAHCIREQ pAhciReq)
 {
     RTGCPHYS  GCPhysAddrCmdTbl;
 
-    AssertMsg(pAhciPort->GCPhysAddrClb && pAhciPort->GCPhysAddrFb, ("%s: GCPhysAddrClb and/or GCPhysAddrFb are 0\n", __FUNCTION__));
+    AssertMsgReturn(pAhciPort->GCPhysAddrClb && pAhciPort->GCPhysAddrFb,
+                    ("%s: GCPhysAddrClb and/or GCPhysAddrFb are 0\n", __FUNCTION__),
+                    false);
 
     /*
@@ -6347 +6368 @@
     GCPhysAddrCmdTbl = AHCI_RTGCPHYS_FROM_U32(pAhciReq->cmdHdr.u32CmdTblAddrUp, pAhciReq->cmdHdr.u32CmdTblAddr);
 
-    AssertMsg((pAhciReq->cmdHdr.u32DescInf & AHCI_CMDHDR_CFL_MASK) * sizeof(uint32_t) == AHCI_CMDFIS_TYPE_H2D_SIZE,
-              ("This is not a command FIS!!\n"));
+    AssertMsgReturn((pAhciReq->cmdHdr.u32DescInf & AHCI_CMDHDR_CFL_MASK) * sizeof(uint32_t) == AHCI_CMDFIS_TYPE_H2D_SIZE,
+                    ("This is not a command FIS!!\n"),
+                    false);
 
     /* Read the command Fis. */
     LogFlow(("%s: PDMDevHlpPhysRead GCPhysAddrCmdTbl=%RGp cbCmdFis=%u\n", __FUNCTION__, GCPhysAddrCmdTbl, AHCI_CMDFIS_TYPE_H2D_SIZE));
     PDMDevHlpPhysRead(pAhciPort->CTX_SUFF(pDevIns), GCPhysAddrCmdTbl, &pAhciReq->cmdFis[0], AHCI_CMDFIS_TYPE_H2D_SIZE);
+
+    AssertMsgReturn(pAhciReq->cmdFis[AHCI_CMDFIS_TYPE] == AHCI_CMDFIS_TYPE_H2D,
+                    ("This is not a command FIS\n"),
+                    false);
 
     /* Set transfer direction. */
@@ -6399 +6425 @@
     }
 #endif
+
+    return true;
 }
 
@@ -6446 +6474 @@
         unsigned idx = 0;
         uint32_t u32Tasks = 0;
+        uint32_t u32RegHbaCtrl = 0;
 
         ASMAtomicWriteBool(&pAhciPort->fWrkThreadSleeping, true);
@@ -6461 +6490 @@
 
         ASMAtomicWriteBool(&pAhciPort->fWrkThreadSleeping, false);
+        ASMAtomicIncU32(&pAhci->cThreadsActive);
+
+        /*
+         * Check whether the global host controller bit is set and go to sleep immediately again
+         * if it is set.
+         */
+        u32RegHbaCtrl = ASMAtomicReadU32(&pAhci->regHbaCtrl);
+        if (   u32RegHbaCtrl & AHCI_HBA_CTRL_HR
+            && !ASMAtomicDecU32(&pAhci->cThreadsActive))
+        {
+            ahciHBAReset(pAhci);
+            continue;
+        }
+
         idx = ASMBitFirstSetU32(u32Tasks);
         while (idx)
@@ -6498 +6541 @@
             ASMAtomicWriteU32(&pAhciPort->u32CurrentCommandSlot, pAhciReq->uTag);
 
-            ahciPortTaskGetCommandFis(pAhciPort, pAhciReq);
+            bool fFisRead = ahciPortTaskGetCommandFis(pAhciPort, pAhciReq);
+            if (RT_UNLIKELY(!fFisRead))
+            {
+                /*
+                 * Couldn't find anything in either the AHCI or SATA spec which
+                 * indicates what should be done if the FIS is not read successfully.
+                 * The closes thing is in the state machine, stating that the device
+                 * should go into idle state again (SATA spec 1.0 chapter 8.7.1).
+                 * Do the same here and ignore any corrupt FIS types, after all
+                 * the guest messed up everything and this behavior is undefined.
+                 */
+                ASMAtomicCmpXchgSize(&pAhciReq->enmTxState, AHCITXSTATE_FREE, AHCITXSTATE_ACTIVE, fXchg);
+                Assert(fXchg);
+                u32Tasks &= ~RT_BIT_32(idx); /* Clear task bit. */
+                idx = ASMBitFirstSetU32(u32Tasks);
+                continue;
+            }
 
             /* Mark the task as processed by the HBA if this is a queued task so that it doesn't occur in the CI register anymore. */
@@ -6515 +6574 @@
                     pAhciPort->fResetDevice = true;
                     ahciSendD2HFis(pAhciPort, pAhciReq, pAhciReq->cmdFis, true);
-
-                    ASMAtomicCmpXchgSize(&pAhciReq->enmTxState, AHCITXSTATE_FREE, AHCITXSTATE_ACTIVE, fXchg);
-                    AssertMsg(fXchg, ("Task is not active\n"));
-                    break;
                 }
                 else if (pAhciPort->fResetDevice) /* The bit is not set and we are in a reset state. */
-                {
                     ahciFinishStorageDeviceReset(pAhciPort, pAhciReq);
-
-                    ASMAtomicCmpXchgSize(&pAhciReq->enmTxState, AHCITXSTATE_FREE, AHCITXSTATE_ACTIVE, fXchg);
-                    AssertMsg(fXchg, ("Task is not active\n"));
-                    break;
-                }
                 else /* We are not in a reset state update the control registers. */
                     AssertMsgFailed(("%s: Update the control register\n", __FUNCTION__));
+
+                ASMAtomicCmpXchgSize(&pAhciReq->enmTxState, AHCITXSTATE_FREE, AHCITXSTATE_ACTIVE, fXchg);
+                AssertMsg(fXchg, ("Task is not active\n"));
+                break;
             }
             else
@@ -6636 +6689 @@
             idx = ASMBitFirstSetU32(u32Tasks);
         } /* while tasks available */
+
+        /*
+         * Check whether a host controller reset is pending and execute the reset
+         * if this is the last active thread.
+         */
+        u32RegHbaCtrl = ASMAtomicReadU32(&pAhci->regHbaCtrl);
+        uint32_t cThreadsActive = ASMAtomicDecU32(&pAhci->cThreadsActive);
+        if (   (u32RegHbaCtrl & AHCI_HBA_CTRL_HR)
+            && !cThreadsActive)
+            ahciHBAReset(pAhci);
     } /* While running */
 
@@ -7998 +8061 @@
     PCIDevSetWord(&pThis->dev, 0xaa, 0x0010);      /* Revision */
     PCIDevSetDWord(&pThis->dev, 0xac, 0x00000028); /* SATA Capability Register 1 */
+
+    pThis->cThreadsActive = 0;
 
     /* Initialize port members. */

trunk/src/VBox/Devices/testcase/tstDeviceStructSizeRC.cpp

@@ -1284 +1284 @@
     GEN_CHECK_OFF(AHCI, lock);
     GEN_CHECK_OFF(AHCI, u32PortsInterrupted);
+    GEN_CHECK_OFF(AHCI, cThreadsActive);
     GEN_CHECK_OFF(AHCI, fReset);
     GEN_CHECK_OFF(AHCI, f64BitAddr);