Re: [PATCH v6 1/7] x86: suppress ERMS for internal use when MISC_ENABLE.FAST_STRING is clear

[Jan Beulich <jbeulich@xxxxxxxx>]

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