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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2023 12:00, Andrew Cooper wrote:
> 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.

Ouch, yes. The labels themselves are used just once, but there's
important fall-through from above here.

>> 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().

Hmm, yes, but is that important here? (If you decide to keep the if()s,
which I'm not opposed to, would you mind adding half a sentence to the
description or maybe a brief code comment?)

Jan

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