Re: [PATCH v5 07/13] KVM: Just resync arch fields when slots_arch_lock gets reacquired

[Maciej S. Szmigiero]

On 20.10.2021 01:55, Sean Christopherson wrote:
> On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
> > From: "Maciej S. Szmigiero" <maciej.szmigiero@xxxxxxxxxx>
> >
> > There is no need to copy the whole memslot data after releasing
> > slots_arch_lock for a moment to install temporary memslots copy in
> > kvm_set_memslot() since this lock only protects the arch field of each
> > memslot.
> >
> > Just resync this particular field after reacquiring slots_arch_lock.
>
> I assume this needed to avoid having a mess when introducing the r-b tree?  If so,
> please call that out.  Iterating over the slots might actually be slower than the
> full memcpy, i.e. as a standalone patch this may or may not be make sense.

Yes, it's an intermediate state of the code to not break bisecting.
The code changed by this patch is then completely replaced later by the
patch 11 of this patchset.

Will add a note about this to the commit message.

> > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@xxxxxxxxxx>
> > ---
> >   virt/kvm/kvm_main.c | 17 ++++++++++++-----
> >   1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 348fae880189..48d182840060 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1482,6 +1482,15 @@ static void kvm_copy_memslots(struct kvm_memslots *to,
> >   	memcpy(to, from, kvm_memslots_size(from->used_slots));
> >   }
> >   
> > +static void kvm_copy_memslots_arch(struct kvm_memslots *to,
> > +				   struct kvm_memslots *from)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < from->used_slots; i++)
> > +		to->memslots[i].arch = from->memslots[i].arch;
>
> This should probably be a memcpy(), I don't know what all shenanigans the compiler
> can throw at us if it gets to copy a struct by value.

Normally, copy-assignment of a struct is a safe operation (this is purely
an internal kernel struct, so there are no worries about padding leakage
to the userspace), but can replace this with a memcpy().

> > +}
> > +
> >   /*
> >    * Note, at a minimum, the current number of used slots must be allocated, even
> >    * when deleting a memslot, as we need a complete duplicate of the memslots for
>
> There's an out-of-sight comment that's now stale, can you revert to the
> pre-slots_arch_lock comment?
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 48d182840060..ef3345428047 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1555,9 +1555,10 @@ static int kvm_set_memslot(struct kvm *kvm,
>                  slot->flags |= KVM_MEMSLOT_INVALID;
>
>                  /*
> -                * We can re-use the memory from the old memslots.
> -                * It will be overwritten with a copy of the new memslots
> -                * after reacquiring the slots_arch_lock below.
> +                * 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);

Will do.

> > @@ -1567,10 +1576,10 @@ static int kvm_set_memslot(struct kvm *kvm,
> >   		/*
> >   		 * The arch-specific fields of the memslots could have changed
> >   		 * between releasing the slots_arch_lock in
> > -		 * install_new_memslots and here, so get a fresh copy of the
> > -		 * slots.
> > +		 * install_new_memslots and here, so get a fresh copy of these
> > +		 * fields.
> >   		 */
> > -		kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id));
> > +		kvm_copy_memslots_arch(slots, __kvm_memslots(kvm, as_id));
> >   	}
> >   
> >   	r = kvm_arch_prepare_memory_region(kvm, old, new, mem, change);
> > @@ -1587,8 +1596,6 @@ static int kvm_set_memslot(struct kvm *kvm,
> >   
> >   out_slots:
> >   	if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
> > -		slot = id_to_memslot(slots, old->id);
> > -		slot->flags &= ~KVM_MEMSLOT_INVALID;
> >   		slots = install_new_memslots(kvm, as_id, slots);
> >   	} else {
>
> The braces can be dropped since both branches are now single lines.
>
> >   		mutex_unlock(&kvm->slots_arch_lock);

Will drop them.

Thanks,
Maciej