Skip to contentSkip to conversation

kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

PATCH In review Joel Schopp 3 participants 6 messages Mar 12, 2015 – Apr 3, 2015

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

Joel Schopp

author
#0 lore raw

PATCH 1/1 kvm: x86: svm: remove SVM_EXIT_READ_CR* intercepts

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 <[email protected]>
---
 arch/x86/kvm/svm.c |   41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)
arch/x86/kvm/svm.c +4 -37
index cc618c8..c3d10e6 100644
@@ -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;
#1 lore raw
Quoted 52 lines · 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 <[email protected]>
---
 arch/x86/kvm/svm.c |   41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)
arch/x86/kvm/svm.c +0 -21
index cc618c8..c3d10e6 100644
@@ -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;

arch/x86/kvm/svm.c static int cr_interception(struct vcpu_svm *svm) · line 2970

Quoted 10 lines
arch/x86/kvm/svm.c +4 -6
-		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

Quoted 42 lines
arch/x86/kvm/svm.c +0 -11
 	}
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw

2015-03-12 17:17-0400, Bandan Das:

Quoted 13 lines

arch/x86/kvm/svm.c static int cr_interception(struct vcpu_svm *svm) · line 2970

Quoted 10 lines · Joel Schopp
arch/x86/kvm/svm.c +4 -2
@@ -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.

#3 lore raw
In reply to Joel Schopp

2015-03-12 15:17-0500, Joel Schopp:

Quoted 3 lines

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.

author
#4 lore raw
Quoted 7 lines · Radim Krčmář

2015-03-12 15:17-0500, Joel Schopp:

Quoted 3 lines

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.

Quoted 6 lines

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

#5 lore raw

2015-03-16 11:16-0500, Joel Schopp:

Quoted 11 lines · Radim Krčmář
Quoted 7 lines

2015-03-12 15:17-0500, Joel Schopp:

Quoted 3 lines

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.

Quoted 3 lines
Quoted 2 lines

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.