* [PATCH v2] x86/altcall: further refine clang workaround @ 2024-07-29 10:30 Roger Pau Monne 2024-07-29 10:47 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Roger Pau Monne @ 2024-07-29 10:30 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Alejandro Vallejo 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> --- xen/arch/x86/include/asm/alternative.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h index e63b45927643..12590504d465 100644 --- a/xen/arch/x86/include/asm/alternative.h +++ b/xen/arch/x86/include/asm/alternative.h @@ -183,13 +183,13 @@ extern void alternative_branches(void); * 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); })\ - } +#define ALT_CALL_ARG(arg, n) \ + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ + unsigned long tmp = 0; \ + *(typeof(arg) *)&tmp = (arg); \ + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ + tmp; \ + }) #else #define ALT_CALL_ARG(arg, n) \ register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \ -- 2.45.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 10:30 [PATCH v2] x86/altcall: further refine clang workaround Roger Pau Monne @ 2024-07-29 10:47 ` Jan Beulich 2024-07-29 11:12 ` Alejandro Vallejo 2024-07-29 12:00 ` Roger Pau Monné 0 siblings, 2 replies; 7+ messages in thread From: Jan Beulich @ 2024-07-29 10:47 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Alejandro Vallejo, xen-devel On 29.07.2024 12:30, Roger Pau Monne wrote: > --- a/xen/arch/x86/include/asm/alternative.h > +++ b/xen/arch/x86/include/asm/alternative.h > @@ -183,13 +183,13 @@ extern void alternative_branches(void); > * 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); })\ > - } > +#define ALT_CALL_ARG(arg, n) \ > + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ > + unsigned long tmp = 0; \ > + *(typeof(arg) *)&tmp = (arg); \ > + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ With this, even more so than before, I think the type of tmp would better be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider that less desirable). As a nit, I also don't think the backslashes need moving out by one position. Finally I'm afraid you're leaving stale the comment ahead of this construct. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 10:47 ` Jan Beulich @ 2024-07-29 11:12 ` Alejandro Vallejo 2024-07-29 12:00 ` Roger Pau Monné 1 sibling, 0 replies; 7+ messages in thread From: Alejandro Vallejo @ 2024-07-29 11:12 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monne; +Cc: Andrew Cooper, xen-devel On Mon Jul 29, 2024 at 11:47 AM BST, Jan Beulich wrote: > On 29.07.2024 12:30, Roger Pau Monne wrote: > > --- a/xen/arch/x86/include/asm/alternative.h > > +++ b/xen/arch/x86/include/asm/alternative.h > > @@ -183,13 +183,13 @@ extern void alternative_branches(void); > > * 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); })\ > > - } > > +#define ALT_CALL_ARG(arg, n) \ > > + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ > > + unsigned long tmp = 0; \ > > + *(typeof(arg) *)&tmp = (arg); \ > > + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ > > With this, even more so than before, I think the type of tmp would better > be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider > that less desirable). As a nit, I also don't think the backslashes need > moving out by one position. Finally I'm afraid you're leaving stale the > comment ahead of this construct. > > Jan I wouldn't be thrilled to create a temp variable of yet another type that is potentially neither "typeof(arg)" nor "unsigned long". No need to create a 3 body problem, where 2 is enough. Adjusting BUILD_BUG_ON() to use the same temp type seems sensible, but I don't mind very much. With the stale comment adjusted: Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Cheers, Alejandro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 10:47 ` Jan Beulich 2024-07-29 11:12 ` Alejandro Vallejo @ 2024-07-29 12:00 ` Roger Pau Monné 2024-07-29 12:41 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Roger Pau Monné @ 2024-07-29 12:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Alejandro Vallejo, xen-devel On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote: > On 29.07.2024 12:30, Roger Pau Monne wrote: > > --- a/xen/arch/x86/include/asm/alternative.h > > +++ b/xen/arch/x86/include/asm/alternative.h > > @@ -183,13 +183,13 @@ extern void alternative_branches(void); > > * 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); })\ > > - } > > +#define ALT_CALL_ARG(arg, n) \ > > + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ > > + unsigned long tmp = 0; \ > > + *(typeof(arg) *)&tmp = (arg); \ > > + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ > > With this, even more so than before, I think the type of tmp would better > be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider > that less desirable). Won't using void * be uglier, as we then need to cast the last tmp statement to unsigned long? > As a nit, I also don't think the backslashes need > moving out by one position. Finally I'm afraid you're leaving stale the > comment ahead of this construct. Yes, thanks for noticing. Will attempt to adjust the comment. Roger. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 12:00 ` Roger Pau Monné @ 2024-07-29 12:41 ` Jan Beulich 2024-07-29 12:47 ` Roger Pau Monné 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2024-07-29 12:41 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Alejandro Vallejo, xen-devel On 29.07.2024 14:00, Roger Pau Monné wrote: > On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote: >> On 29.07.2024 12:30, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/include/asm/alternative.h >>> +++ b/xen/arch/x86/include/asm/alternative.h >>> @@ -183,13 +183,13 @@ extern void alternative_branches(void); >>> * 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); })\ >>> - } >>> +#define ALT_CALL_ARG(arg, n) \ >>> + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ >>> + unsigned long tmp = 0; \ >>> + *(typeof(arg) *)&tmp = (arg); \ >>> + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ >> >> With this, even more so than before, I think the type of tmp would better >> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider >> that less desirable). > > Won't using void * be uglier, as we then need to cast the last tmp > statement to unsigned long? Only if we stick to using unsigned long for a ## n ## _. Afaics there's nothing wrong with making that void *, too. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 12:41 ` Jan Beulich @ 2024-07-29 12:47 ` Roger Pau Monné 2024-07-29 13:03 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Roger Pau Monné @ 2024-07-29 12:47 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Alejandro Vallejo, xen-devel On Mon, Jul 29, 2024 at 02:41:23PM +0200, Jan Beulich wrote: > On 29.07.2024 14:00, Roger Pau Monné wrote: > > On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote: > >> On 29.07.2024 12:30, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/include/asm/alternative.h > >>> +++ b/xen/arch/x86/include/asm/alternative.h > >>> @@ -183,13 +183,13 @@ extern void alternative_branches(void); > >>> * 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); })\ > >>> - } > >>> +#define ALT_CALL_ARG(arg, n) \ > >>> + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ > >>> + unsigned long tmp = 0; \ > >>> + *(typeof(arg) *)&tmp = (arg); \ > >>> + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ > >> > >> With this, even more so than before, I think the type of tmp would better > >> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider > >> that less desirable). > > > > Won't using void * be uglier, as we then need to cast the last tmp > > statement to unsigned long? > > Only if we stick to using unsigned long for a ## n ## _. Afaics there's > nothing wrong with making that void *, too. Right, but then for consistency I would also like to make r{10,11}_ void *, and ALT_CALL_NO_ARG(), which might be too much. My preference is likely to keep it at unsigned long, and adjust the BUILD_BUG_ON(), unless you have a strong opinion to change it to void * (and possibly the rest of the register variables). Thanks, Roger. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86/altcall: further refine clang workaround 2024-07-29 12:47 ` Roger Pau Monné @ 2024-07-29 13:03 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2024-07-29 13:03 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Alejandro Vallejo, xen-devel On 29.07.2024 14:47, Roger Pau Monné wrote: > On Mon, Jul 29, 2024 at 02:41:23PM +0200, Jan Beulich wrote: >> On 29.07.2024 14:00, Roger Pau Monné wrote: >>> On Mon, Jul 29, 2024 at 12:47:09PM +0200, Jan Beulich wrote: >>>> On 29.07.2024 12:30, Roger Pau Monne wrote: >>>>> --- a/xen/arch/x86/include/asm/alternative.h >>>>> +++ b/xen/arch/x86/include/asm/alternative.h >>>>> @@ -183,13 +183,13 @@ extern void alternative_branches(void); >>>>> * 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); })\ >>>>> - } >>>>> +#define ALT_CALL_ARG(arg, n) \ >>>>> + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \ >>>>> + unsigned long tmp = 0; \ >>>>> + *(typeof(arg) *)&tmp = (arg); \ >>>>> + BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); \ >>>> >>>> With this, even more so than before, I think the type of tmp would better >>>> be void * (or the BUILD_BUG_ON() be made use unsigned long, yet I consider >>>> that less desirable). >>> >>> Won't using void * be uglier, as we then need to cast the last tmp >>> statement to unsigned long? >> >> Only if we stick to using unsigned long for a ## n ## _. Afaics there's >> nothing wrong with making that void *, too. > > Right, but then for consistency I would also like to make r{10,11}_ > void *, and ALT_CALL_NO_ARG(), which might be too much. > > My preference is likely to keep it at unsigned long, and adjust the > BUILD_BUG_ON(), unless you have a strong opinion to change it to void > * (and possibly the rest of the register variables). That's okay if you prefer it that way; I said "less desirable" and I really don't mean any more than that. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-07-29 13:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-29 10:30 [PATCH v2] x86/altcall: further refine clang workaround Roger Pau Monne 2024-07-29 10:47 ` Jan Beulich 2024-07-29 11:12 ` Alejandro Vallejo 2024-07-29 12:00 ` Roger Pau Monné 2024-07-29 12:41 ` Jan Beulich 2024-07-29 12:47 ` Roger Pau Monné 2024-07-29 13:03 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.