| Message ID | 1480932571-23547-6-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v) > > if ( w->do_write.msr ) > { > - hvm_msr_write_intercept(w->msr, w->value, 0); > + int rc = hvm_msr_write_intercept(w->msr, w->value, 0); > + > + if ( rc == X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); The use of a local variable looks kind of pointless here. > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > return X86EMUL_OKAY; > > gpf: > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > return X86EMUL_EXCEPTION; > } > > @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) > return result; > > gpf: > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > return X86EMUL_EXCEPTION; > } In cases like these it would certainly be nice to get rid of the now rather pointless goto-s, but of course we can equally well do this in a later patch. > @@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs) > > if ( rc == X86EMUL_OKAY ) > __update_guest_eip(regs, inst_len); > + else if ( rc == X86EMUL_EXCEPTION ) > + hvm_inject_hw_exception(TRAP_gp_fault, 0); else ASSERT_UNREACHABLE(); ? (And then similarly for VMX.) > +/* > + * May return X86EMUL_EXCEPTION, at which point the caller is responsible for > + * injecting a #GP fault. Used to support speculative reads. > + */ > +int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content); > +int hvm_msr_write_intercept( > + unsigned int msr, uint64_t msr_content, bool_t may_defer); Please add __must_check to both. With at least this one taken care of Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 05/12/16 12:10, Jan Beulich wrote: >>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v) >> >> if ( w->do_write.msr ) >> { >> - hvm_msr_write_intercept(w->msr, w->value, 0); >> + int rc = hvm_msr_write_intercept(w->msr, w->value, 0); >> + >> + if ( rc == X86EMUL_EXCEPTION ) >> + hvm_inject_hw_exception(TRAP_gp_fault, 0); > The use of a local variable looks kind of pointless here. The first version had if ( hvm_msr_write_intercept(w->msr, w->value, 0) == X86EMUL_EXCEPTION ) but this looked rather ugly to read. I prefer the version as submitted, but am not too fussed if you insist for the latter? > >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) >> return X86EMUL_OKAY; >> >> gpf: >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> return X86EMUL_EXCEPTION; >> } >> >> @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) >> return result; >> >> gpf: >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> return X86EMUL_EXCEPTION; >> } > In cases like these it would certainly be nice to get rid of the now > rather pointless goto-s, but of course we can equally well do this > in a later patch. I will do a cleanup patch and add it to v2. ~Andrew
>>> On 05.12.16 at 17:29, <andrew.cooper3@citrix.com> wrote: > On 05/12/16 12:10, Jan Beulich wrote: >>>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v) >>> >>> if ( w->do_write.msr ) >>> { >>> - hvm_msr_write_intercept(w->msr, w->value, 0); >>> + int rc = hvm_msr_write_intercept(w->msr, w->value, 0); >>> + >>> + if ( rc == X86EMUL_EXCEPTION ) >>> + hvm_inject_hw_exception(TRAP_gp_fault, 0); >> The use of a local variable looks kind of pointless here. > > The first version had > > if ( hvm_msr_write_intercept(w->msr, w->value, 0) == > X86EMUL_EXCEPTION ) > > but this looked rather ugly to read. I prefer the version as submitted, > but am not too fussed if you insist for the latter? I won't insist, it was just a suggestion to make the code look better to my eyes. If you like it better as is, keep it. Jan
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: Monday, December 05, 2016 6:09 PM > > The current hvm_msr_{read,write}_intercept() infrastructure calls > hvm_inject_hw_exception() directly to latch a fault, and returns > X86EMUL_EXCEPTION to its caller. > > This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the > fault is raised behind the back of the x86 emulator. > > Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns > X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index d0a043b..b182d57 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1531,7 +1531,12 @@ static int hvmemul_read_msr( uint64_t *val, struct x86_emulate_ctxt *ctxt) { - return hvm_msr_read_intercept(reg, val); + int rc = hvm_msr_read_intercept(reg, val); + + if ( rc == X86EMUL_EXCEPTION ) + x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt); + + return rc; } static int hvmemul_write_msr( @@ -1539,7 +1544,12 @@ static int hvmemul_write_msr( uint64_t val, struct x86_emulate_ctxt *ctxt) { - return hvm_msr_write_intercept(reg, val, 1); + int rc = hvm_msr_write_intercept(reg, val, 1); + + if ( rc == X86EMUL_EXCEPTION ) + x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt); + + return rc; } static int hvmemul_wbinvd( diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index ac207e4..863adfc 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v) if ( w->do_write.msr ) { - hvm_msr_write_intercept(w->msr, w->value, 0); + int rc = hvm_msr_write_intercept(w->msr, w->value, 0); + + if ( rc == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + w->do_write.msr = 0; } @@ -3896,7 +3900,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) return ret; gp_fault: - hvm_inject_hw_exception(TRAP_gp_fault, 0); ret = X86EMUL_EXCEPTION; *msr_content = -1ull; goto out; @@ -4054,7 +4057,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, return ret; gp_fault: - hvm_inject_hw_exception(TRAP_gp_fault, 0); return X86EMUL_EXCEPTION; } diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 1588b2f..810b0d4 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) return X86EMUL_OKAY; gpf: - hvm_inject_hw_exception(TRAP_gp_fault, 0); return X86EMUL_EXCEPTION; } @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) return result; gpf: - hvm_inject_hw_exception(TRAP_gp_fault, 0); return X86EMUL_EXCEPTION; } @@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs) if ( rc == X86EMUL_OKAY ) __update_guest_eip(regs, inst_len); + else if ( rc == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); } static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb, diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index afde634..ddfb410 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2691,7 +2691,6 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content) return X86EMUL_OKAY; gp_fault: - hvm_inject_hw_exception(TRAP_gp_fault, 0); return X86EMUL_EXCEPTION; } @@ -2920,7 +2919,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) return X86EMUL_OKAY; gp_fault: - hvm_inject_hw_exception(TRAP_gp_fault, 0); return X86EMUL_EXCEPTION; } @@ -3632,23 +3630,35 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; case EXIT_REASON_MSR_READ: { - uint64_t msr_content; - if ( hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY ) + uint64_t msr_content = 0; + + switch ( hvm_msr_read_intercept(regs->_ecx, &msr_content) ) { - regs->eax = (uint32_t)msr_content; - regs->edx = (uint32_t)(msr_content >> 32); + case X86EMUL_OKAY: + regs->rax = (uint32_t)msr_content; + regs->rdx = (uint32_t)(msr_content >> 32); update_guest_eip(); /* Safe: RDMSR */ + break; + + case X86EMUL_EXCEPTION: + hvm_inject_hw_exception(TRAP_gp_fault, 0); + break; } break; } case EXIT_REASON_MSR_WRITE: - { - uint64_t msr_content; - msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax; - if ( hvm_msr_write_intercept(regs->ecx, msr_content, 1) == X86EMUL_OKAY ) + switch ( hvm_msr_write_intercept( + regs->_ecx, (regs->rdx << 32) | regs->_eax, 1) ) + { + case X86EMUL_OKAY: update_guest_eip(); /* Safe: WRMSR */ + break; + + case X86EMUL_EXCEPTION: + hvm_inject_hw_exception(TRAP_gp_fault, 0); + break; + } break; - } case EXIT_REASON_VMXOFF: if ( nvmx_handle_vmxoff(regs) == X86EMUL_OKAY ) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index e6e9ebd..87f02ef 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1000,6 +1000,7 @@ static void load_shadow_guest_state(struct vcpu *v) struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); u32 control; u64 cr_gh_mask, cr_read_shadow; + int rc; static const u16 vmentry_fields[] = { VM_ENTRY_INTR_INFO, @@ -1021,8 +1022,12 @@ static void load_shadow_guest_state(struct vcpu *v) if ( control & VM_ENTRY_LOAD_GUEST_PAT ) hvm_set_guest_pat(v, get_vvmcs(v, GUEST_PAT)); if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL ) - hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, - get_vvmcs(v, GUEST_PERF_GLOBAL_CTRL), 0); + { + rc = hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, + get_vvmcs(v, GUEST_PERF_GLOBAL_CTRL), 0); + if ( rc == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + } hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0); @@ -1193,7 +1198,7 @@ static void sync_vvmcs_ro(struct vcpu *v) static void load_vvmcs_host_state(struct vcpu *v) { - int i; + int i, rc; u64 r; u32 control; @@ -1211,8 +1216,12 @@ static void load_vvmcs_host_state(struct vcpu *v) if ( control & VM_EXIT_LOAD_HOST_PAT ) hvm_set_guest_pat(v, get_vvmcs(v, HOST_PAT)); if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL ) - hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, - get_vvmcs(v, HOST_PERF_GLOBAL_CTRL), 1); + { + rc = hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, + get_vvmcs(v, HOST_PERF_GLOBAL_CTRL), 1); + if ( rc == X86EMUL_EXCEPTION ) + hvm_inject_hw_exception(TRAP_gp_fault, 0); + } hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0); diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h index 3d767d7..2bff1f4 100644 --- a/xen/include/asm-x86/hvm/support.h +++ b/xen/include/asm-x86/hvm/support.h @@ -122,13 +122,18 @@ int hvm_set_efer(uint64_t value); int hvm_set_cr0(unsigned long value, bool_t may_defer); int hvm_set_cr3(unsigned long value, bool_t may_defer); int hvm_set_cr4(unsigned long value, bool_t may_defer); -int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content); -int hvm_msr_write_intercept( - unsigned int msr, uint64_t msr_content, bool_t may_defer); int hvm_mov_to_cr(unsigned int cr, unsigned int gpr); int hvm_mov_from_cr(unsigned int cr, unsigned int gpr); void hvm_ud_intercept(struct cpu_user_regs *); +/* + * May return X86EMUL_EXCEPTION, at which point the caller is responsible for + * injecting a #GP fault. Used to support speculative reads. + */ +int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content); +int hvm_msr_write_intercept( + unsigned int msr, uint64_t msr_content, bool_t may_defer); + #endif /* __ASM_X86_HVM_SUPPORT_H__ */ /*
The current hvm_msr_{read,write}_intercept() infrastructure calls hvm_inject_hw_exception() directly to latch a fault, and returns X86EMUL_EXCEPTION to its caller. This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the fault is raised behind the back of the x86 emulator. Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- xen/arch/x86/hvm/emulate.c | 14 ++++++++++++-- xen/arch/x86/hvm/hvm.c | 8 +++++--- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 32 +++++++++++++++++++++----------- xen/arch/x86/hvm/vmx/vvmx.c | 19 ++++++++++++++----- xen/include/asm-x86/hvm/support.h | 11 ++++++++--- 6 files changed, 62 insertions(+), 26 deletions(-)