| Message ID | 20150312201746.2953.11716.stgit@joelvm2.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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;
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(-) -- 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