| Message ID | 20180503175917.67937-1-marcorr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
you should also add the arm/mips/power maintainers so that they can review their struct kvm_arch if any of the content is passed directly to the hardware (e.g. with __pa). A quick looks seems to indicate that there is no issue. On 05/03/2018 07:59 PM, Marc Orr wrote: > The kvm struct is (currently) tens of kilo-bytes, which turns out to be a > large amount of memory to allocate contiguously via kzalloc. Thus, this > patch changes the kzalloc to a vzalloc, so that the memory allocation is > less likely to fail in resource-constrained environments. > > Signed-off-by: Marc Orr <marcorr@google.com> s390 parts seem to work Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Change-Id: Ide392e51991142d1325e06351231ac685e11d820 what is that? > --- > arch/x86/kvm/svm.c | 4 ++-- > arch/x86/kvm/vmx.c | 4 ++-- > include/linux/kvm_host.h | 5 +++-- > 3 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index b58787daf9f8..f990fda1b2f6 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1838,13 +1838,13 @@ static void __unregister_enc_region_locked(struct kvm *kvm, > > static struct kvm *svm_vm_alloc(void) > { > - struct kvm_svm *kvm_svm = kzalloc(sizeof(struct kvm_svm), GFP_KERNEL); > + struct kvm_svm *kvm_svm = vzalloc(sizeof(struct kvm_svm)); > return &kvm_svm->kvm; > } > > static void svm_vm_free(struct kvm *kvm) > { > - kfree(to_kvm_svm(kvm)); > + vfree(to_kvm_svm(kvm)); > } > > static void sev_vm_destroy(struct kvm *kvm) > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index aafcc9881e88..26ab153a293f 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -9949,13 +9949,13 @@ STACK_FRAME_NON_STANDARD(vmx_vcpu_run); > > static struct kvm *vmx_vm_alloc(void) > { > - struct kvm_vmx *kvm_vmx = kzalloc(sizeof(struct kvm_vmx), GFP_KERNEL); > + struct kvm_vmx *kvm_vmx = vzalloc(sizeof(struct kvm_vmx)); > return &kvm_vmx->kvm; > } > > static void vmx_vm_free(struct kvm *kvm) > { > - kfree(to_kvm_vmx(kvm)); > + vfree(to_kvm_vmx(kvm)); > } > > static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 6930c63126c7..cf7b6a3a4197 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -19,6 +19,7 @@ > #include <linux/preempt.h> > #include <linux/msi.h> > #include <linux/slab.h> > +#include <linux/vmalloc.h> > #include <linux/rcupdate.h> > #include <linux/ratelimit.h> > #include <linux/err.h> > @@ -810,12 +811,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu); > #ifndef __KVM_HAVE_ARCH_VM_ALLOC > static inline struct kvm *kvm_arch_alloc_vm(void) > { > - return kzalloc(sizeof(struct kvm), GFP_KERNEL); > + return vzalloc(sizeof(struct kvm)); > } > > static inline void kvm_arch_free_vm(struct kvm *kvm) > { > - kfree(kvm); > + vfree(kvm); > } > #endif >
On Fri, May 04, 2018 at 11:11:21AM +0200, Christian Borntraeger wrote: > you should also add the arm/mips/power maintainers so that they can review their > struct kvm_arch if any of the content is passed directly to the hardware (e.g. with > __pa). A quick looks seems to indicate that there is no issue. MIPS has additional restrictions because it has a software managed TLB, so we have to be careful where we access vmalloc'd memory, especially around entry point code which may not expect to receive a TLB refill exception. I'll have a deeper look later to see if it would obviously break anything. Thanks James > > On 05/03/2018 07:59 PM, Marc Orr wrote: > > The kvm struct is (currently) tens of kilo-bytes, which turns out to be a > > large amount of memory to allocate contiguously via kzalloc. Thus, this > > patch changes the kzalloc to a vzalloc, so that the memory allocation is > > less likely to fail in resource-constrained environments. > > > > Signed-off-by: Marc Orr <marcorr@google.com>
[fixing Christoffer's email address] Thanks for looping us in. On 04/05/18 10:11, Christian Borntraeger wrote: > you should also add the arm/mips/power maintainers so that they can review their > struct kvm_arch if any of the content is passed directly to the hardware (e.g. with > __pa). A quick looks seems to indicate that there is no issue. Well, there is a small issue, at least on arm64. Only VAs that are part of the linear mapping can easily be mapped at EL2 (so that we can play our kern_to_hyp_va trick on ARMv8.0 cores), and the kvm structure is assumed to be physically contiguous. Switch to a vmalloc'ed structure breaks these two requirements since, as you noticed it, the kvm struct is more than a single page. I'd suggest you let each architecture buy into this, or provide a kmalloc-based kvm_arch_{alloc,free}_vm for architectures that cannot use the vmalloc area for this. On arm64, we could use vmalloc on VHE-capable cores, and stick to kmalloc for the rest. > > On 05/03/2018 07:59 PM, Marc Orr wrote: >> The kvm struct is (currently) tens of kilo-bytes, which turns out to be a >> large amount of memory to allocate contiguously via kzalloc. Thus, this >> patch changes the kzalloc to a vzalloc, so that the memory allocation is >> less likely to fail in resource-constrained environments. >> >> Signed-off-by: Marc Orr <marcorr@google.com> > > s390 parts seem to work > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> Change-Id: Ide392e51991142d1325e06351231ac685e11d820 > > what is that? > > >> --- >> arch/x86/kvm/svm.c | 4 ++-- >> arch/x86/kvm/vmx.c | 4 ++-- >> include/linux/kvm_host.h | 5 +++-- >> 3 files changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index b58787daf9f8..f990fda1b2f6 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1838,13 +1838,13 @@ static void __unregister_enc_region_locked(struct kvm *kvm, >> >> static struct kvm *svm_vm_alloc(void) >> { >> - struct kvm_svm *kvm_svm = kzalloc(sizeof(struct kvm_svm), GFP_KERNEL); >> + struct kvm_svm *kvm_svm = vzalloc(sizeof(struct kvm_svm)); >> return &kvm_svm->kvm; >> } >> >> static void svm_vm_free(struct kvm *kvm) >> { >> - kfree(to_kvm_svm(kvm)); >> + vfree(to_kvm_svm(kvm)); >> } >> >> static void sev_vm_destroy(struct kvm *kvm) >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index aafcc9881e88..26ab153a293f 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -9949,13 +9949,13 @@ STACK_FRAME_NON_STANDARD(vmx_vcpu_run); >> >> static struct kvm *vmx_vm_alloc(void) >> { >> - struct kvm_vmx *kvm_vmx = kzalloc(sizeof(struct kvm_vmx), GFP_KERNEL); >> + struct kvm_vmx *kvm_vmx = vzalloc(sizeof(struct kvm_vmx)); >> return &kvm_vmx->kvm; >> } >> >> static void vmx_vm_free(struct kvm *kvm) >> { >> - kfree(to_kvm_vmx(kvm)); >> + vfree(to_kvm_vmx(kvm)); >> } >> >> static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs) >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 6930c63126c7..cf7b6a3a4197 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -19,6 +19,7 @@ >> #include <linux/preempt.h> >> #include <linux/msi.h> >> #include <linux/slab.h> >> +#include <linux/vmalloc.h> >> #include <linux/rcupdate.h> >> #include <linux/ratelimit.h> >> #include <linux/err.h> >> @@ -810,12 +811,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu); >> #ifndef __KVM_HAVE_ARCH_VM_ALLOC >> static inline struct kvm *kvm_arch_alloc_vm(void) >> { >> - return kzalloc(sizeof(struct kvm), GFP_KERNEL); >> + return vzalloc(sizeof(struct kvm)); >> } >> >> static inline void kvm_arch_free_vm(struct kvm *kvm) >> { >> - kfree(kvm); >> + vfree(kvm); >> } >> #endif >> > Thanks, M.
On 04/05/2018 11:30, Marc Zyngier wrote: >> you should also add the arm/mips/power maintainers so that they can review their >> struct kvm_arch if any of the content is passed directly to the hardware (e.g. with >> __pa). A quick looks seems to indicate that there is no issue. > Well, there is a small issue, at least on arm64. Only VAs that are part > of the linear mapping can easily be mapped at EL2 (so that we can play > our kern_to_hyp_va trick on ARMv8.0 cores), and the kvm structure is > assumed to be physically contiguous. > > Switch to a vmalloc'ed structure breaks these two requirements since, as > you noticed it, the kvm struct is more than a single page. > > I'd suggest you let each architecture buy into this, or provide a > kmalloc-based kvm_arch_{alloc,free}_vm for architectures that cannot use > the vmalloc area for this. On arm64, we could use vmalloc on VHE-capable > cores, and stick to kmalloc for the rest. > Alternatively you could have a #define __KVM_KMALLOC_STRUCT_KVM and define it in arch/arm*/include/asm/kvm_host.h. Paolo
I just sent out v3, which updates the patch to maintain the kzalloc behavior for arm & arm64. Please review that one going forward. >> Change-Id: Ide392e51991142d1325e06351231ac685e11d820 > what is that? Woops. That's meta-data inserted my git hooks. Thanks for catching this! Thanks, Marc
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index b58787daf9f8..f990fda1b2f6 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -1838,13 +1838,13 @@ static void __unregister_enc_region_locked(struct kvm *kvm, static struct kvm *svm_vm_alloc(void) { - struct kvm_svm *kvm_svm = kzalloc(sizeof(struct kvm_svm), GFP_KERNEL); + struct kvm_svm *kvm_svm = vzalloc(sizeof(struct kvm_svm)); return &kvm_svm->kvm; } static void svm_vm_free(struct kvm *kvm) { - kfree(to_kvm_svm(kvm)); + vfree(to_kvm_svm(kvm)); } static void sev_vm_destroy(struct kvm *kvm) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index aafcc9881e88..26ab153a293f 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -9949,13 +9949,13 @@ STACK_FRAME_NON_STANDARD(vmx_vcpu_run); static struct kvm *vmx_vm_alloc(void) { - struct kvm_vmx *kvm_vmx = kzalloc(sizeof(struct kvm_vmx), GFP_KERNEL); + struct kvm_vmx *kvm_vmx = vzalloc(sizeof(struct kvm_vmx)); return &kvm_vmx->kvm; } static void vmx_vm_free(struct kvm *kvm) { - kfree(to_kvm_vmx(kvm)); + vfree(to_kvm_vmx(kvm)); } static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 6930c63126c7..cf7b6a3a4197 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -19,6 +19,7 @@ #include <linux/preempt.h> #include <linux/msi.h> #include <linux/slab.h> +#include <linux/vmalloc.h> #include <linux/rcupdate.h> #include <linux/ratelimit.h> #include <linux/err.h> @@ -810,12 +811,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu); #ifndef __KVM_HAVE_ARCH_VM_ALLOC static inline struct kvm *kvm_arch_alloc_vm(void) { - return kzalloc(sizeof(struct kvm), GFP_KERNEL); + return vzalloc(sizeof(struct kvm)); } static inline void kvm_arch_free_vm(struct kvm *kvm) { - kfree(kvm); + vfree(kvm); } #endif
The kvm struct is (currently) tens of kilo-bytes, which turns out to be a large amount of memory to allocate contiguously via kzalloc. Thus, this patch changes the kzalloc to a vzalloc, so that the memory allocation is less likely to fail in resource-constrained environments. Signed-off-by: Marc Orr <marcorr@google.com> Change-Id: Ide392e51991142d1325e06351231ac685e11d820 --- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx.c | 4 ++-- include/linux/kvm_host.h | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-)