public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [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