lkml.org 
[lkml]   [2020]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 09/19] KVM: Move setting of memslot into helper routine
    On Tue, Jan 21, 2020 at 02:31:47PM -0800, Sean Christopherson wrote:
    > Split out the core functionality of setting a memslot into a separate
    > helper in preparation for moving memslot deletion into its own routine.
    >
    > Tested-by: Christoffer Dall <christoffer.dall@arm.com>
    > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
    > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
    > ---
    > virt/kvm/kvm_main.c | 106 ++++++++++++++++++++++++++------------------
    > 1 file changed, 63 insertions(+), 43 deletions(-)
    >
    > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
    > index fdf045a5d240..64f6c5d35260 100644
    > --- a/virt/kvm/kvm_main.c
    > +++ b/virt/kvm/kvm_main.c
    > @@ -982,6 +982,66 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
    > return old_memslots;
    > }
    >
    > +static int kvm_set_memslot(struct kvm *kvm,
    > + const struct kvm_userspace_memory_region *mem,
    > + const struct kvm_memory_slot *old,
    > + struct kvm_memory_slot *new, int as_id,
    > + enum kvm_mr_change change)
    > +{
    > + struct kvm_memory_slot *slot;
    > + struct kvm_memslots *slots;
    > + int r;
    > +
    > + slots = kvzalloc(sizeof(struct kvm_memslots), GFP_KERNEL_ACCOUNT);
    > + if (!slots)
    > + return -ENOMEM;
    > + memcpy(slots, __kvm_memslots(kvm, as_id), sizeof(struct kvm_memslots));
    > +
    > + if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
    > + /*
    > + * Note, the INVALID flag needs to be in the appropriate entry
    > + * in the freshly allocated memslots, not in @old or @new.
    > + */
    > + slot = id_to_memslot(slots, old->id);
    > + slot->flags |= KVM_MEMSLOT_INVALID;
    > +
    > + /*
    > + * We can re-use the old memslots, the only difference from the
    > + * newly installed memslots is the invalid flag, which will get
    > + * dropped by update_memslots anyway. We'll also revert to the
    > + * old memslots if preparing the new memory region fails.
    > + */
    > + slots = install_new_memslots(kvm, as_id, slots);
    > +
    > + /* From this point no new shadow pages pointing to a deleted,
    > + * or moved, memslot will be created.
    > + *
    > + * validation of sp->gfn happens in:
    > + * - gfn_to_hva (kvm_read_guest, gfn_to_pfn)
    > + * - kvm_is_visible_gfn (mmu_check_root)
    > + */
    > + kvm_arch_flush_shadow_memslot(kvm, slot);
    > + }
    > +
    > + r = kvm_arch_prepare_memory_region(kvm, new, mem, change);
    > + if (r)
    > + goto out_slots;
    > +
    > + update_memslots(slots, new, change);
    > + slots = install_new_memslots(kvm, as_id, slots);
    > +
    > + kvm_arch_commit_memory_region(kvm, mem, old, new, change);
    > +
    > + kvfree(slots);
    > + return 0;
    > +
    > +out_slots:
    > + if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
    > + slots = install_new_memslots(kvm, as_id, slots);
    > + kvfree(slots);
    > + return r;
    > +}
    > +
    > /*
    > * Allocate some memory and give it an address in the guest physical address
    > * space.
    > @@ -998,7 +1058,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
    > unsigned long npages;
    > struct kvm_memory_slot *slot;
    > struct kvm_memory_slot old, new;
    > - struct kvm_memslots *slots;
    > int as_id, id;
    > enum kvm_mr_change change;
    >
    > @@ -1085,58 +1144,19 @@ int __kvm_set_memory_region(struct kvm *kvm,
    > return r;
    > }
    >
    > - slots = kvzalloc(sizeof(struct kvm_memslots), GFP_KERNEL_ACCOUNT);
    > - if (!slots) {
    > - r = -ENOMEM;
    > - goto out_bitmap;
    > - }
    > - memcpy(slots, __kvm_memslots(kvm, as_id), sizeof(struct kvm_memslots));
    > -
    > - if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
    > - slot = id_to_memslot(slots, id);
    > - slot->flags |= KVM_MEMSLOT_INVALID;
    > -
    > - /*
    > - * We can re-use the old memslots, the only difference from the
    > - * newly installed memslots is the invalid flag, which will get
    > - * dropped by update_memslots anyway. We'll also revert to the
    > - * old memslots if preparing the new memory region fails.
    > - */
    > - slots = install_new_memslots(kvm, as_id, slots);
    > -
    > - /* From this point no new shadow pages pointing to a deleted,
    > - * or moved, memslot will be created.
    > - *
    > - * validation of sp->gfn happens in:
    > - * - gfn_to_hva (kvm_read_guest, gfn_to_pfn)
    > - * - kvm_is_visible_gfn (mmu_check_root)
    > - */
    > - kvm_arch_flush_shadow_memslot(kvm, slot);
    > - }
    > -
    > - r = kvm_arch_prepare_memory_region(kvm, &new, mem, change);
    > - if (r)
    > - goto out_slots;
    > -
    > /* actual memory is freed via old in kvm_free_memslot below */
    > if (change == KVM_MR_DELETE) {
    > new.dirty_bitmap = NULL;
    > memset(&new.arch, 0, sizeof(new.arch));
    > }

    It's not extremely clear to me on why this single block is leftover
    here instead of putting into kvm_set_memslot(). I see that after all
    it'll be removed in patch 12, so it seems OK:

    Reviewed-by: Peter Xu <peterx@redhat.com>

    >
    > - update_memslots(slots, &new, change);
    > - slots = install_new_memslots(kvm, as_id, slots);
    > -
    > - kvm_arch_commit_memory_region(kvm, mem, &old, &new, change);
    > + r = kvm_set_memslot(kvm, mem, &old, &new, as_id, change);
    > + if (r)
    > + goto out_bitmap;
    >
    > kvm_free_memslot(kvm, &old, &new);
    > - kvfree(slots);
    > return 0;
    >
    > -out_slots:
    > - if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
    > - slots = install_new_memslots(kvm, as_id, slots);
    > - kvfree(slots);
    > out_bitmap:
    > if (new.dirty_bitmap && !old.dirty_bitmap)
    > kvm_destroy_dirty_bitmap(&new);
    > --
    > 2.24.1
    >

    --
    Peter Xu

    \
     
     \ /
      Last update: 2020-02-06 17:26    [W:4.188 / U:0.772 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site