Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/08/2021 21:29, Bobby Eshleman wrote:
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index e60af16ddd..d0a4c0ea74 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -858,13 +858,20 @@ static void do_trap(struct cpu_user_regs *regs)
>      if ( regs->error_code & X86_XEC_EXT )
>          goto hardware_trap;
>  
> -    if ( debugger_trap_entry(trapnr, regs) )
> -        return;
> -
>      ASSERT(trapnr < 32);
>  
>      if ( guest_mode(regs) )
>      {
> +        struct vcpu *curr = current;
> +        if ( (trapnr == TRAP_debug || trapnr == TRAP_int3) &&
> +              guest_kernel_mode(curr, regs) &&
> +              curr->domain->debugger_attached )
> +        {
> +            if ( trapnr != TRAP_debug )
> +                curr->arch.gdbsx_vcpu_event = trapnr;
> +            domain_pause_for_debugger();
> +            return;
> +        }

There's no need for this.  Both TRAP_debug and TRAP_int3 have their own
custom handers, and don't use do_trap().

> @@ -1215,6 +1218,12 @@ void do_int3(struct cpu_user_regs *regs)
>  
>          return;
>      }
> +    else if ( guest_kernel_mode(curr, regs) && 
> curr->domain->debugger_attached )

if ( foo ) { ...; return; } else if ( bar )  is a dangerous anti-pattern.

This should just be a plain if().

> @@ -1994,6 +1994,11 @@ void do_debug(struct cpu_user_regs *regs)
>  
>          return;
>      }
> +    else if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )

Same here.

> @@ -2028,6 +2030,12 @@ void do_entry_CP(struct cpu_user_regs *regs)
>       */
>      if ( guest_mode(regs) )
>      {
> +        struct vcpu *curr = current;
> +        if ( guest_kernel_mode(curr, regs) && 
> curr->domain->debugger_attached )
> +        {
> +            domain_pause_for_debugger();
> +            return;
> +        }
>          gprintk(XENLOG_ERR, "Hit #CP[%04x] in guest context %04x:%p\n",
>                  ec, regs->cs, _p(regs->rip));
>          ASSERT_UNREACHABLE();

This path is fatal to Xen, because it should be impossible.  If we ever
get around to supporting CET for PV guests, #CP still isn't #DB/#BP so
shouldn't pause for the debugger.

I can fix all of these on commit.

~Andrew