Re: [PATCH 3/5] x86/emul: Add a pending_dbg field to x86_emulate_ctxt.retire

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 01:21, Andrew Cooper wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -8379,7 +8379,10 @@ x86_emulate(
>      if ( !mode_64bit() )
>          _regs.r(ip) = (uint32_t)_regs.r(ip);
>  
> -    /* Should a singlestep #DB be raised? */
> +    if ( singlestep )
> +        ctxt->retire.pending_dbg |= X86_DR6_BS;

We set "singlestep" about first thing in the function. Is it really correct
to latch that into pending_dbg without regard to rc? (Perhaps yes, seeing
the comment next to the field declaration.)

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -588,15 +588,26 @@ struct x86_emulate_ctxt
>      /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */
>      unsigned int opcode;
>  
> -    /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */
> +    /*
> +     * Retirement state, set by the emulator (valid only on 
> X86EMUL_OKAY/DONE).
> +     *
> +     * TODO: all this state should be input/output from the VMCS PENDING_DBG,
> +     * INTERRUPTIBILITY and ACTIVITIY fields.
> +     */
>      union {
> -        uint8_t raw;
> +        unsigned long raw;
>          struct {
> +            /*
> +             * Accumulated %dr6 trap bits, positive polarity.  Should only be
> +             * interpreted in the case of X86EMUL_OKAY/DONE.
> +             */
> +            unsigned int pending_dbg;
> +
>              bool hlt:1;          /* Instruction HLTed. */
>              bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
>              bool sti:1;          /* Instruction sets STI irq shadow. */
>              bool unblock_nmi:1;  /* Instruction clears NMI blocking. */
> -            bool singlestep:1;   /* Singlestepping was active. */
> +            bool singlestep:1;   /* Singlestepping was active. (TODO, merge 
> into pending_dbg) */
>          };
>      } retire;
>  

DONE has wrongly made it into here, as pointed out for patch 1.

Jan