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
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
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
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
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
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
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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.