> On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.hh, line 86 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20692#file20692line86> > > > > Typo in "boundaries" in this and the next comment. Unless it's some > > variant British spelling I'm unaware of :).
Fixed :) > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.hh, line 102 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20692#file20692line102> > > > > 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. I haven't tidied up the code at all, but I can of course do so. I tried to keep the changes to a minimum and only do the file rename and the absolutely necessary edits. Obviously that doesn't seem to have worked very well. > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.hh, line 129 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20692#file20692line129> > > > > 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. > > Once again, I haven't tidied up the code at all to minimise the changes. I can perform this change, but would rather make it a second "cleanup" patch. > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.cc, line 1 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20693#file20693line1> > > > > 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} I did do them as file renames, just as you have suggested. I did not do that for the first iteration of these changes (as I am sure you remember), and the very first step of the second iteration was exactly the renames you suggest. I don't know why the review board does not like this or gets it wrong. > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.cc, line 78 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20693#file20693line78> > > > > need space after the comma here (and several similar places) Once again, I've tried to not change anything unless I have to, but would be very happy to do so. I'm itching to make things look decent, and really had to try hard not to fix it. > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/fs_translating_port_proxy.cc, line 121 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20693#file20693line121> > > > > 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. I think that is a brilliant suggestion and would be keen to clean it up as well. I would imagine we should wait until Gabe is done with the SE/FS merge. > On 2012-01-11 20:35:11, Steve Reinhardt wrote: > > src/mem/port_proxy.cc, line 40 > > <http://reviews.m5sim.org/r/943/diff/7/?file=20696#file20696line40> > > > > 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. Will do so! - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/943/#review1921 ----------------------------------------------------------- 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
