Re: [PATCH v3 3/4] x86/spec-ctrl: Fix up the RSBA/RRSBA bits as appropriate

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/06/2023 10:30 am, Jan Beulich wrote:
> On 12.06.2023 18:13, Andrew Cooper wrote:
>> @@ -593,15 +596,93 @@ static bool __init retpoline_calculations(void)
>>          return false;
>>  
>>      /*
>> -     * RSBA may be set by a hypervisor to indicate that we may move to a
>> -     * processor which isn't retpoline-safe.
>> +     * 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 Alternative) 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).
>> +     *
>> +     * - CPUs are not expected to enumerate both RSBA and RRSBA.
>> +     *
>> +     * 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 safety for migration.
>> +     *
>> +     * The following states exist:
>> +     *
>> +     * |   | RSBA | EIBRS | RRSBA | Notes              | Action        |
>> +     * |---+------+-------+-------+--------------------+---------------|
>> +     * | 1 |    0 |     0 |     0 | OK (older parts)   | Maybe +RSBA   |
>> +     * | 2 |    0 |     0 |     1 | Broken             | +RSBA, -RRSBA |
>> +     * | 3 |    0 |     1 |     0 | OK (pre-Aug ucode) | +RRSBA        |
>> +     * | 4 |    0 |     1 |     1 | OK                 |               |
>> +     * | 5 |    1 |     0 |     0 | OK                 |               |
>> +     * | 6 |    1 |     0 |     1 | Broken             | -RRSBA        |
>> +     * | 7 |    1 |     1 |     0 | Broken             | -RSBA, +RRSBA |
>> +     * | 8 |    1 |     1 |     1 | Broken             | -RSBA         |
> You've kept the Action column as you had it originally, despite no longer
> applying all the fixups. Wouldn't it make sense to mark those we don't do,
> e.g. by enclosing in parentheses?

Hmm, yes.  How does this look?

|   | RSBA | EIBRS | RRSBA | Notes              | Action (in principle) |
|---+------+-------+-------+--------------------+-----------------------|
| 1 |    0 |     0 |     0 | OK (older parts)   | Maybe +RSBA           |
| 2 |    0 |     0 |     1 | Broken             | (+RSBA, -RRSBA)       |
| 3 |    0 |     1 |     0 | OK (pre-Aug ucode) | +RRSBA                |
| 4 |    0 |     1 |     1 | OK                 |                       |
| 5 |    1 |     0 |     0 | OK                 |                       |
| 6 |    1 |     0 |     1 | Broken             | (-RRSBA)              |
| 7 |    1 |     1 |     0 | Broken             | (-RSBA, +RRSBA)       |
| 8 |    1 |     1 |     1 | Broken             | (-RSBA)               |


>> +     * further investigation.
>> +     */
>> +    if ( cpu_has_eibrs ? cpu_has_rsba  /* Rows 7, 8 */
>> +                       : cpu_has_rrsba /* Rows 2, 6 */ )
>> +    {
>> +        printk(XENLOG_ERR
>> +               "FIRMWARE BUG: CPU %02x-%02x-%02x, ucode 0x%08x: RSBA %u, 
>> EIBRS %u, RRSBA %u\n",
>> +               boot_cpu_data.x86, boot_cpu_data.x86_model,
>> +               boot_cpu_data.x86_mask, ucode_rev,
>> +               cpu_has_rsba, cpu_has_eibrs, cpu_has_rrsba);
> Perhaps with adjustments (as you deem them sensible)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

~Andrew