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.

src/arch/x86/vtophys.cc (Diff revision 1)

Better wording might be "Need access to page tables."

1. Joel Hestness Jan. 7, 2011, 9:03 a.m. (Jan. 7, 2011, 9:03 a.m.)

   I like that change

src/arch/x86/vtophys.cc (Diff revision 1)

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.

1. Joel Hestness Jan. 7, 2011, 9:07 a.m. (Jan. 7, 2011, 9:07 a.m.)

   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.

2. Gabe Black Jan. 9, 2011, 11:38 a.m. (Jan. 9, 2011, 11:38 a.m.)

   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.

src/arch/x86/vtophys.cc (Diff revision 1)

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.

1. Brad Beckmann Jan. 7, 2011, 7:02 a.m. (Jan. 7, 2011, 7:02 a.m.)

   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.

2. Joel Hestness Jan. 7, 2011, 9:31 a.m. (Jan. 7, 2011, 9:31 a.m.)

   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().

3. Gabe Black Jan. 9, 2011, 11:43 a.m. (Jan. 9, 2011, 11:43 a.m.)

   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.

src/arch/x86/vtophys.cc (Diff revision 2)

I really don't like this page size in request size hack. It's confusing and obfuscates what's going on, and it changes the meaning of a well defined field in the request object. There's really no good reason for it, and I'd love to see it go.