----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/943/#review1921 -----------------------------------------------------------
Looks pretty good... several comments below (some minor, a couple more significant ones), and there's also the pointer declaration style issue to iron out, but it looks pretty close. I think this is my last patch to review of your series, Andreas. I think there were only a couple (including this one) I didn't mark as "ship it"... those are the only ones I care to see again before you commit. Thanks for all your hard work! src/mem/fs_translating_port_proxy.hh <http://reviews.m5sim.org/r/943/#comment2420> Typo in "boundaries" in this and the next comment. Unless it's some variant British spelling I'm unaware of :). src/mem/fs_translating_port_proxy.hh <http://reviews.m5sim.org/r/943/#comment2426> Are these read/write/readGtoH/writeHtoG methods used anywhere? I didn't see them on VirtualPort and I don't recall any uses elsewhere in this patch. I'm concerned because we could get into cases where these are unaligned and cross page boundaries, and the current code doesn't deal with that. src/mem/fs_translating_port_proxy.hh <http://reviews.m5sim.org/r/943/#comment2421> If you add a private inline method like: Addr translate(Addr va) { if (_tc == NULL) { return TheISA::vtophys(address); } else { return TheISA::vtophys(_tc, address); } } then you could turn all of these functions into one-liners, and simplify a lot of the functions in the .cc file too. src/mem/fs_translating_port_proxy.cc <http://reviews.m5sim.org/r/943/#comment2423> I'd really like to see these done in mercurial as file renames rather than a combination of new and deleted files... e.g., this should be a rename of vport.cc not a new file. Similarly for translating_port.{cc,hh} -> se_translating_proxy.{cc,hh} src/mem/fs_translating_port_proxy.cc <http://reviews.m5sim.org/r/943/#comment2422> need space after the comma here (and several similar places) src/mem/fs_translating_port_proxy.cc <http://reviews.m5sim.org/r/943/#comment2424> This comment is totally outside the scope of this patch, but seeing this code again reminds me how much it annoys me, mainly because these are global functions that start with a capital (predating, but way in violation of, the style guide). It seems to me that these might be best as methods on ThreadContext, and then they could eventually be usable in SE mode too. src/mem/port_proxy.cc <http://reviews.m5sim.org/r/943/#comment2425> I'd be fine with inlining these into port_proxy.hh and getting rid of this file... seems like overkill to have a separate .cc, and they're short enough it's probably worth inlining them anyway. - Steve On 2012-01-11 03:29:50, Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/943/ > ----------------------------------------------------------- > > (Updated 2012-01-11 03:29:50) > > > Review request for Default. > > > Summary > ------- > > MEM: Add port proxies instead of non-structural ports > > Port proxies are used to replace non-structural ports, and thus enable > all ports in the system to correspond to a structural entity. This has > the advantage of accessing memory through the normal memory subsystem > and thus allowing any constellation of distributed memories, address > maps, etc. Most accesses are done through the "system port" that is > used for loading binaries, debugging etc. For the entities that belong > to the CPU, e.g. threads and thread contexts, they wrap the CPU data > port in a port proxy. > > The following replacements are made: > FunctionalPort > PortProxy > TranslatingPort > SETranslatingPortProxy > VirtualPort > FSTranslatingPortProxy > > > Diffs > ----- > > configs/common/FSConfig.py 508bbec99e58 > configs/example/se.py 508bbec99e58 > configs/ruby/Ruby.py 508bbec99e58 > src/arch/alpha/freebsd/system.cc 508bbec99e58 > src/arch/alpha/linux/process.cc 508bbec99e58 > src/arch/alpha/linux/system.hh 508bbec99e58 > src/arch/alpha/linux/system.cc 508bbec99e58 > src/arch/alpha/linux/threadinfo.hh 508bbec99e58 > src/arch/alpha/remote_gdb.cc 508bbec99e58 > src/arch/alpha/stacktrace.cc 508bbec99e58 > src/arch/alpha/system.hh 508bbec99e58 > src/arch/alpha/system.cc 508bbec99e58 > src/arch/alpha/tru64/process.cc 508bbec99e58 > src/arch/alpha/tru64/system.cc 508bbec99e58 > src/arch/alpha/utility.cc 508bbec99e58 > src/arch/alpha/vtophys.hh 508bbec99e58 > src/arch/alpha/vtophys.cc 508bbec99e58 > src/arch/arm/linux/process.cc 508bbec99e58 > src/arch/arm/linux/system.cc 508bbec99e58 > src/arch/arm/process.cc 508bbec99e58 > src/arch/arm/stacktrace.cc 508bbec99e58 > src/arch/arm/system.hh 508bbec99e58 > src/arch/arm/system.cc 508bbec99e58 > src/arch/arm/utility.cc 508bbec99e58 > src/arch/arm/vtophys.cc 508bbec99e58 > src/arch/mips/linux/process.cc 508bbec99e58 > src/arch/mips/linux/system.cc 508bbec99e58 > src/arch/mips/linux/threadinfo.hh 508bbec99e58 > src/arch/mips/stacktrace.cc 508bbec99e58 > src/arch/mips/utility.cc 508bbec99e58 > src/arch/power/linux/process.cc 508bbec99e58 > src/arch/power/process.cc 508bbec99e58 > src/arch/sparc/linux/syscalls.cc 508bbec99e58 > src/arch/sparc/process.cc 508bbec99e58 > src/arch/sparc/solaris/process.cc 508bbec99e58 > src/arch/sparc/system.hh 508bbec99e58 > src/arch/sparc/system.cc 508bbec99e58 > src/arch/sparc/utility.cc 508bbec99e58 > src/arch/sparc/vtophys.cc 508bbec99e58 > src/arch/x86/bios/intelmp.hh 508bbec99e58 > src/arch/x86/bios/intelmp.cc 508bbec99e58 > src/arch/x86/bios/smbios.hh 508bbec99e58 > src/arch/x86/bios/smbios.cc 508bbec99e58 > src/arch/x86/linux/syscalls.cc 508bbec99e58 > src/arch/x86/linux/system.cc 508bbec99e58 > src/arch/x86/process.cc 508bbec99e58 > src/arch/x86/stacktrace.cc 508bbec99e58 > src/arch/x86/system.cc 508bbec99e58 > src/base/loader/elf_object.hh 508bbec99e58 > src/base/loader/elf_object.cc 508bbec99e58 > src/base/loader/hex_file.hh 508bbec99e58 > src/base/loader/hex_file.cc 508bbec99e58 > src/base/loader/object_file.hh 508bbec99e58 > src/base/loader/object_file.cc 508bbec99e58 > src/base/remote_gdb.cc 508bbec99e58 > src/cpu/checker/thread_context.hh 508bbec99e58 > src/cpu/inorder/cpu.hh 508bbec99e58 > src/cpu/inorder/cpu.cc 508bbec99e58 > src/cpu/inorder/resources/cache_unit.hh 508bbec99e58 > src/cpu/inorder/resources/cache_unit.cc 508bbec99e58 > src/cpu/inorder/thread_context.hh 508bbec99e58 > src/cpu/inorder/thread_context.cc 508bbec99e58 > src/cpu/o3/cpu.hh 508bbec99e58 > src/cpu/o3/cpu.cc 508bbec99e58 > src/cpu/o3/lsq.hh 508bbec99e58 > src/cpu/o3/lsq_impl.hh 508bbec99e58 > src/cpu/o3/thread_context.hh 508bbec99e58 > src/cpu/o3/thread_context_impl.hh 508bbec99e58 > src/cpu/ozone/cpu.hh 508bbec99e58 > src/cpu/ozone/cpu_impl.hh 508bbec99e58 > src/cpu/simple/atomic.hh 508bbec99e58 > src/cpu/simple/atomic.cc 508bbec99e58 > src/cpu/simple/timing.hh 508bbec99e58 > src/cpu/simple/timing.cc 508bbec99e58 > src/cpu/simple_thread.hh 508bbec99e58 > src/cpu/simple_thread.cc 508bbec99e58 > src/cpu/thread_context.hh 508bbec99e58 > src/cpu/thread_state.hh 508bbec99e58 > src/cpu/thread_state.cc 508bbec99e58 > src/dev/simple_disk.cc 508bbec99e58 > src/kern/tru64/tru64.hh 508bbec99e58 > src/kern/tru64/tru64_events.cc 508bbec99e58 > src/mem/SConscript 508bbec99e58 > src/mem/fs_translating_port_proxy.hh PRE-CREATION > src/mem/fs_translating_port_proxy.cc PRE-CREATION > src/mem/port.hh 508bbec99e58 > src/mem/port_proxy.hh PRE-CREATION > src/mem/port_proxy.cc PRE-CREATION > src/mem/ruby/system/RubyPort.cc 508bbec99e58 > src/mem/ruby/system/RubyPortProxy.hh PRE-CREATION > src/mem/ruby/system/RubyPortProxy.cc PRE-CREATION > src/mem/ruby/system/SConscript 508bbec99e58 > src/mem/ruby/system/Sequencer.py 508bbec99e58 > src/mem/se_translating_port_proxy.hh PRE-CREATION > src/mem/se_translating_port_proxy.cc PRE-CREATION > src/mem/translating_port.hh 508bbec99e58 > src/mem/translating_port.cc 508bbec99e58 > src/mem/vport.hh 508bbec99e58 > src/mem/vport.cc 508bbec99e58 > src/sim/arguments.hh 508bbec99e58 > src/sim/process.hh 508bbec99e58 > src/sim/process.cc 508bbec99e58 > src/sim/process_impl.hh 508bbec99e58 > src/sim/syscall_emul.hh 508bbec99e58 > src/sim/syscall_emul.cc 508bbec99e58 > src/sim/system.hh 508bbec99e58 > src/sim/system.cc 508bbec99e58 > src/sim/vptr.hh 508bbec99e58 > tests/configs/inorder-timing.py 508bbec99e58 > tests/configs/memtest.py 508bbec99e58 > tests/configs/o3-timing-mp.py 508bbec99e58 > tests/configs/o3-timing.py 508bbec99e58 > tests/configs/simple-atomic-mp.py 508bbec99e58 > tests/configs/simple-atomic.py 508bbec99e58 > tests/configs/simple-timing-mp-ruby.py 508bbec99e58 > tests/configs/simple-timing-mp.py 508bbec99e58 > tests/configs/simple-timing-ruby.py 508bbec99e58 > tests/configs/simple-timing.py 508bbec99e58 > > Diff: http://reviews.m5sim.org/r/943/diff > > > Testing > ------- > > util/regress all passing (disregarding t1000 and eio) > > > Thanks, > > Andreas > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
