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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-23 at 11:31 +0200, Roger Pau Monne wrote:
> Yet another clang code generation issue when using altcalls.
> 
> The issue this time is with using loop constructs around
> alternative_{,v}call
> instances using parameter types smaller than the register size.
> 
> Given the following example code:
> 
> static void bar(bool b)
> {
>     unsigned int i;
> 
>     for ( i = 0; i < 10; i++ )
>     {
>         int ret_;
>         register union {
>             bool e;
>             unsigned long r;
>         } di asm("rdi") = { .e = b };
>         register unsigned long si asm("rsi");
>         register unsigned long dx asm("rdx");
>         register unsigned long cx asm("rcx");
>         register unsigned long r8 asm("r8");
>         register unsigned long r9 asm("r9");
>         register unsigned long r10 asm("r10");
>         register unsigned long r11 asm("r11");
> 
>         asm volatile ( "call %c[addr]"
>                        : "+r" (di), "=r" (si), "=r" (dx),
>                          "=r" (cx), "=r" (r8), "=r" (r9),
>                          "=r" (r10), "=r" (r11), "=a" (ret_)
>                        : [addr] "i" (&(func)), "g" (func)
>                        : "memory" );
>     }
> }
> 
> See: https://godbolt.org/z/qvxMGd84q
> 
> Clang will generate machine code that only resets the low 8 bits of
> %rdi
> between loop calls, leaving the rest of the register possibly
> containing
> garbage from the use of %rdi inside the called function.  Note also
> that clang
> doesn't truncate the input parameters at the callee, thus breaking
> the psABI.
> 
> Fix this by turning the `e` element in the anonymous union into an
> array that
> consumes the same space as an unsigned long, as this forces clang to
> reset the
> whole %rdi register instead of just the low 8 bits.
> 
> Fixes: 2ce562b2a413 ('x86/altcall: use a union as register type for
> function parameters on clang')
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Adding Oleksii as to whether this could be considered for 4.19: it's
> strictly
> limited to clang builds, plus will need to be backported anyway.
> ---
I am okay to have this change in 4.19 but then it should be reviewed
and merged ASAP as the tree was tagged few minutes ago and I expected
that it will be the last one RC in this release cycle:

Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii

>  xen/arch/x86/include/asm/alternative.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/alternative.h
> b/xen/arch/x86/include/asm/alternative.h
> index 0d3697f1de49..e63b45927643 100644
> --- 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) \