Re: [PATCH 5/5] x86/pv: Rewrite %dr6 handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 01:21, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -729,6 +729,18 @@ static inline void pv_inject_hw_exception(unsigned int 
> vector, int errcode)
>      pv_inject_event(&event);
>  }
>  
> +static inline void pv_inject_DB(unsigned long pending_dbg)
> +{
> +    struct x86_event event = {
> +        .vector      = X86_EXC_DB,
> +        .type        = X86_EVENTTYPE_HW_EXCEPTION,
> +        .error_code  = X86_EVENT_NO_EC,
> +        .pending_dbg = pending_dbg,

This being a sub-field of an unnamed union, the build will break (also
in pv_inject_page_fault() then, for cr2 being switched at the same time)
once again for old enough gcc.

> --- a/xen/arch/x86/pv/emulate.c
> +++ b/xen/arch/x86/pv/emulate.c
> @@ -71,11 +71,9 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, 
> unsigned long rip)
>  {
>      regs->rip = rip;
>      regs->eflags &= ~X86_EFLAGS_RF;
> +
>      if ( regs->eflags & X86_EFLAGS_TF )
> -    {
> -        current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
> -        pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> -    }
> +        pv_inject_DB(X86_DR6_BS);
>  }

This (not your change, the construct) looks bogus at the first and second
glance, because of looking at EFLAGS.TF after emulation, when the initial
state of TF matters. It is only correct (at the third, closer look) because
the function presently is used only from paths not altering the guest's
EFLAGS. Do you think it would make sense to add a comment at this occasion?

Jan