diff mbox series

[v5,03/16] xen: Implement common byte{order,swap}.h

Message ID 20250328134427.874848-4-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series xen: Centralise byteswap infrastructure | expand

Commit Message

Andrew Cooper March 28, 2025, 1:44 p.m. UTC 
From: Lin Liu <lin.liu@citrix.com>

The current swab??() infrastructure is unecesserily complicated, and can be
repated entirely with compiler builtins.

All supported compilers provide __BYTE_ORDER__ and __builtin_bswap??().

Nothing in Xen cares about the values of __{BIG,LITTLE}_ENDIAN; just that one
of them is defined.  Therefore, centralise their definitions in xen/config.h

Signed-off-by: Lin Liu <lin.liu@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Lin Liu <lin.liu@citrix.com>

v5:
 * Rebase substantially
 * Drop PASTE().  It doesn't work when BITS_PER_LONG isn't a plain integer
 * Simplify in light of new toolchain baseline
---
 xen/include/xen/byteorder.h               | 44 +++++++++++++++++++++++
 xen/include/xen/byteorder/big_endian.h    |  4 ---
 xen/include/xen/byteorder/little_endian.h |  4 ---
 xen/include/xen/byteswap.h                | 15 ++++++++
 xen/include/xen/config.h                  |  6 ++++
 5 files changed, 65 insertions(+), 8 deletions(-)
 create mode 100644 xen/include/xen/byteorder.h
 create mode 100644 xen/include/xen/byteswap.h

Comments

Jan Beulich March 31, 2025, 8:32 a.m. UTC | #1 
On 28.03.2025 14:44, Andrew Cooper wrote:
> From: Lin Liu <lin.liu@citrix.com>
> 
> The current swab??() infrastructure is unecesserily complicated, and can be
> repated entirely with compiler builtins.
> 
> All supported compilers provide __BYTE_ORDER__ and __builtin_bswap??().
> 
> Nothing in Xen cares about the values of __{BIG,LITTLE}_ENDIAN; just that one
> of them is defined.  Therefore, centralise their definitions in xen/config.h

And even if we cared somewhere, __ORDER_{BIG,LITTLE}_ENDIAN__ supply them
just fine.

> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two nits taken care of:

> --- /dev/null
> +++ b/xen/include/xen/byteorder.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef XEN_BYTEORDER_H
> +#define XEN_BYTEORDER_H
> +
> +#include <xen/byteswap.h>
> +#include <xen/types.h>

It's stdint.h that's needed here, not types.h?

> +#if defined(__LITTLE_ENDIAN)
> +
> +# define cpu_to_le64(x) (uint64_t)(x)
> +# define le64_to_cpu(x) (uint64_t)(x)
> +# define cpu_to_le32(x) (uint32_t)(x)
> +# define le32_to_cpu(x) (uint32_t)(x)
> +# define cpu_to_le16(x) (uint16_t)(x)
> +# define le16_to_cpu(x) (uint16_t)(x)

(Not just) for Misra these all need another pair of parentheses around the
entire expressions.

> +# define cpu_to_be64(x) bswap64(x)
> +# define be64_to_cpu(x) bswap64(x)
> +# define cpu_to_be32(x) bswap32(x)
> +# define be32_to_cpu(x) bswap32(x)
> +# define cpu_to_be16(x) bswap16(x)
> +# define be16_to_cpu(x) bswap16(x)
> +
> +#elif defined(__BIG_ENDIAN)
> +
> +# define cpu_to_le64(x) bswap64(x)
> +# define le64_to_cpu(x) bswap64(x)
> +# define cpu_to_le32(x) bswap32(x)
> +# define le32_to_cpu(x) bswap32(x)
> +# define cpu_to_le16(x) bswap16(x)
> +# define le16_to_cpu(x) bswap16(x)
> +
> +# define cpu_to_be64(x) (uint64_t)(x)
> +# define be64_to_cpu(x) (uint64_t)(x)
> +# define cpu_to_be32(x) (uint32_t)(x)
> +# define be32_to_cpu(x) (uint32_t)(x)
> +# define cpu_to_be16(x) (uint16_t)(x)
> +# define be16_to_cpu(x) (uint16_t)(x)

Same here, even if Eclair likely wouldn't spot this right now.

Jan
Andrew Cooper March 31, 2025, 1:59 p.m. UTC | #2 
On 31/03/2025 9:32 am, Jan Beulich wrote:
> On 28.03.2025 14:44, Andrew Cooper wrote:
>> From: Lin Liu <lin.liu@citrix.com>
>>
>> The current swab??() infrastructure is unecesserily complicated, and can be
>> repated entirely with compiler builtins.
>>
>> All supported compilers provide __BYTE_ORDER__ and __builtin_bswap??().
>>
>> Nothing in Xen cares about the values of __{BIG,LITTLE}_ENDIAN; just that one
>> of them is defined.  Therefore, centralise their definitions in xen/config.h
> And even if we cared somewhere, __ORDER_{BIG,LITTLE}_ENDIAN__ supply them
> just fine.
>
>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two nits taken care of:
>
>> --- /dev/null
>> +++ b/xen/include/xen/byteorder.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef XEN_BYTEORDER_H
>> +#define XEN_BYTEORDER_H
>> +
>> +#include <xen/byteswap.h>
>> +#include <xen/types.h>
> It's stdint.h that's needed here, not types.h?

Perhaps.

>
>> +#if defined(__LITTLE_ENDIAN)
>> +
>> +# define cpu_to_le64(x) (uint64_t)(x)
>> +# define le64_to_cpu(x) (uint64_t)(x)
>> +# define cpu_to_le32(x) (uint32_t)(x)
>> +# define le32_to_cpu(x) (uint32_t)(x)
>> +# define cpu_to_le16(x) (uint16_t)(x)
>> +# define le16_to_cpu(x) (uint16_t)(x)
> (Not just) for Misra these all need another pair of parentheses around the
> entire expressions.

Why?  For both points?  Eclair is happy, with this at least.

What does fail is a bit a of a curveball.

https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/andyhhp/xen/ECLAIR_normal/xen-bswap/ARM64/9556392204/PROJECT.ecd;/by_service/MC3A2.R20.6.html

cpu_to_le64() turns from a real function to a macro under ARM, meaning
that the #ifdef __BIG_ENDIAN in the middle of a bunch of constants
becomes UB.

I'll need to fix that separately.

~Andrew
Jan Beulich March 31, 2025, 2:08 p.m. UTC | #3 
On 31.03.2025 15:59, Andrew Cooper wrote:
> On 31/03/2025 9:32 am, Jan Beulich wrote:
>> On 28.03.2025 14:44, Andrew Cooper wrote:
>>> +#if defined(__LITTLE_ENDIAN)
>>> +
>>> +# define cpu_to_le64(x) (uint64_t)(x)
>>> +# define le64_to_cpu(x) (uint64_t)(x)
>>> +# define cpu_to_le32(x) (uint32_t)(x)
>>> +# define le32_to_cpu(x) (uint32_t)(x)
>>> +# define cpu_to_le16(x) (uint16_t)(x)
>>> +# define le16_to_cpu(x) (uint16_t)(x)
>> (Not just) for Misra these all need another pair of parentheses around the
>> entire expressions.
> 
> Why?  For both points?  Eclair is happy, with this at least.

As was explained to us, Eclair looks at the final, expanded code. Hence it may
well be that right now, in the code Eclair checks, we have no violations. Ones
may start to appear elsewhere or later, though. The fix then would still be to
get these macros into fully parenthesized shape.

That said, I can't think of any "good" code needing these parentheses here. I
can think of bad code though, which would properly fail to compile if the
parentheses were added, e.g.

    le32_to_cpu(a)++;

I fear I can't answer the 2nd question - what do you mean by "For both points?"?

Jan
diff mbox series

Patch

diff --git a/xen/include/xen/byteorder.h b/xen/include/xen/byteorder.h
new file mode 100644
index 000000000000..570a7fe027e5
--- /dev/null
+++ b/xen/include/xen/byteorder.h
@@ -0,0 +1,44 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef XEN_BYTEORDER_H
+#define XEN_BYTEORDER_H
+
+#include <xen/byteswap.h>
+#include <xen/types.h>
+
+#if defined(__LITTLE_ENDIAN)
+
+# define cpu_to_le64(x) (uint64_t)(x)
+# define le64_to_cpu(x) (uint64_t)(x)
+# define cpu_to_le32(x) (uint32_t)(x)
+# define le32_to_cpu(x) (uint32_t)(x)
+# define cpu_to_le16(x) (uint16_t)(x)
+# define le16_to_cpu(x) (uint16_t)(x)
+
+# define cpu_to_be64(x) bswap64(x)
+# define be64_to_cpu(x) bswap64(x)
+# define cpu_to_be32(x) bswap32(x)
+# define be32_to_cpu(x) bswap32(x)
+# define cpu_to_be16(x) bswap16(x)
+# define be16_to_cpu(x) bswap16(x)
+
+#elif defined(__BIG_ENDIAN)
+
+# define cpu_to_le64(x) bswap64(x)
+# define le64_to_cpu(x) bswap64(x)
+# define cpu_to_le32(x) bswap32(x)
+# define le32_to_cpu(x) bswap32(x)
+# define cpu_to_le16(x) bswap16(x)
+# define le16_to_cpu(x) bswap16(x)
+
+# define cpu_to_be64(x) (uint64_t)(x)
+# define be64_to_cpu(x) (uint64_t)(x)
+# define cpu_to_be32(x) (uint32_t)(x)
+# define be32_to_cpu(x) (uint32_t)(x)
+# define cpu_to_be16(x) (uint16_t)(x)
+# define be16_to_cpu(x) (uint16_t)(x)
+
+#else
+# error Unknown Endianness
+#endif /* __*_ENDIAN */
+
+#endif /* XEN_BYTEORDER_H */
diff --git a/xen/include/xen/byteorder/big_endian.h b/xen/include/xen/byteorder/big_endian.h
index 9cfb567d51d5..512291c76f1b 100644
--- a/xen/include/xen/byteorder/big_endian.h
+++ b/xen/include/xen/byteorder/big_endian.h
@@ -1,10 +1,6 @@ 
 #ifndef __XEN_BYTEORDER_BIG_ENDIAN_H__
 #define __XEN_BYTEORDER_BIG_ENDIAN_H__
 
-#ifndef __BIG_ENDIAN
-#define __BIG_ENDIAN 4321
-#endif
-
 #include <xen/types.h>
 #include <xen/byteorder/swab.h>
 
diff --git a/xen/include/xen/byteorder/little_endian.h b/xen/include/xen/byteorder/little_endian.h
index 96c80eab2b12..bd1afc6a67c3 100644
--- a/xen/include/xen/byteorder/little_endian.h
+++ b/xen/include/xen/byteorder/little_endian.h
@@ -1,10 +1,6 @@ 
 #ifndef __XEN_BYTEORDER_LITTLE_ENDIAN_H__
 #define __XEN_BYTEORDER_LITTLE_ENDIAN_H__
 
-#ifndef __LITTLE_ENDIAN
-#define __LITTLE_ENDIAN 1234
-#endif
-
 #include <xen/types.h>
 #include <xen/byteorder/swab.h>
 
diff --git a/xen/include/xen/byteswap.h b/xen/include/xen/byteswap.h
new file mode 100644
index 000000000000..46d93f88eac9
--- /dev/null
+++ b/xen/include/xen/byteswap.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef XEN_BYTESWAP_H
+#define XEN_BYTESWAP_H
+
+#define bswap16(x) __builtin_bswap16(x)
+#define bswap32(x) __builtin_bswap32(x)
+#define bswap64(x) __builtin_bswap64(x)
+
+#if BITS_PER_LONG == 64
+# define bswapl(x) bswap64(x)
+#elif BITS_PER_LONG == 32
+# define bswapl(x) bswap32(x)
+#endif
+
+#endif /* XEN_BYTESWAP_H */
diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
index d888b2314daf..6815a0ef0c1a 100644
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -98,4 +98,10 @@ 
 #define ZERO_BLOCK_PTR ((void *)-1L)
 #endif
 
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+# define __LITTLE_ENDIAN
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+# define __BIG_ENDIAN
+#endif
+
 #endif /* __XEN_CONFIG_H__ */