| Message ID | 20191203210825.26827-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: vmx: Stop wasting a page for guest_msrs | expand |
> On 3 Dec 2019, at 23:08, Jim Mattson <jmattson@google.com> wrote: > > We will never need more guest_msrs than there are indices in > vmx_msr_index. Thus, at present, the guest_msrs array will not exceed > 168 bytes. > > Signed-off-by: Jim Mattson <jmattson@google.com> Weird that this wasn’t always like this :) Reviewed-by: Liran Alon <liran.alon@oracle.com> -Liran > --- > arch/x86/kvm/vmx/vmx.c | 14 ++------------ > arch/x86/kvm/vmx/vmx.h | 8 +++++++- > 2 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 1b9ab4166397d..0b3c7524456f1 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -443,7 +443,7 @@ static unsigned long host_idt_base; > * support this emulation, IA32_STAR must always be included in > * vmx_msr_index[], even in i386 builds. > */ > -const u32 vmx_msr_index[] = { > +const u32 vmx_msr_index[NR_GUEST_MSRS] = { > #ifdef CONFIG_X86_64 > MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR, > #endif > @@ -6666,7 +6666,6 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > free_vpid(vmx->vpid); > nested_vmx_free_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.user_fpu); > kmem_cache_free(x86_fpu_cache, vmx->vcpu.arch.guest_fpu); > @@ -6723,13 +6722,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > goto uninit_vcpu; > } > > - vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); > - BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) * sizeof(vmx->guest_msrs[0]) > - > PAGE_SIZE); > - > - if (!vmx->guest_msrs) > - goto free_pml; > - > for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) { > u32 index = vmx_msr_index[i]; > u32 data_low, data_high; > @@ -6760,7 +6752,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > err = alloc_loaded_vmcs(&vmx->vmcs01); > if (err < 0) > - goto free_msrs; > + goto free_pml; > > msr_bitmap = vmx->vmcs01.msr_bitmap; > vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R); > @@ -6822,8 +6814,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > free_vmcs: > free_loaded_vmcs(vmx->loaded_vmcs); > -free_msrs: > - kfree(vmx->guest_msrs); > free_pml: > vmx_destroy_pml_buffer(vmx); > uninit_vcpu: > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 7c1b978b2df44..08bc24fa59909 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -22,6 +22,12 @@ extern u32 get_umwait_control_msr(void); > > #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4)) > > +#ifdef CONFIG_X86_64 > +#define NR_GUEST_MSRS 7 > +#else > +#define NR_GUEST_MSRS 4 > +#endif > + > #define NR_LOADSTORE_MSRS 8 > > struct vmx_msrs { > @@ -206,7 +212,7 @@ struct vcpu_vmx { > u32 idt_vectoring_info; > ulong rflags; > > - struct shared_msr_entry *guest_msrs; > + struct shared_msr_entry guest_msrs[NR_GUEST_MSRS]; > int nmsrs; > int save_nmsrs; > bool guest_msrs_ready; > -- > 2.24.0.393.g34dc348eaf-goog >
On Tue, Dec 03, 2019 at 01:08:25PM -0800, Jim Mattson wrote: > We will never need more guest_msrs than there are indices in > vmx_msr_index. Thus, at present, the guest_msrs array will not exceed > 168 bytes. > > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 14 ++------------ > arch/x86/kvm/vmx/vmx.h | 8 +++++++- > 2 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 1b9ab4166397d..0b3c7524456f1 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -443,7 +443,7 @@ static unsigned long host_idt_base; > * support this emulation, IA32_STAR must always be included in > * vmx_msr_index[], even in i386 builds. > */ > -const u32 vmx_msr_index[] = { > +const u32 vmx_msr_index[NR_GUEST_MSRS] = { What if we keep this as is and add BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) != NR_SHARED_MSRS); in setup_msrs()? That way the build will fail if someone adds an MSR but forgets to update the #define. With this change, gcc only spits out a warning if the number of elements exceeds the size of the array and presumably drops the extra elements on the floor. > #ifdef CONFIG_X86_64 > MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR, > #endif > @@ -6666,7 +6666,6 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > free_vpid(vmx->vpid); > nested_vmx_free_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.user_fpu); > kmem_cache_free(x86_fpu_cache, vmx->vcpu.arch.guest_fpu); > @@ -6723,13 +6722,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > goto uninit_vcpu; > } > > - vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); > - BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) * sizeof(vmx->guest_msrs[0]) > - > PAGE_SIZE); > - > - if (!vmx->guest_msrs) > - goto free_pml; > - > for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) { > u32 index = vmx_msr_index[i]; > u32 data_low, data_high; > @@ -6760,7 +6752,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > err = alloc_loaded_vmcs(&vmx->vmcs01); > if (err < 0) > - goto free_msrs; > + goto free_pml; > > msr_bitmap = vmx->vmcs01.msr_bitmap; > vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R); > @@ -6822,8 +6814,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > free_vmcs: > free_loaded_vmcs(vmx->loaded_vmcs); > -free_msrs: > - kfree(vmx->guest_msrs); > free_pml: > vmx_destroy_pml_buffer(vmx); > uninit_vcpu: > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 7c1b978b2df44..08bc24fa59909 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -22,6 +22,12 @@ extern u32 get_umwait_control_msr(void); > > #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4)) > > +#ifdef CONFIG_X86_64 > +#define NR_GUEST_MSRS 7 > +#else > +#define NR_GUEST_MSRS 4 > +#endif As much as I hate the "shared msrs" terminology, "guest msrs" is even more confusing and misleading. NR_SHARED_MSRS? > + > #define NR_LOADSTORE_MSRS 8 > > struct vmx_msrs { > @@ -206,7 +212,7 @@ struct vcpu_vmx { > u32 idt_vectoring_info; > ulong rflags; > > - struct shared_msr_entry *guest_msrs; > + struct shared_msr_entry guest_msrs[NR_GUEST_MSRS]; > int nmsrs; > int save_nmsrs; > bool guest_msrs_ready; > -- > 2.24.0.393.g34dc348eaf-goog >
On Tue, Dec 3, 2019 at 2:59 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Dec 03, 2019 at 01:08:25PM -0800, Jim Mattson wrote: > > We will never need more guest_msrs than there are indices in > > vmx_msr_index. Thus, at present, the guest_msrs array will not exceed > > 168 bytes. > > > > Signed-off-by: Jim Mattson <jmattson@google.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 14 ++------------ > > arch/x86/kvm/vmx/vmx.h | 8 +++++++- > > 2 files changed, 9 insertions(+), 13 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 1b9ab4166397d..0b3c7524456f1 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -443,7 +443,7 @@ static unsigned long host_idt_base; > > * support this emulation, IA32_STAR must always be included in > > * vmx_msr_index[], even in i386 builds. > > */ > > -const u32 vmx_msr_index[] = { > > +const u32 vmx_msr_index[NR_GUEST_MSRS] = { > > What if we keep this as is and add > > BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) != NR_SHARED_MSRS); > > in setup_msrs()? That way the build will fail if someone adds an MSR but > forgets to update the #define. With this change, gcc only spits out a > warning if the number of elements exceeds the size of the array and > presumably drops the extra elements on the floor. Instead of setup_msrs(), what if I add that BUILD_BUG_ON() in vmx_create_vcpu(), where I'm getting rid of the old BUILD_BUG_ON() regarding ARRAY_SIZE(vmx_msr_index)? > > #ifdef CONFIG_X86_64 > > MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR, > > #endif > > @@ -6666,7 +6666,6 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > > free_vpid(vmx->vpid); > > nested_vmx_free_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.user_fpu); > > kmem_cache_free(x86_fpu_cache, vmx->vcpu.arch.guest_fpu); > > @@ -6723,13 +6722,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > goto uninit_vcpu; > > } > > > > - vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); > > - BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) * sizeof(vmx->guest_msrs[0]) > > - > PAGE_SIZE); > > - > > - if (!vmx->guest_msrs) > > - goto free_pml; > > - > > for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) { > > u32 index = vmx_msr_index[i]; > > u32 data_low, data_high; > > @@ -6760,7 +6752,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > > > err = alloc_loaded_vmcs(&vmx->vmcs01); > > if (err < 0) > > - goto free_msrs; > > + goto free_pml; > > > > msr_bitmap = vmx->vmcs01.msr_bitmap; > > vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R); > > @@ -6822,8 +6814,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > > > free_vmcs: > > free_loaded_vmcs(vmx->loaded_vmcs); > > -free_msrs: > > - kfree(vmx->guest_msrs); > > free_pml: > > vmx_destroy_pml_buffer(vmx); > > uninit_vcpu: > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > index 7c1b978b2df44..08bc24fa59909 100644 > > --- a/arch/x86/kvm/vmx/vmx.h > > +++ b/arch/x86/kvm/vmx/vmx.h > > @@ -22,6 +22,12 @@ extern u32 get_umwait_control_msr(void); > > > > #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4)) > > > > +#ifdef CONFIG_X86_64 > > +#define NR_GUEST_MSRS 7 > > +#else > > +#define NR_GUEST_MSRS 4 > > +#endif > > As much as I hate the "shared msrs" terminology, "guest msrs" is even > more confusing and misleading. NR_SHARED_MSRS? > > > + > > #define NR_LOADSTORE_MSRS 8 > > > > struct vmx_msrs { > > @@ -206,7 +212,7 @@ struct vcpu_vmx { > > u32 idt_vectoring_info; > > ulong rflags; > > > > - struct shared_msr_entry *guest_msrs; > > + struct shared_msr_entry guest_msrs[NR_GUEST_MSRS]; > > int nmsrs; > > int save_nmsrs; > > bool guest_msrs_ready; > > -- > > 2.24.0.393.g34dc348eaf-goog > >
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 1b9ab4166397d..0b3c7524456f1 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -443,7 +443,7 @@ static unsigned long host_idt_base; * support this emulation, IA32_STAR must always be included in * vmx_msr_index[], even in i386 builds. */ -const u32 vmx_msr_index[] = { +const u32 vmx_msr_index[NR_GUEST_MSRS] = { #ifdef CONFIG_X86_64 MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR, #endif @@ -6666,7 +6666,6 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) free_vpid(vmx->vpid); nested_vmx_free_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.user_fpu); kmem_cache_free(x86_fpu_cache, vmx->vcpu.arch.guest_fpu); @@ -6723,13 +6722,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) goto uninit_vcpu; } - vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); - BUILD_BUG_ON(ARRAY_SIZE(vmx_msr_index) * sizeof(vmx->guest_msrs[0]) - > PAGE_SIZE); - - if (!vmx->guest_msrs) - goto free_pml; - for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) { u32 index = vmx_msr_index[i]; u32 data_low, data_high; @@ -6760,7 +6752,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) err = alloc_loaded_vmcs(&vmx->vmcs01); if (err < 0) - goto free_msrs; + goto free_pml; msr_bitmap = vmx->vmcs01.msr_bitmap; vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R); @@ -6822,8 +6814,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) free_vmcs: free_loaded_vmcs(vmx->loaded_vmcs); -free_msrs: - kfree(vmx->guest_msrs); free_pml: vmx_destroy_pml_buffer(vmx); uninit_vcpu: diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 7c1b978b2df44..08bc24fa59909 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -22,6 +22,12 @@ extern u32 get_umwait_control_msr(void); #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4)) +#ifdef CONFIG_X86_64 +#define NR_GUEST_MSRS 7 +#else +#define NR_GUEST_MSRS 4 +#endif + #define NR_LOADSTORE_MSRS 8 struct vmx_msrs { @@ -206,7 +212,7 @@ struct vcpu_vmx { u32 idt_vectoring_info; ulong rflags; - struct shared_msr_entry *guest_msrs; + struct shared_msr_entry guest_msrs[NR_GUEST_MSRS]; int nmsrs; int save_nmsrs; bool guest_msrs_ready;
We will never need more guest_msrs than there are indices in vmx_msr_index. Thus, at present, the guest_msrs array will not exceed 168 bytes. Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/vmx/vmx.c | 14 ++------------ arch/x86/kvm/vmx/vmx.h | 8 +++++++- 2 files changed, 9 insertions(+), 13 deletions(-)