> On July 27, 2016, 2:32 p.m., Jason Lowe-Power wrote: > > Thanks for the patch. I'm happy to see the backing store made into a class > > instead of a simple pair. I have a patch to do something similar in my > > queue. > > > > A few things below. I still this this patch needs to be broken up further > > :). We should aim to keep each patch a single logical change. Mayby 1) > > Backing store refactoring, 2) Ruby drain fix, 3) Add reserved range for... > > Though, I'm flexible on this. > > > > The only big question I have is what is the "reserved" range for and why is > > it hardcoded?
Thanks for the review. Sorry for taking so long to get back to you, but hopefully these changes improve the patch. > On July 27, 2016, 2:32 p.m., Jason Lowe-Power wrote: > > src/sim/system.cc, line 330 > > <http://reviews.gem5.org/r/3580/diff/2/?file=57357#file57357line330> > > > > Again.. what is this range reserved for? Why are we hardcoding it? > > > > Also, how does this change relate to the KVM changes? I'm not entirely sure, but without blocking the use of this range KVM crashes immediately if the --mem-size is greater than 4095MB. This is reproducible on both Ubuntu 14.04 and Centos 6. The crash does not happen unless KVM is used. We're hardcoding it because Ruby does not accept split AddrRanges for main memory, so the fix can't be placed in the configuration files. However, since the problem is KVM- and possibly x86-specific and I'm not sure exactly why this works, I'm okay with leaving this change out until we have a better idea what the problem is. > On July 27, 2016, 2:32 p.m., Jason Lowe-Power wrote: > > src/mem/ruby/system/Sequencer.cc, line 254 > > <http://reviews.gem5.org/r/3580/diff/2/?file=57356#file57356line254> > > > > This should be in a seperate patch. Though, I'm OK with you splitting > > this out before pushing and leaving it here for the review. Good idea. I'll make sure it's split out before it's committed. > On July 27, 2016, 2:32 p.m., Jason Lowe-Power wrote: > > src/mem/physical.hh, line 145 > > <http://reviews.gem5.org/r/3580/diff/2/?file=57354#file57354line145> > > > > Maybe make is_in_addr_map default to true. Then you'll have fewer > > changes in physical.cc. This is up to you, though. Good idea. I updated the patch to use a seperate parameter, kvm_map, which makes the patch overall a lot neater. It defaults to True as a Python configuration parameter for AbstractMemory, so unfortunately I can't make use of this particular trick. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3580/#review8549 ----------------------------------------------------------- On July 26, 2016, 11:22 p.m., David Hashe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3580/ > ----------------------------------------------------------- > > (Updated July 26, 2016, 11:22 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11562:7375e1f533fa > --------------------------- > cpu, mem, sim: Enable KVM support for Ruby > > Use heuristic to avoid mapping both main memory and its copy when > --access-backing-store is specified. > > Remember whether a BackingStoreEntry is in the global address map. > > Fix bug causing incomplete draining of Ruby Sequencer. > > Skip mapping ranges reserved by KVM. > > > Diffs > ----- > > src/cpu/kvm/vm.cc 704b0198f747b766b839c577614eb2924fd1dfee > src/mem/physical.hh 704b0198f747b766b839c577614eb2924fd1dfee > src/mem/physical.cc 704b0198f747b766b839c577614eb2924fd1dfee > src/mem/ruby/system/Sequencer.cc 704b0198f747b766b839c577614eb2924fd1dfee > src/sim/system.cc 704b0198f747b766b839c577614eb2924fd1dfee > > Diff: http://reviews.gem5.org/r/3580/diff/ > > > Testing > ------- > > > Thanks, > > David Hashe > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
