Re: [gem5-dev] Review Request 2312: Multi-level page table class.
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
Re: [gem5-dev] Review Request 2312: Multi-level page table class.
--- 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
Re: [gem5-dev] Review Request 2312: Multi-level page table class.
On July 14, 2014, 7: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. Also, please document what the architecture-specific page tables are supposed to do. A good place could be the PageTableBase class. - Andreas --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5188 --- 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
[gem5-dev] Review Request 2312: Multi-level page table class.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- 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
