[PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[Nicholas Piggin]

Increasing the max VCPUs on powerpc makes the kvm_arch member offset
great enough that some assembly breaks due to addressing constants
overflowing field widths.

Moving the vcpus array to the end of struct kvm prevents this from
happening. It has the side benefit that moving the large array out
from the middle of the structure should help keep other commonly
accessed fields in the same or adjacent cache lines.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

It would next be possible to now make this a dynamically sized array,
and make the KVM_MAX_VCPUS more dynamic, however x86 kvm_svm uses its
own scheme rather than kvm_arch for some reason.

Thanks,
Nick

 include/linux/kvm_host.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0f18df7fe874..78cd9b63a6a5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -553,7 +553,6 @@ struct kvm {
 	struct mutex slots_arch_lock;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];
-	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
 
 	/* Used to wait for completion of MMU notifiers.  */
 	spinlock_t mn_invalidate_lock;
@@ -623,6 +622,9 @@ struct kvm {
 	struct notifier_block pm_notifier;
 #endif
 	char stats_id[KVM_STATS_NAME_SIZE];
+
+	/* This array can be very large, so keep it at the bottom */
+	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
 };
 
 #define kvm_err(fmt, ...) \
-- 
2.23.0

Re: [PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[Sean Christopherson]

+Juergen and Marc

On Fri, Nov 05, 2021, Nicholas Piggin wrote:
> Increasing the max VCPUs on powerpc makes the kvm_arch member offset
> great enough that some assembly breaks due to addressing constants
> overflowing field widths.
> 
> Moving the vcpus array to the end of struct kvm prevents this from
> happening. It has the side benefit that moving the large array out
> from the middle of the structure should help keep other commonly
> accessed fields in the same or adjacent cache lines.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> 
> It would next be possible to now make this a dynamically sized array,
> and make the KVM_MAX_VCPUS more dynamic

Marc has a mostly-baked series to use an xarray[1][2] that AFAICT would be well
received.  That has my vote, assuming it can get into 5.16.  Marc or Juergen,
are either of you actively working on that?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/vcpu-xarray
[2] https://lkml.kernel.org/r/871r65wwk7.wl-maz@kernel.org

> however x86 kvm_svm uses its own scheme rather than kvm_arch for some reason.

What's the problem in kvm_svm?

> Thanks,
> Nick
> 
>  include/linux/kvm_host.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0f18df7fe874..78cd9b63a6a5 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -553,7 +553,6 @@ struct kvm {
>  	struct mutex slots_arch_lock;
>  	struct mm_struct *mm; /* userspace tied to this vm */
>  	struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];
> -	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
>  
>  	/* Used to wait for completion of MMU notifiers.  */
>  	spinlock_t mn_invalidate_lock;
> @@ -623,6 +622,9 @@ struct kvm {
>  	struct notifier_block pm_notifier;
>  #endif
>  	char stats_id[KVM_STATS_NAME_SIZE];
> +
> +	/* This array can be very large, so keep it at the bottom */
> +	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
>  };
>  
>  #define kvm_err(fmt, ...) \
> -- 
> 2.23.0
> 

Re: [PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[Marc Zyngier]

On Fri, 05 Nov 2021 14:49:57 +0000,
Sean Christopherson <seanjc@google.com> wrote:
> 
> +Juergen and Marc
> 
> On Fri, Nov 05, 2021, Nicholas Piggin wrote:
> > Increasing the max VCPUs on powerpc makes the kvm_arch member offset
> > great enough that some assembly breaks due to addressing constants
> > overflowing field widths.
> > 
> > Moving the vcpus array to the end of struct kvm prevents this from
> > happening. It has the side benefit that moving the large array out
> > from the middle of the structure should help keep other commonly
> > accessed fields in the same or adjacent cache lines.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > 
> > It would next be possible to now make this a dynamically sized array,
> > and make the KVM_MAX_VCPUS more dynamic
> 
> Marc has a mostly-baked series to use an xarray[1][2] that AFAICT
> would be well received.  That has my vote, assuming it can get into
> 5.16.  Marc or Juergen, are either of you actively working on that?

I've just revived it, as it needed some RISC-V changes. I'll post it
in a jiffy.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[Nicholas Piggin]

Excerpts from Sean Christopherson's message of November 6, 2021 12:49 am:
> +Juergen and Marc
> 
> On Fri, Nov 05, 2021, Nicholas Piggin wrote:
>> Increasing the max VCPUs on powerpc makes the kvm_arch member offset
>> great enough that some assembly breaks due to addressing constants
>> overflowing field widths.
>> 
>> Moving the vcpus array to the end of struct kvm prevents this from
>> happening. It has the side benefit that moving the large array out
>> from the middle of the structure should help keep other commonly
>> accessed fields in the same or adjacent cache lines.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> 
>> It would next be possible to now make this a dynamically sized array,
>> and make the KVM_MAX_VCPUS more dynamic
> 
> Marc has a mostly-baked series to use an xarray[1][2] that AFAICT would be well
> received.  That has my vote, assuming it can get into 5.16.  Marc or Juergen,
> are either of you actively working on that?
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/vcpu-xarray
> [2] https://lkml.kernel.org/r/871r65wwk7.wl-maz@kernel.org

Seems like a good idea if it can allow vcpu structs to be allocated to 
preferred nodes.

>> however x86 kvm_svm uses its own scheme rather than kvm_arch for some reason.
> 
> What's the problem in kvm_svm?

It embeds a struct kvm so it couldn't be variable sized.

Thanks,
Nick

Re: [PATCH] KVM: move struct kvm_vcpu * array to the bottom of struct kvm

[Sean Christopherson]

On Sat, Nov 06, 2021, Nicholas Piggin wrote:
> Excerpts from Sean Christopherson's message of November 6, 2021 12:49 am:
> >> It would next be possible to now make this a dynamically sized array,
> >> and make the KVM_MAX_VCPUS more dynamic
> > 
> > Marc has a mostly-baked series to use an xarray[1][2] that AFAICT would be well
> > received.  That has my vote, assuming it can get into 5.16.  Marc or Juergen,
> > are either of you actively working on that?
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/vcpu-xarray
> > [2] https://lkml.kernel.org/r/871r65wwk7.wl-maz@kernel.org
> 
> Seems like a good idea if it can allow vcpu structs to be allocated to 
> preferred nodes.
> 
> >> however x86 kvm_svm uses its own scheme rather than kvm_arch for some reason.
> > 
> > What's the problem in kvm_svm?
> 
> It embeds a struct kvm so it couldn't be variable sized.

Oooh, when you said "dynamically sized" I thought you meant

	struct kvm_vcpu *vcpus;

...

	kvm->vcpus = kcalloc(...);

Anyways, SVM and VMX are quite different despited both being x86, to keep them
separated without requiring an extra allocation, kvm_svm and kvm_vmx embed kvm.
Ditto for vcpu_vmx and vmx_svm.  Obviously not a hard requirement, but there also
hasn't been a reason not to do embed kvm/kvm_vcpu.