* [PATCH 1/6] x86: Sort headers
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 7:10 ` Jan Beulich
2025-07-16 17:31 ` [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h Andrew Cooper
` (4 subsequent siblings)
5 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
In intel.c, drop asm/mwait.h and asm/uaccess.h, neither of which are used.
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>
---
xen/arch/x86/acpi/cpu_idle.c | 3 ++-
xen/arch/x86/apic.c | 25 ++++++++++++-------------
xen/arch/x86/cpu/intel.c | 14 ++++++--------
xen/arch/x86/cpu/mwait-idle.c | 6 ++++--
4 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 19f710f0dd9d..86f32fd9a6f0 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -45,7 +45,6 @@
#include <xen/softirq.h>
#include <xen/trace.h>
-#include <acpi/cpufreq/cpufreq.h>
#include <asm/apic.h>
#include <asm/cpuidle.h>
#include <asm/hpet.h>
@@ -55,6 +54,8 @@
#include <asm/processor.h>
#include <asm/spec_ctrl.h>
+#include <acpi/cpufreq/cpufreq.h>
+
#include <public/platform.h>
#include <public/sysctl.h>
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index bb86a1c161b3..a058c435878a 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -16,32 +16,31 @@
* Mikael Pettersson : PM converted to driver model.
*/
-#include <xen/perfc.h>
+#include <xen/delay.h>
#include <xen/errno.h>
#include <xen/init.h>
+#include <xen/irq.h>
+#include <xen/kexec.h>
#include <xen/mm.h>
#include <xen/param.h>
+#include <xen/perfc.h>
#include <xen/sched.h>
-#include <xen/irq.h>
-#include <xen/delay.h>
#include <xen/smp.h>
#include <xen/softirq.h>
+#include <asm/apic.h>
+#include <asm/atomic.h>
+#include <asm/flushtlb.h>
+#include <asm/genapic.h>
+#include <asm/guest.h>
+#include <asm/hardirq.h>
#include <asm/io-ports.h>
+#include <asm/io_apic.h>
#include <asm/irq-vectors.h>
#include <asm/mc146818rtc.h>
#include <asm/microcode.h>
-#include <asm/msr.h>
-#include <asm/atomic.h>
#include <asm/mpspec.h>
-#include <asm/flushtlb.h>
-#include <asm/hardirq.h>
-#include <asm/apic.h>
-#include <asm/io_apic.h>
-#include <asm/genapic.h>
-
-#include <xen/kexec.h>
-#include <asm/guest.h>
+#include <asm/msr.h>
#include <asm/nmi.h>
#include <asm/time.h>
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index f7bd0d777289..59b5828fa9e9 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -1,18 +1,16 @@
+#include <xen/bitops.h>
#include <xen/init.h>
#include <xen/kernel.h>
#include <xen/sched.h>
-#include <xen/string.h>
-#include <xen/bitops.h>
#include <xen/smp.h>
+#include <xen/string.h>
-#include <asm/intel-family.h>
-#include <asm/processor.h>
-#include <asm/msr.h>
-#include <asm/mwait.h>
-#include <asm/uaccess.h>
-#include <asm/mpspec.h>
#include <asm/apic.h>
#include <asm/i387.h>
+#include <asm/intel-family.h>
+#include <asm/mpspec.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
#include <asm/trampoline.h>
#include "cpu.h"
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 9ee54c625a90..c733a672d1e0 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -42,18 +42,20 @@
/* un-comment DEBUG to enable pr_debug() statements */
#define DEBUG
-#include <xen/lib.h>
#include <xen/cpu.h>
#include <xen/init.h>
+#include <xen/lib.h>
#include <xen/param.h>
#include <xen/softirq.h>
#include <xen/trace.h>
+
#include <asm/cpuidle.h>
#include <asm/hpet.h>
#include <asm/intel-family.h>
-#include <asm/mwait.h>
#include <asm/msr.h>
+#include <asm/mwait.h>
#include <asm/spec_ctrl.h>
+
#include <acpi/cpufreq/cpufreq.h>
#define MWAIT_IDLE_VERSION "0.4.1"
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
2025-07-16 17:31 ` [PATCH 1/6] x86: Sort headers Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 7:23 ` Jan Beulich
2025-07-16 17:31 ` [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[] Andrew Cooper
` (3 subsequent siblings)
5 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Only 5 files use struct x86_cpu_id, so it should not be in processor.h. This
is in preparation to extend it with VFM support.
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>
---
xen/arch/x86/acpi/cpu_idle.c | 2 +-
xen/arch/x86/apic.c | 1 +
xen/arch/x86/cpu/common.c | 21 +--------------------
xen/arch/x86/cpu/intel.c | 2 +-
xen/arch/x86/cpu/mwait-idle.c | 1 +
xen/arch/x86/include/asm/match-cpu.h | 25 +++++++++++++++++++++++++
xen/arch/x86/include/asm/processor.h | 10 ----------
7 files changed, 30 insertions(+), 32 deletions(-)
create mode 100644 xen/arch/x86/include/asm/match-cpu.h
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 86f32fd9a6f0..fee29353439e 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -50,8 +50,8 @@
#include <asm/hpet.h>
#include <asm/io.h>
#include <asm/iocap.h>
+#include <asm/match-cpu.h>
#include <asm/mwait.h>
-#include <asm/processor.h>
#include <asm/spec_ctrl.h>
#include <acpi/cpufreq/cpufreq.h>
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index a058c435878a..c4272ab4de4f 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -37,6 +37,7 @@
#include <asm/io-ports.h>
#include <asm/io_apic.h>
#include <asm/irq-vectors.h>
+#include <asm/match-cpu.h>
#include <asm/mc146818rtc.h>
#include <asm/microcode.h>
#include <asm/mpspec.h>
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 77364fd728db..cc004fc976f5 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -11,9 +11,9 @@
#include <asm/debugreg.h>
#include <asm/idt.h>
#include <asm/io.h>
+#include <asm/match-cpu.h>
#include <asm/mpspec.h>
#include <asm/msr.h>
-#include <asm/processor.h>
#include <asm/prot-key.h>
#include <asm/random.h>
#include <asm/setup.h>
@@ -998,25 +998,6 @@ void cpu_uninit(unsigned int cpu)
cpumask_clear_cpu(cpu, &cpu_initialized);
}
-/*
- * x86_match_cpu - match the current CPU against an array of
- * x86_cpu_ids
- * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
- * {}.
- * Return the entry if the current CPU matches the entries in the
- * passed x86_cpu_id match table. Otherwise NULL. The match table
- * contains vendor (X86_VENDOR_*), family, model and feature bits or
- * respective wildcard entries.
- *
- * A typical table entry would be to match a specific CPU
- * { X86_VENDOR_INTEL, 6, 0x12 }
- * or to match a specific CPU feature
- * { X86_FEATURE_MATCH(X86_FEATURE_FOOBAR) }
- *
- * This always matches against the boot cpu, assuming models and
-features are
- * consistent over all CPUs.
- */
const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
{
const struct x86_cpu_id *m;
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 59b5828fa9e9..ee1ae92cd7e6 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -8,9 +8,9 @@
#include <asm/apic.h>
#include <asm/i387.h>
#include <asm/intel-family.h>
+#include <asm/match-cpu.h>
#include <asm/mpspec.h>
#include <asm/msr.h>
-#include <asm/processor.h>
#include <asm/trampoline.h>
#include "cpu.h"
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index c733a672d1e0..eec2823cbacf 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -52,6 +52,7 @@
#include <asm/cpuidle.h>
#include <asm/hpet.h>
#include <asm/intel-family.h>
+#include <asm/match-cpu.h>
#include <asm/msr.h>
#include <asm/mwait.h>
#include <asm/spec_ctrl.h>
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
new file mode 100644
index 000000000000..2704b84d74c9
--- /dev/null
+++ b/xen/arch/x86/include/asm/match-cpu.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef X86_MATCH_CPU_H
+#define X86_MATCH_CPU_H
+
+#include <xen/stdint.h>
+
+struct x86_cpu_id {
+ uint16_t vendor;
+ uint16_t family;
+ uint16_t model;
+ uint16_t feature;
+ const void *driver_data;
+};
+
+/*
+ * x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
+ *
+ * @table: Array of x86_cpu_ids. Last entry terminated with {}.
+ *
+ * Returns the matching entry, otherwise NULL. This always matches against
+ * the boot CPU, assuming models and features are consistent over all CPUs.
+ */
+const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[]);
+
+#endif /* X86_MATCH_CPU_H */
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index 04824c3633cf..c58617631bf3 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -80,14 +80,6 @@
struct domain;
struct vcpu;
-struct x86_cpu_id {
- uint16_t vendor;
- uint16_t family;
- uint16_t model;
- uint16_t feature; /* bit index */
- const void *driver_data;
-};
-
extern struct cpuinfo_x86 cpu_data[];
#define current_cpu_data cpu_data[smp_processor_id()]
@@ -104,8 +96,6 @@ extern unsigned int hap_paddr_bits;
/* Maximum width of virtual addresses supported by the hardware. */
extern unsigned int vaddr_bits;
-extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[]);
-
extern void identify_cpu(struct cpuinfo_x86 *c);
extern void setup_clear_cpu_cap(unsigned int cap);
extern void setup_force_cpu_cap(unsigned int cap);
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h
2025-07-16 17:31 ` [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h Andrew Cooper
@ 2025-07-17 7:23 ` Jan Beulich
2025-07-17 17:33 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 7:23 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> Only 5 files use struct x86_cpu_id, so it should not be in processor.h. This
> is in preparation to extend it with VFM support.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two nits:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/match-cpu.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef X86_MATCH_CPU_H
> +#define X86_MATCH_CPU_H
> +
> +#include <xen/stdint.h>
> +
> +struct x86_cpu_id {
> + uint16_t vendor;
> + uint16_t family;
> + uint16_t model;
> + uint16_t feature;
> + const void *driver_data;
> +};
> +
> +/*
> + * x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
> + *
> + * @table: Array of x86_cpu_ids. Last entry terminated with {}.
This reads slightly odd to me: I'd have expected "Last entry needs to be ..."
or "Table terminated with ...".
> + * Returns the matching entry, otherwise NULL.
Perhaps add "first"? Due to use of wildcards, multiple entries could in
principle match.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h
2025-07-17 7:23 ` Jan Beulich
@ 2025-07-17 17:33 ` Andrew Cooper
0 siblings, 0 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 17:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 8:23 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> Only 5 files use struct x86_cpu_id, so it should not be in processor.h. This
>> is in preparation to extend it with VFM support.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> with two nits:
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/match-cpu.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef X86_MATCH_CPU_H
>> +#define X86_MATCH_CPU_H
>> +
>> +#include <xen/stdint.h>
>> +
>> +struct x86_cpu_id {
>> + uint16_t vendor;
>> + uint16_t family;
>> + uint16_t model;
>> + uint16_t feature;
>> + const void *driver_data;
>> +};
>> +
>> +/*
>> + * x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
>> + *
>> + * @table: Array of x86_cpu_ids. Last entry terminated with {}.
> This reads slightly odd to me: I'd have expected "Last entry needs to be ..."
> or "Table terminated with ...".
>
>> + * Returns the matching entry, otherwise NULL.
> Perhaps add "first"? Due to use of wildcards, multiple entries could in
> principle match.
Both fixed. The comment really wasn't great to start with.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[]
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
2025-07-16 17:31 ` [PATCH 1/6] x86: Sort headers Andrew Cooper
2025-07-16 17:31 ` [PATCH 2/6] x86: Break struct x86_cpu_id out of processor.h Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 7:35 ` Jan Beulich
2025-07-16 17:31 ` [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM() Andrew Cooper
` (2 subsequent siblings)
5 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
mwait-idle's ICPU() is the most convenient place to get started. Introduce
X86_MATCH_CPU() and X86_MATCH_VFM() following their Linux counterparts.
This involves match-cpu.h including more headers, which in turn allows us to
drop a few.
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>
We now have X86_FEATURE_ANY and X86_FEATURE_ALWAYS as aliases of LM. Given
the contexts they're used in, I've left the naming as-is.
It's a bit nasty (preprocessing wise) triple-expanding VFM in X86_MATCH_VFM(),
but we need an Integer Constant Expression.
---
xen/arch/x86/cpu/intel.c | 1 -
xen/arch/x86/cpu/mwait-idle.c | 4 +---
xen/arch/x86/include/asm/match-cpu.h | 21 ++++++++++++++++++++-
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index ee1ae92cd7e6..26a171aa363e 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -7,7 +7,6 @@
#include <asm/apic.h>
#include <asm/i387.h>
-#include <asm/intel-family.h>
#include <asm/match-cpu.h>
#include <asm/mpspec.h>
#include <asm/msr.h>
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index eec2823cbacf..e837cbf50eb3 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -51,7 +51,6 @@
#include <asm/cpuidle.h>
#include <asm/hpet.h>
-#include <asm/intel-family.h>
#include <asm/match-cpu.h>
#include <asm/msr.h>
#include <asm/mwait.h>
@@ -1302,8 +1301,7 @@ static const struct idle_cpu idle_cpu_srf = {
};
#define ICPU(model, cpu) \
- { X86_VENDOR_INTEL, 6, INTEL_FAM6_ ## model, X86_FEATURE_ALWAYS, \
- &idle_cpu_ ## cpu}
+ X86_MATCH_VFM(INTEL_ ## model, &idle_cpu_ ## cpu)
static const struct x86_cpu_id intel_idle_ids[] __initconstrel = {
ICPU(NEHALEM_EP, nehalem),
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
index 2704b84d74c9..dcdc50a70d14 100644
--- a/xen/arch/x86/include/asm/match-cpu.h
+++ b/xen/arch/x86/include/asm/match-cpu.h
@@ -4,14 +4,33 @@
#include <xen/stdint.h>
+#include <asm/cpufeature.h>
+#include <asm/intel-family.h>
+#include <asm/x86-vendors.h>
+
+#define X86_FEATURE_ANY X86_FEATURE_LM
+
struct x86_cpu_id {
uint16_t vendor;
uint16_t family;
uint16_t model;
- uint16_t feature;
+ uint16_t feature; /* X86_FEATURE_*, or X86_FEATURE_ANY */
const void *driver_data;
};
+#define X86_MATCH_CPU(v, f, m, feat, data) \
+ { \
+ .vendor = (v), \
+ .family = (f), \
+ .model = (m), \
+ .feature = (feat), \
+ .driver_data = (const void *)(unsigned long)(data), \
+ }
+
+#define X86_MATCH_VFM(vfm, data) \
+ X86_MATCH_CPU(VFM_VENDOR(vfm), VFM_FAMILY(vfm), \
+ VFM_MODEL(vfm), X86_FEATURE_ANY, data)
+
/*
* x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
*
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[]
2025-07-16 17:31 ` [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[] Andrew Cooper
@ 2025-07-17 7:35 ` Jan Beulich
2025-07-17 17:39 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 7:35 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> mwait-idle's ICPU() is the most convenient place to get started. Introduce
> X86_MATCH_CPU() and X86_MATCH_VFM() following their Linux counterparts.
>
> This involves match-cpu.h including more headers, which in turn allows us to
> drop a few.
intel-cpu.h doesn't really need to move, does it? Conceivably there can be users
of match-cpu.h which don't need the Intel constants. Hence no point in forcing
them to see those.
> 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>
>
> We now have X86_FEATURE_ANY and X86_FEATURE_ALWAYS as aliases of LM. Given
> the contexts they're used in, I've left the naming as-is.
What's wrong with sticking to ALWAYS, which we already have?
> It's a bit nasty (preprocessing wise) triple-expanding VFM in X86_MATCH_VFM(),
> but we need an Integer Constant Expression.
Not sure what alternative you're alluding to, or in fact what nastiness you're
seeing. But maybe I'm getting "triple-expanding" wrong: To me that means going
through three layers of expansion, when you may mean the fact that
X86_MATCH_VFM() evaluates its parameter three times. If so - yes, but as you
say: What do you do (in this case).
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[]
2025-07-17 7:35 ` Jan Beulich
@ 2025-07-17 17:39 ` Andrew Cooper
2025-07-18 5:38 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 17:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 8:35 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> mwait-idle's ICPU() is the most convenient place to get started. Introduce
>> X86_MATCH_CPU() and X86_MATCH_VFM() following their Linux counterparts.
>>
>> This involves match-cpu.h including more headers, which in turn allows us to
>> drop a few.
> intel-cpu.h doesn't really need to move, does it? Conceivably there can be users
> of match-cpu.h which don't need the Intel constants. Hence no point in forcing
> them to see those.
There's no point not to. All users of x86_cpu_id want the Intel names.
I've already restricted it to only 5 TUs.
Even if we do get some AMD names (and I'm not entirely sure how that
would end up looking), it's just a few defines.
>
>> 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>
>>
>> We now have X86_FEATURE_ANY and X86_FEATURE_ALWAYS as aliases of LM. Given
>> the contexts they're used in, I've left the naming as-is.
> What's wrong with sticking to ALWAYS, which we already have?
For alternatives, something like:
alternative("", "foo", X86_FEATURE_ALWAYS);
is correct in context. However:
X86_MATCH_?(..., X86_FEATURE_ALWAYS, ...);
is borderline grammatically wrong, and ANY is a better name to use.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[]
2025-07-17 17:39 ` Andrew Cooper
@ 2025-07-18 5:38 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 5:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 17.07.2025 19:39, Andrew Cooper wrote:
> On 17/07/2025 8:35 am, Jan Beulich wrote:
>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>> mwait-idle's ICPU() is the most convenient place to get started. Introduce
>>> X86_MATCH_CPU() and X86_MATCH_VFM() following their Linux counterparts.
>>>
>>> This involves match-cpu.h including more headers, which in turn allows us to
>>> drop a few.
>> intel-cpu.h doesn't really need to move, does it? Conceivably there can be users
>> of match-cpu.h which don't need the Intel constants. Hence no point in forcing
>> them to see those.
>
> There's no point not to. All users of x86_cpu_id want the Intel names.
> I've already restricted it to only 5 TUs.
>
> Even if we do get some AMD names (and I'm not entirely sure how that
> would end up looking), it's just a few defines.
It's just a (slowly growing set of a) few, yes. Still goes against our
desire the limit #include dependencies.
>>> 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>
>>>
>>> We now have X86_FEATURE_ANY and X86_FEATURE_ALWAYS as aliases of LM. Given
>>> the contexts they're used in, I've left the naming as-is.
>> What's wrong with sticking to ALWAYS, which we already have?
>
> For alternatives, something like:
>
> alternative("", "foo", X86_FEATURE_ALWAYS);
>
> is correct in context. However:
>
> X86_MATCH_?(..., X86_FEATURE_ALWAYS, ...);
>
> is borderline grammatically wrong, and ANY is a better name to use.
Well, I don't necessarily agree, but then the extra name also isn't
a severe problem. It was actually you who called out the redundancy.
In any event:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM()
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
` (2 preceding siblings ...)
2025-07-16 17:31 ` [PATCH 3/6] x86/match-cpu: Introduce X86_MATCH_VFM() and convert intel_idle_ids[] Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 7:44 ` Jan Beulich
2025-07-16 17:31 ` [PATCH 5/6] x86/match-cpu: Support matching on steppings Andrew Cooper
2025-07-16 17:31 ` [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*() Andrew Cooper
5 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
This replaces raw model numbers (and comments in some cases) with names. For
probe_mwait_errata(), merge the comments with the table to make it easier to
see which erratum is which, and drop a stray "Problem" in LNL030.
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>
There isn't an obvious helper in Linux to use which can avoid the data
parameter; Linux doesn't really do lists like this which are *just* a VFM
match with nothing else.
Either way, I've opted for a few extra NULLs than a proliferation of
X86_MATCH_*() helpers.
---
xen/arch/x86/acpi/cpu_idle.c | 48 ++++++++++++++-------------------
xen/arch/x86/cpu/intel.c | 51 +++++++++++++++++-------------------
2 files changed, 44 insertions(+), 55 deletions(-)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index fee29353439e..78e98e9c134d 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -583,7 +583,6 @@ bool errata_c6_workaround(void)
if ( unlikely(fix_needed == -1) )
{
-#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
/*
* Errata AAJ72: EOI Transaction May Not be Sent if Software Enters
* Core C6 During an Interrupt Service Routine"
@@ -594,12 +593,12 @@ bool errata_c6_workaround(void)
* there is an EOI pending.
*/
static const struct x86_cpu_id eoi_errata[] = {
- INTEL_FAM6_MODEL(0x1a),
- INTEL_FAM6_MODEL(0x1e),
- INTEL_FAM6_MODEL(0x1f),
- INTEL_FAM6_MODEL(0x25),
- INTEL_FAM6_MODEL(0x2c),
- INTEL_FAM6_MODEL(0x2f),
+ X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
+ X86_MATCH_VFM(INTEL_NEHALEM, NULL),
+ X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
+ X86_MATCH_VFM(INTEL_WESTMERE, NULL),
+ X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
+ X86_MATCH_VFM(INTEL_WESTMERE_EX, NULL),
{ }
};
/*
@@ -617,29 +616,22 @@ bool errata_c6_workaround(void)
* discovered on Haswell hardware, and is affected.
*/
static const struct x86_cpu_id isr_errata[] = {
- /* Haswell */
- INTEL_FAM6_MODEL(0x3c),
- INTEL_FAM6_MODEL(0x3f),
- INTEL_FAM6_MODEL(0x45),
- INTEL_FAM6_MODEL(0x46),
- /* Broadwell */
- INTEL_FAM6_MODEL(0x47),
- INTEL_FAM6_MODEL(0x3d),
- INTEL_FAM6_MODEL(0x4f),
- INTEL_FAM6_MODEL(0x56),
- /* Skylake (client) */
- INTEL_FAM6_MODEL(0x5e),
- INTEL_FAM6_MODEL(0x4e),
- /* {Sky/Cascade}lake (server) */
- INTEL_FAM6_MODEL(0x55),
- /* {Kaby/Coffee/Whiskey/Amber} Lake */
- INTEL_FAM6_MODEL(0x9e),
- INTEL_FAM6_MODEL(0x8e),
- /* Cannon Lake */
- INTEL_FAM6_MODEL(0x66),
+ X86_MATCH_VFM(INTEL_HASWELL, NULL),
+ X86_MATCH_VFM(INTEL_HASWELL_X, NULL),
+ X86_MATCH_VFM(INTEL_HASWELL_L, NULL),
+ X86_MATCH_VFM(INTEL_HASWELL_G, NULL),
+ X86_MATCH_VFM(INTEL_BROADWELL, NULL),
+ X86_MATCH_VFM(INTEL_BROADWELL_G, NULL),
+ X86_MATCH_VFM(INTEL_BROADWELL_X, NULL),
+ X86_MATCH_VFM(INTEL_BROADWELL_D, NULL),
+ X86_MATCH_VFM(INTEL_SKYLAKE_L, NULL),
+ X86_MATCH_VFM(INTEL_SKYLAKE, NULL),
+ X86_MATCH_VFM(INTEL_SKYLAKE_X, NULL),
+ X86_MATCH_VFM(INTEL_KABYLAKE_L, NULL),
+ X86_MATCH_VFM(INTEL_KABYLAKE, NULL),
+ X86_MATCH_VFM(INTEL_CANNONLAKE_L, NULL),
{ }
};
-#undef INTEL_FAM6_MODEL
fix_needed = cpu_has_apic &&
((!directed_eoi_enabled && x86_match_cpu(eoi_errata)) ||
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 26a171aa363e..2028a609453b 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -382,16 +382,12 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
*/
static void probe_c3_errata(const struct cpuinfo_x86 *c)
{
-#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
static const struct x86_cpu_id models[] = {
- /* Nehalem */
- INTEL_FAM6_MODEL(0x1a),
- INTEL_FAM6_MODEL(0x1e),
- INTEL_FAM6_MODEL(0x1f),
- INTEL_FAM6_MODEL(0x2e),
- /* Westmere (note Westmere-EX is not affected) */
- INTEL_FAM6_MODEL(0x2c),
- INTEL_FAM6_MODEL(0x25),
+ X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
+ X86_MATCH_VFM(INTEL_NEHALEM, NULL),
+ X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
+ X86_MATCH_VFM(INTEL_WESTMERE, NULL),
+ X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
{ }
};
@@ -405,29 +401,30 @@ static void probe_c3_errata(const struct cpuinfo_x86 *c)
}
}
-/*
- * APL30: One use of the MONITOR/MWAIT instruction pair is to allow a logical
- * processor to wait in a sleep state until a store to the armed address range
- * occurs. Due to this erratum, stores to the armed address range may not
- * trigger MWAIT to resume execution.
- *
- * ICX143: Under complex microarchitectural conditions, a monitor that is armed
- * with the MWAIT instruction may not be triggered, leading to a processor
- * hang.
- *
- * LNL030: Problem P-cores may not exit power state Core C6 on monitor hit.
- *
- * Force the sending of an IPI in those cases.
- */
static void __init probe_mwait_errata(void)
{
static const struct x86_cpu_id __initconst models[] = {
- INTEL_FAM6_MODEL(INTEL_FAM6_ATOM_GOLDMONT), /* APL30 */
- INTEL_FAM6_MODEL(INTEL_FAM6_ICELAKE_X), /* ICX143 */
- INTEL_FAM6_MODEL(INTEL_FAM6_LUNARLAKE_M), /* LNL030 */
+ /*
+ * APL30: One use of the MONITOR/MWAIT instruction pair is to allow a
+ * logical processor to wait in a sleep state until a store to the
+ * armed address range occurs. Due to this erratum, stores to the
+ * armed address range may not trigger MWAIT to resume execution.
+ */
+ X86_MATCH_VFM(INTEL_ATOM_GOLDMONT, NULL),
+
+ /*
+ * ICX143: Under complex microarchitectural conditions, a monitor that
+ * is armed with the MWAIT instruction may not be triggered, leading
+ * to a processor hang.
+ */
+ X86_MATCH_VFM(INTEL_ICELAKE_X, NULL),
+
+ /*
+ * LNL030: P-cores may not exit power state Core C6 on monitor hit.
+ */
+ X86_MATCH_VFM(INTEL_LUNARLAKE_M, NULL),
{ }
};
-#undef INTEL_FAM6_MODEL
if ( boot_cpu_has(X86_FEATURE_MONITOR) && x86_match_cpu(models) )
{
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM()
2025-07-16 17:31 ` [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM() Andrew Cooper
@ 2025-07-17 7:44 ` Jan Beulich
2025-07-17 17:57 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 7:44 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> This replaces raw model numbers (and comments in some cases) with names. For
> probe_mwait_errata(), merge the comments with the table to make it easier to
> see which erratum is which, and drop a stray "Problem" in LNL030.
>
> 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>
>
> There isn't an obvious helper in Linux to use which can avoid the data
> parameter; Linux doesn't really do lists like this which are *just* a VFM
> match with nothing else.
>
> Either way, I've opted for a few extra NULLs than a proliferation of
> X86_MATCH_*() helpers.
+1
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -583,7 +583,6 @@ bool errata_c6_workaround(void)
>
> if ( unlikely(fix_needed == -1) )
> {
> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
> /*
> * Errata AAJ72: EOI Transaction May Not be Sent if Software Enters
> * Core C6 During an Interrupt Service Routine"
> @@ -594,12 +593,12 @@ bool errata_c6_workaround(void)
> * there is an EOI pending.
> */
> static const struct x86_cpu_id eoi_errata[] = {
> - INTEL_FAM6_MODEL(0x1a),
> - INTEL_FAM6_MODEL(0x1e),
> - INTEL_FAM6_MODEL(0x1f),
> - INTEL_FAM6_MODEL(0x25),
> - INTEL_FAM6_MODEL(0x2c),
> - INTEL_FAM6_MODEL(0x2f),
> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
> + X86_MATCH_VFM(INTEL_WESTMERE_EX, NULL),
> { }
> };
Along the lines of a comment further down, maybe make explicit that Nehalem-EX
is intentionally omitted here (assuming that's not in fact an oversight)?
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -382,16 +382,12 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
> */
> static void probe_c3_errata(const struct cpuinfo_x86 *c)
> {
> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
> static const struct x86_cpu_id models[] = {
> - /* Nehalem */
> - INTEL_FAM6_MODEL(0x1a),
> - INTEL_FAM6_MODEL(0x1e),
> - INTEL_FAM6_MODEL(0x1f),
> - INTEL_FAM6_MODEL(0x2e),
> - /* Westmere (note Westmere-EX is not affected) */
> - INTEL_FAM6_MODEL(0x2c),
> - INTEL_FAM6_MODEL(0x25),
> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
> { }
> };
You lost NEHALEM_EX here. For Westmere-EX I think the comment (part) would
better be retained, to clarify that this isn't an oversight.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM()
2025-07-17 7:44 ` Jan Beulich
@ 2025-07-17 17:57 ` Andrew Cooper
2025-07-18 5:40 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 17:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 8:44 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> --- a/xen/arch/x86/acpi/cpu_idle.c
>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>> @@ -583,7 +583,6 @@ bool errata_c6_workaround(void)
>>
>> if ( unlikely(fix_needed == -1) )
>> {
>> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
>> /*
>> * Errata AAJ72: EOI Transaction May Not be Sent if Software Enters
>> * Core C6 During an Interrupt Service Routine"
>> @@ -594,12 +593,12 @@ bool errata_c6_workaround(void)
>> * there is an EOI pending.
>> */
>> static const struct x86_cpu_id eoi_errata[] = {
>> - INTEL_FAM6_MODEL(0x1a),
>> - INTEL_FAM6_MODEL(0x1e),
>> - INTEL_FAM6_MODEL(0x1f),
>> - INTEL_FAM6_MODEL(0x25),
>> - INTEL_FAM6_MODEL(0x2c),
>> - INTEL_FAM6_MODEL(0x2f),
>> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
>> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
>> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
>> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
>> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
>> + X86_MATCH_VFM(INTEL_WESTMERE_EX, NULL),
>> { }
>> };
> Along the lines of a comment further down, maybe make explicit that Nehalem-EX
> is intentionally omitted here (assuming that's not in fact an oversight)?
It looks to be an oversight. I've submitted a separate patch, so it can
be backported more easily.
In practice, it's covered by probe_c3_errata() which blanket disables C3
and C6 on Nehalem.
>
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -382,16 +382,12 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
>> */
>> static void probe_c3_errata(const struct cpuinfo_x86 *c)
>> {
>> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
>> static const struct x86_cpu_id models[] = {
>> - /* Nehalem */
>> - INTEL_FAM6_MODEL(0x1a),
>> - INTEL_FAM6_MODEL(0x1e),
>> - INTEL_FAM6_MODEL(0x1f),
>> - INTEL_FAM6_MODEL(0x2e),
>> - /* Westmere (note Westmere-EX is not affected) */
>> - INTEL_FAM6_MODEL(0x2c),
>> - INTEL_FAM6_MODEL(0x25),
>> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
>> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
>> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
>> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
>> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
>> { }
>> };
> You lost NEHALEM_EX here.
Oops, too much copy/paste.
> For Westmere-EX I think the comment (part) would
> better be retained, to clarify that this isn't an oversight.
I can't find anything which looks related for Westmere EX. I'll retain
the comment.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM()
2025-07-17 17:57 ` Andrew Cooper
@ 2025-07-18 5:40 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 5:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 17.07.2025 19:57, Andrew Cooper wrote:
> On 17/07/2025 8:44 am, Jan Beulich wrote:
>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/acpi/cpu_idle.c
>>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>>> @@ -583,7 +583,6 @@ bool errata_c6_workaround(void)
>>>
>>> if ( unlikely(fix_needed == -1) )
>>> {
>>> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
>>> /*
>>> * Errata AAJ72: EOI Transaction May Not be Sent if Software Enters
>>> * Core C6 During an Interrupt Service Routine"
>>> @@ -594,12 +593,12 @@ bool errata_c6_workaround(void)
>>> * there is an EOI pending.
>>> */
>>> static const struct x86_cpu_id eoi_errata[] = {
>>> - INTEL_FAM6_MODEL(0x1a),
>>> - INTEL_FAM6_MODEL(0x1e),
>>> - INTEL_FAM6_MODEL(0x1f),
>>> - INTEL_FAM6_MODEL(0x25),
>>> - INTEL_FAM6_MODEL(0x2c),
>>> - INTEL_FAM6_MODEL(0x2f),
>>> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
>>> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
>>> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
>>> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
>>> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
>>> + X86_MATCH_VFM(INTEL_WESTMERE_EX, NULL),
>>> { }
>>> };
>> Along the lines of a comment further down, maybe make explicit that Nehalem-EX
>> is intentionally omitted here (assuming that's not in fact an oversight)?
>
> It looks to be an oversight. I've submitted a separate patch, so it can
> be backported more easily.
>
> In practice, it's covered by probe_c3_errata() which blanket disables C3
> and C6 on Nehalem.
>
>>
>>> --- a/xen/arch/x86/cpu/intel.c
>>> +++ b/xen/arch/x86/cpu/intel.c
>>> @@ -382,16 +382,12 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
>>> */
>>> static void probe_c3_errata(const struct cpuinfo_x86 *c)
>>> {
>>> -#define INTEL_FAM6_MODEL(m) { X86_VENDOR_INTEL, 6, m, X86_FEATURE_ALWAYS }
>>> static const struct x86_cpu_id models[] = {
>>> - /* Nehalem */
>>> - INTEL_FAM6_MODEL(0x1a),
>>> - INTEL_FAM6_MODEL(0x1e),
>>> - INTEL_FAM6_MODEL(0x1f),
>>> - INTEL_FAM6_MODEL(0x2e),
>>> - /* Westmere (note Westmere-EX is not affected) */
>>> - INTEL_FAM6_MODEL(0x2c),
>>> - INTEL_FAM6_MODEL(0x25),
>>> + X86_MATCH_VFM(INTEL_NEHALEM_EP, NULL),
>>> + X86_MATCH_VFM(INTEL_NEHALEM, NULL),
>>> + X86_MATCH_VFM(INTEL_NEHALEM_G, NULL),
>>> + X86_MATCH_VFM(INTEL_WESTMERE, NULL),
>>> + X86_MATCH_VFM(INTEL_WESTMERE_EP, NULL),
>>> { }
>>> };
>> You lost NEHALEM_EX here.
>
> Oops, too much copy/paste.
>
>> For Westmere-EX I think the comment (part) would
>> better be retained, to clarify that this isn't an oversight.
>
> I can't find anything which looks related for Westmere EX. I'll retain
> the comment.
With the adjustments:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
` (3 preceding siblings ...)
2025-07-16 17:31 ` [PATCH 4/6] x86: Convert users of INTEL_FAM6_MODEL() to X86_MATCH_VFM() Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 8:11 ` Jan Beulich
2025-07-16 17:31 ` [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*() Andrew Cooper
5 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
bitmap of steppings.
In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
family fields, neither of which need to be uint16_t in Xen.
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>
Linux supports all fields being optional. This has lead to using
X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
boot_cpu_has(), and is not a construct I think we want to encorage.
---
xen/arch/x86/cpu/common.c | 4 +++-
xen/arch/x86/include/asm/match-cpu.h | 12 ++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cc004fc976f5..fc25935d3109 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
const struct x86_cpu_id *m;
const struct cpuinfo_x86 *c = &boot_cpu_data;
- for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
+ for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
if (c->x86_vendor != m->vendor)
continue;
if (c->x86 != m->family)
continue;
if (c->x86_model != m->model)
continue;
+ if (m->steppings && !(m->steppings & (1U << c->stepping)))
+ continue;
if (!cpu_has(c, m->feature))
continue;
return m;
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
index dcdc50a70d14..3862e766ccfc 100644
--- a/xen/arch/x86/include/asm/match-cpu.h
+++ b/xen/arch/x86/include/asm/match-cpu.h
@@ -8,28 +8,32 @@
#include <asm/intel-family.h>
#include <asm/x86-vendors.h>
+#define X86_STEPPINGS_ANY 0
#define X86_FEATURE_ANY X86_FEATURE_LM
struct x86_cpu_id {
- uint16_t vendor;
- uint16_t family;
+ uint8_t vendor;
+ uint8_t family;
uint16_t model;
+ uint16_t steppings; /* Stepping bitmap, or X86_STEPPINGS_ANY */
uint16_t feature; /* X86_FEATURE_*, or X86_FEATURE_ANY */
const void *driver_data;
};
-#define X86_MATCH_CPU(v, f, m, feat, data) \
+#define X86_MATCH_CPU(v, f, m, steps, feat, data) \
{ \
.vendor = (v), \
.family = (f), \
.model = (m), \
+ .steppings = (steps), \
.feature = (feat), \
.driver_data = (const void *)(unsigned long)(data), \
}
#define X86_MATCH_VFM(vfm, data) \
X86_MATCH_CPU(VFM_VENDOR(vfm), VFM_FAMILY(vfm), \
- VFM_MODEL(vfm), X86_FEATURE_ANY, data)
+ VFM_MODEL(vfm), X86_STEPPINGS_ANY, \
+ X86_FEATURE_ANY, data)
/*
* x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-16 17:31 ` [PATCH 5/6] x86/match-cpu: Support matching on steppings Andrew Cooper
@ 2025-07-17 8:11 ` Jan Beulich
2025-07-17 19:39 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 8:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
> bitmap of steppings.
>
> In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
> family fields, neither of which need to be uint16_t in Xen.
>
> 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>
>
> Linux supports all fields being optional. This has lead to using
> X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
> boot_cpu_has(), and is not a construct I think we want to encorage.
+1
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
> const struct x86_cpu_id *m;
> const struct cpuinfo_x86 *c = &boot_cpu_data;
>
> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
> + for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
Nit: Line length. But - do we need the change at all? It looks entirely
implausible to me to use ->steppings with all of vendor, family, and
model being *_ANY (if, as per below, they would be 0 in the first place).
Tangential: The ->feature check is slightly odd here. With everything
else being a wildcard (assuming these are 0; I can't find any X86_*_ANY
in the code base; INTEL_FAM6_ANY expands to X86_MODEL_ANY, but is itself
also not used anywhere), one wouldn't be able to use FPU, as that's
feature index 0. I notice though that ...
> if (c->x86_vendor != m->vendor)
> continue;
> if (c->x86 != m->family)
> continue;
> if (c->x86_model != m->model)
> continue;
... X86_*_ANY also aren't catered for here. Hence it remains unclear
what value those constants would actually be meant to have.
Further tangential: The vendor check could in principle permit for
multiple vendors (e.g. AMD any Hygon at the same time), considering that
we use bit masks now. That would require the != there to change, though.
> --- a/xen/arch/x86/include/asm/match-cpu.h
> +++ b/xen/arch/x86/include/asm/match-cpu.h
> @@ -8,28 +8,32 @@
> #include <asm/intel-family.h>
> #include <asm/x86-vendors.h>
>
> +#define X86_STEPPINGS_ANY 0
Given the (deliberate aiui) plural, maybe better X86_STEPPINGS_ALL?
Also perhaps use 0xffff as the value, allowing to drop part of the
conditional in x86_match_cpu()?
> #define X86_FEATURE_ANY X86_FEATURE_LM
>
> struct x86_cpu_id {
> - uint16_t vendor;
> - uint16_t family;
> + uint8_t vendor;
Is shrinking this to 8 bits a good idea? We use 5 of them already. (Of
course we can re-enlarge later, if and when the need arises.)
> + uint8_t family;
The family formula allows the value to be up to 0x10e. The return type
of get_cpu_family() is therefore wrong too, strictly speaking. As is
struct cpuinfo_x86's x86 field.
> uint16_t model;
Whereas the model is strictly limited to 8 bits.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-17 8:11 ` Jan Beulich
@ 2025-07-17 19:39 ` Andrew Cooper
2025-07-18 5:53 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 19:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 9:11 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
>> const struct x86_cpu_id *m;
>> const struct cpuinfo_x86 *c = &boot_cpu_data;
>>
>> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
>> + for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
> Nit: Line length. But - do we need the change at all? It looks entirely
> implausible to me to use ->steppings with all of vendor, family, and
> model being *_ANY (if, as per below, they would be 0 in the first place).
I do keep on saying that | like this is pure obfuscation. This is an
excellent example.
It's looking for the {} entry, by looking for 0's in all of the metadata
fields. A better check would be *(uint64_t *)m, or perhaps a unioned
metadata field, but..
This is also a good demonstration of binary | is a bad thing to use, not
only for legibility. Swapping | for || lets the compiler do:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-76 (-76)
Function old new delta
x86_match_cpu 243 167 -76
and the code generation looks much better too:
https://termbin.com/c4m9
Although I'm a little confused as to why it's still done a split cmpw
$0x0,(%rax) and cmpq $0xffff,(%rax) for the loop entry condition, when
cmpq $0 would be the right one.
>
> Tangential: The ->feature check is slightly odd here. With everything
> else being a wildcard (assuming these are 0; I can't find any X86_*_ANY
> in the code base; INTEL_FAM6_ANY expands to X86_MODEL_ANY, but is itself
> also not used anywhere), one wouldn't be able to use FPU, as that's
> feature index 0. I notice though that ...
>
>> if (c->x86_vendor != m->vendor)
>> continue;
>> if (c->x86 != m->family)
>> continue;
>> if (c->x86_model != m->model)
>> continue;
> ... X86_*_ANY also aren't catered for here. Hence it remains unclear
> what value those constants would actually be meant to have.
>
> Further tangential: The vendor check could in principle permit for
> multiple vendors (e.g. AMD any Hygon at the same time), considering that
> we use bit masks now. That would require the != there to change, though.
In Linux, x86_cpu_id is a module ABI and has wildcards on all fields,
because "please load me on any AMD Fam10 CPU" is something they want to
express.
In Xen, we only use it model/stepping specific lookup tables, so we
don't need wildcards for V/F/M like Linux does.
We do have a different layout of X86_VENDOR to Intel, and while that
would allow us to merge an AMD and a Hygon row, I don't think anything
good could come of trying.
One problem Linux has is that X86_VENDOR_INTEL is 0, so they introduced
a flags field with a VALID bit that now replaces the line of |'s. I do
not see any need for that in Xen.
>
>> --- a/xen/arch/x86/include/asm/match-cpu.h
>> +++ b/xen/arch/x86/include/asm/match-cpu.h
>> @@ -8,28 +8,32 @@
>> #include <asm/intel-family.h>
>> #include <asm/x86-vendors.h>
>>
>> +#define X86_STEPPINGS_ANY 0
> Given the (deliberate aiui) plural, maybe better X86_STEPPINGS_ALL?
Hmm, yeah, that's not great grammar. I think I prefer X86_STEPPING_ANY
to X86_STEPPINGS_ALL.
> Also perhaps use 0xffff as the value, allowing to drop part of the
> conditional in x86_match_cpu()?
Interestingly, while it simplifies the C, it undoes most of the code
generation improvements from switching | to ||.
https://termbin.com/h0iu
By removing the "m->steppings &&", gcc has now hoisted the load of
c->stepping out of the loop (in fact, the whole 1U << c->stepping
calculation), but that's now resulted in a spill/restore of %rbx in the
loop, and also doubled up most of the loop. I have no idea what it's
trying to do here...
>
>> #define X86_FEATURE_ANY X86_FEATURE_LM
>>
>> struct x86_cpu_id {
>> - uint16_t vendor;
>> - uint16_t family;
>> + uint8_t vendor;
> Is shrinking this to 8 bits a good idea? We use 5 of them already. (Of
> course we can re-enlarge later, if and when the need arises.)
It's the same size as cpuinfo_x86's field has been for 2 decades.
>
>> + uint8_t family;
> The family formula allows the value to be up to 0x10e. The return type
> of get_cpu_family() is therefore wrong too, strictly speaking. As is
> struct cpuinfo_x86's x86 field.
Again, this is the size of the field in cpuinfo_x86. I don't think
0x10e is anything we're going to have to worry about any time soon.
>
>> uint16_t model;
> Whereas the model is strictly limited to 8 bits.
There is space in here, if we need it, but you can't shrink it without
breaking the check for the NULL entry (going back to the first obfuscation).
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-17 19:39 ` Andrew Cooper
@ 2025-07-18 5:53 ` Jan Beulich
2025-07-18 10:29 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 5:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 17.07.2025 21:39, Andrew Cooper wrote:
> On 17/07/2025 9:11 am, Jan Beulich wrote:
>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
>>> const struct x86_cpu_id *m;
>>> const struct cpuinfo_x86 *c = &boot_cpu_data;
>>>
>>> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
>>> + for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
>> Nit: Line length. But - do we need the change at all? It looks entirely
>> implausible to me to use ->steppings with all of vendor, family, and
>> model being *_ANY (if, as per below, they would be 0 in the first place).
>
> I do keep on saying that | like this is pure obfuscation. This is an
> excellent example.
>
> It's looking for the {} entry, by looking for 0's in all of the metadata
> fields. A better check would be *(uint64_t *)m, or perhaps a unioned
> metadata field, but..
>
> This is also a good demonstration of binary | is a bad thing to use, not
> only for legibility. Swapping | for || lets the compiler do:
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-76 (-76)
> Function old new delta
> x86_match_cpu 243 167 -76
>
> and the code generation looks much better too:
Feel free to switch to ||. (The use of | producing worse code is clearly
a weakness of the compiler. Especially when used on non-adjacent fields
I expect | to be quite a bit better, first and foremost by ending up
with just a single conditional branch. Sadly I haven't seen compilers
do such a transformation for us.)
All of your reply doesn't address my remark regarding whether to check
->steppings here, though. (And no, whether to check it shouldn't be
[solely] justified by the compiler generating better code that way.)
>>> struct x86_cpu_id {
>>> - uint16_t vendor;
>>> - uint16_t family;
>>> + uint8_t vendor;
>> Is shrinking this to 8 bits a good idea? We use 5 of them already. (Of
>> course we can re-enlarge later, if and when the need arises.)
>
> It's the same size as cpuinfo_x86's field has been for 2 decades.
>
>>
>>> + uint8_t family;
>> The family formula allows the value to be up to 0x10e. The return type
>> of get_cpu_family() is therefore wrong too, strictly speaking. As is
>> struct cpuinfo_x86's x86 field.
>
> Again, this is the size of the field in cpuinfo_x86. I don't think
> 0x10e is anything we're going to have to worry about any time soon.
Now that Intel has decided to use higher family numbers, hopefully yes.
>>> uint16_t model;
>> Whereas the model is strictly limited to 8 bits.
>
> There is space in here, if we need it, but you can't shrink it without
> breaking the check for the NULL entry (going back to the first obfuscation).
Breaking? Or merely affecting code generation in a negative way?
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-18 5:53 ` Jan Beulich
@ 2025-07-18 10:29 ` Andrew Cooper
2025-07-18 13:28 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 10:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 18/07/2025 6:53 am, Jan Beulich wrote:
> On 17.07.2025 21:39, Andrew Cooper wrote:
>> On 17/07/2025 9:11 am, Jan Beulich wrote:
>>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/cpu/common.c
>>>> +++ b/xen/arch/x86/cpu/common.c
>>>> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
>>>> const struct x86_cpu_id *m;
>>>> const struct cpuinfo_x86 *c = &boot_cpu_data;
>>>>
>>>> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
>>>> + for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
>>> Nit: Line length. But - do we need the change at all? It looks entirely
>>> implausible to me to use ->steppings with all of vendor, family, and
>>> model being *_ANY (if, as per below, they would be 0 in the first place).
>> I do keep on saying that | like this is pure obfuscation. This is an
>> excellent example.
>>
>> It's looking for the {} entry, by looking for 0's in all of the metadata
>> fields. A better check would be *(uint64_t *)m, or perhaps a unioned
>> metadata field, but..
>>
>> This is also a good demonstration of binary | is a bad thing to use, not
>> only for legibility. Swapping | for || lets the compiler do:
>>
>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-76 (-76)
>> Function old new delta
>> x86_match_cpu 243 167 -76
>>
>> and the code generation looks much better too:
> Feel free to switch to ||. (The use of | producing worse code is clearly
> a weakness of the compiler. Especially when used on non-adjacent fields
> I expect | to be quite a bit better, first and foremost by ending up
> with just a single conditional branch. Sadly I haven't seen compilers
> do such a transformation for us.)
>
> All of your reply doesn't address my remark regarding whether to check
> ->steppings here, though. (And no, whether to check it shouldn't be
> [solely] justified by the compiler generating better code that way.)
Well, as stated: "It's looking for the {} entry, by looking for 0's in
all of the metadata fields."
The intended usage of ->steppings, or ->feature for that matter, is not
relevant to the loop termination condition, which is simply "is all the
metadata 0".
>>>> uint16_t model;
>>> Whereas the model is strictly limited to 8 bits.
>> There is space in here, if we need it, but you can't shrink it without
>> breaking the check for the NULL entry (going back to the first obfuscation).
> Breaking? Or merely affecting code generation in a negative way?
Shrinking model without adding (and checking) a new field would mean the
loop condition no longer covers all metadata.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-18 10:29 ` Andrew Cooper
@ 2025-07-18 13:28 ` Jan Beulich
2025-07-18 13:48 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 13:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 18.07.2025 12:29, Andrew Cooper wrote:
> On 18/07/2025 6:53 am, Jan Beulich wrote:
>> On 17.07.2025 21:39, Andrew Cooper wrote:
>>> On 17/07/2025 9:11 am, Jan Beulich wrote:
>>>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])
>>>>> const struct x86_cpu_id *m;
>>>>> const struct cpuinfo_x86 *c = &boot_cpu_data;
>>>>>
>>>>> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
>>>>> + for (m = table; m->vendor | m->family | m->model | m->steppings | m->feature; m++) {
>>>> Nit: Line length. But - do we need the change at all? It looks entirely
>>>> implausible to me to use ->steppings with all of vendor, family, and
>>>> model being *_ANY (if, as per below, they would be 0 in the first place).
>>> I do keep on saying that | like this is pure obfuscation. This is an
>>> excellent example.
>>>
>>> It's looking for the {} entry, by looking for 0's in all of the metadata
>>> fields. A better check would be *(uint64_t *)m, or perhaps a unioned
>>> metadata field, but..
>>>
>>> This is also a good demonstration of binary | is a bad thing to use, not
>>> only for legibility. Swapping | for || lets the compiler do:
>>>
>>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-76 (-76)
>>> Function old new delta
>>> x86_match_cpu 243 167 -76
>>>
>>> and the code generation looks much better too:
>> Feel free to switch to ||. (The use of | producing worse code is clearly
>> a weakness of the compiler. Especially when used on non-adjacent fields
>> I expect | to be quite a bit better, first and foremost by ending up
>> with just a single conditional branch. Sadly I haven't seen compilers
>> do such a transformation for us.)
>>
>> All of your reply doesn't address my remark regarding whether to check
>> ->steppings here, though. (And no, whether to check it shouldn't be
>> [solely] justified by the compiler generating better code that way.)
>
> Well, as stated: "It's looking for the {} entry, by looking for 0's in
> all of the metadata fields."
>
> The intended usage of ->steppings, or ->feature for that matter, is not
> relevant to the loop termination condition, which is simply "is all the
> metadata 0".
>
>>>>> uint16_t model;
>>>> Whereas the model is strictly limited to 8 bits.
>>> There is space in here, if we need it, but you can't shrink it without
>>> breaking the check for the NULL entry (going back to the first obfuscation).
>> Breaking? Or merely affecting code generation in a negative way?
>
> Shrinking model without adding (and checking) a new field would mean the
> loop condition no longer covers all metadata.
And it doesn't strictly need to. It needs to check enough to not mistake a
valid entry for a sentinel one.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings
2025-07-18 13:28 ` Jan Beulich
@ 2025-07-18 13:48 ` Andrew Cooper
0 siblings, 0 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 13:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 18/07/2025 2:28 pm, Jan Beulich wrote:
>>>>>> uint16_t model;
>>>>> Whereas the model is strictly limited to 8 bits.
>>>> There is space in here, if we need it, but you can't shrink it without
>>>> breaking the check for the NULL entry (going back to the first obfuscation).
>>> Breaking? Or merely affecting code generation in a negative way?
>> Shrinking model without adding (and checking) a new field would mean the
>> loop condition no longer covers all metadata.
> And it doesn't strictly need to. It needs to check enough to not mistake a
> valid entry for a sentinel one.
I've found a nicer way of doing this, but it needs another prereq
patch. I'm preparing a v2 with the remainder.
It also addresses the horrible code generation.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-16 17:31 [PATCH 0/6] x86: Convert x86_cpu_id to VFM Andrew Cooper
` (4 preceding siblings ...)
2025-07-16 17:31 ` [PATCH 5/6] x86/match-cpu: Support matching on steppings Andrew Cooper
@ 2025-07-16 17:31 ` Andrew Cooper
2025-07-17 8:26 ` Jan Beulich
` (2 more replies)
5 siblings, 3 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-16 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
helper to match a specific stepping, and use it to rework deadline_match[].
Notably this removes the overloading of driver_data possibly being a function
pointer, and removes the latent bug where the target functions are missing
ENDBR instructions owing to the lack of the cf_check attribute.
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>
The bloat-o-meter summary shows that the use of functions really wasn't the
wisest idea:
add/remove: 0/3 grow/shrink: 1/2 up/down: 80/-146 (-66)
Function old new delta
deadline_match 224 304 +80
APIC_init_uniprocessor 334 331 -3
skx_deadline_rev 30 - -30
CSWTCH 335 299 -36
hsx_deadline_rev 38 - -38
bdx_deadline_rev 39 - -39
---
xen/arch/x86/apic.c | 79 +++++++---------------------
xen/arch/x86/include/asm/match-cpu.h | 5 ++
2 files changed, 25 insertions(+), 59 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index c4272ab4de4f..744124185189 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1051,64 +1051,32 @@ static void setup_APIC_timer(void)
local_irq_restore(flags);
}
-#define DEADLINE_MODEL_MATCH(m, fr) \
- { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
- .feature = X86_FEATURE_TSC_DEADLINE, \
- .driver_data = (void *)(unsigned long)(fr) }
+static const struct x86_cpu_id __initconst deadline_match[] = {
+ X86_MATCH_VFMS(INTEL_HASWELL_X, 0x2, 0x3a), /* EP */
+ X86_MATCH_VFMS(INTEL_HASWELL_X, 0x4, 0x0f), /* EX */
-static unsigned int __init hsx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x02: return 0x3a; /* EP */
- case 0x04: return 0x0f; /* EX */
- }
+ X86_MATCH_VFM (INTEL_BROADWELL_X, 0x0b000020),
- return ~0U;
-}
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x2, 0x00000011),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x3, 0x0700000e),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x4, 0x0f00000c),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x5, 0x0e000003),
-static unsigned int __init bdx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x02: return 0x00000011;
- case 0x03: return 0x0700000e;
- case 0x04: return 0x0f00000c;
- case 0x05: return 0x0e000003;
- }
+ X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x3, 0x01000136),
+ X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x4, 0x02000014),
- return ~0U;
-}
+ X86_MATCH_VFM (INTEL_HASWELL, 0x22),
+ X86_MATCH_VFM (INTEL_HASWELL_L, 0x20),
+ X86_MATCH_VFM (INTEL_HASWELL_G, 0x17),
-static unsigned int __init skx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x00 ... 0x02: return ~0U;
- case 0x03: return 0x01000136;
- case 0x04: return 0x02000014;
- }
-
- return 0;
-}
-
-static const struct x86_cpu_id __initconstrel deadline_match[] = {
- DEADLINE_MODEL_MATCH(0x3c, 0x22), /* Haswell */
- DEADLINE_MODEL_MATCH(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
- DEADLINE_MODEL_MATCH(0x45, 0x20), /* Haswell D */
- DEADLINE_MODEL_MATCH(0x46, 0x17), /* Haswell H */
+ X86_MATCH_VFM (INTEL_BROADWELL, 0x25),
+ X86_MATCH_VFM (INTEL_BROADWELL_G, 0x17),
- DEADLINE_MODEL_MATCH(0x3d, 0x25), /* Broadwell */
- DEADLINE_MODEL_MATCH(0x47, 0x17), /* Broadwell H */
- DEADLINE_MODEL_MATCH(0x4f, 0x0b000020), /* Broadwell EP/EX */
- DEADLINE_MODEL_MATCH(0x56, bdx_deadline_rev), /* Broadwell D */
+ X86_MATCH_VFM (INTEL_SKYLAKE_L, 0xb2),
+ X86_MATCH_VFM (INTEL_SKYLAKE, 0xb2),
- DEADLINE_MODEL_MATCH(0x4e, 0xb2), /* Skylake M */
- DEADLINE_MODEL_MATCH(0x55, skx_deadline_rev), /* Skylake X */
- DEADLINE_MODEL_MATCH(0x5e, 0xb2), /* Skylake D */
-
- DEADLINE_MODEL_MATCH(0x8e, 0x52), /* Kabylake M */
- DEADLINE_MODEL_MATCH(0x9e, 0x52), /* Kabylake D */
+ X86_MATCH_VFM (INTEL_KABYLAKE_L, 0x52),
+ X86_MATCH_VFM (INTEL_KABYLAKE, 0x52),
{}
};
@@ -1125,14 +1093,7 @@ static void __init check_deadline_errata(void)
if ( !m )
return;
- /*
- * Function pointers will have the MSB set due to address layout,
- * immediate revisions will not.
- */
- if ( (long)m->driver_data < 0 )
- rev = ((unsigned int (*)(void))(m->driver_data))();
- else
- rev = (unsigned long)m->driver_data;
+ rev = (unsigned long)m->driver_data;
if ( this_cpu(cpu_sig).rev >= rev )
return;
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
index 3862e766ccfc..b491232c351f 100644
--- a/xen/arch/x86/include/asm/match-cpu.h
+++ b/xen/arch/x86/include/asm/match-cpu.h
@@ -35,6 +35,11 @@ struct x86_cpu_id {
VFM_MODEL(vfm), X86_STEPPINGS_ANY, \
X86_FEATURE_ANY, data)
+#define X86_MATCH_VFMS(vfm, stepping, data) \
+ X86_MATCH_CPU(VFM_VENDOR(vfm), VFM_FAMILY(vfm), \
+ VFM_MODEL(vfm), 1U << (stepping), \
+ X86_FEATURE_ANY, data)
+
/*
* x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
*
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-16 17:31 ` [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*() Andrew Cooper
@ 2025-07-17 8:26 ` Jan Beulich
2025-07-17 9:02 ` Andrew Cooper
2025-07-17 8:31 ` Jan Beulich
2025-07-18 10:07 ` [PATCH v1.1 " Andrew Cooper
2 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 8:26 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
> helper to match a specific stepping, and use it to rework deadline_match[].
I'm fine with the patch in principle, but I wonder how you envision to support
a match for multiple steppings in one go then. In particular macro-naming-wise.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-17 8:26 ` Jan Beulich
@ 2025-07-17 9:02 ` Andrew Cooper
2025-07-17 9:33 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 9:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 9:26 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>> helper to match a specific stepping, and use it to rework deadline_match[].
> I'm fine with the patch in principle, but I wonder how you envision to support
> a match for multiple steppings in one go then. In particular macro-naming-wise.
The Linux version uses
X86_MATCH_VFM_STEPS(vfm, min_step, max_step, data)
and calls GENMASK(min_step, max_step) but for a single stepping that
causes rows which look like:
X86_MATCH_VFM_STEPS(INTEL_HASWELL_X, 0x2, 0x2, 0x3a), /* EP */
Even in Linux, there are very few examples which take a genuine range,
and nothing so far that we need in Xen, so I implemented a slightly
different helper.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-17 9:02 ` Andrew Cooper
@ 2025-07-17 9:33 ` Jan Beulich
2025-07-17 19:40 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 9:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 17.07.2025 11:02, Andrew Cooper wrote:
> On 17/07/2025 9:26 am, Jan Beulich wrote:
>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>> helper to match a specific stepping, and use it to rework deadline_match[].
>> I'm fine with the patch in principle, but I wonder how you envision to support
>> a match for multiple steppings in one go then. In particular macro-naming-wise.
>
> The Linux version uses
>
> X86_MATCH_VFM_STEPS(vfm, min_step, max_step, data)
Hmm, yes, something like that (naming-wise at least) may be possible to use.
It'll be potentially a little confusing, but I guess we'll manage. Or maybe ...
> and calls GENMASK(min_step, max_step) but for a single stepping that
> causes rows which look like:
>
> X86_MATCH_VFM_STEPS(INTEL_HASWELL_X, 0x2, 0x2, 0x3a), /* EP */
>
>
> Even in Linux, there are very few examples which take a genuine range,
> and nothing so far that we need in Xen, so I implemented a slightly
> different helper.
... we get away without ever needing such.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-17 9:33 ` Jan Beulich
@ 2025-07-17 19:40 ` Andrew Cooper
0 siblings, 0 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 19:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 10:33 am, Jan Beulich wrote:
> On 17.07.2025 11:02, Andrew Cooper wrote:
>> On 17/07/2025 9:26 am, Jan Beulich wrote:
>>> On 16.07.2025 19:31, Andrew Cooper wrote:
>>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>>> helper to match a specific stepping, and use it to rework deadline_match[].
>>> I'm fine with the patch in principle, but I wonder how you envision to support
>>> a match for multiple steppings in one go then. In particular macro-naming-wise.
>> The Linux version uses
>>
>> X86_MATCH_VFM_STEPS(vfm, min_step, max_step, data)
> Hmm, yes, something like that (naming-wise at least) may be possible to use.
> It'll be potentially a little confusing, but I guess we'll manage. Or maybe ...
>
>> and calls GENMASK(min_step, max_step) but for a single stepping that
>> causes rows which look like:
>>
>> X86_MATCH_VFM_STEPS(INTEL_HASWELL_X, 0x2, 0x2, 0x3a), /* EP */
>>
>>
>> Even in Linux, there are very few examples which take a genuine range,
>> and nothing so far that we need in Xen, so I implemented a slightly
>> different helper.
> ... we get away without ever needing such.
We will want it in order to convert spec_ctrl.c
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-16 17:31 ` [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*() Andrew Cooper
2025-07-17 8:26 ` Jan Beulich
@ 2025-07-17 8:31 ` Jan Beulich
2025-07-17 19:45 ` Andrew Cooper
2025-07-18 10:07 ` [PATCH v1.1 " Andrew Cooper
2 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-17 8:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 16.07.2025 19:31, Andrew Cooper wrote:
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1051,64 +1051,32 @@ static void setup_APIC_timer(void)
> local_irq_restore(flags);
> }
>
> -#define DEADLINE_MODEL_MATCH(m, fr) \
> - { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
> - .feature = X86_FEATURE_TSC_DEADLINE, \
> - .driver_data = (void *)(unsigned long)(fr) }
> +static const struct x86_cpu_id __initconst deadline_match[] = {
> + X86_MATCH_VFMS(INTEL_HASWELL_X, 0x2, 0x3a), /* EP */
> + X86_MATCH_VFMS(INTEL_HASWELL_X, 0x4, 0x0f), /* EX */
>
> -static unsigned int __init hsx_deadline_rev(void)
> -{
> - switch ( boot_cpu_data.x86_mask )
> - {
> - case 0x02: return 0x3a; /* EP */
> - case 0x04: return 0x0f; /* EX */
> - }
> + X86_MATCH_VFM (INTEL_BROADWELL_X, 0x0b000020),
>
> - return ~0U;
> -}
> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x2, 0x00000011),
> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x3, 0x0700000e),
> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x4, 0x0f00000c),
> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x5, 0x0e000003),
Hmm, actually - why are Broadwell and ...
> -static unsigned int __init bdx_deadline_rev(void)
> -{
> - switch ( boot_cpu_data.x86_mask )
> - {
> - case 0x02: return 0x00000011;
> - case 0x03: return 0x0700000e;
> - case 0x04: return 0x0f00000c;
> - case 0x05: return 0x0e000003;
> - }
> + X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x3, 0x01000136),
> + X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x4, 0x02000014),
... Skylake each split ...
> - return ~0U;
> -}
> + X86_MATCH_VFM (INTEL_HASWELL, 0x22),
> + X86_MATCH_VFM (INTEL_HASWELL_L, 0x20),
> + X86_MATCH_VFM (INTEL_HASWELL_G, 0x17),
>
> -static unsigned int __init skx_deadline_rev(void)
> -{
> - switch ( boot_cpu_data.x86_mask )
> - {
> - case 0x00 ... 0x02: return ~0U;
> - case 0x03: return 0x01000136;
> - case 0x04: return 0x02000014;
> - }
> -
> - return 0;
> -}
> -
> -static const struct x86_cpu_id __initconstrel deadline_match[] = {
> - DEADLINE_MODEL_MATCH(0x3c, 0x22), /* Haswell */
> - DEADLINE_MODEL_MATCH(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
> - DEADLINE_MODEL_MATCH(0x45, 0x20), /* Haswell D */
> - DEADLINE_MODEL_MATCH(0x46, 0x17), /* Haswell H */
> + X86_MATCH_VFM (INTEL_BROADWELL, 0x25),
> + X86_MATCH_VFM (INTEL_BROADWELL_G, 0x17),
... into disjoint groups (continuing ...
> - DEADLINE_MODEL_MATCH(0x3d, 0x25), /* Broadwell */
> - DEADLINE_MODEL_MATCH(0x47, 0x17), /* Broadwell H */
> - DEADLINE_MODEL_MATCH(0x4f, 0x0b000020), /* Broadwell EP/EX */
> - DEADLINE_MODEL_MATCH(0x56, bdx_deadline_rev), /* Broadwell D */
> + X86_MATCH_VFM (INTEL_SKYLAKE_L, 0xb2),
> + X86_MATCH_VFM (INTEL_SKYLAKE, 0xb2),
... here)? The patch already isn't overly straightforward to review without
that.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-17 8:31 ` Jan Beulich
@ 2025-07-17 19:45 ` Andrew Cooper
0 siblings, 0 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-17 19:45 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/07/2025 9:31 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1051,64 +1051,32 @@ static void setup_APIC_timer(void)
>> local_irq_restore(flags);
>> }
>>
>> -#define DEADLINE_MODEL_MATCH(m, fr) \
>> - { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
>> - .feature = X86_FEATURE_TSC_DEADLINE, \
>> - .driver_data = (void *)(unsigned long)(fr) }
>> +static const struct x86_cpu_id __initconst deadline_match[] = {
>> + X86_MATCH_VFMS(INTEL_HASWELL_X, 0x2, 0x3a), /* EP */
>> + X86_MATCH_VFMS(INTEL_HASWELL_X, 0x4, 0x0f), /* EX */
>>
>> -static unsigned int __init hsx_deadline_rev(void)
>> -{
>> - switch ( boot_cpu_data.x86_mask )
>> - {
>> - case 0x02: return 0x3a; /* EP */
>> - case 0x04: return 0x0f; /* EX */
>> - }
>> + X86_MATCH_VFM (INTEL_BROADWELL_X, 0x0b000020),
>>
>> - return ~0U;
>> -}
>> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x2, 0x00000011),
>> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x3, 0x0700000e),
>> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x4, 0x0f00000c),
>> + X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x5, 0x0e000003),
> Hmm, actually - why are Broadwell and ...
>
>> -static unsigned int __init bdx_deadline_rev(void)
>> -{
>> - switch ( boot_cpu_data.x86_mask )
>> - {
>> - case 0x02: return 0x00000011;
>> - case 0x03: return 0x0700000e;
>> - case 0x04: return 0x0f00000c;
>> - case 0x05: return 0x0e000003;
>> - }
>> + X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x3, 0x01000136),
>> + X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x4, 0x02000014),
> ... Skylake each split ...
>
>> - return ~0U;
>> -}
>> + X86_MATCH_VFM (INTEL_HASWELL, 0x22),
>> + X86_MATCH_VFM (INTEL_HASWELL_L, 0x20),
>> + X86_MATCH_VFM (INTEL_HASWELL_G, 0x17),
>>
>> -static unsigned int __init skx_deadline_rev(void)
>> -{
>> - switch ( boot_cpu_data.x86_mask )
>> - {
>> - case 0x00 ... 0x02: return ~0U;
>> - case 0x03: return 0x01000136;
>> - case 0x04: return 0x02000014;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -static const struct x86_cpu_id __initconstrel deadline_match[] = {
>> - DEADLINE_MODEL_MATCH(0x3c, 0x22), /* Haswell */
>> - DEADLINE_MODEL_MATCH(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
>> - DEADLINE_MODEL_MATCH(0x45, 0x20), /* Haswell D */
>> - DEADLINE_MODEL_MATCH(0x46, 0x17), /* Haswell H */
>> + X86_MATCH_VFM (INTEL_BROADWELL, 0x25),
>> + X86_MATCH_VFM (INTEL_BROADWELL_G, 0x17),
> ... into disjoint groups (continuing ...
>
>> - DEADLINE_MODEL_MATCH(0x3d, 0x25), /* Broadwell */
>> - DEADLINE_MODEL_MATCH(0x47, 0x17), /* Broadwell H */
>> - DEADLINE_MODEL_MATCH(0x4f, 0x0b000020), /* Broadwell EP/EX */
>> - DEADLINE_MODEL_MATCH(0x56, bdx_deadline_rev), /* Broadwell D */
>> + X86_MATCH_VFM (INTEL_SKYLAKE_L, 0xb2),
>> + X86_MATCH_VFM (INTEL_SKYLAKE, 0xb2),
> ... here)? The patch already isn't overly straightforward to review without
> that.
The layout comes from Linux (I was mostly checking that I hadn't broken
anything), although I took the opportunity to optimise the table by
dropping useless rows.
I can't find any way of getting the diff to read nicely. My normal
trick of reordering with a function doesn't work, although it turns out
that removing the blank lines and moving it into check_deadline_errata()
does render nicely. I'll do that in v2.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-16 17:31 ` [PATCH 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*() Andrew Cooper
2025-07-17 8:26 ` Jan Beulich
2025-07-17 8:31 ` Jan Beulich
@ 2025-07-18 10:07 ` Andrew Cooper
2025-07-18 10:19 ` Jan Beulich
2 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 10:07 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
helper to match a specific stepping, and use it to rework deadline_match[].
Notably this removes the overloading of driver_data possibly being a function
pointer, and removes the latent bug where the target functions are missing
ENDBR instructions owing to the lack of the cf_check attribute.
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>
v2:
* Move deadline_match[] into check_deadline_errata() which produces a far
more legible diff.
The bloat-o-meter summary shows that the use of functions really wasn't the
wisest idea:
add/remove: 0/3 grow/shrink: 1/2 up/down: 80/-146 (-66)
Function old new delta
deadline_match 224 304 +80
APIC_init_uniprocessor 334 331 -3
skx_deadline_rev 30 - -30
CSWTCH 335 299 -36
hsx_deadline_rev 38 - -38
bdx_deadline_rev 39 - -39
---
xen/arch/x86/apic.c | 93 +++++++---------------------
xen/arch/x86/include/asm/match-cpu.h | 5 ++
2 files changed, 28 insertions(+), 70 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index c4272ab4de4f..c4a27fa00230 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1051,70 +1051,30 @@ static void setup_APIC_timer(void)
local_irq_restore(flags);
}
-#define DEADLINE_MODEL_MATCH(m, fr) \
- { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
- .feature = X86_FEATURE_TSC_DEADLINE, \
- .driver_data = (void *)(unsigned long)(fr) }
-
-static unsigned int __init hsx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x02: return 0x3a; /* EP */
- case 0x04: return 0x0f; /* EX */
- }
-
- return ~0U;
-}
-
-static unsigned int __init bdx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x02: return 0x00000011;
- case 0x03: return 0x0700000e;
- case 0x04: return 0x0f00000c;
- case 0x05: return 0x0e000003;
- }
-
- return ~0U;
-}
-
-static unsigned int __init skx_deadline_rev(void)
-{
- switch ( boot_cpu_data.x86_mask )
- {
- case 0x00 ... 0x02: return ~0U;
- case 0x03: return 0x01000136;
- case 0x04: return 0x02000014;
- }
-
- return 0;
-}
-
-static const struct x86_cpu_id __initconstrel deadline_match[] = {
- DEADLINE_MODEL_MATCH(0x3c, 0x22), /* Haswell */
- DEADLINE_MODEL_MATCH(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
- DEADLINE_MODEL_MATCH(0x45, 0x20), /* Haswell D */
- DEADLINE_MODEL_MATCH(0x46, 0x17), /* Haswell H */
-
- DEADLINE_MODEL_MATCH(0x3d, 0x25), /* Broadwell */
- DEADLINE_MODEL_MATCH(0x47, 0x17), /* Broadwell H */
- DEADLINE_MODEL_MATCH(0x4f, 0x0b000020), /* Broadwell EP/EX */
- DEADLINE_MODEL_MATCH(0x56, bdx_deadline_rev), /* Broadwell D */
-
- DEADLINE_MODEL_MATCH(0x4e, 0xb2), /* Skylake M */
- DEADLINE_MODEL_MATCH(0x55, skx_deadline_rev), /* Skylake X */
- DEADLINE_MODEL_MATCH(0x5e, 0xb2), /* Skylake D */
-
- DEADLINE_MODEL_MATCH(0x8e, 0x52), /* Kabylake M */
- DEADLINE_MODEL_MATCH(0x9e, 0x52), /* Kabylake D */
-
- {}
-};
-
static void __init check_deadline_errata(void)
{
+ static const struct x86_cpu_id __initconst deadline_match[] = {
+ X86_MATCH_VFM (INTEL_HASWELL, 0x22),
+ X86_MATCH_VFMS(INTEL_HASWELL_X, 0x2, 0x3a),
+ X86_MATCH_VFMS(INTEL_HASWELL_X, 0x4, 0x0f),
+ X86_MATCH_VFM (INTEL_HASWELL_L, 0x20),
+ X86_MATCH_VFM (INTEL_HASWELL_G, 0x17),
+ X86_MATCH_VFM (INTEL_BROADWELL, 0x25),
+ X86_MATCH_VFM (INTEL_BROADWELL_G, 0x17),
+ X86_MATCH_VFM (INTEL_BROADWELL_X, 0x0b000020),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x2, 0x00000011),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x3, 0x0700000e),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x4, 0x0f00000c),
+ X86_MATCH_VFMS(INTEL_BROADWELL_D, 0x5, 0x0e000003),
+ X86_MATCH_VFM (INTEL_SKYLAKE_L, 0xb2),
+ X86_MATCH_VFM (INTEL_SKYLAKE, 0xb2),
+ X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x3, 0x01000136),
+ X86_MATCH_VFMS(INTEL_SKYLAKE_X, 0x4, 0x02000014),
+ X86_MATCH_VFM (INTEL_KABYLAKE_L, 0x52),
+ X86_MATCH_VFM (INTEL_KABYLAKE, 0x52),
+ {}
+ };
+
const struct x86_cpu_id *m;
unsigned int rev;
@@ -1125,14 +1085,7 @@ static void __init check_deadline_errata(void)
if ( !m )
return;
- /*
- * Function pointers will have the MSB set due to address layout,
- * immediate revisions will not.
- */
- if ( (long)m->driver_data < 0 )
- rev = ((unsigned int (*)(void))(m->driver_data))();
- else
- rev = (unsigned long)m->driver_data;
+ rev = (unsigned long)m->driver_data;
if ( this_cpu(cpu_sig).rev >= rev )
return;
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
index 3862e766ccfc..b491232c351f 100644
--- a/xen/arch/x86/include/asm/match-cpu.h
+++ b/xen/arch/x86/include/asm/match-cpu.h
@@ -35,6 +35,11 @@ struct x86_cpu_id {
VFM_MODEL(vfm), X86_STEPPINGS_ANY, \
X86_FEATURE_ANY, data)
+#define X86_MATCH_VFMS(vfm, stepping, data) \
+ X86_MATCH_CPU(VFM_VENDOR(vfm), VFM_FAMILY(vfm), \
+ VFM_MODEL(vfm), 1U << (stepping), \
+ X86_FEATURE_ANY, data)
+
/*
* x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
*
--
2.39.5
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 10:07 ` [PATCH v1.1 " Andrew Cooper
@ 2025-07-18 10:19 ` Jan Beulich
2025-07-18 10:23 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 10:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 18.07.2025 12:07, Andrew Cooper wrote:
> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
> helper to match a specific stepping, and use it to rework deadline_match[].
>
> Notably this removes the overloading of driver_data possibly being a function
> pointer, and removes the latent bug where the target functions are missing
> ENDBR instructions owing to the lack of the cf_check attribute.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> -static const struct x86_cpu_id __initconstrel deadline_match[] = {
Seeing this transformation ...
> static void __init check_deadline_errata(void)
> {
> + static const struct x86_cpu_id __initconst deadline_match[] = {
... of the section placement, we may want to investigate whether with the
toolchain baseline bump we can actually do away with __initconstrel, using
__initconst uniformly everywhere.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 10:19 ` Jan Beulich
@ 2025-07-18 10:23 ` Andrew Cooper
2025-07-18 10:55 ` Andrew Cooper
2025-07-18 14:15 ` Jan Beulich
0 siblings, 2 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 10:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 18/07/2025 11:19 am, Jan Beulich wrote:
> On 18.07.2025 12:07, Andrew Cooper wrote:
>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>> helper to match a specific stepping, and use it to rework deadline_match[].
>>
>> Notably this removes the overloading of driver_data possibly being a function
>> pointer, and removes the latent bug where the target functions are missing
>> ENDBR instructions owing to the lack of the cf_check attribute.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
>
>> -static const struct x86_cpu_id __initconstrel deadline_match[] = {
> Seeing this transformation ...
>
>> static void __init check_deadline_errata(void)
>> {
>> + static const struct x86_cpu_id __initconst deadline_match[] = {
> ... of the section placement, we may want to investigate whether with the
> toolchain baseline bump we can actually do away with __initconstrel, using
> __initconst uniformly everywhere.
To be honest, I'm not even sure why we needed the split in the first
place. We merge both sections together, so it isn't about section
attributes.
But, if you think it's safe to remove, it will definitely be a good
amplification.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 10:23 ` Andrew Cooper
@ 2025-07-18 10:55 ` Andrew Cooper
2025-07-18 14:06 ` Jan Beulich
2025-07-18 14:15 ` Jan Beulich
1 sibling, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 10:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 18/07/2025 11:23 am, Andrew Cooper wrote:
> On 18/07/2025 11:19 am, Jan Beulich wrote:
>> On 18.07.2025 12:07, Andrew Cooper wrote:
>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>> helper to match a specific stepping, and use it to rework deadline_match[].
>>>
>>> Notably this removes the overloading of driver_data possibly being a function
>>> pointer, and removes the latent bug where the target functions are missing
>>> ENDBR instructions owing to the lack of the cf_check attribute.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Thanks.
Actually, this isn't as no-functional-change as I thought.
X86_FEATURE_TSC_DEADLINE has been swapped for X86_FEATURE_ANY in the table.
check_deadline_errata() is called unconditionally, without checking for
TSC_DEADLINE, yet the rows in the table are the CPUs for which an
erratum is known, so they all have the feature.
It does make a difference if e.g. one were to boot with
cpuid=no-tsc-deadline. Previously we'd have exited early, while now
we'll emit the warning.
We could switch back to using TSC_DEADLINE (requiring a more complicated
X86_MATCH_*() wrapper), although a better option would be to predicate
the call to check_deadline_errata() with a feature check, because it's a
much more recent addition to AMD CPUs, and there's no point searching
the errata list on CPUs which lack the feature.
Thoughts?
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 10:55 ` Andrew Cooper
@ 2025-07-18 14:06 ` Jan Beulich
2025-07-18 14:10 ` Andrew Cooper
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 14:06 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 18.07.2025 12:55, Andrew Cooper wrote:
> On 18/07/2025 11:23 am, Andrew Cooper wrote:
>> On 18/07/2025 11:19 am, Jan Beulich wrote:
>>> On 18.07.2025 12:07, Andrew Cooper wrote:
>>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>>> helper to match a specific stepping, and use it to rework deadline_match[].
>>>>
>>>> Notably this removes the overloading of driver_data possibly being a function
>>>> pointer, and removes the latent bug where the target functions are missing
>>>> ENDBR instructions owing to the lack of the cf_check attribute.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> Thanks.
>
> Actually, this isn't as no-functional-change as I thought.
>
> X86_FEATURE_TSC_DEADLINE has been swapped for X86_FEATURE_ANY in the table.
>
> check_deadline_errata() is called unconditionally, without checking for
> TSC_DEADLINE, yet the rows in the table are the CPUs for which an
> erratum is known, so they all have the feature.
>
> It does make a difference if e.g. one were to boot with
> cpuid=no-tsc-deadline. Previously we'd have exited early, while now
> we'll emit the warning.
>
> We could switch back to using TSC_DEADLINE (requiring a more complicated
> X86_MATCH_*() wrapper), although a better option would be to predicate
> the call to check_deadline_errata() with a feature check, because it's a
> much more recent addition to AMD CPUs, and there's no point searching
> the errata list on CPUs which lack the feature.
To be honest in this case I'd be fine with either adjustment. Switching the
feature back is more consistent with the overall purpose of X86_MATCH_*(),
but as you say a table with every entry having the same feature named isn't
very useful to go through when the feature isn't there.
One option to keep things table based, yet still avoiding to run through
the entire table, would be to allow for a "negative" entry (which here
would simply be placed first in the table).
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 14:06 ` Jan Beulich
@ 2025-07-18 14:10 ` Andrew Cooper
0 siblings, 0 replies; 35+ messages in thread
From: Andrew Cooper @ 2025-07-18 14:10 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 18/07/2025 3:06 pm, Jan Beulich wrote:
> On 18.07.2025 12:55, Andrew Cooper wrote:
>> On 18/07/2025 11:23 am, Andrew Cooper wrote:
>>> On 18/07/2025 11:19 am, Jan Beulich wrote:
>>>> On 18.07.2025 12:07, Andrew Cooper wrote:
>>>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>>>> helper to match a specific stepping, and use it to rework deadline_match[].
>>>>>
>>>>> Notably this removes the overloading of driver_data possibly being a function
>>>>> pointer, and removes the latent bug where the target functions are missing
>>>>> ENDBR instructions owing to the lack of the cf_check attribute.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> Thanks.
>> Actually, this isn't as no-functional-change as I thought.
>>
>> X86_FEATURE_TSC_DEADLINE has been swapped for X86_FEATURE_ANY in the table.
>>
>> check_deadline_errata() is called unconditionally, without checking for
>> TSC_DEADLINE, yet the rows in the table are the CPUs for which an
>> erratum is known, so they all have the feature.
>>
>> It does make a difference if e.g. one were to boot with
>> cpuid=no-tsc-deadline. Previously we'd have exited early, while now
>> we'll emit the warning.
>>
>> We could switch back to using TSC_DEADLINE (requiring a more complicated
>> X86_MATCH_*() wrapper), although a better option would be to predicate
>> the call to check_deadline_errata() with a feature check, because it's a
>> much more recent addition to AMD CPUs, and there's no point searching
>> the errata list on CPUs which lack the feature.
> To be honest in this case I'd be fine with either adjustment. Switching the
> feature back is more consistent with the overall purpose of X86_MATCH_*(),
> but as you say a table with every entry having the same feature named isn't
> very useful to go through when the feature isn't there.
>
> One option to keep things table based, yet still avoiding to run through
> the entire table, would be to allow for a "negative" entry (which here
> would simply be placed first in the table).
Except that would be an example of { ANY, ANY, ANY, ANY,
ALT_NOT(TSC_DEADLINE) } which we can't express currently, and we also
said we didn't want to get into the habit of.
I'm going with one extra boot_cpu_has() check, and an expanded commit
message.
~Andrew
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()
2025-07-18 10:23 ` Andrew Cooper
2025-07-18 10:55 ` Andrew Cooper
@ 2025-07-18 14:15 ` Jan Beulich
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2025-07-18 14:15 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 18.07.2025 12:23, Andrew Cooper wrote:
> On 18/07/2025 11:19 am, Jan Beulich wrote:
>> On 18.07.2025 12:07, Andrew Cooper wrote:
>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>> helper to match a specific stepping, and use it to rework deadline_match[].
>>>
>>> Notably this removes the overloading of driver_data possibly being a function
>>> pointer, and removes the latent bug where the target functions are missing
>>> ENDBR instructions owing to the lack of the cf_check attribute.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.
>
>>
>>> -static const struct x86_cpu_id __initconstrel deadline_match[] = {
>> Seeing this transformation ...
>>
>>> static void __init check_deadline_errata(void)
>>> {
>>> + static const struct x86_cpu_id __initconst deadline_match[] = {
>> ... of the section placement, we may want to investigate whether with the
>> toolchain baseline bump we can actually do away with __initconstrel, using
>> __initconst uniformly everywhere.
>
> To be honest, I'm not even sure why we needed the split in the first
> place. We merge both sections together, so it isn't about section
> attributes.
It is about section attributes, but at assembly time. Even an up-to-date
gas will choke on certain conflicting section attributes, when multiple
section "declarations" are present. (Oddly enough I did fiddle with that
code earlier in the day, hence why I have a fresh impression of this
error appearing in practice.)
When you have only constant data (no relocations), the compiler ought to
request an "a" section, whereas when there are relocations it would
request an "aw" one (along the lines of why there is .data.rel.ro). Some
gcc versions and/or some gas versions conflicted in how custom
(__attribute__((section(...)))) sections would have their attributes
specified, causing assembly to fail.
> But, if you think it's safe to remove, it will definitely be a good
> amplification.
As to "think" - I'm not sure, but my recollection is that the issue was
with some gcc 4.x only (or binutils from that time frame).
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread