----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/65/#review108 -----------------------------------------------------------
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. - Gabe On 2010-07-27 16:11:05, Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/65/ > ----------------------------------------------------------- > > (Updated 2010-07-27 16:11:05) > > > Review request for Default. > > > Summary > ------- > > 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://www.mail-archive.com/[email protected]/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. > > > Diffs > ----- > > src/cpu/simple/timing.cc a75564db03c3 > > Diff: http://reviews.m5sim.org/r/65/diff > > > Testing > ------- > > > Thanks, > > Joel > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
