cpu, mem, sim: Change how KVM maps memory
Review Request #3580 - Created July 22, 2016 and submitted
| Information | |
|---|---|
| David Hashe | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 11562:7375e1f533fa
---------------------------
cpu, mem, sim: Change how KVM maps memoryOnly map memories into the KVM guest address space that are
marked as usable by KVM. Create BackingStoreEntry class
containing flags for is_conf_reported, in_addr_map, and
kvm_map.
Maybe this should be two patches? One for the APU changes and one for the changes in to the physical memory. Why are the physical memory changes needed?
Seems like maybe this should be "Enable KVM support for Ruby" and "kvm acceleration for apu...".
Also, does KVM+Ruby+FS work now? Or just KVM+Ruby+SE? Or does this patch only make KVM+Ruby+SE work if you're using the APU?
Thanks! I'm excited to see KVM+Ruby support coming.
Change Summary:
Split config changes into a separate patch
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+98 -20) |
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 (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
Again.. what is this range reserved for? Why are we hardcoding it?
Also, how does this change relate to the KVM changes?
I would really prefer if we didn't have to add this complexity.
I thought we were pretty much in a position where we could remove the horrible shadow memory hack. Could we not pursue that route rather? It would be a much better solution long term.
Change Summary:
Add kvm_map parameter to AbstractMemory.
Store kvm_map in BackingStoreEntry rather than is_in_addr_map.
Simplify KVM mapping logic by using kvm_map.
Remove change to avoid allocating pages within a reserved range.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+86 -17) |
I see how this works as a stop gap, but ultimately I would like to push for the removal of the shadow memory as the first option. Is it really that much effort?
-
src/mem/physical.hh (Diff revision 3) -
in the guest
-
src/mem/physical.hh (Diff revision 3) -
perhaps beef this up a bit:
the host memory?
same size as range?
-
src/mem/physical.hh (Diff revision 3) -
Again not a very descriptive parameter.
-
src/mem/physical.hh (Diff revision 3) -
whitespace line
-
src/mem/physical.cc (Diff revision 3) -
I find the control flow here very odd. The kvm_addr_map is initialised and set on a path different to where it is used? What memory is it affecting? Why is it sticky? What if one is not kvm mapped and the others are etc?
-
src/mem/physical.cc (Diff revision 3) -
Same oddity with the configuration here. We should check each and every memory.
-
src/mem/ruby/system/Sequencer.cc (Diff revision 3) -
This seems completely unrelated.
Change Summary:
Comment changes.
Rewrite kvmMap logic for interleaved and non-interleaved memories in the global address map.
Remove drain fix.
Diff: |
Revision 4 (+87 -17) |
|---|
-
src/mem/AbstractMemory.py (Diff revision 4) -
I am still trying to wrap my head around what the table looks like with in_addr_map, conf_table_Reported, and now kvm_map, for "real memories", "device memories", "shadow memories" etc. We technically have 8 different types of memories now, but I suspect we only really have three. Could we perhaps enumerate them and call that out in the comments.
Perhaps we should consider an enum rather?
-
src/mem/abstract_mem.hh (Diff revision 4) -
could you make this const, and the two other bools as well?
-
src/mem/physical.cc (Diff revision 4) -
whitespace
-
src/mem/physical.cc (Diff revision 4) -
while you are at it, change to .emplace_back(range, pmem, kvm_map)
I think this makes sense overall. I can definitely see use cases where I would want to prevent KVM from punching through to the backing store for some memories. I have two high-level comments:
Could you make sure to update the commit message to reflect that this change now only deals with how KVM maps memory? The current message describes what the patch series does and not what this individual commit does (it has nothing to do with Ruby).
I like the idea of using a struct to describe the backing store, this makes the code much clearer. Could you make it a bit more symmetric and make sure the conf_table_reported flag is included in the struct as well? We should check that flag as well when mapping memories in kvm, but that should probably be a separate commit since it's a bug fix that's orthogonal to this.
Thanks for sorting this out!
Change Summary:
Whitespace and style changes.
Rewrite commit message.
Added additional members to BackingStoreEntry.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+120 -19) |
Ship It!
Ship It!
