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

Commit Message

Maciej S. Szmigiero Sept. 20, 2021, 9:38 p.m. UTC

From: "Maciej S. Szmigiero" <[e-mail]>

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.

Signed-off-by: Maciej S. Szmigiero <[e-mail]>
---
 virt/kvm/kvm_main.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Sean Christopherson Oct. 19, 2021, 11:55 p.m. UTC | #1

On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <[e-mail]>
> 
> 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.

> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
> ---
>  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.

> +}
> +
>  /*
>   * 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);

> @@ -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);

Maciej S. Szmigiero Oct. 20, 2021, 6:41 p.m. UTC | #2

On 20.10.2021 01:55, Sean Christopherson wrote:
> On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>   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

Sean Christopherson Oct. 20, 2021, 6:57 p.m. UTC | #3

On Wed, Oct 20, 2021, Maciej S. Szmigiero wrote:
> On 20.10.2021 01:55, Sean Christopherson wrote:
> > On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
> > 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().

I was more worried about the compiler using SIMD instructions.  I assume the kernel
build process has lots of guards in place to prevent such shenanigans, but on the
other hand I _know_ mempcy() is safe :-)

Maciej S. Szmigiero Oct. 20, 2021, 6:58 p.m. UTC | #4

On 20.10.2021 20:57, Sean Christopherson wrote:
> On Wed, Oct 20, 2021, Maciej S. Szmigiero wrote:
>> On 20.10.2021 01:55, Sean Christopherson wrote:
>>> On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
>>> 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().
> 
> I was more worried about the compiler using SIMD instructions.  I assume the kernel
> build process has lots of guards in place to prevent such shenanigans, but on the
> other hand I _know_ mempcy() is safe :-)
> 

So we will play safe and use mempcy() then :)

Thanks,
Maciej

Patch

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;
+}
+
 /*
  * 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
@@ -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 {
 		mutex_unlock(&kvm->slots_arch_lock);