Moving them all together has several advantages:
 * Collects them all in one part of the struct
 * The `caps` field means that we can drop the "_supported" suffix, as it's
   clear what is meant.

Signed-off-by: George Dunlap <george.dunlap@cloud.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/hvm.c             |  6 +++---
 xen/arch/x86/hvm/svm/svm.c         |  2 +-
 xen/arch/x86/hvm/vlapic.c          |  4 ++--
 xen/arch/x86/hvm/vmx/vmcs.c        |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c         |  8 ++++----
 xen/arch/x86/include/asm/hvm/hvm.h | 29 ++++++++++++++---------------
 6 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ae9d4c4756..aa2f2d054a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -136,7 +136,7 @@ static struct notifier_block cpu_nfb = {
 
 static bool __init hap_supported(struct hvm_function_table *fns)
 {
-    if ( !fns->hap_supported )
+    if ( !fns->caps.hap )
     {
         printk("HVM: Hardware Assisted Paging (HAP) not detected\n");
         return false;
@@ -144,7 +144,7 @@ static bool __init hap_supported(struct hvm_function_table *fns)
 
     if ( !opt_hap_enabled )
     {
-        fns->hap_supported = 0;
+        fns->caps.hap = 0;
         printk("HVM: Hardware Assisted Paging (HAP) detected but disabled\n");
         return false;
     }
@@ -190,7 +190,7 @@ static int __init cf_check hvm_enable(void)
     }
 
     if ( !opt_altp2m_enabled )
-        hvm_funcs.altp2m_supported = 0;
+        hvm_funcs.caps.altp2m = 0;
 
     if ( opt_hvm_fep )
         warning_add(warning_hvm_fep);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 40bc1ffbc6..b551eac807 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2580,7 +2580,7 @@ const struct hvm_function_table * __init start_svm(void)
     if ( !printed )
         printk(" - none\n");
 
-    svm_function_table.hap_supported = cpu_has_svm_npt;
+    svm_function_table.caps.hap = cpu_has_svm_npt;
     svm_function_table.caps.hap_superpage_2mb = true;
     svm_function_table.caps.hap_superpage_1gb = cpu_has_page1gb;
 
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 71a4b954b0..dcbcf4a1fe 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1326,7 +1326,7 @@ int vlapic_has_pending_irq(struct vcpu *v)
     if ( irr == -1 )
         return -1;
 
-    if ( hvm_funcs.virtual_intr_delivery_enabled &&
+    if ( hvm_funcs.caps.virtual_intr_delivery &&
          !nestedhvm_vcpu_in_guestmode(v) )
         return irr;
 
@@ -1361,7 +1361,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack)
     int isr;
 
     if ( !force_ack &&
-         hvm_funcs.virtual_intr_delivery_enabled )
+         hvm_funcs.caps.virtual_intr_delivery )
         return 1;
 
     /* If there's no chance of using APIC assist then bail now. */
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 53f9d81aa9..aff69d5320 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -112,7 +112,7 @@ static int cf_check parse_ept_param_runtime(const char *s)
     struct domain *d;
     int val;
 
-    if ( !cpu_has_vmx_ept || !hvm_funcs.hap_supported ||
+    if ( !cpu_has_vmx_ept || !hvm_funcs.caps.hap ||
          !(hvm_funcs.caps.hap_superpage_2mb ||
            hvm_funcs.caps.hap_superpage_1gb) )
     {
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9cfc0140b4..4bcf436d2c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2963,7 +2963,7 @@ const struct hvm_function_table * __init start_vmx(void)
         return NULL;
     }
 
-    vmx_function_table.singlestep_supported = cpu_has_monitor_trap_flag;
+    vmx_function_table.caps.singlestep = cpu_has_monitor_trap_flag;
 
     if ( cpu_has_vmx_dt_exiting )
         vmx_function_table.set_descriptor_access_exiting =
@@ -2986,8 +2986,8 @@ const struct hvm_function_table * __init start_vmx(void)
                 printk("VMX: Disabling executable EPT superpages due to CVE-2018-12207\n");
         }
 
-        vmx_function_table.hap_supported = 1;
-        vmx_function_table.altp2m_supported = 1;
+        vmx_function_table.caps.hap = 1;
+        vmx_function_table.caps.altp2m = 1;
 
         vmx_function_table.caps.hap_superpage_2mb = cpu_has_vmx_ept_2mb;
         vmx_function_table.caps.hap_superpage_1gb = cpu_has_vmx_ept_1gb;
@@ -3000,7 +3000,7 @@ const struct hvm_function_table * __init start_vmx(void)
         vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
         vmx_function_table.process_isr = vmx_process_isr;
         vmx_function_table.handle_eoi = vmx_handle_eoi;
-        vmx_function_table.virtual_intr_delivery_enabled = true;
+        vmx_function_table.caps.virtual_intr_delivery = true;
     }
 
     if ( cpu_has_vmx_posted_intr_processing )
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index f50476f50f..bbd83a8275 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state {
 struct hvm_function_table {
     const char *name;
 
-    /* Support Hardware-Assisted Paging? */
-    bool hap_supported;
-
-    /* Necessary hardware support for alternate p2m's? */
-    bool altp2m_supported;
-    bool singlestep_supported;
-
-    /* Hardware virtual interrupt delivery enable? */
-    bool virtual_intr_delivery_enabled;
-
     struct {
         /* Indicate HAP capabilities. */
-        bool hap_superpage_1gb:1,
-            hap_superpage_2mb:1;
+        bool hap:1,
+             hap_superpage_1gb:1,
+             hap_superpage_2mb:1,
+
+            /* Altp2m capabilities */
+            altp2m:1,
+            singlestep:1,
+            
+            /* Hardware virtual interrupt delivery enable? */
+            virtual_intr_delivery;
+
     } caps;
 
     /*
@@ -642,18 +641,18 @@ static inline void hvm_enable_msr_interception(struct domain *d, uint32_t msr)
 
 static inline bool hvm_is_singlestep_supported(void)
 {
-    return hvm_funcs.singlestep_supported;
+    return hvm_funcs.caps.singlestep;
 }
 
 static inline bool hvm_hap_supported(void)
 {
-    return hvm_funcs.hap_supported;
+    return hvm_funcs.caps.hap;
 }
 
 /* returns true if hardware supports alternate p2m's */
 static inline bool hvm_altp2m_supported(void)
 {
-    return hvm_funcs.altp2m_supported;
+    return hvm_funcs.caps.altp2m;
 }
 
 /* updates the current hardware p2m */
-- 
2.25.1

On 06.02.2024 02:20, George Dunlap wrote:
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state {
>  struct hvm_function_table {
>      const char *name;
>  
> -    /* Support Hardware-Assisted Paging? */
> -    bool hap_supported;
> -
> -    /* Necessary hardware support for alternate p2m's? */
> -    bool altp2m_supported;
> -    bool singlestep_supported;
> -
> -    /* Hardware virtual interrupt delivery enable? */
> -    bool virtual_intr_delivery_enabled;
> -
>      struct {
>          /* Indicate HAP capabilities. */
> -        bool hap_superpage_1gb:1,
> -            hap_superpage_2mb:1;
> +        bool hap:1,
> +             hap_superpage_1gb:1,
> +             hap_superpage_2mb:1,
> +
> +            /* Altp2m capabilities */
> +            altp2m:1,
> +            singlestep:1,
> +            
> +            /* Hardware virtual interrupt delivery enable? */
> +            virtual_intr_delivery;
> +
>      } caps;

Nit (spotted only while looking at patch 6): You're adding a stray blank
line at the end of the structure. Further I expect virtual_intr_delivery
would also want to be just a single bit?

Jan

On Tue, Feb 20, 2024 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 06.02.2024 02:20, George Dunlap wrote:
> > --- a/xen/arch/x86/include/asm/hvm/hvm.h
> > +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> > @@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state {
> >  struct hvm_function_table {
> >      const char *name;
> >
> > -    /* Support Hardware-Assisted Paging? */
> > -    bool hap_supported;
> > -
> > -    /* Necessary hardware support for alternate p2m's? */
> > -    bool altp2m_supported;
> > -    bool singlestep_supported;
> > -
> > -    /* Hardware virtual interrupt delivery enable? */
> > -    bool virtual_intr_delivery_enabled;
> > -
> >      struct {
> >          /* Indicate HAP capabilities. */
> > -        bool hap_superpage_1gb:1,
> > -            hap_superpage_2mb:1;
> > +        bool hap:1,
> > +             hap_superpage_1gb:1,
> > +             hap_superpage_2mb:1,
> > +
> > +            /* Altp2m capabilities */
> > +            altp2m:1,
> > +            singlestep:1,
> > +
> > +            /* Hardware virtual interrupt delivery enable? */
> > +            virtual_intr_delivery;
> > +
> >      } caps;
>
> Nit (spotted only while looking at patch 6): You're adding a stray blank
> line at the end of the structure. Further I expect virtual_intr_delivery
> would also want to be just a single bit?

Oh yes, good catch.  (I kind of feel like ":1" should be the default
for bools, but hey...)

I'll fix up this and the 0/1 => false/true thing.

 -George

On 06.02.2024 02:20, George Dunlap wrote:
> @@ -144,7 +144,7 @@ static bool __init hap_supported(struct hvm_function_table *fns)
>  
>      if ( !opt_hap_enabled )
>      {
> -        fns->hap_supported = 0;
> +        fns->caps.hap = 0;

As you touch such, would you mind switching to true/false instead of
1/0 at this occasion?

> @@ -3000,7 +3000,7 @@ const struct hvm_function_table * __init start_vmx(void)
>          vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
>          vmx_function_table.process_isr = vmx_process_isr;
>          vmx_function_table.handle_eoi = vmx_handle_eoi;
> -        vmx_function_table.virtual_intr_delivery_enabled = true;
> +        vmx_function_table.caps.virtual_intr_delivery = true;

I'm unsure about this one - it had "enabled" in its name for a good
reason. Then again its (somewhat more involved) derivation from
other capability bits (and a command line option) isn't fundamentally
different from that of, say, hap_supported. Hence I guess with the
other item taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan