Re: [Xen-devel] [PATCH v3 7/7] x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2019 15:58, Andrew Cooper wrote:
> This optimisation is not safe on ARM, because some CPUs do data value
> speculation, which is why the CSDB barrer was introduced.

So if an x86 CPU appeared which did so too, it would immediately be
unsafe as well aiui. I.e. we'd have to again go and fix the logic.
Not to be taken as an outright objection, but to perhaps prompt
further consideration.

> --- a/xen/include/asm-x86/nospec.h
> +++ b/xen/include/asm-x86/nospec.h
> @@ -7,13 +7,20 @@
>  #include <asm/alternative.h>
>  
>  /**
> - * array_index_mask_nospec() - generate a mask that is ~0UL when the
> - *      bounds check succeeds and 0 otherwise
> + * array_index_mask_nospec() - generate a mask to bound an array index
> + * which is safe even under adverse speculation.
>   * @index: array element index
>   * @size: number of elements in array
>   *
> - * Returns:
> + * In general, returns:
>   *     0 - (index < size)
> + *
> + * This yeild ~0UL in within-bounds case, and 0 in the out-of-bounds

Nit: "yields"?

> @@ -21,9 +28,15 @@ static inline unsigned long 
> array_index_mask_nospec(unsigned long index,
>  {
>      unsigned long mask;
>  
> -    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
> -                   : [mask] "=r" (mask)
> -                   : [size] "g" (size), [index] "r" (index) );
> +    if ( __builtin_constant_p(size) && IS_POWER_OF_2(size) )
> +    {
> +        mask = size - 1;
> +        OPTIMIZER_HIDE_VAR(mask);

I can't seem to be able to figure why you need this.

> --- a/xen/include/xen/config.h
> +++ b/xen/include/xen/config.h
> @@ -75,6 +75,7 @@
>  #define GB(_gb)     (_AC(_gb, ULL) << 30)
>  
>  #define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
> +#define IS_POWER_OF_2(val) ((val) && IS_ALIGNED(val, val))

While the risk may seem low for someone to pass an expression with
side effect here, evaluating "val" up to three times here doesn't
look very desirable.

As a minor remark, without considering representation I'd expect
an expression IS_ALIGNED(val, val) to consistently produce "true"
for all non-zero values. E.g. 3 is certainly "aligned" on a
boundary of 3.

Finally this may want guarding against use on signed types - at
the very least it looks to produce the wrong answer for e.g.
INT_MIN or LONG_MIN. I.e. perhaps the expression to the left of
&& wants to be (val) > 0.

Jan

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