[PATCH 0/6] x86: Fix transient build breakage with featureset additions

[PATCH 0/6] x86: Fix transient build breakage with featureset additions

[Andrew Cooper]

See patch 5 for details.  Patch 6 is just an example that demonstrates the fix
working in practice.

Andrew Cooper (6):
  x86/cpu-policy: Drop build time cross-checks of featureset sizes
  x86/cpuid: Rename NCAPINTS to X86_NR_CAPS
  x86/cpuid: Rename FSCAPINTS to X86_NR_FEAT
  x86/cpu-policy: Split cpuid-consts.h out of cpu-policy.h
  x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES
  DO NOT APPLY: Example breakage

 xen/arch/x86/alternative.c             |  2 +-
 xen/arch/x86/cpu-policy.c              | 33 +++++++++++-----------
 xen/arch/x86/cpu/common.c              | 16 +++++------
 xen/arch/x86/cpuid.c                   |  2 +-
 xen/arch/x86/include/asm/cpufeature.h  |  2 +-
 xen/arch/x86/include/asm/cpufeatures.h | 11 +++-----
 xen/arch/x86/include/asm/cpuid.h       |  2 +-
 xen/arch/x86/sysctl.c                  |  8 +++---
 xen/include/xen/lib/x86/cpu-policy.h   | 29 ++++++--------------
 xen/include/xen/lib/x86/cpuid-consts.h | 38 ++++++++++++++++++++++++++
 xen/lib/x86/cpuid.c                    |  8 ++++--
 11 files changed, 88 insertions(+), 63 deletions(-)
 create mode 100644 xen/include/xen/lib/x86/cpuid-consts.h

-- 
2.30.2

[PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Andrew Cooper]

These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
code which looks like:

  uint32_t foo[1] = { 1, 2, 3 };

However, GCC 12 at least does now warn for this:

  foo.c:1:24: error: excess elements in array initializer [-Werror]
    884 | uint32_t foo[1] = { 1, 2, 3 };
        |                        ^
  foo.c:1:24: note: (near initialization for 'foo')

and has found other array length issues which we want to fix.  Drop the cross
check now tools can spot the problem case directly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/cpu-policy.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index ef6a2d0d180a..44c88debf958 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -883,12 +883,6 @@ void __init init_dom0_cpuid_policy(struct domain *d)
 
 static void __init __maybe_unused build_assertions(void)
 {
-    BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(pv_max_featuremask) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_max_featuremask) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_max_featuremask) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FSCAPINTS);
-
     /* Find some more clever allocation scheme if this trips. */
     BUILD_BUG_ON(sizeof(struct cpu_policy) > PAGE_SIZE);
 
-- 
2.30.2

[PATCH 2/6] x86/cpuid: Rename NCAPINTS to X86_NR_CAPS

[Andrew Cooper]

The latter is more legible, and consistent with X86_NR_{SYNTH,BUG} which
already exist.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/alternative.c             |  2 +-
 xen/arch/x86/cpu/common.c              | 12 ++++++------
 xen/arch/x86/cpuid.c                   |  2 +-
 xen/arch/x86/include/asm/cpufeature.h  |  2 +-
 xen/arch/x86/include/asm/cpufeatures.h |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 99482766b51f..0434030693a9 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -200,7 +200,7 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
 
         BUG_ON(a->repl_len > total_len);
         BUG_ON(total_len > sizeof(buf));
-        BUG_ON(a->cpuid >= NCAPINTS * 32);
+        BUG_ON(a->cpuid >= X86_NR_CAPS * 32);
 
         /*
          * Detect sequences of alt_instr's patching the same origin site, and
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index edc4db1335eb..1be049e332ce 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -55,8 +55,8 @@ unsigned int paddr_bits __read_mostly = 36;
 unsigned int hap_paddr_bits __read_mostly = 36;
 unsigned int vaddr_bits __read_mostly = VADDR_BITS;
 
-static unsigned int cleared_caps[NCAPINTS];
-static unsigned int forced_caps[NCAPINTS];
+static unsigned int cleared_caps[X86_NR_CAPS];
+static unsigned int forced_caps[X86_NR_CAPS];
 
 DEFINE_PER_CPU(bool, full_gdt_loaded);
 
@@ -501,7 +501,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
 
 #ifdef NOISY_CAPS
 	printk(KERN_DEBUG "CPU: After vendor identify, caps:");
-	for (i = 0; i < NCAPINTS; i++)
+	for (i = 0; i < X86_NR_CAPS; i++)
 		printk(" %08x", c->x86_capability[i]);
 	printk("\n");
 #endif
@@ -530,7 +530,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
 	for (i = 0; i < FSCAPINTS; ++i)
 		c->x86_capability[i] &= known_features[i];
 
-	for (i = 0 ; i < NCAPINTS ; ++i) {
+	for (i = 0 ; i < X86_NR_CAPS ; ++i) {
 		c->x86_capability[i] |= forced_caps[i];
 		c->x86_capability[i] &= ~cleared_caps[i];
 	}
@@ -548,7 +548,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
 
 #ifdef NOISY_CAPS
 	printk(KERN_DEBUG "CPU: After all inits, caps:");
-	for (i = 0; i < NCAPINTS; i++)
+	for (i = 0; i < X86_NR_CAPS; i++)
 		printk(" %08x", c->x86_capability[i]);
 	printk("\n");
 #endif
@@ -585,7 +585,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
 	 */
 	if ( c != &boot_cpu_data ) {
 		/* AND the already accumulated flags with these */
-		for ( i = 0 ; i < NCAPINTS ; i++ )
+		for ( i = 0 ; i < X86_NR_CAPS ; i++ )
 			boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
 
 		mcheck_init(c, false);
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 455a09b2dd22..fd8021c6f16c 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -19,7 +19,7 @@ bool recheck_cpu_features(unsigned int cpu)
 
     identify_cpu(&c);
 
-    for ( i = 0; i < NCAPINTS; ++i )
+    for ( i = 0; i < X86_NR_CAPS; ++i )
     {
         if ( !(~c.x86_capability[i] & bsp->x86_capability[i]) )
             continue;
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 4140ec0938b2..66bd4e296a18 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -26,7 +26,7 @@ struct cpuinfo_x86 {
     unsigned char x86_mask;
     int cpuid_level;                   /* Maximum supported CPUID level, -1=no CPUID */
     unsigned int extended_cpuid_level; /* Maximum supported CPUID extended level */
-    unsigned int x86_capability[NCAPINTS];
+    unsigned int x86_capability[X86_NR_CAPS];
     char x86_vendor_id[16];
     char x86_model_id[64];
     unsigned int x86_cache_size;       /* in KB - valid only when supported */
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index da0593de8542..e982ee920ce1 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -53,4 +53,4 @@ XEN_CPUFEATURE(IBPB_ENTRY_HVM,    X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen for
 #define X86_BUG_IBPB_NO_RET       X86_BUG( 3) /* IBPB doesn't flush the RSB/RAS */
 
 /* Total number of capability words, inc synth and bug words. */
-#define NCAPINTS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
+#define X86_NR_CAPS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
-- 
2.30.2

[PATCH 3/6] x86/cpuid: Rename FSCAPINTS to X86_NR_FEAT

[Andrew Cooper]

The latter is more legible, and consistent with X86_NR_{CAPS,SYNTH,BUG} which
already exist.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/cpu-policy.c              | 22 +++++++++++-----------
 xen/arch/x86/cpu/common.c              |  4 ++--
 xen/arch/x86/include/asm/cpufeatures.h |  8 ++++----
 xen/arch/x86/include/asm/cpuid.h       |  2 +-
 xen/arch/x86/sysctl.c                  |  8 ++++----
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 44c88debf958..774c512a03bd 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -130,8 +130,8 @@ static int __init cf_check parse_xen_cpuid(const char *s)
 custom_param("cpuid", parse_xen_cpuid);
 
 static bool __initdata dom0_cpuid_cmdline;
-static uint32_t __initdata dom0_enable_feat[FSCAPINTS];
-static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
+static uint32_t __initdata dom0_enable_feat[X86_NR_FEAT];
+static uint32_t __initdata dom0_disable_feat[X86_NR_FEAT];
 
 static void __init cf_check _parse_dom0_cpuid(unsigned int feat, bool val)
 {
@@ -158,10 +158,10 @@ static void sanitise_featureset(uint32_t *fs)
 {
     /* for_each_set_bit() uses unsigned longs.  Extend with zeroes. */
     uint32_t disabled_features[
-        ROUNDUP(FSCAPINTS, sizeof(unsigned long)/sizeof(uint32_t))] = {};
+        ROUNDUP(X86_NR_FEAT, sizeof(unsigned long)/sizeof(uint32_t))] = {};
     unsigned int i;
 
-    for ( i = 0; i < FSCAPINTS; ++i )
+    for ( i = 0; i < X86_NR_FEAT; ++i )
     {
         /* Clamp to known mask. */
         fs[i] &= known_features[i];
@@ -181,7 +181,7 @@ static void sanitise_featureset(uint32_t *fs)
 
         ASSERT(dfs); /* deep_features[] should guarentee this. */
 
-        for ( j = 0; j < FSCAPINTS; ++j )
+        for ( j = 0; j < X86_NR_FEAT; ++j )
         {
             fs[j] &= ~dfs[j];
             disabled_features[j] &= ~dfs[j];
@@ -476,7 +476,7 @@ static void __init guest_common_feature_adjustments(uint32_t *fs)
 static void __init calculate_pv_max_policy(void)
 {
     struct cpu_policy *p = &pv_max_cpu_policy;
-    uint32_t fs[FSCAPINTS];
+    uint32_t fs[X86_NR_FEAT];
     unsigned int i;
 
     *p = host_cpu_policy;
@@ -509,7 +509,7 @@ static void __init calculate_pv_max_policy(void)
 static void __init calculate_pv_def_policy(void)
 {
     struct cpu_policy *p = &pv_def_cpu_policy;
-    uint32_t fs[FSCAPINTS];
+    uint32_t fs[X86_NR_FEAT];
     unsigned int i;
 
     *p = pv_max_cpu_policy;
@@ -529,7 +529,7 @@ static void __init calculate_pv_def_policy(void)
 static void __init calculate_hvm_max_policy(void)
 {
     struct cpu_policy *p = &hvm_max_cpu_policy;
-    uint32_t fs[FSCAPINTS];
+    uint32_t fs[X86_NR_FEAT];
     unsigned int i;
     const uint32_t *mask;
 
@@ -625,7 +625,7 @@ static void __init calculate_hvm_max_policy(void)
 static void __init calculate_hvm_def_policy(void)
 {
     struct cpu_policy *p = &hvm_def_cpu_policy;
-    uint32_t fs[FSCAPINTS];
+    uint32_t fs[X86_NR_FEAT];
     unsigned int i;
     const uint32_t *mask;
 
@@ -723,7 +723,7 @@ void recalculate_cpuid_policy(struct domain *d)
     const struct cpu_policy *max = is_pv_domain(d)
         ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpu_policy : NULL)
         : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpu_policy : NULL);
-    uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
+    uint32_t fs[X86_NR_FEAT], max_fs[X86_NR_FEAT];
     unsigned int i;
 
     if ( !max )
@@ -864,7 +864,7 @@ void __init init_dom0_cpuid_policy(struct domain *d)
     /* Apply dom0-cpuid= command line settings, if provided. */
     if ( dom0_cpuid_cmdline )
     {
-        uint32_t fs[FSCAPINTS];
+        uint32_t fs[X86_NR_FEAT];
         unsigned int i;
 
         x86_cpu_policy_to_featureset(p, fs);
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 1be049e332ce..d12ccea20350 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -80,7 +80,7 @@ void __init setup_clear_cpu_cap(unsigned int cap)
 	if (!dfs)
 		return;
 
-	for (i = 0; i < FSCAPINTS; ++i) {
+	for (i = 0; i < X86_NR_FEAT; ++i) {
 		cleared_caps[i] |= dfs[i];
 		boot_cpu_data.x86_capability[i] &= ~dfs[i];
 		if (!(forced_caps[i] & dfs[i]))
@@ -527,7 +527,7 @@ void identify_cpu(struct cpuinfo_x86 *c)
 	 * The vendor-specific functions might have changed features.  Now
 	 * we do "generic changes."
 	 */
-	for (i = 0; i < FSCAPINTS; ++i)
+	for (i = 0; i < X86_NR_FEAT; ++i)
 		c->x86_capability[i] &= known_features[i];
 
 	for (i = 0 ; i < X86_NR_CAPS ; ++i) {
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index e982ee920ce1..408ab4ba16a5 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -5,11 +5,11 @@
 #include <xen/lib/x86/cpuid-autogen.h>
 
 /* Number of capability words covered by the featureset words. */
-#define FSCAPINTS FEATURESET_NR_ENTRIES
+#define X86_NR_FEAT FEATURESET_NR_ENTRIES
 
 /* Synthetic words follow the featureset words. */
 #define X86_NR_SYNTH 1
-#define X86_SYNTH(x) (FSCAPINTS * 32 + (x))
+#define X86_SYNTH(x) (X86_NR_FEAT * 32 + (x))
 
 /* Synthetic features */
 XEN_CPUFEATURE(CONSTANT_TSC,      X86_SYNTH( 0)) /* TSC ticks at a constant rate */
@@ -45,7 +45,7 @@ XEN_CPUFEATURE(IBPB_ENTRY_HVM,    X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen for
 
 /* Bug words follow the synthetic words. */
 #define X86_NR_BUG 1
-#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x))
+#define X86_BUG(x) ((X86_NR_FEAT + X86_NR_SYNTH) * 32 + (x))
 
 #define X86_BUG_FPU_PTRS          X86_BUG( 0) /* (F)X{SAVE,RSTOR} doesn't save/restore FOP/FIP/FDP. */
 #define X86_BUG_NULL_SEG          X86_BUG( 1) /* NULL-ing a selector preserves the base and limit. */
@@ -53,4 +53,4 @@ XEN_CPUFEATURE(IBPB_ENTRY_HVM,    X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen for
 #define X86_BUG_IBPB_NO_RET       X86_BUG( 3) /* IBPB doesn't flush the RSB/RAS */
 
 /* Total number of capability words, inc synth and bug words. */
-#define X86_NR_CAPS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
+#define X86_NR_CAPS (X86_NR_FEAT + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
diff --git a/xen/arch/x86/include/asm/cpuid.h b/xen/arch/x86/include/asm/cpuid.h
index b32ba0bbfe5c..85b6ca0edb91 100644
--- a/xen/arch/x86/include/asm/cpuid.h
+++ b/xen/arch/x86/include/asm/cpuid.h
@@ -10,7 +10,7 @@
 
 #include <public/sysctl.h>
 
-extern const uint32_t known_features[FSCAPINTS];
+extern const uint32_t known_features[X86_NR_FEAT];
 
 /*
  * Expected levelling capabilities (given cpuid vendor/family information),
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index c107f40c6283..9be0e796628c 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -308,13 +308,13 @@ long arch_do_sysctl(
 #endif
         };
         const struct cpu_policy *p = NULL;
-        uint32_t featureset[FSCAPINTS];
+        uint32_t featureset[X86_NR_FEAT];
         unsigned int nr;
 
         /* Request for maximum number of features? */
         if ( guest_handle_is_null(sysctl->u.cpu_featureset.features) )
         {
-            sysctl->u.cpu_featureset.nr_features = FSCAPINTS;
+            sysctl->u.cpu_featureset.nr_features = X86_NR_FEAT;
             if ( __copy_field_to_guest(u_sysctl, sysctl,
                                        u.cpu_featureset.nr_features) )
                 ret = -EFAULT;
@@ -323,7 +323,7 @@ long arch_do_sysctl(
 
         /* Clip the number of entries. */
         nr = min_t(unsigned int, sysctl->u.cpu_featureset.nr_features,
-                   FSCAPINTS);
+                   X86_NR_FEAT);
 
         /* Look up requested featureset. */
         if ( sysctl->u.cpu_featureset.index < ARRAY_SIZE(policy_table) )
@@ -352,7 +352,7 @@ long arch_do_sysctl(
             ret = -EFAULT;
 
         /* Inform the caller if there was more data to provide. */
-        if ( !ret && nr < FSCAPINTS )
+        if ( !ret && nr < X86_NR_FEAT )
             ret = -ENOBUFS;
 
         break;
-- 
2.30.2

[PATCH 4/6] x86/cpu-policy: Split cpuid-consts.h out of cpu-policy.h

[Andrew Cooper]

In order to disentangle X86_NR_FEAT from FEATURESET_NR_ENTRIES, we need to
adjust asm/cpufeatures.h's inclusion of cpuid-autogen.h, and including all of
cpu-policy.h ends with cyclic dependences and a mess.

Split the FEATURESET_* constants out into a new header.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

I don't particularly like this, but I can't find a better alternative.
---
 xen/include/xen/lib/x86/cpu-policy.h   | 19 +-------------
 xen/include/xen/lib/x86/cpuid-consts.h | 34 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 18 deletions(-)
 create mode 100644 xen/include/xen/lib/x86/cpuid-consts.h

diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index bfa425060464..e9bda14a7595 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -2,24 +2,7 @@
 #ifndef XEN_LIB_X86_POLICIES_H
 #define XEN_LIB_X86_POLICIES_H
 
-#include <xen/lib/x86/cpuid-autogen.h>
-
-#define FEATURESET_1d     0 /* 0x00000001.edx      */
-#define FEATURESET_1c     1 /* 0x00000001.ecx      */
-#define FEATURESET_e1d    2 /* 0x80000001.edx      */
-#define FEATURESET_e1c    3 /* 0x80000001.ecx      */
-#define FEATURESET_Da1    4 /* 0x0000000d:1.eax    */
-#define FEATURESET_7b0    5 /* 0x00000007:0.ebx    */
-#define FEATURESET_7c0    6 /* 0x00000007:0.ecx    */
-#define FEATURESET_e7d    7 /* 0x80000007.edx      */
-#define FEATURESET_e8b    8 /* 0x80000008.ebx      */
-#define FEATURESET_7d0    9 /* 0x00000007:0.edx    */
-#define FEATURESET_7a1   10 /* 0x00000007:1.eax    */
-#define FEATURESET_e21a  11 /* 0x80000021.eax      */
-#define FEATURESET_7b1   12 /* 0x00000007:1.ebx    */
-#define FEATURESET_7d2   13 /* 0x00000007:2.edx    */
-#define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
-#define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
+#include <xen/lib/x86/cpuid-consts.h>
 
 struct cpuid_leaf
 {
diff --git a/xen/include/xen/lib/x86/cpuid-consts.h b/xen/include/xen/lib/x86/cpuid-consts.h
new file mode 100644
index 000000000000..6ca8c39a3df4
--- /dev/null
+++ b/xen/include/xen/lib/x86/cpuid-consts.h
@@ -0,0 +1,34 @@
+/* Common data structures and functions consumed by hypervisor and toolstack */
+#ifndef XEN_LIB_X86_CONSTS_H
+#define XEN_LIB_X86_CONSTS_H
+
+#include <xen/lib/x86/cpuid-autogen.h>
+
+#define FEATURESET_1d     0 /* 0x00000001.edx      */
+#define FEATURESET_1c     1 /* 0x00000001.ecx      */
+#define FEATURESET_e1d    2 /* 0x80000001.edx      */
+#define FEATURESET_e1c    3 /* 0x80000001.ecx      */
+#define FEATURESET_Da1    4 /* 0x0000000d:1.eax    */
+#define FEATURESET_7b0    5 /* 0x00000007:0.ebx    */
+#define FEATURESET_7c0    6 /* 0x00000007:0.ecx    */
+#define FEATURESET_e7d    7 /* 0x80000007.edx      */
+#define FEATURESET_e8b    8 /* 0x80000008.ebx      */
+#define FEATURESET_7d0    9 /* 0x00000007:0.edx    */
+#define FEATURESET_7a1   10 /* 0x00000007:1.eax    */
+#define FEATURESET_e21a  11 /* 0x80000021.eax      */
+#define FEATURESET_7b1   12 /* 0x00000007:1.ebx    */
+#define FEATURESET_7d2   13 /* 0x00000007:2.edx    */
+#define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
+#define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
+
+#endif /* !XEN_LIB_X86_CONSTS_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.30.2

[PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Andrew Cooper]

When adding new words to a featureset, there is a reasonable amount of
boilerplate and it is preforable to split the addition into multiple patches.

GCC 12 spotted a real (transient) error which occurs when splitting additions
like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
highest numeric XEN_CPUFEATURE() value, and can be less than what the
FEATURESET_* constants suggest the length of a featureset bitmap ought to be.

This causes the policy <-> featureset converters to genuinely access
out-of-bounds on the featureset array.

Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
specifically to grow larger than FEATURESET_NR_ENTRIES.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

To preempt what I expect will be the first review question, no FEATURESET_*
can't become an enumeration, because the constants undergo token concatination
in the preprocess as part of making DECL_BITFIELD() work.
---
 xen/arch/x86/cpu-policy.c              | 7 +++++++
 xen/arch/x86/include/asm/cpufeatures.h | 5 +----
 xen/include/xen/lib/x86/cpu-policy.h   | 4 ++--
 xen/include/xen/lib/x86/cpuid-consts.h | 2 ++
 xen/lib/x86/cpuid.c                    | 6 +++---
 5 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 774c512a03bd..00416244a3d8 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -883,6 +883,13 @@ void __init init_dom0_cpuid_policy(struct domain *d)
 
 static void __init __maybe_unused build_assertions(void)
 {
+    /*
+     * Generally these are the same, but tend to differ when adding new
+     * infrastructure split across several patches.  Simply confirm that the
+     * gen-cpuid.py X86_FEATURE_* bits fit within the bitmaps we operate on.
+     */
+    BUILD_BUG_ON(FEATURESET_NR_ENTRIES > X86_NR_FEAT);
+
     /* Find some more clever allocation scheme if this trips. */
     BUILD_BUG_ON(sizeof(struct cpu_policy) > PAGE_SIZE);
 
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index 408ab4ba16a5..8989291bbfd6 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -2,10 +2,7 @@
  * Explicitly intended for multiple inclusion.
  */
 
-#include <xen/lib/x86/cpuid-autogen.h>
-
-/* Number of capability words covered by the featureset words. */
-#define X86_NR_FEAT FEATURESET_NR_ENTRIES
+#include <xen/lib/x86/cpuid-consts.h>
 
 /* Synthetic words follow the featureset words. */
 #define X86_NR_SYNTH 1
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index e9bda14a7595..01431de056c8 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -370,12 +370,12 @@ struct cpu_policy_errors
  * Copy the featureset words out of a cpu_policy object.
  */
 void x86_cpu_policy_to_featureset(const struct cpu_policy *p,
-                                  uint32_t fs[FEATURESET_NR_ENTRIES]);
+                                  uint32_t fs[X86_NR_FEAT]);
 
 /**
  * Copy the featureset words back into a cpu_policy object.
  */
-void x86_cpu_featureset_to_policy(const uint32_t fs[FEATURESET_NR_ENTRIES],
+void x86_cpu_featureset_to_policy(const uint32_t fs[X86_NR_FEAT],
                                   struct cpu_policy *p);
 
 static inline uint64_t cpu_policy_xcr0_max(const struct cpu_policy *p)
diff --git a/xen/include/xen/lib/x86/cpuid-consts.h b/xen/include/xen/lib/x86/cpuid-consts.h
index 6ca8c39a3df4..9fe931b8e31f 100644
--- a/xen/include/xen/lib/x86/cpuid-consts.h
+++ b/xen/include/xen/lib/x86/cpuid-consts.h
@@ -21,6 +21,8 @@
 #define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
 #define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
 
+#define X86_NR_FEAT (FEATURESET_7d1 + 1)
+
 #endif /* !XEN_LIB_X86_CONSTS_H */
 
 /*
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 68aafb404927..76f26e92af8d 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -61,7 +61,7 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor)
 }
 
 void x86_cpu_policy_to_featureset(
-    const struct cpu_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
+    const struct cpu_policy *p, uint32_t fs[X86_NR_FEAT])
 {
     fs[FEATURESET_1d]        = p->basic._1d;
     fs[FEATURESET_1c]        = p->basic._1c;
@@ -82,7 +82,7 @@ void x86_cpu_policy_to_featureset(
 }
 
 void x86_cpu_featureset_to_policy(
-    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpu_policy *p)
+    const uint32_t fs[X86_NR_FEAT], struct cpu_policy *p)
 {
     p->basic._1d             = fs[FEATURESET_1d];
     p->basic._1c             = fs[FEATURESET_1c];
@@ -285,7 +285,7 @@ const uint32_t *x86_cpu_policy_lookup_deep_deps(uint32_t feature)
     static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
     static const struct {
         uint32_t feature;
-        uint32_t fs[FEATURESET_NR_ENTRIES];
+        uint32_t fs[X86_NR_FEAT];
     } deep_deps[] = INIT_DEEP_DEPS;
     unsigned int start = 0, end = ARRAY_SIZE(deep_deps);
 
-- 
2.30.2

[PATCH 6/6] DO NOT APPLY: Example breakage

[Andrew Cooper]

Prior to disentangling X86_NR_FEAT from FEATURESET_NR_ENTRIES, GCC 12
correctly notices:

  lib/x86/cpuid.c: In function 'x86_cpu_policy_to_featureset':
  lib/x86/cpuid.c:82:7: error: array subscript 16 is outside array bounds of 'uint32_t[16]' {aka 'unsigned int[16]'} [-Werror=array-bounds=]
     82 |     fs[FEATURESET_7a2]       = p->feat._7a2;
        |     ~~^~~~~~~~~~~~~~~~
  lib/x86/cpuid.c:64:42: note: at offset 64 into object 'fs' of size [0, 64]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/include/xen/lib/x86/cpu-policy.h   | 6 +++++-
 xen/include/xen/lib/x86/cpuid-consts.h | 2 ++
 xen/lib/x86/cpuid.c                    | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index 01431de056c8..164b3f4aac13 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -192,7 +192,11 @@ struct cpu_policy
             };
 
             /* Subleaf 2. */
-            uint32_t /* a */:32, /* b */:32, /* c */:32;
+            union {
+                uint32_t _7a2;
+                struct { DECL_BITFIELD(7a2); };
+            };
+            uint32_t /* b */:32, /* c */:32;
             union {
                 uint32_t _7d2;
                 struct { DECL_BITFIELD(7d2); };
diff --git a/xen/include/xen/lib/x86/cpuid-consts.h b/xen/include/xen/lib/x86/cpuid-consts.h
index 9fe931b8e31f..5dd9727fec79 100644
--- a/xen/include/xen/lib/x86/cpuid-consts.h
+++ b/xen/include/xen/lib/x86/cpuid-consts.h
@@ -20,8 +20,10 @@
 #define FEATURESET_7d2   13 /* 0x00000007:2.edx    */
 #define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
 #define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
+#define FEATURESET_7a2   16
 
 #define X86_NR_FEAT (FEATURESET_7d1 + 1)
+//#define X86_NR_FEAT (FEATURESET_7a2 + 1)
 
 #endif /* !XEN_LIB_X86_CONSTS_H */
 
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 76f26e92af8d..90bc82a18c30 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -79,6 +79,7 @@ void x86_cpu_policy_to_featureset(
     fs[FEATURESET_7d2]       = p->feat._7d2;
     fs[FEATURESET_7c1]       = p->feat._7c1;
     fs[FEATURESET_7d1]       = p->feat._7d1;
+    fs[FEATURESET_7a2]       = p->feat._7a2;
 }
 
 void x86_cpu_featureset_to_policy(
@@ -100,6 +101,7 @@ void x86_cpu_featureset_to_policy(
     p->feat._7d2             = fs[FEATURESET_7d2];
     p->feat._7c1             = fs[FEATURESET_7c1];
     p->feat._7d1             = fs[FEATURESET_7d1];
+    p->feat._7a2             = fs[FEATURESET_7a2];
 }
 
 void x86_cpu_policy_recalc_synth(struct cpu_policy *p)
-- 
2.30.2

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich]

On 04.05.2023 21:39, Andrew Cooper wrote:
> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
> code which looks like:
> 
>   uint32_t foo[1] = { 1, 2, 3 };
> 
> However, GCC 12 at least does now warn for this:
> 
>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>     884 | uint32_t foo[1] = { 1, 2, 3 };
>         |                        ^
>   foo.c:1:24: note: (near initialization for 'foo')

I'm pretty sure all gcc versions we support diagnose such cases. In turn
the arrays in question don't have explicit dimensions at their
definition sites, and hence they derive their dimensions from their
initializers. So the build-time-checks are about the arrays in fact
obtaining the right dimensions, i.e. the initializers being suitable.

With the core part of the reasoning not being applicable, I'm afraid I
can't even say "okay with an adjusted description".

Jan

> and has found other array length issues which we want to fix.  Drop the cross
> check now tools can spot the problem case directly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>  xen/arch/x86/cpu-policy.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index ef6a2d0d180a..44c88debf958 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -883,12 +883,6 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>  
>  static void __init __maybe_unused build_assertions(void)
>  {
> -    BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS);
> -    BUILD_BUG_ON(ARRAY_SIZE(pv_max_featuremask) != FSCAPINTS);
> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_max_featuremask) != FSCAPINTS);
> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_max_featuremask) != FSCAPINTS);
> -    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FSCAPINTS);
> -
>      /* Find some more clever allocation scheme if this trips. */
>      BUILD_BUG_ON(sizeof(struct cpu_policy) > PAGE_SIZE);
>  

Re: [PATCH 2/6] x86/cpuid: Rename NCAPINTS to X86_NR_CAPS

[Jan Beulich]

On 04.05.2023 21:39, Andrew Cooper wrote:
> The latter is more legible, and consistent with X86_NR_{SYNTH,BUG} which
> already exist.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can live with this as-is, so
Acked-by: Jan Beulich <jbeulich@suse.com>
yet ...

> --- a/xen/arch/x86/include/asm/cpufeatures.h
> +++ b/xen/arch/x86/include/asm/cpufeatures.h
> @@ -53,4 +53,4 @@ XEN_CPUFEATURE(IBPB_ENTRY_HVM,    X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen for
>  #define X86_BUG_IBPB_NO_RET       X86_BUG( 3) /* IBPB doesn't flush the RSB/RAS */
>  
>  /* Total number of capability words, inc synth and bug words. */
> -#define NCAPINTS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
> +#define X86_NR_CAPS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */

... the way the value is computed suggests to me that "CAPS" (i.e.
"capabilities") isn't quite the right term. "features" sadly isn't, either
(or else I'd have suggested that without hesitating), as neither of the
two really fits the inclusion of "bugs", but feels - to me as a non-native
English speaker - still slightly better.

Then again "CAPS" fits x86_capability[] best ...

Jan

Re: [PATCH 3/6] x86/cpuid: Rename FSCAPINTS to X86_NR_FEAT

[Jan Beulich]

On 04.05.2023 21:39, Andrew Cooper wrote:
> The latter is more legible, and consistent with X86_NR_{CAPS,SYNTH,BUG} which
> already exist.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

And in the light of this the name choice in the earlier patch is probably
indeed the least bad one. (I'm sorry, I should have paid attention to the
title of this patch here when replying there.)

Jan

Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Jan Beulich]

On 04.05.2023 21:39, Andrew Cooper wrote:
> When adding new words to a featureset, there is a reasonable amount of
> boilerplate and it is preforable to split the addition into multiple patches.
> 
> GCC 12 spotted a real (transient) error which occurs when splitting additions
> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
> highest numeric XEN_CPUFEATURE() value, and can be less than what the
> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
> 
> This causes the policy <-> featureset converters to genuinely access
> out-of-bounds on the featureset array.
> 
> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
> specifically to grow larger than FEATURESET_NR_ENTRIES.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

While, like you, I could live with the previous patch even if I don't
particularly like it, I'm not convinced of the route you take here. Can't
we instead improve build-time checking, so the issue spotted late in the
build by gcc12 can be spotted earlier and/or be connected better to the
underlying reason?

One idea I have would deal with another aspect I don't like about our
present XEN_CPUFEATURE() as well: The *32 that's there in every use of
the macro. How about

XEN_CPUFEATURE(FSRCS,        10, 12) /*A  Fast Short REP CMPSB/SCASB */

as the common use and e.g.

XEN_CPUFEATURE(16)

or (if that ends up easier in gen-cpuid-py and/or the public header)
something like

XEN_CPUFEATURE(, 16, )

as the placeholder required for (at least trailing) unpopulated slots? Of
course the macro used may also be one of a different name, which may even
be necessary to keep the public header reasonably simple; maybe as much
as avoiding use of compiler extensions there. (This would then mean to
leave alone XEN_CPUFEATURE(), and my secondary adjustment would perhaps
become an independent change to make.)

> To preempt what I expect will be the first review question, no FEATURESET_*
> can't become an enumeration, because the constants undergo token concatination
> in the preprocess as part of making DECL_BITFIELD() work.

Just as a remark: I had trouble understanding this. Which was a result of
you referring to token concatenation being the problem (which is fine when
the results are enumerators), when really the issue is with the result of
the concatenation wanting to be expanded to a literal number.

Then again - do CPUID_BITFIELD_<n> really need to be named that way?
Couldn't they equally well be CPUID_BITFIELD_1d, CPUID_BITFIELD_e1c, and
alike, thus removing the need for intermediate macro expansion?

Jan

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Andrew Cooper]

On 08/05/2023 7:47 am, Jan Beulich wrote:
> On 04.05.2023 21:39, Andrew Cooper wrote:
>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>> code which looks like:
>>
>>   uint32_t foo[1] = { 1, 2, 3 };
>>
>> However, GCC 12 at least does now warn for this:
>>
>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>         |                        ^
>>   foo.c:1:24: note: (near initialization for 'foo')
> I'm pretty sure all gcc versions we support diagnose such cases. In turn
> the arrays in question don't have explicit dimensions at their
> definition sites, and hence they derive their dimensions from their
> initializers. So the build-time-checks are about the arrays in fact
> obtaining the right dimensions, i.e. the initializers being suitable.
>
> With the core part of the reasoning not being applicable, I'm afraid I
> can't even say "okay with an adjusted description".

Now I'm extra confused.

I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
when I was expecting one, and there was a bug in the original featureset
work caused by this going wrong.

But godbolt seems to agree that even GCC 4.1 notices.

Maybe it was some other error (C file not seeing the header properly?)
which disappeared across the upstream review?


Either way, these aren't appropriate, and need deleting before patch 5,
because the check is no longer valid when a featureset can be longer
than the autogen length.

~Andrew

Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Andrew Cooper]

On 08/05/2023 8:45 am, Jan Beulich wrote:
> On 04.05.2023 21:39, Andrew Cooper wrote:
>> When adding new words to a featureset, there is a reasonable amount of
>> boilerplate and it is preforable to split the addition into multiple patches.
>>
>> GCC 12 spotted a real (transient) error which occurs when splitting additions
>> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
>> highest numeric XEN_CPUFEATURE() value, and can be less than what the
>> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
>>
>> This causes the policy <-> featureset converters to genuinely access
>> out-of-bounds on the featureset array.
>>
>> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
>> specifically to grow larger than FEATURESET_NR_ENTRIES.
>>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> While, like you, I could live with the previous patch even if I don't
> particularly like it, I'm not convinced of the route you take here.

It's the route you tentatively agreed to in
https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82bad1@suse.com/

> Can't
> we instead improve build-time checking, so the issue spotted late in the
> build by gcc12 can be spotted earlier and/or be connected better to the
> underlying reason?

I don't understand what you mean by this.  For the transient period of
time, Xen's idea of a featureset *is* longer the autogen idea, hence the
work in this patch to decouple the two.

>
> One idea I have would deal with another aspect I don't like about our
> present XEN_CPUFEATURE() as well: The *32 that's there in every use of
> the macro. How about
>
> XEN_CPUFEATURE(FSRCS,        10, 12) /*A  Fast Short REP CMPSB/SCASB */
>
> as the common use and e.g.
>
> XEN_CPUFEATURE(16)
>
> or (if that ends up easier in gen-cpuid-py and/or the public header)
> something like
>
> XEN_CPUFEATURE(, 16, )
>
> as the placeholder required for (at least trailing) unpopulated slots? Of
> course the macro used may also be one of a different name, which may even
> be necessary to keep the public header reasonably simple; maybe as much
> as avoiding use of compiler extensions there. (This would then mean to
> leave alone XEN_CPUFEATURE(), and my secondary adjustment would perhaps
> become an independent change to make.)

Honestly, I don't want to hide the *32 part of the expression.  This
logic is already magic enough.

If we were to do something like this, I don't see what's wrong with just
having the value as a regular define at the end anyway.

One way or another with this approach, something needs updating in the
tail of cpufeatureset.h, and gen-cpuid.py can easily parse for a
specific named constant, and it will be less magic than overloading
XEN_CPUFEATURE().

>> To preempt what I expect will be the first review question, no FEATURESET_*
>> can't become an enumeration, because the constants undergo token concatination
>> in the preprocess as part of making DECL_BITFIELD() work.
> Just as a remark: I had trouble understanding this. Which was a result of
> you referring to token concatenation being the problem (which is fine when
> the results are enumerators), when really the issue is with the result of
> the concatenation wanting to be expanded to a literal number.
>
> Then again - do CPUID_BITFIELD_<n> really need to be named that way?
> Couldn't they equally well be CPUID_BITFIELD_1d, CPUID_BITFIELD_e1c, and
> alike, thus removing the need for intermediate macro expansion?

gen-cpuid.py doesn't know the short names; only Xen does, which is why
the expansion needs to know the name->word mapping.

I suppose this can be fixed, but it will require more magic comments and
more parsing to achieve.

~Andrew

Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Jan Beulich]

On 09.05.2023 16:03, Andrew Cooper wrote:
> On 08/05/2023 8:45 am, Jan Beulich wrote:
>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>> When adding new words to a featureset, there is a reasonable amount of
>>> boilerplate and it is preforable to split the addition into multiple patches.
>>>
>>> GCC 12 spotted a real (transient) error which occurs when splitting additions
>>> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
>>> highest numeric XEN_CPUFEATURE() value, and can be less than what the
>>> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
>>>
>>> This causes the policy <-> featureset converters to genuinely access
>>> out-of-bounds on the featureset array.
>>>
>>> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
>>> specifically to grow larger than FEATURESET_NR_ENTRIES.
>>>
>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> While, like you, I could live with the previous patch even if I don't
>> particularly like it, I'm not convinced of the route you take here.
> 
> It's the route you tentatively agreed to in
> https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82bad1@suse.com/

Right. Yet I deliberately said "may be the best" there, as something
better might turn up. And getting the two numbers to always agree, as
suggested, might end up being better.

>> Can't
>> we instead improve build-time checking, so the issue spotted late in the
>> build by gcc12 can be spotted earlier and/or be connected better to the
>> underlying reason?
> 
> I don't understand what you mean by this.  For the transient period of
> time, Xen's idea of a featureset *is* longer the autogen idea, hence the
> work in this patch to decouple the two.

Well, this part of my reply was just aiming at diagnosing the issue
as early as possible and as clearly as possible, such that one can
easily and quickly adjust whatever is missing in a change being worked
on. The main goal of course needs to be that this can't easily go
entirely unnoticed (as had happened, which has prompted this attempt
of yours of addressing the issue). I.e. diagnosing late is still far
better than failing to (without the compiler spotting it) altogether.

>> One idea I have would deal with another aspect I don't like about our
>> present XEN_CPUFEATURE() as well: The *32 that's there in every use of
>> the macro. How about
>>
>> XEN_CPUFEATURE(FSRCS,        10, 12) /*A  Fast Short REP CMPSB/SCASB */
>>
>> as the common use and e.g.
>>
>> XEN_CPUFEATURE(16)
>>
>> or (if that ends up easier in gen-cpuid-py and/or the public header)
>> something like
>>
>> XEN_CPUFEATURE(, 16, )
>>
>> as the placeholder required for (at least trailing) unpopulated slots? Of
>> course the macro used may also be one of a different name, which may even
>> be necessary to keep the public header reasonably simple; maybe as much
>> as avoiding use of compiler extensions there. (This would then mean to
>> leave alone XEN_CPUFEATURE(), and my secondary adjustment would perhaps
>> become an independent change to make.)
> 
> Honestly, I don't want to hide the *32 part of the expression.  This
> logic is already magic enough.

Well, I certainly wouldn't insist, but to me it looks pretty odd to have
it on all the lines.

> If we were to do something like this, I don't see what's wrong with just
> having the value as a regular define at the end anyway.
> 
> One way or another with this approach, something needs updating in the
> tail of cpufeatureset.h, and gen-cpuid.py can easily parse for a
> specific named constant, and it will be less magic than overloading
> XEN_CPUFEATURE().

If less overloading is deemed better - fine with me. Looking at the
script I wasn't sure hunting for an entirely different construct would
end up being more tidy.

What isn't really clear to me from your reply: Are you okay with trying
such an alternative approach? Or are you opposed to it? Or something in
the middle, like you being okay, but only if it's not you to actually
try it out?

>>> To preempt what I expect will be the first review question, no FEATURESET_*
>>> can't become an enumeration, because the constants undergo token concatination
>>> in the preprocess as part of making DECL_BITFIELD() work.
>> Just as a remark: I had trouble understanding this. Which was a result of
>> you referring to token concatenation being the problem (which is fine when
>> the results are enumerators), when really the issue is with the result of
>> the concatenation wanting to be expanded to a literal number.
>>
>> Then again - do CPUID_BITFIELD_<n> really need to be named that way?
>> Couldn't they equally well be CPUID_BITFIELD_1d, CPUID_BITFIELD_e1c, and
>> alike, thus removing the need for intermediate macro expansion?
> 
> gen-cpuid.py doesn't know the short names; only Xen does, which is why
> the expansion needs to know the name->word mapping.
> 
> I suppose this can be fixed, but it will require more magic comments and
> more parsing to achieve.

Okay, let's leave this entire aspect aside for now. It started from a
not-to-be-committed remark only anyway.

Jan

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich]

On 09.05.2023 15:04, Andrew Cooper wrote:
> On 08/05/2023 7:47 am, Jan Beulich wrote:
>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>> code which looks like:
>>>
>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>
>>> However, GCC 12 at least does now warn for this:
>>>
>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>         |                        ^
>>>   foo.c:1:24: note: (near initialization for 'foo')
>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>> the arrays in question don't have explicit dimensions at their
>> definition sites, and hence they derive their dimensions from their
>> initializers. So the build-time-checks are about the arrays in fact
>> obtaining the right dimensions, i.e. the initializers being suitable.
>>
>> With the core part of the reasoning not being applicable, I'm afraid I
>> can't even say "okay with an adjusted description".
> 
> Now I'm extra confused.
> 
> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
> when I was expecting one, and there was a bug in the original featureset
> work caused by this going wrong.
> 
> But godbolt seems to agree that even GCC 4.1 notices.
> 
> Maybe it was some other error (C file not seeing the header properly?)
> which disappeared across the upstream review?

Or maybe, by mistake, too few initializer fields? But what exactly it
was probably doesn't matter. If this patch is to stay (see below), some
different description will be needed anyway (or the change be folded
into the one actually invalidating those BUILD_BUG_ON()s).

> Either way, these aren't appropriate, and need deleting before patch 5,
> because the check is no longer valid when a featureset can be longer
> than the autogen length.

Well, they need deleting if we stick to the approach chosen there right
now. If we switched to my proposed alternative, they better would stay.

Jan

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Andrew Cooper]

On 09/05/2023 3:28 pm, Jan Beulich wrote:
> On 09.05.2023 15:04, Andrew Cooper wrote:
>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>>> code which looks like:
>>>>
>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>
>>>> However, GCC 12 at least does now warn for this:
>>>>
>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>         |                        ^
>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>> the arrays in question don't have explicit dimensions at their
>>> definition sites, and hence they derive their dimensions from their
>>> initializers. So the build-time-checks are about the arrays in fact
>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>
>>> With the core part of the reasoning not being applicable, I'm afraid I
>>> can't even say "okay with an adjusted description".
>> Now I'm extra confused.
>>
>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>> when I was expecting one, and there was a bug in the original featureset
>> work caused by this going wrong.
>>
>> But godbolt seems to agree that even GCC 4.1 notices.
>>
>> Maybe it was some other error (C file not seeing the header properly?)
>> which disappeared across the upstream review?
> Or maybe, by mistake, too few initializer fields? But what exactly it
> was probably doesn't matter. If this patch is to stay (see below), some
> different description will be needed anyway (or the change be folded
> into the one actually invalidating those BUILD_BUG_ON()s).
>
>> Either way, these aren't appropriate, and need deleting before patch 5,
>> because the check is no longer valid when a featureset can be longer
>> than the autogen length.
> Well, they need deleting if we stick to the approach chosen there right
> now. If we switched to my proposed alternative, they better would stay.

Given that all versions of GCC do warn, I don't see any justification
for them to stay.

i.e. this should be committed, even if the commit message says "no idea
what they were taken originally, but they're superfluous in the logic as
it exists today".

~Andrew

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich]

On 09.05.2023 17:59, Andrew Cooper wrote:
> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>>>> code which looks like:
>>>>>
>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>
>>>>> However, GCC 12 at least does now warn for this:
>>>>>
>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>         |                        ^
>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>> the arrays in question don't have explicit dimensions at their
>>>> definition sites, and hence they derive their dimensions from their
>>>> initializers. So the build-time-checks are about the arrays in fact
>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>
>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>> can't even say "okay with an adjusted description".
>>> Now I'm extra confused.
>>>
>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>> when I was expecting one, and there was a bug in the original featureset
>>> work caused by this going wrong.
>>>
>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>
>>> Maybe it was some other error (C file not seeing the header properly?)
>>> which disappeared across the upstream review?
>> Or maybe, by mistake, too few initializer fields? But what exactly it
>> was probably doesn't matter. If this patch is to stay (see below), some
>> different description will be needed anyway (or the change be folded
>> into the one actually invalidating those BUILD_BUG_ON()s).
>>
>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>> because the check is no longer valid when a featureset can be longer
>>> than the autogen length.
>> Well, they need deleting if we stick to the approach chosen there right
>> now. If we switched to my proposed alternative, they better would stay.
> 
> Given that all versions of GCC do warn, I don't see any justification
> for them to stay.

All versions warn when the variable declarations / definitions have a
dimension specified, and then there are excess initializers. Yet none
of the five affected arrays have a dimension specified in their
definitions.

Even if dimensions were added, we'd then have only covered half of
what the BUILD_BUG_ON()s cover right now: There could then be fewer
than intended initializer fields, and things may still be screwed. I
think it was for this very reason why BUILD_BUG_ON() was chosen.

Jan

> i.e. this should be committed, even if the commit message says "no idea
> what they were taken originally, but they're superfluous in the logic as
> it exists today".
> 
> ~Andrew

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Andrew Cooper]

On 09/05/2023 5:15 pm, Jan Beulich wrote:
> On 09.05.2023 17:59, Andrew Cooper wrote:
>> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>>>>> code which looks like:
>>>>>>
>>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>>
>>>>>> However, GCC 12 at least does now warn for this:
>>>>>>
>>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>>         |                        ^
>>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>>> the arrays in question don't have explicit dimensions at their
>>>>> definition sites, and hence they derive their dimensions from their
>>>>> initializers. So the build-time-checks are about the arrays in fact
>>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>>
>>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>>> can't even say "okay with an adjusted description".
>>>> Now I'm extra confused.
>>>>
>>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>>> when I was expecting one, and there was a bug in the original featureset
>>>> work caused by this going wrong.
>>>>
>>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>>
>>>> Maybe it was some other error (C file not seeing the header properly?)
>>>> which disappeared across the upstream review?
>>> Or maybe, by mistake, too few initializer fields? But what exactly it
>>> was probably doesn't matter. If this patch is to stay (see below), some
>>> different description will be needed anyway (or the change be folded
>>> into the one actually invalidating those BUILD_BUG_ON()s).
>>>
>>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>>> because the check is no longer valid when a featureset can be longer
>>>> than the autogen length.
>>> Well, they need deleting if we stick to the approach chosen there right
>>> now. If we switched to my proposed alternative, they better would stay.
>> Given that all versions of GCC do warn, I don't see any justification
>> for them to stay.
> All versions warn when the variable declarations / definitions have a
> dimension specified, and then there are excess initializers. Yet none
> of the five affected arrays have a dimension specified in their
> definitions.
>
> Even if dimensions were added, we'd then have only covered half of
> what the BUILD_BUG_ON()s cover right now: There could then be fewer
> than intended initializer fields, and things may still be screwed. I
> think it was for this very reason why BUILD_BUG_ON() was chosen.

???

The dimensions already exist, as proved by the fact GCC can spot the
violation.

On the other hand, zero extending a featureset is explicitly how they're
supposed to work.  How do you think Xapi has coped with migration
compatibility over the years, not to mention the microcode changes which
lengthen a featureset?

So no, there was never any problem with constructs of the form uint32_t
foo[10] = { 1, } in the first place.

The BUILD_BUG_ON()s therefore serve no purpose at all.

~Andrew

Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Andrew Cooper]

On 09/05/2023 3:24 pm, Jan Beulich wrote:
> On 09.05.2023 16:03, Andrew Cooper wrote:
>> On 08/05/2023 8:45 am, Jan Beulich wrote:
>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>> When adding new words to a featureset, there is a reasonable amount of
>>>> boilerplate and it is preforable to split the addition into multiple patches.
>>>>
>>>> GCC 12 spotted a real (transient) error which occurs when splitting additions
>>>> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
>>>> highest numeric XEN_CPUFEATURE() value, and can be less than what the
>>>> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
>>>>
>>>> This causes the policy <-> featureset converters to genuinely access
>>>> out-of-bounds on the featureset array.
>>>>
>>>> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
>>>> specifically to grow larger than FEATURESET_NR_ENTRIES.
>>>>
>>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> While, like you, I could live with the previous patch even if I don't
>>> particularly like it, I'm not convinced of the route you take here.
>> It's the route you tentatively agreed to in
>> https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82bad1@suse.com/
> Right. Yet I deliberately said "may be the best" there, as something
> better might turn up. And getting the two numbers to always agree, as
> suggested, might end up being better.

Then don't write "yes" if what you actually mean is "I'd prefer a
different option if possible", which is a "no".

I cannot read your mind, and we both know I do not have time to waste on
this task.

And now I have to go and spend yet more time doing it differently.

~Andrew

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich]

On 10.05.2023 17:06, Andrew Cooper wrote:
> On 09/05/2023 5:15 pm, Jan Beulich wrote:
>> On 09.05.2023 17:59, Andrew Cooper wrote:
>>> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>>>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>>>>>> code which looks like:
>>>>>>>
>>>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>
>>>>>>> However, GCC 12 at least does now warn for this:
>>>>>>>
>>>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>         |                        ^
>>>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>>>> the arrays in question don't have explicit dimensions at their
>>>>>> definition sites, and hence they derive their dimensions from their
>>>>>> initializers. So the build-time-checks are about the arrays in fact
>>>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>>>
>>>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>>>> can't even say "okay with an adjusted description".
>>>>> Now I'm extra confused.
>>>>>
>>>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>>>> when I was expecting one, and there was a bug in the original featureset
>>>>> work caused by this going wrong.
>>>>>
>>>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>>>
>>>>> Maybe it was some other error (C file not seeing the header properly?)
>>>>> which disappeared across the upstream review?
>>>> Or maybe, by mistake, too few initializer fields? But what exactly it
>>>> was probably doesn't matter. If this patch is to stay (see below), some
>>>> different description will be needed anyway (or the change be folded
>>>> into the one actually invalidating those BUILD_BUG_ON()s).
>>>>
>>>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>>>> because the check is no longer valid when a featureset can be longer
>>>>> than the autogen length.
>>>> Well, they need deleting if we stick to the approach chosen there right
>>>> now. If we switched to my proposed alternative, they better would stay.
>>> Given that all versions of GCC do warn, I don't see any justification
>>> for them to stay.
>> All versions warn when the variable declarations / definitions have a
>> dimension specified, and then there are excess initializers. Yet none
>> of the five affected arrays have a dimension specified in their
>> definitions.
>>
>> Even if dimensions were added, we'd then have only covered half of
>> what the BUILD_BUG_ON()s cover right now: There could then be fewer
>> than intended initializer fields, and things may still be screwed. I
>> think it was for this very reason why BUILD_BUG_ON() was chosen.
> 
> ???
> 
> The dimensions already exist, as proved by the fact GCC can spot the
> violation.

Where? Quoting cpu-policy.c:

const uint32_t known_features[] = INIT_KNOWN_FEATURES;

static const uint32_t __initconst pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES;
static const uint32_t __initconst hvm_hap_max_featuremask[] =
    INIT_HVM_HAP_MAX_FEATURES;
static const uint32_t __initconst pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
static const uint32_t __initconst hvm_shadow_def_featuremask[] =
    INIT_HVM_SHADOW_DEF_FEATURES;
static const uint32_t __initconst hvm_hap_def_featuremask[] =
    INIT_HVM_HAP_DEF_FEATURES;
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;

I notice that known_features[], as an exception, has its dimension declared
in cpuid.h.

> On the other hand, zero extending a featureset is explicitly how they're
> supposed to work.  How do you think Xapi has coped with migration
> compatibility over the years, not to mention the microcode changes which
> lengthen a featureset?
> 
> So no, there was never any problem with constructs of the form uint32_t
> foo[10] = { 1, } in the first place.
> 
> The BUILD_BUG_ON()s therefore serve no purpose at all.

As per above the very minimum would be to accompany their dropping with
adding of explicitly specified dimensions for all the static arrays. I'm
not entirely certain about the other side (the zero-extension), but I'd
likely end up simply trusting you on that.

Jan

Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Jan Beulich]

On 10.05.2023 22:13, Andrew Cooper wrote:
> On 09/05/2023 3:24 pm, Jan Beulich wrote:
>> On 09.05.2023 16:03, Andrew Cooper wrote:
>>> On 08/05/2023 8:45 am, Jan Beulich wrote:
>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>> When adding new words to a featureset, there is a reasonable amount of
>>>>> boilerplate and it is preforable to split the addition into multiple patches.
>>>>>
>>>>> GCC 12 spotted a real (transient) error which occurs when splitting additions
>>>>> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
>>>>> highest numeric XEN_CPUFEATURE() value, and can be less than what the
>>>>> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
>>>>>
>>>>> This causes the policy <-> featureset converters to genuinely access
>>>>> out-of-bounds on the featureset array.
>>>>>
>>>>> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
>>>>> specifically to grow larger than FEATURESET_NR_ENTRIES.
>>>>>
>>>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> While, like you, I could live with the previous patch even if I don't
>>>> particularly like it, I'm not convinced of the route you take here.
>>> It's the route you tentatively agreed to in
>>> https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82bad1@suse.com/
>> Right. Yet I deliberately said "may be the best" there, as something
>> better might turn up. And getting the two numbers to always agree, as
>> suggested, might end up being better.
> 
> Then don't write "yes" if what you actually mean is "I'd prefer a
> different option if possible", which is a "no".
> 
> I cannot read your mind, and we both know I do not have time to waste on
> this task.
> 
> And now I have to go and spend yet more time doing it differently.

I'm sorry for that. Yet please also allow for me to re-consider an earlier
voiced view, once I see things in more detail.

Jan

Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Andrew Cooper]

On 11/05/2023 7:43 am, Jan Beulich wrote:
> On 10.05.2023 17:06, Andrew Cooper wrote:
>> On 09/05/2023 5:15 pm, Jan Beulich wrote:
>>> On 09.05.2023 17:59, Andrew Cooper wrote:
>>>> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>>>>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>>>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic for
>>>>>>>> code which looks like:
>>>>>>>>
>>>>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>>
>>>>>>>> However, GCC 12 at least does now warn for this:
>>>>>>>>
>>>>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>>         |                        ^
>>>>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>>>>> the arrays in question don't have explicit dimensions at their
>>>>>>> definition sites, and hence they derive their dimensions from their
>>>>>>> initializers. So the build-time-checks are about the arrays in fact
>>>>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>>>>
>>>>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>>>>> can't even say "okay with an adjusted description".
>>>>>> Now I'm extra confused.
>>>>>>
>>>>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>>>>> when I was expecting one, and there was a bug in the original featureset
>>>>>> work caused by this going wrong.
>>>>>>
>>>>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>>>>
>>>>>> Maybe it was some other error (C file not seeing the header properly?)
>>>>>> which disappeared across the upstream review?
>>>>> Or maybe, by mistake, too few initializer fields? But what exactly it
>>>>> was probably doesn't matter. If this patch is to stay (see below), some
>>>>> different description will be needed anyway (or the change be folded
>>>>> into the one actually invalidating those BUILD_BUG_ON()s).
>>>>>
>>>>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>>>>> because the check is no longer valid when a featureset can be longer
>>>>>> than the autogen length.
>>>>> Well, they need deleting if we stick to the approach chosen there right
>>>>> now. If we switched to my proposed alternative, they better would stay.
>>>> Given that all versions of GCC do warn, I don't see any justification
>>>> for them to stay.
>>> All versions warn when the variable declarations / definitions have a
>>> dimension specified, and then there are excess initializers. Yet none
>>> of the five affected arrays have a dimension specified in their
>>> definitions.
>>>
>>> Even if dimensions were added, we'd then have only covered half of
>>> what the BUILD_BUG_ON()s cover right now: There could then be fewer
>>> than intended initializer fields, and things may still be screwed. I
>>> think it was for this very reason why BUILD_BUG_ON() was chosen.
>> ???
>>
>> The dimensions already exist, as proved by the fact GCC can spot the
>> violation.
> Where? Quoting cpu-policy.c:
>
> const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>
> static const uint32_t __initconst pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
> static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES;
> static const uint32_t __initconst hvm_hap_max_featuremask[] =
>     INIT_HVM_HAP_MAX_FEATURES;
> static const uint32_t __initconst pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
> static const uint32_t __initconst hvm_shadow_def_featuremask[] =
>     INIT_HVM_SHADOW_DEF_FEATURES;
> static const uint32_t __initconst hvm_hap_def_featuremask[] =
>     INIT_HVM_HAP_DEF_FEATURES;
> static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>
> I notice that known_features[], as an exception, has its dimension declared
> in cpuid.h.

Ah.  I had indeed not spotted that.  Sorry.

It explains why all of my test builds (checking known_features[])
appeared to work.  I will rework these to have dimensions, because it
will remove some reasonably complex logic in gen-cpuid.py.

>
>> On the other hand, zero extending a featureset is explicitly how they're
>> supposed to work.  How do you think Xapi has coped with migration
>> compatibility over the years, not to mention the microcode changes which
>> lengthen a featureset?
>>
>> So no, there was never any problem with constructs of the form uint32_t
>> foo[10] = { 1, } in the first place.
>>
>> The BUILD_BUG_ON()s therefore serve no purpose at all.
> As per above the very minimum would be to accompany their dropping with
> adding of explicitly specified dimensions for all the static arrays. I'm
> not entirely certain about the other side (the zero-extension), but I'd
> likely end up simply trusting you on that.

https://godbolt.org/z/c13Kxcdsh

GCC (on both extremes that godbolt supports) zero extends to the
declaration dimension size.

~Andrew