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

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

On Fri, Mar 01, 2024 at 03:01:36PM +0000, Andrew Cooper wrote:
> On 01/03/2024 1:28 pm, Roger Pau Monné wrote:
> > 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.
> 
> This better?
> 
> +        /*
> +         * 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.
> +         */
> +        __clear_bit(X86_FEATURE_MD_CLEAR);
> +        if ( cpu_has_md_clear )
> +            __set_bit(X86_FEATURE_MD_CLEAR);
> +
> +        __clear_bit(X86_FEATURE_FB_CLEAR);
> +        if ( cpu_has_fb_clear )
> +            __set_bit(X86_FEATURE_FB_CLEAR);

Sure, with that please add my:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.