Re: [PATCH 2/4] x86/spec-ctrl: Synthesize RSBA/RRSBA bits with older microcode

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/05/2023 10:18 am, Jan Beulich wrote:
> On 26.05.2023 13:06, Andrew Cooper wrote:
>> @@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
>>      if ( safe )
>>          return true;
>>  
>> +    /*
>> +     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
>> +     * agreed upon meaning at the time of writing (May 2023) is thus:
>> +     *
>> +     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
>> +     *   alternative predictor on underflow.  Skylake uarch and later all 
>> have
>> +     *   this property.  Broadwell too, when running microcode versions 
>> prior
>> +     *   to Jan 2018.
>> +     *
>> +     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
>> +     *   tagging of predictions with the mode in which they were learned.  
>> So
>> +     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
>> +     *
>> +     * Some parts (Broadwell) are not expected to ever enumerate this
>> +     * behaviour directly.  Other parts have differing enumeration with
>> +     * microcode version.  Fix up Xen's idea, so we can advertise them 
>> safely
>> +     * to guests, and so toolstacks can level a VM safelty for migration.
>> +     */
> If the difference between the two is whether eIBRS is active (as you did
> word it yet more explicitly in e.g. [1]), then ...
>
>> + unsafe_maybe_fixup_rrsba:
>> +    if ( !cpu_has_rrsba )
>> +        setup_force_cpu_cap(X86_FEATURE_RRSBA);
>> +
>> + unsafe_maybe_fixup_rsba:
>> +    if ( !cpu_has_rsba )
>> +        setup_force_cpu_cap(X86_FEATURE_RSBA);
>> +
>>      return false;
>>  }
> ... can both actually be active at the same time? IOW is there a "return
> false" missing ahead of the 2nd label?

I've already got a question out to Intel to this effect.  (I didn't say
the enumeration made much sense...)

> Not having looked at further patches yet it also strikes me as odd that
> each of the two labels is used exactly once only. Leaving the shared
> comment aside, imo this would then better avoid "goto".

They're both used twice, not once.  You asked why it wasn't "return
safe;" in the previous patch?  Well this is why.

> Finally, what use are the two if()s? There's nothing wrong with forcing
> a feature which is already available.

It breaks is_forced_cpu_cap().

Also, I considered having a printk() here.  I've still got it around in
a debug patch, but I decided against it.

~Andrew