[v3,11/24] x86/emul: Implement singlestep as a retire flag

Commit Message

Andrew Cooper Dec. 1, 2016, 11:23 a.m. UTC

On 01/12/16 11:16, Jan Beulich wrote:
>>>> On 30.11.16 at 14:50, <[e-mail]> 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


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

Comments

Tim Deegan Dec. 1, 2016, 11:33 a.m. UTC | #1

At 11:23 +0000 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 <[e-mail]>

Jan Beulich Dec. 1, 2016, 12:05 p.m. UTC | #2

>>> On 01.12.16 at 12:23, <[e-mail]> wrote:
> On 01/12/16 11:16, Jan Beulich wrote:
>>>>> On 30.11.16 at 14:50, <[e-mail]> 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

Patch

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