----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3580/#review8549 -----------------------------------------------------------
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? src/cpu/kvm/vm.cc (line 381) <http://reviews.gem5.org/r/3580/#comment7433> What is this range reservered for? x86 I/O? I don't really like that this is hardcoded here. Though I don't have a suggestion as to how to fix it. src/mem/physical.hh (line 145) <http://reviews.gem5.org/r/3580/#comment7434> Maybe make is_in_addr_map default to true. Then you'll have fewer changes in physical.cc. This is up to you, though. src/mem/ruby/system/Sequencer.cc (line 254) <http://reviews.gem5.org/r/3580/#comment7435> 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. src/sim/system.cc (line 330) <http://reviews.gem5.org/r/3580/#comment7436> Again.. what is this range reserved for? Why are we hardcoding it? Also, how does this change relate to the KVM changes? - Jason Lowe-Power 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
