[PATCH 0/5] x86/svm: Cleanup types

[PATCH 0/5] x86/svm: Cleanup types

[Ross Lagerwall]

Hi,

This series completes the cleanup of the VMCB and related types that was
started several years ago. Andrew suggested doing this before going too
far down the Nested Virt path.

No functional change intended for this entire series.

Thanks,
Ross

Ross Lagerwall (5):
  x86/svm: Cleanup vintr_t type
  x86/svm: Remove ioio_info_t type
  x86/svm: Cleanup virt_ext_t type
  x86/svm: Use C99 types in VMCB struct
  x86/svm: Cleanup ns_hostflags type

 xen/arch/x86/hvm/svm/intr.c              |  16 +-
 xen/arch/x86/hvm/svm/nestedsvm.c         |  63 ++++----
 xen/arch/x86/hvm/svm/svm.c               |  20 ++-
 xen/arch/x86/hvm/svm/vmcb.c              |   8 +-
 xen/arch/x86/hvm/svm/vmcb.h              | 188 ++++++++++-------------
 xen/arch/x86/include/asm/hvm/svm-types.h |   9 +-
 6 files changed, 140 insertions(+), 164 deletions(-)

-- 
2.55.0

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

[Ross Lagerwall]

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@citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 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;
+    intr.vector  = 0;
+    intr.prio    = intack.vector >> 4;
+    intr.ign_tpr = (intack.source != hvm_intsrc_lapic);
     vmcb_set_vintr(vmcb, intr);
     vmcb_set_general1_intercepts(
         vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
@@ -158,7 +158,7 @@ void asmlinkage svm_intr_assist(void)
          * has vGIF, and vGIF is always activated when appropriate.
          */
         if ( intblk == hvm_intblk_nmi_iret &&
-             vmcb->_vintr.fields.vnmi_enable )
+             vmcb->_vintr.vnmi_en )
             intblk = hvm_intblk_none;
 
         if ( intblk == hvm_intblk_svm_gif )
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 81579370b8bf..65556136852b 100644
--- 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;
     }
 
     /* Interrupt state */
@@ -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;
 
     /* Save l1 guest state (= host state) */
     ret = nsvm_vcpu_hostsave(v, inst_len);
@@ -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;
     else
         svm->ns_gif = 0;
 
@@ -997,7 +997,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     /* Virtual Interrupts */
     ns_vmcb->_vintr = n2vmcb->_vintr;
     if ( !svm->ns_hostflags.fields.vintrmask )
-        ns_vmcb->_vintr.fields.intr_masking = 0;
+        ns_vmcb->_vintr.intr_masking = 0;
 
     /* Interrupt state */
     ns_vmcb->int_stat = n2vmcb->int_stat;
@@ -1217,8 +1217,8 @@ nestedsvm_vmexit_defer(struct vcpu *v,
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
     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;
     else
         svm->ns_gif = 0;
 
@@ -1485,8 +1485,8 @@ nestedsvm_gif_isset(struct vcpu *v)
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
 
     /* get the vmcb gif value if using vgif */
-    if ( vmcb->_vintr.fields.vgif_enable )
-        return vmcb->_vintr.fields.vgif;
+    if ( vmcb->_vintr.vgif_en )
+        return vmcb->_vintr.vgif;
     else
         return svm->ns_gif;
 }
@@ -1533,7 +1533,7 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
 
     /* After a CLGI no interrupts should come */
     intr = vmcb_get_vintr(vmcb);
-    intr.fields.irq = 0;
+    intr.irq = 0;
     general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
     vmcb_set_vintr(vmcb, intr);
     vmcb_set_general1_intercepts(vmcb, general1_intercepts);
@@ -1569,12 +1569,12 @@ void svm_nested_features_on_efer_update(struct vcpu *v)
             vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
 
-        if ( !vmcb->_vintr.fields.vgif_enable &&
+        if ( !vmcb->_vintr.vgif_en &&
              cpu_has_svm_vgif )
         {
             vintr = vmcb_get_vintr(vmcb);
-            vintr.fields.vgif = svm->ns_gif;
-            vintr.fields.vgif_enable = 1;
+            vintr.vgif = svm->ns_gif;
+            vintr.vgif_en = 1;
             vmcb_set_vintr(vmcb, vintr);
             general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
@@ -1593,11 +1593,11 @@ void svm_nested_features_on_efer_update(struct vcpu *v)
             vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
 
-        if ( vmcb->_vintr.fields.vgif_enable )
+        if ( vmcb->_vintr.vgif_en )
         {
             vintr = vmcb_get_vintr(vmcb);
-            svm->ns_gif = vintr.fields.vgif;
-            vintr.fields.vgif_enable = 0;
+            svm->ns_gif = vintr.vgif;
+            vintr.vgif_en = 0;
             vmcb_set_vintr(vmcb, vintr);
             general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 71e48351f22c..05e25764f4b3 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -498,8 +498,8 @@ static unsigned cf_check int svm_get_interrupt_shadow(struct vcpu *v)
     if ( vmcb->int_stat.intr_shadow )
         intr_shadow |= HVM_INTR_SHADOW_MOV_SS | HVM_INTR_SHADOW_STI;
 
-    if ( vmcb->_vintr.fields.vnmi_enable
-         ? vmcb->_vintr.fields.vnmi_blocking
+    if ( vmcb->_vintr.vnmi_en
+         ? vmcb->_vintr.vnmi_blocking
          : (vmcb_get_general1_intercepts(vmcb) & GENERAL1_INTERCEPT_IRET) )
         intr_shadow |= HVM_INTR_SHADOW_NMI;
 
@@ -515,8 +515,8 @@ static void cf_check svm_set_interrupt_shadow(
     vmcb->int_stat.intr_shadow =
         !!(intr_shadow & (HVM_INTR_SHADOW_MOV_SS|HVM_INTR_SHADOW_STI));
 
-    if ( vmcb->_vintr.fields.vnmi_enable )
-        vmcb->_vintr.fields.vnmi_blocking = block_nmi;
+    if ( vmcb->_vintr.vnmi_en )
+        vmcb->_vintr.vnmi_blocking = block_nmi;
     else
     {
         uint32_t gen1 = vmcb_get_general1_intercepts(vmcb);
@@ -961,8 +961,7 @@ static void noreturn cf_check svm_do_resume(void)
 
         /* Reflect the vlapic's TPR in the hardware vtpr */
         intr = vmcb_get_vintr(vmcb);
-        intr.fields.tpr =
-            (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
+        intr.tpr = (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
         vmcb_set_vintr(vmcb, intr);
     }
 
@@ -2538,7 +2537,7 @@ void asmlinkage svm_vmexit_handler(void)
     {
         intr = vmcb_get_vintr(vmcb);
         vlapic_set_reg(vlapic, APIC_TASKPRI,
-                   ((intr.fields.tpr & 0x0F) << 4) |
+                   ((intr.tpr & 0x0F) << 4) |
                    (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0x0F));
     }
 
@@ -2772,7 +2771,7 @@ void asmlinkage svm_vmexit_handler(void)
         u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
         intr = vmcb_get_vintr(vmcb);
 
-        intr.fields.irq = 0;
+        intr.irq = 0;
         general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
 
         vmcb_set_vintr(vmcb, intr);
@@ -3063,8 +3062,7 @@ void asmlinkage svm_vmexit_handler(void)
 
     /* The exit may have updated the TPR: reflect this in the hardware vtpr */
     intr = vmcb_get_vintr(vmcb);
-    intr.fields.tpr =
-        (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
+    intr.tpr = (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
     vmcb_set_vintr(vmcb, intr);
 }
 
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index a6f09672a7c5..753f5d295064 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -105,7 +105,7 @@ static int construct_vmcb(struct vcpu *v)
     vmcb->_iopm_base_pa = __pa(v->domain->arch.hvm.io_bitmap);
 
     /* Virtualise EFLAGS.IF and LAPIC TPR (CR8). */
-    vmcb->_vintr.fields.intr_masking = 1;
+    vmcb->_vintr.intr_masking = 1;
 
     /* Don't need to intercept RDTSC if CPU supports TSC rate scaling */
     if ( v->domain->arch.vtsc && !cpu_has_tsc_ratio )
@@ -192,7 +192,7 @@ static int construct_vmcb(struct vcpu *v)
     if ( default_xen_spec_ctrl == SPEC_CTRL_STIBP )
         v->arch.msrs->spec_ctrl.raw = SPEC_CTRL_STIBP;
 
-    vmcb->_vintr.fields.vnmi_enable = cpu_has_svm_vnmi;
+    vmcb->_vintr.vnmi_en = cpu_has_svm_vnmi;
 
     return 0;
 }
@@ -275,7 +275,7 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb_get_iopm_base_pa(vmcb), vmcb_get_msrpm_base_pa(vmcb),
            vmcb_get_tsc_offset(vmcb));
     printk("tlb_control = %#x vintr = %#"PRIx64" int_stat = %#"PRIx64"\n",
-           vmcb->tlb_control, vmcb_get_vintr(vmcb).bytes,
+           vmcb->tlb_control, vmcb_get_vintr(vmcb).raw,
            vmcb->int_stat.raw);
     printk("event_inj %016"PRIx64", valid? %d, ec? %d, type %u, vector %#x\n",
            vmcb->event_inj.raw, vmcb->event_inj.v,
diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
index 3760f71a8625..869d965ba398 100644
--- 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;
+        bool irq:1;
+        bool vgif:1;
+        bool :1;
+        bool vnmi_pending:1;
+        bool vnmi_blocking:1;
+        uint8_t :3;
+        uint8_t prio:4;
+        bool ign_tpr:1;
+        uint8_t rsvd1:3;
+        bool intr_masking:1;
+        bool vgif_en:1;
+        bool vnmi_en:1;
+        uint8_t :5;
+        uint8_t vector;
+    };
+    uint64_t raw;
 } vintr_t;
 
 typedef union
-- 
2.55.0

[PATCH 2/5] x86/svm: Remove ioio_info_t type

[Ross Lagerwall]

This type duplicates what is already in the VMCB struct. Remove it and
have the only user access the VMCB directly.

No functional change intended.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 10 ++++------
 xen/arch/x86/hvm/svm/vmcb.h      | 17 -----------------
 2 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 65556136852b..13a2144fdba4 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -823,18 +823,16 @@ nsvm_vmcb_guest_intercepts_msr(unsigned long *msr_bitmap,
 }
 
 static int
-nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, uint64_t exitinfo1)
+nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, struct vmcb_struct *vmcb)
 {
     unsigned long gfn = iopm_pa >> PAGE_SHIFT;
     unsigned long *io_bitmap;
-    ioio_info_t ioinfo;
     uint16_t port;
     unsigned int size;
     bool intercepted;
 
-    ioinfo.bytes = exitinfo1;
-    port = ioinfo.fields.port;
-    size = ioinfo.fields.sz32 ? 4 : ioinfo.fields.sz16 ? 2 : 1;
+    port = vmcb->ei.io.port;
+    size = vmcb->ei.io.bytes;
 
     switch ( port )
     {
@@ -945,7 +943,7 @@ nsvm_vmcb_guest_intercepts_exitcode(struct vcpu *v,
             break;
         ns_vmcb = nv->nv_vvmcx;
         vmexits = nsvm_vmcb_guest_intercepts_ioio(ns_vmcb->_iopm_base_pa,
-            ns_vmcb->exitinfo1);
+            ns_vmcb);
         if ( vmexits == NESTEDHVM_VMEXIT_HOST )
             return 0;
         break;
diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
index 869d965ba398..2fc10caec136 100644
--- a/xen/arch/x86/hvm/svm/vmcb.h
+++ b/xen/arch/x86/hvm/svm/vmcb.h
@@ -351,23 +351,6 @@ typedef union
     uint64_t raw;
 } vintr_t;
 
-typedef union
-{
-    u64 bytes;
-    struct
-    {
-        u64 type: 1;
-        u64 rsv0: 1;
-        u64 str:  1;
-        u64 rep:  1;
-        u64 sz8:  1;
-        u64 sz16: 1;
-        u64 sz32: 1;
-        u64 rsv1: 9;
-        u64 port: 16;
-    } fields;
-} ioio_info_t;
-
 typedef union
 {
     u64 bytes;
-- 
2.55.0

[PATCH 3/5] x86/svm: Cleanup virt_ext_t type

[Ross Lagerwall]

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@citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 11 +++++------
 xen/arch/x86/hvm/svm/svm.c       |  2 +-
 xen/arch/x86/hvm/svm/vmcb.c      |  2 +-
 xen/arch/x86/hvm/svm/vmcb.h      |  8 ++++----
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 13a2144fdba4..1ef4c5c83f16 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -457,8 +457,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     /* Pending Interrupts */
     n2vmcb->event_inj = ns_vmcb->event_inj;
 
-    n2vmcb->virt_ext.bytes =
-        n1vmcb->virt_ext.bytes | ns_vmcb->virt_ext.bytes;
+    n2vmcb->virt_ext.raw = n1vmcb->virt_ext.raw | ns_vmcb->virt_ext.raw;
 
     /* NextRIP - only evaluated on #VMEXIT. */
 
@@ -1556,11 +1555,11 @@ void svm_nested_features_on_efer_update(struct vcpu *v)
      */
     if ( nsvm_efer_svm_enabled(v) )
     {
-        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+        if ( !vmcb->virt_ext.vloadsave &&
              !paging_mode_shadow(v->domain) &&
              cpu_has_svm_vloadsave )
         {
-            vmcb->virt_ext.fields.vloadsave_enable = 1;
+            vmcb->virt_ext.vloadsave = 1;
             general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
                                      GENERAL2_INTERCEPT_VMSAVE);
@@ -1582,9 +1581,9 @@ void svm_nested_features_on_efer_update(struct vcpu *v)
     }
     else
     {
-        if ( vmcb->virt_ext.fields.vloadsave_enable )
+        if ( vmcb->virt_ext.vloadsave )
         {
-            vmcb->virt_ext.fields.vloadsave_enable = 0;
+            vmcb->virt_ext.vloadsave = 0;
             general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
                                     GENERAL2_INTERCEPT_VMSAVE);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 05e25764f4b3..8a7a58317d60 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1933,7 +1933,7 @@ static int cf_check svm_msr_write_intercept(
         vmcb_set_debugctlmsr(vmcb, msr_content);
         if ( !msr_content || !cpu_has_svm_lbrv )
             break;
-        vmcb->virt_ext.fields.lbr_enable = 1;
+        vmcb->virt_ext.lbr = 1;
         svm_disable_intercept_for_msr(v, MSR_IA32_DEBUGCTLMSR);
         svm_disable_intercept_for_msr(v, MSR_IA32_LASTBRANCHFROMIP);
         svm_disable_intercept_for_msr(v, MSR_IA32_LASTBRANCHTOIP);
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 753f5d295064..694166bc5f39 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -291,7 +291,7 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb_get_sev(vmcb)    ? " SEV"    : "",
            vmcb_get_sev_es(vmcb) ? " SEV_ES" : "");
     printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n",
-           vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes);
+           vmcb->virt_ext.vloadsave, vmcb->virt_ext.raw);
     printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = %#"PRIx64"\n",
            vmcb_get_cpl(vmcb), vmcb_get_efer(vmcb), vmcb->star, vmcb->lstar);
     printk("CR0 = 0x%016"PRIx64" CR2 = 0x%016"PRIx64"\n",
diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
index 2fc10caec136..34308f3b19f2 100644
--- a/xen/arch/x86/hvm/svm/vmcb.h
+++ b/xen/arch/x86/hvm/svm/vmcb.h
@@ -353,12 +353,12 @@ typedef union
 
 typedef union
 {
-    u64 bytes;
     struct
     {
-        u64 lbr_enable:1;
-        u64 vloadsave_enable:1;
-    } fields;
+        bool lbr:1;
+        bool vloadsave:1;
+    };
+    uint64_t raw;
 } virt_ext_t;
 
 typedef union
-- 
2.55.0

[PATCH 4/5] x86/svm: Use C99 types in VMCB struct

[Ross Lagerwall]

No functional change intended.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/vmcb.h | 128 ++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
index 34308f3b19f2..cf80c706bce4 100644
--- a/xen/arch/x86/hvm/svm/vmcb.h
+++ b/xen/arch/x86/hvm/svm/vmcb.h
@@ -387,24 +387,24 @@ typedef union
 
 struct vmcb_struct {
     /* Control area */
-    u32 _cr_intercepts;         /* offset 0x00 - cleanbit 0 */
-    u32 _dr_intercepts;         /* offset 0x04 - cleanbit 0 */
-    u32 _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
-    u32 _general1_intercepts;   /* offset 0x0C - cleanbit 0 */
-    u32 _general2_intercepts;   /* offset 0x10 - cleanbit 0 */
-    u32 _general3_intercepts;   /* offset 0x14 - cleanbit 0 */
-    u32 res01[9];
-    u16 _pause_filter_thresh;   /* offset 0x3C - cleanbit 0 */
-    u16 _pause_filter_count;    /* offset 0x3E - cleanbit 0 */
-    u64 _iopm_base_pa;          /* offset 0x40 - cleanbit 1 */
-    u64 _msrpm_base_pa;         /* offset 0x48 - cleanbit 1 */
-    u64 _tsc_offset;            /* offset 0x50 - cleanbit 0 */
-    u32 _asid;                  /* offset 0x58 - cleanbit 2 */
-    u8  tlb_control;            /* offset 0x5C - TLB_CTRL_* */
-    u8  res07[3];
+    uint32_t _cr_intercepts;         /* offset 0x00 - cleanbit 0 */
+    uint32_t _dr_intercepts;         /* offset 0x04 - cleanbit 0 */
+    uint32_t _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
+    uint32_t _general1_intercepts;   /* offset 0x0C - cleanbit 0 */
+    uint32_t _general2_intercepts;   /* offset 0x10 - cleanbit 0 */
+    uint32_t _general3_intercepts;   /* offset 0x14 - cleanbit 0 */
+    uint32_t res01[9];
+    uint16_t _pause_filter_thresh;   /* offset 0x3C - cleanbit 0 */
+    uint16_t _pause_filter_count;    /* offset 0x3E - cleanbit 0 */
+    uint64_t _iopm_base_pa;          /* offset 0x40 - cleanbit 1 */
+    uint64_t _msrpm_base_pa;         /* offset 0x48 - cleanbit 1 */
+    uint64_t _tsc_offset;            /* offset 0x50 - cleanbit 0 */
+    uint32_t _asid;                  /* offset 0x58 - cleanbit 2 */
+    uint8_t  tlb_control;            /* offset 0x5C - TLB_CTRL_* */
+    uint8_t  res07[3];
     vintr_t _vintr;             /* offset 0x60 - cleanbit 3 */
     intstat_t int_stat;         /* offset 0x68 */
-    u64 exitcode;               /* offset 0x70 */
+    uint64_t exitcode;               /* offset 0x70 */
     union {
         struct {
             uint64_t exitinfo1; /* offset 0x78 */
@@ -468,19 +468,19 @@ struct vmcb_struct {
         };
         uint64_t _np_ctrl;
     };
-    u64 res08[2];
+    uint64_t res08[2];
     intinfo_t event_inj;        /* offset 0xA8 */
-    u64 _h_cr3;                 /* offset 0xB0 - cleanbit 4 */
+    uint64_t _h_cr3;            /* offset 0xB0 - cleanbit 4 */
     virt_ext_t virt_ext;        /* offset 0xB8 */
     vmcbcleanbits_t cleanbits;  /* offset 0xC0 */
-    u32 res09;                  /* offset 0xC4 */
-    u64 nextrip;                /* offset 0xC8 */
-    u8  guest_ins_len;          /* offset 0xD0 */
-    u8  guest_ins[15];          /* offset 0xD1 */
-    u64 res10a[8];              /* offset 0xE0 */
-    u16 bus_lock_count;         /* offset 0x120 */
-    u16 res10b[3];              /* offset 0x122 */
-    u64 res10c[91];             /* offset 0x128 pad to save area */
+    uint32_t res09;             /* offset 0xC4 */
+    uint64_t nextrip;           /* offset 0xC8 */
+    uint8_t  guest_ins_len;     /* offset 0xD0 */
+    uint8_t  guest_ins[15];     /* offset 0xD1 */
+    uint64_t res10a[8];         /* offset 0xE0 */
+    uint16_t bus_lock_count;    /* offset 0x120 */
+    uint16_t res10b[3];         /* offset 0x122 */
+    uint64_t res10c[91];        /* offset 0x128 pad to save area */
 
     /* State Save area */
     union {
@@ -498,44 +498,44 @@ struct vmcb_struct {
     struct segment_register ldtr;
     struct segment_register idtr; /* cleanbit 7 */
     struct segment_register tr;
-    u64 res10[5];
-    u8 res11[3];
-    u8 _cpl;                    /* cleanbit 8 */
-    u32 res12;
-    u64 _efer;                  /* offset 0x400 + 0xD0 - cleanbit 5 */
-    u64 res13[14];
-    u64 _cr4;                   /* offset 0x400 + 0x148 - cleanbit 5 */
-    u64 _cr3;                   /* cleanbit 5 */
-    u64 _cr0;                   /* cleanbit 5 */
-    u64 _dr7;                   /* cleanbit 6 */
-    u64 _dr6;                   /* cleanbit 6 */
-    u64 rflags;
-    u64 rip;
-    u64 res14[11];
-    u64 rsp;
-    u64 _msr_s_cet;             /* offset 0x400 + 0x1E0 - cleanbit 12 */
-    u64 _ssp;                   /* offset 0x400 + 0x1E8   | */
-    u64 _msr_isst;              /* offset 0x400 + 0x1F0   v */
-    u64 rax;
-    u64 star;
-    u64 lstar;
-    u64 cstar;
-    u64 sfmask;
-    u64 kerngsbase;
-    u64 sysenter_cs;
-    u64 sysenter_esp;
-    u64 sysenter_eip;
-    u64 _cr2;                   /* cleanbit 9 */
-    u64 res16[4];
-    u64 _g_pat;                 /* cleanbit 4 */
-    u64 _debugctlmsr;           /* cleanbit 10 */
-    u64 _lastbranchfromip;      /* cleanbit 10 */
-    u64 _lastbranchtoip;        /* cleanbit 10 */
-    u64 _lastintfromip;         /* cleanbit 10 */
-    u64 _lastinttoip;           /* cleanbit 10 */
-    u64 res17[9];
-    u64 spec_ctrl;
-    u64 res18[291];
+    uint64_t res10[5];
+    uint8_t res11[3];
+    uint8_t _cpl;               /* cleanbit 8 */
+    uint32_t res12;
+    uint64_t _efer;             /* offset 0x400 + 0xD0 - cleanbit 5 */
+    uint64_t res13[14];
+    uint64_t _cr4;              /* offset 0x400 + 0x148 - cleanbit 5 */
+    uint64_t _cr3;              /* cleanbit 5 */
+    uint64_t _cr0;              /* cleanbit 5 */
+    uint64_t _dr7;              /* cleanbit 6 */
+    uint64_t _dr6;              /* cleanbit 6 */
+    uint64_t rflags;
+    uint64_t rip;
+    uint64_t res14[11];
+    uint64_t rsp;
+    uint64_t _msr_s_cet;        /* offset 0x400 + 0x1E0 - cleanbit 12 */
+    uint64_t _ssp;              /* offset 0x400 + 0x1E8   | */
+    uint64_t _msr_isst;         /* offset 0x400 + 0x1F0   v */
+    uint64_t rax;
+    uint64_t star;
+    uint64_t lstar;
+    uint64_t cstar;
+    uint64_t sfmask;
+    uint64_t kerngsbase;
+    uint64_t sysenter_cs;
+    uint64_t sysenter_esp;
+    uint64_t sysenter_eip;
+    uint64_t _cr2;              /* cleanbit 9 */
+    uint64_t res16[4];
+    uint64_t _g_pat;            /* cleanbit 4 */
+    uint64_t _debugctlmsr;      /* cleanbit 10 */
+    uint64_t _lastbranchfromip; /* cleanbit 10 */
+    uint64_t _lastbranchtoip;   /* cleanbit 10 */
+    uint64_t _lastintfromip;    /* cleanbit 10 */
+    uint64_t _lastinttoip;      /* cleanbit 10 */
+    uint64_t res17[9];
+    uint64_t spec_ctrl;
+    uint64_t res18[291];
 };
 
 struct vmcb_struct *alloc_vmcb(void);
-- 
2.55.0

[PATCH 5/5] x86/svm: Cleanup ns_hostflags type

[Ross Lagerwall]

Rearrange the union to drop the .fields infix, rename bytes to the more
common raw, and adjust types where appropriate. Adjust the users
accordingly.

No functional change intended.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c         | 12 ++++++------
 xen/arch/x86/include/asm/hvm/svm-types.h |  9 ++++-----
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1ef4c5c83f16..47704d8ded80 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -151,7 +151,7 @@ int cf_check nsvm_vcpu_reset(struct vcpu *v)
     svm->ns_vmcb_guestcr3 = 0;
     svm->ns_vmcb_hostcr3 = 0;
     svm->ns_asid = 0;
-    svm->ns_hostflags.bytes = 0;
+    svm->ns_hostflags.raw = 0;
     svm->ns_vmexit.exitinfo1 = 0;
     svm->ns_vmexit.exitinfo2 = 0;
 
@@ -182,7 +182,7 @@ static int nsvm_vcpu_hostsave(struct vcpu *v, unsigned int inst_len)
     n1vmcb->_cr4 = v->arch.hvm.guest_cr[4];
 
     /* Remember the host interrupt flag */
-    svm->ns_hostflags.fields.rflagsif = !!(n1vmcb->rflags & X86_EFLAGS_IF);
+    svm->ns_hostflags.rflagsif = !!(n1vmcb->rflags & X86_EFLAGS_IF);
 
     return 0;
 }
@@ -651,7 +651,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.intr_masking;
+    svm->ns_hostflags.vintrmask = !!ns_vmcb->_vintr.intr_masking;
 
     /* Save l1 guest state (= host state) */
     ret = nsvm_vcpu_hostsave(v, inst_len);
@@ -993,7 +993,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* Virtual Interrupts */
     ns_vmcb->_vintr = n2vmcb->_vintr;
-    if ( !svm->ns_hostflags.fields.vintrmask )
+    if ( !svm->ns_hostflags.vintrmask )
         ns_vmcb->_vintr.intr_masking = 0;
 
     /* Interrupt state */
@@ -1174,8 +1174,8 @@ enum hvm_intblk cf_check nsvm_intr_blocked(struct vcpu *v)
     {
         struct vmcb_struct *n2vmcb = nv->nv_n2vmcx;
 
-        if ( svm->ns_hostflags.fields.vintrmask &&
-             !svm->ns_hostflags.fields.rflagsif )
+        if ( svm->ns_hostflags.vintrmask &&
+             !svm->ns_hostflags.rflagsif )
             return hvm_intblk_rflags_ie;
 
         /* when l1 guest passes its devices through to the l2 guest
diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h b/xen/arch/x86/include/asm/hvm/svm-types.h
index beab9a3af203..71427b3c11d1 100644
--- a/xen/arch/x86/include/asm/hvm/svm-types.h
+++ b/xen/arch/x86/include/asm/hvm/svm-types.h
@@ -77,12 +77,11 @@ struct nestedsvm {
     } ns_vmexit;
 
     union {
-        uint32_t bytes;
         struct {
-            uint32_t rflagsif:1;
-            uint32_t vintrmask:1;
-            uint32_t reserved:30;
-        } fields;
+            bool rflagsif:1;
+            bool vintrmask:1;
+        };
+        uint32_t raw;
     } ns_hostflags;
 };
 
-- 
2.55.0

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

[Jason Andryuk]

On 2026-09-28 10: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@citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 2/5] x86/svm: Remove ioio_info_t type

[Jason Andryuk]

On 2026-09-28 10:02, Ross Lagerwall wrote:
> This type duplicates what is already in the VMCB struct. Remove it and
> have the only user access the VMCB directly.
> 
> No functional change intended.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 3/5] x86/svm: Cleanup virt_ext_t type

[Jason Andryuk]

On 2026-09-28 10: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@citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 4/5] x86/svm: Use C99 types in VMCB struct

[Jason Andryuk]

On 2026-09-28 10:02, Ross Lagerwall wrote:
> No functional change intended.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 5/5] x86/svm: Cleanup ns_hostflags type

[Jason Andryuk]

On 2026-09-28 10:02, Ross Lagerwall wrote:
> Rearrange the union to drop the .fields infix, rename bytes to the more
> common raw, and adjust types where appropriate. Adjust the users
> accordingly.
> 
> No functional change intended.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

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

[Jan Beulich]

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@citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  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

Re: [PATCH 2/5] x86/svm: Remove ioio_info_t type

[Andrew Cooper]

On 28/09/2026 3:02 pm, Ross Lagerwall wrote:
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index 65556136852b..13a2144fdba4 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -823,18 +823,16 @@ nsvm_vmcb_guest_intercepts_msr(unsigned long *msr_bitmap,
>  }
>  
>  static int
> -nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, uint64_t exitinfo1)
> +nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, struct vmcb_struct *vmcb)
>  {
>      unsigned long gfn = iopm_pa >> PAGE_SHIFT;
>      unsigned long *io_bitmap;
> -    ioio_info_t ioinfo;
>      uint16_t port;
>      unsigned int size;
>      bool intercepted;
>  
> -    ioinfo.bytes = exitinfo1;
> -    port = ioinfo.fields.port;
> -    size = ioinfo.fields.sz32 ? 4 : ioinfo.fields.sz16 ? 2 : 1;
> +    port = vmcb->ei.io.port;
> +    size = vmcb->ei.io.bytes;
>  
>      switch ( port )
>      {
> @@ -945,7 +943,7 @@ nsvm_vmcb_guest_intercepts_exitcode(struct vcpu *v,
>              break;
>          ns_vmcb = nv->nv_vvmcx;
>          vmexits = nsvm_vmcb_guest_intercepts_ioio(ns_vmcb->_iopm_base_pa,
> -            ns_vmcb->exitinfo1);
> +            ns_vmcb);

Given the single caller here, I'd drop the iopm_pa parameter too, and
have nsvm_vmcb_guest_intercepts_ioio() read it out of the VMCB.

~Andrew

Re: [PATCH 3/5] x86/svm: Cleanup virt_ext_t type

[Jan Beulich]

On 28.09.2026 16:02, Ross Lagerwall wrote:
> --- a/xen/arch/x86/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/hvm/svm/vmcb.h
> @@ -353,12 +353,12 @@ typedef union
>  
>  typedef union
>  {
> -    u64 bytes;
>      struct
>      {
> -        u64 lbr_enable:1;
> -        u64 vloadsave_enable:1;
> -    } fields;
> +        bool lbr:1;
> +        bool vloadsave:1;

Like for patch 1 - since you switch to bool, assignments using constants also
want to move to use of false/true.

Jan

Re: [PATCH 4/5] x86/svm: Use C99 types in VMCB struct

[Jan Beulich]

On 29.09.2026 02:00, Jason Andryuk wrote:
> On 2026-09-28 10:02, Ross Lagerwall wrote:
>> No functional change intended.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 5/5] x86/svm: Cleanup ns_hostflags type

[Jan Beulich]

On 28.09.2026 16:02, Ross Lagerwall wrote:
> @@ -182,7 +182,7 @@ static int nsvm_vcpu_hostsave(struct vcpu *v, unsigned int inst_len)
>      n1vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>  
>      /* Remember the host interrupt flag */
> -    svm->ns_hostflags.fields.rflagsif = !!(n1vmcb->rflags & X86_EFLAGS_IF);
> +    svm->ns_hostflags.rflagsif = !!(n1vmcb->rflags & X86_EFLAGS_IF);

No neeed for !! anymore.

> @@ -651,7 +651,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.intr_masking;
> +    svm->ns_hostflags.vintrmask = !!ns_vmcb->_vintr.intr_masking;

Same here.

Jan

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

[Ross Lagerwall]

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@citrix.com>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   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