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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.