[PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Joel Schopp]

There isn't really a valid reason for kvm to intercept cr* reads
on svm hardware.  The current kvm code just ends up returning
the register

Signed-off-by: Joel Schopp <joel.schopp@amd.com>
---
 arch/x86/kvm/svm.c |   41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index cc618c8..c3d10e6 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1090,9 +1090,6 @@ static void init_vmcb(struct vcpu_svm *svm)
 	svm->vcpu.fpu_active = 1;
 	svm->vcpu.arch.hflags = 0;
 
-	set_cr_intercept(svm, INTERCEPT_CR0_READ);
-	set_cr_intercept(svm, INTERCEPT_CR3_READ);
-	set_cr_intercept(svm, INTERCEPT_CR4_READ);
 	set_cr_intercept(svm, INTERCEPT_CR0_WRITE);
 	set_cr_intercept(svm, INTERCEPT_CR3_WRITE);
 	set_cr_intercept(svm, INTERCEPT_CR4_WRITE);
@@ -1174,7 +1171,6 @@ static void init_vmcb(struct vcpu_svm *svm)
 		control->nested_ctl = 1;
 		clr_intercept(svm, INTERCEPT_INVLPG);
 		clr_exception_intercept(svm, PF_VECTOR);
-		clr_cr_intercept(svm, INTERCEPT_CR3_READ);
 		clr_cr_intercept(svm, INTERCEPT_CR3_WRITE);
 		save->g_pat = 0x0007040600070406ULL;
 		save->cr3 = 0;
@@ -2968,29 +2964,10 @@ static int cr_interception(struct vcpu_svm *svm)
 			kvm_queue_exception(&svm->vcpu, UD_VECTOR);
 			return 1;
 		}
-	} else { /* mov from cr */
-		switch (cr) {
-		case 0:
-			val = kvm_read_cr0(&svm->vcpu);
-			break;
-		case 2:
-			val = svm->vcpu.arch.cr2;
-			break;
-		case 3:
-			val = kvm_read_cr3(&svm->vcpu);
-			break;
-		case 4:
-			val = kvm_read_cr4(&svm->vcpu);
-			break;
-		case 8:
-			val = kvm_get_cr8(&svm->vcpu);
-			break;
-		default:
-			WARN(1, "unhandled read from CR%d", cr);
-			kvm_queue_exception(&svm->vcpu, UD_VECTOR);
-			return 1;
-		}
-		kvm_register_write(&svm->vcpu, reg, val);
+	} else { /* mov from cr, should never trap in svm */
+		WARN(1, "unhandled read from CR%d", cr);
+		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
+		return 1;
 	}
 	kvm_complete_insn_gp(&svm->vcpu, err);
 
@@ -3321,10 +3298,6 @@ static int mwait_interception(struct vcpu_svm *svm)
 }
 
 static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
-	[SVM_EXIT_READ_CR0]			= cr_interception,
-	[SVM_EXIT_READ_CR3]			= cr_interception,
-	[SVM_EXIT_READ_CR4]			= cr_interception,
-	[SVM_EXIT_READ_CR8]			= cr_interception,
 	[SVM_EXIT_CR0_SEL_WRITE]		= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR0]			= cr_interception,
 	[SVM_EXIT_WRITE_CR3]			= cr_interception,
@@ -4151,11 +4124,9 @@ static const struct __x86_intercept {
 	u32 exit_code;
 	enum x86_intercept_stage stage;
 } x86_intercept_map[] = {
-	[x86_intercept_cr_read]		= POST_EX(SVM_EXIT_READ_CR0),
 	[x86_intercept_cr_write]	= POST_EX(SVM_EXIT_WRITE_CR0),
 	[x86_intercept_clts]		= POST_EX(SVM_EXIT_WRITE_CR0),
 	[x86_intercept_lmsw]		= POST_EX(SVM_EXIT_WRITE_CR0),
-	[x86_intercept_smsw]		= POST_EX(SVM_EXIT_READ_CR0),
 	[x86_intercept_dr_read]		= POST_EX(SVM_EXIT_READ_DR0),
 	[x86_intercept_dr_write]	= POST_EX(SVM_EXIT_WRITE_DR0),
 	[x86_intercept_sldt]		= POST_EX(SVM_EXIT_LDTR_READ),
@@ -4221,10 +4192,6 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		goto out;
 
 	switch (icpt_info.exit_code) {
-	case SVM_EXIT_READ_CR0:
-		if (info->intercept == x86_intercept_cr_read)
-			icpt_info.exit_code += info->modrm_reg;
-		break;
 	case SVM_EXIT_WRITE_CR0: {
 		unsigned long cr0, val;
 		u64 intercept;

Re: [PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Bandan Das]

Joel Schopp <joel.schopp@amd.com> writes:

> There isn't really a valid reason for kvm to intercept cr* reads
> on svm hardware.  The current kvm code just ends up returning
> the register
>
> Signed-off-by: Joel Schopp <joel.schopp@amd.com>
> ---
>  arch/x86/kvm/svm.c |   41 ++++-------------------------------------
>  1 file changed, 4 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index cc618c8..c3d10e6 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1090,9 +1090,6 @@ static void init_vmcb(struct vcpu_svm *svm)
>  	svm->vcpu.fpu_active = 1;
>  	svm->vcpu.arch.hflags = 0;
>  
> -	set_cr_intercept(svm, INTERCEPT_CR0_READ);
> -	set_cr_intercept(svm, INTERCEPT_CR3_READ);
> -	set_cr_intercept(svm, INTERCEPT_CR4_READ);
>  	set_cr_intercept(svm, INTERCEPT_CR0_WRITE);
>  	set_cr_intercept(svm, INTERCEPT_CR3_WRITE);
>  	set_cr_intercept(svm, INTERCEPT_CR4_WRITE);
> @@ -1174,7 +1171,6 @@ static void init_vmcb(struct vcpu_svm *svm)
>  		control->nested_ctl = 1;
>  		clr_intercept(svm, INTERCEPT_INVLPG);
>  		clr_exception_intercept(svm, PF_VECTOR);
> -		clr_cr_intercept(svm, INTERCEPT_CR3_READ);
>  		clr_cr_intercept(svm, INTERCEPT_CR3_WRITE);
>  		save->g_pat = 0x0007040600070406ULL;
>  		save->cr3 = 0;
> @@ -2968,29 +2964,10 @@ static int cr_interception(struct vcpu_svm *svm)
>  			kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>  			return 1;
>  		}
> -	} else { /* mov from cr */
> -		switch (cr) {
> -		case 0:
> -			val = kvm_read_cr0(&svm->vcpu);
> -			break;
> -		case 2:
> -			val = svm->vcpu.arch.cr2;
> -			break;
> -		case 3:
> -			val = kvm_read_cr3(&svm->vcpu);
> -			break;
> -		case 4:
> -			val = kvm_read_cr4(&svm->vcpu);
> -			break;
> -		case 8:
> -			val = kvm_get_cr8(&svm->vcpu);
> -			break;
> -		default:
> -			WARN(1, "unhandled read from CR%d", cr);
> -			kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> -			return 1;
> -		}
> -		kvm_register_write(&svm->vcpu, reg, val);
> +	} else { /* mov from cr, should never trap in svm */
> +		WARN(1, "unhandled read from CR%d", cr);
> +		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> +		return 1;

Can we end up here if a nested hypervisor sets cr read interception ?

Bandan

>  	}
>  	kvm_complete_insn_gp(&svm->vcpu, err);
>  
> @@ -3321,10 +3298,6 @@ static int mwait_interception(struct vcpu_svm *svm)
>  }
>  
>  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
> -	[SVM_EXIT_READ_CR0]			= cr_interception,
> -	[SVM_EXIT_READ_CR3]			= cr_interception,
> -	[SVM_EXIT_READ_CR4]			= cr_interception,
> -	[SVM_EXIT_READ_CR8]			= cr_interception,
>  	[SVM_EXIT_CR0_SEL_WRITE]		= emulate_on_interception,
>  	[SVM_EXIT_WRITE_CR0]			= cr_interception,
>  	[SVM_EXIT_WRITE_CR3]			= cr_interception,
> @@ -4151,11 +4124,9 @@ static const struct __x86_intercept {
>  	u32 exit_code;
>  	enum x86_intercept_stage stage;
>  } x86_intercept_map[] = {
> -	[x86_intercept_cr_read]		= POST_EX(SVM_EXIT_READ_CR0),
>  	[x86_intercept_cr_write]	= POST_EX(SVM_EXIT_WRITE_CR0),
>  	[x86_intercept_clts]		= POST_EX(SVM_EXIT_WRITE_CR0),
>  	[x86_intercept_lmsw]		= POST_EX(SVM_EXIT_WRITE_CR0),
> -	[x86_intercept_smsw]		= POST_EX(SVM_EXIT_READ_CR0),
>  	[x86_intercept_dr_read]		= POST_EX(SVM_EXIT_READ_DR0),
>  	[x86_intercept_dr_write]	= POST_EX(SVM_EXIT_WRITE_DR0),
>  	[x86_intercept_sldt]		= POST_EX(SVM_EXIT_LDTR_READ),
> @@ -4221,10 +4192,6 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
>  		goto out;
>  
>  	switch (icpt_info.exit_code) {
> -	case SVM_EXIT_READ_CR0:
> -		if (info->intercept == x86_intercept_cr_read)
> -			icpt_info.exit_code += info->modrm_reg;
> -		break;
>  	case SVM_EXIT_WRITE_CR0: {
>  		unsigned long cr0, val;
>  		u64 intercept;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Radim Krčmář]

2015-03-12 15:17-0500, Joel Schopp:
> There isn't really a valid reason for kvm to intercept cr* reads
> on svm hardware.  The current kvm code just ends up returning
> the register

There is no need to intercept CR* if the value that the guest should see
is equal to what we set there, but that is not always the case:
- CR0 might differ from what the guest should see because of lazy fpu
- CR3 isn't intercepted with nested paging and it should differ
  otherwise
- CR4 contains PAE bit when run without nested paging

CR2 and CR8 already aren't intercepted, so it looks like only CR0 and
CR4 could use some optimizations.

Re: [PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Joel Schopp]

On 03/12/2015 04:20 PM, Radim Krčmář wrote:
> 2015-03-12 15:17-0500, Joel Schopp:
>> There isn't really a valid reason for kvm to intercept cr* reads
>> on svm hardware.  The current kvm code just ends up returning
>> the register
> There is no need to intercept CR* if the value that the guest should see
> is equal to what we set there, but that is not always the case:
> - CR0 might differ from what the guest should see because of lazy fpu
Based on our previous conversations I understand why we have to trap the
write to the CR0 ts bit for lazy fpu, but don't understand why that
should affect a read.  I'll take another look at the code to see what
I'm missing.  You are probably correct in which case I'll modify the
patch to only turn off the read intercepts when lazy fpu isn't active.

> - CR3 isn't intercepted with nested paging and it should differ
>   otherwise
> - CR4 contains PAE bit when run without nested paging
>
> CR2 and CR8 already aren't intercepted, so it looks like only CR0 and
> CR4 could use some optimizations.
I'll send out a v2 with these less aggressive optimizations.

Re: [PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Radim Krčmář]

2015-03-16 11:16-0500, Joel Schopp:
> On 03/12/2015 04:20 PM, Radim Krčmář wrote:
> > 2015-03-12 15:17-0500, Joel Schopp:
> >> There isn't really a valid reason for kvm to intercept cr* reads
> >> on svm hardware.  The current kvm code just ends up returning
> >> the register
> > There is no need to intercept CR* if the value that the guest should see
> > is equal to what we set there, but that is not always the case:
> > - CR0 might differ from what the guest should see because of lazy fpu
> Based on our previous conversations I understand why we have to trap the
> write to the CR0 ts bit for lazy fpu, but don't understand why that
> should affect a read.

KVM keeps one CR0 with guest's state (svm.vcpu.arch.cr0) and a second
one that is loaded to hardware CR0 on VMRUN (svm.vmcb->save.cr0);
these two might not match.
If we didn't intercept read, it would return hardware CR0, so the guest
could do CLTS (change svm.vcpu.arch.cr0) and read CR0.TS = 1, because of
lazy FPU.

Correct emulation is what we want.

> > CR2 and CR8 already aren't intercepted, so it looks like only CR0 and
> > CR4 could use some optimizations.
> I'll send out a v2 with these less aggressive optimizations.

Thanks.

Re: [PATCH] kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

[Radim Krčmář]

2015-03-12 17:17-0400, Bandan Das:
> Joel Schopp <joel.schopp@amd.com> writes:
> > @@ -2968,29 +2964,10 @@ static int cr_interception(struct vcpu_svm *svm)
> >  			kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> >  			return 1;
> >  		}
> > -	} else { /* mov from cr */
> > -		[reads of CR 0..8]
> > +	} else { /* mov from cr, should never trap in svm */
> > +		WARN(1, "unhandled read from CR%d", cr);
> > +		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> > +		return 1;
> 
> Can we end up here if a nested hypervisor sets cr read interception ?

No.  If the nested hypervisor sets intercept bits, we're going to detect
them in 'handle_exit -> nested_svm_exit_handled -> nested_svm_intercept'
and enter L1 before the cr_interception handler.