Skip to contentSkip to conversation

KVM: nSVM: ondemand nested state allocation

PATCH Applied Maxim Levitsky 2 participants 6 messages Oct 1, 2020 – Oct 19, 2020

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Maxim Levitsky

author
#0 lore raw

v7 cover KVM: nSVM: ondemand nested state allocation

This is the next version of this patch series.

In V5 I adopted Sean Christopherson's suggestion to make .set_efer return a negative error (-ENOMEM in this case) which in most cases in kvm propagates to the userspace.

I noticed though that wrmsr emulation code doesn't do this and instead it injects #GP to the guest on _any_ error.

So I fixed the wrmsr code to behave in a similar way to the rest of the kvm code.
(#GP only on a positive error value, and forward the negative error to the userspace)

I had to adjust one wrmsr handler (xen_hvm_config) to stop it from returning
negative values so that new WRMSR emulation behavior doesn't break it.
This patch was only compile tested.

The memory allocation failure was tested by always returning -ENOMEM from svm_allocate_nested.

The nested allocation itself was tested by countless attempts to run nested guests, do nested migration on both my AMD and Intel machines. I wasn't able to break it.

Changes from V5: addressed Sean Christopherson's review feedback. Changes from V6: rebased the code on latest kvm/queue

Best regards,
Maxim Levitsky

Maxim Levitsky (4):
KVM: x86: xen_hvm_config: cleanup return values
KVM: x86: report negative values from wrmsr emulation to userspace
KVM: x86: allow kvm_x86_ops.set_efer to return an error value
KVM: nSVM: implement on demand allocation of the nested state

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/emulate.c          |  4 +--
 arch/x86/kvm/svm/nested.c       | 42 ++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c          | 64 ++++++++++++++++++---------------
 arch/x86/kvm/svm/svm.h          | 10 +++++-
 arch/x86/kvm/vmx/vmx.c          |  6 ++--
 arch/x86/kvm/x86.c              | 39 ++++++++++----------
 7 files changed, 114 insertions(+), 53 deletions(-)
Signature · 1 line
2.26.2
author
#1 lore raw

PATCH v7 1/4 KVM: x86: xen_hvm_config: cleanup return values

Return 1 on errors that are caused by wrong guest behavior (which will inject #GP to the guest)

And return a negative error value on issues that are the kernel's fault (e.g -ENOMEM)

Signed-off-by Maxim Levitsky <[email protected]>
---
 arch/x86/kvm/x86.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)
arch/x86/kvm/x86.c +9 -14
index c4015a43cc8a7..09a0cad49af51 100644
@@ -2812,24 +2812,19 @@ static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
 	u32 page_num = data & ~PAGE_MASK;
 	u64 page_addr = data & PAGE_MASK;
 	u8 *page;
-	int r;
 
-	r = -E2BIG;
 	if (page_num >= blob_size)
-		goto out;
-	r = -ENOMEM;
+		return 1;
+
 	page = memdup_user(blob_addr + (page_num * PAGE_SIZE), PAGE_SIZE);
-	if (IS_ERR(page)) {
-		r = PTR_ERR(page);
-		goto out;
+	if (IS_ERR(page))
+		return PTR_ERR(page);
+
+	if (kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE)) {
+		kfree(page);
+		return 1;
 	}
-	if (kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE))
-		goto out_free;
-	r = 0;
-out_free:
-	kfree(page);
-out:
-	return r;
+	return 0;
 }
 
 static inline bool kvm_pv_async_pf_enabled(struct kvm_vcpu *vcpu)
Signature · 1 line
2.26.2
author
#2 lore raw

PATCH v7 2/4 KVM: x86: report negative values from wrmsr emulation to userspace

In reply to Maxim Levitsky

This will allow the KVM to report such errors (e.g -ENOMEM) to the userspace.

Signed-off-by Maxim Levitsky <[email protected]>
---
 arch/x86/kvm/emulate.c | 4 ++--
 arch/x86/kvm/x86.c     | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)
arch/x86/kvm/emulate.c +2 -2
index 0cc0db500f718..0d917eb703194 100644
@@ -3712,10 +3712,10 @@ static int em_wrmsr(struct x86_emulate_ctxt *ctxt)
 	if (r == X86EMUL_IO_NEEDED)
 		return r;
 
-	if (r)
+	if (r > 0)
 		return emulate_gp(ctxt, 0);
 
-	return X86EMUL_CONTINUE;
+	return r < 0 ? X86EMUL_UNHANDLEABLE : X86EMUL_CONTINUE;
 }
 
 static int em_rdmsr(struct x86_emulate_ctxt *ctxt)
arch/x86/kvm/x86.c +6 -3
index 09a0cad49af51..7af04f9e20b48 100644
@@ -1737,13 +1737,16 @@ int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
 	r = kvm_set_msr(vcpu, ecx, data);
 
 	/* MSR write failed? See if we should ask user space */
-	if (r && kvm_set_msr_user_space(vcpu, ecx, data, r)) {
+	if (r && kvm_set_msr_user_space(vcpu, ecx, data, r))
 		/* Bounce to user space */
 		return 0;
-	}
+
+	/* Signal all other negative errors to userspace */
+	if (r < 0)
+		return r;
 
 	/* MSR write failed? Inject a #GP */
-	if (r) {
+	if (r > 0) {
 		trace_kvm_msr_write_ex(ecx, data);
 		kvm_inject_gp(vcpu, 0);
 		return 1;
Signature · 1 line
2.26.2
author
#3 lore raw

PATCH v7 3/4 KVM: x86: allow kvm_x86_ops.set_efer to return an error value

In reply to Maxim Levitsky

This will be used to signal an error to the userspace, in case the vendor code failed during handling of this msr. (e.g -ENOMEM)

Signed-off-by Maxim Levitsky <[email protected]>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/svm/svm.c          | 3 ++-
 arch/x86/kvm/svm/svm.h          | 2 +-
 arch/x86/kvm/vmx/vmx.c          | 6 ++++--
 arch/x86/kvm/x86.c              | 7 ++++++-
 5 files changed, 14 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -1
index d0f77235da923..99657051ee8a0 100644
@@ -1086,7 +1086,7 @@ struct kvm_x86_ops {
 	void (*get_cs_db_l_bits)(struct kvm_vcpu *vcpu, int *db, int *l);
 	void (*set_cr0)(struct kvm_vcpu *vcpu, unsigned long cr0);
 	int (*set_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4);
-	void (*set_efer)(struct kvm_vcpu *vcpu, u64 efer);
+	int (*set_efer)(struct kvm_vcpu *vcpu, u64 efer);
 	void (*get_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
arch/x86/kvm/svm/svm.c +2 -1
index 4f401fc6a05d9..57e0f27ff7d20 100644
@@ -263,7 +263,7 @@ static int get_max_npt_level(void)
 #endif
 }
 
-void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
+int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	vcpu->arch.efer = efer;

@@ -283,6 +283,7 @@ void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	svm->vmcb->save.efer = efer | EFER_SVME;
 	vmcb_mark_dirty(svm->vmcb, VMCB_CR);
+	return 0;
 }
 
 static int is_external_interrupt(u32 info)
arch/x86/kvm/svm/svm.h +1 -1
index a7f997459b870..e7af21e6fe1e0 100644
@@ -350,7 +350,7 @@ static inline bool gif_set(struct vcpu_svm *svm)
 #define MSR_INVALID				0xffffffffU
 
 u32 svm_msrpm_offset(u32 msr);
-void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer);
+int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer);
 void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
 int svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
 void svm_flush_tlb(struct kvm_vcpu *vcpu);
arch/x86/kvm/vmx/vmx.c +4 -2
index 4551a7e80ebc3..2115d35094bc5 100644
@@ -2815,13 +2815,14 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 	kvm_mmu_reset_context(vcpu);
 }
 
-void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
+int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmx_uret_msr *msr = vmx_find_uret_msr(vmx, MSR_EFER);
 
+	/* Nothing to do if hardware doesn't support EFER. */
 	if (!msr)
-		return;
+		return 0;
 
 	vcpu->arch.efer = efer;
 	if (efer & EFER_LMA) {

@@ -2833,6 +2834,7 @@ void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 		msr->data = efer & ~EFER_LME;
 	}
 	setup_msrs(vmx);
+	return 0;
 }
 
 #ifdef CONFIG_X86_64
arch/x86/kvm/x86.c +6 -1
index 7af04f9e20b48..80e3dcd542382 100644
@@ -1457,6 +1457,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	u64 old_efer = vcpu->arch.efer;
 	u64 efer = msr_info->data;
+	int r;
 
 	if (efer & efer_reserved_bits)
 		return 1;

@@ -1473,7 +1474,11 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
-	kvm_x86_ops.set_efer(vcpu, efer);
+	r = kvm_x86_ops.set_efer(vcpu, efer);
+	if (r) {
+		WARN_ON(r > 0);
+		return r;
+	}
 
 	/* Update reserved bits */
 	if ((efer ^ old_efer) & EFER_NX)
Signature · 1 line
2.26.2
author
#4 lore raw

PATCH v7 4/4 KVM: nSVM: implement on demand allocation of the nested state

In reply to Maxim Levitsky

This way we don't waste memory on VMs which don't use nesting virtualization even when the host enabled it for them.

Signed-off-by Maxim Levitsky <[email protected]>
---
 arch/x86/kvm/svm/nested.c | 42 +++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    | 61 +++++++++++++++++++++------------------
 arch/x86/kvm/svm/svm.h    |  8 +++++
 3 files changed, 83 insertions(+), 28 deletions(-)
arch/x86/kvm/svm/nested.c +42 -0
index ba50ff6e35c7c..9e4c226dbf7d9 100644
@@ -481,6 +481,9 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 
 	vmcb12 = map.hva;
 
+	if (WARN_ON_ONCE(!svm->nested.initialized))
+		return -EINVAL;
+
 	if (!nested_vmcb_checks(svm, vmcb12)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;

@@ -698,6 +701,45 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	return 0;
 }
 
+int svm_allocate_nested(struct vcpu_svm *svm)
+{
+	struct page *hsave_page;
+
+	if (svm->nested.initialized)
+		return 0;
+
+	hsave_page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
+	if (!hsave_page)
+		return -ENOMEM;
+	svm->nested.hsave = page_address(hsave_page);
+
+	svm->nested.msrpm = svm_vcpu_alloc_msrpm();
+	if (!svm->nested.msrpm)
+		goto err_free_hsave;
+	svm_vcpu_init_msrpm(&svm->vcpu, svm->nested.msrpm);
+
+	svm->nested.initialized = true;
+	return 0;
+
+err_free_hsave:
+	__free_page(hsave_page);
+	return -ENOMEM;
+}
+
+void svm_free_nested(struct vcpu_svm *svm)
+{
+	if (!svm->nested.initialized)
+		return;
+
+	svm_vcpu_free_msrpm(svm->nested.msrpm);
+	svm->nested.msrpm = NULL;
+
+	__free_page(virt_to_page(svm->nested.hsave));
+	svm->nested.hsave = NULL;
+
+	svm->nested.initialized = false;
+}
+
 /*
  * Forcibly leave nested mode in order to be able to reset the VCPU later on.
  */
arch/x86/kvm/svm/svm.c +33 -28

136 lines not on this page · raw message

arch/x86/kvm/svm/svm.h +8 -0
index e7af21e6fe1e0..1d853fe4c778b 100644
@@ -97,6 +97,8 @@ struct svm_nested_state {
 
 	/* cache for control fields of the guest */
 	struct vmcb_control_area ctl;
+
+	bool initialized;
 };
 
 struct vcpu_svm {

@@ -350,6 +352,10 @@ static inline bool gif_set(struct vcpu_svm *svm)
 #define MSR_INVALID				0xffffffffU
 
 u32 svm_msrpm_offset(u32 msr);
+u32 *svm_vcpu_alloc_msrpm(void);
+void svm_vcpu_init_msrpm(struct kvm_vcpu *vcpu, u32 *msrpm);
+void svm_vcpu_free_msrpm(u32 *msrpm);
+
 int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer);
 void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
 int svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);

@@ -391,6 +397,8 @@ static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
 int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
 			 struct vmcb *nested_vmcb);
 void svm_leave_nested(struct vcpu_svm *svm);
+void svm_free_nested(struct vcpu_svm *svm);
+int svm_allocate_nested(struct vcpu_svm *svm);
 int nested_svm_vmrun(struct vcpu_svm *svm);
 void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb);
 int nested_svm_vmexit(struct vcpu_svm *svm);
Signature · 1 line
2.26.2
maintainer Applied
#5 lore raw
In reply to Maxim Levitsky
Quoted 37 lines · Maxim Levitsky

This is the next version of this patch series.

In V5 I adopted Sean Christopherson's suggestion to make .set_efer return a negative error (-ENOMEM in this case) which in most cases in kvm propagates to the userspace.

I noticed though that wrmsr emulation code doesn't do this and instead it injects #GP to the guest on _any_ error.

So I fixed the wrmsr code to behave in a similar way to the rest of the kvm code.
(#GP only on a positive error value, and forward the negative error to the userspace)

I had to adjust one wrmsr handler (xen_hvm_config) to stop it from returning
negative values so that new WRMSR emulation behavior doesn't break it.
This patch was only compile tested.

The memory allocation failure was tested by always returning -ENOMEM from svm_allocate_nested.

The nested allocation itself was tested by countless attempts to run nested guests, do nested migration on both my AMD and Intel machines. I wasn't able to break it.

Changes from V5: addressed Sean Christopherson's review feedback. Changes from V6: rebased the code on latest kvm/queue

Best regards,
Maxim Levitsky

Maxim Levitsky (4):
KVM: x86: xen_hvm_config: cleanup return values
KVM: x86: report negative values from wrmsr emulation to userspace
KVM: x86: allow kvm_x86_ops.set_efer to return an error value
KVM: nSVM: implement on demand allocation of the nested state

Quoted 9 lines
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/emulate.c          |  4 +--
 arch/x86/kvm/svm/nested.c       | 42 ++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c          | 64 ++++++++++++++++++---------------
 arch/x86/kvm/svm/svm.h          | 10 +++++-
 arch/x86/kvm/vmx/vmx.c          |  6 ++--
 arch/x86/kvm/x86.c              | 39 ++++++++++----------
 7 files changed, 114 insertions(+), 53 deletions(-)

Queued, thanks.

Paolo