[PATCH v2] x86/altcall: further refine clang workaround

[PATCH v2] x86/altcall: further refine clang workaround

[Roger Pau Monne]

The current code in ALT_CALL_ARG() won't successfully workaround the clang
code-generation issue if the arg parameter has a size that's not a power of 2.
While there are no such sized parameters at the moment, improve the workaround
to also be effective when such sizes are used.

Instead of using a union with a long use an unsigned long that's first
initialized to 0 and afterwards set to the argument value.

Reported-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/alternative.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index e63b45927643..12590504d465 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -183,13 +183,13 @@ extern void alternative_branches(void);
  * https://github.com/llvm/llvm-project/issues/12579
  * https://github.com/llvm/llvm-project/issues/82598
  */
-#define ALT_CALL_ARG(arg, n)                                            \
-    register union {                                                    \
-        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
-        unsigned long r;                                                \
-    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
-        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
-    }
+#define ALT_CALL_ARG(arg, n)                                             \
+     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
+         unsigned long tmp = 0;                                          \
+         *(typeof(arg) *)&tmp = (arg);                                   \
+         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
+         tmp;                                                            \
+     })
 #else
 #define ALT_CALL_ARG(arg, n) \
     register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
-- 
2.45.2

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Jan Beulich]

On 29.07.2024 12:30, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -183,13 +183,13 @@ extern void alternative_branches(void);
>   * https://github.com/llvm/llvm-project/issues/12579
>   * https://github.com/llvm/llvm-project/issues/82598
>   */
> -#define ALT_CALL_ARG(arg, n)                                            \
> -    register union {                                                    \
> -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
> -        unsigned long r;                                                \
> -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
> -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> -    }
> +#define ALT_CALL_ARG(arg, n)                                             \
> +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
> +         unsigned long tmp = 0;                                          \
> +         *(typeof(arg) *)&tmp = (arg);                                   \
> +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \

With this, even more so than before, I think the type of tmp would better
be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
that less desirable). As a nit, I also don't think the backslashes need
moving out by one position. Finally I'm afraid you're leaving stale the
comment ahead of this construct.

Jan

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Alejandro Vallejo]

On Mon Jul 29, 2024 at 11:47 AM BST, Jan Beulich wrote:
> On 29.07.2024 12:30, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -183,13 +183,13 @@ extern void alternative_branches(void);
> >   * https://github.com/llvm/llvm-project/issues/12579
> >   * https://github.com/llvm/llvm-project/issues/82598
> >   */
> > -#define ALT_CALL_ARG(arg, n)                                            \
> > -    register union {                                                    \
> > -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
> > -        unsigned long r;                                                \
> > -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
> > -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> > -    }
> > +#define ALT_CALL_ARG(arg, n)                                             \
> > +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
> > +         unsigned long tmp = 0;                                          \
> > +         *(typeof(arg) *)&tmp = (arg);                                   \
> > +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
>
> With this, even more so than before, I think the type of tmp would better
> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
> that less desirable). As a nit, I also don't think the backslashes need
> moving out by one position. Finally I'm afraid you're leaving stale the
> comment ahead of this construct.
>
> Jan

I wouldn't be thrilled to create a temp variable of yet another type that is
potentially neither "typeof(arg)" nor "unsigned long". No need to create a 3
body problem, where 2 is enough. Adjusting BUILD_BUG_ON() to use the same temp
type seems sensible, but I don't mind very much.

With the stale comment adjusted:

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Roger Pau Monné]

On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote:
> On 29.07.2024 12:30, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -183,13 +183,13 @@ extern void alternative_branches(void);
> >   * https://github.com/llvm/llvm-project/issues/12579
> >   * https://github.com/llvm/llvm-project/issues/82598
> >   */
> > -#define ALT_CALL_ARG(arg, n)                                            \
> > -    register union {                                                    \
> > -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
> > -        unsigned long r;                                                \
> > -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
> > -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> > -    }
> > +#define ALT_CALL_ARG(arg, n)                                             \
> > +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
> > +         unsigned long tmp = 0;                                          \
> > +         *(typeof(arg) *)&tmp = (arg);                                   \
> > +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
> 
> With this, even more so than before, I think the type of tmp would better
> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
> that less desirable).

Won't using void * be uglier, as we then need to cast the last tmp
statement to unsigned long?

> As a nit, I also don't think the backslashes need
> moving out by one position. Finally I'm afraid you're leaving stale the
> comment ahead of this construct.

Yes, thanks for noticing.  Will attempt to adjust the comment.

Roger.

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Jan Beulich]

On 29.07.2024 14:00, Roger Pau Monné wrote:
> On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote:
>> On 29.07.2024 12:30, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/include/asm/alternative.h
>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>> @@ -183,13 +183,13 @@ extern void alternative_branches(void);
>>>   * https://github.com/llvm/llvm-project/issues/12579
>>>   * https://github.com/llvm/llvm-project/issues/82598
>>>   */
>>> -#define ALT_CALL_ARG(arg, n)                                            \
>>> -    register union {                                                    \
>>> -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
>>> -        unsigned long r;                                                \
>>> -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
>>> -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
>>> -    }
>>> +#define ALT_CALL_ARG(arg, n)                                             \
>>> +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
>>> +         unsigned long tmp = 0;                                          \
>>> +         *(typeof(arg) *)&tmp = (arg);                                   \
>>> +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
>>
>> With this, even more so than before, I think the type of tmp would better
>> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
>> that less desirable).
> 
> Won't using void * be uglier, as we then need to cast the last tmp
> statement to unsigned long?

Only if we stick to using unsigned long for a ## n ## _. Afaics there's
nothing wrong with making that void *, too.

Jan

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Roger Pau Monné]

On Mon, Jul 29, 2024 at 02:41:23PM +0200, Jan Beulich wrote:
> On 29.07.2024 14:00, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote:
> >> On 29.07.2024 12:30, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/include/asm/alternative.h
> >>> +++ b/xen/arch/x86/include/asm/alternative.h
> >>> @@ -183,13 +183,13 @@ extern void alternative_branches(void);
> >>>   * https://github.com/llvm/llvm-project/issues/12579
> >>>   * https://github.com/llvm/llvm-project/issues/82598
> >>>   */
> >>> -#define ALT_CALL_ARG(arg, n)                                            \
> >>> -    register union {                                                    \
> >>> -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
> >>> -        unsigned long r;                                                \
> >>> -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
> >>> -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> >>> -    }
> >>> +#define ALT_CALL_ARG(arg, n)                                             \
> >>> +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
> >>> +         unsigned long tmp = 0;                                          \
> >>> +         *(typeof(arg) *)&tmp = (arg);                                   \
> >>> +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
> >>
> >> With this, even more so than before, I think the type of tmp would better
> >> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
> >> that less desirable).
> > 
> > Won't using void * be uglier, as we then need to cast the last tmp
> > statement to unsigned long?
> 
> Only if we stick to using unsigned long for a ## n ## _. Afaics there's
> nothing wrong with making that void *, too.

Right, but then for consistency I would also like to make r{10,11}_
void *, and ALT_CALL_NO_ARG(), which might be too much.

My preference is likely to keep it at unsigned long, and adjust the
BUILD_BUG_ON(), unless you have a strong opinion to change it to void
* (and possibly the rest of the register variables).

Thanks, Roger.

Re: [PATCH v2] x86/altcall: further refine clang workaround

[Jan Beulich]

On 29.07.2024 14:47, Roger Pau Monné wrote:
> On Mon, Jul 29, 2024 at 02:41:23PM +0200, Jan Beulich wrote:
>> On 29.07.2024 14:00, Roger Pau Monné wrote:
>>> On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote:
>>>> On 29.07.2024 12:30, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/include/asm/alternative.h
>>>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>>>> @@ -183,13 +183,13 @@ extern void alternative_branches(void);
>>>>>   * https://github.com/llvm/llvm-project/issues/12579
>>>>>   * https://github.com/llvm/llvm-project/issues/82598
>>>>>   */
>>>>> -#define ALT_CALL_ARG(arg, n)                                            \
>>>>> -    register union {                                                    \
>>>>> -        typeof(arg) e[sizeof(long) / sizeof(arg)];                      \
>>>>> -        unsigned long r;                                                \
>>>>> -    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
>>>>> -        .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
>>>>> -    }
>>>>> +#define ALT_CALL_ARG(arg, n)                                             \
>>>>> +     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
>>>>> +         unsigned long tmp = 0;                                          \
>>>>> +         *(typeof(arg) *)&tmp = (arg);                                   \
>>>>> +         BUILD_BUG_ON(sizeof(arg) > sizeof(void *));                     \
>>>>
>>>> With this, even more so than before, I think the type of tmp would better
>>>> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider
>>>> that less desirable).
>>>
>>> Won't using void * be uglier, as we then need to cast the last tmp
>>> statement to unsigned long?
>>
>> Only if we stick to using unsigned long for a ## n ## _. Afaics there's
>> nothing wrong with making that void *, too.
> 
> Right, but then for consistency I would also like to make r{10,11}_
> void *, and ALT_CALL_NO_ARG(), which might be too much.
> 
> My preference is likely to keep it at unsigned long, and adjust the
> BUILD_BUG_ON(), unless you have a strong opinion to change it to void
> * (and possibly the rest of the register variables).

That's okay if you prefer it that way; I said "less desirable" and I really
don't mean any more than that.

Jan