AMD support of hvm_vcpu.single_step

AMD support of hvm_vcpu.single_step

[Mukesh Rathor]

Hi AMD folks,

I am unable to see support of single step on amd platform. On intel,
hvm_vcpu.single_step will cause MTF to be set in vmx_intr_assist:

    /* Block event injection when single step with MTF. */
    if ( unlikely(v->arch.hvm_vcpu.single_step) )
    {           
        v->arch.hvm_vmx.exec_control |= CPU_BASED_MONITOR_TRAP_FLAG;
        vmx_update_cpu_exec_control(v);
        return;
    }       

I don't see equivalent on amd? 

Furthermore, as a second issue, in __update_guest_eip(), imo the exception 
is injected into the guest prematurely :

    if ( regs->eflags & X86_EFLAGS_TF )
            hvm_inject_hw_exception(TRAP_debug,
    HVM_DELIVER_NO_ERROR_CODE);

Reason being, in the BP path, __update_guest_eip is called before
domain_pause_for_debugger(). As such, if the BP is handled, there is no
need to inject into the guest. right?

I've the following patch in my tree for the single step issue. LMK what
you think.

thanks
Mukesh


diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 023151a..28d2c6d 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -139,6 +139,12 @@ void svm_intr_assist(void)
     struct hvm_intack intack;
     enum hvm_intblk intblk;
 
+    if ( unlikely(v->arch.hvm_vcpu.single_step) )
+    {
+        guest_cpu_user_regs()->eflags |= X86_EFLAGS_TF;
+        return;
+    }
+
     /* Crank the handle on interrupt state. */
     pt_update_irq(v);
 
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index d307dfb..b55825b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2340,12 +2368,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case VMEXIT_EXCEPTION_DB:
+        if ( !v->domain->debugger_attached )
+            goto exit_and_crash;
+        else
+            regs->eflags &= ~X86_EFLAGS_TF;
-        if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
         domain_pause_for_debugger();
         break;

Re: AMD support of hvm_vcpu.single_step

[Boris Ostrovsky]

On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
> Hi AMD folks,
>
> I am unable to see support of single step on amd platform. On intel,
> hvm_vcpu.single_step will cause MTF to be set in vmx_intr_assist:
>
>      /* Block event injection when single step with MTF. */
>      if ( unlikely(v->arch.hvm_vcpu.single_step) )
>      {
>          v->arch.hvm_vmx.exec_control |= CPU_BASED_MONITOR_TRAP_FLAG;
>          vmx_update_cpu_exec_control(v);
>          return;
>      }
>
> I don't see equivalent on amd?

I think HW performs TF bit manipulations that your patch is suggesting 
(per 13.1.4 of APMv2)

>
> Furthermore, as a second issue, in __update_guest_eip(), imo the exception
> is injected into the guest prematurely :
>
>      if ( regs->eflags & X86_EFLAGS_TF )
>              hvm_inject_hw_exception(TRAP_debug,
>      HVM_DELIVER_NO_ERROR_CODE);
>
> Reason being, in the BP path, __update_guest_eip is called before
> domain_pause_for_debugger(). As such, if the BP is handled, there is no
> need to inject into the guest. right?

TF should be cleared when INT3 is executed according to the same section.

(Besides, it looks like VMX code does the same.)

-boris

>
> I've the following patch in my tree for the single step issue. LMK what
> you think.
>
> thanks
> Mukesh
>
>
> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
> index 023151a..28d2c6d 100644
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -139,6 +139,12 @@ void svm_intr_assist(void)
>       struct hvm_intack intack;
>       enum hvm_intblk intblk;
>   
> +    if ( unlikely(v->arch.hvm_vcpu.single_step) )
> +    {
> +        guest_cpu_user_regs()->eflags |= X86_EFLAGS_TF;
> +        return;
> +    }
> +
>       /* Crank the handle on interrupt state. */
>       pt_update_irq(v);
>   
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index d307dfb..b55825b 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2340,12 +2368,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>           break;
>   
>       case VMEXIT_EXCEPTION_DB:
> +        if ( !v->domain->debugger_attached )
> +            goto exit_and_crash;
> +        else
> +            regs->eflags &= ~X86_EFLAGS_TF;
> -        if ( !v->domain->debugger_attached )
> -            goto exit_and_crash;
>           domain_pause_for_debugger();
>           break;
>

Re: AMD support of hvm_vcpu.single_step

[Mukesh Rathor]

On Wed, 11 Jun 2014 11:56:15 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
> > Hi AMD folks,
> >
> > I am unable to see support of single step on amd platform. On intel,
> > hvm_vcpu.single_step will cause MTF to be set in vmx_intr_assist:
> >
> >      /* Block event injection when single step with MTF. */
> >      if ( unlikely(v->arch.hvm_vcpu.single_step) )
> >      {
> >          v->arch.hvm_vmx.exec_control |=
> > CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v);
> >          return;
> >      }
> >
> > I don't see equivalent on amd?
> 
> I think HW performs TF bit manipulations that your patch is
> suggesting (per 13.1.4 of APMv2)

It only clears it, not set it. Patch on the way.

thanks
mukesh

Re: AMD support of hvm_vcpu.single_step

[Boris Ostrovsky]

On 06/11/2014 06:32 PM, Mukesh Rathor wrote:
> On Wed, 11 Jun 2014 11:56:15 -0400
> Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>
>> On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
>>> Hi AMD folks,
>>>
>>> I am unable to see support of single step on amd platform. On intel,
>>> hvm_vcpu.single_step will cause MTF to be set in vmx_intr_assist:
>>>
>>>       /* Block event injection when single step with MTF. */
>>>       if ( unlikely(v->arch.hvm_vcpu.single_step) )
>>>       {
>>>           v->arch.hvm_vmx.exec_control |=
>>> CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v);
>>>           return;
>>>       }
>>>
>>> I don't see equivalent on amd?
>> I think HW performs TF bit manipulations that your patch is
>> suggesting (per 13.1.4 of APMv2)
> It only clears it, not set it. Patch on the way.


Does it need to be set? My understanding is that it is supposed to be 
clear in the handler (in the guest) but set on the exception's stack so 
when the guest does the IRET it loads TF=1.

-boris

Re: AMD support of hvm_vcpu.single_step

[Mukesh Rathor]

On Wed, 11 Jun 2014 18:55:53 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 06/11/2014 06:32 PM, Mukesh Rathor wrote:
> > On Wed, 11 Jun 2014 11:56:15 -0400
> > Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> >
> >> On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
> >>> Hi AMD folks,
> >>>
> >>> I am unable to see support of single step on amd platform. On
> >>> intel, hvm_vcpu.single_step will cause MTF to be set in
> >>> vmx_intr_assist:
> >>>
> >>>       /* Block event injection when single step with MTF. */
> >>>       if ( unlikely(v->arch.hvm_vcpu.single_step) )
> >>>       {
> >>>           v->arch.hvm_vmx.exec_control |=
> >>> CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v);
> >>>           return;
> >>>       }
> >>>
> >>> I don't see equivalent on amd?
> >> I think HW performs TF bit manipulations that your patch is
> >> suggesting (per 13.1.4 of APMv2)
> > It only clears it, not set it. Patch on the way.
> 
> 
> Does it need to be set? My understanding is that it is supposed to be 
> clear in the handler (in the guest) but set on the exception's stack
> so when the guest does the IRET it loads TF=1.

If a debugger wants to single step, it needs to set the TF. So this
is for the debugger path. The hardware clears it upon execution of next
instruction, and generates #DB.

-Mukesh

Re: AMD support of hvm_vcpu.single_step

[Boris Ostrovsky]

On 06/11/2014 07:04 PM, Mukesh Rathor wrote:
> On Wed, 11 Jun 2014 18:55:53 -0400
> Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>
>> On 06/11/2014 06:32 PM, Mukesh Rathor wrote:
>>> On Wed, 11 Jun 2014 11:56:15 -0400
>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>>
>>>> On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
>>>>> Hi AMD folks,
>>>>>
>>>>> I am unable to see support of single step on amd platform. On
>>>>> intel, hvm_vcpu.single_step will cause MTF to be set in
>>>>> vmx_intr_assist:
>>>>>
>>>>>        /* Block event injection when single step with MTF. */
>>>>>        if ( unlikely(v->arch.hvm_vcpu.single_step) )
>>>>>        {
>>>>>            v->arch.hvm_vmx.exec_control |=
>>>>> CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v);
>>>>>            return;
>>>>>        }
>>>>>
>>>>> I don't see equivalent on amd?
>>>> I think HW performs TF bit manipulations that your patch is
>>>> suggesting (per 13.1.4 of APMv2)
>>> It only clears it, not set it. Patch on the way.
>>
>> Does it need to be set? My understanding is that it is supposed to be
>> clear in the handler (in the guest) but set on the exception's stack
>> so when the guest does the IRET it loads TF=1.
> If a debugger wants to single step, it needs to set the TF. So this
> is for the debugger path. The hardware clears it upon execution of next
> instruction, and generates #DB.

The patch seems to be setting TF when returning from #DB intercept, 
right? Won't this result in another #DB on the first instruction of 
guest's #DB handler?

-boris

Re: AMD support of hvm_vcpu.single_step

[Mukesh Rathor]

On Wed, 11 Jun 2014 19:46:12 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 06/11/2014 07:04 PM, Mukesh Rathor wrote:
> > On Wed, 11 Jun 2014 18:55:53 -0400
> > Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> >
> >> On 06/11/2014 06:32 PM, Mukesh Rathor wrote:
> >>> On Wed, 11 Jun 2014 11:56:15 -0400
> >>> Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> >>>
> >>>> On 06/10/2014 05:49 PM, Mukesh Rathor wrote:
> >>>>> Hi AMD folks,
> >>>>>
> >>>>> I am unable to see support of single step on amd platform. On
> >>>>> intel, hvm_vcpu.single_step will cause MTF to be set in
> >>>>> vmx_intr_assist:
> >>>>>
> >>>>>        /* Block event injection when single step with MTF. */
> >>>>>        if ( unlikely(v->arch.hvm_vcpu.single_step) )
> >>>>>        {
> >>>>>            v->arch.hvm_vmx.exec_control |=
> >>>>> CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v);
> >>>>>            return;
> >>>>>        }
> >>>>>
> >>>>> I don't see equivalent on amd?
> >>>> I think HW performs TF bit manipulations that your patch is
> >>>> suggesting (per 13.1.4 of APMv2)
> >>> It only clears it, not set it. Patch on the way.
> >>
> >> Does it need to be set? My understanding is that it is supposed to
> >> be clear in the handler (in the guest) but set on the exception's
> >> stack so when the guest does the IRET it loads TF=1.
> > If a debugger wants to single step, it needs to set the TF. So this
> > is for the debugger path. The hardware clears it upon execution of
> > next instruction, and generates #DB.
> 
> The patch seems to be setting TF when returning from #DB intercept, 
> right? Won't this result in another #DB on the first instruction of 
> guest's #DB handler?

Nop, we are setting the TF in the resume path, returning from whatever
intercept it was. It could have been anything. If an external debugger set the
single_step flag during this time, then just like vmx, svm must honor that 
and do what it takes, in this case setting the TF (vmx sets MTF) when
it's resuming the guest. 

thanks,
Mukesh