----------------------------------------------------------- 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
