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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/29/26 5:05 PM, Jan Beulich wrote:
> 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.
OK.

> > --- 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?
Yes.

> > @@ -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?
This follows the same style from the previous cleanup to intinfo_t and parts of
vmcb_struct. I don't have a strong opinion about it but maybe Andrew does since
he did the previous cleanup?

> > +        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?
Yes, sure.

Ross