shrink vcpu_vmx down to order 2
Subsystems (3)
v7 cover shrink vcpu_vmx down to order 2
Compared to the last version, I've:
(1) Added a comment to explain the FPU checks in kvm_arch_init()
(2) Changed the kmem_cache_create_usercopy() to kmem_cache_create()
Marc Orr (2):
kvm: x86: Use task structs fpu field for user
kvm: x86: Dynamically allocate guest_fpu
arch/x86/include/asm/kvm_host.h | 10 +++--- arch/x86/kvm/svm.c | 10 ++++++ arch/x86/kvm/vmx.c | 10 ++++++ arch/x86/kvm/x86.c | 55 ++++++++++++++++++++++++--------- 4 files changed, 65 insertions(+), 20 deletions(-)
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
PATCH v7 1/2 kvm: x86: Use task structs fpu field for user
Previously, x86's instantiation of 'struct kvm_vcpu_arch' added an fpu field to save/restore fpu-related architectural state, which will differ from kvm's fpu state. However, this is redundant to the 'struct fpu' field, called fpu, embedded in the task struct, via the thread field. Thus, this patch removes the user_fpu field from the kvm_vcpu_arch struct and replaces it with the task struct's fpu field.
This change is significant because the fpu struct is actually quite large. For example, on the system used to develop this patch, this change reduces the size of the vcpu_vmx struct from 23680 bytes down to 19520 bytes, when building the kernel with kvmconfig. This reduction in the size of the vcpu_vmx struct moves us closer to being able to allocate the struct at order 2, rather than order 3.
--- arch/x86/include/asm/kvm_host.h | 7 +++---- arch/x86/kvm/x86.c | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -4
@@ -601,16 +601,15 @@ struct kvm_vcpu_arch {/* * QEMU userspace and the guest each have their own FPU state. -* In vcpu_run, we switch between the user and guest FPU contexts. -* While running a VCPU, the VCPU thread will have the guest FPU -* context. +* In vcpu_run, we switch between the user, maintained in the +* task_struct struct, and guest FPU contexts. While running a VCPU, +* the VCPU thread will have the guest FPU context. * * Note that while the PKRU state lives inside the fpu registers, * it is switched out separately at VMENTER and VMEXIT time. The * "guest_fpu" state here contains the guest FPU context, with the * host PRKU bits. */ -struct fpu user_fpu;struct fpu guest_fpu;u64 xcr0;
arch/x86/kvm/x86.c +2 -2
@@ -7999,7 +7999,7 @@ static int complete_emulated_mmio(struct kvm_vcpu *vcpu)static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu) {preempt_disable (); -copy_fpregs_to_fpstate (&vcpu->arch.user_fpu); +copy_fpregs_to_fpstate (¤t->thread.fpu);/* PKRU is separately restored in kvm_x86_ops->run. */ __copy_kernel_to_fpregs (&vcpu->arch.guest_fpu.state, ~XFEATURE_MASK_PKRU );
@@ -8012,7 +8012,7 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) {preempt_disable ();copy_fpregs_to_fpstate (&vcpu->arch.guest_fpu); -copy_kernel_to_fpregs (&vcpu->arch.user_fpu.state); +copy_kernel_to_fpregs (¤t->thread.fpu.state);preempt_enable (); ++vcpu->stat.fpu_reload;trace_kvm_fpu (0 );
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
PATCH v7 2/2 kvm: x86: Dynamically allocate guest_fpu
In reply to Marc OrrPreviously, the guest_fpu field was embedded in the kvm_vcpu_arch struct. Unfortunately, the field is quite large, (e.g., 4352 bytes on my current setup). This bloats the kvm_vcpu_arch struct for x86 into an order 3 memory allocation, which can become a problem on overcommitted machines. Thus, this patch moves the fpu state outside of the kvm_vcpu_arch struct.
With this patch applied, the kvm_vcpu_arch struct is reduced to 15168 bytes for vmx on my setup when building the kernel with kvmconfig.
--- arch/x86/include/asm/kvm_host.h | 3 +- arch/x86/kvm/svm.c | 10 +++++++ arch/x86/kvm/vmx.c | 10 +++++++ arch/x86/kvm/x86.c | 51 ++++++++++++++++++++++++--------- 4 files changed, 60 insertions(+), 14 deletions(-)
arch/x86/include/asm/kvm_host.h +2 -1
@@ -610,7 +610,7 @@ struct kvm_vcpu_arch {* "guest_fpu" state here contains the guest FPU context, with the * host PRKU bits. */ -struct fpu guest_fpu; +struct fpu *guest_fpu;u64 xcr0;u64 guest_supported_xcr0;
@@ -1194,6 +1194,7 @@ struct kvm_arch_async_pf { };extern struct kvm_x86_ops *kvm_x86_ops; +extern struct kmem_cache *x86_fpu_cache;#define __KVM_HAVE_ARCH_VM_ALLOCstatic inline struct kvm *kvm_arch_alloc_vm (void )
arch/x86/kvm/svm.c +10 -0
@@ -2121,6 +2121,13 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) goto out; } + svm->vcpu.arch.guest_fpu =kmem_cache_zalloc (x86_fpu_cache,GFP_KERNEL ); +if (!svm->vcpu.arch.guest_fpu) { +printk (KERN_ERR "kvm: failed to allocate vcpu's fpu\n" ); + err = -ENOMEM ; + goto free_partial_svm; + } + err =kvm_vcpu_init (&svm->vcpu, kvm, id);if (err) goto free_svm;
@@ -2180,6 +2187,8 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) uninit:kvm_vcpu_uninit (&svm->vcpu); free_svm: +kmem_cache_free (x86_fpu_cache, svm->vcpu.arch.guest_fpu); +free_partial_svm:kmem_cache_free (kvm_vcpu_cache, svm); out:return ERR_PTR (err);
@@ -2194,6 +2203,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)__free_page (virt_to_page (svm->nested.hsave));__free_pages (virt_to_page (svm->nested.msrpm),MSRPM_ALLOC_ORDER );kvm_vcpu_uninit (vcpu); +kmem_cache_free (x86_fpu_cache, svm->vcpu.arch.guest_fpu);kmem_cache_free (kvm_vcpu_cache, svm);/* * The vmcb page can be recycled, causing a false negative in
arch/x86/kvm/vmx.c +10 -0
@@ -11476,6 +11476,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)free_loaded_vmcs (vmx->loaded_vmcs);kfree (vmx->guest_msrs);kvm_vcpu_uninit (vcpu); +kmem_cache_free (x86_fpu_cache, vmx->vcpu.arch.guest_fpu);kmem_cache_free (kvm_vcpu_cache, vmx); }
@@ -11489,6 +11490,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)if (!vmx)return ERR_PTR (-ENOMEM ); + vmx->vcpu.arch.guest_fpu =kmem_cache_zalloc (x86_fpu_cache,GFP_KERNEL ); +if (!vmx->vcpu.arch.guest_fpu) { +printk (KERN_ERR "kvm: failed to allocate vcpu's fpu\n" ); + err = -ENOMEM ; + goto free_partial_vcpu; + } + vmx->vpid =allocate_vpid (); err =kvm_vcpu_init (&vmx->vcpu, kvm, id);
@@ -11576,6 +11584,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)kvm_vcpu_uninit (&vmx->vcpu); free_vcpu:free_vpid (vmx->vpid); +kmem_cache_free (x86_fpu_cache, vmx->vcpu.arch.guest_fpu); +free_partial_vcpu:kmem_cache_free (kvm_vcpu_cache, vmx);return ERR_PTR (err); }
arch/x86/kvm/x86.c +38 -13
143 lines not on this page · raw message
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
Re: [kvm PATCH v7 2/2] kvm: x86: Dynamically allocate guest_fpu
arch/x86/kvm/x86.c int kvm_arch_init(void *opaque) · line 6838
Quoted 9 lines · Marc Orr
arch/x86/kvm/x86.c +8
-0r = -ENOMEM ; + x86_fpu_cache =kmem_cache_create_usercopy ( +"x86_fpu" , + fpu_kernel_xstate_size, + __alignof__(struct fpu ), +SLAB_ACCOUNT , + offsetof(struct fpu , state), + fpu_kernel_xstate_size, +NULL );
I thought we came to the conclusion with Paulo that this should not be "usercopy" at all.
Did you send out an old version?
Re: [kvm PATCH v7 2/2] kvm: x86: Dynamically allocate guest_fpu
Quoted 11 lines · Dave Hansen
arch/x86/kvm/x86.c int kvm_arch_init(void *opaque) · line 6838
Quoted 9 lines · Marc Orr
r = -ENOMEM;arch/x86/kvm/x86.c +8
-0+ x86_fpu_cache =kmem_cache_create_usercopy ( +"x86_fpu" , + fpu_kernel_xstate_size, + __alignof__(struct fpu ), +SLAB_ACCOUNT , + offsetof(struct fpu , state), + fpu_kernel_xstate_size, +NULL );
Quoted 5 lines
I thought we came to the conclusion with Paulo that this should not be "usercopy" at all.
Did you send out an old version?
Oops. Yes, I sent the old version. Re-sending now.
Paolo Bonzini
maintainerRe: [kvm PATCH v7 2/2] kvm: x86: Dynamically allocate guest_fpu
In reply to Marc Orrarch/x86/kvm/x86.c int kvm_arch_init(void *opaque) · line 6833
Quoted 3 lines · Marc Orr
arch/x86/kvm/x86.c +3
-0+ x86_fpu_cache = kmem_cache_create_usercopy( + "x86_fpu", + fpu_kernel_xstate_size,
This unfortunately is wrong because there are other members in struct fpu before the fpregs_state union. It's enough to run a guest and then rmmod kvm to see slub errors which are actually caused by memory corruption.
The right way to size it is shown in fpu__init_task_struct_size but for now I'll revert it to sizeof(struct fpu). I have plans to move fsave/fxsave/xsave directly in KVM, without using the kernel FPU helpers, and actually this guest_fpu thing will come in handy for that. :) Once it's done, the size of the object in the cache will be something like kvm_xstate_size.
Paolo
Quoted 5 lines
arch/x86/kvm/x86.c +5
-0+ __alignof__(struct fpu ), +SLAB_ACCOUNT , + offsetof(struct fpu , state), + fpu_kernel_xstate_size, + NULL);
Re: [kvm PATCH v7 2/2] kvm: x86: Dynamically allocate guest_fpu
Quoted 11 lines · Paolo Bonzini
arch/x86/kvm/x86.c int kvm_arch_init(void *opaque) · line 6833
Quoted 3 lines · Marc Orr
arch/x86/kvm/x86.c +3
-0+ x86_fpu_cache = kmem_cache_create_usercopy( + "x86_fpu", + fpu_kernel_xstate_size,This unfortunately is wrong because there are other members in struct fpu before the
fpregs_stateunion. It's enough to run a guest and then rmmod kvm to see slub errors which are actually caused by memory corruption.
Quoted 16 lines
The right way to size it is shown in
fpu__init_task_struct_sizebut for now I'll revert it to sizeof(struct fpu). I have plans to move fsave/fxsave/xsave directly in KVM, without using the kernel FPU helpers, and actually thisguest_fputhing will come in handy for that. :) Once it's done, the size of the object in the cache will be something likekvm_xstate_size.Paolo
Quoted 5 lines
arch/x86/kvm/x86.c +5
-0+ __alignof__(struct fpu ), +SLAB_ACCOUNT , + offsetof(struct fpu , state), + fpu_kernel_xstate_size, + NULL);
Oops. Thanks for debugging, explaining and fixing!