Re: [PATCH v2 3/3] x86/cpu-policy: Derive RSBA/RRSBA for guest policies

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.06.2023 16:48, Andrew Cooper wrote:
> The RSBA bit, "RSB Alternative", means that the RSB may use alternative
> predictors when empty.  From a practical point of view, this mean "Retpoline
> not safe".
> 
> Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
> statement that IBRS is implemented in hardware (as opposed to the form
> retrofitted to existing CPUs in microcode).
> 
> The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
> property that predictions are tagged with the mode in which they were learnt.
> Therefore, it means "when eIBRS is active, the RSB may fall back to
> alternative predictors but restricted to the current prediction mode".  As
> such, it's stronger statement than RSBA, but still means "Retpoline not safe".
> 
> CPUs are not expected to enumerate both RSBA and RRSBA.
> 
> Add feature dependencies for EIBRS and RRSBA.  While technically they're not
> linked, absolutely nothing good can of letting the guest see RRSBA without

Nit: missing "come"?

> EIBRS.  Nor can anything good come of a guest seeing EIBRS without IBRSB.
> Furthermore, we use this dependency to simplify the max derivation logic.
>[...]
> @@ -532,6 +541,21 @@ static void __init calculate_pv_def_policy(void)
>      guest_common_default_feature_adjustments(fs);
>  
>      sanitise_featureset(fs);
> +
> +    /*
> +     * If the host suffers from RSBA of any form, and the guest can see
> +     * MSR_ARCH_CAPS, reflect the appropriate RSBA/RRSBA property to the 
> guest
> +     * depending on the visibility of eIBRS.
> +     */
> +    if ( test_bit(X86_FEATURE_ARCH_CAPS, fs) &&
> +         (cpu_has_rsba || cpu_has_rrsba) )
> +    {
> +        bool eibrs = test_bit(X86_FEATURE_EIBRS, fs);
> +
> +        __set_bit(eibrs ? X86_FEATURE_RRSBA
> +                        : X86_FEATURE_RSBA, fs);
> +    }
> +
>      x86_cpu_featureset_to_policy(fs, p);
>      recalculate_xstate(p);
>  }
> @@ -664,6 +688,21 @@ static void __init calculate_hvm_def_policy(void)
>          __set_bit(X86_FEATURE_VIRT_SSBD, fs);
>  
>      sanitise_featureset(fs);
> +
> +    /*
> +     * If the host suffers from RSBA of any form, and the guest can see
> +     * MSR_ARCH_CAPS, reflect the appropriate RSBA/RRSBA property to the 
> guest
> +     * depending on the visibility of eIBRS.
> +     */
> +    if ( test_bit(X86_FEATURE_ARCH_CAPS, fs) &&
> +         (cpu_has_rsba || cpu_has_rrsba) )
> +    {
> +        bool eibrs = test_bit(X86_FEATURE_EIBRS, fs);
> +
> +        __set_bit(eibrs ? X86_FEATURE_RRSBA
> +                        : X86_FEATURE_RSBA, fs);
> +    }
> +
>      x86_cpu_featureset_to_policy(fs, p);
>      recalculate_xstate(p);
>  }
> @@ -786,6 +825,20 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      sanitise_featureset(fs);
>  
> +    /*
> +     * If the host suffers from RSBA of any form, and the guest can see
> +     * MSR_ARCH_CAPS, reflect the appropriate RSBA/RRSBA property to the 
> guest
> +     * depending on the visibility of eIBRS.
> +     */
> +    if ( test_bit(X86_FEATURE_ARCH_CAPS, fs) &&
> +         (cpu_has_rsba || cpu_has_rrsba) )
> +    {
> +        bool eibrs = test_bit(X86_FEATURE_EIBRS, fs);
> +
> +        __set_bit(eibrs ? X86_FEATURE_RRSBA
> +                        : X86_FEATURE_RSBA, fs);
> +    }

Now that we have the same code and comment even a 3rd time, surely this
wants to be put in a helper?

What about a tool stack request leading to us setting the 2nd of the two
bits here, while the other was already set? IOW wouldn't we better clear
the other bit explicitly? (Due to the EIBRS dependency or RRSBA I think
this can really only happen when the tool stack requests RSBA+EIBRS, as
the deep deps clearing doesn't know the concept of "negative"
dependencies.)

Similarly what about a tool stack (and ultimately admin) request setting
both RSBA and RRSBA? Wouldn't we better clear the respectively wrong bit
then? People may do this in their guest configs "just to be on the safe
side" (once expressing this in guest configs is possible, of course),
and due to the max policies having both bits set this also won't occur
"automatically".

Jan