| Message ID | 1480513841-7565-14-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 30 November 2016 13:51 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Tim > (Xen.org) <tim@xen.org>; George Dunlap <George.Dunlap@citrix.com> > Subject: [PATCH v3 13/24] x86/emul: Rework emulator event injection > > The emulator needs to gain an understanding of interrupts and exceptions > generated by its actions. > > Move hvm_emulate_ctxt.{exn_pending,trap} into struct x86_emulate_ctxt > so they > are visible to the emulator. This removes the need for the > inject_{hw_exception,sw_interrupt}() hooks, which are dropped and > replaced > with x86_emul_{hw_exception,software_event,reset_event}() instead. > > For exceptions raised by x86_emulate() itself (rather than its callbacks), the > shadow pagetable and PV uses of x86_emulate() previously failed with > X86EMUL_UNHANDLEABLE due to the lack of inject_*() hooks. > > This behaviour has changed, and such cases will now return > X86EMUL_EXCEPTION > with event_pending set. Until the callers of x86_emulate() have been > updated > to inject events back into the guest, divert the event_pending case back into > the X86EMUL_UNHANDLEABLE path to maintain the same guest-visible > behaviour. > > No overall functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > CC: Tim Deegan <tim@xen.org> > CC: George Dunlap <george.dunlap@eu.citrix.com> > > v3: > * Rework how the event_pending case is currently handled > v2: > * Change x86_emul_hw_exception()'s error_code parameter to being > signed > * Clarify how software interrupt injection happens. > * More ASSERT()'s and description of how event_pending works without the > inject_sw_interrupt() hook > --- > xen/arch/x86/hvm/emulate.c | 81 ++++------------------------------ > xen/arch/x86/hvm/hvm.c | 4 +- > xen/arch/x86/hvm/io.c | 4 +- > xen/arch/x86/hvm/vmx/realmode.c | 16 +++---- > xen/arch/x86/mm.c | 26 +++++++++++ > xen/arch/x86/mm/shadow/multi.c | 17 +++++++ > xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++-- > xen/arch/x86/x86_emulate/x86_emulate.h | 76 > +++++++++++++++++++++++++------ > xen/include/asm-x86/hvm/emulate.h | 3 -- > 9 files changed, 132 insertions(+), 107 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 91c79fa..4b8c9a0 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -568,12 +568,9 @@ static int hvmemul_virtual_to_linear( > return X86EMUL_UNHANDLEABLE; > > /* This is a singleton operation: fail it with an exception. */ > - hvmemul_ctxt->exn_pending = 1; > - hvmemul_ctxt->trap.vector = > - (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault; > - hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION; > - hvmemul_ctxt->trap.error_code = 0; > - hvmemul_ctxt->trap.insn_len = 0; > + x86_emul_hw_exception((seg == x86_seg_ss) > + ? TRAP_stack_error > + : TRAP_gp_fault, 0, &hvmemul_ctxt->ctxt); > return X86EMUL_EXCEPTION; > } > > @@ -1562,59 +1559,6 @@ int hvmemul_cpuid( > return X86EMUL_OKAY; > } > > -static int hvmemul_inject_hw_exception( > - uint8_t vector, > - int32_t error_code, > - struct x86_emulate_ctxt *ctxt) > -{ > - struct hvm_emulate_ctxt *hvmemul_ctxt = > - container_of(ctxt, struct hvm_emulate_ctxt, ctxt); > - > - hvmemul_ctxt->exn_pending = 1; > - hvmemul_ctxt->trap.vector = vector; > - hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION; > - hvmemul_ctxt->trap.error_code = error_code; > - hvmemul_ctxt->trap.insn_len = 0; > - > - return X86EMUL_OKAY; > -} > - > -static int hvmemul_inject_sw_interrupt( > - enum x86_swint_type type, > - uint8_t vector, > - uint8_t insn_len, > - struct x86_emulate_ctxt *ctxt) > -{ > - struct hvm_emulate_ctxt *hvmemul_ctxt = > - container_of(ctxt, struct hvm_emulate_ctxt, ctxt); > - > - switch ( type ) > - { > - case x86_swint_icebp: > - hvmemul_ctxt->trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION; > - break; > - > - case x86_swint_int3: > - case x86_swint_into: > - hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_EXCEPTION; > - break; > - > - case x86_swint_int: > - hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_INTERRUPT; > - break; > - > - default: > - return X86EMUL_UNHANDLEABLE; > - } > - > - hvmemul_ctxt->exn_pending = 1; > - hvmemul_ctxt->trap.vector = vector; > - hvmemul_ctxt->trap.error_code = X86_EVENT_NO_EC; > - hvmemul_ctxt->trap.insn_len = insn_len; > - > - return X86EMUL_OKAY; > -} > - > static int hvmemul_get_fpu( > void (*exception_callback)(void *, struct cpu_user_regs *), > void *exception_callback_arg, > @@ -1678,8 +1622,7 @@ static int hvmemul_invlpg( > * hvmemul_virtual_to_linear() raises exceptions for type/limit > * violations, so squash them. > */ > - hvmemul_ctxt->exn_pending = 0; > - hvmemul_ctxt->trap = (struct x86_event){}; > + x86_emul_reset_event(ctxt); > rc = X86EMUL_OKAY; > } > > @@ -1696,7 +1639,7 @@ static int hvmemul_vmfunc( > > rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs); > if ( rc != X86EMUL_OKAY ) > - hvmemul_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, > ctxt); > + x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt); > > return rc; > } > @@ -1720,8 +1663,6 @@ static const struct x86_emulate_ops > hvm_emulate_ops = { > .write_msr = hvmemul_write_msr, > .wbinvd = hvmemul_wbinvd, > .cpuid = hvmemul_cpuid, > - .inject_hw_exception = hvmemul_inject_hw_exception, > - .inject_sw_interrupt = hvmemul_inject_sw_interrupt, > .get_fpu = hvmemul_get_fpu, > .put_fpu = hvmemul_put_fpu, > .invlpg = hvmemul_invlpg, > @@ -1747,8 +1688,6 @@ static const struct x86_emulate_ops > hvm_emulate_ops_no_write = { > .write_msr = hvmemul_write_msr_discard, > .wbinvd = hvmemul_wbinvd_discard, > .cpuid = hvmemul_cpuid, > - .inject_hw_exception = hvmemul_inject_hw_exception, > - .inject_sw_interrupt = hvmemul_inject_sw_interrupt, > .get_fpu = hvmemul_get_fpu, > .put_fpu = hvmemul_put_fpu, > .invlpg = hvmemul_invlpg, > @@ -1870,8 +1809,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, > unsigned long gla) > hvm_dump_emulation_state(XENLOG_G_WARNING "MMCFG", &ctxt); > break; > case X86EMUL_EXCEPTION: > - if ( ctxt.exn_pending ) > - hvm_inject_event(&ctxt.trap); > + if ( ctxt.ctxt.event_pending ) > + hvm_inject_event(&ctxt.ctxt.event); > /* fallthrough */ > default: > hvm_emulate_writeback(&ctxt); > @@ -1930,8 +1869,8 @@ void hvm_emulate_one_vm_event(enum > emul_kind kind, unsigned int trapnr, > hvm_inject_hw_exception(trapnr, errcode); > break; > case X86EMUL_EXCEPTION: > - if ( ctx.exn_pending ) > - hvm_inject_event(&ctx.trap); > + if ( ctx.ctxt.event_pending ) > + hvm_inject_event(&ctx.ctxt.event); > break; > } > > @@ -2006,8 +1945,6 @@ void hvm_emulate_init_per_insn( > hvmemul_ctxt->insn_buf_bytes = insn_bytes; > memcpy(hvmemul_ctxt->insn_buf, insn_buf, insn_bytes); > } > - > - hvmemul_ctxt->exn_pending = 0; > } > > void hvm_emulate_writeback( > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index b950842..ef83100 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4076,8 +4076,8 @@ void hvm_ud_intercept(struct cpu_user_regs > *regs) > hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); > break; > case X86EMUL_EXCEPTION: > - if ( ctxt.exn_pending ) > - hvm_inject_event(&ctxt.trap); > + if ( ctxt.ctxt.event_pending ) > + hvm_inject_event(&ctxt.ctxt.event); > /* fall through */ > default: > hvm_emulate_writeback(&ctxt); > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c > index 1279f68..abb9d51 100644 > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -102,8 +102,8 @@ int handle_mmio(void) > hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt); > return 0; > case X86EMUL_EXCEPTION: > - if ( ctxt.exn_pending ) > - hvm_inject_event(&ctxt.trap); > + if ( ctxt.ctxt.event_pending ) > + hvm_inject_event(&ctxt.ctxt.event); > break; > default: > break; > diff --git a/xen/arch/x86/hvm/vmx/realmode.c > b/xen/arch/x86/hvm/vmx/realmode.c > index 9002638..dc3ab44 100644 > --- a/xen/arch/x86/hvm/vmx/realmode.c > +++ b/xen/arch/x86/hvm/vmx/realmode.c > @@ -122,7 +122,7 @@ void vmx_realmode_emulate_one(struct > hvm_emulate_ctxt *hvmemul_ctxt) > > if ( rc == X86EMUL_EXCEPTION ) > { > - if ( !hvmemul_ctxt->exn_pending ) > + if ( !hvmemul_ctxt->ctxt.event_pending ) > { > unsigned long intr_info; > > @@ -133,27 +133,27 @@ void vmx_realmode_emulate_one(struct > hvm_emulate_ctxt *hvmemul_ctxt) > gdprintk(XENLOG_ERR, "Exception pending but no info.\n"); > goto fail; > } > - hvmemul_ctxt->trap.vector = (uint8_t)intr_info; > - hvmemul_ctxt->trap.insn_len = 0; > + hvmemul_ctxt->ctxt.event.vector = (uint8_t)intr_info; > + hvmemul_ctxt->ctxt.event.insn_len = 0; > } > > if ( unlikely(curr->domain->debugger_attached) && > - ((hvmemul_ctxt->trap.vector == TRAP_debug) || > - (hvmemul_ctxt->trap.vector == TRAP_int3)) ) > + ((hvmemul_ctxt->ctxt.event.vector == TRAP_debug) || > + (hvmemul_ctxt->ctxt.event.vector == TRAP_int3)) ) > { > domain_pause_for_debugger(); > } > else if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE ) > { > gdprintk(XENLOG_ERR, "Exception %02x in protected mode.\n", > - hvmemul_ctxt->trap.vector); > + hvmemul_ctxt->ctxt.event.vector); > goto fail; > } > else > { > realmode_deliver_exception( > - hvmemul_ctxt->trap.vector, > - hvmemul_ctxt->trap.insn_len, > + hvmemul_ctxt->ctxt.event.vector, > + hvmemul_ctxt->ctxt.event.insn_len, > hvmemul_ctxt); > } > } > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 231c7bf..5d59479 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5379,6 +5379,19 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned > long addr, > page_unlock(page); > put_page(page); > > + /* > + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised > + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such > exceptions > + * now set event_pending instead. Exceptions raised behind the back of > + * the emulator don't yet set event_pending. > + * > + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE > path, > + * for no functional change from before. Future patches will fix this > + * properly. > + */ > + if ( rc == X86EMUL_EXCEPTION && ptwr_ctxt.ctxt.event_pending ) > + rc = X86EMUL_UNHANDLEABLE; > + > if ( rc == X86EMUL_UNHANDLEABLE ) > goto bail; > > @@ -5506,6 +5519,19 @@ int mmio_ro_do_page_fault(struct vcpu *v, > unsigned long addr, > else > rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops); > > + /* > + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised > + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such > exceptions > + * now set event_pending instead. Exceptions raised behind the back of > + * the emulator don't yet set event_pending. > + * > + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE > path, > + * for no functional change from before. Future patches will fix this > + * properly. > + */ > + if ( rc == X86EMUL_EXCEPTION && ctxt.event_pending ) > + rc = X86EMUL_UNHANDLEABLE; > + > if ( rc == X86EMUL_UNHANDLEABLE ) > return 0; > > diff --git a/xen/arch/x86/mm/shadow/multi.c > b/xen/arch/x86/mm/shadow/multi.c > index ddfb815..56c40f8 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3374,6 +3374,19 @@ static int sh_page_fault(struct vcpu *v, > r = x86_emulate(&emul_ctxt.ctxt, emul_ops); > > /* > + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised > + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such > exceptions > + * now set event_pending instead. Exceptions raised behind the back of > + * the emulator don't yet set event_pending. > + * > + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE > path, > + * for no functional change from before. Future patches will fix this > + * properly. > + */ > + if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) > + r = X86EMUL_UNHANDLEABLE; > + > + /* > * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it > * would be a good unshadow hint. If we *do* decide to unshadow-on- > fault > * then it must be 'failable': we cannot require the unshadow to succeed. > @@ -3443,6 +3456,10 @@ static int sh_page_fault(struct vcpu *v, > shadow_continue_emulation(&emul_ctxt, regs); > v->arch.paging.last_write_was_pt = 0; > r = x86_emulate(&emul_ctxt.ctxt, emul_ops); > + > + if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) > + r = X86EMUL_UNHANDLEABLE; > + > if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) > { > emulation_count++; > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c > b/xen/arch/x86/x86_emulate/x86_emulate.c > index 6adfdbe..0fb2c09 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -680,9 +680,8 @@ static inline int mkec(uint8_t e, int32_t ec, ...) > > #define generate_exception_if(p, e, ec...) \ > ({ if ( (p) ) { \ > - fail_if(ops->inject_hw_exception == NULL); \ > - rc = ops->inject_hw_exception(e, mkec(e, ##ec, 0), ctxt) \ > - ? : X86EMUL_EXCEPTION; \ > + x86_emul_hw_exception(e, mkec(e, ##ec, 0), ctxt); \ > + rc = X86EMUL_EXCEPTION; \ > goto done; \ > } \ > }) > @@ -1604,9 +1603,6 @@ static int inject_swint(enum x86_swint_type type, > { > int rc, error_code, fault_type = EXC_GP; > > - fail_if(ops->inject_sw_interrupt == NULL); > - fail_if(ops->inject_hw_exception == NULL); > - > /* > * Without hardware support, injecting software interrupts/exceptions is > * problematic. > @@ -1701,7 +1697,8 @@ static int inject_swint(enum x86_swint_type type, > } > } > > - rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); > + x86_emul_software_event(type, vector, insn_len, ctxt); > + rc = X86EMUL_OKAY; > > done: > return rc; > @@ -1909,6 +1906,7 @@ x86_decode( > > /* Initialise output state in x86_emulate_ctxt */ > ctxt->retire.raw = 0; > + x86_emul_reset_event(ctxt); > > op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt- > >addr_size/8; > if ( op_bytes == 8 ) > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h > b/xen/arch/x86/x86_emulate/x86_emulate.h > index da8924b..3c0b25d 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -396,19 +396,6 @@ struct x86_emulate_ops > unsigned int *edx, > struct x86_emulate_ctxt *ctxt); > > - /* inject_hw_exception */ > - int (*inject_hw_exception)( > - uint8_t vector, > - int32_t error_code, > - struct x86_emulate_ctxt *ctxt); > - > - /* inject_sw_interrupt */ > - int (*inject_sw_interrupt)( > - enum x86_swint_type type, > - uint8_t vector, > - uint8_t insn_len, > - struct x86_emulate_ctxt *ctxt); > - > /* > * get_fpu: Load emulated environment's FPU state onto processor. > * @exn_callback: On any FPU or SIMD exception, pass control to > @@ -486,6 +473,9 @@ struct x86_emulate_ctxt > bool singlestep:1; /* Singlestepping was active. */ > }; > } retire; > + > + bool event_pending; > + struct x86_event event; > }; > > /* > @@ -584,6 +574,19 @@ static inline int x86_emulate_wrapper( > if ( rc == X86EMUL_EXCEPTION ) > ASSERT(ctxt->regs->eip == orig_eip); > > + /* > + * TODO: Make this true: > + * > + ASSERT(ctxt->event_pending == (rc == X86EMUL_EXCEPTION)); > + * > + * Some codepaths still raise exceptions behind the back of the > + * emulator. (i.e. return X86EMUL_EXCEPTION but without > + * event_pending being set). In the meantime, use a slightly > + * relaxed check... > + */ > + if ( ctxt->event_pending ) > + ASSERT(rc == X86EMUL_EXCEPTION); > + > return rc; > } > > @@ -633,4 +636,51 @@ void x86_emulate_free_state(struct > x86_emulate_state *state); > > #endif > > +static inline void x86_emul_hw_exception( > + unsigned int vector, int error_code, struct x86_emulate_ctxt *ctxt) > +{ > + ASSERT(!ctxt->event_pending); > + > + ctxt->event.vector = vector; > + ctxt->event.type = X86_EVENTTYPE_HW_EXCEPTION; > + ctxt->event.error_code = error_code; > + > + ctxt->event_pending = true; > +} > + > +static inline void x86_emul_software_event( > + enum x86_swint_type type, uint8_t vector, uint8_t insn_len, > + struct x86_emulate_ctxt *ctxt) > +{ > + ASSERT(!ctxt->event_pending); > + > + switch ( type ) > + { > + case x86_swint_icebp: > + ctxt->event.type = X86_EVENTTYPE_PRI_SW_EXCEPTION; > + break; > + > + case x86_swint_int3: > + case x86_swint_into: > + ctxt->event.type = X86_EVENTTYPE_SW_EXCEPTION; > + break; > + > + case x86_swint_int: > + ctxt->event.type = X86_EVENTTYPE_SW_INTERRUPT; > + break; > + } > + > + ctxt->event.vector = vector; > + ctxt->event.error_code = X86_EVENT_NO_EC; > + ctxt->event.insn_len = insn_len; > + > + ctxt->event_pending = true; > +} > + > +static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt) > +{ > + ctxt->event_pending = false; > + ctxt->event = (struct x86_event){}; > +} > + > #endif /* __X86_EMULATE_H__ */ > diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm- > x86/hvm/emulate.h > index 3b7ec33..d64d834 100644 > --- a/xen/include/asm-x86/hvm/emulate.h > +++ b/xen/include/asm-x86/hvm/emulate.h > @@ -29,9 +29,6 @@ struct hvm_emulate_ctxt { > unsigned long seg_reg_accessed; > unsigned long seg_reg_dirty; > > - bool_t exn_pending; > - struct x86_event trap; > - > uint32_t intr_shadow; > > bool_t set_context; > -- > 2.1.4
At 13:50 +0000 on 30 Nov (1480513830), Andrew Cooper wrote: > The emulator needs to gain an understanding of interrupts and exceptions > generated by its actions. > > Move hvm_emulate_ctxt.{exn_pending,trap} into struct x86_emulate_ctxt so they > are visible to the emulator. This removes the need for the > inject_{hw_exception,sw_interrupt}() hooks, which are dropped and replaced > with x86_emul_{hw_exception,software_event,reset_event}() instead. > > For exceptions raised by x86_emulate() itself (rather than its callbacks), the > shadow pagetable and PV uses of x86_emulate() previously failed with > X86EMUL_UNHANDLEABLE due to the lack of inject_*() hooks. > > This behaviour has changed, and such cases will now return X86EMUL_EXCEPTION > with event_pending set. Until the callers of x86_emulate() have been updated > to inject events back into the guest, divert the event_pending case back into > the X86EMUL_UNHANDLEABLE path to maintain the same guest-visible behaviour. > > No overall functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Acked-by: Tim Deegan <tim@xen.org>
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > The emulator needs to gain an understanding of interrupts and exceptions > generated by its actions. > > Move hvm_emulate_ctxt.{exn_pending,trap} into struct x86_emulate_ctxt so they > are visible to the emulator. This removes the need for the > inject_{hw_exception,sw_interrupt}() hooks, which are dropped and replaced > with x86_emul_{hw_exception,software_event,reset_event}() instead. > > For exceptions raised by x86_emulate() itself (rather than its callbacks), the > shadow pagetable and PV uses of x86_emulate() previously failed with > X86EMUL_UNHANDLEABLE due to the lack of inject_*() hooks. > > This behaviour has changed, and such cases will now return X86EMUL_EXCEPTION > with event_pending set. Until the callers of x86_emulate() have been updated > to inject events back into the guest, divert the event_pending case back into > the X86EMUL_UNHANDLEABLE path to maintain the same guest-visible behaviour. > > No overall functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 91c79fa..4b8c9a0 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -568,12 +568,9 @@ static int hvmemul_virtual_to_linear( return X86EMUL_UNHANDLEABLE; /* This is a singleton operation: fail it with an exception. */ - hvmemul_ctxt->exn_pending = 1; - hvmemul_ctxt->trap.vector = - (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault; - hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION; - hvmemul_ctxt->trap.error_code = 0; - hvmemul_ctxt->trap.insn_len = 0; + x86_emul_hw_exception((seg == x86_seg_ss) + ? TRAP_stack_error + : TRAP_gp_fault, 0, &hvmemul_ctxt->ctxt); return X86EMUL_EXCEPTION; } @@ -1562,59 +1559,6 @@ int hvmemul_cpuid( return X86EMUL_OKAY; } -static int hvmemul_inject_hw_exception( - uint8_t vector, - int32_t error_code, - struct x86_emulate_ctxt *ctxt) -{ - struct hvm_emulate_ctxt *hvmemul_ctxt = - container_of(ctxt, struct hvm_emulate_ctxt, ctxt); - - hvmemul_ctxt->exn_pending = 1; - hvmemul_ctxt->trap.vector = vector; - hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION; - hvmemul_ctxt->trap.error_code = error_code; - hvmemul_ctxt->trap.insn_len = 0; - - return X86EMUL_OKAY; -} - -static int hvmemul_inject_sw_interrupt( - enum x86_swint_type type, - uint8_t vector, - uint8_t insn_len, - struct x86_emulate_ctxt *ctxt) -{ - struct hvm_emulate_ctxt *hvmemul_ctxt = - container_of(ctxt, struct hvm_emulate_ctxt, ctxt); - - switch ( type ) - { - case x86_swint_icebp: - hvmemul_ctxt->trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION; - break; - - case x86_swint_int3: - case x86_swint_into: - hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_EXCEPTION; - break; - - case x86_swint_int: - hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_INTERRUPT; - break; - - default: - return X86EMUL_UNHANDLEABLE; - } - - hvmemul_ctxt->exn_pending = 1; - hvmemul_ctxt->trap.vector = vector; - hvmemul_ctxt->trap.error_code = X86_EVENT_NO_EC; - hvmemul_ctxt->trap.insn_len = insn_len; - - return X86EMUL_OKAY; -} - static int hvmemul_get_fpu( void (*exception_callback)(void *, struct cpu_user_regs *), void *exception_callback_arg, @@ -1678,8 +1622,7 @@ static int hvmemul_invlpg( * hvmemul_virtual_to_linear() raises exceptions for type/limit * violations, so squash them. */ - hvmemul_ctxt->exn_pending = 0; - hvmemul_ctxt->trap = (struct x86_event){}; + x86_emul_reset_event(ctxt); rc = X86EMUL_OKAY; } @@ -1696,7 +1639,7 @@ static int hvmemul_vmfunc( rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs); if ( rc != X86EMUL_OKAY ) - hvmemul_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt); + x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt); return rc; } @@ -1720,8 +1663,6 @@ static const struct x86_emulate_ops hvm_emulate_ops = { .write_msr = hvmemul_write_msr, .wbinvd = hvmemul_wbinvd, .cpuid = hvmemul_cpuid, - .inject_hw_exception = hvmemul_inject_hw_exception, - .inject_sw_interrupt = hvmemul_inject_sw_interrupt, .get_fpu = hvmemul_get_fpu, .put_fpu = hvmemul_put_fpu, .invlpg = hvmemul_invlpg, @@ -1747,8 +1688,6 @@ static const struct x86_emulate_ops hvm_emulate_ops_no_write = { .write_msr = hvmemul_write_msr_discard, .wbinvd = hvmemul_wbinvd_discard, .cpuid = hvmemul_cpuid, - .inject_hw_exception = hvmemul_inject_hw_exception, - .inject_sw_interrupt = hvmemul_inject_sw_interrupt, .get_fpu = hvmemul_get_fpu, .put_fpu = hvmemul_put_fpu, .invlpg = hvmemul_invlpg, @@ -1870,8 +1809,8 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) hvm_dump_emulation_state(XENLOG_G_WARNING "MMCFG", &ctxt); break; case X86EMUL_EXCEPTION: - if ( ctxt.exn_pending ) - hvm_inject_event(&ctxt.trap); + if ( ctxt.ctxt.event_pending ) + hvm_inject_event(&ctxt.ctxt.event); /* fallthrough */ default: hvm_emulate_writeback(&ctxt); @@ -1930,8 +1869,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, hvm_inject_hw_exception(trapnr, errcode); break; case X86EMUL_EXCEPTION: - if ( ctx.exn_pending ) - hvm_inject_event(&ctx.trap); + if ( ctx.ctxt.event_pending ) + hvm_inject_event(&ctx.ctxt.event); break; } @@ -2006,8 +1945,6 @@ void hvm_emulate_init_per_insn( hvmemul_ctxt->insn_buf_bytes = insn_bytes; memcpy(hvmemul_ctxt->insn_buf, insn_buf, insn_bytes); } - - hvmemul_ctxt->exn_pending = 0; } void hvm_emulate_writeback( diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index b950842..ef83100 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4076,8 +4076,8 @@ void hvm_ud_intercept(struct cpu_user_regs *regs) hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); break; case X86EMUL_EXCEPTION: - if ( ctxt.exn_pending ) - hvm_inject_event(&ctxt.trap); + if ( ctxt.ctxt.event_pending ) + hvm_inject_event(&ctxt.ctxt.event); /* fall through */ default: hvm_emulate_writeback(&ctxt); diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index 1279f68..abb9d51 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -102,8 +102,8 @@ int handle_mmio(void) hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt); return 0; case X86EMUL_EXCEPTION: - if ( ctxt.exn_pending ) - hvm_inject_event(&ctxt.trap); + if ( ctxt.ctxt.event_pending ) + hvm_inject_event(&ctxt.ctxt.event); break; default: break; diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c index 9002638..dc3ab44 100644 --- a/xen/arch/x86/hvm/vmx/realmode.c +++ b/xen/arch/x86/hvm/vmx/realmode.c @@ -122,7 +122,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt) if ( rc == X86EMUL_EXCEPTION ) { - if ( !hvmemul_ctxt->exn_pending ) + if ( !hvmemul_ctxt->ctxt.event_pending ) { unsigned long intr_info; @@ -133,27 +133,27 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt) gdprintk(XENLOG_ERR, "Exception pending but no info.\n"); goto fail; } - hvmemul_ctxt->trap.vector = (uint8_t)intr_info; - hvmemul_ctxt->trap.insn_len = 0; + hvmemul_ctxt->ctxt.event.vector = (uint8_t)intr_info; + hvmemul_ctxt->ctxt.event.insn_len = 0; } if ( unlikely(curr->domain->debugger_attached) && - ((hvmemul_ctxt->trap.vector == TRAP_debug) || - (hvmemul_ctxt->trap.vector == TRAP_int3)) ) + ((hvmemul_ctxt->ctxt.event.vector == TRAP_debug) || + (hvmemul_ctxt->ctxt.event.vector == TRAP_int3)) ) { domain_pause_for_debugger(); } else if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE ) { gdprintk(XENLOG_ERR, "Exception %02x in protected mode.\n", - hvmemul_ctxt->trap.vector); + hvmemul_ctxt->ctxt.event.vector); goto fail; } else { realmode_deliver_exception( - hvmemul_ctxt->trap.vector, - hvmemul_ctxt->trap.insn_len, + hvmemul_ctxt->ctxt.event.vector, + hvmemul_ctxt->ctxt.event.insn_len, hvmemul_ctxt); } } diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 231c7bf..5d59479 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5379,6 +5379,19 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr, page_unlock(page); put_page(page); + /* + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such exceptions + * now set event_pending instead. Exceptions raised behind the back of + * the emulator don't yet set event_pending. + * + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path, + * for no functional change from before. Future patches will fix this + * properly. + */ + if ( rc == X86EMUL_EXCEPTION && ptwr_ctxt.ctxt.event_pending ) + rc = X86EMUL_UNHANDLEABLE; + if ( rc == X86EMUL_UNHANDLEABLE ) goto bail; @@ -5506,6 +5519,19 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr, else rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops); + /* + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such exceptions + * now set event_pending instead. Exceptions raised behind the back of + * the emulator don't yet set event_pending. + * + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path, + * for no functional change from before. Future patches will fix this + * properly. + */ + if ( rc == X86EMUL_EXCEPTION && ctxt.event_pending ) + rc = X86EMUL_UNHANDLEABLE; + if ( rc == X86EMUL_UNHANDLEABLE ) return 0; diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index ddfb815..56c40f8 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3374,6 +3374,19 @@ static int sh_page_fault(struct vcpu *v, r = x86_emulate(&emul_ctxt.ctxt, emul_ops); /* + * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised + * by the emulator itself to become X86EMUL_UNHANDLEABLE. Such exceptions + * now set event_pending instead. Exceptions raised behind the back of + * the emulator don't yet set event_pending. + * + * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path, + * for no functional change from before. Future patches will fix this + * properly. + */ + if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) + r = X86EMUL_UNHANDLEABLE; + + /* * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it * would be a good unshadow hint. If we *do* decide to unshadow-on-fault * then it must be 'failable': we cannot require the unshadow to succeed. @@ -3443,6 +3456,10 @@ static int sh_page_fault(struct vcpu *v, shadow_continue_emulation(&emul_ctxt, regs); v->arch.paging.last_write_was_pt = 0; r = x86_emulate(&emul_ctxt.ctxt, emul_ops); + + if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending ) + r = X86EMUL_UNHANDLEABLE; + if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) { emulation_count++; diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 6adfdbe..0fb2c09 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -680,9 +680,8 @@ static inline int mkec(uint8_t e, int32_t ec, ...) #define generate_exception_if(p, e, ec...) \ ({ if ( (p) ) { \ - fail_if(ops->inject_hw_exception == NULL); \ - rc = ops->inject_hw_exception(e, mkec(e, ##ec, 0), ctxt) \ - ? : X86EMUL_EXCEPTION; \ + x86_emul_hw_exception(e, mkec(e, ##ec, 0), ctxt); \ + rc = X86EMUL_EXCEPTION; \ goto done; \ } \ }) @@ -1604,9 +1603,6 @@ static int inject_swint(enum x86_swint_type type, { int rc, error_code, fault_type = EXC_GP; - fail_if(ops->inject_sw_interrupt == NULL); - fail_if(ops->inject_hw_exception == NULL); - /* * Without hardware support, injecting software interrupts/exceptions is * problematic. @@ -1701,7 +1697,8 @@ static int inject_swint(enum x86_swint_type type, } } - rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); + x86_emul_software_event(type, vector, insn_len, ctxt); + rc = X86EMUL_OKAY; done: return rc; @@ -1909,6 +1906,7 @@ x86_decode( /* Initialise output state in x86_emulate_ctxt */ ctxt->retire.raw = 0; + x86_emul_reset_event(ctxt); op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt->addr_size/8; if ( op_bytes == 8 ) diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index da8924b..3c0b25d 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -396,19 +396,6 @@ struct x86_emulate_ops unsigned int *edx, struct x86_emulate_ctxt *ctxt); - /* inject_hw_exception */ - int (*inject_hw_exception)( - uint8_t vector, - int32_t error_code, - struct x86_emulate_ctxt *ctxt); - - /* inject_sw_interrupt */ - int (*inject_sw_interrupt)( - enum x86_swint_type type, - uint8_t vector, - uint8_t insn_len, - struct x86_emulate_ctxt *ctxt); - /* * get_fpu: Load emulated environment's FPU state onto processor. * @exn_callback: On any FPU or SIMD exception, pass control to @@ -486,6 +473,9 @@ struct x86_emulate_ctxt bool singlestep:1; /* Singlestepping was active. */ }; } retire; + + bool event_pending; + struct x86_event event; }; /* @@ -584,6 +574,19 @@ static inline int x86_emulate_wrapper( if ( rc == X86EMUL_EXCEPTION ) ASSERT(ctxt->regs->eip == orig_eip); + /* + * TODO: Make this true: + * + ASSERT(ctxt->event_pending == (rc == X86EMUL_EXCEPTION)); + * + * Some codepaths still raise exceptions behind the back of the + * emulator. (i.e. return X86EMUL_EXCEPTION but without + * event_pending being set). In the meantime, use a slightly + * relaxed check... + */ + if ( ctxt->event_pending ) + ASSERT(rc == X86EMUL_EXCEPTION); + return rc; } @@ -633,4 +636,51 @@ void x86_emulate_free_state(struct x86_emulate_state *state); #endif +static inline void x86_emul_hw_exception( + unsigned int vector, int error_code, struct x86_emulate_ctxt *ctxt) +{ + ASSERT(!ctxt->event_pending); + + ctxt->event.vector = vector; + ctxt->event.type = X86_EVENTTYPE_HW_EXCEPTION; + ctxt->event.error_code = error_code; + + ctxt->event_pending = true; +} + +static inline void x86_emul_software_event( + enum x86_swint_type type, uint8_t vector, uint8_t insn_len, + struct x86_emulate_ctxt *ctxt) +{ + ASSERT(!ctxt->event_pending); + + switch ( type ) + { + case x86_swint_icebp: + ctxt->event.type = X86_EVENTTYPE_PRI_SW_EXCEPTION; + break; + + case x86_swint_int3: + case x86_swint_into: + ctxt->event.type = X86_EVENTTYPE_SW_EXCEPTION; + break; + + case x86_swint_int: + ctxt->event.type = X86_EVENTTYPE_SW_INTERRUPT; + break; + } + + ctxt->event.vector = vector; + ctxt->event.error_code = X86_EVENT_NO_EC; + ctxt->event.insn_len = insn_len; + + ctxt->event_pending = true; +} + +static inline void x86_emul_reset_event(struct x86_emulate_ctxt *ctxt) +{ + ctxt->event_pending = false; + ctxt->event = (struct x86_event){}; +} + #endif /* __X86_EMULATE_H__ */ diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h index 3b7ec33..d64d834 100644 --- a/xen/include/asm-x86/hvm/emulate.h +++ b/xen/include/asm-x86/hvm/emulate.h @@ -29,9 +29,6 @@ struct hvm_emulate_ctxt { unsigned long seg_reg_accessed; unsigned long seg_reg_dirty; - bool_t exn_pending; - struct x86_event trap; - uint32_t intr_shadow; bool_t set_context;
The emulator needs to gain an understanding of interrupts and exceptions generated by its actions. Move hvm_emulate_ctxt.{exn_pending,trap} into struct x86_emulate_ctxt so they are visible to the emulator. This removes the need for the inject_{hw_exception,sw_interrupt}() hooks, which are dropped and replaced with x86_emul_{hw_exception,software_event,reset_event}() instead. For exceptions raised by x86_emulate() itself (rather than its callbacks), the shadow pagetable and PV uses of x86_emulate() previously failed with X86EMUL_UNHANDLEABLE due to the lack of inject_*() hooks. This behaviour has changed, and such cases will now return X86EMUL_EXCEPTION with event_pending set. Until the callers of x86_emulate() have been updated to inject events back into the guest, divert the event_pending case back into the X86EMUL_UNHANDLEABLE path to maintain the same guest-visible behaviour. No overall functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> v3: * Rework how the event_pending case is currently handled v2: * Change x86_emul_hw_exception()'s error_code parameter to being signed * Clarify how software interrupt injection happens. * More ASSERT()'s and description of how event_pending works without the inject_sw_interrupt() hook --- xen/arch/x86/hvm/emulate.c | 81 ++++------------------------------ xen/arch/x86/hvm/hvm.c | 4 +- xen/arch/x86/hvm/io.c | 4 +- xen/arch/x86/hvm/vmx/realmode.c | 16 +++---- xen/arch/x86/mm.c | 26 +++++++++++ xen/arch/x86/mm/shadow/multi.c | 17 +++++++ xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++-- xen/arch/x86/x86_emulate/x86_emulate.h | 76 +++++++++++++++++++++++++------ xen/include/asm-x86/hvm/emulate.h | 3 -- 9 files changed, 132 insertions(+), 107 deletions(-)