Re: [PATCH 1/5] x86/svm: Cleanup vintr_t type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2026 16:02, Ross Lagerwall wrote:
> 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>
> ---
>  xen/arch/x86/hvm/svm/intr.c      | 16 +++++++--------
>  xen/arch/x86/hvm/svm/nestedsvm.c | 32 ++++++++++++++---------------
>  xen/arch/x86/hvm/svm/svm.c       | 18 ++++++++--------
>  xen/arch/x86/hvm/svm/vmcb.c      |  6 +++---
>  xen/arch/x86/hvm/svm/vmcb.h      | 35 ++++++++++++++++----------------
>  5 files changed, 52 insertions(+), 55 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
> index 4b0debfa9a2e..883fde873e73 100644
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -33,9 +33,9 @@ static void svm_inject_nmi(struct vcpu *v)
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>      intinfo_t event;
>  
> -    if ( vmcb->_vintr.fields.vnmi_enable )
> +    if ( vmcb->_vintr.vnmi_en )
>      {
> -        vmcb->_vintr.fields.vnmi_pending = true;
> +        vmcb->_vintr.vnmi_pending = true;
>          return;
>      }
>  
> @@ -90,7 +90,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct 
> hvm_intack intack)
>               */
>              ASSERT(gvmcb != NULL);
>              intr = vmcb_get_vintr(gvmcb);
> -            if ( intr.fields.irq )
> +            if ( intr.irq )
>                  return;
>          }
>      }
> @@ -119,10 +119,10 @@ static void svm_enable_intr_window(struct vcpu *v, 
> struct hvm_intack intack)
>          return;
>  
>      intr = vmcb_get_vintr(vmcb);
> -    intr.fields.irq     = 1;
> -    intr.fields.vector  = 0;
> -    intr.fields.prio    = intack.vector >> 4;
> -    intr.fields.ign_tpr = (intack.source != hvm_intsrc_lapic);
> +    intr.irq     = 1;

The field changes to bool - imo that means we ewant to use "true" here.

> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -442,7 +442,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct 
> cpu_user_regs *regs)
>      if ( !clean.tpr )
>      {
>          n2vmcb->_vintr = ns_vmcb->_vintr;
> -        n2vmcb->_vintr.fields.intr_masking = 1;
> +        n2vmcb->_vintr.intr_masking = 1;

Same here.

> @@ -652,7 +652,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs 
> *regs,
>      svm->ns_hap_enabled = vmcb_get_np(ns_vmcb);
>  
>      /* Remember the V_INTR_MASK in hostflags */
> -    svm->ns_hostflags.fields.vintrmask = 
> !!ns_vmcb->_vintr.fields.intr_masking;
> +    svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.intr_masking;

No need for !! anymore?

> @@ -738,8 +738,8 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct 
> cpu_user_regs *regs,
>      struct vmcb_struct *ns_vmcb;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>  
> -    if ( vmcb->_vintr.fields.vgif_enable )
> -        vmcb->_vintr.fields.vgif = 0;
> +    if ( vmcb->_vintr.vgif_en )
> +        vmcb->_vintr.vgif = 0;

As per above, "false" here? (And I'll stop enumerating those cases here,
there are more further down.)

> --- a/xen/arch/x86/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/hvm/svm/vmcb.h
> @@ -330,26 +330,25 @@ typedef union {
>  
>  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;
> +        uint8_t tpr;

While "unsigned int tpr:8" would be an option here, I don't mind the type
choice in this case.

> +        bool irq:1;
> +        bool vgif:1;
> +        bool :1;
> +        bool vnmi_pending:1;
> +        bool vnmi_blocking:1;
> +        uint8_t :3;
> +        uint8_t prio:4;

For these two (and two more below) I question it though: Why can't these
be unsigned int? There's no need to engage an extension here, is there?

> +        bool ign_tpr:1;
> +        uint8_t rsvd1:3;

Other reserved fields are unnamed. Can't this field's name also be dropped
as part of the tidying?

Jan