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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2019 14:58, Andrew Cooper wrote:
> On 25/10/2019 13:24, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> @@ -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.
> 
> Because I found cases where the AND was elided by the compiler entirely
> without it.

Would you mind mentioning this in the description, or in a comment?

>>> --- 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.
> 
> That is easy to fix.
> 
>> 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.
> 
> IS_ALIGNED() comes with an expectation of being against a power of 2,
> because otherwise you'd need a DIV instruction for the general case.
> 
> Some users can't cope with a compile time check.
> 
>> 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.
> 
> How about the above expansion fix becoming:
> 
> ({
>     unsigned typeof(val) _val = val;
>     _val && (_val & (_val - 1)) == 0;
> })

Well, if the "unsigned typeof()" construct works - why not (with
"val" properly parenthesized, and preferable the leading underscore
changed to a trailing one).

> This check makes no sense on negative numbers.

Of course not, but someone might use it on a signed type and get
back true when it was supposed to be false, just because the value
used ended up being a negative number.

Jan

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