> On 2011-01-07 04:45:16, Gabe Black wrote: > > src/arch/x86/vtophys.cc, line 58 > > <http://reviews.m5sim.org/r/385/diff/1/?file=9054#file9054line58> > > > > Better wording might be "Need access to page tables."
I like that change > On 2011-01-07 04:45:16, Gabe Black wrote: > > src/arch/x86/vtophys.cc, line 70 > > <http://reviews.m5sim.org/r/385/diff/1/?file=9054#file9054line70> > > > > Having a temporary variable here seems unnecessary unless it's to > > prevent having to wrap the next line. It's not a big deal, though. As far as I can tell, convention in ALL other code is to store the fault as a temporary variable, even if it could simply be pushed into the if-clause. > 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. > > Brad Beckmann wrote: > 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. The size field of the request is set in the functional portion of Walker::WalkerState::startWalk in my other patch for review. The physical address that is returned from vtophys needs to include the offset into the page, which in x86 can have multiple different sizes. The page table contains the information about the page size, so it needs to be passed in the request object through startFunctional(). - Joel ----------------------------------------------------------- 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
