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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2024 17:53, Roger Pau Monne wrote:
> 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@xxxxxxxxx>
> Suggested-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Albeit if you don't mind ...

> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -169,27 +169,25 @@ extern void alternative_branches(void);
>  
>  #ifdef CONFIG_CC_IS_CLANG
>  /*
> - * Use a union with an unsigned long in order to prevent clang from
> - * skipping a possible truncation of the value.  By using the union any
> - * truncation is carried before the call instruction, in turn covering
> - * for ABI-non-compliance in that the necessary clipping / extension of
> - * the value is supposed to be carried out in the callee.
> + * Clang doesn't follow the psABI and doesn't truncate parameter values at 
> the
> + * callee.  This can lead to bad code being generated when using alternative
> + * calls.
>   *
> - * Note this behavior is not mandated by the standard, and hence could
> - * stop being a viable workaround, or worse, could cause a different set
> - * of code-generation issues in future clang versions.
> + * Workaround it by using a temporary intermediate variable that's zeroed
> + * before being assigned the parameter value, as that forces clang to zero 
> the
> + * register at the caller.
>   *
>   * This has been reported upstream:
>   * 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); })\
> -    }
> +    register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({   \
> +        unsigned long tmp = 0;                                          \
> +        *(typeof(arg) *)&tmp = (arg);                                   \
> +        BUILD_BUG_ON(sizeof(arg) > sizeof(unsigned long));              \

... I'd like to switch around these two lines while committing.

Jan

> +        tmp;                                                            \
> +    })
>  #else
>  #define ALT_CALL_ARG(arg, n) \
>      register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \