| Message ID | 20240524200338.1232391-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/bitops: Untangle ffs()/fls() infrastructure | expand |
On 24.05.2024 22:03, Andrew Cooper wrote: > * Rename __attribute_pure__ to just __pure before it gains users. > * Introduce __constructor which is going to be used in lib/, and is > unconditionally cf_check. > * Identify the areas of xen/bitops.h which are a mess. > * Introduce xen/boot-check.h as helpers for compile and boot time testing. > This provides a statement of the ABI, and a confirmation that arch-specific > implementations behave as expected. > > Sadly Clang 7 and older isn't happy with the compile time checks. Skip them, > and just rely on the runtime checks. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Further remarks, though: > --- > xen/include/xen/bitops.h | 13 ++++++-- > xen/include/xen/boot-check.h | 60 ++++++++++++++++++++++++++++++++++++ > xen/include/xen/compiler.h | 3 +- > 3 files changed, 72 insertions(+), 4 deletions(-) > create mode 100644 xen/include/xen/boot-check.h The bulk of the changes isn't about bitops; it's just that you're intending to first use it for testing there. The subject prefix therefore is somewhat misleading. > --- /dev/null > +++ b/xen/include/xen/boot-check.h > @@ -0,0 +1,60 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > + > +/* > + * Helpers for boot-time checks of basic logic, including confirming that > + * examples which should be calculated by the compiler are. > + */ > +#ifndef XEN_BOOT_CHECK_H > +#define XEN_BOOT_CHECK_H > + > +#include <xen/lib.h> > + > +/* Hide a value from the optimiser. */ > +#define HIDE(x) \ > + ({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; }) In principle this is a macro that could be of use elsewhere. That's also reflected in its entirely generic name. It therefore feels mis-placed in this header. Otoh though the use of "+r" is more restricting than truly necessary: While I'm not sure if "+g" would work, i.e. if that wouldn't cause issues with literals, pretty surely "+rm" ought to work, removing the strict requirement for the compiler to put a certain value in a register. Assuming you may have reservations against "+g" / "+rm" (and hence the construct wants keeping here), maybe rename to e.g. BOOT_CHECK_HIDE()? Alternatively, if generalized, moving to xen/macros.h would seem appropriate to me. Finally, plainly as a remark with no request for any change (but possibly a minor argument against moving to xen/macros.h), this construct won't, afaict, work if x is of array(-of-const) type. A more specialized variant may need introducing, should any such use ever appear. Jan
On 27/05/2024 9:24 am, Jan Beulich wrote: > On 24.05.2024 22:03, Andrew Cooper wrote: >> * Rename __attribute_pure__ to just __pure before it gains users. >> * Introduce __constructor which is going to be used in lib/, and is >> unconditionally cf_check. >> * Identify the areas of xen/bitops.h which are a mess. >> * Introduce xen/boot-check.h as helpers for compile and boot time testing. >> This provides a statement of the ABI, and a confirmation that arch-specific >> implementations behave as expected. >> >> Sadly Clang 7 and older isn't happy with the compile time checks. Skip them, >> and just rely on the runtime checks. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > > Further remarks, though: > >> --- >> xen/include/xen/bitops.h | 13 ++++++-- >> xen/include/xen/boot-check.h | 60 ++++++++++++++++++++++++++++++++++++ >> xen/include/xen/compiler.h | 3 +- >> 3 files changed, 72 insertions(+), 4 deletions(-) >> create mode 100644 xen/include/xen/boot-check.h > The bulk of the changes isn't about bitops; it's just that you're intending > to first use it for testing there. The subject prefix therefore is somewhat > misleading. I'll change to "Cleanup and infrastructure ahead ..." but the bitops aspect is still reasonably important. >> --- /dev/null >> +++ b/xen/include/xen/boot-check.h >> @@ -0,0 +1,60 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> + >> +/* >> + * Helpers for boot-time checks of basic logic, including confirming that >> + * examples which should be calculated by the compiler are. >> + */ >> +#ifndef XEN_BOOT_CHECK_H >> +#define XEN_BOOT_CHECK_H Given that CONFIG_SELF_TESTS was subsequently approved, I've renamed this file to match. >> + >> +#include <xen/lib.h> >> + >> +/* Hide a value from the optimiser. */ >> +#define HIDE(x) \ >> + ({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; }) > In principle this is a macro that could be of use elsewhere. That's also > reflected in its entirely generic name. It therefore feels mis-placed in > this header. I'd forgotten that we several variations of this already. compiler.h has both OPTIMIZER_HIDE_VAR() and RELOC_HIDE(). > Otoh though the use of "+r" is more restricting than truly > necessary: While I'm not sure if "+g" would work, i.e. if that wouldn't > cause issues with literals, OPTIMIZER_HIDE_VAR() is indeed buggy using "+g", and RELOC_HIDE() even explains how "g" tickles a bug in a compiler we probably don't care about any more. [Slightly out of order] the use of OPTIMIZER_HIDE_VAR() in gsi_vioapic() is bogus AFAICT, and is actively creating the problem the commit message says it was trying to avoid. > pretty surely "+rm" ought to work, removing > the strict requirement for the compiler to put a certain value in a > register. "+rm" would be ideal in theory, we can't use it in practice because Clang will (still!) interpret it as "+m" and force a spill. While that's not necessarily a problem for the SELF_TESTS, it really is a problem in array_index_mask_nospec(), which is latently buggy even now. If the compiler really uses the flexibility offered by OPTIMIZER_HIDE_VAR() to spill the value, array_index_mask_nospec() has entirely failed at its purpose. > Assuming you may have reservations against "+g" / "+rm" (and hence the > construct wants keeping here), maybe rename to e.g. BOOT_CHECK_HIDE()? > Alternatively, if generalized, moving to xen/macros.h would seem > appropriate to me. I've moved it to macros.h (because we should consolidate around it), but kept as "+r" for both Clang and array_index_mask_nospec() reasons. I don't expect HIDE() is ever actually going to be used in a case where letting the value stay in memory is a useful thing overall. But if you still feel strongly about it, we can debate further when consolidating the other users. ~Andrew
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index e3c5a4ccf321..9b40f20381a2 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -1,5 +1,7 @@ -#ifndef _LINUX_BITOPS_H -#define _LINUX_BITOPS_H +#ifndef XEN_BITOPS_H +#define XEN_BITOPS_H + +#include <xen/compiler.h> #include <xen/types.h> /* @@ -103,8 +105,13 @@ static inline int generic_flsl(unsigned long x) * Include this here because some architectures need generic_ffs/fls in * scope */ + +/* --------------------- Please tidy above here --------------------- */ + #include <asm/bitops.h> +/* --------------------- Please tidy below here --------------------- */ + #ifndef find_next_bit /** * find_next_bit - find the next set bit in a memory region @@ -294,4 +301,4 @@ static inline __u32 ror32(__u32 word, unsigned int shift) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) -#endif +#endif /* XEN_BITOPS_H */ diff --git a/xen/include/xen/boot-check.h b/xen/include/xen/boot-check.h new file mode 100644 index 000000000000..250f9a40d3b0 --- /dev/null +++ b/xen/include/xen/boot-check.h @@ -0,0 +1,60 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +/* + * Helpers for boot-time checks of basic logic, including confirming that + * examples which should be calculated by the compiler are. + */ +#ifndef XEN_BOOT_CHECK_H +#define XEN_BOOT_CHECK_H + +#include <xen/lib.h> + +/* Hide a value from the optimiser. */ +#define HIDE(x) \ + ({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; }) + +/* + * Check that fn(val) can be calcuated by the compiler, and that it gives the + * expected answer. + * + * Clang < 8 can't fold constants through static inlines, causing this to + * fail. Simply skip it for incredibly old compilers. + */ +#if !CONFIG_CC_IS_CLANG || CONFIG_CLANG_VERSION >= 80000 +#define COMPILE_CHECK(fn, val, res) \ + do { \ + typeof(fn(val)) real = fn(val); \ + \ + if ( !__builtin_constant_p(real) ) \ + asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" ); \ + else if ( real != res ) \ + asm ( ".error \"Compile time check '" STR(fn(val) == res) "' failed\"" ); \ + } while ( 0 ) +#else +#define COMPILE_CHECK(fn, val, res) +#endif + +/* + * Check that Xen's runtime logic for fn(val) gives the expected answer. This + * requires using HIDE() to prevent the optimiser from collapsing the logic + * into a constant. + */ +#define RUNTIME_CHECK(fn, val, res) \ + do { \ + typeof(fn(val)) real = fn(HIDE(val)); \ + \ + if ( real != res ) \ + panic("%s: %s(%s) expected %u, got %u\n", \ + __func__, #fn, #val, real, res); \ + } while ( 0 ) + +/* + * Perform compiletime and runtime checks for fn(val) == res. + */ +#define CHECK(fn, val, res) \ + do { \ + COMPILE_CHECK(fn, val, res); \ + RUNTIME_CHECK(fn, val, res); \ + } while ( 0 ) + +#endif /* XEN_BOOT_CHECK_H */ diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 179ff23e62c5..444bf80142c7 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -86,7 +86,8 @@ #define inline inline __init #endif -#define __attribute_pure__ __attribute__((__pure__)) +#define __constructor __attribute__((__constructor__)) cf_check +#define __pure __attribute__((__pure__)) #define __attribute_const__ __attribute__((__const__)) #define __transparent__ __attribute__((__transparent_union__))
* Rename __attribute_pure__ to just __pure before it gains users. * Introduce __constructor which is going to be used in lib/, and is unconditionally cf_check. * Identify the areas of xen/bitops.h which are a mess. * Introduce xen/boot-check.h as helpers for compile and boot time testing. This provides a statement of the ABI, and a confirmation that arch-specific implementations behave as expected. Sadly Clang 7 and older isn't happy with the compile time checks. Skip them, and just rely on the runtime checks. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> CC: consulting@bugseng.com <consulting@bugseng.com> CC: Simone Ballarin <simone.ballarin@bugseng.com> CC: Federico Serafini <federico.serafini@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> v2: * Break macros out into a header as they're going to be used elsewhere too * Use panic() rather than BUG_ON() to be more helpful when something fails * Brackets in HIDE() * Alignment adjustments * Skip COMPILE_CHECK() for Clang < 8 --- xen/include/xen/bitops.h | 13 ++++++-- xen/include/xen/boot-check.h | 60 ++++++++++++++++++++++++++++++++++++ xen/include/xen/compiler.h | 3 +- 3 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 xen/include/xen/boot-check.h