Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag
>>> On 01.12.16 at 12:23, wrote: > On 01/12/16 11:16, Jan Beulich wrote: > On 30.11.16 at 14:50, wrote: >>> @@ -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) > > Correct > >> but also the trace_shadow_emulate(), which I don't think is wanted. > > Hmm. It is only the PAE case we want to skip. Perhaps changing the PAE > entry condition to > > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index c45d260..28ff945 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3480,7 +3480,7 @@ static int sh_page_fault(struct vcpu *v, > } > > #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ > -if ( r == X86EMUL_OKAY ) { > +if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) { > int i, emulation_count=0; > this_cpu(trace_emulate_initial_va) = va; > /* Emulate up to four extra instructions in the hope of catching > > would be better, along with suitable comments and style fixes? Yes I think so (and I see Tim has said so too). >>> @@ -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? > > I am currently reading up about this in the manual. Tell me if you find anything. All I have is my memory of good old DOS days, where I recall single stepping over %ss loads always surprised me (over time of course with a fading level of surprise) in taking two steps. The only thing I can't tell for sure is whether this maybe was a cute feature of the debugger (recognizing the %ss load instructions). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag
At 11:23 + on 01 Dec (1480591394), Andrew Cooper wrote: > Hmm. It is only the PAE case we want to skip. Perhaps changing the PAE > entry condition to > > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index c45d260..28ff945 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3480,7 +3480,7 @@ static int sh_page_fault(struct vcpu *v, > } > > #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ > -if ( r == X86EMUL_OKAY ) { > +if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) { > int i, emulation_count=0; > this_cpu(trace_emulate_initial_va) = va; > /* Emulate up to four extra instructions in the hope of catching > > would be better, along with suitable comments and style fixes? That would be OK by me, and with that change, Acked-by: Tim Deegan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag
On 01/12/16 11:16, Jan Beulich wrote: On 30.11.16 at 14:50, 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) Correct > but also the trace_shadow_emulate(), which I don't think is wanted. Hmm. It is only the PAE case we want to skip. Perhaps changing the PAE entry condition to diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index c45d260..28ff945 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3480,7 +3480,7 @@ static int sh_page_fault(struct vcpu *v, } #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ -if ( r == X86EMUL_OKAY ) { +if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw ) { int i, emulation_count=0; this_cpu(trace_emulate_initial_va) = va; /* Emulate up to four extra instructions in the hope of catching would be better, along with suitable comments and style fixes? > >> @@ -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). Ok. > >> @@ -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. "failed to find the second half of the write". In combination with a suitable comment in the hunk above, this should be fine as is. > >> @@ -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? I am currently reading up about this in the manual. I need more coffee. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag
>>> On 30.11.16 at 14:50, 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag
> -Original Message- > From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: 30 November 2016 13:50 > To: Xen-devel > Cc: Andrew Cooper ; Jan Beulich > ; Tim (Xen.org) ; Paul Durrant > > 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 > --- > CC: Jan Beulich > CC: Tim Deegan > CC: Paul Durrant Reviewed-by: Paul Durrant > > 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 > --
[Xen-devel] [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 --- CC: Jan Beulich CC: Tim Deegan CC: Paul Durrant 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