On 31/01/2019 15:50, Jan Beulich wrote:
>>>> On 31.01.19 at 15:54, <andrew.coop...@citrix.com> wrote:
>> On 31/01/2019 14:25, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>>>
>>> ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>>>
>>> - /* For safety in release builds. Debug builds will hit the ASSERT() */
>>> - modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
>>> -
>>> - return (void *)regs + byte_reg_offsets[modrm_reg];
>>> + return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
>>> }
>> Actually, the &= here wasn't by accident. When the array size is an
>> power of two and known to the compiler, it is a rather lower overhead
>> alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
>> the asm volatile statement.
>>
>> I wonder if there is a sensible way cope with this in
>> array_access_nospec(). Perhaps something like:
>>
>> #define array_access_nospec(array, index)
>> ({
>> size_t _s = ARRAY_SIZE(array);
>>
>> if ( !(_s & (_s - 1)) )
>> {
>> typeof(index) _i = index & (_s - 1);
>> OPTIMIZER_HIDE_VAR(_i);
>> (array)[_i];
>> }
>> else
>> (array)[array_index_nospec(index, ARRAY_SIZE(array))];
>> })
>>
>> As _s is known at compile time, only one half of the if condition will
>> be emitted by the compiler.
> Except that this won't work as an lvalue anymore, yet we want
> to use it as such in some cases. I can't seem to immediately think
> of a way to overcome this.
Does the lvalue use result in safe asm? Irrespective, this macro can
probably be expressed as a ternary operation and retain its lvalue-ness,
albeit at the expense of readability.
>
> As just said on the call, in the re-based AVX512F gather patch
> the respective hunk is now
>
> --- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -662,9 +662,10 @@ static inline unsigned long *decode_gpr(
> BUILD_BUG_ON(ARRAY_SIZE(cpu_user_regs_gpr_offsets) &
> (ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1));
>
> - ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
> + /* Note that this also acts as array_access_nospec() stand-in. */
> + modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
>
> - return (void *)regs + array_access_nospec(cpu_user_regs_gpr_offsets,
> modrm);
> + return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
> }
>
> /* Unhandleable read, write or instruction fetch */
>
> I could obviously make the patch here simply insert that comment
> instead of adding actual uses of the macro.
Ah ok - this looks fine, however it ends up looking.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
