From 4d77ea7a5783d1de87a8eb804b17a6ef352998ce Mon Sep 17 00:00:00 2001 From: Steve Reinhardt Date: Tue, 23 Mar 2010 08:50:57 -0700 Subject: 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. --- 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 ++++++++++------------ 5 files changed, 43 insertions(+), 28 deletions(-) (limited to 'src/cpu') diff --git a/src/cpu/inorder/resources/cache_unit.cc b/src/cpu/inorder/resources/cache_unit.cc index 376ea8d26..d12f11a2c 100644 --- a/src/cpu/inorder/resources/cache_unit.cc +++ b/src/cpu/inorder/resources/cache_unit.cc @@ -443,6 +443,10 @@ CacheUnit::read(DynInstPtr inst, Addr addr, T &data, unsigned flags) //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 @@ CacheUnit::write(DynInstPtr inst, T data, Addr addr, unsigned flags, //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 --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc index 05b4ca3e2..7740434d8 100644 --- a/src/cpu/simple/atomic.cc +++ b/src/cpu/simple/atomic.cc @@ -451,6 +451,7 @@ AtomicSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) if (traceData) { traceData->setAddr(addr); + traceData->setData(data); } //The block size of our peer. @@ -530,12 +531,6 @@ AtomicSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) //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 --git a/src/cpu/simple/base.cc b/src/cpu/simple/base.cc index 0104e1b1f..17ba6a10b 100644 --- a/src/cpu/simple/base.cc +++ b/src/cpu/simple/base.cc @@ -205,6 +205,27 @@ change_thread_state(ThreadID tid, int activate, int priority) { } +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 --git a/src/cpu/simple/base.hh b/src/cpu/simple/base.hh index 39961fb88..87e211521 100644 --- a/src/cpu/simple/base.hh +++ b/src/cpu/simple/base.hh @@ -232,16 +232,8 @@ class BaseSimpleCPU : public BaseCPU 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 --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc index 221cb0d0d..7583c09e6 100644 --- a/src/cpu/simple/timing.cc +++ b/src/cpu/simple/timing.cc @@ -426,6 +426,10 @@ TimingSimpleCPU::read(Addr addr, T &data, unsigned flags) 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 @@ TimingSimpleCPU::read(Addr addr, T &data, unsigned flags) thread->dtb->translateTiming(req, tc, translation, mode); } - if (traceData) { - traceData->setData(data); - traceData->setAddr(addr); - } - return NoFault; } @@ -548,6 +547,11 @@ TimingSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) 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 @@ TimingSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) thread->dtb->translateTiming(req, tc, translation, mode); } - if (traceData) { - traceData->setAddr(req->getVaddr()); - traceData->setData(data); - } - - // If the write needs to have a fault on the access, consider calling - // changeStatus() and changing it to "bad addr write" or something. + // Translation faults will be returned via finishTranslation() return NoFault; } -- cgit v1.2.3 From f066bfc2f5944353ea11fd4c6853179c4ea1af78 Mon Sep 17 00:00:00 2001 From: Steve Reinhardt Date: Tue, 23 Mar 2010 08:50:59 -0700 Subject: cpu: get rid of uncached access "events" These recordEvent() calls could cause crashes since they access the req pointer after it's potentially been deleted during a failed translation call. (Similar problem to the traceData bug fixed in the previous cset.) Moving them above the translation call (as was done recentlyi in cset 8b2b8e5e7d35) avoids the crash but doesn't work, since at that point we don't know if the access is uncached or not. It's not clear why these calls are there, and no one seems to use them, so we'll just delete them. If they are needed, they should be moved to somewhere that's guaranteed to be after the translation completes but before the request is possibly deleted, e.g., in finishTranslation(). --- src/cpu/simple/atomic.cc | 8 -------- src/cpu/simple/timing.cc | 8 -------- 2 files changed, 16 deletions(-) (limited to 'src/cpu') diff --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc index 7740434d8..d96adffd5 100644 --- a/src/cpu/simple/atomic.cc +++ b/src/cpu/simple/atomic.cc @@ -351,10 +351,6 @@ AtomicSimpleCPU::read(Addr addr, T &data, unsigned flags) } } - // This will need a new way to tell if it has a dcache attached. - if (req->isUncacheable()) - recordEvent("Uncached Read"); - //If there's a fault, return it if (fault != NoFault) { if (req->isPrefetch()) { @@ -523,10 +519,6 @@ AtomicSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) } } - // This will need a new way to tell if it's hooked up to a cache or not. - if (req->isUncacheable()) - recordEvent("Uncached Write"); - //If there's a fault or we don't need to access a second cache line, //stop now. if (fault != NoFault || secondAddr <= addr) diff --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc index 7583c09e6..b8fc5ab84 100644 --- a/src/cpu/simple/timing.cc +++ b/src/cpu/simple/timing.cc @@ -436,10 +436,6 @@ TimingSimpleCPU::read(Addr addr, T &data, unsigned flags) Addr split_addr = roundDown(addr + data_size - 1, block_size); assert(split_addr <= addr || split_addr - addr < block_size); - // This will need a new way to tell if it's hooked up to a cache or not. - if (req->isUncacheable()) - recordEvent("Uncached Write"); - _status = DTBWaitResponse; if (split_addr > addr) { RequestPtr req1, req2; @@ -558,10 +554,6 @@ TimingSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res) Addr split_addr = roundDown(addr + data_size - 1, block_size); assert(split_addr <= addr || split_addr - addr < block_size); - // This will need a new way to tell if it's hooked up to a cache or not. - if (req->isUncacheable()) - recordEvent("Uncached Write"); - T *dataP = new T; *dataP = TheISA::htog(data); _status = DTBWaitResponse; -- cgit v1.2.3 From 6b293c73fd19b73758547e1bfbe38a23d1800747 Mon Sep 17 00:00:00 2001 From: "Timothy M. Jones" Date: Thu, 25 Mar 2010 12:43:52 +0000 Subject: CPU: Added comments to address translation classes. --- src/cpu/translation.hh | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-) (limited to 'src/cpu') diff --git a/src/cpu/translation.hh b/src/cpu/translation.hh index 33e810710..983a748cf 100644 --- a/src/cpu/translation.hh +++ b/src/cpu/translation.hh @@ -35,6 +35,16 @@ #include "sim/tlb.hh" +/** + * This class captures the state of an address translation. A translation + * can be split in two if the ISA supports it and the memory access crosses + * a page boundary. In this case, this class is shared by two data + * translations (below). Otherwise it is used by a single data translation + * class. When each part of the translation is finished, the finish + * function is called which will indicate whether the whole translation is + * completed or not. There are also functions for accessing parts of the + * translation state which deal with the possible split correctly. + */ class WholeTranslationState { protected: @@ -50,7 +60,10 @@ class WholeTranslationState uint64_t *res; BaseTLB::Mode mode; - /** Single translation state. */ + /** + * Single translation state. We set the number of outstanding + * translations to one and indicate that it is not split. + */ WholeTranslationState(RequestPtr _req, uint8_t *_data, uint64_t *_res, BaseTLB::Mode _mode) : outstanding(1), isSplit(false), mainReq(_req), sreqLow(NULL), @@ -60,7 +73,11 @@ class WholeTranslationState assert(mode == BaseTLB::Read || mode == BaseTLB::Write); } - /** Split translation state. */ + /** + * Split translation state. We copy all state into this class, set the + * number of outstanding translations to two and then mark this as a + * split translation. + */ WholeTranslationState(RequestPtr _req, RequestPtr _sreqLow, RequestPtr _sreqHigh, uint8_t *_data, uint64_t *_res, BaseTLB::Mode _mode) @@ -71,6 +88,13 @@ class WholeTranslationState assert(mode == BaseTLB::Read || mode == BaseTLB::Write); } + /** + * Finish part of a translation. If there is only one request then this + * translation is completed. If the request has been split in two then + * the outstanding count determines whether the translation is complete. + * In this case, flags from the split request are copied to the main + * request to make it easier to access them later on. + */ bool finish(Fault fault, int index) { @@ -89,6 +113,10 @@ class WholeTranslationState return outstanding == 0; } + /** + * Determine whether this translation produced a fault. Both parts of the + * translation must be checked if this is a split translation. + */ Fault getFault() const { @@ -102,36 +130,54 @@ class WholeTranslationState return NoFault; } + /** Remove all faults from the translation. */ void setNoFault() { faults[0] = faults[1] = NoFault; } + /** + * Check if this request is uncacheable. We only need to check the main + * request because the flags will have been copied here on a split + * translation. + */ bool isUncacheable() const { return mainReq->isUncacheable(); } + /** + * Check if this request is a prefetch. We only need to check the main + * request because the flags will have been copied here on a split + * translation. + */ bool isPrefetch() const { return mainReq->isPrefetch(); } + /** Get the physical address of this request. */ Addr getPaddr() const { return mainReq->getPaddr(); } + /** + * Get the flags associated with this request. We only need to access + * the main request because the flags will have been copied here on a + * split translation. + */ unsigned getFlags() { return mainReq->getFlags(); } + /** Delete all requests that make up this translation. */ void deleteReqs() { @@ -143,6 +189,16 @@ class WholeTranslationState } }; + +/** + * This class represents part of a data address translation. All state for + * the translation is held in WholeTranslationState (above). Therefore this + * class does not need to know whether the translation is split or not. The + * index variable determines this but is simply passed on to the state class. + * When this part of the translation is completed, finish is called. If the + * translation state class indicate that the whole translation is complete + * then the execution context is informed. + */ template class DataTranslation : public BaseTLB::Translation { @@ -163,6 +219,10 @@ class DataTranslation : public BaseTLB::Translation { } + /** + * Finish this part of the translation and indicate that the whole + * translation is complete if the state says so. + */ void finish(Fault fault, RequestPtr req, ThreadContext *tc, BaseTLB::Mode mode) -- cgit v1.2.3