----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5188 -----------------------------------------------------------
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 <http://reviews.gem5.org/r/2312/#comment4744> 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 " src/mem/multi_level_page_table_impl.hh <http://reviews.gem5.org/r/2312/#comment4737> 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(). src/mem/page_table.hh <http://reviews.gem5.org/r/2312/#comment4738> 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. src/sim/Process.py <http://reviews.gem5.org/r/2312/#comment4745> 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" - Andreas Sandberg On July 11, 2014, 5:57 p.m., Alexandru Dutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2312/ > ----------------------------------------------------------- > > (Updated July 11, 2014, 5:57 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10253:359daed0c723 > --------------------------- > Multi-level page table class. > This is part of the larger effort of supporting virtualized execution in SE > mode. > > > Diffs > ----- > > src/arch/alpha/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/arm/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/mips/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/power/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/sparc/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/pagetable.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/pagetable_walker.cc c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/system.hh c625a3c51bac050879457e666dd83299a36d761b > src/mem/SConscript c625a3c51bac050879457e666dd83299a36d761b > src/mem/multi_level_page_table.hh PRE-CREATION > src/mem/multi_level_page_table.cc PRE-CREATION > src/mem/multi_level_page_table_impl.hh PRE-CREATION > src/mem/page_table.hh c625a3c51bac050879457e666dd83299a36d761b > src/sim/Process.py c625a3c51bac050879457e666dd83299a36d761b > src/sim/SConscript c625a3c51bac050879457e666dd83299a36d761b > src/sim/process.hh c625a3c51bac050879457e666dd83299a36d761b > src/sim/process.cc c625a3c51bac050879457e666dd83299a36d761b > > Diff: http://reviews.gem5.org/r/2312/diff/ > > > Testing > ------- > > Regressions passed. > > > Thanks, > > Alexandru Dutu > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
