Re: [PATCH v1 2/4] hvm/svm: Enable Breakpoint events

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On Vi, 2018-02-02 at 08:42 -0700, Tamas K Lengyel wrote:
> On Fri, Feb 2, 2018 at 2:37 AM, Alexandru Isaila
> <aisaila@bitdefender.com> wrote:
> > 
> > This commit enables the breakpoint events for svm.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c    | 52
> > ++++++++++++++++++++++++++++++++++++-------
> >  xen/include/asm-x86/monitor.h |  3 ++-
> >  2 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/svm/svm.c
> > b/xen/arch/x86/hvm/svm/svm.c
> > index dcbd550..14a5f60 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -59,6 +59,7 @@
> >  #include <asm/hap.h>
> >  #include <asm/apic.h>
> >  #include <asm/debugger.h>
> > +#include <asm/hvm/monitor.h>
> >  #include <asm/xstate.h>
> > 
> >  void svm_asm_do_resume(void);
> > @@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu
> > *v)
> >  static void noreturn svm_do_resume(struct vcpu *v)
> >  {
> >      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > -    bool_t debug_state = v->domain->debugger_attached;
> > +    bool_t debug_state = v->domain->debugger_attached
> > +                || v->domain-
> > >arch.monitor.software_breakpoint_enabled;
> >      bool_t vcpu_guestmode = 0;
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> > 
> > @@ -2407,6 +2409,23 @@ static bool svm_get_pending_event(struct
> > vcpu *v, struct x86_event *info)
> >      return true;
> >  }
> > 
> > +static void svm_propagate_intr(struct vcpu *v, unsigned long
> > insn_len)
> > +{
> > +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > +    struct x86_event event = {
> > +        .vector = vmcb->eventinj.fields.type,
> > +        .type = vmcb->eventinj.fields.type,
> > +        .error_code = vmcb->exitinfo1,
> > +    };
> > +
> > +    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
> > +        event.insn_len = insn_len;seems
> > +    else
> > +        event.insn_len = 0;
> > +
> > +    hvm_inject_event(&event);
> > +}
> > +
> >  static struct hvm_function_table __initdata svm_function_table = {
> >      .name                 = "SVM",
> >      .cpu_up_prepare       = svm_cpu_up_prepare,
> > @@ -2619,14 +2638,31 @@ void svm_vmexit_handler(struct
> > cpu_user_regs *regs)
> >          break;
> > 
> >      case VMEXIT_EXCEPTION_BP:
> > -        if ( !v->domain->debugger_attached )
> > -            goto unexpected_exit_type;
> > -        /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not
> > update RIP. */
> > -        if ( (inst_len = __get_instruction_length(v, INSTR_INT3))
> > == 0 )
> > +        inst_len = __get_instruction_length(v, INSTR_INT3);
> > +
> > +        if ( inst_len == 0 )
> >              break;
> > -        __update_guest_eip(regs, inst_len);
> > -        current->arch.gdbsx_vcpu_event = TRAP_int3;
> > -        domain_pause_for_debugger();
> > +
> > +        if ( !v->domain->debugger_attached )
> I think this would be easier to follow if you switched it around.
This is just the way that is on the vmx side. If you think it's better
to change it then I will
> 
> > 
> > +        {
> > +         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not
> > update RIP. */
> > +            int rc;
> > +
> > +            rc = hvm_monitor_debug(regs->rip,
> > +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT
> > ,
> > +                                   X86_EVENTTYPE_SW_EXCEPTION,
> > +                                   inst_len);
> > +            if ( rc < 0 )
> > +                goto unexpected_exit_type;
> > +            if ( !rc )
> > +                svm_propagate_intr(v, inst_len);
> > +        }
> > +        else
> > +        {
> > +            __update_guest_eip(regs, inst_len);
> > +            current->arch.gdbsx_vcpu_event = TRAP_int3;
> > +            domain_pause_for_debugger();
> > +        }
> >          break;
> > 
> >      case VMEXIT_EXCEPTION_NM:
> > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-
> > x86/monitor.h
> > index 3706b7a..68a210a 100644
> > --- a/xen/include/asm-x86/monitor.h
> > +++ b/xen/include/asm-x86/monitor.h
> > @@ -94,7 +94,8 @@ static inline uint32_t
> > arch_monitor_get_capabilities(struct domain *d)
> >      }
> >      else if ( cpu_has_svm )
> >      {
> > -        capabilities = (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> > +        capabilities = (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> > +                       (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> Since breakpoints are also supported for both svm and vmx, you can
> just set it once, no need for the extra if block.
> 
> Tamas
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel