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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 31, 2024 at 08:26:02AM +0200, Jan Beulich wrote:
> 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.

Sure, feel free to swap them.

Thanks, Roger.