changeset e21fe6a62b1c in /z/repo/m5
details: http://repo.m5sim.org/m5?cmd=changeset;node=e21fe6a62b1c
description:
cpu: fix exec tracing memory corruption bug
Accessing traceData (to call setAddress() and/or setData())
after initiating a timing translation was causing crashes,
since a failed translation could delete the traceData
object before returning.
It turns out that there was never a need to access traceData
after initiating the translation, as the traced data was
always available earlier; this ordering was merely
historical. Furthermore, traceData->setAddress() and
traceData->setData() were being called both from the CPU
model and the ISA definition, often redundantly.
This patch standardizes all setAddress and setData calls
for memory instructions to be in the CPU models and not
in the ISA definition. It also moves those calls above
the translation calls to eliminate the crashes.
diffstat:
11 files changed, 43 insertions(+), 52 deletions(-)
src/arch/alpha/isa/mem.isa | 6 ------
src/arch/arm/isa/formats/mem.isa | 2 --
src/arch/mips/isa/formats/mem.isa | 8 --------
src/arch/mips/isa/formats/util.isa | 3 ---
src/arch/power/isa/formats/mem.isa | 2 --
src/arch/power/isa/formats/util.isa | 3 ---
src/cpu/inorder/resources/cache_unit.cc | 9 +++++++++
src/cpu/simple/atomic.cc | 7 +------
src/cpu/simple/base.cc | 21 +++++++++++++++++++++
src/cpu/simple/base.hh | 12 ++----------
src/cpu/simple/timing.cc | 22 ++++++++++------------
diffs (truncated from 310 to 300 lines):
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/alpha/isa/mem.isa
--- a/src/arch/alpha/isa/mem.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/alpha/isa/mem.isa Tue Mar 23 08:50:57 2010 -0700
@@ -275,7 +275,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -310,7 +309,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, &write_result);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -344,7 +342,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
return fault;
@@ -478,9 +475,6 @@
mem_flags = makeList(mem_flags)
inst_flags = makeList(inst_flags)
- # add hook to get effective addresses into execution trace output.
- ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
# Some CPU models execute the memory operation as an atomic unit,
# while others want to separate them into an effective address
# computation and a memory access operation. As a result, we need
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/arm/isa/formats/mem.isa
--- a/src/arch/arm/isa/formats/mem.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/arm/isa/formats/mem.isa Tue Mar 23 08:50:57 2010 -0700
@@ -172,7 +172,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -204,7 +203,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
// Need to write back any potential address register update
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/mips/isa/formats/mem.isa
--- a/src/arch/mips/isa/formats/mem.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/mips/isa/formats/mem.isa Tue Mar 23 08:50:57 2010 -0700
@@ -305,7 +305,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -342,7 +341,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -377,7 +375,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, &write_result);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -411,7 +408,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
return fault;
@@ -435,8 +431,6 @@
if (fault == NoFault) {
%(op_wb)s;
-
- if (traceData) { traceData->setData(getMemData(xc, pkt)); }
}
return fault;
@@ -459,8 +453,6 @@
if (fault == NoFault) {
%(op_wb)s;
-
- if (traceData) { traceData->setData(getMemData(xc, pkt)); }
}
return fault;
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/mips/isa/formats/util.isa
--- a/src/arch/mips/isa/formats/util.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/mips/isa/formats/util.isa Tue Mar 23 08:50:57 2010 -0700
@@ -38,9 +38,6 @@
mem_flags = makeList(mem_flags)
inst_flags = makeList(inst_flags)
- # add hook to get effective addresses into execution trace output.
- ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
# Some CPU models execute the memory operation as an atomic unit,
# while others want to separate them into an effective address
# computation and a memory access operation. As a result, we need
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/power/isa/formats/mem.isa
--- a/src/arch/power/isa/formats/mem.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/power/isa/formats/mem.isa Tue Mar 23 08:50:57 2010 -0700
@@ -166,7 +166,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
if (fault == NoFault) {
@@ -196,7 +195,6 @@
if (fault == NoFault) {
fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
memAccessFlags, NULL);
- if (traceData) { traceData->setData(Mem); }
}
// Need to write back any potential address register update
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/arch/power/isa/formats/util.isa
--- a/src/arch/power/isa/formats/util.isa Tue Mar 23 00:29:10 2010 -0400
+++ b/src/arch/power/isa/formats/util.isa Tue Mar 23 08:50:57 2010 -0700
@@ -97,9 +97,6 @@
mem_flags = makeList(mem_flags)
inst_flags = makeList(inst_flags)
- # add hook to get effective addresses into execution trace output.
- ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
# Generate InstObjParams for the memory access.
iop = InstObjParams(name, Name, base_class,
{'ea_code': ea_code,
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/cpu/inorder/resources/cache_unit.cc
--- a/src/cpu/inorder/resources/cache_unit.cc Tue Mar 23 00:29:10 2010 -0400
+++ b/src/cpu/inorder/resources/cache_unit.cc Tue Mar 23 08:50:57 2010 -0700
@@ -443,6 +443,10 @@
//The size of the data we're trying to read.
int dataSize = sizeof(T);
+ if (inst->traceData) {
+ inst->traceData->setAddr(addr);
+ }
+
if (inst->split2ndAccess) {
dataSize = inst->split2ndSize;
cache_req->splitAccess = true;
@@ -541,6 +545,11 @@
//The size of the data we're trying to read.
int dataSize = sizeof(T);
+ if (inst->traceData) {
+ inst->traceData->setAddr(addr);
+ inst->traceData->setData(data);
+ }
+
if (inst->split2ndAccess) {
dataSize = inst->split2ndSize;
cache_req->splitAccess = true;
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/cpu/simple/atomic.cc
--- a/src/cpu/simple/atomic.cc Tue Mar 23 00:29:10 2010 -0400
+++ b/src/cpu/simple/atomic.cc Tue Mar 23 08:50:57 2010 -0700
@@ -451,6 +451,7 @@
if (traceData) {
traceData->setAddr(addr);
+ traceData->setData(data);
}
//The block size of our peer.
@@ -530,12 +531,6 @@
//stop now.
if (fault != NoFault || secondAddr <= addr)
{
- // If the write needs to have a fault on the access, consider
- // calling changeStatus() and changing it to "bad addr write"
- // or something.
- if (traceData) {
- traceData->setData(gtoh(data));
- }
if (req->isLocked() && fault == NoFault) {
assert(locked);
locked = false;
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/cpu/simple/base.cc
--- a/src/cpu/simple/base.cc Tue Mar 23 00:29:10 2010 -0400
+++ b/src/cpu/simple/base.cc Tue Mar 23 08:50:57 2010 -0700
@@ -205,6 +205,27 @@
{
}
+void
+BaseSimpleCPU::prefetch(Addr addr, unsigned flags)
+{
+ if (traceData) {
+ traceData->setAddr(addr);
+ }
+
+ // need to do this...
+}
+
+void
+BaseSimpleCPU::writeHint(Addr addr, int size, unsigned flags)
+{
+ if (traceData) {
+ traceData->setAddr(addr);
+ }
+
+ // need to do this...
+}
+
+
Fault
BaseSimpleCPU::copySrcTranslate(Addr src)
{
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/cpu/simple/base.hh
--- a/src/cpu/simple/base.hh Tue Mar 23 00:29:10 2010 -0400
+++ b/src/cpu/simple/base.hh Tue Mar 23 08:50:57 2010 -0700
@@ -232,16 +232,8 @@
Addr getEA() { panic("BaseSimpleCPU::getEA() not implemented\n");
M5_DUMMY_RETURN}
- void prefetch(Addr addr, unsigned flags)
- {
- // need to do this...
- }
-
- void writeHint(Addr addr, int size, unsigned flags)
- {
- // need to do this...
- }
-
+ void prefetch(Addr addr, unsigned flags);
+ void writeHint(Addr addr, int size, unsigned flags);
Fault copySrcTranslate(Addr src);
diff -r 8a05ebc9d372 -r e21fe6a62b1c src/cpu/simple/timing.cc
--- a/src/cpu/simple/timing.cc Tue Mar 23 00:29:10 2010 -0400
+++ b/src/cpu/simple/timing.cc Tue Mar 23 08:50:57 2010 -0700
@@ -426,6 +426,10 @@
int data_size = sizeof(T);
BaseTLB::Mode mode = BaseTLB::Read;
+ if (traceData) {
+ traceData->setAddr(addr);
+ }
+
RequestPtr req = new Request(asid, addr, data_size,
flags, pc, _cpuId, tid);
@@ -460,11 +464,6 @@
thread->dtb->translateTiming(req, tc, translation, mode);
}
- if (traceData) {
- traceData->setData(data);
- traceData->setAddr(addr);
- }
-
return NoFault;
}
@@ -548,6 +547,11 @@
int data_size = sizeof(T);
BaseTLB::Mode mode = BaseTLB::Write;
+ if (traceData) {
+ traceData->setAddr(addr);
+ traceData->setData(data);
+ }
+
RequestPtr req = new Request(asid, addr, data_size,
flags, pc, _cpuId, tid);
@@ -584,13 +588,7 @@
thread->dtb->translateTiming(req, tc, translation, mode);
}
- if (traceData) {
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
