* 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
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__))
--
2.30.2
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
© 2016 - 2026 Red Hat, Inc.