| Message ID | 845737d3-e16e-61d7-7733-0f8b9eddfb45@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM: correct CPUID event processing | expand |
On 9/19/19 6:37 AM, Jan Beulich wrote: > hvm_monitor_cpuid() expects the input registers, not two of the outputs. > > However, once having made the necessary adjustment, the SVM and VMX > functions are so similar that they should be folded (thus avoiding > further similar asymmetries to get introduced). Use the best of both > worlds by e.g. using "curr" consistently. This then being the only > caller of hvm_check_cpuid_faulting(), fold in that function as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
On 9/19/19 11:07 PM, Boris Ostrovsky wrote: > On 9/19/19 6:37 AM, Jan Beulich wrote: >> hvm_monitor_cpuid() expects the input registers, not two of the outputs. >> >> However, once having made the necessary adjustment, the SVM and VMX >> functions are so similar that they should be folded (thus avoiding >> further similar asymmetries to get introduced). Use the best of both >> worlds by e.g. using "curr" consistently. This then being the only >> caller of hvm_check_cpuid_faulting(), fold in that function as well. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Thanks, Razvan
On 19.09.2019 13:37, Jan Beulich wrote: > hvm_monitor_cpuid() expects the input registers, not two of the outputs. > > However, once having made the necessary adjustment, the SVM and VMX > functions are so similar that they should be folded (thus avoiding > further similar asymmetries to get introduced). Use the best of both > worlds by e.g. using "curr" consistently. This then being the only > caller of hvm_check_cpuid_faulting(), fold in that function as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3349,14 +3349,28 @@ unsigned long copy_from_user_hvm(void *t > return rc ? len : 0; /* fake a copy_from_user() return code */ > } > Useful fold. Maybe a small comment with the reason to have one function would help. Something like both SVM and VMX do the same thing, but it's up to you if it is necessary. > -bool hvm_check_cpuid_faulting(struct vcpu *v) > +int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) Alex
On 20.09.2019 10:39, Alexandru Stefan ISAILA wrote: > > > On 19.09.2019 13:37, Jan Beulich wrote: >> hvm_monitor_cpuid() expects the input registers, not two of the outputs. >> >> However, once having made the necessary adjustment, the SVM and VMX >> functions are so similar that they should be folded (thus avoiding >> further similar asymmetries to get introduced). Use the best of both >> worlds by e.g. using "curr" consistently. This then being the only >> caller of hvm_check_cpuid_faulting(), fold in that function as well. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com> Thanks. >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -3349,14 +3349,28 @@ unsigned long copy_from_user_hvm(void *t >> return rc ? len : 0; /* fake a copy_from_user() return code */ >> } >> > > Useful fold. Maybe a small comment with the reason to have one function > would help. Something like both SVM and VMX do the same thing, but it's > up to you if it is necessary. > >> -bool hvm_check_cpuid_faulting(struct vcpu *v) >> +int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) The patch description explicitly says so. Since having common code for things not differing between vendors is the rule rather than an exception, commenting individual functions to this effect would seem rather odd to me. Jan
On 19/09/2019 11:37, Jan Beulich wrote: > hvm_monitor_cpuid() expects the input registers, not two of the outputs. Perhaps worth nothing that this has been broken since its introduction in c/s d05f1eb3741b85 ? > > However, once having made the necessary adjustment, the SVM and VMX > functions are so similar that they should be folded (thus avoiding > further similar asymmetries to get introduced). Use the best of both > worlds by e.g. using "curr" consistently. This then being the only > caller of hvm_check_cpuid_faulting(), fold in that function as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 20.09.2019 10:50, Andrew Cooper wrote: > On 19/09/2019 11:37, Jan Beulich wrote: >> hvm_monitor_cpuid() expects the input registers, not two of the outputs. > > Perhaps worth nothing that this has been broken since its introduction > in c/s d05f1eb3741b85 ? Ah, yes, I should have done this. I've added half a sentence. >> However, once having made the necessary adjustment, the SVM and VMX >> functions are so similar that they should be folded (thus avoiding >> further similar asymmetries to get introduced). Use the best of both >> worlds by e.g. using "curr" consistently. This then being the only >> caller of hvm_check_cpuid_faulting(), fold in that function as well. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. Jan
On 19.09.19 12:37, Jan Beulich wrote: > hvm_monitor_cpuid() expects the input registers, not two of the outputs. > > However, once having made the necessary adjustment, the SVM and VMX > functions are so similar that they should be folded (thus avoiding > further similar asymmetries to get introduced). Use the best of both > worlds by e.g. using "curr" consistently. This then being the only > caller of hvm_check_cpuid_faulting(), fold in that function as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
> From: Jan Beulich [mailto:jbeulich@suse.com] > Sent: Thursday, September 19, 2019 6:38 PM > > hvm_monitor_cpuid() expects the input registers, not two of the outputs. > > However, once having made the necessary adjustment, the SVM and VMX > functions are so similar that they should be folded (thus avoiding > further similar asymmetries to get introduced). Use the best of both > worlds by e.g. using "curr" consistently. This then being the only > caller of hvm_check_cpuid_faulting(), fold in that function as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3349,14 +3349,28 @@ unsigned long copy_from_user_hvm(void *t return rc ? len : 0; /* fake a copy_from_user() return code */ } -bool hvm_check_cpuid_faulting(struct vcpu *v) +int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) { - const struct vcpu_msrs *msrs = v->arch.msrs; + struct vcpu *curr = current; + unsigned int leaf = regs->eax, subleaf = regs->ecx; + struct cpuid_leaf res; - if ( !msrs->misc_features_enables.cpuid_faulting ) - return false; + if ( curr->arch.msrs->misc_features_enables.cpuid_faulting && + hvm_get_cpl(curr) > 0 ) + { + hvm_inject_hw_exception(TRAP_gp_fault, 0); + return 1; /* Don't advance the guest IP! */ + } - return hvm_get_cpl(v) > 0; + guest_cpuid(curr, leaf, subleaf, &res); + HVMTRACE_6D(CPUID, leaf, subleaf, res.a, res.b, res.c, res.d); + + regs->rax = res.a; + regs->rbx = res.b; + regs->rcx = res.c; + regs->rdx = res.d; + + return hvm_monitor_cpuid(inst_len, leaf, subleaf); } static uint64_t _hvm_rdtsc_intercept(void) --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1784,28 +1784,6 @@ static void svm_fpu_dirty_intercept(void vmcb_set_cr0(vmcb, vmcb_get_cr0(vmcb) & ~X86_CR0_TS); } -static int svm_vmexit_do_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) -{ - struct vcpu *curr = current; - struct cpuid_leaf res; - - if ( hvm_check_cpuid_faulting(curr) ) - { - hvm_inject_hw_exception(TRAP_gp_fault, 0); - return 1; /* Don't advance the guest IP! */ - } - - guest_cpuid(curr, regs->eax, regs->ecx, &res); - HVMTRACE_5D(CPUID, regs->eax, res.a, res.b, res.c, res.d); - - regs->rax = res.a; - regs->rbx = res.b; - regs->rcx = res.c; - regs->rdx = res.d; - - return hvm_monitor_cpuid(inst_len, regs->eax, regs->ecx); -} - static void svm_vmexit_do_cr_access( struct vmcb_struct *vmcb, struct cpu_user_regs *regs) { @@ -2828,7 +2806,7 @@ void svm_vmexit_handler(struct cpu_user_ if ( inst_len == 0 ) break; - rc = svm_vmexit_do_cpuid(regs, inst_len); + rc = hvm_vmexit_cpuid(regs, inst_len); if ( rc < 0 ) goto unexpected_exit_type; --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2489,29 +2389,6 @@ static void vmx_fpu_dirty_intercept(void } } -static int vmx_do_cpuid(struct cpu_user_regs *regs) -{ - struct vcpu *curr = current; - uint32_t leaf = regs->eax, subleaf = regs->ecx; - struct cpuid_leaf res; - - if ( hvm_check_cpuid_faulting(current) ) - { - hvm_inject_hw_exception(TRAP_gp_fault, 0); - return 1; /* Don't advance the guest IP! */ - } - - guest_cpuid(curr, leaf, subleaf, &res); - HVMTRACE_5D(CPUID, leaf, res.a, res.b, res.c, res.d); - - regs->rax = res.a; - regs->rbx = res.b; - regs->rcx = res.c; - regs->rdx = res.d; - - return hvm_monitor_cpuid(get_instruction_length(), leaf, subleaf); -} - static void vmx_dr_access(unsigned long exit_qualification, struct cpu_user_regs *regs) { @@ -3862,7 +3839,7 @@ void vmx_vmexit_handler(struct cpu_user_ } case EXIT_REASON_CPUID: { - int rc = vmx_do_cpuid(regs); + int rc = hvm_vmexit_cpuid(regs, get_instruction_length()); /* * rc < 0 error in monitor/vm_event, crash --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -280,7 +280,7 @@ void hvm_set_segment_register(struct vcp bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val); -bool hvm_check_cpuid_faulting(struct vcpu *v); +int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len); void hvm_migrate_timers(struct vcpu *v); void hvm_do_resume(struct vcpu *v); void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v);
hvm_monitor_cpuid() expects the input registers, not two of the outputs. However, once having made the necessary adjustment, the SVM and VMX functions are so similar that they should be folded (thus avoiding further similar asymmetries to get introduced). Use the best of both worlds by e.g. using "curr" consistently. This then being the only caller of hvm_check_cpuid_faulting(), fold in that function as well. Signed-off-by: Jan Beulich <jbeulich@suse.com>