Posted (Jan. 6, 2011, 8:45 p.m.)
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."
-
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.
-
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.
Posted (Jan. 6, 2011, 8:53 p.m.)
Review request changed
Updated (Jan. 10, 2011, 3:26 a.m.)
Summary: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 2 (+22) |
Posted (Jan. 16, 2011, 5:07 p.m.)
-
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.
