Could you make sure that every file that is added has an appropriate license header?
You may want to exclude some files that are not needed...

Looks good. Two questions: First, how about the compile-time constants through #defines. Is there a way we can make these run-time configurable? The cacheline size is one prime example. Second, there still appears to be files without a license header.

ext/mc2lib/contrib/mcversi/m5_host_support.h (Diff revision 5)

Is there any way we could make these non-compile time constants? Ultimately it would be good if all of these was parameters for the wrapper.

Show all issues

1. Marco Elver June 4, 2016, 9:09 a.m. (June 4, 2016, 9:09 a.m.)

   Not quite sure what you mean here "parameters for the wrapper". All of this is not used by gem5 directly but the guest workload that should be compiled by a user for their respective guest OS, etc. The way this is meant to be set is by defining this at compile time via "-DCACHELINE_SIZE=XX".
   Also note that this is merely an optimization, that is used 1) during initialization 2) during cache flushing. For 2) with the provided host-side implementation (other patch), this is not used even as the host passes the used addresses to the guest (CACHELINES_SIZE is used as a fallback option if this is not provided). Since not all hosts may be able to do so (say something other than gem5), the workload itself is written to support all options, and hence this can be safely ignored for the gem5 use-case.

2. Andreas Hansson June 4, 2016, 10:18 a.m. (June 4, 2016, 10:18 a.m.)

   Thanks for the clarification. If this is not being included in gem5 compilation it's fine. My mistake.

ext/mc2lib/contrib/mcversi/m5_host_support.h (Diff revision 5)

does x86 support flushing? both classic and ruby?

Show all issues

1. Marco Elver June 4, 2016, 9:09 a.m. (June 4, 2016, 9:09 a.m.)

   No, I had to implement this for each protocol that I used this with (it was a hacky solution and Ruby specific).
   Cache flushing is not critical (does not affect correctness), but is an optimization, as otherwise cache pollution could result in the same test exhibiting too different behaviour and the test evolution may not be as effective. For someone using this seriously, they should probably implement it for their protocol.

ext/mc2lib/contrib/mcversi/m5_host_support.h (Diff revision 5)

should we just get it done rather?

Show all issues

1. Marco Elver June 4, 2016, 9:09 a.m. (June 4, 2016, 9:09 a.m.)

   Cache flushing is not essential, but recommended (see note about x86 clflush). It would be nice to have, but at the moment I don't have time to chase up on how to implement this for ARMv7 (contributions welcome!).
   Note that even without this, I'm able to find a rather hard to find bug in ARM+Classic (see http://www.mail-archive.com/gem5-dev@gem5.org/msg18940.html ).

2. Andreas Hansson June 4, 2016, 10:18 a.m. (June 4, 2016, 10:18 a.m.)

   Could you elaborate on what it has to accomplish (to make it easier for someone to actually fill in the blanks)?

3. Marco Elver June 5, 2016, 7:36 a.m. (June 5, 2016, 7:36 a.m.)

   It should invalidate the cache line that contains the specified address from all levels of the cache hierarchy and any other buffer.
   I looked into this a while ago, and found that flushing the D-cache only is easy enough. It seems to get hairy when wanting to invalidate lower levels as well (L2, L3, ... Ln).

ext/mc2lib/contrib/mcversi/run-10-8KB.sh (Diff revision 5)

Technically all of these need a license header.

Show all issues