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