Re: [PATCH] x86/spec-ctrl: Knobs for STIBP and PSFD, and follow hardware STIBP hint

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2022 15:00, Andrew Cooper wrote:
> STIBP and PSFD are slightly weird bits, because they're both implied by other
> bits in MSR_SPEC_CTRL.  Add fine grain controls for them, and take the
> implications into account when setting IBRS/SSBD.
> 
> Rearrange the IBPB text/variables/logic to keep all the MSR_SPEC_CTRL bits
> together, for consistency.
> 
> However, AMD have a hardware hint CPUID bit recommending that STIBP be set
> uniaterally.  This is advertised on Zen3, so follow the recommendation.  This
> is the only default change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

In principle
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
but I have two comments:

> @@ -170,12 +174,18 @@ static int __init cf_check parse_spec_ctrl(const char 
> *s)
>              else
>                  rc = -EINVAL;
>          }
> +
>          else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
>              opt_ibrs = val;
> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
> -            opt_ibpb = val;
> +        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
> +            opt_stibp = val;
>          else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
>              opt_ssbd = val;
> +        else if ( (val = parse_boolean("psfd", s, ss)) >= 0 )
> +            opt_psfd = val;
> +
> +        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
> +            opt_ibpb = val;
>          else if ( (val = parse_boolean("eager-fpu", s, ss)) >= 0 )
>              opt_eager_fpu = val;
>          else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )

Personally I find blank lines ahead of "else if" misleading. Could I
talk you into moving ibrs+stibp and ssbd+psfd close to the end of this
(immediately ahead of "else"), prefixing each pair with a comment about
one feature implying the other (and hence the comments replacing the
blank lines)?

Otoh I notice that we already have blank lines elsewhere in the middle
if this block of code, but at least there they're accompanied by a
comment making more obvious why there is such separation. Which means
as an intermediate approach I'd be okay with no re-ordering, but with
comments added.

> @@ -1070,12 +1083,50 @@ void __init init_speculation_mitigations(void)
>  
>      /* If we have IBRS available, see whether we should use it. */
>      if ( has_spec_ctrl && ibrs )
> +    {
> +        /* IBRS implies STIBP.  */
> +        if ( opt_stibp == -1 )
> +            opt_stibp = 1;
> +
>          default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
> +    }
> +
> +    /* Use STIBP by default if the hardware hint is set. */
> +    if ( opt_stibp == -1 && boot_cpu_has(X86_FEATURE_STIBP_ALWAYS) )
> +        opt_stibp = 1;
> +
> +    /*
> +     * Otherwise, don't use STIBP by default.  It has some severe performance
> +     * implications on older hardware.
> +     */
> +    if ( opt_stibp == -1 )
> +        opt_stibp = 0;

I'd find this easier to read if written along the lines of

    if ( opt_stibp == -1 )
    {
        /*
         * Use STIBP by default if the hardware hint is set.  Otherwise,
         * don't use STIBP by default.  It has some severe performance
         * implications on older hardware.
         */
        opt_stibp = !!boot_cpu_has(X86_FEATURE_STIBP_ALWAYS);
    }

FTAOD I'm not going to insist on either adjustment.

Jan