----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/467/#review837 -----------------------------------------------------------
Generally this looks pretty good. I think your changes are correct except the way you dealt with t1 and t2 being switched in JMP_FAR_I. They're reversed like you said, but JMP_FAR_M and JMP_FAR_P and jmpFarWork match and should be left alone. t1 and t2 in JMP_FAR_I should be exchanged instead. JMP_FAR_M and JMP_FAR_P are using far pointers in memory, normally addressed in the first case and RIP relative in the second. They calculate the address of the start of the pointer and put it in t1, use that to find the selector which is farther into memory and put that in t2, and then finally overwrite t1 with the offset part of the pointer. I've historically had trouble with the ordering of the components of far pointers so you should probably double check that. I also suggest a temporary variable as described below. Aside from the actual code, your commit message is good, but with mercurial the first line is considered a summary and will be displayed by itself in some circumstances. Your first line looks like a summary, but since there's a newline in the middle it'll get cut off. If that's just an artifact of review board then never mind. Also, even though these are small and related changes, they're still separate changes that should probably go in separate patches and be committed separately. It's reasonable to put them all on review board together to make life easier for reviewers (aka me), but when these go in we'll want to split them up. src/arch/x86/tlb.cc <http://reviews.m5sim.org/r/467/#comment1194> !entry->writable && (inUser || cr0.wp) is used here and in the if below. This would be easier to read if that was in a temporary variable, maybe called badWrite. Nate would probably yell at me that it should be bad_write because it's local. He'd be right, but that would be inconsistent with the existing code. - Gabe On 2011-02-04 05:28:39, Tim Harris wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/467/ > ----------------------------------------------------------- > > (Updated 2011-02-04 05:28:39) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > X86: ISA bug fixes identified when bringing up Barrelfish > on M5. (#1) During iret access LDT/GDT at CPL0 rather than > after transition to user mode (if I'm reading the Intel IA-64 > architecture spec correctly, the contents of the descriptor > table are read before the CPL is updated). (#2) During > JMP_FAR_I, use srl to extract the segment selector from the > top of the destination. (#3) Switch use of t1 and t2 inputs > in jmpFarWork (which was inconsistent with call from JMP_FAR_I) > (#4) During SYSCALL_64, use dataSize=8 when handling new rip > (ref http://www.intel.com/Assets/PDF/manual/253668.pdf 5.8.8 > IA32_LSTAR is a 64-bit address), (#5) If cr0.wp ("write protect" > bit) is clear then do not generate page faults when writing to > write-protected pages in kernel mode. > > I'm not sure I've got the correct fix for #3. I see there are > other calls into jmpFarWork from JMP_FAR_M&P. I'm not sure > I understand what these are doing, so I'm worried that I've > either missed consequential changes to the use of t1/t2 > somewhere (or that the correct fix is to modify JMP_FAR_I > rather tahn jmpFarWork). > > > Diffs > ----- > > > src/arch/x86/isa/insts/general_purpose/control_transfer/interrupts_and_exceptions.py > 4afd05b9485e > src/arch/x86/isa/insts/general_purpose/control_transfer/jump.py > 4afd05b9485e > src/arch/x86/isa/insts/general_purpose/system_calls.py 4afd05b9485e > src/arch/x86/tlb.cc 4afd05b9485e > > Diff: http://reviews.m5sim.org/r/467/diff > > > Testing > ------- > > > Thanks, > > Tim > >
_______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
