[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH for-4.19] x86/altcall: fix clang code-gen when using altcall in loop constructs
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@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)? 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |