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

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

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:
> > 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.
> > ---
> >  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) \
> 
> 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)?

> 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?

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.

However it's not possible for sizeof(arg) > 8 due to the existing
BUILD_BUG_ON(), so the union can never be bigger than a long.

Thanks, Roger.