Re: [PATCH 3/6] x86: don't assume #DB is always caused by singlestep if EFLAGS.TF is set

[Jinoh Kang <jinoh.kang.kr@xxxxxxxxx>]

On 8/19/23 00:47, Jinoh Kang wrote:
> Today, when a HVM (or PVH) guest triggers a hardware breakpoint while
> EFLAGS.TF is set, Xen incorrectly assumes that this is a single stepping
> exception and sets DR_STEP in dr6 in addition to DR_TRAP<n>.
> 
> This causes problems with Linux HW breakpoint handler, which ignores
> DR_TRAP<n> bits when DR_STEP is set.  This prevents user-mode debuggers
> from recognizing hardware breakpoints if EFLAGS.TF is set.
> 
> Fix this by not setting DR_STEP in {vmx,svm}_inject_event, unless the
> emulator explicitly signals the single-stepping mode via the newly added
> "singlestep" boolean field of struct x86_event.

s/the newly added "singlestep" boolean field/the 'extra' field/.  oops.

> 
> Fixes: 8b831f4189 ("x86: single step after instruction emulation")
> Signed-off-by: Jinoh Kang <jinoh.kang.kr@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/emulate.c             |  3 ++-
>  xen/arch/x86/hvm/svm/svm.c             |  6 +++---
>  xen/arch/x86/hvm/vmx/vmx.c             |  6 +++---
>  xen/arch/x86/include/asm/hvm/hvm.h     | 12 ++++++++++++
>  xen/arch/x86/mm/shadow/multi.c         |  5 +++--
>  xen/arch/x86/x86_emulate/x86_emulate.h |  4 +++-
>  6 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 9b6e4c8bc61b..5ad372466e1d 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -26,6 +26,7 @@
>  #include <asm/hvm/support.h>
>  #include <asm/iocap.h>
>  #include <asm/vm_event.h>
> +#include <asm/debugreg.h>
>  
>  struct hvmemul_cache
>  {
> @@ -2673,7 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>      }
>  
>      if ( hvmemul_ctxt->ctxt.retire.singlestep )
> -        hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +        hvm_inject_debug_exception(DR_STEP);
>  
>      new_intr_shadow = hvmemul_ctxt->intr_shadow;
>  
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index d5e8cb0722ca..f25d6a53f092 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -96,7 +96,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, 
> unsigned int inst_len)
>      curr->arch.hvm.svm.vmcb->int_stat.intr_shadow = 0;
>  
>      if ( regs->eflags & X86_EFLAGS_TF )
> -        hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +        hvm_inject_debug_exception(DR_STEP);
>  }
>  
>  static void cf_check svm_cpu_down(void)
> @@ -1328,10 +1328,10 @@ static void cf_check svm_inject_event(const struct 
> x86_event *event)
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case X86_EXC_DB:
> -        if ( regs->eflags & X86_EFLAGS_TF )
> +        if ( event->extra )
>          {
>              __restore_debug_registers(vmcb, curr);
> -            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
> +            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->extra);
>          }
>          /* fall through */
>      case X86_EXC_BP:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 8823ca13e55d..1795b9479cf9 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2022,10 +2022,10 @@ static void cf_check vmx_inject_event(const struct 
> x86_event *event)
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case X86_EXC_DB:
> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> +        if ( event->extra )
>          {
>              __restore_debug_registers(curr);
> -            write_debugreg(6, read_debugreg(6) | DR_STEP);
> +            write_debugreg(6, read_debugreg(6) | event->extra);
>          }
>          if ( !nestedhvm_vcpu_in_guestmode(curr) ||
>               !nvmx_intercepts_exception(curr, X86_EXC_DB, _event.error_code) 
> )
> @@ -3068,7 +3068,7 @@ void update_guest_eip(void)
>      }
>  
>      if ( regs->eflags & X86_EFLAGS_TF )
> -        hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +        hvm_inject_debug_exception(DR_STEP);
>  }
>  
>  static void cf_check vmx_fpu_dirty_intercept(void)
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
> b/xen/arch/x86/include/asm/hvm/hvm.h
> index f3f6310ab684..6a0b9e3ff01e 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -538,6 +538,18 @@ static inline void hvm_inject_page_fault(int errcode, 
> unsigned long cr2)
>      hvm_inject_event(&event);
>  }
>  
> +static inline void hvm_inject_debug_exception(unsigned long pending_dbg)
> +{
> +    struct x86_event event = {
> +        .vector = X86_EXC_DB,
> +        .type = X86_EVENTTYPE_HW_EXCEPTION,
> +        .error_code = X86_EVENT_NO_EC,
> +        .extra = pending_dbg,
> +    };
> +
> +    hvm_inject_event(&event);
> +}
> +
>  static inline bool hvm_event_pending(const struct vcpu *v)
>  {
>      return alternative_call(hvm_funcs.event_pending, v);
> diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
> index cf74fdf5dda6..365af5169750 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -24,6 +24,7 @@
>  #include <asm/hvm/cacheattr.h>
>  #include <asm/mtrr.h>
>  #include <asm/guest_pt.h>
> +#include <asm/debugreg.h>
>  #include <public/sched.h>
>  #include "private.h"
>  #include "types.h"
> @@ -2788,7 +2789,7 @@ static int cf_check sh_page_fault(
>  #endif
>  
>      if ( emul_ctxt.ctxt.retire.singlestep )
> -        hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +        hvm_inject_debug_exception(DR_STEP);
>  
>  #if GUEST_PAGING_LEVELS == 3 /* PAE guest */
>      /*
> @@ -2829,7 +2830,7 @@ static int cf_check sh_page_fault(
>                  TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED);
>  
>                  if ( emul_ctxt.ctxt.retire.singlestep )
> -                    hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +                    hvm_inject_debug_exception(DR_STEP);
>  
>                  break; /* Don't emulate again if we failed! */
>              }
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h 
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index bad957f9bcb2..868a64ab20e6 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -78,7 +78,9 @@ struct x86_event {
>      uint8_t       type;         /* X86_EVENTTYPE_* */
>      uint8_t       insn_len;     /* Instruction length */
>      int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
> -    unsigned long extra;        /* CR2 if X86_EXC_PF h/w exception */
> +
> +    /* Type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */
> +    unsigned long extra;
>  };
>  
>  /*

-- 
Sincerely,
Jinoh Kang