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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> @@ -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?

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.

Jan



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