----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/611/#review1330 -----------------------------------------------------------
This patch has all the functionality we need. Also I really like the inheritance structure. I have a few minor questions and suggestion below. The only significant change/question I have is regarding the ruby system pointer. Right now the pointer is passed all throughout the ruby configuration process. Instead of doing this, can you leverage the parent.any functionality? I know it can be confusing to avoid cycles in configuration, while leveraging parent.any, but it seems that the current system should work. You already do the back pointer setting in C++ through the register pointer callbacks. configs/ruby/Ruby.py <http://reviews.m5sim.org/r/611/#comment1771> Why are you exec the strings instead of just directly including the commands? src/mem/protocol/MOESI_CMP_directory-dir.sm <http://reviews.m5sim.org/r/611/#comment1769> Why are you commenting out this check? It seems to me that we can still include it, but if we cannot, we should remove the line. Don't comment it out. src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/611/#comment1770> Same here? Why comment this out? src/mem/ruby/system/AbstractMemory.hh <http://reviews.m5sim.org/r/611/#comment1773> It seems that the function "makeFunctionalAccess" is not turning a request into a functional access, but instead is actually performing the functional access. How about we split these functions to "doFunctionalRead" and "doFunctionalWrite"? That seems more appropriate and is more consistent with the rubyPort functions. src/mem/ruby/system/AbstractMemory.py <http://reviews.m5sim.org/r/611/#comment1772> Can you turn this into a parent any call and remove the need to pass around the ruby system pointer to all functions? src/mem/ruby/system/CacheMemory.cc <http://reviews.m5sim.org/r/611/#comment1774> It is possible to have a "-1" value here? It seems that one only calls this function if the getPermission function returns Read_Only or Read_Write? Therefore aren't you guaranteed to find the block? src/mem/ruby/system/DirectoryMemory.cc <http://reviews.m5sim.org/r/611/#comment1775> Similar thing here. It is possible to have the block not present here? It seems that one only calls this function if the getPermission function returns Read_Only or Read_Write? Therefore aren't you guaranteed to find the block? - Brad On 2011-06-12 14:55:53, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/611/ > ----------------------------------------------------------- > > (Updated 2011-06-12 14:55:53) > > > Review request for Default. > > > Summary > ------- > > Ruby: Add support for functional accesses > This patch is meant for implementing functional access support in Ruby. > Currently only the M5Port of RubyPort supports functional accesses. The > support for functional through the PioPort will be added as a separate > patch. The patch has been tested for MI, MOESI directory, MOESI hammer > and MESI directory protocols. It seems that MOESI token protocol cannot > support functional accesses with it current implementation, there seems > to be some problem with the L2 cache controller. > > > Diffs > ----- > > configs/example/ruby_direct_test.py ac4da9f8ea80 > configs/example/ruby_fs.py ac4da9f8ea80 > configs/example/ruby_mem_test.py ac4da9f8ea80 > configs/example/ruby_network_test.py ac4da9f8ea80 > configs/example/ruby_random_test.py ac4da9f8ea80 > configs/ruby/MESI_CMP_directory.py ac4da9f8ea80 > configs/ruby/MI_example.py ac4da9f8ea80 > configs/ruby/MOESI_CMP_directory.py ac4da9f8ea80 > configs/ruby/MOESI_CMP_token.py ac4da9f8ea80 > configs/ruby/MOESI_hammer.py ac4da9f8ea80 > configs/ruby/Ruby.py ac4da9f8ea80 > src/cpu/testers/memtest/memtest.hh ac4da9f8ea80 > src/cpu/testers/memtest/memtest.cc ac4da9f8ea80 > src/mem/packet.hh ac4da9f8ea80 > src/mem/packet.cc ac4da9f8ea80 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm ac4da9f8ea80 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm ac4da9f8ea80 > src/mem/protocol/MOESI_CMP_directory-dir.sm ac4da9f8ea80 > src/mem/protocol/MOESI_hammer-cache.sm ac4da9f8ea80 > src/mem/protocol/MOESI_hammer-dir.sm ac4da9f8ea80 > src/mem/ruby/network/Network.cc ac4da9f8ea80 > src/mem/ruby/network/Network.py ac4da9f8ea80 > src/mem/ruby/profiler/Profiler.cc ac4da9f8ea80 > src/mem/ruby/profiler/Profiler.py ac4da9f8ea80 > src/mem/ruby/recorder/Tracer.cc ac4da9f8ea80 > src/mem/ruby/recorder/Tracer.py ac4da9f8ea80 > src/mem/ruby/slicc_interface/AbstractController.hh ac4da9f8ea80 > src/mem/ruby/slicc_interface/AbstractController.cc PRE-CREATION > src/mem/ruby/slicc_interface/Controller.py ac4da9f8ea80 > src/mem/ruby/slicc_interface/SConscript ac4da9f8ea80 > src/mem/ruby/system/AbstractMemory.hh PRE-CREATION > src/mem/ruby/system/AbstractMemory.cc PRE-CREATION > src/mem/ruby/system/AbstractMemory.py PRE-CREATION > src/mem/ruby/system/Cache.py ac4da9f8ea80 > src/mem/ruby/system/CacheMemory.hh ac4da9f8ea80 > src/mem/ruby/system/CacheMemory.cc ac4da9f8ea80 > src/mem/ruby/system/DirectoryMemory.hh ac4da9f8ea80 > src/mem/ruby/system/DirectoryMemory.cc ac4da9f8ea80 > src/mem/ruby/system/DirectoryMemory.py ac4da9f8ea80 > src/mem/ruby/system/RubyPort.hh ac4da9f8ea80 > src/mem/ruby/system/RubyPort.cc ac4da9f8ea80 > src/mem/ruby/system/RubySystem.py ac4da9f8ea80 > src/mem/ruby/system/SConscript ac4da9f8ea80 > src/mem/ruby/system/Sequencer.py ac4da9f8ea80 > src/mem/ruby/system/System.hh ac4da9f8ea80 > src/mem/ruby/system/System.cc ac4da9f8ea80 > src/mem/slicc/ast/MemberExprAST.py ac4da9f8ea80 > > Diff: http://reviews.m5sim.org/r/611/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
