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: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
Cc: Bertrand Marquis <bertrand.marquis@arm.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Changes in v3:
- Check __has_builtin instead of GNUC version
Changes in v2:
- Add fallback for compilers without __builtin_bswap
- Implement with plain C instead of macros
---
xen/arch/arm/include/asm/byteorder.h | 14 ++-----
xen/arch/x86/include/asm/byteorder.h | 34 ++---------------
xen/include/xen/byteorder.h | 56 ++++++++++++++++++++++++++++
xen/include/xen/byteswap.h | 44 ++++++++++++++++++++++
xen/include/xen/compiler.h | 12 ++++++
5 files changed, 120 insertions(+), 40 deletions(-)
create mode 100644 xen/include/xen/byteorder.h
create mode 100644 xen/include/xen/byteswap.h
diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h
index 9c712c4788..622eeaba07 100644
--- a/xen/arch/arm/include/asm/byteorder.h
+++ b/xen/arch/arm/include/asm/byteorder.h
@@ -1,16 +1,10 @@
#ifndef __ASM_ARM_BYTEORDER_H__
#define __ASM_ARM_BYTEORDER_H__
-#define __BYTEORDER_HAS_U64__
+#ifndef __BYTE_ORDER__
+ #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
+#endif
-#include <xen/byteorder/little_endian.h>
+#include <xen/byteorder.h>
#endif /* __ASM_ARM_BYTEORDER_H__ */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/include/asm/byteorder.h b/xen/arch/x86/include/asm/byteorder.h
index 1f77e502a5..82aadee7bd 100644
--- a/xen/arch/x86/include/asm/byteorder.h
+++ b/xen/arch/x86/include/asm/byteorder.h
@@ -1,36 +1,10 @@
#ifndef __ASM_X86_BYTEORDER_H__
#define __ASM_X86_BYTEORDER_H__
-#include <asm/types.h>
-#include <xen/compiler.h>
+#ifndef __BYTE_ORDER__
+ #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
+#endif
-static inline __attribute_const__ __u32 ___arch__swab32(__u32 x)
-{
- asm("bswap %0" : "=r" (x) : "0" (x));
- return x;
-}
-
-static inline __attribute_const__ __u64 ___arch__swab64(__u64 val)
-{
- union {
- struct { __u32 a,b; } s;
- __u64 u;
- } v;
- v.u = val;
- asm("bswapl %0 ; bswapl %1 ; xchgl %0,%1"
- : "=r" (v.s.a), "=r" (v.s.b)
- : "0" (v.s.a), "1" (v.s.b));
- return v.u;
-}
-
-/* Do not define swab16. Gcc is smart enough to recognize "C" version and
- convert it into rotation or exhange. */
-
-#define __arch__swab64(x) ___arch__swab64(x)
-#define __arch__swab32(x) ___arch__swab32(x)
-
-#define __BYTEORDER_HAS_U64__
-
-#include <xen/byteorder/little_endian.h>
+#include <xen/byteorder.h>
#endif /* __ASM_X86_BYTEORDER_H__ */
diff --git a/xen/include/xen/byteorder.h b/xen/include/xen/byteorder.h
new file mode 100644
index 0000000000..2ec434e6a6
--- /dev/null
+++ b/xen/include/xen/byteorder.h
@@ -0,0 +1,56 @@
+#ifndef __XEN_BYTEORDER_H__
+#define __XEN_BYTEORDER_H__
+
+#include <xen/byteswap.h>
+
+#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 /* __XEN_BYTEORDER_H__ */
diff --git a/xen/include/xen/byteswap.h b/xen/include/xen/byteswap.h
new file mode 100644
index 0000000000..0dd5567557
--- /dev/null
+++ b/xen/include/xen/byteswap.h
@@ -0,0 +1,44 @@
+#ifndef __XEN_BYTESWAP_H__
+#define __XEN_BYTESWAP_H__
+
+#include <xen/types.h>
+#include <xen/lib.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) PASTE(bswap,BITS_PER_LONG)(x)
+
+#endif /* __XEN_BYTESWAP_H__ */
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 933aec09a9..05b1b8b24d 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -185,4 +185,16 @@
# define CLANG_DISABLE_WARN_GCC_COMPAT_END
#endif
+#ifndef __has_builtin
+/*
+ * 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
Hi, On 10/05/2022 11:15, Lin Liu wrote: > swab() is massively over complicated and can be simplified by builtins. NIT: "by builtins" -> "by re-implementing using compiler 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: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien@xen.org> > Cc: Bertrand Marquis <bertrand.marquis@arm.com> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Changes in v3: > - Check __has_builtin instead of GNUC version > > Changes in v2: > - Add fallback for compilers without __builtin_bswap > - Implement with plain C instead of macros > --- > xen/arch/arm/include/asm/byteorder.h | 14 ++----- > xen/arch/x86/include/asm/byteorder.h | 34 ++--------------- > xen/include/xen/byteorder.h | 56 ++++++++++++++++++++++++++++ > xen/include/xen/byteswap.h | 44 ++++++++++++++++++++++ > xen/include/xen/compiler.h | 12 ++++++ > 5 files changed, 120 insertions(+), 40 deletions(-) > create mode 100644 xen/include/xen/byteorder.h > create mode 100644 xen/include/xen/byteswap.h > > diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h > index 9c712c4788..622eeaba07 100644 > --- a/xen/arch/arm/include/asm/byteorder.h > +++ b/xen/arch/arm/include/asm/byteorder.h > @@ -1,16 +1,10 @@ > #ifndef __ASM_ARM_BYTEORDER_H__ > #define __ASM_ARM_BYTEORDER_H__ > > -#define __BYTEORDER_HAS_U64__ > +#ifndef __BYTE_ORDER__ > + #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__ > +#endif > > -#include <xen/byteorder/little_endian.h> > +#include <xen/byteorder.h> > > #endif /* __ASM_ARM_BYTEORDER_H__ */ > -/* > - * Local variables: > - * mode: C > - * c-file-style: "BSD" > - * c-basic-offset: 4 > - * indent-tabs-mode: nil > - * End: > - */ This change looks unrelated to this patch. Can you explain it? Cheers, -- Julien Grall
Hi, On 10/05/2022 11:15, Lin Liu wrote: > swab() is massively over complicated and can be simplified by builtins. NIT: "by builtins" -> "by re-implementing using compiler builtins". > The compilers provide builtin function to swap bytes. > * gcc: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FOther-Builtins.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HDTF1LDJcD2PLSCuM%2FjIz%2FWGf1CrYk0e%2FLox22%2FXnvQ%3D&reserved=0 > * clang: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fclang.llvm.org%2Fdocs%2FLanguageExtensions.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EvWcLMi2vtT9haQVo%2F9uXmjBh2zVLUzZAgU57i%2FFMNo%3D&reserved=0 > This patch simplify swab() with builtins and fallback for old compilers. > > Signed-off-by: Lin Liu <lin.liu@citrix.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien@xen.org> > Cc: Bertrand Marquis <bertrand.marquis@arm.com> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Changes in v3: > - Check __has_builtin instead of GNUC version > > Changes in v2: > - Add fallback for compilers without __builtin_bswap > - Implement with plain C instead of macros > --- > xen/arch/arm/include/asm/byteorder.h | 14 ++----- > xen/arch/x86/include/asm/byteorder.h | 34 ++--------------- > xen/include/xen/byteorder.h | 56 ++++++++++++++++++++++++++++ > xen/include/xen/byteswap.h | 44 ++++++++++++++++++++++ > xen/include/xen/compiler.h | 12 ++++++ > 5 files changed, 120 insertions(+), 40 deletions(-) > create mode 100644 xen/include/xen/byteorder.h > create mode 100644 xen/include/xen/byteswap.h > > diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h > index 9c712c4788..622eeaba07 100644 > --- a/xen/arch/arm/include/asm/byteorder.h > +++ b/xen/arch/arm/include/asm/byteorder.h > @@ -1,16 +1,10 @@ > #ifndef __ASM_ARM_BYTEORDER_H__ > #define __ASM_ARM_BYTEORDER_H__ > > -#define __BYTEORDER_HAS_U64__ > +#ifndef __BYTE_ORDER__ > + #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__ > +#endif > > -#include <xen/byteorder/little_endian.h> > +#include <xen/byteorder.h> > > #endif /* __ASM_ARM_BYTEORDER_H__ */ > -/* > - * Local variables: > - * mode: C > - * c-file-style: "BSD" > - * c-basic-offset: 4 > - * indent-tabs-mode: nil > - * End: > - */ >> This change looks unrelated to this patch. Can you explain it? Do you mean following code block? Yes, it is unrelated, I removed it as I found some files does not include such field. Will revert such field in V4. > -/* > - * Local variables: > - * mode: C > - * c-file-style: "BSD" > - * c-basic-offset: 4 > - * indent-tabs-mode: nil > - * End: > - */
On 10/05/2022 13:10, Lin Liu （刘林） wrote: > On 10/05/2022 11:15, Lin Liu wrote: >> swab() is massively over complicated and can be simplified by builtins. > > NIT: "by builtins" -> "by re-implementing using compiler builtins". > >> The compilers provide builtin function to swap bytes. >> * gcc: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FOther-Builtins.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HDTF1LDJcD2PLSCuM%2FjIz%2FWGf1CrYk0e%2FLox22%2FXnvQ%3D&reserved=0 >> * clang: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fclang.llvm.org%2Fdocs%2FLanguageExtensions.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EvWcLMi2vtT9haQVo%2F9uXmjBh2zVLUzZAgU57i%2FFMNo%3D&reserved=0 >> This patch simplify swab() with builtins and fallback for old compilers. >> >> Signed-off-by: Lin Liu <lin.liu@citrix.com> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien@xen.org> >> Cc: Bertrand Marquis <bertrand.marquis@arm.com> >> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: George Dunlap <george.dunlap@citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Wei Liu <wl@xen.org> >> Cc: "Roger Pau Monné" <roger.pau@citrix.com> >> Changes in v3: >> - Check __has_builtin instead of GNUC version >> >> Changes in v2: >> - Add fallback for compilers without __builtin_bswap >> - Implement with plain C instead of macros >> --- >> xen/arch/arm/include/asm/byteorder.h | 14 ++----- >> xen/arch/x86/include/asm/byteorder.h | 34 ++--------------- >> xen/include/xen/byteorder.h | 56 ++++++++++++++++++++++++++++ >> xen/include/xen/byteswap.h | 44 ++++++++++++++++++++++ >> xen/include/xen/compiler.h | 12 ++++++ >> 5 files changed, 120 insertions(+), 40 deletions(-) >> create mode 100644 xen/include/xen/byteorder.h >> create mode 100644 xen/include/xen/byteswap.h >> >> diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h >> index 9c712c4788..622eeaba07 100644 >> --- a/xen/arch/arm/include/asm/byteorder.h >> +++ b/xen/arch/arm/include/asm/byteorder.h >> @@ -1,16 +1,10 @@ >> #ifndef __ASM_ARM_BYTEORDER_H__ >> #define __ASM_ARM_BYTEORDER_H__ >> >> -#define __BYTEORDER_HAS_U64__ >> +#ifndef __BYTE_ORDER__ >> + #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__ >> +#endif >> >> -#include <xen/byteorder/little_endian.h> >> +#include <xen/byteorder.h> >> >> #endif /* __ASM_ARM_BYTEORDER_H__ */ >> -/* >> - * Local variables: >> - * mode: C >> - * c-file-style: "BSD" >> - * c-basic-offset: 4 >> - * indent-tabs-mode: nil >> - * End: >> - */ > >>> This change looks unrelated to this patch. Can you explain it? > > Do you mean following code block? Yes, it is unrelated, I removed it as I found some files does not include such field. So in general we try to avoid unrelated change within a same patch. In this case, the emacs magic should be present in the files rather than the other way around. Cheers, -- Julien Grall
On 10/05/2022 11:15, Lin Liu wrote: > 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. Arguably, this patch introduces a new byteswapping infrastructure in terms of compiler builtins and bswapXX(), so the swab() infrastructure can be retired. > diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h > index 9c712c4788..622eeaba07 100644 > --- a/xen/arch/arm/include/asm/byteorder.h > +++ b/xen/arch/arm/include/asm/byteorder.h > @@ -1,16 +1,10 @@ > #ifndef __ASM_ARM_BYTEORDER_H__ > #define __ASM_ARM_BYTEORDER_H__ > > -#define __BYTEORDER_HAS_U64__ > +#ifndef __BYTE_ORDER__ > + #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__ > +#endif This won't actually do what you want on GCC 4.5 or older. You also want #ifndef __ORDER_LITTLE_ENDIAN__ # define __ORDER_LITTLE_ENDIAN__ 1234 #endif #ifndef __ORDER_BIG_ENDIAN__ # define __ORDER_BIG_ENDIAN__ 4321 #endif in compiler.h to cope with older GCC. Otherwise, LGTM. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> I can fix this on commit if its the only issue issue. Otherwise, please correct it when posting v4. ~Andrew
© 2016 - 2026 Red Hat, Inc.