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.

configs/common/CacheConfig.py (Diff revision 1)

This seems obviously specific to sst here

1. Curtis Dunham March 3, 2015, 4:16 p.m. (March 3, 2015, 4:16 p.m.)

   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.

configs/common/CacheConfig.py (Diff revision 1)

I assume this naming is also specific to sst

1. Curtis Dunham March 3, 2015, 4:16 p.m. (March 3, 2015, 4:16 p.m.)

   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.

src/mem/ExternalSlave.py (Diff revision 1)

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.

1. Curtis Dunham March 3, 2015, 4:16 p.m. (March 3, 2015, 4:16 p.m.)

   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.

Looks much nicer. Thanks for the changes!  Sorry for the long wait for the review.
Nothing big, just could use a couple more comments I think.

configs/common/CacheConfig.py (Diff revision 5)

a comment explaining the need/purpose for this class would be nice

Show all issues

configs/common/CacheConfig.py (Diff revision 5)

please add a comment here explaining that there is an assumption being made on how the external memory system names its ports

Show all issues

ext/sst/tests/test6_arm_4c.py (Diff revision 5)

so does this require you to edit the file to replace "/XXX/abs-path-to"?  couldn't this path be read from an environment variable, or perhaps you could leave it as a relative path so it would work if you ran from the "gem5" directory?

Show all issues

1. Curtis Dunham March 27, 2015, 3:32 p.m. (March 27, 2015, 3:32 p.m.)

   Yes, it does.  It can work as relative; I will change it to that.

Ship It!