Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2022 09:44, Andrew Cooper wrote:
> With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests.
> 
> Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
> changes), and explicitly enable the CPUID bits for HVM guests.
> 
> 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>
> 
> Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
> than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
> it's also a bit misleading to say 'A' when the PV side won't engage at all
> yet.

I agree with using 'S' at this point for most of them. I'm unsure for
SSB_NO, though - there 'A' would seem more appropriate, and afaict it
would then also be visible to PV guests (since you validly don't make
it dependent upon IBRS or anything else). Aiui this could have been
done even ahead of this work of yours.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -433,6 +433,8 @@ static void __init 
> guest_common_feature_adjustments(uint32_t *fs)
>       */
>      if ( test_bit(X86_FEATURE_IBRSB, fs) )
>          __set_bit(X86_FEATURE_STIBP, fs);
> +    if ( test_bit(X86_FEATURE_IBRS, fs) )
> +        __set_bit(X86_FEATURE_AMD_STIBP, fs);
>  
>      /*
>       * On hardware which supports IBRS/IBPB, we can offer IBPB independently

Following this comment is a cross-vendor adjustment. Shouldn't there
be more of these now? Or has this been a one-off for some reason?

> @@ -456,11 +458,14 @@ static void __init calculate_pv_max_policy(void)
>          pv_featureset[i] &= pv_max_featuremask[i];
>  
>      /*
> -     * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of
> -     * administrator choice, hide the feature.
> +     * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
> +     * availability, or admin choice), hide the feature.
> +     */

Unintended replacement of "PV" by "HVM"?

>      if ( !boot_cpu_has(X86_FEATURE_SC_MSR_PV) )
> +    {
>          __clear_bit(X86_FEATURE_IBRSB, pv_featureset);
> +        __clear_bit(X86_FEATURE_IBRS, pv_featureset);
> +    }
>  
>      guest_common_feature_adjustments(pv_featureset);

What I had done in a local (and incomplete) patch is pass
X86_FEATURE_SC_MSR_{PV,HVM} into guest_common_feature_adjustments()
and do what you extend above (and then again for HVM) centrally
there. (My gen-cpuid.py adjustment was smaller, so there were even
more bits to clear, and hence it became yet more relevant to avoid
doing this in two places.)

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -290,6 +290,11 @@ def crunch_numbers(state):
>  
>          # In principle the TSXLDTRK insns could also be considered 
> independent.
>          RTM: [TSXLDTRK],
> +
> +        # AMD speculative controls
> +        IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
> +               IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
> +        AMD_STIBP: [STIBP_ALWAYS],
>      }

Could I talk you into moving this ahead of RTM, such that it sits
next to the related Intel stuff?

Jan