| Message ID | 1480513841-7565-12-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:50 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > <JBeulich@suse.com>; Tim (Xen.org) <tim@xen.org>; Paul Durrant > <Paul.Durrant@citrix.com> > Subject: [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag > > The behaviour of singlestep is to raise #DB after the instruction has been > completed, but implementing it with inject_hw_exception() causes > x86_emulate() > to return X86EMUL_EXCEPTION, despite succesfully completing execution of > the > instruction, including register writeback. > > Instead, use a retire flag to indicate singlestep, which causes x86_emulate() > to return X86EMUL_OKAY. > > Update all callers of x86_emulate() to use the new retire flag. This fixes > the behaviour of singlestep for shadow pagetable updates and > mmcfg/mmio_ro > intercepts, which previously discarded the exception. > > With this change, all uses of X86EMUL_EXCEPTION from x86_emulate() are > believed to have strictly fault semantics. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > > v3: > * New > --- > xen/arch/x86/hvm/emulate.c | 3 +++ > xen/arch/x86/mm.c | 11 ++++++++++- > xen/arch/x86/mm/shadow/multi.c | 21 ++++++++++++++++++++- > xen/arch/x86/x86_emulate/x86_emulate.c | 9 ++++----- > xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++ > 5 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index fe62500..91c79fa 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -1788,6 +1788,9 @@ static int _hvm_emulate_one(struct > hvm_emulate_ctxt *hvmemul_ctxt, > if ( rc != X86EMUL_OKAY ) > return rc; > > + if ( hvmemul_ctxt->ctxt.retire.singlestep ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + > new_intr_shadow = hvmemul_ctxt->intr_shadow; > > /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index b7c7122..231c7bf 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5382,6 +5382,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned > long addr, > if ( rc == X86EMUL_UNHANDLEABLE ) > goto bail; > > + if ( ptwr_ctxt.ctxt.retire.singlestep ) > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + > perfc_incr(ptwr_emulations); > return EXCRET_fault_fixed; > > @@ -5503,7 +5506,13 @@ int mmio_ro_do_page_fault(struct vcpu *v, > unsigned long addr, > else > rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops); > > - return rc != X86EMUL_UNHANDLEABLE ? EXCRET_fault_fixed : 0; > + if ( rc == X86EMUL_UNHANDLEABLE ) > + return 0; > + > + if ( ctxt.retire.singlestep ) > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + > + return EXCRET_fault_fixed; > } > > void *alloc_xen_pagetable(void) > diff --git a/xen/arch/x86/mm/shadow/multi.c > b/xen/arch/x86/mm/shadow/multi.c > index 9ee48a8..ddfb815 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3422,6 +3422,16 @@ static int sh_page_fault(struct vcpu *v, > v->arch.paging.last_write_emul_ok = 0; > #endif > > + if ( emul_ctxt.ctxt.retire.singlestep ) > + { > + if ( is_hvm_vcpu(v) ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + else > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + > + goto emulate_done; > + } > + > #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ > if ( r == X86EMUL_OKAY ) { > int i, emulation_count=0; > @@ -3433,7 +3443,7 @@ 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_OKAY ) > + if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) > { > emulation_count++; > if ( v->arch.paging.last_write_was_pt ) > @@ -3449,6 +3459,15 @@ static int sh_page_fault(struct vcpu *v, > { > perfc_incr(shadow_em_ex_fail); > > TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); > + > + if ( emul_ctxt.ctxt.retire.singlestep ) > + { > + if ( is_hvm_vcpu(v) ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + else > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + } > + > break; /* Don't emulate again if we failed! */ > } > } > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c > b/xen/arch/x86/x86_emulate/x86_emulate.c > index 8a1f1f5..0af532e 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2417,7 +2417,6 @@ x86_emulate( > struct x86_emulate_state state; > int rc; > uint8_t b, d; > - bool tf = ctxt->regs->eflags & EFLG_TF; > struct operand src = { .reg = PTR_POISON }; > struct operand dst = { .reg = PTR_POISON }; > enum x86_swint_type swint_type; > @@ -5415,11 +5414,11 @@ x86_emulate( > if ( !mode_64bit() ) > _regs.eip = (uint32_t)_regs.eip; > > - *ctxt->regs = _regs; > + /* Was singestepping active at the start of this instruction? */ > + if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) ) > + ctxt->retire.singlestep = true; > > - /* Inject #DB if single-step tracing was enabled at instruction start. */ > - if ( tf && (rc == X86EMUL_OKAY) && ops->inject_hw_exception ) > - rc = ops->inject_hw_exception(EXC_DB, -1, ctxt) ? : > X86EMUL_EXCEPTION; > + *ctxt->regs = _regs; > > done: > _put_fpu(); > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h > b/xen/arch/x86/x86_emulate/x86_emulate.h > index fc28976..da8924b 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -483,6 +483,7 @@ struct x86_emulate_ctxt > bool hlt:1; /* Instruction HLTed. */ > bool mov_ss:1; /* Instruction sets MOV-SS irq shadow. */ > bool sti:1; /* Instruction sets STI irq shadow. */ > + bool singlestep:1; /* Singlestepping was active. */ > }; > } retire; > }; > @@ -572,12 +573,17 @@ static inline int x86_emulate_wrapper( > struct x86_emulate_ctxt *ctxt, > const struct x86_emulate_ops *ops) > { > + unsigned long orig_eip = ctxt->regs->eip; > int rc = x86_emulate(ctxt, ops); > > /* Retire flags should only be set for successful instruction emulation. */ > if ( rc != X86EMUL_OKAY ) > ASSERT(ctxt->retire.raw == 0); > > + /* All cases returning X86EMUL_EXCEPTION should have fault semantics. > */ > + if ( rc == X86EMUL_EXCEPTION ) > + ASSERT(ctxt->regs->eip == orig_eip); > + > return rc; > } > > -- > 2.1.4
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > The behaviour of singlestep is to raise #DB after the instruction has been > completed, but implementing it with inject_hw_exception() causes x86_emulate() > to return X86EMUL_EXCEPTION, despite succesfully completing execution of the > instruction, including register writeback. Nice, I think that'll help simplify the privop patch a bit. > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3422,6 +3422,16 @@ static int sh_page_fault(struct vcpu *v, > v->arch.paging.last_write_emul_ok = 0; > #endif > > + if ( emul_ctxt.ctxt.retire.singlestep ) > + { > + if ( is_hvm_vcpu(v) ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + else > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + > + goto emulate_done; This results in skipping the PAE special code (which I think is intended) but also the trace_shadow_emulate(), which I don't think is wanted. > @@ -3433,7 +3443,7 @@ 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_OKAY ) > + if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) I think this wants to have a comment attached explaining why a blanket check of all current and future retire flags here is the right thing (or benign). > @@ -3449,6 +3459,15 @@ static int sh_page_fault(struct vcpu *v, > { > perfc_incr(shadow_em_ex_fail); > TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); > + > + if ( emul_ctxt.ctxt.retire.singlestep ) > + { > + if ( is_hvm_vcpu(v) ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + else > + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + } > + > break; /* Don't emulate again if we failed! */ This comment is now slightly stale. > @@ -5415,11 +5414,11 @@ x86_emulate( > if ( !mode_64bit() ) > _regs.eip = (uint32_t)_regs.eip; > > - *ctxt->regs = _regs; > + /* Was singestepping active at the start of this instruction? */ > + if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) ) > + ctxt->retire.singlestep = true; Don't we need to avoid doing this when mov_ss is set? Or does the hardware perhaps do the necessary deferring for us? Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index fe62500..91c79fa 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1788,6 +1788,9 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, if ( rc != X86EMUL_OKAY ) return rc; + if ( hvmemul_ctxt->ctxt.retire.singlestep ) + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + new_intr_shadow = hvmemul_ctxt->intr_shadow; /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index b7c7122..231c7bf 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5382,6 +5382,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr, if ( rc == X86EMUL_UNHANDLEABLE ) goto bail; + if ( ptwr_ctxt.ctxt.retire.singlestep ) + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + perfc_incr(ptwr_emulations); return EXCRET_fault_fixed; @@ -5503,7 +5506,13 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr, else rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops); - return rc != X86EMUL_UNHANDLEABLE ? EXCRET_fault_fixed : 0; + if ( rc == X86EMUL_UNHANDLEABLE ) + return 0; + + if ( ctxt.retire.singlestep ) + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + + return EXCRET_fault_fixed; } void *alloc_xen_pagetable(void) diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 9ee48a8..ddfb815 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3422,6 +3422,16 @@ static int sh_page_fault(struct vcpu *v, v->arch.paging.last_write_emul_ok = 0; #endif + if ( emul_ctxt.ctxt.retire.singlestep ) + { + if ( is_hvm_vcpu(v) ) + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + else + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + + goto emulate_done; + } + #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ if ( r == X86EMUL_OKAY ) { int i, emulation_count=0; @@ -3433,7 +3443,7 @@ 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_OKAY ) + if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) { emulation_count++; if ( v->arch.paging.last_write_was_pt ) @@ -3449,6 +3459,15 @@ static int sh_page_fault(struct vcpu *v, { perfc_incr(shadow_em_ex_fail); TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); + + if ( emul_ctxt.ctxt.retire.singlestep ) + { + if ( is_hvm_vcpu(v) ) + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + else + pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + } + break; /* Don't emulate again if we failed! */ } } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 8a1f1f5..0af532e 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2417,7 +2417,6 @@ x86_emulate( struct x86_emulate_state state; int rc; uint8_t b, d; - bool tf = ctxt->regs->eflags & EFLG_TF; struct operand src = { .reg = PTR_POISON }; struct operand dst = { .reg = PTR_POISON }; enum x86_swint_type swint_type; @@ -5415,11 +5414,11 @@ x86_emulate( if ( !mode_64bit() ) _regs.eip = (uint32_t)_regs.eip; - *ctxt->regs = _regs; + /* Was singestepping active at the start of this instruction? */ + if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) ) + ctxt->retire.singlestep = true; - /* Inject #DB if single-step tracing was enabled at instruction start. */ - if ( tf && (rc == X86EMUL_OKAY) && ops->inject_hw_exception ) - rc = ops->inject_hw_exception(EXC_DB, -1, ctxt) ? : X86EMUL_EXCEPTION; + *ctxt->regs = _regs; done: _put_fpu(); diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index fc28976..da8924b 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -483,6 +483,7 @@ struct x86_emulate_ctxt bool hlt:1; /* Instruction HLTed. */ bool mov_ss:1; /* Instruction sets MOV-SS irq shadow. */ bool sti:1; /* Instruction sets STI irq shadow. */ + bool singlestep:1; /* Singlestepping was active. */ }; } retire; }; @@ -572,12 +573,17 @@ static inline int x86_emulate_wrapper( struct x86_emulate_ctxt *ctxt, const struct x86_emulate_ops *ops) { + unsigned long orig_eip = ctxt->regs->eip; int rc = x86_emulate(ctxt, ops); /* Retire flags should only be set for successful instruction emulation. */ if ( rc != X86EMUL_OKAY ) ASSERT(ctxt->retire.raw == 0); + /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */ + if ( rc == X86EMUL_EXCEPTION ) + ASSERT(ctxt->regs->eip == orig_eip); + return rc; }
The behaviour of singlestep is to raise #DB after the instruction has been completed, but implementing it with inject_hw_exception() causes x86_emulate() to return X86EMUL_EXCEPTION, despite succesfully completing execution of the instruction, including register writeback. Instead, use a retire flag to indicate singlestep, which causes x86_emulate() to return X86EMUL_OKAY. Update all callers of x86_emulate() to use the new retire flag. This fixes the behaviour of singlestep for shadow pagetable updates and mmcfg/mmio_ro intercepts, which previously discarded the exception. With this change, all uses of X86EMUL_EXCEPTION from x86_emulate() are believed to have strictly fault semantics. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Paul Durrant <paul.durrant@citrix.com> v3: * New --- xen/arch/x86/hvm/emulate.c | 3 +++ xen/arch/x86/mm.c | 11 ++++++++++- xen/arch/x86/mm/shadow/multi.c | 21 ++++++++++++++++++++- xen/arch/x86/x86_emulate/x86_emulate.c | 9 ++++----- xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++ 5 files changed, 43 insertions(+), 7 deletions(-)