> 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. > > Joel Hestness wrote: > 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.
I'd call that coincidence and not convention, and definitely not enough to justify doing it here. Then again, you don't really need to justify it because it's not really wrong. > 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. > > Joel Hestness wrote: > 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(). So you're using the size field of the request object to pass back the size of the page from a functional page walk? I don't like that. It's pretty sneaky and I can easily imagine someone tripping over that perhaps expecting size to be what it's supposed to be or assuming it isn't anything, basically like Brad and I were. Since you're making up a functional function and it doesn't have to make sense in other contexts, just go ahead and add a page size parameter that's pass by reference. Also, if the request doesn't do anything but communicate an address for this function, go ahead and make it an address. If it needs to be a request to prevent the walker's internals from getting too messy then it's fine to leave it that way. - Gabe ----------------------------------------------------------- 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 [email protected] http://m5sim.org/mailman/listinfo/m5-dev
