[v5,12/19] KVM: Move memslot deletion to helper function
diff mbox series

Message ID 20200121223157.15263-13-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: Dynamically size memslot arrays
Related show

Commit Message

Sean Christopherson Jan. 21, 2020, 10:31 p.m. UTC
Move memslot deletion into its own routine so that the success path for
other memslot updates does not need to use kvm_free_memslot(), i.e. can
explicitly destroy the dirty bitmap when necessary.  This paves the way
for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
NULL for @dont.

Add a comment above the code to make a copy of the existing memslot
prior to deletion, it is not at all obvious that the pointer will become
stale during sorting and/or installation of new memslots.

Note, kvm_arch_commit_memory_region() allows an architecture to free
resources when moving a memslot or changing its flags, e.g. x86 frees
its arch specific memslot metadata during commit_memory_region().

Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Tested-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 virt/kvm/kvm_main.c | 73 +++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 29 deletions(-)

Comments

Peter Xu Feb. 6, 2020, 4:14 p.m. UTC | #1
On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote:
> Move memslot deletion into its own routine so that the success path for
> other memslot updates does not need to use kvm_free_memslot(), i.e. can
> explicitly destroy the dirty bitmap when necessary.  This paves the way
> for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
> NULL for @dont.
> 
> Add a comment above the code to make a copy of the existing memslot
> prior to deletion, it is not at all obvious that the pointer will become
> stale during sorting and/or installation of new memslots.

Could you help explain a bit on this explicit comment?  I can follow
up with the patch itself which looks all correct to me, but I failed
to catch what this extra comment wants to emphasize...

Thanks,

> 
> Note, kvm_arch_commit_memory_region() allows an architecture to free
> resources when moving a memslot or changing its flags, e.g. x86 frees
> its arch specific memslot metadata during commit_memory_region().
Sean Christopherson Feb. 6, 2020, 4:28 p.m. UTC | #2
On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote:
> On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote:
> > Move memslot deletion into its own routine so that the success path for
> > other memslot updates does not need to use kvm_free_memslot(), i.e. can
> > explicitly destroy the dirty bitmap when necessary.  This paves the way
> > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
> > NULL for @dont.
> > 
> > Add a comment above the code to make a copy of the existing memslot
> > prior to deletion, it is not at all obvious that the pointer will become
> > stale during sorting and/or installation of new memslots.
> 
> Could you help explain a bit on this explicit comment?  I can follow
> up with the patch itself which looks all correct to me, but I failed
> to catch what this extra comment wants to emphasize...

It's tempting to write the code like this (I know, because I did it):

	if (!mem->memory_size)
		return kvm_delete_memslot(kvm, mem, slot, as_id);

	new = *slot;

Where @slot is a pointer to the memslot to be deleted.  At first, second,
and third glances, this seems perfectly sane.

The issue is that slot was pulled from struct kvm_memslots.memslots, e.g.

	slot = &slots->memslots[index];

Note that slots->memslots holds actual "struct kvm_memory_slot" objects,
not pointers to slots.  When update_memslots() sorts the slots, it swaps
the actual slot objects, not pointers.  I.e. after update_memslots(), even
though @slot points at the same address, it's could be pointing at a
different slot.  As a result kvm_free_memslot() in kvm_delete_memslot()
will free the dirty page info and arch-specific points for some random
slot, not the intended slot, and will set npages=0 for that random slot.
Peter Xu Feb. 6, 2020, 4:51 p.m. UTC | #3
On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote:
> On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote:
> > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote:
> > > Move memslot deletion into its own routine so that the success path for
> > > other memslot updates does not need to use kvm_free_memslot(), i.e. can
> > > explicitly destroy the dirty bitmap when necessary.  This paves the way
> > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
> > > NULL for @dont.
> > > 
> > > Add a comment above the code to make a copy of the existing memslot
> > > prior to deletion, it is not at all obvious that the pointer will become
> > > stale during sorting and/or installation of new memslots.
> > 
> > Could you help explain a bit on this explicit comment?  I can follow
> > up with the patch itself which looks all correct to me, but I failed
> > to catch what this extra comment wants to emphasize...
> 
> It's tempting to write the code like this (I know, because I did it):
> 
> 	if (!mem->memory_size)
> 		return kvm_delete_memslot(kvm, mem, slot, as_id);
> 
> 	new = *slot;
> 
> Where @slot is a pointer to the memslot to be deleted.  At first, second,
> and third glances, this seems perfectly sane.
> 
> The issue is that slot was pulled from struct kvm_memslots.memslots, e.g.
> 
> 	slot = &slots->memslots[index];
> 
> Note that slots->memslots holds actual "struct kvm_memory_slot" objects,
> not pointers to slots.  When update_memslots() sorts the slots, it swaps
> the actual slot objects, not pointers.  I.e. after update_memslots(), even
> though @slot points at the same address, it's could be pointing at a
> different slot.  As a result kvm_free_memslot() in kvm_delete_memslot()
> will free the dirty page info and arch-specific points for some random
> slot, not the intended slot, and will set npages=0 for that random slot.

Ah I see, thanks.  Another alternative is we move the "old = *slot"
copy into kvm_delete_memslot(), which could be even clearer imo.
However I'm not sure whether it's a good idea to drop the test-by for
this.  Considering that comment change should not affect it, would you
mind enrich the comment into something like this (or anything better)?

/*
 * Make a full copy of the old memslot, the pointer will become stale
 * when the memslots are re-sorted by update_memslots() in
 * kvm_delete_memslot(), while to make the kvm_free_memslot() work as
 * expected later on, we still need the cached memory slot.
 */

In all cases:

Reviewed-by: Peter Xu <peterx@redhat.com>
Sean Christopherson Feb. 7, 2020, 5:59 p.m. UTC | #4
On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote:
> On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote:
> > On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote:
> > > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote:
> > > > Move memslot deletion into its own routine so that the success path for
> > > > other memslot updates does not need to use kvm_free_memslot(), i.e. can
> > > > explicitly destroy the dirty bitmap when necessary.  This paves the way
> > > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
> > > > NULL for @dont.
> > > > 
> > > > Add a comment above the code to make a copy of the existing memslot
> > > > prior to deletion, it is not at all obvious that the pointer will become
> > > > stale during sorting and/or installation of new memslots.
> > > 
> > > Could you help explain a bit on this explicit comment?  I can follow
> > > up with the patch itself which looks all correct to me, but I failed
> > > to catch what this extra comment wants to emphasize...
> > 
> > It's tempting to write the code like this (I know, because I did it):
> > 
> > 	if (!mem->memory_size)
> > 		return kvm_delete_memslot(kvm, mem, slot, as_id);
> > 
> > 	new = *slot;
> > 
> > Where @slot is a pointer to the memslot to be deleted.  At first, second,
> > and third glances, this seems perfectly sane.
> > 
> > The issue is that slot was pulled from struct kvm_memslots.memslots, e.g.
> > 
> > 	slot = &slots->memslots[index];
> > 
> > Note that slots->memslots holds actual "struct kvm_memory_slot" objects,
> > not pointers to slots.  When update_memslots() sorts the slots, it swaps
> > the actual slot objects, not pointers.  I.e. after update_memslots(), even
> > though @slot points at the same address, it's could be pointing at a
> > different slot.  As a result kvm_free_memslot() in kvm_delete_memslot()
> > will free the dirty page info and arch-specific points for some random
> > slot, not the intended slot, and will set npages=0 for that random slot.
> 
> Ah I see, thanks.  Another alternative is we move the "old = *slot"
> copy into kvm_delete_memslot(), which could be even clearer imo.

The copy is also needed in __kvm_set_memory_region() for the MOVE case.

> However I'm not sure whether it's a good idea to drop the test-by for
> this.  Considering that comment change should not affect it, would you
> mind enrich the comment into something like this (or anything better)?
> 
> /*
>  * Make a full copy of the old memslot, the pointer will become stale
>  * when the memslots are re-sorted by update_memslots() in
>  * kvm_delete_memslot(), while to make the kvm_free_memslot() work as
>  * expected later on, we still need the cached memory slot.
>  */

As above, it's more subtle than just the kvm_delete_memslot() case.

	/*
	 * Make a full copy of the old memslot, the pointer will become stale
	 * when the memslots are re-sorted by update_memslots() when deleting
	 * or moving a memslot, and additional modifications to the old memslot
	 * need to be made after calling update_memslots().
	 */
Sean Christopherson Feb. 7, 2020, 6:07 p.m. UTC | #5
On Fri, Feb 07, 2020 at 09:59:12AM -0800, Sean Christopherson wrote:
> On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote:
> > /*
> >  * Make a full copy of the old memslot, the pointer will become stale
> >  * when the memslots are re-sorted by update_memslots() in
> >  * kvm_delete_memslot(), while to make the kvm_free_memslot() work as
> >  * expected later on, we still need the cached memory slot.
> >  */
> 
> As above, it's more subtle than just the kvm_delete_memslot() case.
> 
> 	/*
> 	 * Make a full copy of the old memslot, the pointer will become stale
> 	 * when the memslots are re-sorted by update_memslots() when deleting
> 	 * or moving a memslot, and additional modifications to the old memslot
> 	 * need to be made after calling update_memslots().
> 	 */

Actually, that's not quite correct, as the same is true for all memslot
updates, and we still query @old after update_memslots() for CREATE and
FLAGS.  This is better.

        /*
         * Make a full copy of the old memslot, the pointer will become stale
         * when the memslots are re-sorted by update_memslots(), and the old
	 * memslot needs to be referenced after calling update_memslots(), e.g.
	 * to free its resources and for arch specific behavior.
         */
Peter Xu Feb. 7, 2020, 6:17 p.m. UTC | #6
On Fri, Feb 07, 2020 at 09:59:12AM -0800, Sean Christopherson wrote:
> On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote:
> > On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote:
> > > On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote:
> > > > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote:
> > > > > Move memslot deletion into its own routine so that the success path for
> > > > > other memslot updates does not need to use kvm_free_memslot(), i.e. can
> > > > > explicitly destroy the dirty bitmap when necessary.  This paves the way
> > > > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass
> > > > > NULL for @dont.
> > > > > 
> > > > > Add a comment above the code to make a copy of the existing memslot
> > > > > prior to deletion, it is not at all obvious that the pointer will become
> > > > > stale during sorting and/or installation of new memslots.
> > > > 
> > > > Could you help explain a bit on this explicit comment?  I can follow
> > > > up with the patch itself which looks all correct to me, but I failed
> > > > to catch what this extra comment wants to emphasize...
> > > 
> > > It's tempting to write the code like this (I know, because I did it):
> > > 
> > > 	if (!mem->memory_size)
> > > 		return kvm_delete_memslot(kvm, mem, slot, as_id);
> > > 
> > > 	new = *slot;
> > > 
> > > Where @slot is a pointer to the memslot to be deleted.  At first, second,
> > > and third glances, this seems perfectly sane.
> > > 
> > > The issue is that slot was pulled from struct kvm_memslots.memslots, e.g.
> > > 
> > > 	slot = &slots->memslots[index];
> > > 
> > > Note that slots->memslots holds actual "struct kvm_memory_slot" objects,
> > > not pointers to slots.  When update_memslots() sorts the slots, it swaps
> > > the actual slot objects, not pointers.  I.e. after update_memslots(), even
> > > though @slot points at the same address, it's could be pointing at a
> > > different slot.  As a result kvm_free_memslot() in kvm_delete_memslot()
> > > will free the dirty page info and arch-specific points for some random
> > > slot, not the intended slot, and will set npages=0 for that random slot.
> > 
> > Ah I see, thanks.  Another alternative is we move the "old = *slot"
> > copy into kvm_delete_memslot(), which could be even clearer imo.
> 
> The copy is also needed in __kvm_set_memory_region() for the MOVE case.

Right.  I actually meant to do all "old = *slot" in any function we
need to cache the data explicitly, with that we also need another one
after calling kvm_delete_memslot() for move.  But with the comment as
you suggested below it looks good to me too.

Thanks,

> 
> > However I'm not sure whether it's a good idea to drop the test-by for
> > this.  Considering that comment change should not affect it, would you
> > mind enrich the comment into something like this (or anything better)?
> > 
> > /*
> >  * Make a full copy of the old memslot, the pointer will become stale
> >  * when the memslots are re-sorted by update_memslots() in
> >  * kvm_delete_memslot(), while to make the kvm_free_memslot() work as
> >  * expected later on, we still need the cached memory slot.
> >  */
> 
> As above, it's more subtle than just the kvm_delete_memslot() case.
> 
> 	/*
> 	 * Make a full copy of the old memslot, the pointer will become stale
> 	 * when the memslots are re-sorted by update_memslots() when deleting
> 	 * or moving a memslot, and additional modifications to the old memslot
> 	 * need to be made after calling update_memslots().
> 	 */
>

Patch
diff mbox series

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 69d6158cb405..5ae2dd6d9993 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1042,6 +1042,27 @@  static int kvm_set_memslot(struct kvm *kvm,
 	return r;
 }
 
+static int kvm_delete_memslot(struct kvm *kvm,
+			      const struct kvm_userspace_memory_region *mem,
+			      struct kvm_memory_slot *old, int as_id)
+{
+	struct kvm_memory_slot new;
+	int r;
+
+	if (!old->npages)
+		return -EINVAL;
+
+	memset(&new, 0, sizeof(new));
+	new.id = old->id;
+
+	r = kvm_set_memslot(kvm, mem, old, &new, as_id, KVM_MR_DELETE);
+	if (r)
+		return r;
+
+	kvm_free_memslot(kvm, old, NULL);
+	return 0;
+}
+
 /*
  * Allocate some memory and give it an address in the guest physical address
  * space.
@@ -1091,7 +1112,15 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	if (npages > KVM_MEM_MAX_NR_PAGES)
 		return -EINVAL;
 
-	new = old = *slot;
+	/*
+	 * Make a full copy of the old memslot, the pointer will become stale
+	 * when the memslots are re-sorted by update_memslots().
+	 */
+	old = *slot;
+	if (!mem->memory_size)
+		return kvm_delete_memslot(kvm, mem, &old, as_id);
+
+	new = old;
 
 	new.id = id;
 	new.base_gfn = base_gfn;
@@ -1099,29 +1128,20 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	new.flags = mem->flags;
 	new.userspace_addr = mem->userspace_addr;
 
-	if (npages) {
-		if (!old.npages)
-			change = KVM_MR_CREATE;
-		else { /* Modify an existing slot. */
-			if ((new.userspace_addr != old.userspace_addr) ||
-			    (npages != old.npages) ||
-			    ((new.flags ^ old.flags) & KVM_MEM_READONLY))
-				return -EINVAL;
-
-			if (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;
-		}
-	} else {
-		if (!old.npages)
+	if (!old.npages) {
+		change = KVM_MR_CREATE;
+	} else { /* Modify an existing slot. */
+		if ((new.userspace_addr != old.userspace_addr) ||
+		    (npages != old.npages) ||
+		    ((new.flags ^ old.flags) & KVM_MEM_READONLY))
 			return -EINVAL;
 
-		change = KVM_MR_DELETE;
-		new.base_gfn = 0;
-		new.flags = 0;
+		if (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;
 	}
 
 	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
@@ -1144,17 +1164,12 @@  int __kvm_set_memory_region(struct kvm *kvm,
 			return r;
 	}
 
-	/* 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));
-	}
-
 	r = kvm_set_memslot(kvm, mem, &old, &new, as_id, change);
 	if (r)
 		goto out_bitmap;
 
-	kvm_free_memslot(kvm, &old, &new);
+	if (old.dirty_bitmap && !new.dirty_bitmap)
+		kvm_destroy_dirty_bitmap(&old);
 	return 0;
 
 out_bitmap: