Re: [PATCH] x86/cpu-policy: Allow for levelling of VERW side effects

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 29, 2024 at 06:13:54PM +0000, Andrew Cooper wrote:
> MD_CLEAR and FB_CLEAR need OR-ing across a migrate pool.  Allow this, by
> having them unconditinally set in max, with the host values reflected in
> default.  Annotate the bits as having special properies.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/cpu-policy.c                   | 24 +++++++++++++++++++++
>  xen/include/public/arch-x86/cpufeatureset.h |  4 ++--
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index f3ed2d3a3227..41123e6cf778 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -442,6 +442,16 @@ static void __init 
> guest_common_max_feature_adjustments(uint32_t *fs)
>          __set_bit(X86_FEATURE_RSBA, fs);
>          __set_bit(X86_FEATURE_RRSBA, fs);
>  
> +        /*
> +         * These bits indicate that the VERW instruction may have gained
> +         * scrubbing side effects.  With pooling, they mean "you might 
> migrate
> +         * somewhere where scrubbing is necessary", and may need exposing on
> +         * unaffected hardware.  This is fine, because the VERW instruction
> +         * has been around since the 286.
> +         */
> +        __set_bit(X86_FEATURE_MD_CLEAR, fs);
> +        __set_bit(X86_FEATURE_FB_CLEAR, fs);
> +
>          /*
>           * The Gather Data Sampling microcode mitigation (August 2023) has an
>           * adverse performance impact on the CLWB instruction on SKX/CLX/CPX.
> @@ -476,6 +486,20 @@ static void __init 
> guest_common_default_feature_adjustments(uint32_t *fs)
>               cpu_has_rdrand && !is_forced_cpu_cap(X86_FEATURE_RDRAND) )
>              __clear_bit(X86_FEATURE_RDRAND, fs);
>  
> +        /*
> +         * These bits indicate that the VERW instruction may have gained
> +         * scrubbing side effects.  The max policy has them set for migration
> +         * reasons, so reset the default policy back to the host values in
> +         * case we're unaffected.
> +         */
> +        fs[FEATURESET_7d0]   &= ~cpufeat_mask(X86_FEATURE_MD_CLEAR);
> +        fs[FEATURESET_m10Al] &= ~cpufeat_mask(X86_FEATURE_FB_CLEAR);
> +
> +        fs[FEATURESET_7d0]   |= 
> (boot_cpu_data.x86_capability[FEATURESET_7d0] &
> +                                 cpufeat_mask(X86_FEATURE_MD_CLEAR));
> +        fs[FEATURESET_m10Al] |= 
> (boot_cpu_data.x86_capability[FEATURESET_m10Al] &
> +                                 cpufeat_mask(X86_FEATURE_FB_CLEAR));

This seems quite convoluted, why not use:

__clear_bit(X86_FEATURE_MD_CLEAR, fs);
if ( boot_cpu_has(X86_FEATURE_MD_CLEAR) )
    __set_bit(X86_FEATURE_MD_CLEAR, fs);

And the same for FB_CLEAR. I think that's quite easier to read.

Thanks, Roger.