SIMPLE TIMING: when a request is NO_ACCESS (x86 CDA microinstruction), TimingSimpleCPU::completeDataAccess must still complete
Review Request #65 - Created July 27, 2010 and discarded
SIMPLE TIMING: when a request is NO_ACCESS (x86 CDA microinstruction), TimingSimpleCPU::completeDataAccess must still complete ./cpu/simple/timing.cc: fix for x86 CDA microop - since CDA doesn't read or update memory, completeDataAccess needs to handle the case where the current status of the CPU is _status = Running caused by a request NO_ACCESS This change is RE: Booting Linux, X86_FS Timing CPU (http://firstname.lastname@example.org/msg07290.html) Gabe Black: "The assert is, as you said, from NO_ACCESS skipping the call out to the memory system and going right to the code that finishes off execution of that instruction, surprising that code by never having left the Running state. Under any other circumstance, though, the CPU shouldn't be in the Running state, and if we just added that to the assert we wouldn't catch those bugs. What I think would be a better fix is to move the assert (but not the assignment to _status) up above the code that aggregates the components of a split packet and add pkt->req->getFlags().isSet(Request::NO_ACCESS) or something similar to the or. This isn't perfect because it asserts every time the function is called and not just once all the fragments (should be only two) are gathered, but it's safer and the overhead should be minimal." This change seems to have fixed the problem for X86_FS. Since no other architectures use the request NO_ACCESS flag, it is unlikely they will be impacted, though they still need to be tested.
Posted (July 27, 2010, 10:05 a.m.)
This looks good, except that your line needs to wrap at 80 characters. Also, I don't know if you intend for your review summary to be used directly as the change description, but if you do please be sure that the first line is a complete sentence (ie doesn't end on line 2+) which summarizes the change. It would be nice if it wasn't much longer than 80 characters either, but I've cheated on that myself occasionally.
Posted (July 27, 2010, 10:16 a.m.)
src/cpu/simple/timing.cc (Diff revision 1)
Need to wrap at < 80 columns.