Re: [Xen-devel] [PATCH] x86/spec_ctrl: Extend repoline safey calcuations for eIBRS and Atom parts

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/03/2019 11:58, Jan Beulich wrote:
>>>> On 18.03.19 at 12:27, <andrew.cooper3@xxxxxxxxxx> wrote:
>> However, an additional meaning of Enhanced IRBS is that the processor may not
>> be retpoline-safe.  The Gemini Lake platform, based on the Goldmont+
>> microarchitecture is the first Atom processor to support eIBRS, even though 
>> it
>> is in practice safe.
> But afaict you don't mark them as safe.

Correct.  That is deliberate.

> But then again I don't recall:
> Performance-wise, is retpoline considered better or worse than IBRS?
> Depending on that ...

Retpoline is more performant than IRBS as implemented in microcode, but
that is not what we are comparing to here.

Goldmont+ is the first processor with hardware fixes for Spectre v2
issues.  This means that IBRS isn't implemented behind the scenes with
"turn off the BTB" or "flush via convoluted means".  There is a proper
O(1) flush of the BTB for IBPB, and the BTB now tracks the privilege at
which a prediction was learnt.

Cascade Lake will (AFAIA) be the first Big-Core processor with eIBRS.

I honestly don't know whether retpoline or legacy-IBRS with new hardware
will be faster, but it doesn't really matter - using repotline on
Goldmont+ is not spec compliant.

Teaching Xen about eIBRS is moderately high on my priority list, after
which eIBRS will be the most performant option without a shadow of a doubt.

>
>> --- a/xen/arch/x86/spec_ctrl.c
>> +++ b/xen/arch/x86/spec_ctrl.c
>> @@ -316,8 +316,11 @@ static bool __init retpoline_safe(uint64_t caps)
>>      /*
>>       * RSBA may be set by a hypervisor to indicate that we may move to a
>>       * processor which isn't retpoline-safe.
>> +     *
>> +     * Processors offering Enhanced IBRS are not guarenteed to be
>> +     * repoline-safe.
>>       */
>> -    if ( caps & ARCH_CAPS_RSBA )
>> +    if ( caps & (ARCH_CAPABILITIES_IBRS_ALL | ARCH_CAPS_RSBA) )
>>          return false;
> ... are there plans to retrofit EIBRS onto any of the models explicitly
> named in the subsequent switch()? If not, wouldn't you better
> check for it further down, adding Goldmont Plus into ...

EIBRS requires new silicon.  It can't be retrofitted in microcode.

>
>> @@ -377,6 +380,23 @@ static bool __init retpoline_safe(uint64_t caps)
>>      case 0x9e:
>>          return false;
>>  
>> +        /*
>> +         * Atom processors before Goldmont+/Gemini Lake are retpoline-safe.
>> +         */
>> +    case 0x1c: /* Pineview */
>> +    case 0x26: /* Lincroft */
>> +    case 0x27: /* Penwell */
>> +    case 0x35: /* Cloverview */
>> +    case 0x36: /* Cedarview */
>> +    case 0x37: /* Baytrail / Valleyview (Silvermont) */
>> +    case 0x4d: /* Avaton / Rangely (Silvermont) */
>> +    case 0x4c: /* Cherrytrail / Brasswell */
>> +    case 0x4a: /* Merrifield */
>> +    case 0x5a: /* Moorefield */
>> +    case 0x5c: /* Goldmont */
>> +    case 0x5f: /* Denverton */
>> +        return true;
> ... this set? Or am I simply misreading the patch description?

The switch statement is only intended for "legacy" processors which
don't enumerate explicit information, specifically to prevent it needing
to grow with future fixed silicon.

> As an aside - as mentioned on the other (still unfinished) thread
> regarding one of my patches, there's no mention of Goldmont+
> anywhere in the SDM afaics, it's always Goldmont Plus. I still don't
> understand why you want us to deliberately diverge.

Probably a side effect of shorthands.  If Goldmont+ really doesn't exist
anywhere, I'll switch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel