Re: [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with 'x86_cpu_id'

[Ingo Molnar <mingo@kernel.org>]

* 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