Re: [PATCH v2 02/13] xen/bitops: Cleanup ahead of rearrangements

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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