Changeset 78394 in vbox for trunk

Timestamp:

May 6, 2019 3:14:09 PM (6 years ago)

Author:

vboxsync

Message:

DevATA: Minor cleanup, better compatibility with old ATA devices, streamline task register reads when busy (see bugref:5869).

File:

• trunk/src/VBox/Devices/Storage/DevATA.cpp (modified) (9 diffs)

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

@@ -1740 +1740 @@
     if (RT_SUCCESS(rc))
     {
-        ataR3SetSector(s, iLBA + cSectors);
+        /* When READ SECTORS etc. finishes, the address in the task
+         * file register points at the last sector read, not at the next
+         * sector that would be read. This ensures the registers always
+         * contain a valid sector address.
+         */
         if (s->cbElementaryTransfer == s->cbTotalTransfer)
+        {
             s->iSourceSink = ATAFN_SS_NULL;
+            ataR3SetSector(s, iLBA + cSectors - 1);
+        }
+        else
+            ataR3SetSector(s, iLBA + cSectors);
+        s->uATARegNSector -= cSectors;
         ataR3CmdOK(s, ATA_STAT_SEEK);
     }
@@ -3960 +3970 @@
     ataR3LockEnter(pCtl);
     ataR3CmdOK(s, ATA_STAT_SEEK);
-    ataHCSetIRQ(s);
     return false;
 }
@@ -3974 +3983 @@
     ataR3LockEnter(pCtl);
     ataR3CmdOK(s, ATA_STAT_SEEK);
-    ataHCSetIRQ(s);
     return false;
 }
@@ -4357 +4365 @@
  *     Device 1 responds to writes (except commands are not executed) but does
  *     not respond to reads. If Device 1 selected, normal behavior applies.
- *     See ATAPI-6 clause 9.16.2 and Table 15 in clause 7.1.
+ *     See ATA-6 clause 9.16.2 and Table 15 in clause 7.1.
+ *
+ * Note: Task file register writes with BSY=1 and/or DRQ=1 are problematic.
+ * Newer ATA/ATAPI specifications define that writes with BSY=0 and DRQ=1
+ * are a "host malfunction" and ignored (see e.g. Table 15 in clause 7.1 of
+ * the ATA-6 specification).
+ *  However, the results of writes with BSY=1 are "indeterminate", with the
+ * sole exception of writing the DEVICE RESET command (if supported). We
+ * choose to ignore the writes when BSY=1.
+ *
+ * Note: Ignoring writes to the Device/Head register when BSY=1 or DRQ=1 has the
+ * convenient side effect that the non-selected device (if any) is guaranteed
+ * to have BSY=0 and DRQ=0.
  */
 
@@ -4367 +4387 @@
     {
         case 0:
+            Assert(0);  /* This handler is not used for the Data register! */
             break;
         case 1: /* feature register */
@@ -4410 +4431 @@
             break;
         case 6: /* drive/head */
+            if (pCtl->aIfs[pCtl->iAIOIf].uATARegStatus & ATA_STAT_DRQ)
+            {
+                Log(("DRQ=1, register write ignored!\n"));
+                break;
+            }
             pCtl->aIfs[0].uATARegSelect = (val & ~0x10) | 0xa0;
             pCtl->aIfs[1].uATARegSelect = (val | 0x10) | 0xa0;
@@ -4466 +4492 @@
 
 
+/*
+ * Note: When the BSY bit is set, generally no other bits in the Status register
+ * are valid and no other registers can be read. Newer ATA specs explicitly define
+ * (e.g. clause 7.1, table 16 in ATA-6) that if any command block register is
+ * read with BSY=1, the Status register contents are always returned instead.
+ */
 static int ataIOPortReadU8(PATACONTROLLER pCtl, uint32_t addr, uint32_t *pu32)
 {
@@ -4502 +4534 @@
         }
     }
+
+    /*
+     * If the device is busy, the Status register is always read, but when reading the
+     * Status register proper, special semantics apply. See status register handling
+     * with interrupt clearing and yields below.
+     * NB: This can't happen for non-present devices as those never become busy.
+     */
+    if (RT_UNLIKELY(s->uATARegStatus & ATA_STAT_BUSY))
+    {
+        Assert(s->pDrvMedia);
+        if ((addr & 7) != 7)
+        {
+            *pu32 = s->uATARegStatus;
+            Log2(("%s: LUN#%d addr=%#x val=%#04x (BSY=1, returning status!)\n", __FUNCTION__, s->iLUN, addr, *pu32));
+            return VINF_SUCCESS;
+        }
+    }
+
     fHOB = !!(s->uATARegDevCtl & (1 << 7));
     switch (addr & 7)
     {
+        default:/* only to satisfy certain compilers, all cases are already handled */
         case 0: /* data register */
+            Assert(0);  /* This handler is not used for the Data register! */
             val = 0xff;
             break;
@@ -4552 +4604 @@
                 val = s->uATARegSelect;
             break;
-        default:
         case 7: /* primary status */
         {