* [PATCH v3 1/6] xen: implement byteswap
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-10 10:50 ` Andrew Cooper
2022-05-10 11:10 ` Julien Grall
2022-05-10 10:15 ` [PATCH v3 2/6] crypto/vmac: Simplify code with byteswap Lin Liu
` (4 subsequent siblings)
5 siblings, 2 replies; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel
Cc: Lin Liu, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 1/6] xen: implement byteswap
2022-05-10 10:15 ` [PATCH v3 1/6] xen: implement byteswap Lin Liu
@ 2022-05-10 10:50 ` Andrew Cooper
2022-05-10 11:10 ` Julien Grall
1 sibling, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2022-05-10 10:50 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/6] xen: implement byteswap
2022-05-10 10:15 ` [PATCH v3 1/6] xen: implement byteswap Lin Liu
2022-05-10 10:50 ` Andrew Cooper
@ 2022-05-10 11:10 ` Julien Grall
2022-05-10 12:10 ` Lin Liu （刘林）
1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2022-05-10 11:10 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/6] xen: implement byteswap
2022-05-10 11:10 ` Julien Grall
@ 2022-05-10 12:10 ` Lin Liu （刘林）
2022-05-10 16:18 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Lin Liu （刘林） @ 2022-05-10 12:10 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monne
[-- Attachment #1: Type: text/plain, Size: 3461 bytes --]
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:
> - */
[-- Attachment #2: Type: text/html, Size: 7216 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/6] xen: implement byteswap
2022-05-10 12:10 ` Lin Liu （刘林）
@ 2022-05-10 16:18 ` Julien Grall
0 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-10 16:18 UTC (permalink / raw)
To: Lin Liu （刘林）, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 2/6] crypto/vmac: Simplify code with byteswap
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
2022-05-10 10:15 ` [PATCH v3 1/6] xen: implement byteswap Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-10 10:51 ` Andrew Cooper
2022-05-10 10:15 ` [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab() Lin Liu
` (3 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel
Cc: Lin Liu, Jan Beulich, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
This file has its own implementation of swap bytes. Clean up
the code with xen/byteswap.h.
No functional change.
Signed-off-by: Lin Liu <lin.liu@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
---
xen/crypto/vmac.c | 76 ++---------------------------------------------
1 file changed, 3 insertions(+), 73 deletions(-)
diff --git a/xen/crypto/vmac.c b/xen/crypto/vmac.c
index 294dd16a52..acb4e015f5 100644
--- a/xen/crypto/vmac.c
+++ b/xen/crypto/vmac.c
@@ -8,6 +8,7 @@
/* start for Xen */
#include <xen/init.h>
+#include <xen/byteswap.h>
#include <xen/types.h>
#include <xen/lib.h>
#include <crypto/vmac.h>
@@ -50,7 +51,6 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
* MUL64: 64x64->128-bit multiplication
* PMUL64: assumes top bits cleared on inputs
* ADD128: 128x128->128-bit addition
- * GET_REVERSED_64: load and byte-reverse 64-bit word
* ----------------------------------------------------------------------- */
/* ----------------------------------------------------------------------- */
@@ -68,22 +68,6 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
#define PMUL64 MUL64
-#define GET_REVERSED_64(p) \
- ({uint64_t x; \
- asm ("bswapq %0" : "=r" (x) : "0"(*(uint64_t *)(p))); x;})
-
-/* ----------------------------------------------------------------------- */
-#elif (__GNUC__ && __i386__)
-/* ----------------------------------------------------------------------- */
-
-#define GET_REVERSED_64(p) \
- ({ uint64_t x; \
- uint32_t *tp = (uint32_t *)(p); \
- asm ("bswap %%edx\n\t" \
- "bswap %%eax" \
- : "=A"(x) \
- : "a"(tp[1]), "d"(tp[0])); \
- x; })
/* ----------------------------------------------------------------------- */
#elif (__GNUC__ && __ppc64__)
@@ -103,37 +87,6 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
#define PMUL64 MUL64
-#define GET_REVERSED_64(p) \
- ({ uint32_t hi, lo, *_p = (uint32_t *)(p); \
- asm volatile ("lwbrx %0, %1, %2" : "=r"(lo) : "b%"(0), "r"(_p) ); \
- asm volatile ("lwbrx %0, %1, %2" : "=r"(hi) : "b%"(4), "r"(_p) ); \
- ((uint64_t)hi << 32) | (uint64_t)lo; } )
-
-/* ----------------------------------------------------------------------- */
-#elif (__GNUC__ && (__ppc__ || __PPC__))
-/* ----------------------------------------------------------------------- */
-
-#define GET_REVERSED_64(p) \
- ({ uint32_t hi, lo, *_p = (uint32_t *)(p); \
- asm volatile ("lwbrx %0, %1, %2" : "=r"(lo) : "b%"(0), "r"(_p) ); \
- asm volatile ("lwbrx %0, %1, %2" : "=r"(hi) : "b%"(4), "r"(_p) ); \
- ((uint64_t)hi << 32) | (uint64_t)lo; } )
-
-/* ----------------------------------------------------------------------- */
-#elif (__GNUC__ && (__ARMEL__ || __ARM__))
-/* ----------------------------------------------------------------------- */
-
-#define bswap32(v) \
-({ uint32_t tmp,out; \
- asm volatile( \
- "eor %1, %2, %2, ror #16\n" \
- "bic %1, %1, #0x00ff0000\n" \
- "mov %0, %2, ror #8\n" \
- "eor %0, %0, %1, lsr #8" \
- : "=r" (out), "=&r" (tmp) \
- : "r" (v)); \
- out;})
-
/* ----------------------------------------------------------------------- */
#elif _MSC_VER
/* ----------------------------------------------------------------------- */
@@ -154,11 +107,6 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
(rh) += (ih) + ((rl) < (_il)); \
}
-#if _MSC_VER >= 1300
-#define GET_REVERSED_64(p) _byteswap_uint64(*(uint64_t *)(p))
-#pragma intrinsic(_byteswap_uint64)
-#endif
-
#if _MSC_VER >= 1400 && \
(!defined(__INTEL_COMPILER) || __INTEL_COMPILER >= 1000)
#define MUL32(i1,i2) (__emulu((uint32_t)(i1),(uint32_t)(i2)))
@@ -219,24 +167,6 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
}
#endif
-#ifndef GET_REVERSED_64
-#ifndef bswap64
-#ifndef bswap32
-#define bswap32(x) \
- ({ uint32_t bsx = (x); \
- ((((bsx) & 0xff000000u) >> 24) | (((bsx) & 0x00ff0000u) >> 8) | \
- (((bsx) & 0x0000ff00u) << 8) | (((bsx) & 0x000000ffu) << 24)); })
-#endif
-#define bswap64(x) \
- ({ union { uint64_t ll; uint32_t l[2]; } w, r; \
- w.ll = (x); \
- r.l[0] = bswap32 (w.l[1]); \
- r.l[1] = bswap32 (w.l[0]); \
- r.ll; })
-#endif
-#define GET_REVERSED_64(p) bswap64(*(uint64_t *)(p))
-#endif
-
/* ----------------------------------------------------------------------- */
#if (VMAC_PREFER_BIG_ENDIAN)
@@ -247,9 +177,9 @@ const uint64_t mpoly = UINT64_C(0x1fffffff1fffffff); /* Poly key mask */
#if (VMAC_ARCH_BIG_ENDIAN)
# define get64BE(ptr) (*(uint64_t *)(ptr))
-# define get64LE(ptr) GET_REVERSED_64(ptr)
+# define get64LE(ptr) bswap64(*(uint64_t *)(ptr))
#else /* assume little-endian */
-# define get64BE(ptr) GET_REVERSED_64(ptr)
+# define get64BE(ptr) bswap64(*(uint64_t *)(ptr))
# define get64LE(ptr) (*(uint64_t *)(ptr))
#endif
--
2.27.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab()
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
2022-05-10 10:15 ` [PATCH v3 1/6] xen: implement byteswap Lin Liu
2022-05-10 10:15 ` [PATCH v3 2/6] crypto/vmac: Simplify code with byteswap Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-10 10:52 ` Andrew Cooper
2022-05-10 11:05 ` Julien Grall
2022-05-10 10:15 ` [PATCH v3 4/6] xen: Switch to byteswap Lin Liu
` (2 subsequent siblings)
5 siblings, 2 replies; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel
Cc: Lin Liu, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
ext2 has nothing to do with this logic. Clean up the code with
xen/byteswap.h which now has an unsigned long helper.
No functional change.
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>
This patche is not well tested without running environment for arm
---
xen/arch/arm/arm64/lib/find_next_bit.c | 36 +++++---------------------
1 file changed, 6 insertions(+), 30 deletions(-)
diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c b/xen/arch/arm/arm64/lib/find_next_bit.c
index 8ebf8bfe97..e3b3720ff4 100644
--- a/xen/arch/arm/arm64/lib/find_next_bit.c
+++ b/xen/arch/arm/arm64/lib/find_next_bit.c
@@ -161,30 +161,6 @@ EXPORT_SYMBOL(find_first_zero_bit);
#ifdef __BIG_ENDIAN
-/* include/linux/byteorder does not support "unsigned long" type */
-static inline unsigned long ext2_swabp(const unsigned long * x)
-{
-#if BITS_PER_LONG == 64
- return (unsigned long) __swab64p((u64 *) x);
-#elif BITS_PER_LONG == 32
- return (unsigned long) __swab32p((u32 *) x);
-#else
-#error BITS_PER_LONG not defined
-#endif
-}
-
-/* include/linux/byteorder doesn't support "unsigned long" type */
-static inline unsigned long ext2_swab(const unsigned long y)
-{
-#if BITS_PER_LONG == 64
- return (unsigned long) __swab64((u64) y);
-#elif BITS_PER_LONG == 32
- return (unsigned long) __swab32((u32) y);
-#else
-#error BITS_PER_LONG not defined
-#endif
-}
-
#ifndef find_next_zero_bit_le
unsigned long find_next_zero_bit_le(const void *addr, unsigned
long size, unsigned long offset)
@@ -199,7 +175,7 @@ unsigned long find_next_zero_bit_le(const void *addr, unsigned
size -= result;
offset &= (BITS_PER_LONG - 1UL);
if (offset) {
- tmp = ext2_swabp(p++);
+ tmp = bswap_ul(*p++);
tmp |= (~0UL >> (BITS_PER_LONG - offset));
if (size < BITS_PER_LONG)
goto found_first;
@@ -217,7 +193,7 @@ unsigned long find_next_zero_bit_le(const void *addr, unsigned
}
if (!size)
return result;
- tmp = ext2_swabp(p);
+ tmp = bswap_ul(*p);
found_first:
tmp |= ~0UL << size;
if (tmp == ~0UL) /* Are any bits zero? */
@@ -226,7 +202,7 @@ found_middle:
return result + ffz(tmp);
found_middle_swap:
- return result + ffz(ext2_swab(tmp));
+ return result + ffz(bswap_ul(tmp));
}
EXPORT_SYMBOL(find_next_zero_bit_le);
#endif
@@ -245,7 +221,7 @@ unsigned long find_next_bit_le(const void *addr, unsigned
size -= result;
offset &= (BITS_PER_LONG - 1UL);
if (offset) {
- tmp = ext2_swabp(p++);
+ tmp = bswap_ul(*p++);
tmp &= (~0UL << offset);
if (size < BITS_PER_LONG)
goto found_first;
@@ -264,7 +240,7 @@ unsigned long find_next_bit_le(const void *addr, unsigned
}
if (!size)
return result;
- tmp = ext2_swabp(p);
+ tmp = bswap_ul(*p);
found_first:
tmp &= (~0UL >> (BITS_PER_LONG - size));
if (tmp == 0UL) /* Are any bits set? */
@@ -273,7 +249,7 @@ found_middle:
return result + __ffs(tmp);
found_middle_swap:
- return result + __ffs(ext2_swab(tmp));
+ return result + __ffs(bswap_ul(tmp));
}
EXPORT_SYMBOL(find_next_bit_le);
#endif
--
2.27.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab()
2022-05-10 10:15 ` [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab() Lin Liu
@ 2022-05-10 10:52 ` Andrew Cooper
2022-05-10 11:05 ` Julien Grall
1 sibling, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2022-05-10 10:52 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
On 10/05/2022 11:15, Lin Liu wrote:
> ext2 has nothing to do with this logic. Clean up the code with
> xen/byteswap.h which now has an unsigned long helper.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab()
2022-05-10 10:15 ` [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab() Lin Liu
2022-05-10 10:52 ` Andrew Cooper
@ 2022-05-10 11:05 ` Julien Grall
1 sibling, 0 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-10 11:05 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 10/05/2022 11:15, Lin Liu wrote:
> ext2 has nothing to do with this logic.
This code was a verbatim copy from Linux. Looking at the history, I am
not sure there was even a connection with the ext2 filesystem (I guess
this is what you mean?).
So I would drop this and simply say that we could use the new helpers.
> Clean up the code with
> xen/byteswap.h which now has an unsigned long helper.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
Other that what I wrote above:
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
` (2 preceding siblings ...)
2022-05-10 10:15 ` [PATCH v3 3/6] arm64/find_next_bit: Remove ext2_swab() Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-10 10:51 ` Julien Grall
2022-05-10 14:32 ` Anthony PERARD
2022-05-10 10:15 ` [PATCH v3 5/6] byteorder: Remove byteorder Lin Liu
2022-05-10 10:15 ` [PATCH v3 6/6] tools: Remove unnecessary header Lin Liu
5 siblings, 2 replies; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel
Cc: Lin Liu, Stefano Stabellini, Julien Grall, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Update to use byteswap to swap bytes.
No functional change.
Signed-off-by: Lin Liu <lin.liu@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
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>
Changes in v3:
- Update xen/common/device_tree.c to use be32_to_cpu
- Keep const in type cast in unaligned.h
---
xen/common/device_tree.c | 44 +++++++++++++++---------------
xen/common/libelf/libelf-private.h | 6 ++--
xen/common/xz/private.h | 2 +-
xen/include/xen/unaligned.h | 24 ++++++++--------
4 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..70d3be3be6 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
if ( !val || len < sizeof(*out_value) )
return 0;
- *out_value = be32_to_cpup(val);
+ *out_value = be32_to_cpu(*val);
return 1;
}
@@ -264,7 +264,7 @@ int dt_property_read_variable_u32_array(const struct dt_device_node *np,
count = sz;
while ( count-- )
- *out_values++ = be32_to_cpup(val++);
+ *out_values++ = be32_to_cpu(*val++);
return sz;
}
@@ -490,7 +490,7 @@ static int __dt_n_addr_cells(const struct dt_device_node *np, bool_t parent)
ip = dt_get_property(np, "#address-cells", NULL);
if ( ip )
- return be32_to_cpup(ip);
+ return be32_to_cpu(*ip);
} while ( np->parent );
/* No #address-cells property for the root node */
return DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
@@ -507,7 +507,7 @@ int __dt_n_size_cells(const struct dt_device_node *np, bool_t parent)
ip = dt_get_property(np, "#size-cells", NULL);
if ( ip )
- return be32_to_cpup(ip);
+ return be32_to_cpu(*ip);
} while ( np->parent );
/* No #address-cells property for the root node */
return DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
@@ -660,7 +660,7 @@ static void dt_bus_pci_count_cells(const struct dt_device_node *np,
static unsigned int dt_bus_pci_get_flags(const __be32 *addr)
{
unsigned int flags = 0;
- u32 w = be32_to_cpup(addr);
+ u32 w = be32_to_cpu(*addr);
switch((w >> 24) & 0x03) {
case 0x01:
@@ -1077,7 +1077,7 @@ dt_irq_find_parent(const struct dt_device_node *child)
if ( parp == NULL )
p = dt_get_parent(child);
else
- p = dt_find_node_by_phandle(be32_to_cpup(parp));
+ p = dt_find_node_by_phandle(be32_to_cpu(*parp));
child = p;
} while ( p && dt_get_property(p, "#interrupt-cells", NULL) == NULL );
@@ -1110,7 +1110,7 @@ unsigned int dt_number_of_irq(const struct dt_device_node *device)
intlen /= sizeof(*intspec);
dt_dprintk(" using 'interrupts' property\n");
- dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen);
+ dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpu(*intspec), intlen);
/* Look for the interrupt parent. */
p = dt_irq_find_parent(device);
@@ -1241,7 +1241,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
imaplen -= addrsize + intsize;
/* Get the interrupt parent */
- ipar = dt_find_node_by_phandle(be32_to_cpup(imap));
+ ipar = dt_find_node_by_phandle(be32_to_cpu(*imap));
imap++;
--imaplen;
@@ -1358,8 +1358,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
int match, i;
dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
- parent->full_name, be32_to_cpup(intspec),
- be32_to_cpup(intspec + 1), ointsize);
+ parent->full_name, be32_to_cpu(*intspec),
+ be32_to_cpu(*(intspec+1)), ointsize);
ipar = parent;
@@ -1471,7 +1471,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
dt_dprintk(" -> match=%d (imaplen=%d)\n", match, imaplen);
/* Get the interrupt parent */
- newpar = dt_find_node_by_phandle(be32_to_cpup(imap));
+ newpar = dt_find_node_by_phandle(be32_to_cpu(*imap));
imap++;
--imaplen;
@@ -1565,7 +1565,7 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,
intlen /= sizeof(*intspec);
dt_dprintk(" using 'interrupts' property\n");
- dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen);
+ dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpu(*intspec), intlen);
/* Look for the interrupt parent. */
p = dt_irq_find_parent(device);
@@ -1676,7 +1676,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
* If phandle is 0, then it is an empty entry with no
* arguments. Skip forward to the next entry.
* */
- phandle = be32_to_cpup(list++);
+ phandle = be32_to_cpu(*list++);
if ( phandle )
{
/*
@@ -1745,7 +1745,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
out_args->np = node;
out_args->args_count = count;
for ( i = 0; i < count; i++ )
- out_args->args[i] = be32_to_cpup(list++);
+ out_args->args[i] = be32_to_cpu(*list++);
}
/* Found it! return success */
@@ -1826,7 +1826,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
int has_name = 0;
int new_format = 0;
- tag = be32_to_cpup((__be32 *)(*p));
+ tag = be32_to_cpu(*(__be32 *)(*p));
if ( tag != FDT_BEGIN_NODE )
{
printk(XENLOG_WARNING "Weird tag at start of node: %x\n", tag);
@@ -1919,7 +1919,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
u32 sz, noff;
const char *pname;
- tag = be32_to_cpup((__be32 *)(*p));
+ tag = be32_to_cpu(*(__be32 *)(*p));
if ( tag == FDT_NOP )
{
*p += 4;
@@ -1928,8 +1928,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
if ( tag != FDT_PROP )
break;
*p += 4;
- sz = be32_to_cpup((__be32 *)(*p));
- noff = be32_to_cpup((__be32 *)((*p) + 4));
+ sz = be32_to_cpu(*(__be32 *)(*p));
+ noff = be32_to_cpu(*(__be32 *)((*p) + 4));
*p += 8;
if ( fdt_version(fdt) < 0x10 )
*p = ROUNDUP(*p, sz >= 8 ? 8 : 4);
@@ -1956,13 +1956,13 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
(strcmp(pname, "linux,phandle") == 0) )
{
if ( np->phandle == 0 )
- np->phandle = be32_to_cpup((__be32*)*p);
+ np->phandle = be32_to_cpu(*(__be32*)*p);
}
/* And we process the "ibm,phandle" property
* used in pSeries dynamic device tree
* stuff */
if ( strcmp(pname, "ibm,phandle") == 0 )
- np->phandle = be32_to_cpup((__be32 *)*p);
+ np->phandle = be32_to_cpu(*(__be32 *)*p);
pp->name = pname;
pp->length = sz;
pp->value = (void *)*p;
@@ -2034,7 +2034,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
*p += 4;
else
mem = unflatten_dt_node(fdt, mem, p, np, allnextpp, fpsize);
- tag = be32_to_cpup((__be32 *)(*p));
+ tag = be32_to_cpu(*(__be32 *)(*p));
}
if ( tag != FDT_END_NODE )
{
@@ -2086,7 +2086,7 @@ static void __init __unflatten_device_tree(const void *fdt,
/* Second pass, do actual unflattening */
start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
unflatten_dt_node(fdt, mem, &start, NULL, &allnextp, 0);
- if ( be32_to_cpup((__be32 *)start) != FDT_END )
+ if ( be32_to_cpu(*(__be32 *)start) != FDT_END )
printk(XENLOG_WARNING "Weird tag at end of tree: %08x\n",
*((u32 *)start));
if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeef )
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 47db679966..6062598fb8 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -31,9 +31,9 @@
printk(fmt, ## args )
#define strtoull(str, end, base) simple_strtoull(str, end, base)
-#define bswap_16(x) swab16(x)
-#define bswap_32(x) swab32(x)
-#define bswap_64(x) swab64(x)
+#define bswap_16(x) bswap16(x)
+#define bswap_32(x) bswap32(x)
+#define bswap_64(x) bswap64(x)
#else /* !__XEN__ */
diff --git a/xen/common/xz/private.h b/xen/common/xz/private.h
index 511343fcc2..97131fa714 100644
--- a/xen/common/xz/private.h
+++ b/xen/common/xz/private.h
@@ -28,7 +28,7 @@ static inline void put_unaligned_le32(u32 val, void *p)
#endif
-#define get_le32(p) le32_to_cpup((const uint32_t *)(p))
+#define get_le32(p) le32_to_cpu(*(const uint32_t *)(p))
#define false 0
#define true 1
diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
index 0a2b16d05d..16b2e6f5f0 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -20,62 +20,62 @@
static inline uint16_t get_unaligned_be16(const void *p)
{
- return be16_to_cpup(p);
+ return be16_to_cpu(*(const uint16_t *)p);
}
static inline void put_unaligned_be16(uint16_t val, void *p)
{
- *(__force __be16*)p = cpu_to_be16(val);
+ *(__be16 *)p = cpu_to_be16(val);
}
static inline uint32_t get_unaligned_be32(const void *p)
{
- return be32_to_cpup(p);
+ return be32_to_cpu(*(const uint32_t *)p);
}
static inline void put_unaligned_be32(uint32_t val, void *p)
{
- *(__force __be32*)p = cpu_to_be32(val);
+ *(__be32 *)p = cpu_to_be32(val);
}
static inline uint64_t get_unaligned_be64(const void *p)
{
- return be64_to_cpup(p);
+ return be64_to_cpu(*(const uint64_t *)p);
}
static inline void put_unaligned_be64(uint64_t val, void *p)
{
- *(__force __be64*)p = cpu_to_be64(val);
+ *(__be64 *)p = cpu_to_be64(val);
}
static inline uint16_t get_unaligned_le16(const void *p)
{
- return le16_to_cpup(p);
+ return le16_to_cpu(*(const uint16_t *)p);
}
static inline void put_unaligned_le16(uint16_t val, void *p)
{
- *(__force __le16*)p = cpu_to_le16(val);
+ *(__le16 *)p = cpu_to_le16(val);
}
static inline uint32_t get_unaligned_le32(const void *p)
{
- return le32_to_cpup(p);
+ return le32_to_cpu(*(const uint32_t *)p);
}
static inline void put_unaligned_le32(uint32_t val, void *p)
{
- *(__force __le32*)p = cpu_to_le32(val);
+ *(__le32 *)p = cpu_to_le32(val);
}
static inline uint64_t get_unaligned_le64(const void *p)
{
- return le64_to_cpup(p);
+ return le64_to_cpu(*(const uint64_t *)p);
}
static inline void put_unaligned_le64(uint64_t val, void *p)
{
- *(__force __le64*)p = cpu_to_le64(val);
+ *(__le64 *)p = cpu_to_le64(val);
}
#endif /* __XEN_UNALIGNED_H__ */
--
2.27.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 10:15 ` [PATCH v3 4/6] xen: Switch to byteswap Lin Liu
@ 2022-05-10 10:51 ` Julien Grall
2022-05-10 11:09 ` Andrew Cooper
2022-05-11 6:30 ` Lin Liu （刘林）
2022-05-10 14:32 ` Anthony PERARD
1 sibling, 2 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-10 10:51 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
Hi,
On 10/05/2022 11:15, Lin Liu wrote:
> Update to use byteswap to swap bytes.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> 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>
> Changes in v3:
> - Update xen/common/device_tree.c to use be32_to_cpu
> - Keep const in type cast in unaligned.h
> ---
> xen/common/device_tree.c | 44 +++++++++++++++---------------
> xen/common/libelf/libelf-private.h | 6 ++--
> xen/common/xz/private.h | 2 +-
> xen/include/xen/unaligned.h | 24 ++++++++--------
> 4 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..70d3be3be6 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
> if ( !val || len < sizeof(*out_value) )
> return 0;
>
> - *out_value = be32_to_cpup(val);
> + *out_value = be32_to_cpu(*val);
This code has been taken from Linux and I would rather prefer to keep
the *cpup* helpers to avoid any changes when backporting.
> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..16b2e6f5f0 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -20,62 +20,62 @@
>
> static inline uint16_t get_unaligned_be16(const void *p)
> {
> - return be16_to_cpup(p);
> + return be16_to_cpu(*(const uint16_t *)p)
I haven't checked the existing implementation of be16_to_cpup().
However, this new approach would allow the compiler to use a single load
instruction to read the 16-bit value from memory. So this change may
break on platform where unaligned access is forbidden (such as arm32).
> }
>
> static inline void put_unaligned_be16(uint16_t val, void *p)
> {
> - *(__force __be16*)p = cpu_to_be16(val);
> + *(__be16 *)p = cpu_to_be16(val);
Why did you drop the __force?
> }
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 10:51 ` Julien Grall
@ 2022-05-10 11:09 ` Andrew Cooper
2022-05-10 11:17 ` Julien Grall
2022-05-11 6:30 ` Lin Liu （刘林）
1 sibling, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2022-05-10 11:09 UTC (permalink / raw)
To: Julien Grall, Lin Liu, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
On 10/05/2022 11:51, Julien Grall wrote:
> On 10/05/2022 11:15, Lin Liu wrote:
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4aae281e89..70d3be3be6 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct
>> dt_device_node *np,
>> if ( !val || len < sizeof(*out_value) )
>> return 0;
>> - *out_value = be32_to_cpup(val);
>> + *out_value = be32_to_cpu(*val);
>
> This code has been taken from Linux and I would rather prefer to keep
> the *cpup* helpers to avoid any changes when backporting.
I specifically requested that this be de-obfuscated. Hiding indirection
is a fantastic way to introduce bugs, and we've had XSAs in the past
because of it (admittedly in libxl, but still...).
This file is already Xen style, not Linux, so won't be taking backports
directly, and the resulting compiler diagnostic will make it obvious
what is going on. be32_to_cpu(*val) works fine on older versions of Xen too.
In this case, the cost of changing is well worth the improvements and
simplifications gained. See the 0/6 diffstat and see that the compiler
can make better optimisations when it can see the builtin.
>
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..16b2e6f5f0 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -20,62 +20,62 @@
>> static inline uint16_t get_unaligned_be16(const void *p)
>> {
>> - return be16_to_cpup(p);
>> + return be16_to_cpu(*(const uint16_t *)p)
>
> I haven't checked the existing implementation of be16_to_cpup().
It's a plain dereference, just like this. AFAICT, it wasn't unaligned
safe before, either.
It should be reasonably easy to fix in a followup patch. Just memcpy()
to/from the void pointer to a stack variable of the appropriate type.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 11:09 ` Andrew Cooper
@ 2022-05-10 11:17 ` Julien Grall
2022-05-10 11:34 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2022-05-10 11:17 UTC (permalink / raw)
To: Andrew Cooper, Lin Liu, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
Hi,
On 10/05/2022 12:09, Andrew Cooper wrote:
> On 10/05/2022 11:51, Julien Grall wrote:
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 4aae281e89..70d3be3be6 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct
>>> dt_device_node *np,
>>> if ( !val || len < sizeof(*out_value) )
>>> return 0;
>>> - *out_value = be32_to_cpup(val);
>>> + *out_value = be32_to_cpu(*val);
>>
>> This code has been taken from Linux and I would rather prefer to keep
>> the *cpup* helpers to avoid any changes when backporting.
>
> I specifically requested that this be de-obfuscated. Hiding indirection
> is a fantastic way to introduce bugs, and we've had XSAs in the past
> because of it (admittedly in libxl, but still...).
Care providing a link to those XSAs? But I don't really see what's the
problem here, this is no better no worth than passing pointer to other
functions...
>
> This file is already Xen style, not Linux, so won't be taking backports
> directly, and the resulting compiler diagnostic will make it obvious
> what is going on. be32_to_cpu(*val) works fine on older versions of Xen too.
>
> In this case, the cost of changing is well worth the improvements and
> simplifications gained. See the 0/6 diffstat and see that the compiler
> can make better optimisations when it can see the builtin.
I take your point... However, the commit message provides virtually zero
justification into why we should switch to be32_to_cpup(). So to me, the
changes so far looks unwanted.
>
>>
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..16b2e6f5f0 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -20,62 +20,62 @@
>>> static inline uint16_t get_unaligned_be16(const void *p)
>>> {
>>> - return be16_to_cpup(p);
>>> + return be16_to_cpu(*(const uint16_t *)p)
>>
>> I haven't checked the existing implementation of be16_to_cpup().
>
> It's a plain dereference, just like this. AFAICT, it wasn't unaligned
> safe before, either.
Well, technically an architecture could provide an override for the
copy. I agree that arm32 is already bogus but...
>
> It should be reasonably easy to fix in a followup patch. Just memcpy()
> to/from the void pointer to a stack variable of the appropriate type.
... I disagree that it should be fixed in a follow-up patch. It should
be fixed now as this is where the badness is spread to any architecture.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 11:17 ` Julien Grall
@ 2022-05-10 11:34 ` Andrew Cooper
2022-05-10 11:47 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2022-05-10 11:34 UTC (permalink / raw)
To: Julien Grall, Lin Liu, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Bertrand Marquis
On 10/05/2022 12:17, Julien Grall wrote:
>>
>>>
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -20,62 +20,62 @@
>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>> {
>>>> - return be16_to_cpup(p);
>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>
>>> I haven't checked the existing implementation of be16_to_cpup().
>>
>> It's a plain dereference, just like this. AFAICT, it wasn't unaligned
>> safe before, either.
>
> Well, technically an architecture could provide an override for the
> copy. I agree that arm32 is already bogus but...
>
>>
>> It should be reasonably easy to fix in a followup patch. Just memcpy()
>> to/from the void pointer to a stack variable of the appropriate type.
> ... I disagree that it should be fixed in a follow-up patch. It should
> be fixed now as this is where the badness is spread to any architecture.
No. That is an inappropriate request to make.
Lin's patch does not alter the broken-ness of unaligned on arm32, and
does improve the aspect of the hypervisor that it pertains to. It
therefore stands on its own merit.
Your choices are to either fix it yourself (after all, you are the
maintainer who cares about this unrelated bug), or you ask Lin kindly if
he has time to look into fixing the unrelated bug after this series is
complete.
It is not reasonable to say "this unrelated thing is broken, and you
need to fix it first to get your series in". Requests like that are,
I'm sure, part of what Bertrand raised in the community call as
unnecessary fiction getting work submitted.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 11:34 ` Andrew Cooper
@ 2022-05-10 11:47 ` Julien Grall
2022-05-11 3:12 ` Stefano Stabellini
2022-05-11 9:56 ` Andrew Cooper
0 siblings, 2 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-10 11:47 UTC (permalink / raw)
To: Andrew Cooper, Lin Liu, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Bertrand Marquis
Hi,
On 10/05/2022 12:34, Andrew Cooper wrote:
> On 10/05/2022 12:17, Julien Grall wrote:
>>>
>>>>
>>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>> --- a/xen/include/xen/unaligned.h
>>>>> +++ b/xen/include/xen/unaligned.h
>>>>> @@ -20,62 +20,62 @@
>>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>>> {
>>>>> - return be16_to_cpup(p);
>>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>>
>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>
>>> It's a plain dereference, just like this. AFAICT, it wasn't unaligned
>>> safe before, either.
>>
>> Well, technically an architecture could provide an override for the
>> copy. I agree that arm32 is already bogus but...
>>
>>>
>>> It should be reasonably easy to fix in a followup patch. Just memcpy()
>>> to/from the void pointer to a stack variable of the appropriate type.
>> ... I disagree that it should be fixed in a follow-up patch. It should
>> be fixed now as this is where the badness is spread to any architecture.
>
> No. That is an inappropriate request to make.
>
> Lin's patch does not alter the broken-ness of unaligned on arm32, and
> does improve the aspect of the hypervisor that it pertains to. It
> therefore stands on its own merit.
I am not sure sure why switching from *cpup* improves things... and as
usual you haven't answered to the clarification questions.
>
> Your choices are to either fix it yourself (after all, you are the
> maintainer who cares about this unrelated bug), or you ask Lin kindly if
> he has time to look into fixing the unrelated bug after this series is
> complete.
Or 3) keep *cpup* so there is only one place to fix it.
>
> It is not reasonable to say "this unrelated thing is broken, and you
> need to fix it first to get your series in". Requests like that are,
> I'm sure, part of what Bertrand raised in the community call as
> unnecessary fiction getting work submitted.
To be honest, you put the contributor in this situation. I would have
been perfectly happy if we keep *cpup* around as there would be only a
place to fix.
With this approach, you are effectively going to increase the work later
one because now we would have to chase all the open-coded version of
*cpup* and check which one is not safe.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 11:47 ` Julien Grall
@ 2022-05-11 3:12 ` Stefano Stabellini
2022-05-11 8:21 ` Julien Grall
2022-05-11 14:16 ` Bertrand Marquis
2022-05-11 9:56 ` Andrew Cooper
1 sibling, 2 replies; 34+ messages in thread
From: Stefano Stabellini @ 2022-05-11 3:12 UTC (permalink / raw)
To: Julien Grall
Cc: Andrew Cooper, Lin Liu, xen-devel, Stefano Stabellini,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Bertrand Marquis
[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]
On Tue, 10 May 2022, Julien Grall wrote:
> > It is not reasonable to say "this unrelated thing is broken, and you
> > need to fix it first to get your series in". Requests like that are,
> > I'm sure, part of what Bertrand raised in the community call as
> > unnecessary fiction getting work submitted.
>
> To be honest, you put the contributor in this situation. I would have been
> perfectly happy if we keep *cpup* around as there would be only a place to
> fix.
>
> With this approach, you are effectively going to increase the work later one
> because now we would have to chase all the open-coded version of *cpup* and
> check which one is not safe.
Without disagreeing with Julien or Andrew, I am actually happy to see an
effort to make the review process faster. We have lot of room for
improvement and spotting opportunities to do so is the first step toward
improving the process. I have actually been thinking about how to make
things faster in cases like this and I have a suggesion below.
In this case all of the options are OK. Whether we fix the alignment
problem as part of this patch or soon after it doesn't make much of a
difference. It is more important that we don't get bogged down in a long
discussion. Coding is faster and more fun.
It would take less time for Julien (or Andrew) to write the code than to
explain to the contributor how to do it. English is a good language for
an architectural discussion, but simply replying with the example code
in C would be much faster in cases like this one.
So my suggestion is that it would be best if the reviewer (Julien in
this case) replied directly with the code snipper he wants added. Just
an example without looking too closely:
---
Please do this instead so that alignment also gets fixed:
return be16_to_cpu(*(const __packed uint16_t *)p);
---
Alternatively, I also think that taking this patch as is without
alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
alignment could be fixed afterwards. The key is that I think it should
be one of the maintainers to write the follow-up fix. Both of you are
very fast coders and would certainly finish the patch before finishing
the explanation on what needs to be done, which then would need to be
understood and implemented by the contributor (opportunity for
misunderstandings), and verified again by the reviewer on v2. That would
take an order of magnitude more of collective time and effort.
Of course this doesn't apply to all cases, but it should apply to quite
a few.
In short, less English, more C ;-)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 3:12 ` Stefano Stabellini
@ 2022-05-11 8:21 ` Julien Grall
2022-05-11 14:16 ` Bertrand Marquis
1 sibling, 0 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-11 8:21 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Lin Liu, xen-devel, Andrew Cooper, George Dunlap,
Jan Beulich, Wei Liu, Bertrand Marquis
Hi,
On 11/05/2022 04:12, Stefano Stabellini wrote:
> Alternatively, I also think that taking this patch as is without
> alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
> alignment could be fixed afterwards. The key is that I think it should
> be one of the maintainers to write the follow-up fix. Both of you are
> very fast coders and would certainly finish the patch before finishing
> the explanation on what needs to be done, which then would need to be
> understood and implemented by the contributor (opportunity for
> misunderstandings), and verified again by the reviewer on v2. That would
> take an order of magnitude more of collective time and effort.
I am happy to write a follow-up patch so long be16_to_cpup() (& co) are
kept (because this is where I want to fix this issue so it is done for
everyone).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 3:12 ` Stefano Stabellini
2022-05-11 8:21 ` Julien Grall
@ 2022-05-11 14:16 ` Bertrand Marquis
1 sibling, 0 replies; 34+ messages in thread
From: Bertrand Marquis @ 2022-05-11 14:16 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Julien Grall, Andrew Cooper, Lin Liu, xen-devel, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Hi,
> On 11 May 2022, at 04:12, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Tue, 10 May 2022, Julien Grall wrote:
>>> It is not reasonable to say "this unrelated thing is broken, and you
>>> need to fix it first to get your series in". Requests like that are,
>>> I'm sure, part of what Bertrand raised in the community call as
>>> unnecessary fiction getting work submitted.
>>
>> To be honest, you put the contributor in this situation. I would have been
>> perfectly happy if we keep *cpup* around as there would be only a place to
>> fix.
>>
>> With this approach, you are effectively going to increase the work later one
>> because now we would have to chase all the open-coded version of *cpup* and
>> check which one is not safe.
>
>
> Without disagreeing with Julien or Andrew, I am actually happy to see an
> effort to make the review process faster. We have lot of room for
> improvement and spotting opportunities to do so is the first step toward
> improving the process. I have actually been thinking about how to make
> things faster in cases like this and I have a suggesion below.
Definitely with you here, it is good to see that my message on review process
and effort is actually in people’s minds :-)
>
> In this case all of the options are OK. Whether we fix the alignment
> problem as part of this patch or soon after it doesn't make much of a
> difference. It is more important that we don't get bogged down in a long
> discussion. Coding is faster and more fun.
I would just make a small exception here (which corresponds to something
Julien kind of suggested during the discussion): unless we introduce a
temporary bug between the patches.
But this could actually be solved here by making a patch upfront and merging
it before the one in discussion (which might require some rebasing).
>
> It would take less time for Julien (or Andrew) to write the code than to
> explain to the contributor how to do it. English is a good language for
> an architectural discussion, but simply replying with the example code
> in C would be much faster in cases like this one.
>
> So my suggestion is that it would be best if the reviewer (Julien in
> this case) replied directly with the code snipper he wants added. Just
> an example without looking too closely:
>
> ---
> Please do this instead so that alignment also gets fixed:
>
> return be16_to_cpu(*(const __packed uint16_t *)p);
> ---
>
>
> Alternatively, I also think that taking this patch as is without
> alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
> alignment could be fixed afterwards. The key is that I think it should
> be one of the maintainers to write the follow-up fix. Both of you are
> very fast coders and would certainly finish the patch before finishing
> the explanation on what needs to be done, which then would need to be
> understood and implemented by the contributor (opportunity for
> misunderstandings), and verified again by the reviewer on v2. That would
> take an order of magnitude more of collective time and effort.
Agree with the exception I mentioned.
>
> Of course this doesn't apply to all cases, but it should apply to quite
> a few.
>
> In short, less English, more C ;-)
Same goes for things like “please add a comment” or “please say
something in the commit message”, it would be most of the time easier
for everyone to do: Could you add the comment “xxx” on top of this or
the sentence “yyy” in your commit (or even better ask the contributor if
he is ok with it and do it on commit when it not modifying the code).
Cheers
Bertrand
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 11:47 ` Julien Grall
2022-05-11 3:12 ` Stefano Stabellini
@ 2022-05-11 9:56 ` Andrew Cooper
2022-05-11 10:55 ` Julien Grall
1 sibling, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2022-05-11 9:56 UTC (permalink / raw)
To: Julien Grall, Lin Liu （刘林）, xen-devel
Cc: Stefano Stabellini, George Dunlap, Jan Beulich, Wei Liu,
Bertrand Marquis
On 10/05/2022 12:47, Julien Grall wrote:
> Hi,
>
> On 10/05/2022 12:34, Andrew Cooper wrote:
>> On 10/05/2022 12:17, Julien Grall wrote:
>>>>
>>>>>
>>>>>> diff --git a/xen/include/xen/unaligned.h
>>>>>> b/xen/include/xen/unaligned.h
>>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>>> --- a/xen/include/xen/unaligned.h
>>>>>> +++ b/xen/include/xen/unaligned.h
>>>>>> @@ -20,62 +20,62 @@
>>>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>>>> {
>>>>>> - return be16_to_cpup(p);
>>>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>>>
>>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>>
>>>> It's a plain dereference, just like this. AFAICT, it wasn't unaligned
>>>> safe before, either.
>>>
>>> Well, technically an architecture could provide an override for the
>>> copy. I agree that arm32 is already bogus but...
>>>
>>>>
>>>> It should be reasonably easy to fix in a followup patch. Just
>>>> memcpy()
>>>> to/from the void pointer to a stack variable of the appropriate type.
>>> ... I disagree that it should be fixed in a follow-up patch. It should
>>> be fixed now as this is where the badness is spread to any
>>> architecture.
>>
>> No. That is an inappropriate request to make.
>>
>> Lin's patch does not alter the broken-ness of unaligned on arm32, and
>> does improve the aspect of the hypervisor that it pertains to. It
>> therefore stands on its own merit.
> I am not sure sure why switching from *cpup* improves things... and as
> usual you haven't answered to the clarification questions.
Adjust your tone back to something appropriate to the conversation.
The p helpers hide a unsafe typecast&dereference which will erroneously
compile both of these:
void foo(void *ptr)
{
blah_p(ptr);
}
void bar(baz *ptr)
{
blah_p(ptr);
}
and potentially malfunction as a consequence, not to mention that it's
sufficient obfuscation to trick a maintainer into believe an unrelated
area of code was safe when it wasn't.
Deleting the p helpers is a specific objective of the work, because it
forces the author to resolve to an integral type, and have the deference
chain visible in a single location which improves code clarity.
On a hunch, I checked the MISRA spec, and it turns out there is a rule
against the p helpers (specifically the type safety aspect), so this
series is removing a whole load of DIR 4.9 violations from the codebase.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 9:56 ` Andrew Cooper
@ 2022-05-11 10:55 ` Julien Grall
0 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-11 10:55 UTC (permalink / raw)
To: Andrew Cooper, Lin Liu （刘林）, xen-devel
Cc: Stefano Stabellini, George Dunlap, Jan Beulich, Wei Liu,
Bertrand Marquis
On 11/05/2022 10:56, Andrew Cooper wrote:
> On 10/05/2022 12:47, Julien Grall wrote:
>> Hi,
>>
>> On 10/05/2022 12:34, Andrew Cooper wrote:
>>> On 10/05/2022 12:17, Julien Grall wrote:
>>>>>
>>>>>>
>>>>>>> diff --git a/xen/include/xen/unaligned.h
>>>>>>> b/xen/include/xen/unaligned.h
>>>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>>>> --- a/xen/include/xen/unaligned.h
>>>>>>> +++ b/xen/include/xen/unaligned.h
>>>>>>> @@ -20,62 +20,62 @@
>>>>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>>>>> {
>>>>>>> - return be16_to_cpup(p);
>>>>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>>>>
>>>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>>>
>>>>> It's a plain dereference, just like this. AFAICT, it wasn't unaligned
>>>>> safe before, either.
>>>>
>>>> Well, technically an architecture could provide an override for the
>>>> copy. I agree that arm32 is already bogus but...
>>>>
>>>>>
>>>>> It should be reasonably easy to fix in a followup patch. Just
>>>>> memcpy()
>>>>> to/from the void pointer to a stack variable of the appropriate type.
>>>> ... I disagree that it should be fixed in a follow-up patch. It should
>>>> be fixed now as this is where the badness is spread to any
>>>> architecture.
>>>
>>> No. That is an inappropriate request to make.
>>>
>>> Lin's patch does not alter the broken-ness of unaligned on arm32, and
>>> does improve the aspect of the hypervisor that it pertains to. It
>>> therefore stands on its own merit.
>> I am not sure sure why switching from *cpup* improves things... and as
>> usual you haven't answered to the clarification questions.
>
> Adjust your tone back to something appropriate to the conversation.
It was indeed harsh. Sorry for that.
>
> The p helpers hide a unsafe typecast&dereference which will erroneously
> compile both of these:
>
> void foo(void *ptr)
> {
> blah_p(ptr);
> }
>
> void bar(baz *ptr)
> {
> blah_p(ptr);
> }
I am assuming that blah would expect a (blah *).
>
> and potentially malfunction as a consequence, not to mention that it's
> sufficient obfuscation to trick a maintainer into believe an unrelated
> area of code was safe when it wasn't.
I looked at the helpers, they are static inline and use a proper type.
Therefore, I am not sure why bar would compile in this situation.
In fact, to me it seems this is an inherent issue to C: any void pointer
can be casted to anything. You are removing one here, but there are
hundreds of other potential "unsafe" place in Xen.
>
> Deleting the p helpers is a specific objective of the work, because it
> forces the author to resolve to an integral type, and have the deference
> chain visible in a single location which improves code clarity.
See above, I think dropping p helpers is not solving the underlying
problem (we are not going to be able to remove pointers in Xen).
What would solve the problem is forbidding cast from void pointer to any
pointer. At which point, keeping *cpup* is not going to be problem.
>
> On a hunch, I checked the MISRA spec, and it turns out there is a rule
> against the p helpers (specifically the type safety aspect), so this
> series is removing a whole load of DIR 4.9 violations from the codebase.
I read through DIR 4.9., AFAIU it is about using function rather than
macro. The current implementation of *cpup* are using function so I
don't understand how removing *cpup* would help.
So I am afraid, I still see no justifications to drop *cpup* here and I
actually prefer them the open-code version. I will not Nack it, but I
will not support.
In any case, the commit message should contain some information why they
are dropped.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 10:51 ` Julien Grall
2022-05-10 11:09 ` Andrew Cooper
@ 2022-05-11 6:30 ` Lin Liu （刘林）
2022-05-11 8:34 ` Julien Grall
1 sibling, 1 reply; 34+ messages in thread
From: Lin Liu （刘林） @ 2022-05-11 6:30 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]
Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
Hi,
On 10/05/2022 11:15, Lin Liu wrote:
> Update to use byteswap to swap bytes.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> 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>
> Changes in v3:
> - Update xen/common/device_tree.c to use be32_to_cpu
> - Keep const in type cast in unaligned.h
> ---
> xen/common/device_tree.c | 44 +++++++++++++++---------------
> xen/common/libelf/libelf-private.h | 6 ++--
> xen/common/xz/private.h | 2 +-
> xen/include/xen/unaligned.h | 24 ++++++++--------
> 4 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..70d3be3be6 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
> if ( !val || len < sizeof(*out_value) )
> return 0;
>
> - *out_value = be32_to_cpup(val);
> + *out_value = be32_to_cpu(*val);
> This code has been taken from Linux and I would rather prefer to keep
>the *cpup* helpers to avoid any changes when backporting.
> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..16b2e6f5f0 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -20,62 +20,62 @@
>
> static inline uint16_t get_unaligned_be16(const void *p)
> {
> - return be16_to_cpup(p);
> + return be16_to_cpu(*(const uint16_t *)p)
> I haven't checked the existing implementation of be16_to_cpup().
> However, this new approach would allow the compiler to use a single load
> instruction to read the 16-bit value from memory. So this change may
> break on platform where unaligned access is forbidden (such as arm32).
> }
>
> static inline void put_unaligned_be16(uint16_t val, void *p)
> {
> - *(__force __be16*)p = cpu_to_be16(val);
> + *(__be16 *)p = cpu_to_be16(val);
>> Why did you drop the __force?
Google told me __force is used in linux kernel to suppress warning in sparse,
https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
Is sparse also used in xen?
> }
>
[-- Attachment #2: Type: text/html, Size: 6379 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 6:30 ` Lin Liu （刘林）
@ 2022-05-11 8:34 ` Julien Grall
2022-05-11 12:11 ` George Dunlap
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2022-05-11 8:34 UTC (permalink / raw)
To: Lin Liu （刘林）, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
Hi,
Please configure your e-mail client to send in plain text.
On 11/05/2022 07:30, Lin Liu （刘林） wrote:
> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
> On 10/05/2022 11:15, Lin Liu wrote:
>> Update to use byteswap to swap bytes.
>>
>> No functional change.
>>
>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien@xen.org>
>> 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>
>> Changes in v3:
>> - Update xen/common/device_tree.c to use be32_to_cpu
>> - Keep const in type cast in unaligned.h
>> ---
>> xen/common/device_tree.c | 44 +++++++++++++++---------------
>> xen/common/libelf/libelf-private.h | 6 ++--
>> xen/common/xz/private.h | 2 +-
>> xen/include/xen/unaligned.h | 24 ++++++++--------
>> 4 files changed, 38 insertions(+), 38 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4aae281e89..70d3be3be6 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>> if ( !val || len < sizeof(*out_value) )
>> return 0;
>>
>> - *out_value = be32_to_cpup(val);
>> + *out_value = be32_to_cpu(*val);
>
>> This code has been taken from Linux and I would rather prefer to keep
>> the *cpup* helpers to avoid any changes when backporting.
>
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..16b2e6f5f0 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -20,62 +20,62 @@
>>
>> static inline uint16_t get_unaligned_be16(const void *p)
>> {
>> - return be16_to_cpup(p);
>> + return be16_to_cpu(*(const uint16_t *)p)
>
>> I haven't checked the existing implementation of be16_to_cpup().
>> However, this new approach would allow the compiler to use a single load
>> instruction to read the 16-bit value from memory. So this change may
>> break on platform where unaligned access is forbidden (such as arm32).
>
>> }
>>
>> static inline void put_unaligned_be16(uint16_t val, void *p)
>> {
>> - *(__force __be16*)p = cpu_to_be16(val);
>> + *(__be16 *)p = cpu_to_be16(val);
>
>>> Why did you drop the __force?
>
> Google told me __force is used in linux kernel to suppress warning in sparse,
> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
> Is sparse also used in xen?
I am not aware of any use of Sparse in Xen, but it would technically be
possible.
However, my point here is more that this change seems to be unrelated to
what the patch is meant to do (i.e. switching to byteswap). So if it is
unnecessary, then it should be dropped from this patch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 8:34 ` Julien Grall
@ 2022-05-11 12:11 ` George Dunlap
2022-05-11 12:39 ` Julien Grall
2022-05-11 14:21 ` Bertrand Marquis
0 siblings, 2 replies; 34+ messages in thread
From: George Dunlap @ 2022-05-11 12:11 UTC (permalink / raw)
To: Julien Grall
Cc: Lin Liu （刘林）,
xen-devel, Stefano Stabellini, Andrew Cooper, Jan Beulich,
Wei Liu
[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]
> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> Please configure your e-mail client to send in plain text.
>
> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> Update to use byteswap to swap bytes.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien@xen.org>
>>> 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>
>>> Changes in v3:
>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>> - Keep const in type cast in unaligned.h
>>> ---
>>> xen/common/device_tree.c | 44 +++++++++++++++---------------
>>> xen/common/libelf/libelf-private.h | 6 ++--
>>> xen/common/xz/private.h | 2 +-
>>> xen/include/xen/unaligned.h | 24 ++++++++--------
>>> 4 files changed, 38 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 4aae281e89..70d3be3be6 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>> if ( !val || len < sizeof(*out_value) )
>>> return 0;
>>>
>>> - *out_value = be32_to_cpup(val);
>>> + *out_value = be32_to_cpu(*val);
>>> This code has been taken from Linux and I would rather prefer to keep
>>> the *cpup* helpers to avoid any changes when backporting.
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..16b2e6f5f0 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -20,62 +20,62 @@
>>>
>>> static inline uint16_t get_unaligned_be16(const void *p)
>>> {
>>> - return be16_to_cpup(p);
>>> + return be16_to_cpu(*(const uint16_t *)p)
>>> I haven't checked the existing implementation of be16_to_cpup().
>>> However, this new approach would allow the compiler to use a single load
>>> instruction to read the 16-bit value from memory. So this change may
>>> break on platform where unaligned access is forbidden (such as arm32).
>>> }
>>>
>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>> {
>>> - *(__force __be16*)p = cpu_to_be16(val);
>>> + *(__be16 *)p = cpu_to_be16(val);
>>>> Why did you drop the __force?
>> Google told me __force is used in linux kernel to suppress warning in sparse,
>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>> Is sparse also used in xen?
>
> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>
> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
8<—
While here:
- Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool
—>8
I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.
-George
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 12:11 ` George Dunlap
@ 2022-05-11 12:39 ` Julien Grall
2022-05-11 14:21 ` Bertrand Marquis
1 sibling, 0 replies; 34+ messages in thread
From: Julien Grall @ 2022-05-11 12:39 UTC (permalink / raw)
To: George Dunlap
Cc: Lin Liu （刘林）,
xen-devel, Stefano Stabellini, Andrew Cooper, Jan Beulich,
Wei Liu
Hi George,
On 11/05/2022 13:11, George Dunlap wrote:
>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>> Is sparse also used in xen?
>>
>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>>
>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
>
> I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
I suggested to drop from the patch because I don't think we should
remove the __force. In fact, I have contemplated a few times in the past
to use sparse in Xen.
Sorry I should have been clearer.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 12:11 ` George Dunlap
2022-05-11 12:39 ` Julien Grall
@ 2022-05-11 14:21 ` Bertrand Marquis
2022-05-17 14:59 ` Jan Beulich
1 sibling, 1 reply; 34+ messages in thread
From: Bertrand Marquis @ 2022-05-11 14:21 UTC (permalink / raw)
To: George Dunlap
Cc: Julien Grall, Lin Liu （刘林）,
xen-devel, Stefano Stabellini, Andrew Cooper, Jan Beulich,
Wei Liu
Hi George,
> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@citrix.com> wrote:
>
>
>
>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> Please configure your e-mail client to send in plain text.
>>
>> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>> Update to use byteswap to swap bytes.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien@xen.org>
>>>> 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>
>>>> Changes in v3:
>>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>>> - Keep const in type cast in unaligned.h
>>>> ---
>>>> xen/common/device_tree.c | 44 +++++++++++++++---------------
>>>> xen/common/libelf/libelf-private.h | 6 ++--
>>>> xen/common/xz/private.h | 2 +-
>>>> xen/include/xen/unaligned.h | 24 ++++++++--------
>>>> 4 files changed, 38 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index 4aae281e89..70d3be3be6 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>>> if ( !val || len < sizeof(*out_value) )
>>>> return 0;
>>>>
>>>> - *out_value = be32_to_cpup(val);
>>>> + *out_value = be32_to_cpu(*val);
>>>> This code has been taken from Linux and I would rather prefer to keep
>>>> the *cpup* helpers to avoid any changes when backporting.
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -20,62 +20,62 @@
>>>>
>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>> {
>>>> - return be16_to_cpup(p);
>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>> However, this new approach would allow the compiler to use a single load
>>>> instruction to read the 16-bit value from memory. So this change may
>>>> break on platform where unaligned access is forbidden (such as arm32).
>>>> }
>>>>
>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>> {
>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>> Why did you drop the __force?
>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>> Is sparse also used in xen?
>>
>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>>
>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
>
> I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
>
> 8<—
>
> While here:
> - Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool
>
> —>8
>
> I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.
I fully agree here. The effort involved by splitting a patch in several ones (both for the
contributor and the maintainers) means it should be prevented unless the original patch
could not be reviewed as is (patch to long or to complex to test for example but there
might be other valid cases).
Modifying the commit message to describe all changes is definitely mandatory
though (but could be done without a v+1).
Bertrand
>
> -George
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-11 14:21 ` Bertrand Marquis
@ 2022-05-17 14:59 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2022-05-17 14:59 UTC (permalink / raw)
To: Bertrand Marquis, George Dunlap
Cc: Julien Grall, Lin Liu （刘林）,
xen-devel, Stefano Stabellini, Andrew Cooper, Wei Liu
On 11.05.2022 16:21, Bertrand Marquis wrote:
>> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@citrix.com> wrote:
>>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
>>> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>>> {
>>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>>> Why did you drop the __force?
>>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>>> Is sparse also used in xen?
>>>
>>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>>>
>>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
>>
>> I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
>>
>> 8<—
>>
>> While here:
>> - Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool
>>
>> —>8
>>
>> I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.
>
> I fully agree here. The effort involved by splitting a patch in several ones (both for the
> contributor and the maintainers) means it should be prevented unless the original patch
> could not be reviewed as is (patch to long or to complex to test for example but there
> might be other valid cases).
This is fine for uncontroversial changes, but not in cases like this
one. Like Julien, I think we should rather strive towards completing
the Linux-inherited annotations like __force. Even without the actual
use of sparse they serve a documentation purpose.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/6] xen: Switch to byteswap
2022-05-10 10:15 ` [PATCH v3 4/6] xen: Switch to byteswap Lin Liu
2022-05-10 10:51 ` Julien Grall
@ 2022-05-10 14:32 ` Anthony PERARD
1 sibling, 0 replies; 34+ messages in thread
From: Anthony PERARD @ 2022-05-10 14:32 UTC (permalink / raw)
To: Lin Liu
Cc: xen-devel, Stefano Stabellini, Julien Grall, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
On Tue, May 10, 2022 at 06:15:22AM -0400, Lin Liu wrote:
> Update to use byteswap to swap bytes.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
FYI, this patch breaks build of stubdomain:
In file included from /var/tmp/git.xen.lU52/stubdom/include/../../xen/common/unxz.c:124,
from xg_dom_decompress_unsafe_xz.c:40:
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/dec_stream.c: In function ‘dec_stream_header’:
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/private.h:31:21: error: implicit declaration of function ‘le32_to_cpu’; did you mean ‘le32_to_cpup’? [-Werror=implicit-function-declaration]
31 | #define get_le32(p) le32_to_cpu(*(const uint32_t *)(p))
| ^~~~~~~~~~~
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/dec_stream.c:393:7: note: in expansion of macro ‘get_le32’
393 | != get_le32(s->temp.buf + HEADER_MAGIC_SIZE + 2))
| ^~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [/var/tmp/git.xen.lU52/stubdom/libs-x86_64/guest/../../../tools/Rules.mk:150: xg_dom_decompress_unsafe_xz.o] Error 1
make[1]: *** [Makefile:367: libs-x86_64/guest/libxenguest.a] Error 2
make: *** [Makefile:73: build-stubdom] Error 2
Cheers,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 5/6] byteorder: Remove byteorder
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
` (3 preceding siblings ...)
2022-05-10 10:15 ` [PATCH v3 4/6] xen: Switch to byteswap Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-10 11:09 ` Andrew Cooper
2022-05-10 10:15 ` [PATCH v3 6/6] tools: Remove unnecessary header Lin Liu
5 siblings, 1 reply; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel
Cc: Lin Liu, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
include/xen/byteswap.h has simplify the interface, just clean
the old interface
No functional change
Signed-off-by: Lin Liu <lin.liu@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
---
xen/include/xen/byteorder/big_endian.h | 102 ------------
xen/include/xen/byteorder/generic.h | 68 --------
xen/include/xen/byteorder/little_endian.h | 102 ------------
xen/include/xen/byteorder/swab.h | 183 ----------------------
4 files changed, 455 deletions(-)
delete mode 100644 xen/include/xen/byteorder/big_endian.h
delete mode 100644 xen/include/xen/byteorder/generic.h
delete mode 100644 xen/include/xen/byteorder/little_endian.h
delete mode 100644 xen/include/xen/byteorder/swab.h
diff --git a/xen/include/xen/byteorder/big_endian.h b/xen/include/xen/byteorder/big_endian.h
deleted file mode 100644
index 40eb80a390..0000000000
--- a/xen/include/xen/byteorder/big_endian.h
+++ /dev/null
@@ -1,102 +0,0 @@
-#ifndef __XEN_BYTEORDER_BIG_ENDIAN_H__
-#define __XEN_BYTEORDER_BIG_ENDIAN_H__
-
-#ifndef __BIG_ENDIAN
-#define __BIG_ENDIAN 4321
-#endif
-#ifndef __BIG_ENDIAN_BITFIELD
-#define __BIG_ENDIAN_BITFIELD
-#endif
-
-#include <xen/types.h>
-#include <xen/byteorder/swab.h>
-
-#define __constant_cpu_to_le64(x) ((__force __le64)___constant_swab64((x)))
-#define __constant_le64_to_cpu(x) ___constant_swab64((__force __u64)(__le64)(x))
-#define __constant_cpu_to_le32(x) ((__force __le32)___constant_swab32((x)))
-#define __constant_le32_to_cpu(x) ___constant_swab32((__force __u32)(__le32)(x))
-#define __constant_cpu_to_le16(x) ((__force __le16)___constant_swab16((x)))
-#define __constant_le16_to_cpu(x) ___constant_swab16((__force __u16)(__le16)(x))
-#define __constant_cpu_to_be64(x) ((__force __be64)(__u64)(x))
-#define __constant_be64_to_cpu(x) ((__force __u64)(__be64)(x))
-#define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x))
-#define __constant_be32_to_cpu(x) ((__force __u32)(__be32)(x))
-#define __constant_cpu_to_be16(x) ((__force __be16)(__u16)(x))
-#define __constant_be16_to_cpu(x) ((__force __u16)(__be16)(x))
-#define __cpu_to_le64(x) ((__force __le64)__swab64((x)))
-#define __le64_to_cpu(x) __swab64((__force __u64)(__le64)(x))
-#define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
-#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
-#define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
-#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
-#define __cpu_to_be64(x) ((__force __be64)(__u64)(x))
-#define __be64_to_cpu(x) ((__force __u64)(__be64)(x))
-#define __cpu_to_be32(x) ((__force __be32)(__u32)(x))
-#define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
-#define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
-#define __be16_to_cpu(x) ((__force __u16)(__be16)(x))
-
-static inline __le64 __cpu_to_le64p(const __u64 *p)
-{
- return (__force __le64)__swab64p(p);
-}
-static inline __u64 __le64_to_cpup(const __le64 *p)
-{
- return __swab64p((__u64 *)p);
-}
-static inline __le32 __cpu_to_le32p(const __u32 *p)
-{
- return (__force __le32)__swab32p(p);
-}
-static inline __u32 __le32_to_cpup(const __le32 *p)
-{
- return __swab32p((__u32 *)p);
-}
-static inline __le16 __cpu_to_le16p(const __u16 *p)
-{
- return (__force __le16)__swab16p(p);
-}
-static inline __u16 __le16_to_cpup(const __le16 *p)
-{
- return __swab16p((__u16 *)p);
-}
-static inline __be64 __cpu_to_be64p(const __u64 *p)
-{
- return (__force __be64)*p;
-}
-static inline __u64 __be64_to_cpup(const __be64 *p)
-{
- return (__force __u64)*p;
-}
-static inline __be32 __cpu_to_be32p(const __u32 *p)
-{
- return (__force __be32)*p;
-}
-static inline __u32 __be32_to_cpup(const __be32 *p)
-{
- return (__force __u32)*p;
-}
-static inline __be16 __cpu_to_be16p(const __u16 *p)
-{
- return (__force __be16)*p;
-}
-static inline __u16 __be16_to_cpup(const __be16 *p)
-{
- return (__force __u16)*p;
-}
-#define __cpu_to_le64s(x) __swab64s((x))
-#define __le64_to_cpus(x) __swab64s((x))
-#define __cpu_to_le32s(x) __swab32s((x))
-#define __le32_to_cpus(x) __swab32s((x))
-#define __cpu_to_le16s(x) __swab16s((x))
-#define __le16_to_cpus(x) __swab16s((x))
-#define __cpu_to_be64s(x) do {} while (0)
-#define __be64_to_cpus(x) do {} while (0)
-#define __cpu_to_be32s(x) do {} while (0)
-#define __be32_to_cpus(x) do {} while (0)
-#define __cpu_to_be16s(x) do {} while (0)
-#define __be16_to_cpus(x) do {} while (0)
-
-#include <xen/byteorder/generic.h>
-
-#endif /* __XEN_BYTEORDER_BIG_ENDIAN_H__ */
diff --git a/xen/include/xen/byteorder/generic.h b/xen/include/xen/byteorder/generic.h
deleted file mode 100644
index 8a0006b755..0000000000
--- a/xen/include/xen/byteorder/generic.h
+++ /dev/null
@@ -1,68 +0,0 @@
-#ifndef __XEN_BYTEORDER_GENERIC_H__
-#define __XEN_BYTEORDER_GENERIC_H__
-
-/*
- * Generic Byte-reordering support
- *
- * The "... p" macros, like le64_to_cpup, can be used with pointers
- * to unaligned data, but there will be a performance penalty on
- * some architectures. Use get_unaligned for unaligned data.
- *
- * The following macros are to be defined by <asm/byteorder.h>:
- *
- * Conversion of XX-bit integers (16- 32- or 64-)
- * between native CPU format and little/big endian format
- * 64-bit stuff only defined for proper architectures
- * cpu_to_[bl]eXX(__uXX x)
- * [bl]eXX_to_cpu(__uXX x)
- *
- * The same, but takes a pointer to the value to convert
- * cpu_to_[bl]eXXp(__uXX x)
- * [bl]eXX_to_cpup(__uXX x)
- *
- * The same, but change in situ
- * cpu_to_[bl]eXXs(__uXX x)
- * [bl]eXX_to_cpus(__uXX x)
- *
- * See asm-foo/byteorder.h for examples of how to provide
- * architecture-optimized versions
- */
-
-#define cpu_to_le64 __cpu_to_le64
-#define le64_to_cpu __le64_to_cpu
-#define cpu_to_le32 __cpu_to_le32
-#define le32_to_cpu __le32_to_cpu
-#define cpu_to_le16 __cpu_to_le16
-#define le16_to_cpu __le16_to_cpu
-#define cpu_to_be64 __cpu_to_be64
-#define be64_to_cpu __be64_to_cpu
-#define cpu_to_be32 __cpu_to_be32
-#define be32_to_cpu __be32_to_cpu
-#define cpu_to_be16 __cpu_to_be16
-#define be16_to_cpu __be16_to_cpu
-#define cpu_to_le64p __cpu_to_le64p
-#define le64_to_cpup __le64_to_cpup
-#define cpu_to_le32p __cpu_to_le32p
-#define le32_to_cpup __le32_to_cpup
-#define cpu_to_le16p __cpu_to_le16p
-#define le16_to_cpup __le16_to_cpup
-#define cpu_to_be64p __cpu_to_be64p
-#define be64_to_cpup __be64_to_cpup
-#define cpu_to_be32p __cpu_to_be32p
-#define be32_to_cpup __be32_to_cpup
-#define cpu_to_be16p __cpu_to_be16p
-#define be16_to_cpup __be16_to_cpup
-#define cpu_to_le64s __cpu_to_le64s
-#define le64_to_cpus __le64_to_cpus
-#define cpu_to_le32s __cpu_to_le32s
-#define le32_to_cpus __le32_to_cpus
-#define cpu_to_le16s __cpu_to_le16s
-#define le16_to_cpus __le16_to_cpus
-#define cpu_to_be64s __cpu_to_be64s
-#define be64_to_cpus __be64_to_cpus
-#define cpu_to_be32s __cpu_to_be32s
-#define be32_to_cpus __be32_to_cpus
-#define cpu_to_be16s __cpu_to_be16s
-#define be16_to_cpus __be16_to_cpus
-
-#endif /* __XEN_BYTEORDER_GENERIC_H__ */
diff --git a/xen/include/xen/byteorder/little_endian.h b/xen/include/xen/byteorder/little_endian.h
deleted file mode 100644
index 4955632793..0000000000
--- a/xen/include/xen/byteorder/little_endian.h
+++ /dev/null
@@ -1,102 +0,0 @@
-#ifndef __XEN_BYTEORDER_LITTLE_ENDIAN_H__
-#define __XEN_BYTEORDER_LITTLE_ENDIAN_H__
-
-#ifndef __LITTLE_ENDIAN
-#define __LITTLE_ENDIAN 1234
-#endif
-#ifndef __LITTLE_ENDIAN_BITFIELD
-#define __LITTLE_ENDIAN_BITFIELD
-#endif
-
-#include <xen/types.h>
-#include <xen/byteorder/swab.h>
-
-#define __constant_cpu_to_le64(x) ((__force __le64)(__u64)(x))
-#define __constant_le64_to_cpu(x) ((__force __u64)(__le64)(x))
-#define __constant_cpu_to_le32(x) ((__force __le32)(__u32)(x))
-#define __constant_le32_to_cpu(x) ((__force __u32)(__le32)(x))
-#define __constant_cpu_to_le16(x) ((__force __le16)(__u16)(x))
-#define __constant_le16_to_cpu(x) ((__force __u16)(__le16)(x))
-#define __constant_cpu_to_be64(x) ((__force __be64)___constant_swab64((x)))
-#define __constant_be64_to_cpu(x) ___constant_swab64((__force __u64)(__be64)(x))
-#define __constant_cpu_to_be32(x) ((__force __be32)___constant_swab32((x)))
-#define __constant_be32_to_cpu(x) ___constant_swab32((__force __u32)(__be32)(x))
-#define __constant_cpu_to_be16(x) ((__force __be16)___constant_swab16((x)))
-#define __constant_be16_to_cpu(x) ___constant_swab16((__force __u16)(__be16)(x))
-#define __cpu_to_le64(x) ((__force __le64)(__u64)(x))
-#define __le64_to_cpu(x) ((__force __u64)(__le64)(x))
-#define __cpu_to_le32(x) ((__force __le32)(__u32)(x))
-#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
-#define __cpu_to_le16(x) ((__force __le16)(__u16)(x))
-#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
-#define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
-#define __be64_to_cpu(x) __swab64((__force __u64)(__be64)(x))
-#define __cpu_to_be32(x) ((__force __be32)__swab32((x)))
-#define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
-#define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
-#define __be16_to_cpu(x) __swab16((__force __u16)(__be16)(x))
-
-static inline __le64 __cpu_to_le64p(const __u64 *p)
-{
- return (__force __le64)*p;
-}
-static inline __u64 __le64_to_cpup(const __le64 *p)
-{
- return (__force __u64)*p;
-}
-static inline __le32 __cpu_to_le32p(const __u32 *p)
-{
- return (__force __le32)*p;
-}
-static inline __u32 __le32_to_cpup(const __le32 *p)
-{
- return (__force __u32)*p;
-}
-static inline __le16 __cpu_to_le16p(const __u16 *p)
-{
- return (__force __le16)*p;
-}
-static inline __u16 __le16_to_cpup(const __le16 *p)
-{
- return (__force __u16)*p;
-}
-static inline __be64 __cpu_to_be64p(const __u64 *p)
-{
- return (__force __be64)__swab64p(p);
-}
-static inline __u64 __be64_to_cpup(const __be64 *p)
-{
- return __swab64p((__u64 *)p);
-}
-static inline __be32 __cpu_to_be32p(const __u32 *p)
-{
- return (__force __be32)__swab32p(p);
-}
-static inline __u32 __be32_to_cpup(const __be32 *p)
-{
- return __swab32p((__u32 *)p);
-}
-static inline __be16 __cpu_to_be16p(const __u16 *p)
-{
- return (__force __be16)__swab16p(p);
-}
-static inline __u16 __be16_to_cpup(const __be16 *p)
-{
- return __swab16p((__u16 *)p);
-}
-#define __cpu_to_le64s(x) do {} while (0)
-#define __le64_to_cpus(x) do {} while (0)
-#define __cpu_to_le32s(x) do {} while (0)
-#define __le32_to_cpus(x) do {} while (0)
-#define __cpu_to_le16s(x) do {} while (0)
-#define __le16_to_cpus(x) do {} while (0)
-#define __cpu_to_be64s(x) __swab64s((x))
-#define __be64_to_cpus(x) __swab64s((x))
-#define __cpu_to_be32s(x) __swab32s((x))
-#define __be32_to_cpus(x) __swab32s((x))
-#define __cpu_to_be16s(x) __swab16s((x))
-#define __be16_to_cpus(x) __swab16s((x))
-
-#include <xen/byteorder/generic.h>
-
-#endif /* __XEN_BYTEORDER_LITTLE_ENDIAN_H__ */
diff --git a/xen/include/xen/byteorder/swab.h b/xen/include/xen/byteorder/swab.h
deleted file mode 100644
index b7e30f0503..0000000000
--- a/xen/include/xen/byteorder/swab.h
+++ /dev/null
@@ -1,183 +0,0 @@
-#ifndef __XEN_BYTEORDER_SWAB_H__
-#define __XEN_BYTEORDER_SWAB_H__
-
-/*
- * Byte-swapping, independently from CPU endianness
- * swabXX[ps]?(foo)
- *
- * Francois-Rene Rideau <fare@tunes.org> 19971205
- * separated swab functions from cpu_to_XX,
- * to clean up support for bizarre-endian architectures.
- */
-
-/* casts are necessary for constants, because we never know how for sure
- * how U/UL/ULL map to __u16, __u32, __u64. At least not in a portable way.
- */
-#define ___swab16(x) \
-({ \
- __u16 __x = (x); \
- ((__u16)( \
- (((__u16)(__x) & (__u16)0x00ffU) << 8) | \
- (((__u16)(__x) & (__u16)0xff00U) >> 8) )); \
-})
-
-#define ___swab32(x) \
-({ \
- __u32 __x = (x); \
- ((__u32)( \
- (((__u32)(__x) & (__u32)0x000000ffUL) << 24) | \
- (((__u32)(__x) & (__u32)0x0000ff00UL) << 8) | \
- (((__u32)(__x) & (__u32)0x00ff0000UL) >> 8) | \
- (((__u32)(__x) & (__u32)0xff000000UL) >> 24) )); \
-})
-
-#define ___swab64(x) \
-({ \
- __u64 __x = (x); \
- ((__u64)( \
- (__u64)(((__u64)(__x) & (__u64)0x00000000000000ffULL) << 56) | \
- (__u64)(((__u64)(__x) & (__u64)0x000000000000ff00ULL) << 40) | \
- (__u64)(((__u64)(__x) & (__u64)0x0000000000ff0000ULL) << 24) | \
- (__u64)(((__u64)(__x) & (__u64)0x00000000ff000000ULL) << 8) | \
- (__u64)(((__u64)(__x) & (__u64)0x000000ff00000000ULL) >> 8) | \
- (__u64)(((__u64)(__x) & (__u64)0x0000ff0000000000ULL) >> 24) | \
- (__u64)(((__u64)(__x) & (__u64)0x00ff000000000000ULL) >> 40) | \
- (__u64)(((__u64)(__x) & (__u64)0xff00000000000000ULL) >> 56) )); \
-})
-
-#define ___constant_swab16(x) \
- ((__u16)( \
- (((__u16)(x) & (__u16)0x00ffU) << 8) | \
- (((__u16)(x) & (__u16)0xff00U) >> 8) ))
-#define ___constant_swab32(x) \
- ((__u32)( \
- (((__u32)(x) & (__u32)0x000000ffUL) << 24) | \
- (((__u32)(x) & (__u32)0x0000ff00UL) << 8) | \
- (((__u32)(x) & (__u32)0x00ff0000UL) >> 8) | \
- (((__u32)(x) & (__u32)0xff000000UL) >> 24) ))
-#define ___constant_swab64(x) \
- ((__u64)( \
- (__u64)(((__u64)(x) & (__u64)0x00000000000000ffULL) << 56) | \
- (__u64)(((__u64)(x) & (__u64)0x000000000000ff00ULL) << 40) | \
- (__u64)(((__u64)(x) & (__u64)0x0000000000ff0000ULL) << 24) | \
- (__u64)(((__u64)(x) & (__u64)0x00000000ff000000ULL) << 8) | \
- (__u64)(((__u64)(x) & (__u64)0x000000ff00000000ULL) >> 8) | \
- (__u64)(((__u64)(x) & (__u64)0x0000ff0000000000ULL) >> 24) | \
- (__u64)(((__u64)(x) & (__u64)0x00ff000000000000ULL) >> 40) | \
- (__u64)(((__u64)(x) & (__u64)0xff00000000000000ULL) >> 56) ))
-
-/*
- * provide defaults when no architecture-specific optimization is detected
- */
-#ifndef __arch__swab16
-# define __arch__swab16(x) ({ __u16 __tmp = (x) ; ___swab16(__tmp); })
-#endif
-#ifndef __arch__swab32
-# define __arch__swab32(x) ({ __u32 __tmp = (x) ; ___swab32(__tmp); })
-#endif
-#ifndef __arch__swab64
-# define __arch__swab64(x) ({ __u64 __tmp = (x) ; ___swab64(__tmp); })
-#endif
-
-#ifndef __arch__swab16p
-# define __arch__swab16p(x) __arch__swab16(*(x))
-#endif
-#ifndef __arch__swab32p
-# define __arch__swab32p(x) __arch__swab32(*(x))
-#endif
-#ifndef __arch__swab64p
-# define __arch__swab64p(x) __arch__swab64(*(x))
-#endif
-
-#ifndef __arch__swab16s
-# define __arch__swab16s(x) do { *(x) = __arch__swab16p((x)); } while (0)
-#endif
-#ifndef __arch__swab32s
-# define __arch__swab32s(x) do { *(x) = __arch__swab32p((x)); } while (0)
-#endif
-#ifndef __arch__swab64s
-# define __arch__swab64s(x) do { *(x) = __arch__swab64p((x)); } while (0)
-#endif
-
-
-/*
- * Allow constant folding
- */
-#if defined(__GNUC__) && defined(__OPTIMIZE__)
-# define __swab16(x) \
-(__builtin_constant_p((__u16)(x)) ? \
- ___swab16((x)) : \
- __fswab16((x)))
-# define __swab32(x) \
-(__builtin_constant_p((__u32)(x)) ? \
- ___swab32((x)) : \
- __fswab32((x)))
-# define __swab64(x) \
-(__builtin_constant_p((__u64)(x)) ? \
- ___swab64((x)) : \
- __fswab64((x)))
-#else
-# define __swab16(x) __fswab16(x)
-# define __swab32(x) __fswab32(x)
-# define __swab64(x) __fswab64(x)
-#endif /* OPTIMIZE */
-
-
-static inline __attribute_const__ __u16 __fswab16(__u16 x)
-{
- return __arch__swab16(x);
-}
-static inline __u16 __swab16p(const __u16 *x)
-{
- return __arch__swab16p(x);
-}
-static inline void __swab16s(__u16 *addr)
-{
- __arch__swab16s(addr);
-}
-
-static inline __attribute_const__ __u32 __fswab32(__u32 x)
-{
- return __arch__swab32(x);
-}
-static inline __u32 __swab32p(const __u32 *x)
-{
- return __arch__swab32p(x);
-}
-static inline void __swab32s(__u32 *addr)
-{
- __arch__swab32s(addr);
-}
-
-#ifdef __BYTEORDER_HAS_U64__
-static inline __attribute_const__ __u64 __fswab64(__u64 x)
-{
-# ifdef __SWAB_64_THRU_32__
- __u32 h = x >> 32;
- __u32 l = x & ((1ULL<<32)-1);
- return (((__u64)__swab32(l)) << 32) | ((__u64)(__swab32(h)));
-# else
- return __arch__swab64(x);
-# endif
-}
-static inline __u64 __swab64p(const __u64 *x)
-{
- return __arch__swab64p(x);
-}
-static inline void __swab64s(__u64 *addr)
-{
- __arch__swab64s(addr);
-}
-#endif /* __BYTEORDER_HAS_U64__ */
-
-#define swab16 __swab16
-#define swab32 __swab32
-#define swab64 __swab64
-#define swab16p __swab16p
-#define swab32p __swab32p
-#define swab64p __swab64p
-#define swab16s __swab16s
-#define swab32s __swab32s
-#define swab64s __swab64s
-
-#endif /* __XEN_BYTEORDER_SWAB_H__ */
--
2.27.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 5/6] byteorder: Remove byteorder
2022-05-10 10:15 ` [PATCH v3 5/6] byteorder: Remove byteorder Lin Liu
@ 2022-05-10 11:09 ` Andrew Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2022-05-10 11:09 UTC (permalink / raw)
To: Lin Liu, xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
On 10/05/2022 11:15, Lin Liu wrote:
> include/xen/byteswap.h has simplify the interface, just clean
> the old interface
>
> No functional change
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Wei Liu <wl@xen.org>
> ---
> xen/include/xen/byteorder/big_endian.h | 102 ------------
> xen/include/xen/byteorder/generic.h | 68 --------
> xen/include/xen/byteorder/little_endian.h | 102 ------------
> xen/include/xen/byteorder/swab.h | 183 ----------------------
> 4 files changed, 455 deletions(-)
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Good riddance.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 6/6] tools: Remove unnecessary header
2022-05-10 10:15 [PATCH v3 0/6] Implement byteswap and update references Lin Liu
` (4 preceding siblings ...)
2022-05-10 10:15 ` [PATCH v3 5/6] byteorder: Remove byteorder Lin Liu
@ 2022-05-10 10:15 ` Lin Liu
2022-05-17 15:01 ` Jan Beulich
5 siblings, 1 reply; 34+ messages in thread
From: Lin Liu @ 2022-05-10 10:15 UTC (permalink / raw)
To: xen-devel; +Cc: Lin Liu, Wei Liu, Anthony PERARD, Juergen Gross
xen/byteorder/little_endian.h is included but not used, Remove it.
No functional change.
Signed-off-by: Lin Liu <lin.liu@citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
tools/libs/guest/xg_dom_decompress_unsafe_zstd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c b/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
index 01eafaaaa6..47e071574d 100644
--- a/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
+++ b/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
@@ -31,7 +31,6 @@ typedef uint64_t __be64;
#define __BYTEORDER_HAS_U64__
#define __TYPES_H__ /* xen/types.h guard */
-#include "../../xen/include/xen/byteorder/little_endian.h"
#define __ASM_UNALIGNED_H__ /* asm/unaligned.h guard */
#include "../../xen/include/xen/unaligned.h"
#include "../../xen/include/xen/xxhash.h"
--
2.27.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 6/6] tools: Remove unnecessary header
2022-05-10 10:15 ` [PATCH v3 6/6] tools: Remove unnecessary header Lin Liu
@ 2022-05-17 15:01 ` Jan Beulich
2022-05-17 15:18 ` 回复: " Lin Liu （刘林）
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2022-05-17 15:01 UTC (permalink / raw)
To: Lin Liu; +Cc: Wei Liu, Anthony PERARD, Juergen Gross, xen-devel
On 10.05.2022 12:15, Lin Liu wrote:
> --- a/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
> +++ b/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
> @@ -31,7 +31,6 @@ typedef uint64_t __be64;
>
> #define __BYTEORDER_HAS_U64__
> #define __TYPES_H__ /* xen/types.h guard */
> -#include "../../xen/include/xen/byteorder/little_endian.h"
> #define __ASM_UNALIGNED_H__ /* asm/unaligned.h guard */
> #include "../../xen/include/xen/unaligned.h"
> #include "../../xen/include/xen/xxhash.h"
Doesn't this need to come ahead of what is now patch 5?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* 回复: [PATCH v3 6/6] tools: Remove unnecessary header
2022-05-17 15:01 ` Jan Beulich
@ 2022-05-17 15:18 ` Lin Liu （刘林）
0 siblings, 0 replies; 34+ messages in thread
From: Lin Liu （刘林） @ 2022-05-17 15:18 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Anthony Perard, Juergen Gross, xen-devel
On 10.05.2022 12:15, Lin Liu wrote:
>> --- a/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
>> +++ b/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
>> @@ -31,7 +31,6 @@ typedef uint64_t __be64;
>>
>> #define __BYTEORDER_HAS_U64__
>> #define __TYPES_H__ /* xen/types.h guard */ -#include
>> "../../xen/include/xen/byteorder/little_endian.h"
>> #define __ASM_UNALIGNED_H__ /* asm/unaligned.h guard */ #include
>> "../../xen/include/xen/unaligned.h"
>> #include "../../xen/include/xen/xxhash.h"
> Doesn't this need to come ahead of what is now patch 5?
> Jan
Thanks for the feedback,
I am preparing a new patch series for this.
^ permalink raw reply [flat|nested] 34+ messages in thread