> On Feb. 25, 2015, 4:32 p.m., Steve Reinhardt wrote: > > Took me longer to review this just because I'm not sure what to think of > > it. It's not pretty, but I don't have better ideas, so it's hard to object. > > > > One thing that bothers me is that, while it's noble to try and generalize > > things, despite the generic-sounding "--external-memory-system" switch, > > many of the low-level details seem very sst-specific (like > > 'port_type="sst"' as an obvious one). Would it be more appropriate just to > > call the option "--sst-memory"? > > > > Also, the ExternalCache thing looks like it should be a subclass, even > > though it's just a function. I can see how the function is more > > convenient, but making it a subclass would allow some better encapsulation > > of the cpu_side __getattr__/__setattr__ hack. > > > > These are just general thoughts though... I'm very interested in others' > > input.
There is some truth to this complaint. I think there is a little room for improvement, though. > On Feb. 25, 2015, 4:32 p.m., Steve Reinhardt wrote: > > configs/common/CacheConfig.py, line 130 > > <http://reviews.gem5.org/r/2668/diff/1/?file=43775#file43775line130> > > > > I assume this naming is also specific to sst Not here. The naming just has to agree with the configuration "on the other side", which in this case is the ext/sst/tests/test6_arm_4c.py in this patchset. However as long as any other simulator we hook up with is cool with the same naming, it doesn't need to be changed. > On Feb. 25, 2015, 4:32 p.m., Steve Reinhardt wrote: > > configs/common/CacheConfig.py, line 50 > > <http://reviews.gem5.org/r/2668/diff/1/?file=43775#file43775line50> > > > > This seems obviously specific to sst here Correct that port_type="sst" is SST specific because the ext/sst/Ext{Master,Slave} code registers a handler for handler_name "sst". Perhaps --external-memory-system could take a string argument that could be stuck here? We'd use --external-memory-system="sst" on the command line. > On Feb. 25, 2015, 4:32 p.m., Steve Reinhardt wrote: > > src/mem/ExternalSlave.py, line 57 > > <http://reviews.gem5.org/r/2668/diff/1/?file=43782#file43782line57> > > > > To me, this clearly straddles the line between "clever" and > > "horrifying" :). If nothing else, it would be nice to put these in an > > ExternalCache subclass. Clever/Horrifying... Yes. :) My concern here is that creating a new class doesn't seem to solve the problem. As best I can tell, you just end up moving this unsightly hack to the new class instead. But the value of not having to create new conditional statements to set either "port" (for external memory) or "cpu_side" (for gem5's internal caches) seems worth this bit of nastiness. - Curtis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2668/#review5914 ----------------------------------------------------------- On Feb. 20, 2015, 6:47 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2668/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2015, 6:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10709:a3b771cd744c > --------------------------- > config: Support full-system with SST's memory system > > This patch adds an example configuration in ext/sst/tests/ that allows > an SST/gem5 instance to simulate a 4-core AArch64 system with SST's > memHierarchy components providing all the caches and memories. > > > Diffs > ----- > > configs/common/CacheConfig.py c6cb94a14fea4c59780d73d1623d7031bcede6af > configs/common/FSConfig.py c6cb94a14fea4c59780d73d1623d7031bcede6af > configs/common/MemConfig.py c6cb94a14fea4c59780d73d1623d7031bcede6af > configs/common/Options.py c6cb94a14fea4c59780d73d1623d7031bcede6af > configs/example/fs.py c6cb94a14fea4c59780d73d1623d7031bcede6af > ext/sst/tests/test6_arm_4c.py PRE-CREATION > src/dev/arm/RealView.py c6cb94a14fea4c59780d73d1623d7031bcede6af > src/mem/ExternalSlave.py c6cb94a14fea4c59780d73d1623d7031bcede6af > > Diff: http://reviews.gem5.org/r/2668/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
