In general, I like the idea of having a proper, architected, page table in SE-mode. Long-term, this could hopefully mean that we can get rid of many of the differences between SE- and FS-mode.

High-level comments:

* Write a proper commit message (see http://www.m5sim.org/Commit_Access). Specifically, include a short one-line summary and a longer description of what the patch does and why. There is currently now way of telling what this patch intends to accomplish.

* Don't set the execute bit on src/sim/SConscript.

* Split into an architecture-agnostic patch and an x86-specific patch.

src/mem/multi_level_page_table.hh (Diff revision 1)

Please focus on what the class does instead of where it is intended to be used. I'd write the description along these lines:

"This class implements an in-memory page table that follows the x86 page table specification. This can used instead of the PageTable class in SE mode to allow CPU models (mainly the X86KvmCPU) to do a normal page table walk.

@see Link to suitable documentation
"

Show all issues

1. Alexandru Dutu July 16, 2014, 9:46 p.m. (July 16, 2014, 9:46 p.m.)

   Well pointed out, I will change this.

src/mem/multi_level_page_table_impl.hh (Diff revision 1)

This method is identical to map() with the exception of the parameter to setPTEFields. Please create a separate helper method that is used by moth map() and mapNotPresent().

Show all issues

1. Alexandru Dutu July 16, 2014, 9:46 p.m. (July 16, 2014, 9:46 p.m.)

   Actually this shouldn't have been included in this patch.

src/mem/page_table.hh (Diff revision 1)

Since you need to make the PageTable class a base class, could you also take the opportunity to redesign it slightly to make it more obvious what's going on here?

Specifically, I'd like to see a design with these classes:

PageTableBase - Should declare the interface used by all page table implementations. Methods like map/remap/unmap/etc. should be purely virtual. This might be a good place to document what the arch-specific page tables are meant to do

SEPageTable (or some other sensible name) - Inherits from PageTableBase and implements the page table functionality currently in PageTable.

NoArchPageTable (& and arch-specific ones) - Inherit from PageTableBase.

Show all issues

1. Alexandru Dutu July 16, 2014, 9:46 p.m. (July 16, 2014, 9:46 p.m.)

   Agree, I have a couple of redesign ideas to try out before changing the current one. It seems that the major difference between PageTable and MultiLevelPageTable is the way the page table is stored and traversed. So map/remap/unmap/etc. can be shared and not duplicated as long there is a common interface to access the page table storage.

src/sim/Process.py (Diff revision 1)

Re-phrase this, I hardly get what this means even after reading the rest of the patch. What your patch is doing is really to maintain an in-memory version of the page table in the architecture-specific format. Make sure that's clear from the description. You could use something like this:

"maintain an in-memory version of the page table in an architecture-specific format"

Show all issues

1. Alexandru Dutu July 16, 2014, 9:46 p.m. (July 16, 2014, 9:46 p.m.)

   I agree, a detailed message won't hurt.

src/mem/multi_level_page_table.hh (Diff revision 2)

It'd be great if you could write a short description of how the page table works. I know that this is sort of obvious from how page tables are usually implemented, but having it documented means that it is easier to reuse the code.

Pseudo code would be nice... :)

Show all issues

1. Alexandru Dutu Aug. 14, 2014, 5:32 p.m. (Aug. 14, 2014, 5:32 p.m.)

   I gave the pseudo code in one paragraph. However, it took a lot more paragraphs to explain how it works. I tried to be a bit more descriptive and not just "it's a radix tree with parts of the virtual address used to traverse the tree".

src/mem/multi_level_page_table.hh (Diff revision 2)

In general, I prefer having a abstract base classes for interfaces since that makes documentation and compile-time testing easier and the code becomes more self-documenting. I don't think the performance impact in this case would be noticeable.

Could you either refactor the code so that the ISA code inherits from MultiLevelPageTable instead of using the template? (Or at the very least document the ISAOps interface.)

Show all issues

1. Alexandru Dutu Aug. 14, 2014, 5:32 p.m. (Aug. 14, 2014, 5:32 p.m.)

   I have issues with the header files and cyclic dependencies. We'll see if I can get a version with inheritance.

2. Alexandru Dutu Aug. 27, 2014, 7:45 a.m. (Aug. 27, 2014, 7:45 a.m.)

   I have tried to refactor the page table code to have an abstract base, and remembered trying this in the past. It seems that this variant requires a lot more code to be changed because of the introduced cyclic dependencies (page_table.hh -> tlb.hh -> arch/x86/pagetable.hh -> multi_level_page_table.hh). TLBEntry and PageTableEntry have to be move from arch/x86/pagetable.hh etc. In the end there are still thousands of lines of unresolved BitUnion64 and TheISA because of the way headers are included in sim/process.cc.

src/mem/multi_level_page_table.hh (Diff revision 2)

Constant?

Show all issues

src/mem/multi_level_page_table.hh (Diff revision 2)

Constant?

Show all issues

src/mem/multi_level_page_table_impl.hh (Diff revision 2)

These should probably go into the initialization list.

Show all issues

src/mem/page_table.hh (Diff revision 2)

Any particular reason why these aren't const any more? I kinda like having constants declared as such since that means the compiler brings out the stick when I screw up...

Show all issues

1. Alexandru Dutu Aug. 14, 2014, 5:32 p.m. (Aug. 14, 2014, 5:32 p.m.)

   Not sure why these got modified. Thanks for pointing it out.

The issues above are really minor issues. The only major thing is that I'd like you to consider refactoring the code slightly to get rid of the template. Keep up the good work!

Looks good. Thanks for addressing the issues I raised earlier!

Please make sure that "Mem:" is all lower case when you commit so it complies with the commit guidelines.