Re: [gem5-dev] Review Request 2312: Mem: adding a multi-level page table class
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5312 --- Ship it! 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. - Andreas Sandberg On Aug. 25, 2014, 10:08 p.m., Alexandru Dutu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- (Updated Aug. 25, 2014, 10:08 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10264:5cf3d07a2e8b --- Mem: adding a multi-level page table class This patch defines a multi-level page table class that stores the page table in system memory, consistent with ISA specifications. In this way, cpu models that use the actual hardware to execute (e.g. KvmCPU), are able to traverse the page table. Diffs - 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 c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/page_table.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/se_translating_port_proxy.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 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: Mem: adding a multi-level page table class
On Aug. 14, 2014, 9:28 a.m., Andreas Sandberg wrote: src/mem/multi_level_page_table.hh, line 57 http://reviews.gem5.org/r/2312/diff/2/?file=40429#file40429line57 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.) Alexandru Dutu wrote: I have issues with the header files and cyclic dependencies. We'll see if I can get a version with inheritance. 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. - Alexandru --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5245 --- On Aug. 25, 2014, 9:08 p.m., Alexandru Dutu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- (Updated Aug. 25, 2014, 9:08 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10264:5cf3d07a2e8b --- Mem: adding a multi-level page table class This patch defines a multi-level page table class that stores the page table in system memory, consistent with ISA specifications. In this way, cpu models that use the actual hardware to execute (e.g. KvmCPU), are able to traverse the page table. Diffs - 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 c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/page_table.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/se_translating_port_proxy.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 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: Mem: adding a multi-level page table class
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- (Updated Aug. 25, 2014, 9:08 p.m.) Review request for Default. Changes --- Addressed the opened issues. It turns out refactoring with a base abstract class for the page table is even more complicated than I thought initially, so the class hierarchy is remains the same. Repository: gem5 Description (updated) --- Changeset 10264:5cf3d07a2e8b --- Mem: adding a multi-level page table class This patch defines a multi-level page table class that stores the page table in system memory, consistent with ISA specifications. In this way, cpu models that use the actual hardware to execute (e.g. KvmCPU), are able to traverse the page table. Diffs (updated) - 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 c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/page_table.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/se_translating_port_proxy.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 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: Mem: adding a multi-level page table class
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5245 --- src/mem/multi_level_page_table.hh http://reviews.gem5.org/r/2312/#comment4786 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... :) src/mem/multi_level_page_table.hh http://reviews.gem5.org/r/2312/#comment4784 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.) src/mem/multi_level_page_table.hh http://reviews.gem5.org/r/2312/#comment4787 Constant? src/mem/multi_level_page_table.hh http://reviews.gem5.org/r/2312/#comment4788 Constant? src/mem/multi_level_page_table_impl.hh http://reviews.gem5.org/r/2312/#comment4789 These should probably go into the initialization list. src/mem/page_table.hh http://reviews.gem5.org/r/2312/#comment4783 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... 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! - Andreas Sandberg On July 28, 2014, 11:29 p.m., Alexandru Dutu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- (Updated July 28, 2014, 11:29 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10264:49232146d7c8 --- Mem: adding a multi-level page table class This patch defines a multi-level page table class that stores the page table in system memory, consistent with ISA specifications. In this way, cpu models that use the actual hardware to execute (e.g. KvmCPU), are able to traverse the page table. Diffs - 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 c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/page_table.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/se_translating_port_proxy.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 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: Mem: adding a multi-level page table class
On Aug. 14, 2014, 9:28 a.m., Andreas Sandberg wrote: src/mem/page_table.hh, line 71 http://reviews.gem5.org/r/2312/diff/2/?file=40432#file40432line71 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... Not sure why these got modified. Thanks for pointing it out. On Aug. 14, 2014, 9:28 a.m., Andreas Sandberg wrote: src/mem/multi_level_page_table.hh, line 54 http://reviews.gem5.org/r/2312/diff/2/?file=40429#file40429line54 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... :) 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. On Aug. 14, 2014, 9:28 a.m., Andreas Sandberg wrote: src/mem/multi_level_page_table.hh, line 57 http://reviews.gem5.org/r/2312/diff/2/?file=40429#file40429line57 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.) I have issues with the header files and cyclic dependencies. We'll see if I can get a version with inheritance. On Aug. 14, 2014, 9:28 a.m., Alexandru Dutu wrote: 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! Thank you Andreas! Working on getting an inheritance version. I expect the performance penalty to be marginal. - Alexandru --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/#review5245 --- On July 28, 2014, 10:29 p.m., Alexandru Dutu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2312/ --- (Updated July 28, 2014, 10:29 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10264:49232146d7c8 --- Mem: adding a multi-level page table class This patch defines a multi-level page table class that stores the page table in system memory, consistent with ISA specifications. In this way, cpu models that use the actual hardware to execute (e.g. KvmCPU), are able to traverse the page table. Diffs - 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 c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/page_table.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/mem/se_translating_port_proxy.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.hh c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 src/sim/process.cc c00b5ba43967e7e48a28b7ddc48c9f4afaf2ab76 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
