Before we start actually adjusting behavior when ERMS is available,
follow Linux commit 161ec53c702c ("x86, mem, intel: Initialize Enhanced
REP MOVSB/STOSB") and zap the CPUID-derived feature flag when the MSR
bit is clear. Don't extend the artificial clearing to guest view,
though: Guests can take their own decision in this regard, as they can
read (most of) MISC_ENABLE.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Would be nice if "cpuid=no-erms" propagated to guest view (for
"cpuid=" generally meaning to affect guests as well as Xen), but
since both disabling paths use setup_clear_cpu_cap() they're
indistinguishable in guest_common_feature_adjustments(). A separate
boolean could take care of this, but would look clumsy to me.
---
v5: Correct guest_common_max_feature_adjustments() addition.
v4: Also adjust guest_common_max_feature_adjustments().
v3: New.
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -366,8 +366,18 @@ static void cf_check early_init_intel(st
paddr_bits = 36;
if (c == &boot_cpu_data) {
+ uint64_t misc_enable;
+
check_memory_type_self_snoop_errata();
+ /*
+ * If fast string is not enabled in IA32_MISC_ENABLE for any reason,
+ * clear the enhanced fast string CPU capability.
+ */
+ rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
+ if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING))
+ setup_clear_cpu_cap(X86_FEATURE_ERMS);
+
intel_init_levelling();
}
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -487,6 +487,12 @@ static void __init guest_common_max_feat
*/
if ( test_bit(X86_FEATURE_RTM, fs) )
__set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs);
+
+ /*
+ * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when
+ * FAST_STRING is not set should not affect the view of migrating-in guests.
+ */
+ __set_bit(X86_FEATURE_ERMS, fs);
}
static void __init guest_common_default_feature_adjustments(uint32_t *fs)
@@ -567,6 +573,16 @@ static void __init guest_common_default_
__clear_bit(X86_FEATURE_RTM, fs);
__set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs);
}
+
+ /*
+ * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when
+ * FAST_STRING is not set should not propagate to guest view. Guests can
+ * judge on their own whether to ignore the CPUID bit when the MSR bit is
+ * clear. The bit being uniformly set in the max policies, we only need
+ * to clear it here (if hardware doesn't have it).
+ */
+ if ( !raw_cpu_policy.feat.erms )
+ __clear_bit(X86_FEATURE_ERMS, fs);
}
static void __init guest_common_feature_adjustments(uint32_t *fs)
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -493,6 +493,7 @@
#define MSR_IA32_THERM_INTERRUPT 0x0000019b
#define MSR_IA32_THERM_STATUS 0x0000019c
#define MSR_IA32_MISC_ENABLE 0x000001a0
+#define MSR_IA32_MISC_ENABLE_FAST_STRING (1<<0)
#define MSR_IA32_MISC_ENABLE_PERF_AVAIL (1<<7)
#define MSR_IA32_MISC_ENABLE_BTS_UNAVAIL (1<<11)
#define MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL (1<<12)On 16/06/2025 1:59 pm, Jan Beulich wrote: > --- a/xen/arch/x86/cpu-policy.c > +++ b/xen/arch/x86/cpu-policy.c > @@ -487,6 +487,12 @@ static void __init guest_common_max_feat > */ > if ( test_bit(X86_FEATURE_RTM, fs) ) > __set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs); > + > + /* > + * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when > + * FAST_STRING is not set should not affect the view of migrating-in guests. > + */ The logic is ok, but the justification wants to be different. "ERMS is a performance hint. A VM which previously saw ERMS will function correctly when migrated here, even if ERMS isn't available." What Xen chooses to do with the bit isn't relevant to why we unconditionally set it in the max featureset. > + __set_bit(X86_FEATURE_ERMS, fs); > } > > static void __init guest_common_default_feature_adjustments(uint32_t *fs) > @@ -567,6 +573,16 @@ static void __init guest_common_default_ > __clear_bit(X86_FEATURE_RTM, fs); > __set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs); > } > + > + /* > + * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when > + * FAST_STRING is not set should not propagate to guest view. Guests can > + * judge on their own whether to ignore the CPUID bit when the MSR bit is > + * clear. The bit being uniformly set in the max policies, we only need > + * to clear it here (if hardware doesn't have it). > + */ "ERMS is a performance hint, so is set unconditionally in the max policy. However, the guest should default to the host setting." > + if ( !raw_cpu_policy.feat.erms ) This wants to be the host policy, not the raw policy. That's why `cpuid=no-erms` isn't working in the way you'd expect. cpuid=no-$foo means "Xen should behave as if $foo wasn't reported by hardware", and that includes not giving it to guests by default. > + __clear_bit(X86_FEATURE_ERMS, fs); > } > It occurs to me that there are quite a few examples of clear/cond-set which could be converted to cond-clear like this I'll do a prep patch to make things consistent. It shouldn't conflict with this, but I'd prefer to keep the function logic consistent; it's hard enough to follow already. ~Andrew P.S. I don't have time right now, but this is yet another example that could be eliminated with an annotation meaning "set in max, host in default". I'll try to find some time to make this happen, because there's clearly a pattern now.
On 26.06.2025 17:31, Andrew Cooper wrote: > On 16/06/2025 1:59 pm, Jan Beulich wrote: >> --- a/xen/arch/x86/cpu-policy.c >> +++ b/xen/arch/x86/cpu-policy.c >> @@ -487,6 +487,12 @@ static void __init guest_common_max_feat >> */ >> if ( test_bit(X86_FEATURE_RTM, fs) ) >> __set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs); >> + >> + /* >> + * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when >> + * FAST_STRING is not set should not affect the view of migrating-in guests. >> + */ > > The logic is ok, but the justification wants to be different. > > "ERMS is a performance hint. A VM which previously saw ERMS will > function correctly when migrated here, even if ERMS isn't available." > > What Xen chooses to do with the bit isn't relevant to why we > unconditionally set it in the max featureset. It's different words for effectively the same thing, to me at least. I can certainly use your wording, ... >> @@ -567,6 +573,16 @@ static void __init guest_common_default_ >> __clear_bit(X86_FEATURE_RTM, fs); >> __set_bit(X86_FEATURE_RTM_ALWAYS_ABORT, fs); >> } >> + >> + /* >> + * We expose MISC_ENABLE to guests, so our internal clearing of ERMS when >> + * FAST_STRING is not set should not propagate to guest view. Guests can >> + * judge on their own whether to ignore the CPUID bit when the MSR bit is >> + * clear. The bit being uniformly set in the max policies, we only need >> + * to clear it here (if hardware doesn't have it). >> + */ > > "ERMS is a performance hint, so is set unconditionally in the max > policy. However, the guest should default to the host setting." ... also here. >> + if ( !raw_cpu_policy.feat.erms ) > > This wants to be the host policy, not the raw policy. That's why > `cpuid=no-erms` isn't working in the way you'd expect. > > cpuid=no-$foo means "Xen should behave as if $foo wasn't reported by > hardware", and that includes not giving it to guests by default. Hmm, interesting. That's definitely not the meaning I give this. To me it means merely Xen shouldn't use the feature (with an impact on guests only when the feature in hardware is required to surface it to guests). I don't think we have the precise meaning of this option written down anywhere? >> + __clear_bit(X86_FEATURE_ERMS, fs); >> } >> > > It occurs to me that there are quite a few examples of clear/cond-set > which could be converted to cond-clear like this > > I'll do a prep patch to make things consistent. It shouldn't conflict > with this, but I'd prefer to keep the function logic consistent; it's > hard enough to follow already. Right, I too noticed that there are others which could be swapped over. I actually had it the other way around in early versions of the series, and it was only in the course of some re-work where I noticed it might be a little tidier this way. But I first wanted to see whether perhaps I have some thinko there, so didn't want to convert anything pre-existing. Jan
On 26.06.2025 17:45, Jan Beulich wrote: > On 26.06.2025 17:31, Andrew Cooper wrote: >> On 16/06/2025 1:59 pm, Jan Beulich wrote: >>> + if ( !raw_cpu_policy.feat.erms ) >> >> This wants to be the host policy, not the raw policy. That's why >> `cpuid=no-erms` isn't working in the way you'd expect. >> >> cpuid=no-$foo means "Xen should behave as if $foo wasn't reported by >> hardware", and that includes not giving it to guests by default. > > Hmm, interesting. That's definitely not the meaning I give this. To me it > means merely Xen shouldn't use the feature (with an impact on guests only > when the feature in hardware is required to surface it to guests). I > don't think we have the precise meaning of this option written down > anywhere? Then again I notice what you ask for is in line with uses of cpu_has_* (where available) elsewhere (e.g. in your "x86/cpu-policy: Simplify logic in guest_common_default_feature_adjustments()"), with calculate_host_policy() simply copying boot_cpu_data.x86_capability into the host policy. So yes, I guess I'll need to adjust. Ftaod, this still leaves open what exact meaning "cpuid=no-..." ought to have. While kind of an example in the opposite direction, consider e.g. "cpuid=no-lm": This cannot sensibly have any effect on Xen itself. It could plausibly mean to permit only 32-bit guests. Jan
On 2025-06-16 08:59, Jan Beulich wrote:
> Before we start actually adjusting behavior when ERMS is available,
> follow Linux commit 161ec53c702c ("x86, mem, intel: Initialize Enhanced
> REP MOVSB/STOSB") and zap the CPUID-derived feature flag when the MSR
> bit is clear. Don't extend the artificial clearing to guest view,
> though: Guests can take their own decision in this regard, as they can
> read (most of) MISC_ENABLE.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
© 2016 - 2025 Red Hat, Inc.