| Message ID | 20240730155305.49172-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86/altcall: further refine clang workaround | expand |
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@cloud.com> > Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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 ) = \
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@cloud.com> > > Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > 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.
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h index e63b45927643..c5fa242e76b3 100644 --- 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)); \ + tmp; \ + }) #else #define ALT_CALL_ARG(arg, n) \ register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
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@cloud.com> Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - Fix indentation and style issues. - Adjust comment to match the new workaround. --- xen/arch/x86/include/asm/alternative.h | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)