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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/02/18 09:37, Alexandru Isaila wrote:
> This commit enables the breakpoint events for svm.
>
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> ---
>  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;

As a minor note, please clean up bool_t => bool as you end up making
changes.

>      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;
> +    else
> +        event.insn_len = 0;

IIRC, you need to always set insn_len.  The length handling is vastly
complicated (depends on event type and hardware availability, and in
some cases needs emulating anyway), but the lower injection levels
should DTRT.

If they don't, can you provide a concrete example which doesn't work and
we can see about what to do.

> +
> +    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 )
> +        {
> +         /* 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);

Please introduce an extra pair of brackets around the whole thing, so
automatic indentation in most editors DTRT.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel