| Message ID | 20200121223157.15263-15-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Dynamically size memslot arrays | expand |
On Tue, Jan 21, 2020 at 02:31:52PM -0800, Sean Christopherson wrote: [...] > @@ -1101,52 +1099,55 @@ int __kvm_set_memory_region(struct kvm *kvm, > if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) > return -EINVAL; > > - slot = id_to_memslot(__kvm_memslots(kvm, as_id), id); > - base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; > - npages = mem->memory_size >> PAGE_SHIFT; > - > - if (npages > KVM_MEM_MAX_NR_PAGES) > - return -EINVAL; > - > /* > * Make a full copy of the old memslot, the pointer will become stale > * when the memslots are re-sorted by update_memslots(). > */ > - old = *slot; > + tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); > + old = *tmp; > + tmp = NULL; Shall we keep this chunk to the patch where it will be used? Other than that, it looks good to me. Thanks,
On Thu, Feb 06, 2020 at 02:06:41PM -0500, Peter Xu wrote: > On Tue, Jan 21, 2020 at 02:31:52PM -0800, Sean Christopherson wrote: > > [...] > > > @@ -1101,52 +1099,55 @@ int __kvm_set_memory_region(struct kvm *kvm, > > if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) > > return -EINVAL; > > > > - slot = id_to_memslot(__kvm_memslots(kvm, as_id), id); > > - base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; > > - npages = mem->memory_size >> PAGE_SHIFT; > > - > > - if (npages > KVM_MEM_MAX_NR_PAGES) > > - return -EINVAL; > > - > > /* > > * Make a full copy of the old memslot, the pointer will become stale > > * when the memslots are re-sorted by update_memslots(). > > */ > > - old = *slot; > > + tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); > > + old = *tmp; > > + tmp = NULL; > > Shall we keep this chunk to the patch where it will be used? Other > than that, it looks good to me. I assume you're talking about doing this instead of using @tmp? old = *id_to_memslot(__kvm_memslots(kvm, as_id), id); It's obviously possible, but I really like resulting diff for __kvm_set_memory_region() in "KVM: Terminate memslot walks via used_slots" when tmp is used. @@ -1104,8 +1203,13 @@ int __kvm_set_memory_region(struct kvm *kvm, * when the memslots are re-sorted by update_memslots(). */ tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); - old = *tmp; - tmp = NULL; + if (tmp) { + old = *tmp; + tmp = NULL; + } else { + memset(&old, 0, sizeof(old)); + old.id = id; + }
On Thu, Feb 06, 2020 at 11:22:30AM -0800, Sean Christopherson wrote: > On Thu, Feb 06, 2020 at 02:06:41PM -0500, Peter Xu wrote: > > On Tue, Jan 21, 2020 at 02:31:52PM -0800, Sean Christopherson wrote: > > > > [...] > > > > > @@ -1101,52 +1099,55 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) > > > return -EINVAL; > > > > > > - slot = id_to_memslot(__kvm_memslots(kvm, as_id), id); > > > - base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; > > > - npages = mem->memory_size >> PAGE_SHIFT; > > > - > > > - if (npages > KVM_MEM_MAX_NR_PAGES) > > > - return -EINVAL; > > > - > > > /* > > > * Make a full copy of the old memslot, the pointer will become stale > > > * when the memslots are re-sorted by update_memslots(). > > > */ > > > - old = *slot; > > > + tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); > > > + old = *tmp; > > > + tmp = NULL; > > > > Shall we keep this chunk to the patch where it will be used? Other > > than that, it looks good to me. > > I assume you're talking about doing this instead of using @tmp? > > old = *id_to_memslot(__kvm_memslots(kvm, as_id), id); Yes. > > It's obviously possible, but I really like resulting diff for > __kvm_set_memory_region() in "KVM: Terminate memslot walks via used_slots" > when tmp is used. > > @@ -1104,8 +1203,13 @@ int __kvm_set_memory_region(struct kvm *kvm, > * when the memslots are re-sorted by update_memslots(). > */ > tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); > - old = *tmp; > - tmp = NULL; > + if (tmp) { > + old = *tmp; > + tmp = NULL; > + } else { > + memset(&old, 0, sizeof(old)); > + old.id = id; > + } I normally don't do that, for each patch I'll try to make it consistent to itself, assuming that follow-up patches can be rejected. I don't have strong opinion either, please feel free to keep them if no one disagrees.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3278ddf5a56a..6210738cf2f6 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1070,13 +1070,11 @@ static int kvm_delete_memslot(struct kvm *kvm, int __kvm_set_memory_region(struct kvm *kvm, const struct kvm_userspace_memory_region *mem) { - int r; - gfn_t base_gfn; - unsigned long npages; - struct kvm_memory_slot *slot; struct kvm_memory_slot old, new; - int as_id, id; + struct kvm_memory_slot *tmp; enum kvm_mr_change change; + int as_id, id; + int r; r = check_memory_region_flags(mem); if (r) @@ -1101,52 +1099,55 @@ int __kvm_set_memory_region(struct kvm *kvm, if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) return -EINVAL; - slot = id_to_memslot(__kvm_memslots(kvm, as_id), id); - base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; - npages = mem->memory_size >> PAGE_SHIFT; - - if (npages > KVM_MEM_MAX_NR_PAGES) - return -EINVAL; - /* * Make a full copy of the old memslot, the pointer will become stale * when the memslots are re-sorted by update_memslots(). */ - old = *slot; + tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id); + old = *tmp; + tmp = NULL; + if (!mem->memory_size) return kvm_delete_memslot(kvm, mem, &old, as_id); - new = old; - new.id = id; - new.base_gfn = base_gfn; - new.npages = npages; + new.base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; + new.npages = mem->memory_size >> PAGE_SHIFT; new.flags = mem->flags; new.userspace_addr = mem->userspace_addr; + if (new.npages > KVM_MEM_MAX_NR_PAGES) + return -EINVAL; + if (!old.npages) { change = KVM_MR_CREATE; + new.dirty_bitmap = NULL; + memset(&new.arch, 0, sizeof(new.arch)); } else { /* Modify an existing slot. */ if ((new.userspace_addr != old.userspace_addr) || - (npages != old.npages) || + (new.npages != old.npages) || ((new.flags ^ old.flags) & KVM_MEM_READONLY)) return -EINVAL; - if (base_gfn != old.base_gfn) + if (new.base_gfn != old.base_gfn) change = KVM_MR_MOVE; else if (new.flags != old.flags) change = KVM_MR_FLAGS_ONLY; else /* Nothing to change. */ return 0; + + /* Copy dirty_bitmap and arch from the current memslot. */ + new.dirty_bitmap = old.dirty_bitmap; + memcpy(&new.arch, &old.arch, sizeof(new.arch)); } if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) { /* Check for overlaps */ - kvm_for_each_memslot(slot, __kvm_memslots(kvm, as_id)) { - if (slot->id == id) + kvm_for_each_memslot(tmp, __kvm_memslots(kvm, as_id)) { + if (tmp->id == id) continue; - if (!((base_gfn + npages <= slot->base_gfn) || - (base_gfn >= slot->base_gfn + slot->npages))) + if (!((new.base_gfn + new.npages <= tmp->base_gfn) || + (new.base_gfn >= tmp->base_gfn + tmp->npages))) return -EEXIST; } }
Clean up __kvm_set_memory_region() to achieve several goals: - Remove local variables that serve no real purpose - Improve the readability of the code - Better show the relationship between the 'old' and 'new' memslot - Prepare for dynamically sizing memslots. Note, using 'tmp' to hold the initial memslot is not strictly necessary at this juncture, e.g. 'old' could be directly copied from id_to_memslot(), but keep the pointer usage as id_to_memslot() will be able to return a NULL pointer once memslots are dynamically sized. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- virt/kvm/kvm_main.c | 47 +++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 23 deletions(-)