| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zz73mGclo4np8tVt@gmail.com>
Date: Thu, 21 Nov 2024 10:04:24 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Dave Hansen <dave.hansen@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
bp@...en8.de, kan.liang@...ux.intel.com
Subject: Re: [RFC][PATCH 2/4] x86/cpu: Replace 'x86_cpu_desc' use with
'x86_cpu_id'
* Dave Hansen <dave.hansen@...ux.intel.com> wrote:
>
> From: Dave Hansen <dave.hansen@...ux.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
Powered by blists - more mailing lists