| Message ID | 20211105034949.1397997-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: move struct kvm_vcpu * array to the bottom of struct kvm | expand |
+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 >
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.
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
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.
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, ...) \
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(-)