Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2023 17:26, Jinoh Kang wrote:
> @@ -62,9 +63,16 @@ void pv_inject_event(const struct x86_event *event)
>              error_code |= PFEC_user_mode;
>  
>          trace_pv_page_fault(event->cr2, error_code);
> -    }
> -    else
> +        break;
> +
> +    case X86_EXC_DB:
> +        curr->arch.dr6 |= event->pending_dbg;
> +        /* Fallthrough */

I guess I have another question here, perhaps more to Andrew: How come
this is just an OR? Not only with some of the bits having inverted sense
and earlier logic being ...

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>          return;
>      }
>  
> -    /* Save debug status register where guest OS can peek at it */
> -    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> -    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);

... an OR and an AND, but also with ...

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -78,7 +78,10 @@ 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 cr2;          /* Only for X86_EXC_PF h/w exception */
> +    union {
> +        unsigned long cr2;         /* #PF */
> +        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) 
> */

... the comment here saying "positive polarity", which I understand
to mean that inverted bits need inverting by the consumer of this
field. If this is solely because none of the inverted bits are
supported for PV, then I guess this wants a comment at the use site
(not the least because it would need adjusting as soon as one such
would become supported).

Jan