changeset 6f2b525b8f16 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=6f2b525b8f16
description:
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.
diffstat:
src/dev/ide_ctrl.cc | 35 ++++++++++++++++++++------
src/dev/ide_disk.cc | 69 ++++++++++++++++++++++++++++++++++++++++++++++++---
src/dev/ide_disk.hh | 17 ++++++++++++-
src/sim/serialize.hh | 2 +-
util/cpt_upgrader.py | 13 ++++++++-
5 files changed, 120 insertions(+), 16 deletions(-)
diffs (truncated from 305 to 300 lines):
diff -r 5d8722ab804b -r 6f2b525b8f16 src/dev/ide_ctrl.cc
--- a/src/dev/ide_ctrl.cc Thu Oct 31 13:41:13 2013 -0500
+++ b/src/dev/ide_ctrl.cc Thu Oct 31 13:41:13 2013 -0500
@@ -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 @@
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 @@
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 -r 5d8722ab804b -r 6f2b525b8f16 src/dev/ide_disk.cc
--- a/src/dev/ide_disk.cc Thu Oct 31 13:41:13 2013 -0500
+++ b/src/dev/ide_disk.cc Thu Oct 31 13:41:13 2013 -0500
@@ -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 @@
drqBytesLeft = 0;
dmaRead = false;
intrPending = false;
+ dmaAborted = false;
// set the device state to idle
dmaState = Dma_Idle;
@@ -319,6 +332,12 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
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 -r 5d8722ab804b -r 6f2b525b8f16 src/dev/ide_disk.hh
--- a/src/dev/ide_disk.hh Thu Oct 31 13:41:13 2013 -0500
+++ b/src/dev/ide_disk.hh Thu Oct 31 13:41:13 2013 -0500
@@ -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 @@
// DMA protocol
Prepare_Data_Dma,
- Transfer_Data_Dma
+ Transfer_Data_Dma,
+ Device_Dma_Abort
} DevState_t;
typedef enum DmaState {
@@ -236,6 +249,8 @@
int devID;
/** Interrupt pending */
bool intrPending;
+ /** DMA Aborted */
+ bool dmaAborted;
Stats::Scalar dmaReadFullPages;
Stats::Scalar dmaReadBytes;
diff -r 5d8722ab804b -r 6f2b525b8f16 src/sim/serialize.hh
--- a/src/sim/serialize.hh Thu Oct 31 13:41:13 2013 -0500
+++ b/src/sim/serialize.hh Thu Oct 31 13:41:13 2013 -0500
@@ -57,7 +57,7 @@
* 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 ¶m);
diff -r 5d8722ab804b -r 6f2b525b8f16 util/cpt_upgrader.py
--- a/util/cpt_upgrader.py Thu Oct 31 13:41:13 2013 -0500
+++ b/util/cpt_upgrader.py Thu Oct 31 13:41:13 2013 -0500
@@ -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 @@
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_3)
migrations.append(from_4)
migrations.append(from_5)
+migrations.append(from_6)
verbose_print = False
@@ -274,7 +284,6 @@
verboseprint("\t...completed")
cpt.write(file(path, 'w'))
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
