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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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);

~Andrew