[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 13/24] x86/emul: Rework emulator event injection
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx] > Sent: 30 November 2016 13:51 > To: Xen-devel <xen-devel@xxxxxxxxxxxxx> > Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich > <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Tim > (Xen.org) <tim@xxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx> > 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@xxxxxxxxxx> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Paul Durrant <paul.durrant@xxxxxxxxxx> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx> > CC: Tim Deegan <tim@xxxxxxx> > CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx> > > 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |