* 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