Re: [PATCH for-4.19] x86/altcall: fix clang code-gen when using altcall in loop constructs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2024 18:24, Alejandro Vallejo wrote:
> On Tue Jul 23, 2024 at 5:09 PM BST, Roger Pau Monné wrote:
>> On Tue, Jul 23, 2024 at 04:37:12PM +0100, Alejandro Vallejo wrote:
>>> On Tue Jul 23, 2024 at 10:31 AM BST, Roger Pau Monne wrote:
>>>> --- a/xen/arch/x86/include/asm/alternative.h
>>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>>> @@ -185,10 +185,10 @@ extern void alternative_branches(void);
>>>>   */
>>>>  #define ALT_CALL_ARG(arg, n)                                            \
>>>>      register union {                                                    \
>>>> -        typeof(arg) e;                                                  \
>>>> +        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
>>>>          unsigned long r;                                                \
>>>>      } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
>>>> -        .e = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })   \
>>>> +        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
>>>>      }
>>>>  #else
>>>>  #define ALT_CALL_ARG(arg, n) \
>>>
>>> Don't we want BUILD_BUG_ON(sizeof(long) % sizeof(arg) == 0) instead?
>>
>> I think you meant BUILD_BUG_ON(sizeof(long) % sizeof(arg) != 0)?
> 
> Bah, yes. I wrote it as a COMPILE_ASSERT().
> 
>>
>>> Otherwise
>>> odd sizes will cause the wrong union size to prevail, and while I can't see
>>> today how those might come to happen there's Murphy's law.
>>
>> The overall union size would still be fine, because it has the
>> unsigned long element, it's just that the array won't cover all the
>> space assigned to the long member?
> 
> I explained myself poorly. If the current BUILD_BUG_ON() stays as-is that's
> right, but...
> 
>>
>> IOW if sizeof(arg) == 7, then we would define an array with only 1
>> element, which won't make the size of the union change, but won't
>> cover the same space that's used by the long member.
> 
> ... I thought the point of the patch was to cover the full union with the
> array, and not just a subset. My proposed alternative merely tries to ensure
> the argument is always a submultiple in size of a long so the array is always 
> a
> perfect match.

Question is whether there's an issue with odd sized values in Clang. I
wouldn't want to exclude such (admittedly somewhat exotic) uses "just
in case". My understanding here is that the issue the patch addresses
is not merely the treatment of the union by Clang, but the combination
thereof with it violating the psABI when it comes to passing bool
around.

Jan