| Message ID | 4fe76484-fa35-807f-2323-276087469bc2@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] x86/CPUID: bump max leaf values for guest exposure | expand |
On 17/08/2023 7:22 am, Jan Beulich wrote: > --- a/xen/lib/x86/cpuid.c > +++ b/xen/lib/x86/cpuid.c > @@ -104,6 +104,22 @@ void x86_cpu_featureset_to_policy( > p->feat._7d1 = fs[FEATURESET_7d1]; > p->arch_caps.lo = fs[FEATURESET_m10Al]; > p->arch_caps.hi = fs[FEATURESET_m10Ah]; > + > + /* > + * We may force-enable certain features, which then needs reflecting in > + * respective max leaf / subleaf values. > + * > + * ARCH_CAPS lives in 7d0. > + */ > + if ( p->feat._7d0 && p->basic.max_leaf < 7 ) > + p->basic.max_leaf = 7; > + > + /* > + * AMD's speculation related features (e.g. LFENCE_DISPATCH) live in > + * leaf e21a. > + */ > + if ( p->extd.e21a && p->extd.max_leaf < 0x80000021 ) > + p->extd.max_leaf = 0x80000021; This logic cannot live here - this function is a simple deserialisation of an array. Such logic belongs in create compatible policy, the patch for which has been pending even longer. The toolstack does need to take when extending like this, and it is not safe to do it automatically like this. ~Andrew
On 18.10.2023 12:59, Andrew Cooper wrote: > On 17/08/2023 7:22 am, Jan Beulich wrote: >> --- a/xen/lib/x86/cpuid.c >> +++ b/xen/lib/x86/cpuid.c >> @@ -104,6 +104,22 @@ void x86_cpu_featureset_to_policy( >> p->feat._7d1 = fs[FEATURESET_7d1]; >> p->arch_caps.lo = fs[FEATURESET_m10Al]; >> p->arch_caps.hi = fs[FEATURESET_m10Ah]; >> + >> + /* >> + * We may force-enable certain features, which then needs reflecting in >> + * respective max leaf / subleaf values. >> + * >> + * ARCH_CAPS lives in 7d0. >> + */ >> + if ( p->feat._7d0 && p->basic.max_leaf < 7 ) >> + p->basic.max_leaf = 7; >> + >> + /* >> + * AMD's speculation related features (e.g. LFENCE_DISPATCH) live in >> + * leaf e21a. >> + */ >> + if ( p->extd.e21a && p->extd.max_leaf < 0x80000021 ) >> + p->extd.max_leaf = 0x80000021; > > This logic cannot live here - this function is a simple deserialisation > of an array. > > Such logic belongs in create compatible policy, the patch for which has > been pending even longer. What's that patch's status? Also the extended leaf bumping previously lived in calculate_host_policy() alone, which isn't anywhere near "create compatible policy" either. > The toolstack does need to take when extending like this, and it is not > safe to do it automatically like this. Feels like there's a word missing in the sentence, so it leaves some guessing room. Question is - if it can't be done automatically (see also the RFC: remark in the patch), how do we achieve a consistent resulting policy? Plus I'm not sure we can distinguish the tool stack requesting certain max leaf values merely because of finding them this way, from a lower value being the result of a guest config setting. Jan
--- a/xen/arch/x86/cpu-policy.c +++ b/xen/arch/x86/cpu-policy.c @@ -360,7 +360,6 @@ void calculate_raw_cpu_policy(void) static void __init calculate_host_policy(void) { struct cpu_policy *p = &host_cpu_policy; - unsigned int max_extd_leaf; *p = raw_cpu_policy; @@ -368,20 +367,8 @@ static void __init calculate_host_policy min_t(uint32_t, p->basic.max_leaf, ARRAY_SIZE(p->basic.raw) - 1); p->feat.max_subleaf = min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1); - - max_extd_leaf = p->extd.max_leaf; - - /* - * For AMD/Hygon hardware before Zen3, we unilaterally modify LFENCE to be - * dispatch serialising for Spectre mitigations. Extend max_extd_leaf - * beyond what hardware supports, to include the feature leaf containing - * this information. - */ - if ( cpu_has_lfence_dispatch ) - max_extd_leaf = max(max_extd_leaf, 0x80000021); - - p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff, - ARRAY_SIZE(p->extd.raw) - 1); + p->extd.max_leaf = 0x80000000 | min(p->extd.max_leaf & 0xffffUL, + ARRAY_SIZE(p->extd.raw) - 1); x86_cpu_featureset_to_policy(boot_cpu_data.x86_capability, p); recalculate_xstate(p); --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -104,6 +104,22 @@ void x86_cpu_featureset_to_policy( p->feat._7d1 = fs[FEATURESET_7d1]; p->arch_caps.lo = fs[FEATURESET_m10Al]; p->arch_caps.hi = fs[FEATURESET_m10Ah]; + + /* + * We may force-enable certain features, which then needs reflecting in + * respective max leaf / subleaf values. + * + * ARCH_CAPS lives in 7d0. + */ + if ( p->feat._7d0 && p->basic.max_leaf < 7 ) + p->basic.max_leaf = 7; + + /* + * AMD's speculation related features (e.g. LFENCE_DISPATCH) live in + * leaf e21a. + */ + if ( p->extd.e21a && p->extd.max_leaf < 0x80000021 ) + p->extd.max_leaf = 0x80000021; } void x86_cpu_policy_recalc_synth(struct cpu_policy *p)
Generalize what was done for LFENCE_DISPATCH: To make certain features that we artificially enable recognizable by guests, respective maximum leaf values may need to be bumped as well. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- RFC: The call to the function from recalculate_cpuid_policy() is somewhat problematic: It's not clear whether a tool stack request with a low maximum leaf value should be honored despite our desire to expose certain features. This omits checking basic/extended leaf 1 features, some of which we also force. I think requests to run guests with max basic/extended leaf set to zero are sufficiently insane. Among the large set of changes I don't think I can identify the commit to validly name in a possible Fixes: tag here. The cpu-policy.c change would imo look quite a bit better on top of "x86/CPUID: move bounding of max_{,sub}leaf fields to library code", which has been pending for a long time.