1. Patchew
  2. Xen
  3. [PATCH v2 0/7] Implement byteswap and update references
  4. View patch

[PATCH v2 1/7] xen: implement byteswap.h

Lin Liu posted 7 patches 4 years, 3 months ago
There is a newer version of this series
[PATCH v2 1/7] xen: implement byteswap.h
Posted by Lin Liu 4 years, 3 months ago 
swab() is massively over complicated and can be simplified by builtins.
The compilers provide builtin function to swap bytes.
* gcc:   https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
* clang: https://clang.llvm.org/docs/LanguageExtensions.html
This patch simplify swab() with builtins and fallback for old compilers.

Signed-off-by: Lin Liu <lin.liu@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Changes in v2:
- Add fallback for compilers without __builtin_bswap
- Implement with plain C instead of macros
---
 xen/include/xen/byteswap.h | 93 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/compiler.h | 12 +++++
 2 files changed, 105 insertions(+)
 create mode 100644 xen/include/xen/byteswap.h

diff --git a/xen/include/xen/byteswap.h b/xen/include/xen/byteswap.h
new file mode 100644
index 0000000000..848a4bbaee
--- /dev/null
+++ b/xen/include/xen/byteswap.h
@@ -0,0 +1,93 @@
+#ifndef _BYTESWAP_H
+#define _BYTESWAP_H
+
+#include <xen/types.h>
+
+#if !__has_builtin(__builtin_bswap16)
+static always_inline uint16_t __builtin_bswap16(uint16_t val)
+{
+    return ((val & 0x00FF) << 8) | ((val & 0xFF00) >> 8);
+}
+#endif
+
+#if !__has_builtin(__builtin_bswap32)
+static always_inline uint32_t __builtin_bswap32(uint32_t val)
+{
+    return ((val & 0x000000FF) << 24) |
+           ((val & 0x0000FF00) <<  8) |
+           ((val & 0x00FF0000) >>  8) |
+           ((val & 0xFF000000) >> 24);
+}
+#endif
+
+#if !__has_builtin(__builtin_bswap64)
+static always_inline uint64_t __builtin_bswap64(uint64_t val)
+{
+    return ((val & 0x00000000000000FF) << 56) |
+           ((val & 0x000000000000FF00) << 40) |
+           ((val & 0x0000000000FF0000) << 24) |
+           ((val & 0x00000000FF000000) <<  8) |
+           ((val & 0x000000FF00000000) >>  8) |
+           ((val & 0x0000FF0000000000) >> 24) |
+           ((val & 0x00FF000000000000) >> 40) |
+           ((val & 0xFF00000000000000) >> 56);
+}
+#endif
+
+#define bswap16(x) __builtin_bswap16(x)
+#define bswap32(x) __builtin_bswap32(x)
+#define bswap64(x) __builtin_bswap64(x)
+
+#define bswap_ul(x) bswap##BITS_PER_LONG(x)
+
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+
+#  ifndef __LITTLE_ENDIAN
+#    define __LITTLE_ENDIAN 1234
+#  endif
+
+#  ifndef __LITTLE_ENDIAN_BITFIELD
+#    define __LITTLE_ENDIAN_BITFIELD
+#  endif
+
+#  define cpu_to_le64(x) (x)
+#  define le64_to_cpu(x) (x)
+#  define cpu_to_le32(x) (x)
+#  define le32_to_cpu(x) (x)
+#  define cpu_to_le16(x) (x)
+#  define le16_to_cpu(x) (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 __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+
+#  ifndef __BIG_ENDIAN
+#    define __BIG_ENDIAN 4321
+#  endif
+
+#  ifndef __BIG_ENDIAN_BITFIELD
+#    define __BIG_ENDIAN_BITFIELD
+#  endif
+
+#  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) (x)
+#  define be64_to_cpu(x) (x)
+#  define cpu_to_be32(x) (x)
+#  define be32_to_cpu(x) (x)
+#  define cpu_to_be16(x) (x)
+#  define be16_to_cpu(x) (x)
+
+#else
+#  error "Unknown Endianness"
+#endif /* __BYTE_ORDER__ */
+
+#endif /* _LINUX_BITOPS_H */
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 696c7eb89e..68f28082a5 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -179,4 +179,16 @@
 # define CLANG_DISABLE_WARN_GCC_COMPAT_END
 #endif
 
+#if (!defined(__clang__) && (__GNUC__ < 10))
+/*
+ * Backwards compatibility for GCC < 10.
+ * All supported versions of Clang support __has_builtin
+ * */
+#define __has_builtin(x) GCC_has ## x
+
+#define GCC_has__builtin_bswap16 (CONFIG_GCC_VERSION >= 40800)
+#define GCC_has__builtin_bswap32 (CONFIG_GCC_VERSION >= 40400)
+#define GCC_has__builtin_bswap64 (CONFIG_GCC_VERSION >= 40400)
+#endif
+
 #endif /* __LINUX_COMPILER_H */
-- 
2.27.0
Re: [PATCH v2 1/7] xen: implement byteswap.h
Posted by Andrew Cooper 4 years, 3 months ago 
On 22/10/2021 11:47, Lin Liu wrote:
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index 696c7eb89e..68f28082a5 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -179,4 +179,16 @@
>  # define CLANG_DISABLE_WARN_GCC_COMPAT_END
>  #endif
>  
> +#if (!defined(__clang__) && (__GNUC__ < 10))

This too lost my feedback.

It needs to be #ifndef __has_builtin because otherwise a random GCC 10
build I have fails with:

/local/security/xen.git/xen/include/xen/byteswap.h:6:19: error: missing
binary operator before token "("
    6 | #if !__has_builtin(__builtin_bswap16)


I suspect it was a build of GCC 10 before __has_builtin() support was
added, but either way, we should be predicating on __has_builtin itself,
not version guesswork.

~Andrew
Re: [PATCH v2 1/7] xen: implement byteswap.h
Posted by Jan Beulich 4 years, 3 months ago 
On 22.10.2021 12:47, Lin Liu wrote:
> --- /dev/null
> +++ b/xen/include/xen/byteswap.h
> @@ -0,0 +1,93 @@
> +#ifndef _BYTESWAP_H
> +#define _BYTESWAP_H
> +
> +#include <xen/types.h>
> +
> +#if !__has_builtin(__builtin_bswap16)
> +static always_inline uint16_t __builtin_bswap16(uint16_t val)
> +{
> +    return ((val & 0x00FF) << 8) | ((val & 0xFF00) >> 8);
> +}
> +#endif
> +
> +#if !__has_builtin(__builtin_bswap32)
> +static always_inline uint32_t __builtin_bswap32(uint32_t val)
> +{
> +    return ((val & 0x000000FF) << 24) |
> +           ((val & 0x0000FF00) <<  8) |
> +           ((val & 0x00FF0000) >>  8) |
> +           ((val & 0xFF000000) >> 24);
> +}
> +#endif
> +
> +#if !__has_builtin(__builtin_bswap64)
> +static always_inline uint64_t __builtin_bswap64(uint64_t val)
> +{
> +    return ((val & 0x00000000000000FF) << 56) |
> +           ((val & 0x000000000000FF00) << 40) |
> +           ((val & 0x0000000000FF0000) << 24) |
> +           ((val & 0x00000000FF000000) <<  8) |
> +           ((val & 0x000000FF00000000) >>  8) |
> +           ((val & 0x0000FF0000000000) >> 24) |
> +           ((val & 0x00FF000000000000) >> 40) |
> +           ((val & 0xFF00000000000000) >> 56);
> +}
> +#endif
> +
> +#define bswap16(x) __builtin_bswap16(x)
> +#define bswap32(x) __builtin_bswap32(x)
> +#define bswap64(x) __builtin_bswap64(x)
> +
> +#define bswap_ul(x) bswap##BITS_PER_LONG(x)

I don't see how this is supposed to work - the compiler isn't going to
expand BITS_PER_LONG before the token concatenation. You'll need helper
macros to achieve that. Linux has __PASTE(); we may want to "steal" that
(albeit preferably without any leading underscores).

> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

While you've worked towards abstracting older vs newer compiler
versions, I'm afraid these constants aren't available in gcc 4.1 yet.
They look to have appeared in 4.6.

> +#  ifndef __LITTLE_ENDIAN
> +#    define __LITTLE_ENDIAN 1234
> +#  endif
> +
> +#  ifndef __LITTLE_ENDIAN_BITFIELD
> +#    define __LITTLE_ENDIAN_BITFIELD
> +#  endif

These are definitions which I don't think belong into a header of this
name. They're imo well placed in byteorder/*.h.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.