| Message ID | 20240729103050.38401-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/altcall: further refine clang workaround | expand |
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
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
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.
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
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.
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
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 ) = \
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(-)