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
