[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.