Re: [V0 PATCH] SVM: set/unset TF flag for single_step

Re: [V0 PATCH] SVM: set/unset TF flag for single_step

[Boris Ostrovsky]

----- mukesh.rathor@oracle.com wrote:

> 
> Also strangely, when SVM VMEXIT_EXCEPTION_DB occurs, the TF flag is
> not cleared. This patch addresses that too.
..
>  
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 76616ac..8addb94 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct cpu_user_regs
> *regs)
>      case VMEXIT_EXCEPTION_DB:
>          if ( !v->domain->debugger_attached )
>              goto exit_and_crash;
> +        else
> +            regs->eflags &= ~X86_EFLAGS_TF;
>          domain_pause_for_debugger();
>          break;


I poked around debugger code and it looks to me that tools/debugger/gdbsx/xg:_change_TF() manages this flag.

-boris

Re: [V0 PATCH] SVM: set/unset TF flag for single_step

[Jan Beulich]

>>> On 02.07.14 at 05:36, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct cpu_user_regs
>> *regs)
>>      case VMEXIT_EXCEPTION_DB:
>>          if ( !v->domain->debugger_attached )
>>              goto exit_and_crash;
>> +        else
>> +            regs->eflags &= ~X86_EFLAGS_TF;
>>          domain_pause_for_debugger();
>>          break;
> 
> 
> I poked around debugger code and it looks to me that 
> tools/debugger/gdbsx/xg:_change_TF() manages this flag.

Indeed it shouldn't be the hypervisor to deal with that (unless it had
a built-in one), but the debugger attached.

That said, Mukesh, just as a side note: There's no point for an "else"
after an "if" leading to a "goto".

Jan

Re: [V0 PATCH] SVM: set/unset TF flag for single_step

[Mukesh Rathor]

On Wed, 02 Jul 2014 09:38:40 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 02.07.14 at 05:36, <boris.ostrovsky@oracle.com> wrote:
> >> --- a/xen/arch/x86/hvm/svm/svm.c
> >> +++ b/xen/arch/x86/hvm/svm/svm.c
> >> @@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct cpu_user_regs
> >> *regs)
> >>      case VMEXIT_EXCEPTION_DB:
> >>          if ( !v->domain->debugger_attached )
> >>              goto exit_and_crash;
> >> +        else
> >> +            regs->eflags &= ~X86_EFLAGS_TF;
> >>          domain_pause_for_debugger();
> >>          break;
> > 
> > 
> > I poked around debugger code and it looks to me that 
> > tools/debugger/gdbsx/xg:_change_TF() manages this flag.

_change_TF() only manages TF if XEN_DOMCTL_debug_op fails. Moreover, TF
handling is intended for PV. For HVM, xen provides 
XEN_DOMCTL_debug_op which sets v->arch.hvm_vcpu.single_step.

> Indeed it shouldn't be the hypervisor to deal with that (unless it had
> a built-in one), but the debugger attached.

Correct. But, the hyp provides abi hvm_debug_op 
(XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON) that it needs to honor for svm like
it does for vmx.

> That said, Mukesh, just as a side note: There's no point for an "else"
> after an "if" leading to a "goto".

Right. My "doh!".

Thanks
Mukesh

Re: [V0 PATCH] SVM: set/unset TF flag for single_step

[Mukesh Rathor]

On Wed, 2 Jul 2014 14:52:38 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Wed, 02 Jul 2014 09:38:40 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
> > >>> On 02.07.14 at 05:36, <boris.ostrovsky@oracle.com> wrote:
> > >> --- a/xen/arch/x86/hvm/svm/svm.c
> > >> +++ b/xen/arch/x86/hvm/svm/svm.c
> > >> @@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct
> > >> cpu_user_regs *regs)
> > >>      case VMEXIT_EXCEPTION_DB:
> > >>          if ( !v->domain->debugger_attached )
> > >>              goto exit_and_crash;
> > >> +        else
> > >> +            regs->eflags &= ~X86_EFLAGS_TF;
> > >>          domain_pause_for_debugger();
> > >>          break;
> > > 
> > > 
> > > I poked around debugger code and it looks to me that 
> > > tools/debugger/gdbsx/xg:_change_TF() manages this flag.
> 
> _change_TF() only manages TF if XEN_DOMCTL_debug_op fails. Moreover,
> TF handling is intended for PV. For HVM, xen provides 
> XEN_DOMCTL_debug_op which sets v->arch.hvm_vcpu.single_step.
> 
> > Indeed it shouldn't be the hypervisor to deal with that (unless it
> > had a built-in one), but the debugger attached.
> 
> Correct. But, the hyp provides abi hvm_debug_op 
> (XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON) that it needs to honor for svm
> like it does for vmx.

I should rephrase that from "it needs" to "it would be nice to" as 
all debuggers then can just use hvm_vcpu.single_step internally or
via hvm_debug_op(). 

Boris rightly points out that the cpu_has_monitor_trap_flag check in
hvm_debug_op() would still be an issue, and I missed that check. It could
be removed or an intel check could be added in that if statement.

hvm_debug_op():
            rc = -ENOSYS;
            if ( !cpu_has_monitor_trap_flag ) <---
                break;

hope that makes sense. if in agreement, i can send v1.

thanks
Mukesh

Re: [V0 PATCH] SVM: set/unset TF flag for single_step

[Jan Beulich]

>>> On 02.07.14 at 23:52, <mukesh.rathor@oracle.com> wrote:
> On Wed, 02 Jul 2014 09:38:40 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 02.07.14 at 05:36, <boris.ostrovsky@oracle.com> wrote:
>> >> --- a/xen/arch/x86/hvm/svm/svm.c
>> >> +++ b/xen/arch/x86/hvm/svm/svm.c
>> >> @@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct cpu_user_regs
>> >> *regs)
>> >>      case VMEXIT_EXCEPTION_DB:
>> >>          if ( !v->domain->debugger_attached )
>> >>              goto exit_and_crash;
>> >> +        else
>> >> +            regs->eflags &= ~X86_EFLAGS_TF;
>> >>          domain_pause_for_debugger();
>> >>          break;
>> > 
>> > 
>> > I poked around debugger code and it looks to me that 
>> > tools/debugger/gdbsx/xg:_change_TF() manages this flag.
> 
> _change_TF() only manages TF if XEN_DOMCTL_debug_op fails. Moreover, TF
> handling is intended for PV. For HVM, xen provides 
> XEN_DOMCTL_debug_op which sets v->arch.hvm_vcpu.single_step.

Maybe this is how things are expected to work right now (I admit
to know very little about the specific debugger interface in Xen),
but conceptually this is wrong (and I say this having written a
number of kernel debuggers outside of Xen): Since the next thing
done in the code path above is pausing the domain for debugging,
and since the debugger ought to have control over the entire
register state, it clearly has the power of setting TF to its liking,
and hence no action on it should be needed here.

Jan

[V0 PATCH] SVM: set/unset TF flag for single_step

[Mukesh Rathor]

Noticed on AMD (cpu family : 16, model : 2), that SVM does not honor
arch.hvm_vcpu.single_step flag.

When arch.hvm_vcpu.single_step is set on VMX, it sets MTF. Since there
is no MTF equivalent on AMD, it must set EFLAGS.TF.

Also strangely, when SVM VMEXIT_EXCEPTION_DB occurs, the TF flag is
not cleared. This patch addresses that too.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/intr.c | 6 ++++++
 xen/arch/x86/hvm/svm/svm.c  | 2 ++
 2 files changed, 8 insertions(+)

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 76616ac..8addb94 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2350,6 +2350,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     case VMEXIT_EXCEPTION_DB:
         if ( !v->domain->debugger_attached )
             goto exit_and_crash;
+        else
+            regs->eflags &= ~X86_EFLAGS_TF;
         domain_pause_for_debugger();
         break;
 
-- 
1.8.3.1