kvm: vmx: Stop wasting a page for guest_msrs

PATCH Superseded Jim Mattson 3 participants 4 messages Dec 3, 2019

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jim Mattson

author

2019-12-03 21:08 UTC #0 lore raw

PATCH 1/1 kvm: vmx: Stop wasting a page for guest_msrs

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 <[email protected]>

---
 arch/x86/kvm/vmx/vmx.c | 14 ++------------
 arch/x86/kvm/vmx/vmx.h |  8 +++++++-
 2 files changed, 9 insertions(+), 13 deletions(-)

arch/x86/kvm/vmx/vmx.c +2 ~~-12~~

index 1b9ab4166397d..0b3c7524456f1 100644
@@ -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:

arch/x86/kvm/vmx/vmx.h +7 -1

index 7c1b978b2df44..08bc24fa59909 100644
@@ -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;

Signature · 1 line

2.24.0.393.g34dc348eaf-goog

Liran Alon

Reviewed-by on v1 1/1

2019-12-03 22:39 UTC #1 lore raw

Quoted 9 lines · Jim Mattson

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 <[email protected]>

Weird that this wasn’t always like this :)

Reviewed-by Liran Alon <[email protected]>

-Liran

Quoted 87 lines

---
arch/x86/kvm/vmx/vmx.c | 14 ++------------
arch/x86/kvm/vmx/vmx.h | 8 +++++++-

2 files changed, 9 insertions(+), 13 deletions(-)

arch/x86/kvm/vmx/vmx.c +2 ~~-10~~

index 1b9ab4166397d..0b3c7524456f1 100644
@@ -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);

arch/x86/kvm/vmx/vmx.c +0 -2

-free_msrs:
-	kfree(vmx->guest_msrs);

free_pml:

	vmx_destroy_pml_buffer(vmx);

uninit_vcpu:

arch/x86/kvm/vmx/vmx.h +7 -1

index 7c1b978b2df44..08bc24fa59909 100644
@@ -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;

Signature · 1 line

2.24.0.393.g34dc348eaf-goog

Sean Christopherson

2019-12-03 22:59 UTC #2 lore raw

In reply to Jim Mattson

Quoted 15 lines · Jim Mattson

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 <[email protected]>
---
 arch/x86/kvm/vmx/vmx.c | 14 ++------------
 arch/x86/kvm/vmx/vmx.h |  8 +++++++-
 2 files changed, 9 insertions(+), 13 deletions(-)
arch/x86/kvm/vmx/vmx.c +0 -0
index 1b9ab4166397d..0b3c7524456f1 100644

arch/x86/kvm/vmx/vmx.c static unsigned long host_idt_base; · line 446

Quoted 5 lines

arch/x86/kvm/vmx/vmx.c +1 -1

@@ -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.

Quoted 48 lines

arch/x86/kvm/vmx/vmx.c +1 ~~-11~~

 #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:

arch/x86/kvm/vmx/vmx.h +0 -0

index 7c1b978b2df44..08bc24fa59909 100644

arch/x86/kvm/vmx/vmx.h extern u32 get_umwait_control_msr(void); · line 29

Quoted 8 lines

arch/x86/kvm/vmx/vmx.h +5 -0

@@ -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?

Quoted 16 lines

arch/x86/kvm/vmx/vmx.h +2 -1

+
 #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;

Signature · 1 line

2.24.0.393.g34dc348eaf-goog

Jim Mattson

author

2019-12-03 23:24 UTC #3 lore raw

Quoted 25 lines · Sean Christopherson

Quoted 15 lines · Jim Mattson
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 <[email protected]>
---
 arch/x86/kvm/vmx/vmx.c | 14 ++------------
 arch/x86/kvm/vmx/vmx.h |  8 +++++++-
 2 files changed, 9 insertions(+), 13 deletions(-)
arch/x86/kvm/vmx/vmx.c +0 -0
index 1b9ab4166397d..0b3c7524456f1 100644
arch/x86/kvm/vmx/vmx.c static unsigned long host_idt_base; · line 446
Quoted 5 lines
arch/x86/kvm/vmx/vmx.c +1 -1
@@ -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

Quoted 6 lines

        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)?

Quoted 76 lines

Quoted 48 lines

arch/x86/kvm/vmx/vmx.c +1 ~~-11~~

 #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:

arch/x86/kvm/vmx/vmx.h +0 -0

index 7c1b978b2df44..08bc24fa59909 100644

arch/x86/kvm/vmx/vmx.h extern u32 get_umwait_control_msr(void); · line 29

Quoted 8 lines

arch/x86/kvm/vmx/vmx.h +5 -0

@@ -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?

Quoted 16 lines

arch/x86/kvm/vmx/vmx.h +2 -2

+
 #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