> On July 14, 2014, 5:36 p.m., Andreas Sandberg wrote: > > 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. > > > > Andreas Sandberg wrote: > Also, please document what the architecture-specific page tables are > supposed to do. A good place could be the PageTableBase class.
Thank you for your review! Breaking this patch into two makes more sense. Should I close this review request and start 2 new ones? > On July 14, 2014, 5:36 p.m., Andreas Sandberg wrote: > > src/mem/multi_level_page_table.hh, line 50 > > <http://reviews.gem5.org/r/2312/diff/1/?file=40368#file40368line50> > > > > 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 > > " > > Well pointed out, I will change this. > On July 14, 2014, 5:36 p.m., Andreas Sandberg wrote: > > src/sim/Process.py, line 41 > > <http://reviews.gem5.org/r/2312/diff/1/?file=40372#file40372line41> > > > > 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" > > I agree, a detailed message won't hurt. > On July 14, 2014, 5:36 p.m., Andreas Sandberg wrote: > > src/mem/multi_level_page_table_impl.hh, line 168 > > <http://reviews.gem5.org/r/2312/diff/1/?file=40370#file40370line168> > > > > 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(). Actually this shouldn't have been included in this patch. > On July 14, 2014, 5:36 p.m., Andreas Sandberg wrote: > > src/mem/page_table.hh, line 54 > > <http://reviews.gem5.org/r/2312/diff/1/?file=40371#file40371line54> > > > > 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. 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. - Alexandru ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5188 ----------------------------------------------------------- On July 11, 2014, 3: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, 3: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
