Other than fixing one comment, this patch looks good to me.

By the way, have you tested an existing directory protocol with the memtester using functional accesses?  I can't remember if the regression tester turns on functional accesses for the ruby-memtester, and I suspect that even if it does, it may not run long enough to catch all bugs.  I just want to make sure that nothing breaks with this change, though I don't anticipate a problem.

src/mem/ruby/system/RubyPort.cc (Diff revision 1)

Could you clarify this comment a bit.  What is the second if/else statement you are referring to?  Also I don't believe you can say that if you reach the else statement, that the block exists in the cache hierarchy.  For directory protocols, one could reach the else statement with memory containing the only valid copy.

1. Lisa Hsu Aug. 17, 2011, 3 a.m. (Aug. 17, 2011, 3 a.m.)

   I suppose the comment isn't clear.  It's not meant to mean that there is a *valid* copy in the cache hierarchy, just that it exists in some form, since the if statement should cover the case where it doesn't exist at all (is Invalid or Not Present) throughout all cache controllers but exists in memory.  I also didn't mean there's a second if/else, just the two cases separated by the if/else.  The two cases (if and else) are separated because if you always consider memory a readable block, then in the case where it is stale and the loop gets to memory first, you don't want it to be the read value, but in the case that it doesn't exist in any of the cache controllers, you definitely want to return the block in memory. In other words, you only want to return the value in memory sometimes, so you have to separate it into the if/else, which should not be collapsed into one large statement.  I'm not sure if I'm answering your question - maybe you can drop by the cube if I'm missing your point.

src/mem/ruby/system/RubyPort.cc (Diff revision 1)

Thanks for adding the spacing!  Much easier to read.