* [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure
@ 2024-11-20 20:24 Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 1/4] x86/cpu: Introduce new microcode matching helper Dave Hansen
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Dave Hansen @ 2024-11-20 20:24 UTC (permalink / raw)
To: linux-kernel; +Cc: x86, tglx, bp, kan.liang, Dave Hansen
x86 has generic CPU matching infrastructure. This lets you build
tables of CPUs with some property. It's mostly used for enumerating
model-specific features, but it is quite a bit more flexible than
that. In includes a facility to match steppings and microcode
versions. This generic infrastructure is built around 'struct
x86_cpu_id'.
There is a less generic, parallel CPU matching facility built around
'struct x86_cpu_desc'. It is used only for matching specific microcode
revisions. All of the 'struct x86_cpu_desc' users can be converted to
'struct x86_cpu_id'.
Do that conversion then remove the 'struct x86_cpu_desc'
infrastructure.
Testing or acks would be much appreciated!
--
events/intel/core.c | 72 +++++++++++++++++++-------------------
include/asm/cpu_device_id.h | 55 ++++++++++-------------------
kernel/cpu/amd.c | 8 ++--
kernel/cpu/match.c | 28 ++------------
4 files changed, 63 insertions(+), 100 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 1/4] x86/cpu: Introduce new microcode matching helper
2024-11-20 20:24 [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure Dave Hansen
@ 2024-11-20 20:24 ` Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id' Dave Hansen
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Dave Hansen @ 2024-11-20 20:24 UTC (permalink / raw)
To: linux-kernel; +Cc: x86, tglx, bp, kan.liang, Dave Hansen
From: Dave Hansen <dave.hansen@linux.intel.com>
The 'x86_cpu_id' and 'x86_cpu_desc' structures are very similar and
need to be consolidated. There is a microcode version matching
function for 'x86_cpu_desc' but not 'x86_cpu_id'.
Create one for 'x86_cpu_id'.
This essentially just leverages the x86_cpu_id->driver_data field
to replace the less generic x86_cpu_desc->x86_microcode_rev field.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/include/asm/cpu_device_id.h | 1 +
b/arch/x86/kernel/cpu/match.c | 11 +++++++++++
2 files changed, 12 insertions(+)
diff -puN arch/x86/include/asm/cpu_device_id.h~min-ucode-rev arch/x86/include/asm/cpu_device_id.h
--- a/arch/x86/include/asm/cpu_device_id.h~min-ucode-rev 2024-11-20 12:22:04.944380677 -0800
+++ b/arch/x86/include/asm/cpu_device_id.h 2024-11-20 12:22:04.948380830 -0800
@@ -278,5 +278,6 @@ struct x86_cpu_desc {
extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
+extern bool x86_match_min_microcode_rev(const struct x86_cpu_id *table);
#endif /* _ASM_X86_CPU_DEVICE_ID */
diff -puN arch/x86/kernel/cpu/match.c~min-ucode-rev arch/x86/kernel/cpu/match.c
--- a/arch/x86/kernel/cpu/match.c~min-ucode-rev 2024-11-20 12:22:04.944380677 -0800
+++ b/arch/x86/kernel/cpu/match.c 2024-11-20 12:22:04.948380830 -0800
@@ -86,3 +86,14 @@ bool x86_cpu_has_min_microcode_rev(const
return true;
}
EXPORT_SYMBOL_GPL(x86_cpu_has_min_microcode_rev);
+
+bool x86_match_min_microcode_rev(const struct x86_cpu_id *table)
+{
+ const struct x86_cpu_id *res = x86_match_cpu(table);
+
+ if (!res || res->driver_data > boot_cpu_data.microcode)
+ return false;
+
+ return true;
+}
+EXPORT_SYMBOL_GPL(x86_match_min_microcode_rev);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id'
2024-11-20 20:24 [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 1/4] x86/cpu: Introduce new microcode matching helper Dave Hansen
@ 2024-11-20 20:24 ` Dave Hansen
2024-11-21 9:04 ` Ingo Molnar
2024-11-20 20:24 ` [RFC][PATCH 3/4] x86/cpu: Move AMD erratum 1386 table over to 'x86_cpu_id' Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 4/4] x86/cpu: Remove 'x86_cpu_desc' infrastructure Dave Hansen
3 siblings, 1 reply; 6+ messages in thread
From: Dave Hansen @ 2024-11-20 20:24 UTC (permalink / raw)
To: linux-kernel; +Cc: x86, tglx, bp, kan.liang, Dave Hansen
From: Dave Hansen <dave.hansen@linux.intel.com>
The 'x86_cpu_desc' and 'x86_cpu_id' structures are very similar.
Reduce duplicate infrastructure by moving the few users of
'x86_cpu_id' to the much more common variant.
The existing X86_MATCH_VFM_STEPPINGS() helper matches ranges of
steppings. Introduce a new helper to match a single stepping to make
the macro use a bit less verbose.
I'm a _bit_ nervous about this because
X86_MATCH_VFM_STEPPING(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
and
X86_MATCH_VFM_STEPPINGS(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
look very similar but the second one is buggy. Any suggestions for
making this more foolproof would be appreciated.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/events/intel/core.c | 72 ++++++++++++++++-----------------
b/arch/x86/include/asm/cpu_device_id.h | 18 ++++++++
2 files changed, 54 insertions(+), 36 deletions(-)
diff -puN arch/x86/events/intel/core.c~zap-x86_cpu_desc arch/x86/events/intel/core.c
--- a/arch/x86/events/intel/core.c~zap-x86_cpu_desc 2024-11-20 12:22:05.468400705 -0800
+++ b/arch/x86/events/intel/core.c 2024-11-20 12:22:05.472400858 -0800
@@ -5340,42 +5340,42 @@ static __init void intel_clovertown_quir
x86_pmu.pebs_constraints = NULL;
}
-static const struct x86_cpu_desc isolation_ucodes[] = {
- INTEL_CPU_DESC(INTEL_HASWELL, 3, 0x0000001f),
- INTEL_CPU_DESC(INTEL_HASWELL_L, 1, 0x0000001e),
- INTEL_CPU_DESC(INTEL_HASWELL_G, 1, 0x00000015),
- INTEL_CPU_DESC(INTEL_HASWELL_X, 2, 0x00000037),
- INTEL_CPU_DESC(INTEL_HASWELL_X, 4, 0x0000000a),
- INTEL_CPU_DESC(INTEL_BROADWELL, 4, 0x00000023),
- INTEL_CPU_DESC(INTEL_BROADWELL_G, 1, 0x00000014),
- INTEL_CPU_DESC(INTEL_BROADWELL_D, 2, 0x00000010),
- INTEL_CPU_DESC(INTEL_BROADWELL_D, 3, 0x07000009),
- INTEL_CPU_DESC(INTEL_BROADWELL_D, 4, 0x0f000009),
- INTEL_CPU_DESC(INTEL_BROADWELL_D, 5, 0x0e000002),
- INTEL_CPU_DESC(INTEL_BROADWELL_X, 1, 0x0b000014),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 3, 0x00000021),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 4, 0x00000000),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 5, 0x00000000),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 6, 0x00000000),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 7, 0x00000000),
- INTEL_CPU_DESC(INTEL_SKYLAKE_X, 11, 0x00000000),
- INTEL_CPU_DESC(INTEL_SKYLAKE_L, 3, 0x0000007c),
- INTEL_CPU_DESC(INTEL_SKYLAKE, 3, 0x0000007c),
- INTEL_CPU_DESC(INTEL_KABYLAKE, 9, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE_L, 9, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE_L, 10, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE_L, 11, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE_L, 12, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE, 10, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE, 11, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE, 12, 0x0000004e),
- INTEL_CPU_DESC(INTEL_KABYLAKE, 13, 0x0000004e),
+static const struct x86_cpu_id isolation_ucodes[] = {
+ X86_MATCH_VFM_STEPPING(INTEL_HASWELL, 3, 0x0000001f),
+ X86_MATCH_VFM_STEPPING(INTEL_HASWELL_L, 1, 0x0000001e),
+ X86_MATCH_VFM_STEPPING(INTEL_HASWELL_G, 1, 0x00000015),
+ X86_MATCH_VFM_STEPPING(INTEL_HASWELL_X, 2, 0x00000037),
+ X86_MATCH_VFM_STEPPING(INTEL_HASWELL_X, 4, 0x0000000a),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL, 4, 0x00000023),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_G, 1, 0x00000014),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_D, 2, 0x00000010),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_D, 3, 0x07000009),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_D, 4, 0x0f000009),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_D, 5, 0x0e000002),
+ X86_MATCH_VFM_STEPPING(INTEL_BROADWELL_X, 1, 0x0b000014),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 3, 0x00000021),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 4, 0x00000000),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 5, 0x00000000),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 6, 0x00000000),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 7, 0x00000000),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_X, 11, 0x00000000),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE_L, 3, 0x0000007c),
+ X86_MATCH_VFM_STEPPING(INTEL_SKYLAKE, 3, 0x0000007c),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE, 9, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE_L, 9, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE_L, 10, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE_L, 11, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE_L, 12, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE, 10, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE, 11, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE, 12, 0x0000004e),
+ X86_MATCH_VFM_STEPPING(INTEL_KABYLAKE, 13, 0x0000004e),
{}
};
static void intel_check_pebs_isolation(void)
{
- x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
+ x86_pmu.pebs_no_isolation = !x86_match_min_microcode_rev(isolation_ucodes);
}
static __init void intel_pebs_isolation_quirk(void)
@@ -5385,16 +5385,16 @@ static __init void intel_pebs_isolation_
intel_check_pebs_isolation();
}
-static const struct x86_cpu_desc pebs_ucodes[] = {
- INTEL_CPU_DESC(INTEL_SANDYBRIDGE, 7, 0x00000028),
- INTEL_CPU_DESC(INTEL_SANDYBRIDGE_X, 6, 0x00000618),
- INTEL_CPU_DESC(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
+static const struct x86_cpu_id pebs_ucodes[] = {
+ X86_MATCH_VFM_STEPPING(INTEL_SANDYBRIDGE, 7, 0x00000028),
+ X86_MATCH_VFM_STEPPING(INTEL_SANDYBRIDGE_X, 6, 0x00000618),
+ X86_MATCH_VFM_STEPPING(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
{}
};
static bool intel_snb_pebs_broken(void)
{
- return !x86_cpu_has_min_microcode_rev(pebs_ucodes);
+ return !x86_match_min_microcode_rev(pebs_ucodes);
}
static void intel_snb_check_microcode(void)
diff -puN arch/x86/include/asm/cpu_device_id.h~zap-x86_cpu_desc arch/x86/include/asm/cpu_device_id.h
--- a/arch/x86/include/asm/cpu_device_id.h~zap-x86_cpu_desc 2024-11-20 12:22:05.468400705 -0800
+++ b/arch/x86/include/asm/cpu_device_id.h 2024-11-20 12:22:05.472400858 -0800
@@ -226,6 +226,24 @@
steppings, X86_FEATURE_ANY, data)
/**
+ * X86_MATCH_VFM_STEPPING - Match encoded vendor/family/model/stepping
+ * @vfm: Encoded 8-bits each for vendor, family, model
+ * @stepping: A single integer stepping
+ * @data: Driver specific data or NULL. The internal storage
+ * format is unsigned long. The supplied value, pointer
+ * etc. is cast to unsigned long internally.
+ *
+ * feature is set to wildcard
+ */
+#define X86_MATCH_VFM_STEPPING(vfm, stepping, data) \
+ X86_MATCH_VENDORID_FAM_MODEL_STEPPINGS_FEATURE( \
+ VFM_VENDOR(vfm), \
+ VFM_FAMILY(vfm), \
+ VFM_MODEL(vfm), \
+ X86_STEPPINGS(stepping, stepping), \
+ X86_FEATURE_ANY, data)
+
+/**
* X86_MATCH_VFM_FEATURE - Match encoded vendor/family/model/feature
* @vfm: Encoded 8-bits each for vendor, family, model
* @feature: A X86_FEATURE bit
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 3/4] x86/cpu: Move AMD erratum 1386 table over to 'x86_cpu_id'
2024-11-20 20:24 [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 1/4] x86/cpu: Introduce new microcode matching helper Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id' Dave Hansen
@ 2024-11-20 20:24 ` Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 4/4] x86/cpu: Remove 'x86_cpu_desc' infrastructure Dave Hansen
3 siblings, 0 replies; 6+ messages in thread
From: Dave Hansen @ 2024-11-20 20:24 UTC (permalink / raw)
To: linux-kernel; +Cc: x86, tglx, bp, kan.liang, Dave Hansen
From: Dave Hansen <dave.hansen@linux.intel.com>
The AMD erratum 1386 detection code uses and old style 'x86_cpu_desc'
table. Replace it with 'x86_cpu_id' so the old style can be removed.
I did not create a new helper macro here. The new table is certainly
more noisy than the old and it can be improved on. But I was hesitant
to create a new macro just for a single site that is only two ugly
lines in the end.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/kernel/cpu/amd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff -puN arch/x86/kernel/cpu/amd.c~amd-x86_cpu_id arch/x86/kernel/cpu/amd.c
--- a/arch/x86/kernel/cpu/amd.c~amd-x86_cpu_id 2024-11-20 12:22:05.988420580 -0800
+++ b/arch/x86/kernel/cpu/amd.c 2024-11-20 12:22:05.992420733 -0800
@@ -795,9 +795,9 @@ static void init_amd_bd(struct cpuinfo_x
clear_rdrand_cpuid_bit(c);
}
-static const struct x86_cpu_desc erratum_1386_microcode[] = {
- AMD_CPU_DESC(0x17, 0x1, 0x2, 0x0800126e),
- AMD_CPU_DESC(0x17, 0x31, 0x0, 0x08301052),
+static const struct x86_cpu_id erratum_1386_microcode[] = {
+ X86_MATCH_VFM_STEPPING(VFM_MAKE(X86_VENDOR_AMD, 0x17, 0x01), 0x2, 0x0800126e),
+ X86_MATCH_VFM_STEPPING(VFM_MAKE(X86_VENDOR_AMD, 0x17, 0x31), 0x0, 0x08301052),
};
static void fix_erratum_1386(struct cpuinfo_x86 *c)
@@ -813,7 +813,7 @@ static void fix_erratum_1386(struct cpui
* Clear the feature flag only on microcode revisions which
* don't have the fix.
*/
- if (x86_cpu_has_min_microcode_rev(erratum_1386_microcode))
+ if (x86_match_min_microcode_rev(erratum_1386_microcode))
return;
clear_cpu_cap(c, X86_FEATURE_XSAVES);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 4/4] x86/cpu: Remove 'x86_cpu_desc' infrastructure
2024-11-20 20:24 [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure Dave Hansen
` (2 preceding siblings ...)
2024-11-20 20:24 ` [RFC][PATCH 3/4] x86/cpu: Move AMD erratum 1386 table over to 'x86_cpu_id' Dave Hansen
@ 2024-11-20 20:24 ` Dave Hansen
3 siblings, 0 replies; 6+ messages in thread
From: Dave Hansen @ 2024-11-20 20:24 UTC (permalink / raw)
To: linux-kernel; +Cc: x86, tglx, bp, kan.liang, Dave Hansen
From: Dave Hansen <dave.hansen@linux.intel.com>
All the users of 'x86_cpu_desc' are gone. Zap it from the tree.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/include/asm/cpu_device_id.h | 36 ---------------------------------
b/arch/x86/kernel/cpu/match.c | 31 ----------------------------
2 files changed, 67 deletions(-)
diff -puN arch/x86/include/asm/cpu_device_id.h~zap-x86_cpu_desc-3 arch/x86/include/asm/cpu_device_id.h
--- a/arch/x86/include/asm/cpu_device_id.h~zap-x86_cpu_desc-3 2024-11-20 12:22:06.484439538 -0800
+++ b/arch/x86/include/asm/cpu_device_id.h 2024-11-20 12:22:06.488439691 -0800
@@ -260,42 +260,6 @@
VFM_MODEL(vfm), \
X86_STEPPING_ANY, feature, data)
-/*
- * Match specific microcode revisions.
- *
- * vendor/family/model/stepping must be all set.
- *
- * Only checks against the boot CPU. When mixed-stepping configs are
- * valid for a CPU model, add a quirk for every valid stepping and
- * do the fine-tuning in the quirk handler.
- */
-
-struct x86_cpu_desc {
- u8 x86_family;
- u8 x86_vendor;
- u8 x86_model;
- u8 x86_stepping;
- u32 x86_microcode_rev;
-};
-
-#define INTEL_CPU_DESC(vfm, stepping, revision) { \
- .x86_family = VFM_FAMILY(vfm), \
- .x86_vendor = VFM_VENDOR(vfm), \
- .x86_model = VFM_MODEL(vfm), \
- .x86_stepping = (stepping), \
- .x86_microcode_rev = (revision), \
-}
-
-#define AMD_CPU_DESC(fam, model, stepping, revision) { \
- .x86_family = (fam), \
- .x86_vendor = X86_VENDOR_AMD, \
- .x86_model = (model), \
- .x86_stepping = (stepping), \
- .x86_microcode_rev = (revision), \
-}
-
-extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
-extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
extern bool x86_match_min_microcode_rev(const struct x86_cpu_id *table);
#endif /* _ASM_X86_CPU_DEVICE_ID */
diff -puN arch/x86/kernel/cpu/match.c~zap-x86_cpu_desc-3 arch/x86/kernel/cpu/match.c
--- a/arch/x86/kernel/cpu/match.c~zap-x86_cpu_desc-3 2024-11-20 12:22:06.484439538 -0800
+++ b/arch/x86/kernel/cpu/match.c 2024-11-20 12:22:06.488439691 -0800
@@ -56,37 +56,6 @@ const struct x86_cpu_id *x86_match_cpu(c
}
EXPORT_SYMBOL(x86_match_cpu);
-static const struct x86_cpu_desc *
-x86_match_cpu_with_stepping(const struct x86_cpu_desc *match)
-{
- struct cpuinfo_x86 *c = &boot_cpu_data;
- const struct x86_cpu_desc *m;
-
- for (m = match; m->x86_family | m->x86_model; m++) {
- if (c->x86_vendor != m->x86_vendor)
- continue;
- if (c->x86 != m->x86_family)
- continue;
- if (c->x86_model != m->x86_model)
- continue;
- if (c->x86_stepping != m->x86_stepping)
- continue;
- return m;
- }
- return NULL;
-}
-
-bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table)
-{
- const struct x86_cpu_desc *res = x86_match_cpu_with_stepping(table);
-
- if (!res || res->x86_microcode_rev > boot_cpu_data.microcode)
- return false;
-
- return true;
-}
-EXPORT_SYMBOL_GPL(x86_cpu_has_min_microcode_rev);
-
bool x86_match_min_microcode_rev(const struct x86_cpu_id *table)
{
const struct x86_cpu_id *res = x86_match_cpu(table);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id'
2024-11-20 20:24 ` [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id' Dave Hansen
@ 2024-11-21 9:04 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2024-11-21 9:04 UTC (permalink / raw)
To: Dave Hansen; +Cc: linux-kernel, x86, tglx, bp, kan.liang
* Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> The 'x86_cpu_desc' and 'x86_cpu_id' structures are very similar.
> Reduce duplicate infrastructure by moving the few users of
> 'x86_cpu_id' to the much more common variant.
>
> The existing X86_MATCH_VFM_STEPPINGS() helper matches ranges of
> steppings. Introduce a new helper to match a single stepping to make
> the macro use a bit less verbose.
>
> I'm a _bit_ nervous about this because
>
> X86_MATCH_VFM_STEPPING(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
> and
> X86_MATCH_VFM_STEPPINGS(INTEL_SANDYBRIDGE_X, 7, 0x0000070c),
>
> look very similar but the second one is buggy. Any suggestions for
> making this more foolproof would be appreciated.
> +static const struct x86_cpu_id isolation_ucodes[] = {
> + X86_MATCH_VFM_STEPPING(INTEL_HASWELL, 3, 0x0000001f),
> + X86_MATCH_VFM_STEPPING(INTEL_HASWELL_L, 1, 0x0000001e),
> + X86_MATCH_VFM_STEPPING(INTEL_HASWELL_G, 1, 0x00000015),
> + X86_MATCH_VFM_STEPPING(INTEL_HASWELL_X, 2, 0x00000037),
> /**
> + * X86_MATCH_VFM_STEPPING - Match encoded vendor/family/model/stepping
> + * @vfm: Encoded 8-bits each for vendor, family, model
> + * @stepping: A single integer stepping
> + * @data: Driver specific data or NULL. The internal storage
> + * format is unsigned long. The supplied value, pointer
> + * etc. is cast to unsigned long internally.
> + *
> + * feature is set to wildcard
> + */
> +#define X86_MATCH_VFM_STEPPING(vfm, stepping, data) \
> + X86_MATCH_VENDORID_FAM_MODEL_STEPPINGS_FEATURE( \
> + VFM_VENDOR(vfm), \
> + VFM_FAMILY(vfm), \
> + VFM_MODEL(vfm), \
> + X86_STEPPINGS(stepping, stepping), \
> + X86_FEATURE_ANY, data)
Yeah, this mixed with X86_MATCH_VFM_STEPPINGS() indeed looks fragile:
/**
* X86_MATCH_VFM_STEPPINGS - Match encoded vendor/family/model/stepping
* @vfm: Encoded 8-bits each for vendor, family, model
* @steppings: Bitmask of steppings to match
* @data: Driver specific data or NULL. The internal storage
* format is unsigned long. The supplied value, pointer
* etc. is cast to unsigned long internally.
*
* feature is set to wildcard
*/
#define X86_MATCH_VFM_STEPPINGS(vfm, steppings, data) \
X86_MATCH_VENDORID_FAM_MODEL_STEPPINGS_FEATURE( \
VFM_VENDOR(vfm), \
VFM_FAMILY(vfm), \
VFM_MODEL(vfm), \
steppings, X86_FEATURE_ANY, data)
I'd solve this by unifying on a single min-max range-interface:
X86_MATCH_VFM_STEPPINGS(vfm, stepping_min, stepping_max, data)
which simply passes GENMASK(stepping_min, stepping_max) to .steppings
field.
Note how almost all existing uses of X86_MATCH_VFM_STEPPINGS() already
open-codes this:
arch/x86/include/asm/cpu_device_id.h: * X86_MATCH_VFM_STEPPINGS - Match encoded vendor/family/model/stepping
arch/x86/include/asm/cpu_device_id.h:#define X86_MATCH_VFM_STEPPINGS(vfm, steppings, data) \
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_HASWELL_X, X86_STEPPINGS(0x2, 0x2), 0x3a), /* EP */
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_HASWELL_X, X86_STEPPINGS(0x4, 0x4), 0x0f), /* EX */
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_BROADWELL_D, X86_STEPPINGS(0x2, 0x2), 0x00000011),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_BROADWELL_D, X86_STEPPINGS(0x3, 0x3), 0x0700000e),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_BROADWELL_D, X86_STEPPINGS(0x4, 0x4), 0x0f00000c),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_BROADWELL_D, X86_STEPPINGS(0x5, 0x5), 0x0e000003),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_SKYLAKE_X, X86_STEPPINGS(0x3, 0x3), 0x01000136),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_SKYLAKE_X, X86_STEPPINGS(0x4, 0x4), 0x02000014),
arch/x86/kernel/apic/apic.c: X86_MATCH_VFM_STEPPINGS(INTEL_SKYLAKE_X, X86_STEPPINGS(0x5, 0xf), 0),
arch/x86/kernel/cpu/common.c: X86_MATCH_VFM_STEPPINGS(vfm, steppings, issues)
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ATOM_TREMONT_D, X86_STEPPINGS(0x0, 0x3), &i10nm_cfg0),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ATOM_TREMONT_D, X86_STEPPINGS(0x4, 0xf), &i10nm_cfg1),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ICELAKE_X, X86_STEPPINGS(0x0, 0x3), &i10nm_cfg0),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ICELAKE_X, X86_STEPPINGS(0x4, 0xf), &i10nm_cfg1),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ICELAKE_D, X86_STEPPINGS(0x0, 0xf), &i10nm_cfg1),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_SAPPHIRERAPIDS_X, X86_STEPPINGS(0x0, 0xf), &spr_cfg),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_EMERALDRAPIDS_X, X86_STEPPINGS(0x0, 0xf), &spr_cfg),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_GRANITERAPIDS_X, X86_STEPPINGS(0x0, 0xf), &gnr_cfg),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ATOM_CRESTMONT_X, X86_STEPPINGS(0x0, 0xf), &gnr_cfg),
drivers/edac/i10nm_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_ATOM_CRESTMONT, X86_STEPPINGS(0x0, 0xf), &gnr_cfg),
drivers/edac/skx_base.c: X86_MATCH_VFM_STEPPINGS(INTEL_SKYLAKE_X, X86_STEPPINGS(0x0, 0xf), &skx_cfg),
So I'd start by a patch that changes X86_MATCH_VFM_STEPPINGS() and
converts these usecases, and then your patch can just use the expanded
parameters of X86_MATCH_VFM_STEPPINGS() with the same min-max value:
X86_MATCH_VFM_STEPPINGS(INTEL_HASWELL, 3, 3, 0x0000001f),
That tiny bit of verbosity is far better than the fragility of the
proposed interface, IMHO.
Also, sometimes single-stepping ranges will expand as quirks/features
expand in scope, so this is the more natural interface anyway.
... or you can define a trivial single-stepping wrapper:
X86_MATCH_VFM_STEPPING(vfm, stepping, data) \
X86_MATCH_VFM_STEPPINGS(vfm, stepping, stepping, data)
Note how this is not nearly as fragile, as typoing the interface would
result in a build failure, not a silently broken kernel.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-21 9:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-20 20:24 [RFC][PATCH 0/4] x86/cpu: Remove duplicate microcode version matching infrastructure Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 1/4] x86/cpu: Introduce new microcode matching helper Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id' Dave Hansen
2024-11-21 9:04 ` Ingo Molnar
2024-11-20 20:24 ` [RFC][PATCH 3/4] x86/cpu: Move AMD erratum 1386 table over to 'x86_cpu_id' Dave Hansen
2024-11-20 20:24 ` [RFC][PATCH 4/4] x86/cpu: Remove 'x86_cpu_desc' infrastructure Dave Hansen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox