| Message ID | 20240313172716.2325427-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/bitops: Reduce the mess, starting with ffs() | expand |
Hi Andrew, On 3/13/24 12:27 PM, Andrew Cooper wrote: > diff --git a/xen/common/bitops.c b/xen/common/bitops.c > new file mode 100644 > index 000000000000..4c07191b4030 > --- /dev/null > +++ b/xen/common/bitops.c > @@ -0,0 +1,41 @@ > +#include <xen/bitops.h> > +#include <xen/bug.h> > +#include <xen/init.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. > + */ > +#define COMPILE_CHECK(fn, val, res) \ > + do { \ > + if ( fn(val) != res ) \ > + asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \ > + } while ( 0 ) > + For improved diagnostics, I think it might make sense to explicitly check if the expression can be evaluated at compile time and emit a different error if not. Perhaps something like the following: #define COMPILE_CHECK(fn, val, res) \ do { \ __typeof__(fn(val)) actual = fn(val); \ if ( !__builtin_constant_p(actual) ) \ asm (".error \"Unable to evaluate '" STR(fn(val)) "' at compile time\"\n"); \ else if ( actual != res ) \ asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \ } while ( 0 )
On 13/03/2024 5:27 pm, Andrew Cooper wrote: > diff --git a/xen/common/bitops.c b/xen/common/bitops.c > new file mode 100644 > index 000000000000..4c07191b4030 > --- /dev/null > +++ b/xen/common/bitops.c > @@ -0,0 +1,41 @@ > +#include <xen/bitops.h> > +#include <xen/bug.h> > +#include <xen/init.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. > + */ > +#define COMPILE_CHECK(fn, val, res) \ > + do { \ > + if ( fn(val) != res ) \ > + asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \ > + } while ( 0 ) It turns out that Clang doesn't like this https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/6387413632 despite it being capable of reducing the expression to a constant. This also calls into question whether it's a viable replacement for __bad_bitop_size() et al. ~Andrew
On 13.03.2024 18:27, Andrew Cooper wrote: > * Rename __attribute_pure__ to just __pure before it gains users. > * Identify the areas of xen/bitops.h which are a mess. > * Create common/bitops.c for compile and runtime testing. This provides a > statement of the ABI, and a confirmation that arch-specific implementations > behave as expected. If this is the sole purpose of the new file, then ... > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_ARGO) += argo.o > obj-y += bitmap.o > +obj-y += bitops.o obj-bin-y += bitops.init.o please. > --- /dev/null > +++ b/xen/common/bitops.c > @@ -0,0 +1,41 @@ > +#include <xen/bitops.h> > +#include <xen/bug.h> > +#include <xen/init.h> > + > +/* Hide a value from the optimiser. */ > +#define HIDE(x) ({ typeof(x) _x = x; asm volatile ( "" : "+r" (_x) ); _x; }) Irrespective of the question of leading underscores, x wants parenthesizing here. > +/* > + * Check that fn(val) can be calcuated by the compiler, and that it gives the > + * expected answer. > + */ > +#define COMPILE_CHECK(fn, val, res) \ > + do { \ > + if ( fn(val) != res ) \ > + asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \ Nit: Blanks missing immediately inside the outermost pair of parentheses. (As per your own reply it's unclear whether this would actually survive.) > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -85,7 +85,8 @@ > #define inline inline __init > #endif > > -#define __attribute_pure__ __attribute__((__pure__)) > +#define __pure __attribute__((__pure__)) I'd say either there be just a single padding blank or enough to align the rhs with ... > #define __attribute_const__ __attribute__((__const__)) > #define __transparent__ __attribute__((__transparent_union__)) ... these. Jan
diff --git a/xen/common/Makefile b/xen/common/Makefile index e5eee19a8537..1f8ca9a2f4f8 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_ARGO) += argo.o obj-y += bitmap.o +obj-y += bitops.o obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o obj-$(CONFIG_HYPFS_CONFIG) += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o diff --git a/xen/common/bitops.c b/xen/common/bitops.c new file mode 100644 index 000000000000..4c07191b4030 --- /dev/null +++ b/xen/common/bitops.c @@ -0,0 +1,41 @@ +#include <xen/bitops.h> +#include <xen/bug.h> +#include <xen/init.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. + */ +#define COMPILE_CHECK(fn, val, res) \ + do { \ + if ( fn(val) != res ) \ + asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \ + } while ( 0 ) + +/* + * Check that Xen's runtime logic for fn(val) gives the expected answer. This + * requires using HIDE() to prevent the optimiser from emitting the full + * calculation. + */ +#define RUNTIME_CHECK(fn, val, res) \ + do { \ + BUG_ON(fn(HIDE(val)) != 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 ) + +static int __init cf_check test_bitops(void) +{ + return 0; +} +__initcall(test_bitops); 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/compiler.h b/xen/include/xen/compiler.h index 16d554f2a593..972719df55b3 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -85,7 +85,8 @@ #define inline inline __init #endif -#define __attribute_pure__ __attribute__((__pure__)) +#define __pure __attribute__((__pure__)) + #define __attribute_const__ __attribute__((__const__)) #define __transparent__ __attribute__((__transparent_union__))
* Rename __attribute_pure__ to just __pure before it gains users. * Identify the areas of xen/bitops.h which are a mess. * Create common/bitops.c for compile and runtime testing. This provides a statement of the ABI, and a confirmation that arch-specific implementations behave as expected. 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> I expect MISRA will have something to say about the macros here, but they are in aid of better testing. --- xen/common/Makefile | 1 + xen/common/bitops.c | 41 ++++++++++++++++++++++++++++++++++++++ xen/include/xen/bitops.h | 13 +++++++++--- xen/include/xen/compiler.h | 3 ++- 4 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 xen/common/bitops.c