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

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Tue Jul 23, 2024 at 5:09 PM BST, Roger Pau Monné wrote:
> 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:
> > > 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@suse.com>
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > 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)?

Bah, yes. I wrote it as a COMPILE_ASSERT().

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

I explained myself poorly. If the current BUILD_BUG_ON() stays as-is that's
right, but...

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

... I thought the point of the patch was to cover the full union with the
array, and not just a subset. My proposed alternative merely tries to ensure
the argument is always a submultiple in size of a long so the array is always a
perfect match.

Though admittedly, it wouldn't be rare for this to be enough to work around the
bug.

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

Cheers,
Alejandro