> On 2011-01-07 04:45:16, Gabe Black wrote: > > I think you forgot some files so this I suppose this is only a partial > > review. It looks like this could be cleanly split into three different > > changes, and the fact that you have sub-commit messages for those > > independent parts suggests that you already knew that. It's best to split > > it up and commit it as separate changes.
This is one of Joel's patches, so I don't know for sure, but I believe the description is dated. The pagetable_walker and tlb modifications were moved to different patches, so I think Joel came to the same realization as you did. > On 2011-01-07 04:45:16, Gabe Black wrote: > > src/arch/x86/vtophys.cc, line 73 > > <http://reviews.m5sim.org/r/385/diff/1/?file=9054#file9054line73> > > > > This is very suspicious. The request size was set to 0 when you > > constructed the request object, so this is anding the original address with > > -1. That doesn't do anything, so you're really just oring the addresses > > together. The TLB will already have taken care of any page offset/page > > number munging that you need. Actually, this whole function is suspect (not > > because of your code) since there's no guarantee code/data and/or different > > forms of data will be translated the same, or that flags aren't important. I agree, something seems off here. However, I'll let Joel respond before changing it. At least there needs to be a comment explaining why this calculation is necessary. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/385/#review642 ----------------------------------------------------------- On 2011-01-06 15:59:24, Brad Beckmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/385/ > ----------------------------------------------------------- > > (Updated 2011-01-06 15:59:24) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > x86: page table walker functional support > > src/arch/x86/pagetable_walker.hh: Added method to functionally walk page table > src/arch/x86/pagetable_walker.cc: Added method to functionally walk page table > src/arch/x86/tlb.cc: Added method to return pointer to walker > src/arch/x86/tlb.hh: Added method to return pointer to walker > src/arch/x86/vtophys.cc: Calls walker to look up virt. to phys. page mapping > > > Diffs > ----- > > src/arch/x86/vtophys.cc 9f9e10967912 > > Diff: http://reviews.m5sim.org/r/385/diff > > > Testing > ------- > > > Thanks, > > Brad > >
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
