Re: [PATCH v2] x86/svm: Cleanup vintr_t type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2026 00:56, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> Rearrange the union to drop the .fields infix, rename bytes to the more
> common raw, adjust types where appropriate, and simplify some names.
> Adjust the users accordingly.
> 
> No functional change intended.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with a suggestion (below). Also - doesn't this now need your S-o-b as well?

> @@ -153,15 +153,14 @@ void asmlinkage svm_intr_assist(void)
>          intblk = hvm_interrupt_blocked(v, intack);
>  
>          /*
> -         * When vNMI is active, NMIs can be injected by setting vnmi_pending
> +         * When vNMI is active, NMIs can be injected by setting nmi_pending
>           * and hardware will deliver them at the next appropriate 
> opportunity.
>           * Consider them not blocked, to avoid trying to open an NMI Window.
>           *
>           * Correctness here relies on the fact that all vNMI capable hardware
>           * has vGIF, and vGIF is always activated when appropriate.
>           */
> -        if ( intblk == hvm_intblk_nmi_iret &&
> -             vmcb->_vintr.fields.vnmi_enable )
> +        if ( intblk == hvm_intblk_nmi_iret && vmcb->_vintr.vnmi_en )
>              intblk = hvm_intblk_none;

In the comment, may I suggest to prefix nmi_pending with a dot, to clarify that
a struct/union field is meant?

> --- a/xen/arch/x86/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/hvm/svm/vmcb.h
> @@ -328,28 +328,25 @@ typedef union {
>      uint64_t raw;
>  } intstat_t;
>  
> -typedef union
> -{
> -    u64 bytes;
> -    struct
> -    {
> -        u64 tpr:          8;
> -        u64 irq:          1;
> -        u64 vgif:         1;
> -        u64 :             1;
> -        u64 vnmi_pending: 1;
> -        u64 vnmi_blocking:1;
> -        u64 :             3;
> -        u64 prio:         4;
> -        u64 ign_tpr:      1;
> -        u64 rsvd1:        3;
> -        u64 intr_masking: 1;
> -        u64 vgif_enable:  1;
> -        u64 vnmi_enable:  1;
> -        u64 :             5;
> -        u64 vector:       8;
> -        u64 rsvd3:       24;
> -    } fields;
> +typedef union {
> +    struct {         /* Cleanbit 3 (TPR),  APM Name */
> +        uint8_t tpr;                /* c   V_TPR */
> +        bool    intr_pending:1;     /* c   V_IRQ */
> +        bool    gif:1;              /*     VGIF */
> +        bool    :1;
> +        bool    nmi_pending:1;      /* c   V_NMI */
> +        bool    nmi_blocked:1;      /* c   V_NMI_MASK */
> +        uint8_t :3;
> +        uint8_t intr_prio:4;        /* c   V_INTR_PRIO */
> +        bool    ign_tpr:1;          /* c   V_IGN_TPR */
> +        uint8_t :3;
> +        bool    vintr_masking:1;    /* c   V_INTR_MASKING */
> +        bool    vgif_en:1;          /*     VGIF_EN */
> +        bool    vnmi_en:1;          /*     V_NMI_ENABLE */
> +        uint8_t :5;
> +        uint8_t vector;             /* c   V_INTR_VECTOR */
> +    };
> +    uint64_t raw;
>  } vintr_t;

As said in reply to Ross'es v1 - imo uint8_t bitfields would better be
avoided; afaict there's no issue with using unsigned int there. (While
I'd like us to avoid such unnecessary uses of compiler extensions, I'm
also not going to insist here, as we have too many other instances of
this same pattern.)

Jan