[PATCH 0/8] KVM: nSVM: ondemand nested state allocation + nested guest state caching

[PATCH 0/8] KVM: nSVM: ondemand nested state allocation + nested guest state caching

[Maxim Levitsky]

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

-- 
2.26.2

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

[Maxim Levitsky]

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

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 03dd7bac8034..562a79e3e63a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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;
 }
-- 
2.26.2

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

[Maxim Levitsky]

No functional changes.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index fb68467e6049..d9755eab2199 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -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);
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 562a79e3e63a..4338d2a2596e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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);
 	}
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a798e1731709..03f2f082ef10 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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 */
-- 
2.26.2

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

[Paolo Bonzini]

On 20/08/20 11:13, Maxim Levitsky wrote:
> No functional changes.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  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(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index fb68467e6049..d9755eab2199 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -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);
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 562a79e3e63a..4338d2a2596e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -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);
>  	}
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index a798e1731709..03f2f082ef10 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 11:56 +0200, Paolo Bonzini wrote:
> On 20/08/20 11:13, Maxim Levitsky wrote:
> > No functional changes.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  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(-)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index fb68467e6049..d9755eab2199 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -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);
> >  
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 562a79e3e63a..4338d2a2596e 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -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);
> >  	}
> >  
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index a798e1731709..03f2f082ef10 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -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).
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.

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

> 

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

[Paolo Bonzini]

On 20/08/20 12:00, Maxim Levitsky wrote:
>> 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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 12:19 +0200, Paolo Bonzini wrote:
> On 20/08/20 12:00, Maxim Levitsky wrote:
> > > 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

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

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

[Paolo Bonzini]

On 20/08/20 12:23, Maxim Levitsky wrote:
> 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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 12:56 +0200, Paolo Bonzini wrote:
> On 20/08/20 12:23, Maxim Levitsky wrote:
> > 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.
This makes sense.
Best regards,
	Maxim Levitsky

> 
> Paolo
> 

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

[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 <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 4338d2a2596e..e072ecace466 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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:
-- 
2.26.2

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

[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 <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5ab3af7275d8..bd0519e26053 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e072ecace466..ce0773c9a7fa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 03f2f082ef10..ef16f708ed1c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 46ba2e03a892..e90b9e68c7ea 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2db369a64f29..cad5d9778a21 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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)
-- 
2.26.2

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

[Maxim Levitsky]

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

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index d9755eab2199..b6704611fc02 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -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.
  */
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ce0773c9a7fa..d941acc36b50 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -266,6 +266,7 @@ static int get_max_npt_level(void)
 int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	u64 old_efer = vcpu->arch.efer;
 	vcpu->arch.efer = efer;
 
 	if (!npt_enabled) {
@@ -276,14 +277,31 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 			efer &= ~EFER_LME;
 	}
 
-	if (!(efer & EFER_SVME)) {
-		svm_leave_nested(svm);
-		svm_set_gif(svm, true);
+	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
+		if (!(efer & EFER_SVME)) {
+			svm_leave_nested(svm);
+			svm_set_gif(svm, true);
+
+			/*
+			 * Free the nested state unless we are in SMM, in which
+			 * case the exit from SVM mode is only for duration of the SMI
+			 * handler
+			 */
+			if (!is_smm(&svm->vcpu))
+				svm_free_nested(svm);
+
+		} else {
+			if (svm_allocate_nested(svm))
+				goto error;
+		}
 	}
 
 	svm->vmcb->save.efer = efer | EFER_SVME;
 	vmcb_mark_dirty(svm->vmcb, VMCB_CR);
 	return 0;
+error:
+	vcpu->arch.efer = old_efer;
+	return 1;
 }
 
 static int is_external_interrupt(u32 info)
@@ -610,7 +628,7 @@ static void set_msr_interception(u32 *msrpm, unsigned msr,
 	msrpm[offset] = tmp;
 }
 
-static u32 *svm_vcpu_alloc_msrpm(void)
+u32 *svm_vcpu_alloc_msrpm(void)
 {
 	int i;
 	u32 *msrpm;
@@ -630,7 +648,7 @@ static u32 *svm_vcpu_alloc_msrpm(void)
 	return msrpm;
 }
 
-static void svm_vcpu_free_msrpm(u32 *msrpm)
+void svm_vcpu_free_msrpm(u32 *msrpm)
 {
 	__free_pages(virt_to_page(msrpm), MSRPM_ALLOC_ORDER);
 }
@@ -1184,7 +1202,6 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm;
 	struct page *vmcb_page;
-	struct page *hsave_page;
 	int err;
 
 	BUILD_BUG_ON(offsetof(struct vcpu_svm, vcpu) != 0);
@@ -1195,13 +1212,9 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	if (!vmcb_page)
 		goto out;
 
-	hsave_page = alloc_page(GFP_KERNEL_ACCOUNT);
-	if (!hsave_page)
-		goto free_page1;
-
 	err = avic_init_vcpu(svm);
 	if (err)
-		goto free_page2;
+		goto out;
 
 	/* We initialize this flag to true to make sure that the is_running
 	 * bit would be set the first time the vcpu is loaded.
@@ -1209,16 +1222,9 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	if (irqchip_in_kernel(vcpu->kvm) && kvm_apicv_activated(vcpu->kvm))
 		svm->avic_is_running = true;
 
-	svm->nested.hsave = page_address(hsave_page);
-	clear_page(svm->nested.hsave);
-
 	svm->msrpm = svm_vcpu_alloc_msrpm();
 	if (!svm->msrpm)
-		goto free_page2;
-
-	svm->nested.msrpm = svm_vcpu_alloc_msrpm();
-	if (!svm->nested.msrpm)
-		goto free_page3;
+		goto free_page;
 
 	svm->vmcb = page_address(vmcb_page);
 	clear_page(svm->vmcb);
@@ -1231,11 +1237,7 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 
 	return 0;
 
-free_page3:
-	svm_vcpu_free_msrpm(svm->msrpm);
-free_page2:
-	__free_page(hsave_page);
-free_page1:
+free_page:
 	__free_page(vmcb_page);
 out:
 	return err;
@@ -1260,10 +1262,10 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 	 */
 	svm_clear_current_vmcb(svm->vmcb);
 
+	svm_free_nested(svm);
+
 	__free_page(pfn_to_page(__sme_clr(svm->vmcb_pa) >> PAGE_SHIFT));
 	__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
-	__free_page(virt_to_page(svm->nested.hsave));
-	__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
 }
 
 static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -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) {
+		/*
+		 * 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;
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index ef16f708ed1c..9dca64a2edb5 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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);
-- 
2.26.2

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

[Paolo Bonzini]

On 20/08/20 11:13, Maxim Levitsky wrote:
> @@ -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) {
> +		/*
> +		 * 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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 11:58 +0200, Paolo Bonzini wrote:
> On 20/08/20 11:13, Maxim Levitsky wrote:
> > @@ -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) {
> > +		/*
> > +		 * 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
> 
Makes sense, will do.

Best regards,
	Maxim Levitsky

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

[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 <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index b6704611fc02..c9bb17e9ba11 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -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,
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d941acc36b50..0af51b54c9f5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 9dca64a2edb5..1669755f796e 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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,
-- 
2.26.2

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

[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 <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index c9bb17e9ba11..acc4b26fcfcc 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -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;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 0af51b54c9f5..06668e0f93e7 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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);
 	}
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 1669755f796e..80231ef8de6f 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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;
 
-- 
2.26.2

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

[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 <mlevitsk@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index acc4b26fcfcc..f3eef48caee6 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -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;
 }
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 06668e0f93e7..f0bb7f622dca 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 80231ef8de6f..4a383c519fdf 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -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);
-- 
2.26.2

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

[Paolo Bonzini]

On 20/08/20 11:13, Maxim Levitsky wrote:
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 06668e0f93e7..f0bb7f622dca 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 11:55 +0200, Paolo Bonzini wrote:
> On 20/08/20 11:13, Maxim Levitsky wrote:
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 06668e0f93e7..f0bb7f622dca 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -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?

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

Best regards,
	Maxim Levitsky

> 
> Paolo
> 

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

[Paolo Bonzini]

On 20/08/20 11:13, Maxim Levitsky wrote:
> +	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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 12:01 +0200, Paolo Bonzini wrote:
> On 20/08/20 11:13, Maxim Levitsky wrote:
> > +	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

> 
> Paolo
> 

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

[Paolo Bonzini]

On 20/08/20 12:05, Maxim Levitsky wrote:
>> 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

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

[Maxim Levitsky]

On Thu, 2020-08-20 at 12:18 +0200, Paolo Bonzini wrote:
> On 20/08/20 12:05, Maxim Levitsky wrote:
> > > 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.
It makes sense I guess. The vmcb02 would then play the role of the cache of
vmcb12

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

OK, on it now.

Best regards,
	Maxim Levitsky
>