Re: [Xen-devel] [PATCH v3 11/24] x86/emul: Implement singlestep as a retire flag

2016-12-01 Thread Jan Beulich 
>>> 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

2016-12-01 Thread Tim Deegan 
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

2016-12-01 Thread Andrew Cooper 
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(_ctxt, regs);
>>  v->arch.paging.last_write_was_pt = 0;
>>  r = x86_emulate(_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

2016-12-01 Thread Jan Beulich 
>>> 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(_ctxt, regs);
>  v->arch.paging.last_write_was_pt = 0;
>  r = x86_emulate(_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

2016-11-30 Thread Paul Durrant 
> -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(, _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(_ctxt, regs);
>  v->arch.paging.last_write_was_pt = 0;
>  r = x86_emulate(_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; /*