dev: Fix race conditions in IDE device on newer kernels

Newer linux kernels and distros exercise more functionality in the IDE device
than previously, exposing 2 races. The first race is the handling of aborted
DMA commands would immediately report the device is ready back to the kernel
and cause already in flight commands to assert the simulator when they returned
and discovered an inconsitent device state.  The second race was due to the
Status register not being handled correctly, the interrupt status bit would get
stuck at 1 and the driver eventually views this as a bad state and logs the
condition to the terminal.  This patch fixes these two conditions by making the
device handle aborted commands gracefully and properly handles clearing the
interrupt status bit in the Status register.

diff --git a/src/dev/ide_ctrl.cc b/src/dev/ide_ctrl.cc
index df02bf5f56911b2f1b99a3ab7c15ea8770716155..bbc0e379d60cbf99ad4997ed2d217469123cdbba 100644 (file)
--- a/src/dev/ide_ctrl.cc
+++ b/src/dev/ide_ctrl.cc
@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2013 ARM Limited
+ * All rights reserved
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 2004-2005 The Regents of The University of Michigan
  * All rights reserved.
  *
@@ -111,11 +123,11 @@ IdeController::IdeController(Params *p)
 
     if ((BARAddrs[0] & ~BAR_IO_MASK) && (!legacyIO[0] || ioShift)) {
         primary.cmdAddr = BARAddrs[0];  primary.cmdSize = BARSize[0];
-        primary.ctrlAddr = BARAddrs[1]; primary.ctrlSize = BARAddrs[1];
+        primary.ctrlAddr = BARAddrs[1]; primary.ctrlSize = BARSize[1];
     }
     if ((BARAddrs[2] & ~BAR_IO_MASK) && (!legacyIO[2] || ioShift)) {
         secondary.cmdAddr = BARAddrs[2];  secondary.cmdSize = BARSize[2];
-        secondary.ctrlAddr = BARAddrs[3]; secondary.ctrlSize = BARAddrs[3];
+        secondary.ctrlAddr = BARAddrs[3]; secondary.ctrlSize = BARSize[3];
     }
 
     ioEnabled = (config.command & htole(PCI_CMD_IOSE));
@@ -414,14 +426,21 @@ IdeController::Channel::accessBMI(Addr offset,
                 newVal.active = oldVal.active;
 
                 // to reset (set 0) IDEINTS and IDEDMAE, write 1 to each
-                if (oldVal.intStatus && newVal.intStatus)
+                if ((oldVal.intStatus == 1) && (newVal.intStatus == 1)) {
                     newVal.intStatus = 0; // clear the interrupt?
-                else
-                    newVal.intStatus = oldVal.intStatus;
-                if (oldVal.dmaError && newVal.dmaError)
+                } else {
+                    // Assigning two bitunion fields to each other does not
+                    // work as intended, so we need to use this temporary variable
+                    // to get around the bug.
+                    uint8_t tmp = oldVal.intStatus;
+                    newVal.intStatus = tmp;
+                }
+                if ((oldVal.dmaError == 1) && (newVal.dmaError == 1)) {
                     newVal.dmaError = 0;
-                else
-                    newVal.dmaError = oldVal.dmaError;
+                } else {
+                    uint8_t tmp = oldVal.dmaError;
+                    newVal.dmaError = tmp;
+                }
 
                 bmiRegs.status = newVal;
             }

diff --git a/src/dev/ide_disk.cc b/src/dev/ide_disk.cc
index bc7210d42166556988f6aeb2cefab27b218a9b30..b910c8a6a13e659141a45a61659ce2b7780a472d 100644 (file)
--- a/src/dev/ide_disk.cc
+++ b/src/dev/ide_disk.cc
@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2013 ARM Limited
+ * All rights reserved
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 2004-2005 The Regents of The University of Michigan
  * All rights reserved.
  *
@@ -143,6 +155,7 @@ IdeDisk::reset(int id)
     drqBytesLeft = 0;
     dmaRead = false;
     intrPending = false;
+    dmaAborted = false;
 
     // set the device state to idle
     dmaState = Dma_Idle;
@@ -319,6 +332,12 @@ IdeDisk::writeControl(const Addr offset, int size, const uint8_t *data)
 void
 IdeDisk::doDmaTransfer()
 {
+    if (dmaAborted) {
+        DPRINTF(IdeDisk, "DMA Aborted before reading PRD entry\n");
+        updateState(ACT_CMD_ERROR);
+        return;
+    }
+
     if (dmaState != Dma_Transfer || devState != Transfer_Data_Dma)
         panic("Inconsistent DMA transfer state: dmaState = %d devState = %d\n",
               dmaState, devState);
@@ -334,6 +353,11 @@ IdeDisk::doDmaTransfer()
 void
 IdeDisk::dmaPrdReadDone()
 {
+    if (dmaAborted) {
+        DPRINTF(IdeDisk, "DMA Aborted while reading PRD entry\n");
+        updateState(ACT_CMD_ERROR);
+        return;
+    }
 
     DPRINTF(IdeDisk,
             "PRD: baseAddr:%#x (%#x) byteCount:%d (%d) eot:%#x sector:%d\n",
@@ -396,6 +420,14 @@ IdeDisk::regStats()
 void
 IdeDisk::doDmaRead()
 {
+    if (dmaAborted) {
+        DPRINTF(IdeDisk, "DMA Aborted in middle of Dma Read\n");
+        if (dmaReadCG)
+            delete dmaReadCG;
+        dmaReadCG = NULL;
+        updateState(ACT_CMD_ERROR);
+        return;
+    }
 
     if (!dmaReadCG) {
         // clear out the data buffer
@@ -427,10 +459,8 @@ IdeDisk::doDmaRead()
 void
 IdeDisk::dmaReadDone()
 {
-
     uint32_t bytesWritten = 0;
 
-
     // write the data to the disk image
     for (bytesWritten = 0; bytesWritten < curPrd.getByteCount();
          bytesWritten += SectorSize) {
@@ -475,7 +505,14 @@ IdeDisk::doDmaDataWrite()
 void
 IdeDisk::doDmaWrite()
 {
-    DPRINTF(IdeDisk, "doDmaWrite: rescheduling\n");
+    if (dmaAborted) {
+        DPRINTF(IdeDisk, "DMA Aborted while doing DMA Write\n");
+        if (dmaWriteCG)
+            delete dmaWriteCG;
+        dmaWriteCG = NULL;
+        updateState(ACT_CMD_ERROR);
+        return;
+    }
     if (!dmaWriteCG) {
         // clear out the data buffer
         dmaWriteCG = new ChunkGenerator(curPrd.getBaseAddr(),
@@ -980,7 +1017,10 @@ IdeDisk::updateState(DevAction_t action)
         break;
 
       case Transfer_Data_Dma:
-        if (action == ACT_CMD_ERROR || action == ACT_DMA_DONE) {
+        if (action == ACT_CMD_ERROR) {
+            dmaAborted = true;
+            devState = Device_Dma_Abort;
+        } else if (action == ACT_DMA_DONE) {
             // clear the BSY bit
             setComplete();
             // set the seek bit
@@ -997,6 +1037,25 @@ IdeDisk::updateState(DevAction_t action)
         }
         break;
 
+      case Device_Dma_Abort:
+        if (action == ACT_CMD_ERROR) {
+            setComplete();
+            status |= STATUS_SEEK_BIT;
+            ctrl->setDmaComplete(this);
+            dmaAborted = false;
+            dmaState = Dma_Idle;
+
+            if (!isIENSet()) {
+                devState = Device_Idle_SI;
+                intrPost();
+            } else {
+                devState = Device_Idle_S;
+            }
+        } else {
+            DPRINTF(IdeDisk, "Disk still busy aborting previous DMA command\n");
+        }
+        break;
+
       default:
         panic("Unknown IDE device state: %#x\n", devState);
     }
@@ -1074,6 +1133,7 @@ IdeDisk::serialize(ostream &os)
     SERIALIZE_SCALAR(curSector);
     SERIALIZE_SCALAR(dmaRead);
     SERIALIZE_SCALAR(intrPending);
+    SERIALIZE_SCALAR(dmaAborted);
     SERIALIZE_ENUM(devState);
     SERIALIZE_ENUM(dmaState);
     SERIALIZE_ARRAY(dataBuffer, MAX_DMA_SIZE);
@@ -1126,6 +1186,7 @@ IdeDisk::unserialize(Checkpoint *cp, const string &section)
     UNSERIALIZE_SCALAR(curSector);
     UNSERIALIZE_SCALAR(dmaRead);
     UNSERIALIZE_SCALAR(intrPending);
+    UNSERIALIZE_SCALAR(dmaAborted);
     UNSERIALIZE_ENUM(devState);
     UNSERIALIZE_ENUM(dmaState);
     UNSERIALIZE_ARRAY(dataBuffer, MAX_DMA_SIZE);

diff --git a/src/dev/ide_disk.hh b/src/dev/ide_disk.hh
index b29d13870ef5d5e43ea1161132fb0259c5e79937..6ccca985eba3c50b43dcc5467dc2dad47000dbef 100644 (file)
--- a/src/dev/ide_disk.hh
+++ b/src/dev/ide_disk.hh
@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2013 ARM Limited
+ * All rights reserved
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 2004-2005 The Regents of The University of Michigan
  * All rights reserved.
  *
@@ -177,7 +189,8 @@ typedef enum DevState {
 
     // DMA protocol
     Prepare_Data_Dma,
-    Transfer_Data_Dma
+    Transfer_Data_Dma,
+    Device_Dma_Abort
 } DevState_t;
 
 typedef enum DmaState {
@@ -236,6 +249,8 @@ class IdeDisk : public SimObject
     int devID;
     /** Interrupt pending */
     bool intrPending;
+    /** DMA Aborted */
+    bool dmaAborted;
 
     Stats::Scalar dmaReadFullPages;
     Stats::Scalar dmaReadBytes;

diff --git a/src/sim/serialize.hh b/src/sim/serialize.hh
index aedb2d1aec4586a751e21f7f155663461fe1c30d..46d3fe82c2c4af8519bf464fe7e46fc9c88d0a91 100644 (file)
--- a/src/sim/serialize.hh
+++ b/src/sim/serialize.hh
@@ -57,7 +57,7 @@ class SimObject;
  * SimObject shouldn't cause the version number to increase, only changes to
  * existing objects such as serializing/unserializing more state, changing sizes
  * of serialized arrays, etc. */
-static const uint64_t gem5CheckpointVersion = 0x0000000000000006;
+static const uint64_t gem5CheckpointVersion = 0x0000000000000007;
 
 template <class T>
 void paramOut(std::ostream &os, const std::string &name, const T &param);

diff --git a/util/cpt_upgrader.py b/util/cpt_upgrader.py
index 623c9b2972b38512d217bdcc5690f426b627d66d..b5b54c1f265e8ed6380f10c04fb2846321c781bd 100755 (executable)
--- a/util/cpt_upgrader.py
+++ b/util/cpt_upgrader.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 
-# Copyright (c) 2012 ARM Limited
+# Copyright (c) 2012-2013 ARM Limited
 # All rights reserved
 #
 # The license below extends only to copyright in the software and shall
@@ -209,6 +209,15 @@ def from_5(cpt):
     else:
         print "ISA is not x86"
 
+# Version 7 of the checkpoint adds support for the IDE dmaAbort flag
+def from_6(cpt):
+    # Update IDE disk devices with dmaAborted
+    for sec in cpt.sections():
+        # curSector only exists in IDE devices, so key on that attribute
+        if cpt.has_option(sec, "curSector"):
+            cpt.set(sec, "dmaAborted", "false")
+
+
 migrations = []
 migrations.append(from_0)
 migrations.append(from_1)
@@ -216,6 +225,7 @@ migrations.append(from_2)
 migrations.append(from_3)
 migrations.append(from_4)
 migrations.append(from_5)
+migrations.append(from_6)
 
 verbose_print = False
 
@@ -274,7 +284,6 @@ def process_file(path, **kwargs):
     verboseprint("\t...completed")
     cpt.write(file(path, 'w'))
 
-
 if __name__ == '__main__':
     from optparse import OptionParser
     parser = OptionParser("usage: %prog [options] <filename or directory>")