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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx>
> > ---
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Julien Grall <julien@xxxxxxx>
> > Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > 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