| Message ID | 2699787cd4ba1d71448bbcdf190d927e180e80b9.1652170719.git.lin.liu@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement byteswap and update references | expand |
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
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,
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,
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 */
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