KVM: nSVM: ondemand nested state allocation + nested guest state caching

PATCH Superseded Maxim Levitsky 3 participants 24 messages Aug 20, 2020 – Aug 25, 2020

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Maxim Levitsky

author

2020-08-20 09:13 UTC #0 lore raw

cover KVM: nSVM: ondemand nested state allocation + nested guest state caching

Hi!

This patch series implements caching of the whole nested guest vmcb as opposed to current code that only caches its control area. This allows us to avoid race in which guest changes the data area while we are verifying it.

In adddition to that I also implemented on demand nested state area to compensate a bit for memory usage increase from this caching. This way at least guests that don't use nesting won't waste memory on nested state.

Patches 1,2,3 are just refactoring,

Patches 4,5 are for ondemand nested state, while patches 6,7,8 are for caching of the nested state.

Patch 8 is more of an optimization and can be dropped if you like to.

The series was tested with various nested guests, in one case even with L3 running, but note that due to unrelated issue, migration with nested guest running didn't work for me with or without this series. I am investigating this currently.

Best regards,
Maxim Levitsky

Maxim Levitsky (8):
KVM: SVM: rename a variable in the svm_create_vcpu
KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places
KVM: SVM: refactor msr permission bitmap allocation
KVM: x86: allow kvm_x86_ops.set_efer to return a value
KVM: nSVM: implement ondemand allocation of the nested state
SVM: nSVM: cache whole nested vmcb instead of only its control area
KVM: nSVM: implement caching of nested vmcb save area
KVM: nSVM: read only changed fields of the nested guest data area

 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/svm/nested.c       | 296 +++++++++++++++++++++++---------
 arch/x86/kvm/svm/svm.c          | 129 +++++++-------
 arch/x86/kvm/svm/svm.h          |  32 ++--
 arch/x86/kvm/vmx/vmx.c          |   5 +-
 arch/x86/kvm/x86.c              |   3 +-
 6 files changed, 312 insertions(+), 155 deletions(-)

Signature · 1 line

2.26.2

Maxim Levitsky

author

2020-08-20 09:13 UTC #1 lore raw

PATCH 1/8 KVM: SVM: rename a variable in the svm_create_vcpu

The 'page' is to hold the vcpu's vmcb so name it as such to avoid confusion.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/svm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/kvm/svm/svm.c +6 -6

index 03dd7bac8034..562a79e3e63a 100644
@@ -1171,7 +1171,7 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm;
-	struct page *page;
+	struct page *vmcb_page;
 	struct page *msrpm_pages;
 	struct page *hsave_page;
 	struct page *nested_msrpm_pages;

@@ -1181,8 +1181,8 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	svm = to_svm(vcpu);
 
 	err = -ENOMEM;
-	page = alloc_page(GFP_KERNEL_ACCOUNT);
-	if (!page)
+	vmcb_page = alloc_page(GFP_KERNEL_ACCOUNT);
+	if (!vmcb_page)
 		goto out;
 
 	msrpm_pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);

@@ -1216,9 +1216,9 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	svm->nested.msrpm = page_address(nested_msrpm_pages);
 	svm_vcpu_init_msrpm(svm->nested.msrpm);
 
-	svm->vmcb = page_address(page);
+	svm->vmcb = page_address(vmcb_page);
 	clear_page(svm->vmcb);
-	svm->vmcb_pa = __sme_set(page_to_pfn(page) << PAGE_SHIFT);
+	svm->vmcb_pa = __sme_set(page_to_pfn(vmcb_page) << PAGE_SHIFT);
 	svm->asid_generation = 0;
 	init_vmcb(svm);

@@ -1234,7 +1234,7 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 free_page2:
 	__free_pages(msrpm_pages, MSRPM_ALLOC_ORDER);
 free_page1:
-	__free_page(page);
+	__free_page(vmcb_page);
 out:
 	return err;
 }

Signature · 1 line

2.26.2

Maxim Levitsky

author

2020-08-20 09:16 UTC #2 lore raw

PATCH 2/8 KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

In reply to Maxim Levitsky

No functional changes.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 10 +++++-----
 arch/x86/kvm/svm/svm.c    | 13 +++++++------
 arch/x86/kvm/svm/svm.h    |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

arch/x86/kvm/svm/nested.c +5 -5

index fb68467e6049..d9755eab2199 100644
@@ -431,7 +431,7 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
 {
 	int ret;
 
-	svm->nested.vmcb = vmcb_gpa;
+	svm->nested.vmcb_gpa = vmcb_gpa;
 	load_nested_vmcb_control(svm, &nested_vmcb->control);
 	nested_prepare_vmcb_save(svm, nested_vmcb);
 	nested_prepare_vmcb_control(svm);

@@ -568,7 +568,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	struct vmcb *vmcb = svm->vmcb;
 	struct kvm_host_map map;
 
-	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb), &map);
+	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb_gpa), &map);
 	if (rc) {
 		if (rc == -EINVAL)
 			kvm_inject_gp(&svm->vcpu, 0);

@@ -579,7 +579,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 
 	/* Exit Guest-Mode */
 	leave_guest_mode(&svm->vcpu);
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	WARN_ON_ONCE(svm->nested.nested_run_pending);
 
 	/* in case we halted in L2 */

@@ -1018,7 +1018,7 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 
 	/* First fill in the header and copy it out.  */
 	if (is_guest_mode(vcpu)) {
-		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb;
+		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb_gpa;
 		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
 		kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;

@@ -1128,7 +1128,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
 	hsave->save = save;
 
-	svm->nested.vmcb = kvm_state->hdr.svm.vmcb_pa;
+	svm->nested.vmcb_gpa = kvm_state->hdr.svm.vmcb_pa;
 	load_nested_vmcb_control(svm, &ctl);
 	nested_prepare_vmcb_control(svm);

arch/x86/kvm/svm/svm.c +7 -6

index 562a79e3e63a..4338d2a2596e 100644
@@ -1102,7 +1102,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	}
 	svm->asid_generation = 0;
 
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	svm->vcpu.arch.hflags = 0;
 
 	if (!kvm_pause_in_guest(svm->vcpu.kvm)) {

@@ -3884,7 +3884,7 @@ static int svm_pre_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
 		/* FED8h - SVM Guest */
 		put_smstate(u64, smstate, 0x7ed8, 1);
 		/* FEE0h - SVM Guest VMCB Physical Address */
-		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb);
+		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb_gpa);
 
 		svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
 		svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];

@@ -3903,17 +3903,18 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 	struct vmcb *nested_vmcb;
 	struct kvm_host_map map;
 	u64 guest;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	int ret = 0;
 
 	guest = GET_SMSTATE(u64, smstate, 0x7ed8);
-	vmcb = GET_SMSTATE(u64, smstate, 0x7ee0);
+	vmcb_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
 
 	if (guest) {
-		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL)
+		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
+
 		nested_vmcb = map.hva;
-		ret = enter_svm_guest_mode(svm, vmcb, nested_vmcb);
+		ret = enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb);
 		kvm_vcpu_unmap(&svm->vcpu, &map, true);
 	}

arch/x86/kvm/svm/svm.h +1 -1

index a798e1731709..03f2f082ef10 100644
@@ -85,7 +85,7 @@ struct svm_nested_state {
 	struct vmcb *hsave;
 	u64 hsave_msr;
 	u64 vm_cr_msr;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	u32 host_intercept_exceptions;
 
 	/* These are the merged vectors */

Signature · 1 line

2.26.2

Paolo Bonzini

maintainer

2020-08-20 12:16 UTC #3 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 109 lines · Maxim Levitsky

No functional changes.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 10 +++++-----
 arch/x86/kvm/svm/svm.c    | 13 +++++++------
 arch/x86/kvm/svm/svm.h    |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

arch/x86/kvm/svm/nested.c +5 -5

index fb68467e6049..d9755eab2199 100644
@@ -431,7 +431,7 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
 {
 	int ret;
 
-	svm->nested.vmcb = vmcb_gpa;
+	svm->nested.vmcb_gpa = vmcb_gpa;
 	load_nested_vmcb_control(svm, &nested_vmcb->control);
 	nested_prepare_vmcb_save(svm, nested_vmcb);
 	nested_prepare_vmcb_control(svm);

@@ -568,7 +568,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	struct vmcb *vmcb = svm->vmcb;
 	struct kvm_host_map map;
 
-	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb), &map);
+	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb_gpa), &map);
 	if (rc) {
 		if (rc == -EINVAL)
 			kvm_inject_gp(&svm->vcpu, 0);

@@ -579,7 +579,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 
 	/* Exit Guest-Mode */
 	leave_guest_mode(&svm->vcpu);
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	WARN_ON_ONCE(svm->nested.nested_run_pending);
 
 	/* in case we halted in L2 */

@@ -1018,7 +1018,7 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 
 	/* First fill in the header and copy it out.  */
 	if (is_guest_mode(vcpu)) {
-		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb;
+		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb_gpa;
 		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
 		kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;

@@ -1128,7 +1128,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
 	hsave->save = save;
 
-	svm->nested.vmcb = kvm_state->hdr.svm.vmcb_pa;
+	svm->nested.vmcb_gpa = kvm_state->hdr.svm.vmcb_pa;
 	load_nested_vmcb_control(svm, &ctl);
 	nested_prepare_vmcb_control(svm);

arch/x86/kvm/svm/svm.c +7 -6

index 562a79e3e63a..4338d2a2596e 100644
@@ -1102,7 +1102,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	}
 	svm->asid_generation = 0;
 
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	svm->vcpu.arch.hflags = 0;
 
 	if (!kvm_pause_in_guest(svm->vcpu.kvm)) {

@@ -3884,7 +3884,7 @@ static int svm_pre_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
 		/* FED8h - SVM Guest */
 		put_smstate(u64, smstate, 0x7ed8, 1);
 		/* FEE0h - SVM Guest VMCB Physical Address */
-		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb);
+		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb_gpa);
 
 		svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
 		svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];

@@ -3903,17 +3903,18 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 	struct vmcb *nested_vmcb;
 	struct kvm_host_map map;
 	u64 guest;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	int ret = 0;
 
 	guest = GET_SMSTATE(u64, smstate, 0x7ed8);
-	vmcb = GET_SMSTATE(u64, smstate, 0x7ee0);
+	vmcb_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
 
 	if (guest) {
-		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL)
+		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
+
 		nested_vmcb = map.hva;
-		ret = enter_svm_guest_mode(svm, vmcb, nested_vmcb);
+		ret = enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb);
 		kvm_vcpu_unmap(&svm->vcpu, &map, true);
 	}

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

index a798e1731709..03f2f082ef10 100644

arch/x86/kvm/svm/svm.h struct svm_nested_state { · line 91

Quoted 8 lines

arch/x86/kvm/svm/svm.h +1 -1

@@ -85,7 +85,7 @@ struct svm_nested_state {
 	struct vmcb *hsave;
 	u64 hsave_msr;
 	u64 vm_cr_msr;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	u32 host_intercept_exceptions;
 
 	/* These are the merged vectors */

Please use vmcb12_gpa, and svm->nested.vmcb12 for the VMCB in patch 6.

(You probably also what to have local variables named vmcb12 in patch 6 to avoid too-long lines).

Paolo

Maxim Levitsky

author

2020-08-20 11:56 UTC #4 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 116 lines · Paolo Bonzini

Quoted 115 lines · Maxim Levitsky

No functional changes.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 10 +++++-----
 arch/x86/kvm/svm/svm.c    | 13 +++++++------
 arch/x86/kvm/svm/svm.h    |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

arch/x86/kvm/svm/nested.c +5 -5

index fb68467e6049..d9755eab2199 100644
@@ -431,7 +431,7 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
 {
 	int ret;
 
-	svm->nested.vmcb = vmcb_gpa;
+	svm->nested.vmcb_gpa = vmcb_gpa;
 	load_nested_vmcb_control(svm, &nested_vmcb->control);
 	nested_prepare_vmcb_save(svm, nested_vmcb);
 	nested_prepare_vmcb_control(svm);

@@ -568,7 +568,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	struct vmcb *vmcb = svm->vmcb;
 	struct kvm_host_map map;
 
-	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb), &map);
+	rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(svm->nested.vmcb_gpa), &map);
 	if (rc) {
 		if (rc == -EINVAL)
 			kvm_inject_gp(&svm->vcpu, 0);

@@ -579,7 +579,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 
 	/* Exit Guest-Mode */
 	leave_guest_mode(&svm->vcpu);
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	WARN_ON_ONCE(svm->nested.nested_run_pending);
 
 	/* in case we halted in L2 */

@@ -1018,7 +1018,7 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 
 	/* First fill in the header and copy it out.  */
 	if (is_guest_mode(vcpu)) {
-		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb;
+		kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb_gpa;
 		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
 		kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;

@@ -1128,7 +1128,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
 	hsave->save = save;
 
-	svm->nested.vmcb = kvm_state->hdr.svm.vmcb_pa;
+	svm->nested.vmcb_gpa = kvm_state->hdr.svm.vmcb_pa;
 	load_nested_vmcb_control(svm, &ctl);
 	nested_prepare_vmcb_control(svm);

arch/x86/kvm/svm/svm.c +7 -6

index 562a79e3e63a..4338d2a2596e 100644
@@ -1102,7 +1102,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	}
 	svm->asid_generation = 0;
 
-	svm->nested.vmcb = 0;
+	svm->nested.vmcb_gpa = 0;
 	svm->vcpu.arch.hflags = 0;
 
 	if (!kvm_pause_in_guest(svm->vcpu.kvm)) {

@@ -3884,7 +3884,7 @@ static int svm_pre_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
 		/* FED8h - SVM Guest */
 		put_smstate(u64, smstate, 0x7ed8, 1);
 		/* FEE0h - SVM Guest VMCB Physical Address */
-		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb);
+		put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb_gpa);
 
 		svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
 		svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];

@@ -3903,17 +3903,18 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 	struct vmcb *nested_vmcb;
 	struct kvm_host_map map;
 	u64 guest;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	int ret = 0;
 
 	guest = GET_SMSTATE(u64, smstate, 0x7ed8);
-	vmcb = GET_SMSTATE(u64, smstate, 0x7ee0);
+	vmcb_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
 
 	if (guest) {
-		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL)
+		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
+
 		nested_vmcb = map.hva;
-		ret = enter_svm_guest_mode(svm, vmcb, nested_vmcb);
+		ret = enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb);
 		kvm_vcpu_unmap(&svm->vcpu, &map, true);
 	}

arch/x86/kvm/svm/svm.h +1 -1

index a798e1731709..03f2f082ef10 100644
@@ -85,7 +85,7 @@ struct svm_nested_state {
 	struct vmcb *hsave;
 	u64 hsave_msr;
 	u64 vm_cr_msr;
-	u64 vmcb;
+	u64 vmcb_gpa;
 	u32 host_intercept_exceptions;

Quoted 7 lines

arch/x86/kvm/svm/svm.h struct svm_nested_state { · line 91
Quoted 2 lines
arch/x86/kvm/svm/svm.h +0 -0
 	/* These are the merged vectors */
Please use vmcb12_gpa, and svm->nested.vmcb12 for the VMCB in patch 6.
(You probably also what to have local variables named vmcb12 in patch 6 to avoid too-long lines).

The limit was raised to 100 chars recently, thats why I allowed some lines to go over 80 characters to avoid adding too much noise.

Quoted 2 lines

Paolo

I was thinking to to this, but since this field already sits in ->nested I was thinking that this is a bit redundant, but I don't have anything against doing it.

Best regards,
Maxim Levitsky

Quoted 1 line

Paolo Bonzini

maintainer

2020-08-20 10:34 UTC #5 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 7 lines · Maxim Levitsky

Quoted 4 lines
Please use vmcb12_gpa, and svm->nested.vmcb12 for the VMCB in patch 6.
(You probably also what to have local variables named vmcb12 in patch 6 to avoid too-long lines).
The limit was raised to 100 chars recently, thats why I allowed some lines to go over 80 characters to avoid adding too much noise.

True, but having svm->nested.vmcb12->control repeated all over isn't pretty. :)

Since you're going to touch all lines anyway, adding the local variable is a good idea.

Paolo

Maxim Levitsky

author

2020-08-20 10:24 UTC #6 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 11 lines · Paolo Bonzini

Quoted 7 lines · Maxim Levitsky
Quoted 4 lines
Please use vmcb12_gpa, and svm->nested.vmcb12 for the VMCB in patch 6.
(You probably also what to have local variables named vmcb12 in patch 6 to avoid too-long lines).
The limit was raised to 100 chars recently, thats why I allowed some lines to go over 80 characters to avoid adding too much noise.
True, but having svm->nested.vmcb12->control repeated all over isn't pretty. :)

I fully agree that adding local variable is a good idea anyway.

I was just noting that svm->nested.vmcb is already about the nested (e.g vmcb12) thus I was thinking that naming this field vmcb12 would be redundant and not accepted this way.

Best regards,
Maxim Levitsky

Quoted 6 lines

Since you're going to touch all lines anyway, adding the local variable is a good idea.
Paolo

Paolo Bonzini

maintainer

2020-08-20 10:56 UTC #7 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 5 lines · Maxim Levitsky

I fully agree that adding local variable is a good idea anyway.
I was just noting that svm->nested.vmcb is already about the nested (e.g vmcb12) thus I was thinking that naming this field vmcb12 would be redundant and not accepted this way.

We want to have both svm->nested.vmcb12 and svm->nested.vmcb02 in there, and hsave is also a VMCB of sort (somewhat like a vmcb01 that is only used while running a nested guest). So it is clearer to write _which_ vmcb it is, and it also helps by making terminology consistent between VMX and SVM.

Paolo

Maxim Levitsky

author

2020-08-20 11:53 UTC #8 lore raw

Re: [PATCH 2/8] KVM: nSVM: rename nested 'vmcb' to vmcb_gpa in few places

Quoted 7 lines · Paolo Bonzini

Quoted 5 lines · Maxim Levitsky
I fully agree that adding local variable is a good idea anyway.
I was just noting that svm->nested.vmcb is already about the nested (e.g vmcb12) thus I was thinking that naming this field vmcb12 would be redundant and not accepted this way.

Quoted 5 lines

We want to have both svm->nested.vmcb12 and svm->nested.vmcb02 in there, and hsave is also a VMCB of sort (somewhat like a vmcb01 that is only used while running a nested guest). So it is clearer to write _which_ vmcb it is, and it also helps by making terminology consistent between VMX and SVM.

This makes sense.
Best regards,
Maxim Levitsky

Quoted 3 lines

Paolo

Maxim Levitsky

author

2020-08-20 09:13 UTC #9 lore raw

PATCH 3/8 KVM: SVM: refactor msr permission bitmap allocation

In reply to Maxim Levitsky

Replace svm_vcpu_init_msrpm with svm_vcpu_alloc_msrpm, that also allocates the msr bitmap and add svm_vcpu_free_msrpm to free it.

This will be used later to move the nested msr permission bitmap allocation to nested.c

No functional change intended.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/svm.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

arch/x86/kvm/svm/svm.c +23 ~~-22~~

index 4338d2a2596e..e072ecace466 100644
@@ -609,18 +609,29 @@ static void set_msr_interception(u32 *msrpm, unsigned msr,
 	msrpm[offset] = tmp;
 }
 
-static void svm_vcpu_init_msrpm(u32 *msrpm)
+static u32 *svm_vcpu_alloc_msrpm(void)
 {
 	int i;
+	u32 *msrpm;
+	struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
+
+	if (!pages)
+		return NULL;
 
+	msrpm = page_address(pages);
 	memset(msrpm, 0xff, PAGE_SIZE * (1 << MSRPM_ALLOC_ORDER));
 
 	for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++) {
 		if (!direct_access_msrs[i].always)
 			continue;
-
 		set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
 	}
+	return msrpm;
+}
+
+static void svm_vcpu_free_msrpm(u32 *msrpm)
+{
+	__free_pages(virt_to_page(msrpm), MSRPM_ALLOC_ORDER);
 }
 
 static void add_msr_offset(u32 offset)

@@ -1172,9 +1183,7 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm;
 	struct page *vmcb_page;
-	struct page *msrpm_pages;
 	struct page *hsave_page;
-	struct page *nested_msrpm_pages;
 	int err;
 
 	BUILD_BUG_ON(offsetof(struct vcpu_svm, vcpu) != 0);

@@ -1185,21 +1194,13 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	if (!vmcb_page)
 		goto out;
 
-	msrpm_pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
-	if (!msrpm_pages)
-		goto free_page1;
-
-	nested_msrpm_pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
-	if (!nested_msrpm_pages)
-		goto free_page2;
-
 	hsave_page = alloc_page(GFP_KERNEL_ACCOUNT);
 	if (!hsave_page)
-		goto free_page3;
+		goto free_page1;
 
 	err = avic_init_vcpu(svm);
 	if (err)
-		goto free_page4;
+		goto free_page2;
 
 	/* We initialize this flag to true to make sure that the is_running
 	 * bit would be set the first time the vcpu is loaded.

@@ -1210,11 +1211,13 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	svm->nested.hsave = page_address(hsave_page);
 	clear_page(svm->nested.hsave);
 
-	svm->msrpm = page_address(msrpm_pages);
-	svm_vcpu_init_msrpm(svm->msrpm);
+	svm->msrpm = svm_vcpu_alloc_msrpm();
+	if (!svm->msrpm)
+		goto free_page2;
 
-	svm->nested.msrpm = page_address(nested_msrpm_pages);
-	svm_vcpu_init_msrpm(svm->nested.msrpm);
+	svm->nested.msrpm = svm_vcpu_alloc_msrpm();
+	if (!svm->nested.msrpm)
+		goto free_page3;
 
 	svm->vmcb = page_address(vmcb_page);
 	clear_page(svm->vmcb);

@@ -1227,12 +1230,10 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 
 	return 0;
 
-free_page4:
-	__free_page(hsave_page);
 free_page3:
-	__free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER);
+	svm_vcpu_free_msrpm(svm->msrpm);
 free_page2:
-	__free_pages(msrpm_pages, MSRPM_ALLOC_ORDER);
+	__free_page(hsave_page);
 free_page1:
 	__free_page(vmcb_page);
 out:

Signature · 1 line

2.26.2

Maxim Levitsky

author

2020-08-20 09:14 UTC #10 lore raw

PATCH 4/8 KVM: x86: allow kvm_x86_ops.set_efer to return a value

In reply to Maxim Levitsky

This will be used later to return an error when setting this msr fails.

For VMX, it already has an error condition when EFER is not in the shared MSR list, so return an error in this case.

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          | 5 +++--
 arch/x86/kvm/x86.c              | 3 ++-
 5 files changed, 9 insertions(+), 6 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -1

index 5ab3af7275d8..bd0519e26053 100644
@@ -1069,7 +1069,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 e072ecace466..ce0773c9a7fa 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 03f2f082ef10..ef16f708ed1c 100644
@@ -349,7 +349,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 +3 -2

index 46ba2e03a892..e90b9e68c7ea 100644
@@ -2862,13 +2862,13 @@ 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 shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER);
 
 	if (!msr)
-		return;
+		return 1;
 
 	vcpu->arch.efer = efer;
 	if (efer & EFER_LMA) {

@@ -2880,6 +2880,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 +2 -1

index 2db369a64f29..cad5d9778a21 100644
@@ -1471,7 +1471,8 @@ 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);
+	if (kvm_x86_ops.set_efer(vcpu, efer))
+		return 1;
 
 	/* Update reserved bits */
 	if ((efer ^ old_efer) & EFER_NX)

Signature · 1 line

2.26.2

kernel test robot

build bot

2020-08-25 22:49 UTC #11 lore raw

Re: [PATCH 4/8] KVM: x86: allow kvm_x86_ops.set_efer to return a value

I love your patch! Yet something to improve:

Hi Maxim,

I love your patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v5.9-rc2 next-20200825]
[cannot apply to vhost/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maxim-Levitsky/KVM-nSVM-ondemand-nested-state-allocation-nested-guest-state-caching/20200820-171720
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-randconfig-a001-20200826 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 77e5a195f818b9ace91f7b12ab948b21d7918238)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> arch/x86/kvm/vmx/vmx.c:2865:5: error: conflicting types for 'vmx_set_efer'
   int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
       ^
   arch/x86/kvm/vmx/vmx.h:337:6: note: previous declaration is here
   void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer);
        ^
>> arch/x86/kvm/vmx/vmx.c:7913:14: error: incompatible function pointer types initializing 'int (*)(struct kvm_vcpu *, u64)' (aka 'int (*)(struct kvm_vcpu *, unsigned long long)') with an expression of type 'void (struct kvm_vcpu *, u64)' (aka 'void (struct kvm_vcpu *, unsigned long long)') [-Werror,-Wincompatible-function-pointer-types]

.set_efer = vmx_set_efer,
^~~~~~~~~~~~
2 errors generated.

# https://github.com/0day-ci/linux/commit/9196b5409d5cb9f0b5536914ecb139ce456ea889
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Maxim-Levitsky/KVM-nSVM-ondemand-nested-state-allocation-nested-guest-state-caching/20200820-171720 git checkout 9196b5409d5cb9f0b5536914ecb139ce456ea889
vim +/vmx_set_efer +2865 arch/x86/kvm/vmx/vmx.c

2864

> 2865	int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)

2866 {
2867 struct vcpu_vmx *vmx = to_vmx(vcpu);
2868 struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER);
2869
2870 if (!msr)
2871 return 1;
2872
2873 vcpu->arch.efer = efer;
2874 if (efer & EFER_LMA) {
2875 vm_entry_controls_setbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
2876 msr->data = efer;
2877 } else {
2878 vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
2879
2880 msr->data = efer & ~EFER_LME;
2881 }
2882 setup_msrs(vmx);
2883 return 0;
2884 }
2885

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Maxim Levitsky

author

2020-08-20 09:14 UTC #12 lore raw

PATCH 5/8 KVM: nSVM: implement ondemand allocation of the nested state

In reply to Maxim Levitsky

This way we don't waste memory on VMs which don't enable nesting virtualization

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 43 +++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    | 62 +++++++++++++++++++++++----------------
 arch/x86/kvm/svm/svm.h    |  6 ++++
 3 files changed, 85 insertions(+), 26 deletions(-)

arch/x86/kvm/svm/nested.c +43 -0

index d9755eab2199..b6704611fc02 100644
@@ -473,6 +473,9 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 
 	nested_vmcb = map.hva;
 
+	if (WARN_ON(!svm->nested.initialized))
+		return 1;
+
 	if (!nested_vmcb_checks(svm, nested_vmcb)) {
 		nested_vmcb->control.exit_code    = SVM_EXIT_ERR;
 		nested_vmcb->control.exit_code_hi = 0;

@@ -686,6 +689,46 @@ 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);
+	if (!hsave_page)
+		goto free_page1;
+
+	svm->nested.hsave = page_address(hsave_page);
+	clear_page(svm->nested.hsave);
+
+	svm->nested.msrpm = svm_vcpu_alloc_msrpm();
+	if (!svm->nested.msrpm)
+		goto free_page2;
+
+	svm->nested.initialized = true;
+	return 0;
+free_page2:
+	__free_page(hsave_page);
+free_page1:
+	return 1;
+}
+
+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 +36 ~~-26~~

133 lines not on this page · raw message

arch/x86/kvm/svm/svm.h +6 -0

index ef16f708ed1c..9dca64a2edb5 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 {

@@ -349,6 +351,8 @@ 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_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);

@@ -390,6 +394,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

Paolo Bonzini

maintainer

2020-08-20 12:06 UTC #13 lore raw

Re: [PATCH 5/8] KVM: nSVM: implement ondemand allocation of the nested state

Quoted 4 lines · Maxim Levitsky

arch/x86/kvm/svm/svm.c +0 -0

@@ -3912,6 +3914,14 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 	vmcb_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
 
 	if (guest) {

arch/x86/kvm/svm/svm.c static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) · line 3926

Quoted 10 lines

arch/x86/kvm/svm/svm.c +8 -0

+		/*
+		 * This can happen if SVM was not enabled prior to #SMI,
+		 * but guest corrupted the #SMI state and marked it as
+		 * enabled it there
+		 */
+		if (!svm->nested.initialized)
+			return 1;
+
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;

This can also happen if you live migrate while in SMM (EFER.SVME=0). You need to check for the SVME bit in the SMM state save area, and:

1) triple fault if it is clear

2) call svm_allocate_nested if it is set.

Paolo

Maxim Levitsky

author

2020-08-20 11:55 UTC #14 lore raw

Re: [PATCH 5/8] KVM: nSVM: implement ondemand allocation of the nested state

Quoted 19 lines · Paolo Bonzini

Quoted 4 lines · Maxim Levitsky
arch/x86/kvm/svm/svm.c +0 -0
@@ -3912,6 +3914,14 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 	vmcb_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
 
 	if (guest) {
arch/x86/kvm/svm/svm.c static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) · line 3926
Quoted 10 lines
arch/x86/kvm/svm/svm.c +8 -0
+		/*
+		 * This can happen if SVM was not enabled prior to #SMI,
+		 * but guest corrupted the #SMI state and marked it as
+		 * enabled it there
+		 */
+		if (!svm->nested.initialized)
+			return 1;
+
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
This can also happen if you live migrate while in SMM (EFER.SVME=0). You need to check for the SVME bit in the SMM state save area, and:

Quoted 6 lines

1) triple fault if it is clear
2) call svm_allocate_nested if it is set.
Paolo

Makes sense, will do.

Best regards,
Maxim Levitsky

Maxim Levitsky

author

2020-08-20 09:14 UTC #15 lore raw

PATCH 6/8 SVM: nSVM: cache whole nested vmcb instead of only its control area

In reply to Maxim Levitsky

Until now we were only caching the 'control' are of the vmcb, but we will want soon to have some checks on the data area as well and this caching will allow us to fix various races that can happen if a (malicious) guest changes parts of the 'save' area during vm entry.

No functional change intended other that slightly higher memory usage, since this patch doesn't touch the data area of the cached vmcb.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 96 +++++++++++++++++++++++----------------
 arch/x86/kvm/svm/svm.c    | 10 ++--
 arch/x86/kvm/svm/svm.h    | 15 +++---
 3 files changed, 69 insertions(+), 52 deletions(-)

arch/x86/kvm/svm/nested.c +56 ~~-40~~

index b6704611fc02..c9bb17e9ba11 100644
@@ -54,7 +54,7 @@ static void nested_svm_inject_npf_exit(struct kvm_vcpu *vcpu,
 static u64 nested_svm_get_tdp_pdptr(struct kvm_vcpu *vcpu, int index)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
-	u64 cr3 = svm->nested.ctl.nested_cr3;
+	u64 cr3 = svm->nested.vmcb->control.nested_cr3;
 	u64 pdpte;
 	int ret;

@@ -69,7 +69,7 @@ static unsigned long nested_svm_get_tdp_cr3(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	return svm->nested.ctl.nested_cr3;
+	return svm->nested.vmcb->control.nested_cr3;
 }
 
 static void nested_svm_init_mmu_context(struct kvm_vcpu *vcpu)

@@ -81,7 +81,7 @@ static void nested_svm_init_mmu_context(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.mmu = &vcpu->arch.guest_mmu;
 	kvm_init_shadow_npt_mmu(vcpu, X86_CR0_PG, hsave->save.cr4, hsave->save.efer,
-				svm->nested.ctl.nested_cr3);
+				svm->nested.vmcb->control.nested_cr3);
 	vcpu->arch.mmu->get_guest_pgd     = nested_svm_get_tdp_cr3;
 	vcpu->arch.mmu->get_pdptr         = nested_svm_get_tdp_pdptr;
 	vcpu->arch.mmu->inject_page_fault = nested_svm_inject_npf_exit;

@@ -106,7 +106,7 @@ void recalc_intercepts(struct vcpu_svm *svm)
 
 	c = &svm->vmcb->control;
 	h = &svm->nested.hsave->control;
-	g = &svm->nested.ctl;
+	g = &svm->nested.vmcb->control;
 
 	svm->nested.host_intercept_exceptions = h->intercept_exceptions;

@@ -176,7 +176,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 	 */
 	int i;
 
-	if (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_MSR_PROT)))
+	if (!(svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
 		return true;
 
 	for (i = 0; i < MSRPM_OFFSETS; i++) {

@@ -187,7 +187,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 			break;
 
 		p      = msrpm_offsets[i];
-		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
+		offset = svm->nested.vmcb->control.msrpm_base_pa + (p * 4);
 
 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4))
 			return false;

@@ -255,12 +255,12 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
 static void load_nested_vmcb_control(struct vcpu_svm *svm,
 				     struct vmcb_control_area *control)
 {
-	copy_vmcb_control_area(&svm->nested.ctl, control);
+	copy_vmcb_control_area(&svm->nested.vmcb->control, control);
 
 	/* Copy it here because nested_svm_check_controls will check it.  */
-	svm->nested.ctl.asid           = control->asid;
-	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
-	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
+	svm->nested.vmcb->control.asid           = control->asid;
+	svm->nested.vmcb->control.msrpm_base_pa &= ~0x0fffULL;
+	svm->nested.vmcb->control.iopm_base_pa  &= ~0x0fffULL;
 }
 
 /*

@@ -270,12 +270,12 @@ static void load_nested_vmcb_control(struct vcpu_svm *svm,
 void sync_nested_vmcb_control(struct vcpu_svm *svm)
 {
 	u32 mask;
-	svm->nested.ctl.event_inj      = svm->vmcb->control.event_inj;
-	svm->nested.ctl.event_inj_err  = svm->vmcb->control.event_inj_err;
+	svm->nested.vmcb->control.event_inj      = svm->vmcb->control.event_inj;
+	svm->nested.vmcb->control.event_inj_err  = svm->vmcb->control.event_inj_err;
 
 	/* Only a few fields of int_ctl are written by the processor.  */
 	mask = V_IRQ_MASK | V_TPR_MASK;
-	if (!(svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) &&
+	if (!(svm->nested.vmcb->control.int_ctl & V_INTR_MASKING_MASK) &&
 	    svm_is_intercept(svm, INTERCEPT_VINTR)) {
 		/*
 		 * In order to request an interrupt window, L0 is usurping

@@ -287,8 +287,8 @@ void sync_nested_vmcb_control(struct vcpu_svm *svm)
 		 */
 		mask &= ~V_IRQ_MASK;
 	}
-	svm->nested.ctl.int_ctl        &= ~mask;
-	svm->nested.ctl.int_ctl        |= svm->vmcb->control.int_ctl & mask;
+	svm->nested.vmcb->control.int_ctl        &= ~mask;
+	svm->nested.vmcb->control.int_ctl        |= svm->vmcb->control.int_ctl & mask;
 }
 
 /*

@@ -330,7 +330,7 @@ static void nested_vmcb_save_pending_event(struct vcpu_svm *svm,
 
 static inline bool nested_npt_enabled(struct vcpu_svm *svm)
 {
-	return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
+	return svm->nested.vmcb->control.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
 }
 
 /*

@@ -399,20 +399,20 @@ static void nested_prepare_vmcb_control(struct vcpu_svm *svm)
 		nested_svm_init_mmu_context(&svm->vcpu);
 
 	svm->vmcb->control.tsc_offset = svm->vcpu.arch.tsc_offset =
-		svm->vcpu.arch.l1_tsc_offset + svm->nested.ctl.tsc_offset;
+		svm->vcpu.arch.l1_tsc_offset + svm->nested.vmcb->control.tsc_offset;
 
 	svm->vmcb->control.int_ctl             =
-		(svm->nested.ctl.int_ctl & ~mask) |
+		(svm->nested.vmcb->control.int_ctl & ~mask) |
 		(svm->nested.hsave->control.int_ctl & mask);
 
-	svm->vmcb->control.virt_ext            = svm->nested.ctl.virt_ext;
-	svm->vmcb->control.int_vector          = svm->nested.ctl.int_vector;
-	svm->vmcb->control.int_state           = svm->nested.ctl.int_state;
-	svm->vmcb->control.event_inj           = svm->nested.ctl.event_inj;
-	svm->vmcb->control.event_inj_err       = svm->nested.ctl.event_inj_err;
+	svm->vmcb->control.virt_ext            = svm->nested.vmcb->control.virt_ext;
+	svm->vmcb->control.int_vector          = svm->nested.vmcb->control.int_vector;
+	svm->vmcb->control.int_state           = svm->nested.vmcb->control.int_state;
+	svm->vmcb->control.event_inj           = svm->nested.vmcb->control.event_inj;
+	svm->vmcb->control.event_inj_err       = svm->nested.vmcb->control.event_inj_err;
 
-	svm->vmcb->control.pause_filter_count  = svm->nested.ctl.pause_filter_count;
-	svm->vmcb->control.pause_filter_thresh = svm->nested.ctl.pause_filter_thresh;
+	svm->vmcb->control.pause_filter_count  = svm->nested.vmcb->control.pause_filter_count;
+	svm->vmcb->control.pause_filter_thresh = svm->nested.vmcb->control.pause_filter_thresh;
 
 	/* Enter Guest-Mode */
 	enter_guest_mode(&svm->vcpu);

@@ -622,10 +622,10 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	if (svm->nrips_enabled)
 		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
 
-	nested_vmcb->control.int_ctl           = svm->nested.ctl.int_ctl;
-	nested_vmcb->control.tlb_ctl           = svm->nested.ctl.tlb_ctl;
-	nested_vmcb->control.event_inj         = svm->nested.ctl.event_inj;
-	nested_vmcb->control.event_inj_err     = svm->nested.ctl.event_inj_err;
+	nested_vmcb->control.int_ctl           = svm->nested.vmcb->control.int_ctl;
+	nested_vmcb->control.tlb_ctl           = svm->nested.vmcb->control.tlb_ctl;
+	nested_vmcb->control.event_inj         = svm->nested.vmcb->control.event_inj;
+	nested_vmcb->control.event_inj_err     = svm->nested.vmcb->control.event_inj_err;
 
 	nested_vmcb->control.pause_filter_count =
 		svm->vmcb->control.pause_filter_count;

@@ -638,7 +638,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	svm->vmcb->control.tsc_offset = svm->vcpu.arch.tsc_offset =
 		svm->vcpu.arch.l1_tsc_offset;
 
-	svm->nested.ctl.nested_cr3 = 0;
+	svm->nested.vmcb->control.nested_cr3 = 0;
 
 	/* Restore selected save entries */
 	svm->vmcb->save.es = hsave->save.es;

@@ -692,6 +692,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 int svm_allocate_nested(struct vcpu_svm *svm)
 {
 	struct page *hsave_page;
+	struct page *vmcb_page;
 
 	if (svm->nested.initialized)
 		return 0;

@@ -707,8 +708,18 @@ int svm_allocate_nested(struct vcpu_svm *svm)
 	if (!svm->nested.msrpm)
 		goto free_page2;
 
+	vmcb_page = alloc_page(GFP_KERNEL_ACCOUNT);
+	if (!vmcb_page)
+		goto free_page3;
+
+	svm->nested.vmcb = page_address(vmcb_page);
+	clear_page(svm->nested.vmcb);
+
 	svm->nested.initialized = true;
 	return 0;
+
+free_page3:
+	svm_vcpu_free_msrpm(svm->nested.msrpm);
 free_page2:
 	__free_page(hsave_page);
 free_page1:

@@ -726,6 +737,9 @@ void svm_free_nested(struct vcpu_svm *svm)
 	__free_page(virt_to_page(svm->nested.hsave));
 	svm->nested.hsave = NULL;
 
+	__free_page(virt_to_page(svm->nested.vmcb));
+	svm->nested.vmcb = NULL;
+
 	svm->nested.initialized = false;
 }

@@ -750,7 +764,7 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 	u32 offset, msr, value;
 	int write, mask;
 
-	if (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_MSR_PROT)))
+	if (!(svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
 		return NESTED_EXIT_HOST;
 
 	msr    = svm->vcpu.arch.regs[VCPU_REGS_RCX];

@@ -764,7 +778,9 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 	/* Offset is in 32 bit units but need in 8 bit units */
 	offset *= 4;
 
-	if (kvm_vcpu_read_guest(&svm->vcpu, svm->nested.ctl.msrpm_base_pa + offset, &value, 4))
+	if (kvm_vcpu_read_guest(&svm->vcpu,
+				svm->nested.vmcb->control.msrpm_base_pa + offset,
+				&value, 4))
 		return NESTED_EXIT_DONE;
 
 	return (value & mask) ? NESTED_EXIT_DONE : NESTED_EXIT_HOST;

@@ -777,13 +793,13 @@ static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
 	u8 start_bit;
 	u64 gpa;
 
-	if (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_IOIO_PROT)))
+	if (!(svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_IOIO_PROT)))
 		return NESTED_EXIT_HOST;
 
 	port = svm->vmcb->control.exit_info_1 >> 16;
 	size = (svm->vmcb->control.exit_info_1 & SVM_IOIO_SIZE_MASK) >>
 		SVM_IOIO_SIZE_SHIFT;
-	gpa  = svm->nested.ctl.iopm_base_pa + (port / 8);
+	gpa  = svm->nested.vmcb->control.iopm_base_pa + (port / 8);
 	start_bit = port % 8;
 	iopm_len = (start_bit + size > 8) ? 2 : 1;
 	mask = (0xf >> (4 - size)) << start_bit;

@@ -809,13 +825,13 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 		break;
 	case SVM_EXIT_READ_CR0 ... SVM_EXIT_WRITE_CR8: {
 		u32 bit = 1U << (exit_code - SVM_EXIT_READ_CR0);
-		if (svm->nested.ctl.intercept_cr & bit)
+		if (svm->nested.vmcb->control.intercept_cr & bit)
 			vmexit = NESTED_EXIT_DONE;
 		break;
 	}
 	case SVM_EXIT_READ_DR0 ... SVM_EXIT_WRITE_DR7: {
 		u32 bit = 1U << (exit_code - SVM_EXIT_READ_DR0);
-		if (svm->nested.ctl.intercept_dr & bit)
+		if (svm->nested.vmcb->control.intercept_dr & bit)
 			vmexit = NESTED_EXIT_DONE;
 		break;
 	}

@@ -834,7 +850,7 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 	}
 	default: {
 		u64 exit_bits = 1ULL << (exit_code - SVM_EXIT_INTR);
-		if (svm->nested.ctl.intercept & exit_bits)
+		if (svm->nested.vmcb->control.intercept & exit_bits)
 			vmexit = NESTED_EXIT_DONE;
 	}
 	}

@@ -874,7 +890,7 @@ static bool nested_exit_on_exception(struct vcpu_svm *svm)
 {
 	unsigned int nr = svm->vcpu.arch.exception.nr;
 
-	return (svm->nested.ctl.intercept_exceptions & (1 << nr));
+	return (svm->nested.vmcb->control.intercept_exceptions & (1 << nr));
 }
 
 static void nested_svm_inject_exception_vmexit(struct vcpu_svm *svm)

@@ -942,7 +958,7 @@ static void nested_svm_intr(struct vcpu_svm *svm)
 
 static inline bool nested_exit_on_init(struct vcpu_svm *svm)
 {
-	return (svm->nested.ctl.intercept & (1ULL << INTERCEPT_INIT));
+	return (svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_INIT));
 }
 
 static void nested_svm_init(struct vcpu_svm *svm)

@@ -1084,7 +1100,7 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 	 */
 	if (clear_user(user_vmcb, KVM_STATE_NESTED_SVM_VMCB_SIZE))
 		return -EFAULT;
-	if (copy_to_user(&user_vmcb->control, &svm->nested.ctl,
+	if (copy_to_user(&user_vmcb->control, &svm->nested.vmcb->control,
 			 sizeof(user_vmcb->control)))
 		return -EFAULT;
 	if (copy_to_user(&user_vmcb->save, &svm->nested.hsave->save,

arch/x86/kvm/svm/svm.c +5 -5

index d941acc36b50..0af51b54c9f5 100644
@@ -1400,8 +1400,8 @@ static void svm_clear_vintr(struct vcpu_svm *svm)
 		svm->nested.hsave->control.int_ctl &= mask;
 
 		WARN_ON((svm->vmcb->control.int_ctl & V_TPR_MASK) !=
-			(svm->nested.ctl.int_ctl & V_TPR_MASK));
-		svm->vmcb->control.int_ctl |= svm->nested.ctl.int_ctl & ~mask;
+			(svm->nested.vmcb->control.int_ctl & V_TPR_MASK));
+		svm->vmcb->control.int_ctl |= svm->nested.vmcb->control.int_ctl & ~mask;
 	}
 
 	vmcb_mark_dirty(svm->vmcb, VMCB_INTR);

@@ -2224,7 +2224,7 @@ static bool check_selective_cr0_intercepted(struct vcpu_svm *svm,
 	bool ret = false;
 	u64 intercept;
 
-	intercept = svm->nested.ctl.intercept;
+	intercept = svm->nested.vmcb->control.intercept;
 
 	if (!is_guest_mode(&svm->vcpu) ||
 	    (!(intercept & (1ULL << INTERCEPT_SELECTIVE_CR0))))

@@ -3132,7 +3132,7 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
 
 	if (is_guest_mode(vcpu)) {
 		/* As long as interrupts are being delivered...  */
-		if ((svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK)
+		if ((svm->nested.vmcb->control.int_ctl & V_INTR_MASKING_MASK)
 		    ? !(svm->nested.hsave->save.rflags & X86_EFLAGS_IF)
 		    : !(kvm_get_rflags(vcpu) & X86_EFLAGS_IF))
 			return true;

@@ -3751,7 +3751,7 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		    info->intercept == x86_intercept_clts)
 			break;
 
-		intercept = svm->nested.ctl.intercept;
+		intercept = svm->nested.vmcb->control.intercept;
 
 		if (!(intercept & (1ULL << INTERCEPT_SELECTIVE_CR0)))
 			break;

arch/x86/kvm/svm/svm.h +8 -7

index 9dca64a2edb5..1669755f796e 100644
@@ -85,7 +85,11 @@ struct svm_nested_state {
 	struct vmcb *hsave;
 	u64 hsave_msr;
 	u64 vm_cr_msr;
+
+	/* guest mode vmcb, aka vmcb12*/
+	struct vmcb *vmcb;
 	u64 vmcb_gpa;
+
 	u32 host_intercept_exceptions;
 
 	/* These are the merged vectors */

@@ -95,9 +99,6 @@ struct svm_nested_state {
 	 * we cannot inject a nested vmexit yet.  */
 	bool nested_run_pending;
 
-	/* cache for control fields of the guest */
-	struct vmcb_control_area ctl;
-
 	bool initialized;
 };

@@ -373,22 +374,22 @@ static inline bool nested_svm_virtualize_tpr(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	return is_guest_mode(vcpu) && (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK);
+	return is_guest_mode(vcpu) && (svm->nested.vmcb->control.int_ctl & V_INTR_MASKING_MASK);
 }
 
 static inline bool nested_exit_on_smi(struct vcpu_svm *svm)
 {
-	return (svm->nested.ctl.intercept & (1ULL << INTERCEPT_SMI));
+	return (svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_SMI));
 }
 
 static inline bool nested_exit_on_intr(struct vcpu_svm *svm)
 {
-	return (svm->nested.ctl.intercept & (1ULL << INTERCEPT_INTR));
+	return (svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_INTR));
 }
 
 static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
 {
-	return (svm->nested.ctl.intercept & (1ULL << INTERCEPT_NMI));
+	return (svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_NMI));
 }
 
 int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,

Signature · 1 line

2.26.2

Maxim Levitsky

author

2020-08-20 09:14 UTC #16 lore raw

PATCH 7/8 KVM: nSVM: implement caching of nested vmcb save area

In reply to Maxim Levitsky

Soon we will have some checks on the save area and this caching will allow us to read the guest's vmcb save area once and then use it, thus avoiding case when a malicious guest changes it after we verified it, but before we copied parts of it to the main vmcb.

On nested vmexit also sync the updated save state area of the guest, to the cache, although this is probably overkill.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 131 ++++++++++++++++++++++++++------------
 arch/x86/kvm/svm/svm.c    |   6 +-
 arch/x86/kvm/svm/svm.h    |   4 +-
 3 files changed, 97 insertions(+), 44 deletions(-)

arch/x86/kvm/svm/nested.c +92 ~~-39~~

index c9bb17e9ba11..acc4b26fcfcc 100644
@@ -215,9 +215,11 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
 	return true;
 }
 
-static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
+static bool nested_vmcb_checks(struct vcpu_svm *svm)
 {
 	bool nested_vmcb_lma;
+	struct vmcb *vmcb = svm->nested.vmcb;
+
 	if ((vmcb->save.efer & EFER_SVME) == 0)
 		return false;

@@ -263,6 +265,43 @@ static void load_nested_vmcb_control(struct vcpu_svm *svm,
 	svm->nested.vmcb->control.iopm_base_pa  &= ~0x0fffULL;
 }
 
+static void load_nested_vmcb_save(struct vcpu_svm *svm,
+				  struct vmcb_save_area *save)
+{
+	svm->nested.vmcb->save.rflags = save->rflags;
+	svm->nested.vmcb->save.rax    = save->rax;
+	svm->nested.vmcb->save.rsp    = save->rsp;
+	svm->nested.vmcb->save.rip    = save->rip;
+
+	svm->nested.vmcb->save.es  = save->es;
+	svm->nested.vmcb->save.cs  = save->cs;
+	svm->nested.vmcb->save.ss  = save->ss;
+	svm->nested.vmcb->save.ds  = save->ds;
+	svm->nested.vmcb->save.cpl = save->cpl;
+
+	svm->nested.vmcb->save.gdtr = save->gdtr;
+	svm->nested.vmcb->save.idtr = save->idtr;
+
+	svm->nested.vmcb->save.efer = save->efer;
+	svm->nested.vmcb->save.cr3 = save->cr3;
+	svm->nested.vmcb->save.cr4 = save->cr4;
+	svm->nested.vmcb->save.cr0 = save->cr0;
+
+	svm->nested.vmcb->save.cr2 = save->cr2;
+
+	svm->nested.vmcb->save.dr7 = save->dr7;
+	svm->nested.vmcb->save.dr6 = save->dr6;
+
+	svm->nested.vmcb->save.g_pat = save->g_pat;
+}
+
+void load_nested_vmcb(struct vcpu_svm *svm, struct vmcb *nested_vmcb, u64 vmcb_gpa)
+{
+	svm->nested.vmcb_gpa = vmcb_gpa;
+	load_nested_vmcb_control(svm, &nested_vmcb->control);
+	load_nested_vmcb_save(svm, &nested_vmcb->save);
+}
+
 /*
  * Synchronize fields that are written by the processor, so that
  * they can be copied back into the nested_vmcb.

@@ -364,31 +403,31 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
 	return 0;
 }
 
-static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_vmcb)
+static void nested_prepare_vmcb_save(struct vcpu_svm *svm)
 {
 	/* Load the nested guest state */
-	svm->vmcb->save.es = nested_vmcb->save.es;
-	svm->vmcb->save.cs = nested_vmcb->save.cs;
-	svm->vmcb->save.ss = nested_vmcb->save.ss;
-	svm->vmcb->save.ds = nested_vmcb->save.ds;
-	svm->vmcb->save.gdtr = nested_vmcb->save.gdtr;
-	svm->vmcb->save.idtr = nested_vmcb->save.idtr;
-	kvm_set_rflags(&svm->vcpu, nested_vmcb->save.rflags);
-	svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
-	svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
-	svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
-	svm->vmcb->save.cr2 = svm->vcpu.arch.cr2 = nested_vmcb->save.cr2;
-	kvm_rax_write(&svm->vcpu, nested_vmcb->save.rax);
-	kvm_rsp_write(&svm->vcpu, nested_vmcb->save.rsp);
-	kvm_rip_write(&svm->vcpu, nested_vmcb->save.rip);
+	svm->vmcb->save.es = svm->nested.vmcb->save.es;
+	svm->vmcb->save.cs = svm->nested.vmcb->save.cs;
+	svm->vmcb->save.ss = svm->nested.vmcb->save.ss;
+	svm->vmcb->save.ds = svm->nested.vmcb->save.ds;
+	svm->vmcb->save.gdtr = svm->nested.vmcb->save.gdtr;
+	svm->vmcb->save.idtr = svm->nested.vmcb->save.idtr;
+	kvm_set_rflags(&svm->vcpu, svm->nested.vmcb->save.rflags);
+	svm_set_efer(&svm->vcpu, svm->nested.vmcb->save.efer);
+	svm_set_cr0(&svm->vcpu, svm->nested.vmcb->save.cr0);
+	svm_set_cr4(&svm->vcpu, svm->nested.vmcb->save.cr4);
+	svm->vmcb->save.cr2 = svm->vcpu.arch.cr2 = svm->nested.vmcb->save.cr2;
+	kvm_rax_write(&svm->vcpu, svm->nested.vmcb->save.rax);
+	kvm_rsp_write(&svm->vcpu, svm->nested.vmcb->save.rsp);
+	kvm_rip_write(&svm->vcpu, svm->nested.vmcb->save.rip);
 
 	/* In case we don't even reach vcpu_run, the fields are not updated */
-	svm->vmcb->save.rax = nested_vmcb->save.rax;
-	svm->vmcb->save.rsp = nested_vmcb->save.rsp;
-	svm->vmcb->save.rip = nested_vmcb->save.rip;
-	svm->vmcb->save.dr7 = nested_vmcb->save.dr7;
-	svm->vcpu.arch.dr6  = nested_vmcb->save.dr6;
-	svm->vmcb->save.cpl = nested_vmcb->save.cpl;
+	svm->vmcb->save.rax = svm->nested.vmcb->save.rax;
+	svm->vmcb->save.rsp = svm->nested.vmcb->save.rsp;
+	svm->vmcb->save.rip = svm->nested.vmcb->save.rip;
+	svm->vmcb->save.dr7 = svm->nested.vmcb->save.dr7;
+	svm->vcpu.arch.dr6  = svm->nested.vmcb->save.dr6;
+	svm->vmcb->save.cpl = svm->nested.vmcb->save.cpl;
 }
 
 static void nested_prepare_vmcb_control(struct vcpu_svm *svm)

@@ -426,17 +465,13 @@ static void nested_prepare_vmcb_control(struct vcpu_svm *svm)
 	vmcb_mark_all_dirty(svm->vmcb);
 }
 
-int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
-			  struct vmcb *nested_vmcb)
+int enter_svm_guest_mode(struct vcpu_svm *svm)
 {
 	int ret;
-
-	svm->nested.vmcb_gpa = vmcb_gpa;
-	load_nested_vmcb_control(svm, &nested_vmcb->control);
-	nested_prepare_vmcb_save(svm, nested_vmcb);
+	nested_prepare_vmcb_save(svm);
 	nested_prepare_vmcb_control(svm);
 
-	ret = nested_svm_load_cr3(&svm->vcpu, nested_vmcb->save.cr3,
+	ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.vmcb->save.cr3,
 				  nested_npt_enabled(svm));
 	if (ret)
 		return ret;

@@ -476,7 +511,9 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 	if (WARN_ON(!svm->nested.initialized))
 		return 1;
 
-	if (!nested_vmcb_checks(svm, nested_vmcb)) {
+	load_nested_vmcb(svm, nested_vmcb, vmcb_gpa);
+
+	if (!nested_vmcb_checks(svm)) {
 		nested_vmcb->control.exit_code    = SVM_EXIT_ERR;
 		nested_vmcb->control.exit_code_hi = 0;
 		nested_vmcb->control.exit_info_1  = 0;

@@ -485,15 +522,15 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 	}
 
 	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
-			       nested_vmcb->save.rip,
-			       nested_vmcb->control.int_ctl,
-			       nested_vmcb->control.event_inj,
-			       nested_vmcb->control.nested_ctl);
+			       svm->nested.vmcb->save.rip,
+			       svm->nested.vmcb->control.int_ctl,
+			       svm->nested.vmcb->control.event_inj,
+			       svm->nested.vmcb->control.nested_ctl);
 
-	trace_kvm_nested_intercepts(nested_vmcb->control.intercept_cr & 0xffff,
-				    nested_vmcb->control.intercept_cr >> 16,
-				    nested_vmcb->control.intercept_exceptions,
-				    nested_vmcb->control.intercept);
+	trace_kvm_nested_intercepts(svm->nested.vmcb->control.intercept_cr & 0xffff,
+				    svm->nested.vmcb->control.intercept_cr >> 16,
+				    svm->nested.vmcb->control.intercept_exceptions,
+				    svm->nested.vmcb->control.intercept);
 
 	/* Clear internal status */
 	kvm_clear_exception_queue(&svm->vcpu);

@@ -525,7 +562,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 
 	svm->nested.nested_run_pending = 1;
 
-	if (enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb))
+	if (enter_svm_guest_mode(svm))
 		goto out_exit_err;
 
 	if (nested_svm_vmrun_msrpm(svm))

@@ -632,6 +669,15 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	nested_vmcb->control.pause_filter_thresh =
 		svm->vmcb->control.pause_filter_thresh;
 
+	/*
+	 * Write back the nested vmcb state that we just updated
+	 * to the nested vmcb cache to keep it up to date
+	 *
+	 * Note: since CPU might have changed the values we can't
+	 * trust clean bits
+	 */
+	load_nested_vmcb_save(svm, &nested_vmcb->save);
+
 	/* Restore the original control entries */
 	copy_vmcb_control_area(&vmcb->control, &hsave->control);

@@ -1191,6 +1237,13 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	load_nested_vmcb_control(svm, &ctl);
 	nested_prepare_vmcb_control(svm);
 
+	/*
+	 * TODO: cached nested guest vmcb data area is not restored, thus
+	 * it will be invalid till nested guest vmexits.
+	 * It shoudn't matter much since the area is not supposed to be
+	 * in sync with cpu anyway while nested guest is running
+	 */
+
 out_set_gif:
 	svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
 	return 0;

arch/x86/kvm/svm/svm.c +3 -3

index 0af51b54c9f5..06668e0f93e7 100644
@@ -3904,7 +3904,6 @@ static int svm_pre_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
 static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
-	struct vmcb *nested_vmcb;
 	struct kvm_host_map map;
 	u64 guest;
 	u64 vmcb_gpa;

@@ -3925,8 +3924,9 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
 
-		nested_vmcb = map.hva;
-		ret = enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb);
+		load_nested_vmcb(svm, map.hva, vmcb);
+		ret = enter_svm_guest_mode(svm);
+
 		kvm_vcpu_unmap(&svm->vcpu, &map, true);
 	}

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

index 1669755f796e..80231ef8de6f 100644
@@ -392,8 +392,7 @@ static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
 	return (svm->nested.vmcb->control.intercept & (1ULL << INTERCEPT_NMI));
 }
 
-int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
-			 struct vmcb *nested_vmcb);
+int enter_svm_guest_mode(struct vcpu_svm *svm);
 void svm_leave_nested(struct vcpu_svm *svm);
 void svm_free_nested(struct vcpu_svm *svm);
 int svm_allocate_nested(struct vcpu_svm *svm);

@@ -406,6 +405,7 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 			       bool has_error_code, u32 error_code);
 int nested_svm_exit_special(struct vcpu_svm *svm);
 void sync_nested_vmcb_control(struct vcpu_svm *svm);
+void load_nested_vmcb(struct vcpu_svm *svm, struct vmcb *nested_vmcb, u64 vmcb_gpa);
 
 extern struct kvm_x86_nested_ops svm_nested_ops;

Signature · 1 line

2.26.2

Maxim Levitsky

author

2020-08-20 09:15 UTC #17 lore raw

PATCH 8/8 KVM: nSVM: read only changed fields of the nested guest data area

In reply to Maxim Levitsky

This allows us to only read fields that are marked as dirty by the nested guest on vmentry.

I doubt that this has any perf impact but this way it is a bit closer to real hardware.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/svm/nested.c | 58 +++++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c    |  2 +-
 arch/x86/kvm/svm/svm.h    |  5 ++++
 3 files changed, 44 insertions(+), 21 deletions(-)

arch/x86/kvm/svm/nested.c +38 ~~-20~~

index acc4b26fcfcc..f3eef48caee6 100644
@@ -266,40 +266,57 @@ static void load_nested_vmcb_control(struct vcpu_svm *svm,
 }
 
 static void load_nested_vmcb_save(struct vcpu_svm *svm,
-				  struct vmcb_save_area *save)
+				  struct vmcb_save_area *save,
+				  u32 clean)
 {
 	svm->nested.vmcb->save.rflags = save->rflags;
 	svm->nested.vmcb->save.rax    = save->rax;
 	svm->nested.vmcb->save.rsp    = save->rsp;
 	svm->nested.vmcb->save.rip    = save->rip;
 
-	svm->nested.vmcb->save.es  = save->es;
-	svm->nested.vmcb->save.cs  = save->cs;
-	svm->nested.vmcb->save.ss  = save->ss;
-	svm->nested.vmcb->save.ds  = save->ds;
-	svm->nested.vmcb->save.cpl = save->cpl;
+	if (is_dirty(clean, VMCB_SEG)) {
+		svm->nested.vmcb->save.es  = save->es;
+		svm->nested.vmcb->save.cs  = save->cs;
+		svm->nested.vmcb->save.ss  = save->ss;
+		svm->nested.vmcb->save.ds  = save->ds;
+		svm->nested.vmcb->save.cpl = save->cpl;
+	}
 
-	svm->nested.vmcb->save.gdtr = save->gdtr;
-	svm->nested.vmcb->save.idtr = save->idtr;
+	if (is_dirty(clean, VMCB_DT)) {
+		svm->nested.vmcb->save.gdtr = save->gdtr;
+		svm->nested.vmcb->save.idtr = save->idtr;
+	}
 
-	svm->nested.vmcb->save.efer = save->efer;
-	svm->nested.vmcb->save.cr3 = save->cr3;
-	svm->nested.vmcb->save.cr4 = save->cr4;
-	svm->nested.vmcb->save.cr0 = save->cr0;
+	if (is_dirty(clean, VMCB_CR)) {
+		svm->nested.vmcb->save.efer = save->efer;
+		svm->nested.vmcb->save.cr3 = save->cr3;
+		svm->nested.vmcb->save.cr4 = save->cr4;
+		svm->nested.vmcb->save.cr0 = save->cr0;
+	}
 
-	svm->nested.vmcb->save.cr2 = save->cr2;
+	if (is_dirty(clean, VMCB_CR2))
+		svm->nested.vmcb->save.cr2 = save->cr2;
 
-	svm->nested.vmcb->save.dr7 = save->dr7;
-	svm->nested.vmcb->save.dr6 = save->dr6;
+	if (is_dirty(clean, VMCB_DR)) {
+		svm->nested.vmcb->save.dr7 = save->dr7;
+		svm->nested.vmcb->save.dr6 = save->dr6;
+	}
 
-	svm->nested.vmcb->save.g_pat = save->g_pat;
+	if ((clean & VMCB_NPT) == 0)
+		svm->nested.vmcb->save.g_pat = save->g_pat;
 }
 
 void load_nested_vmcb(struct vcpu_svm *svm, struct vmcb *nested_vmcb, u64 vmcb_gpa)
 {
-	svm->nested.vmcb_gpa = vmcb_gpa;
+	u32 clean = nested_vmcb->control.clean;
+
+	if (svm->nested.vmcb_gpa != vmcb_gpa) {
+		svm->nested.vmcb_gpa = vmcb_gpa;
+		clean = 0;
+	}
+
 	load_nested_vmcb_control(svm, &nested_vmcb->control);
-	load_nested_vmcb_save(svm, &nested_vmcb->save);
+	load_nested_vmcb_save(svm, &nested_vmcb->save, clean);
 }
 
 /*

@@ -619,7 +636,6 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 
 	/* Exit Guest-Mode */
 	leave_guest_mode(&svm->vcpu);
-	svm->nested.vmcb_gpa = 0;
 	WARN_ON_ONCE(svm->nested.nested_run_pending);
 
 	/* in case we halted in L2 */

@@ -676,7 +692,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	 * Note: since CPU might have changed the values we can't
 	 * trust clean bits
 	 */
-	load_nested_vmcb_save(svm, &nested_vmcb->save);
+	load_nested_vmcb_save(svm, &nested_vmcb->save, 0);
 
 	/* Restore the original control entries */
 	copy_vmcb_control_area(&vmcb->control, &hsave->control);

@@ -759,6 +775,7 @@ int svm_allocate_nested(struct vcpu_svm *svm)
 		goto free_page3;
 
 	svm->nested.vmcb = page_address(vmcb_page);
+	svm->nested.vmcb_gpa = U64_MAX;
 	clear_page(svm->nested.vmcb);
 
 	svm->nested.initialized = true;

@@ -785,6 +802,7 @@ void svm_free_nested(struct vcpu_svm *svm)
 
 	__free_page(virt_to_page(svm->nested.vmcb));
 	svm->nested.vmcb = NULL;
+	svm->nested.vmcb_gpa = U64_MAX;
 
 	svm->nested.initialized = false;
 }

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

index 06668e0f93e7..f0bb7f622dca 100644
@@ -3924,7 +3924,7 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
 
-		load_nested_vmcb(svm, map.hva, vmcb);
+		load_nested_vmcb(svm, map.hva, vmcb_gpa);
 		ret = enter_svm_guest_mode(svm);
 
 		kvm_vcpu_unmap(&svm->vcpu, &map, true);

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

index 80231ef8de6f..4a383c519fdf 100644
@@ -204,6 +204,11 @@ static inline void vmcb_mark_dirty(struct vmcb *vmcb, int bit)
 	vmcb->control.clean &= ~(1 << bit);
 }
 
+static inline bool is_dirty(u32 clean, int bit)
+{
+	return (clean & (1 << bit)) == 0;
+}
+
 static inline struct vcpu_svm *to_svm(struct kvm_vcpu *vcpu)
 {
 	return container_of(vcpu, struct vcpu_svm, vcpu);

Signature · 1 line

2.26.2

Paolo Bonzini

maintainer

2020-08-20 10:07 UTC #18 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

arch/x86/kvm/svm/nested.c static void load_nested_vmcb_control(struct vcpu_svm *svm, · line 316

Quoted 6 lines · Maxim Levitsky

arch/x86/kvm/svm/nested.c +6 -0

+	u32 clean = nested_vmcb->control.clean;
+
+	if (svm->nested.vmcb_gpa != vmcb_gpa) {
+		svm->nested.vmcb_gpa = vmcb_gpa;
+		clean = 0;
+	}

You probably should set clean to 0 also if the guest doesn't have the VMCBCLEAN feature (so, you first need an extra patch to add the VMCBCLEAN feature to cpufeatures.h). It's probably best to cache the guest vmcbclean in struct vcpu_svm, too.

Paolo

Maxim Levitsky

author

2020-08-20 11:30 UTC #19 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

Quoted 12 lines · Paolo Bonzini

arch/x86/kvm/svm/nested.c static void load_nested_vmcb_control(struct vcpu_svm *svm, · line 316
Quoted 6 lines · Maxim Levitsky
arch/x86/kvm/svm/nested.c +6 -0
+	u32 clean = nested_vmcb->control.clean;
+
+	if (svm->nested.vmcb_gpa != vmcb_gpa) {
+		svm->nested.vmcb_gpa = vmcb_gpa;
+		clean = 0;
+	}
You probably should set clean to 0 also if the guest doesn't have the VMCBCLEAN feature (so, you first need an extra patch to add the VMCBCLEAN feature to cpufeatures.h). It's probably best to cache the guest vmcbclean in struct vcpu_svm, too.

Right, I totally forgot about this one.

One thing why I made this patch optional, is that I can instead drop it, and not 'read back' the saved area on vmexit, this will probably be faster that what this optimization does. What do you think? Is this patch worth it? (I submitted it because I already implemented this and wanted to hear opinion on this).

Best regards,
Maxim Levitsky

Quoted 3 lines

Paolo

Paolo Bonzini

maintainer

2020-08-20 10:36 UTC #20 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

Quoted 11 lines · Maxim Levitsky

Quoted 4 lines
You probably should set clean to 0 also if the guest doesn't have the VMCBCLEAN feature (so, you first need an extra patch to add the VMCBCLEAN feature to cpufeatures.h). It's probably best to cache the guest vmcbclean in struct vcpu_svm, too.
Right, I totally forgot about this one.
One thing why I made this patch optional, is that I can instead drop it, and not 'read back' the saved area on vmexit, this will probably be faster that what this optimization does. What do you think? Is this patch worth it? (I submitted it because I already implemented this and wanted to hear opinion on this).

Yeah, good point. It's one copy either way, either on vmexit (and partly on vmentry depending on clean bits) or on vmentry. I had not considered the need to copy from vmcb02 to the cached vmcb12 on vmexit. :(

Let's shelve this for a bit, and revisit it once we have separate vmcb01 and vmcb02. Then we might still use the clean bits to avoid copying data from vmcb12 to vmcb02, including avoiding consistency checks because we know the vmcb02 data is legit.

Patches 1-5 are still worthwhile, so you can clean them up and send them.

Paolo

Maxim Levitsky

author

2020-08-20 10:55 UTC #21 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

Quoted 13 lines · Paolo Bonzini

Quoted 11 lines · Maxim Levitsky
Quoted 4 lines
You probably should set clean to 0 also if the guest doesn't have the VMCBCLEAN feature (so, you first need an extra patch to add the VMCBCLEAN feature to cpufeatures.h). It's probably best to cache the guest vmcbclean in struct vcpu_svm, too.
Right, I totally forgot about this one.
One thing why I made this patch optional, is that I can instead drop it, and not 'read back' the saved area on vmexit, this will probably be faster that what this optimization does. What do you think? Is this patch worth it? (I submitted it because I already implemented this and wanted to hear opinion on this).

Quoted 8 lines

Yeah, good point. It's one copy either way, either on vmexit (and partly on vmentry depending on clean bits) or on vmentry. I had not considered the need to copy from vmcb02 to the cached vmcb12 on vmexit. :(
Let's shelve this for a bit, and revisit it once we have separate vmcb01 and vmcb02. Then we might still use the clean bits to avoid copying data from vmcb12 to vmcb02, including avoiding consistency checks because we know the vmcb02 data is legit.

It makes sense I guess. The vmcb02 would then play the role of the cache of vmcb12

Quoted 4 lines

Patches 1-5 are still worthwhile, so you can clean them up and send them.
Paolo

OK, on it now.

Best regards,
Maxim Levitsky

Quoted 1 line

Paolo Bonzini

maintainer

2020-08-20 12:20 UTC #22 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

In reply to Maxim Levitsky

arch/x86/kvm/svm/svm.c static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) · line 3929

Quoted 12 lines · Maxim Levitsky

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

index 06668e0f93e7..f0bb7f622dca 100644
@@ -3924,7 +3924,7 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;
 
-		load_nested_vmcb(svm, map.hva, vmcb);
+		load_nested_vmcb(svm, map.hva, vmcb_gpa);
 		ret = enter_svm_guest_mode(svm);

Wrong patch?

Paolo

Maxim Levitsky

author

2020-08-20 12:10 UTC #23 lore raw

Re: [PATCH 8/8] KVM: nSVM: read only changed fields of the nested guest data area

Quoted 9 lines · Paolo Bonzini

Quoted 8 lines · Maxim Levitsky

arch/x86/kvm/svm/svm.c +0 -0

index 06668e0f93e7..f0bb7f622dca 100644
@@ -3924,7 +3924,7 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 		if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map) == -EINVAL)
 			return 1;

Quoted 6 lines

arch/x86/kvm/svm/svm.c static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) · line 3929
Quoted 4 lines
arch/x86/kvm/svm/svm.c +1 -1
-		load_nested_vmcb(svm, map.hva, vmcb);
+		load_nested_vmcb(svm, map.hva, vmcb_gpa);
 		ret = enter_svm_guest_mode(svm);
 
Wrong patch?

Absolutely. I reordered the refactoring patches to be at the beginning, and didn't test this enough.

Best regards,
Maxim Levitsky

Quoted 3 lines

Paolo