| Message ID | 20230915150038.602577-9-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/spec-ctrl: AMD DIV fix, and VERW prerequisite bugfixes | expand |
On 15.09.2023 17:00, Andrew Cooper wrote: > --- a/xen/arch/x86/include/asm/amd.h > +++ b/xen/arch/x86/include/asm/amd.h > @@ -140,6 +140,17 @@ > AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ > AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) > > +/* > + * The Zen1 and Zen2 microarchitectures are implemented by AMD (Fam17h) and > + * Hygon (Fam18h) but without simple model number rules. Instead, use STIBP > + * as a heuristic that distinguishes the two. > + * > + * The caller is required to perform the appropriate vendor/family checks > + * first. > + */ > +#define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) > +#define is_zen2_uarch() boot_cpu_has(X86_FEATURE_AMD_STIBP) > + > struct cpuinfo_x86; > int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); With one simply the opposite of the other, and with the requirement of a family check up front, do we really need both? Personally I'd prefer if we had just the latter. Yet in any event Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 18/09/2023 12:07 pm, Jan Beulich wrote: > On 15.09.2023 17:00, Andrew Cooper wrote: >> --- a/xen/arch/x86/include/asm/amd.h >> +++ b/xen/arch/x86/include/asm/amd.h >> @@ -140,6 +140,17 @@ >> AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ >> AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) >> >> +/* >> + * The Zen1 and Zen2 microarchitectures are implemented by AMD (Fam17h) and >> + * Hygon (Fam18h) but without simple model number rules. Instead, use STIBP >> + * as a heuristic that distinguishes the two. >> + * >> + * The caller is required to perform the appropriate vendor/family checks >> + * first. >> + */ >> +#define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) >> +#define is_zen2_uarch() boot_cpu_has(X86_FEATURE_AMD_STIBP) >> + >> struct cpuinfo_x86; >> int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); > With one simply the opposite of the other, and with the requirement of a > family check up front, do we really need both? Personally I'd prefer if > we had just the latter. Yet in any event > Reviewed-by: Jan Beulich <jbeulich@suse.com> We specifically do want both, because they're use is not symmetric at callsites. In particular, having only one would make the following patch illogical to read. ~Andrew
On 18.09.2023 16:02, Andrew Cooper wrote: > On 18/09/2023 12:07 pm, Jan Beulich wrote: >> On 15.09.2023 17:00, Andrew Cooper wrote: >>> --- a/xen/arch/x86/include/asm/amd.h >>> +++ b/xen/arch/x86/include/asm/amd.h >>> @@ -140,6 +140,17 @@ >>> AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ >>> AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) >>> >>> +/* >>> + * The Zen1 and Zen2 microarchitectures are implemented by AMD (Fam17h) and >>> + * Hygon (Fam18h) but without simple model number rules. Instead, use STIBP >>> + * as a heuristic that distinguishes the two. >>> + * >>> + * The caller is required to perform the appropriate vendor/family checks >>> + * first. >>> + */ >>> +#define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) >>> +#define is_zen2_uarch() boot_cpu_has(X86_FEATURE_AMD_STIBP) >>> + >>> struct cpuinfo_x86; >>> int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); >> With one simply the opposite of the other, and with the requirement of a >> family check up front, do we really need both? Personally I'd prefer if >> we had just the latter. Yet in any event >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > We specifically do want both, because they're use is not symmetric at > callsites. > > In particular, having only one would make the following patch illogical > to read. I don't think it would, but that's perhaps one more of the many areas where we take different perspectives. Jan
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index bbf7887f2e1d..4f27187f92ec 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -882,15 +882,13 @@ void amd_set_legacy_ssbd(bool enable) * non-branch instructions to be ignored. It is to be set unilaterally in * newer microcode. * - * This chickenbit is something unrelated on Zen1, and Zen1 vs Zen2 isn't a - * simple model number comparison, so use STIBP as a heuristic to separate the - * two uarches in Fam17h(AMD)/18h(Hygon). + * This chickenbit is something unrelated on Zen1. */ void amd_init_spectral_chicken(void) { uint64_t val, chickenbit = 1 << 1; - if (cpu_has_hypervisor || !boot_cpu_has(X86_FEATURE_AMD_STIBP)) + if (cpu_has_hypervisor || !is_zen2_uarch()) return; if (rdmsr_safe(MSR_AMD64_DE_CFG2, val) == 0 && !(val & chickenbit)) @@ -939,11 +937,8 @@ void amd_check_zenbleed(void) * With the Fam17h check above, most parts getting here are * Zen1. They're not affected. Assume Zen2 ones making it * here are affected regardless of microcode version. - * - * Zen1 vs Zen2 isn't a simple model number comparison, so use - * STIBP as a heuristic to distinguish. */ - if (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) + if (is_zen1_uarch()) return; good_rev = ~0U; break; @@ -1298,12 +1293,7 @@ static int __init cf_check zen2_c6_errata_check(void) */ s_time_t delta; - /* - * Zen1 vs Zen2 isn't a simple model number comparison, so use STIBP as - * a heuristic to separate the two uarches in Fam17h. - */ - if (cpu_has_hypervisor || boot_cpu_data.x86 != 0x17 || - !boot_cpu_has(X86_FEATURE_AMD_STIBP)) + if (cpu_has_hypervisor || boot_cpu_data.x86 != 0x17 || !is_zen2_uarch()) return 0; /* diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h index 09ee52dc1c09..d862cb7972a1 100644 --- a/xen/arch/x86/include/asm/amd.h +++ b/xen/arch/x86/include/asm/amd.h @@ -140,6 +140,17 @@ AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) +/* + * The Zen1 and Zen2 microarchitectures are implemented by AMD (Fam17h) and + * Hygon (Fam18h) but without simple model number rules. Instead, use STIBP + * as a heuristic that distinguishes the two. + * + * The caller is required to perform the appropriate vendor/family checks + * first. + */ +#define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) +#define is_zen2_uarch() boot_cpu_has(X86_FEATURE_AMD_STIBP) + struct cpuinfo_x86; int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...);
We already have 3 cases using STIBP as a Zen1/2 heuristic, and are about to introduce a 4th. Wrap the heuristic into a pair of predictes rather than opencoding it, and the explaination of the heursitic, at each usage site. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2: * New --- xen/arch/x86/cpu/amd.c | 18 ++++-------------- xen/arch/x86/include/asm/amd.h | 11 +++++++++++ 2 files changed, 15 insertions(+), 14 deletions(-)