Re: [PATCH v3 1/6] xen: implement byteswap

[Julien Grall <julien@xen.org>]

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