Re: [PATCH 1/7] xen/bitops: Cleanup ahead of rearrangements

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Oleksii Kurochko" <oleksii.kurochko@gmail.com>,
	"Shawn Anastasio" <sanastasio@raptorengineering.com>,
	"consulting @ bugseng . com" <consulting@bugseng.com>,
	"Simone Ballarin" <simone.ballarin@bugseng.com>,
	"Federico Serafini" <federico.serafini@bugseng.com>,
	"Nicola Vetrini" <nicola.vetrini@bugseng.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/7] xen/bitops: Cleanup ahead of rearrangements
Date: Thu, 14 Mar 2024 14:59:13 +0100	[thread overview]
Message-ID: <141bc66c-79f4-474e-8421-34f9aaa2d47c@suse.com> (raw)
In-Reply-To: <20240313172716.2325427-2-andrew.cooper3@citrix.com>

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