Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.05.2023 13:06, 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".

Just for my own understanding: Whether retpoline is safe with RRSBA does
depend on the level of control a less privileged entity has over a more
privileged entity's alternative predictor state? If so, maybe add
"probably" to the quoted statement?

> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -423,8 +423,14 @@ static void __init 
> guest_common_max_feature_adjustments(uint32_t *fs)
>           * Retpoline not safe)", so these need to be visible to a guest in 
> all
>           * cases, even when it's only some other server in the pool which
>           * suffers the identified behaviour.
> +         *
> +         * We can always run any VM which has previously (or will
> +         * subsequently) run on hardware where Retpoline is not safe.  Note:
> +         * The dependency logic may hide RRSBA for other reasons.
>           */
>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
> +        __set_bit(X86_FEATURE_RSBA, fs);
> +        __set_bit(X86_FEATURE_RRSBA, fs);
>      }
>  }

Similar question to what I raised before: Can't this lead to both bits being
set, which according to descriptions earlier in the series and elsewhere
ought to not be possible?

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
>          # as dependent features simplifies Xen's logic, and prevents the 
> guest
>          # from seeing implausible configurations.
> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],

Is this really an architecturally established dependency? From an abstract
pov having just eIBRS ought to be enough of an indicator? And hence it would
be wrong to hide it just because IBRSB isn't also set. Plus aiui ...

> @@ -328,6 +328,9 @@ def crunch_numbers(state):
>  
>          # The ARCH_CAPS CPUID bit enumerates the availability of the whole 
> register.
>          ARCH_CAPS: list(range(RDCL_NO, RDCL_NO + 64)),
> +
> +        # The behaviour described by RRSBA depend on eIBRS being active.
> +        EIBRS: [RRSBA],

... for the purpose of the change here this dependency is all you need.

Jan