On 25/10/2019 13:24, Jan Beulich wrote:
> 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.
Actually any masking approach, even cmp/sbb, would be unsafe so I
suppose this note isn't accurate.
I'm aware of one x86 plan for data value speculation, which was delayed
indefinitely following the fallout from Spectre/Meltdown, especially as
L1TF at its core is a speculative address prediction bug. Suffice it to
say that the vendors are aware that any plans along these lines will
need to be done with great care.
>
>> --- 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"?
Oops yes.
>
>> @@ -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.
>
>> --- 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;
})
This check makes no sense on negative numbers.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
