[PATCH v3 0/2] Implement endianess swap macros for RISC-V

[PATCH v3 0/2] Implement endianess swap macros for RISC-V

[Ignacio Encinas]

Motivated by [1]. A couple of things to note:

RISC-V needs a default implementation to fall back on. There is one
available in include/uapi/linux/swab.h but that header can't be included
from arch/riscv/include/asm/swab.h. Therefore, the first patch in this
series moves the default implementation into asm-generic.

Tested with crc_kunit as pointed out here [2]. I can't provide
performance numbers as I don't have RISC-V hardware yet.

[1] https://lore.kernel.org/all/20250302220426.GC2079@quark.localdomain/
[2] https://lore.kernel.org/all/20250216225530.306980-1-ebiggers@kernel.org/

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
Changes in v3:

PATCH 2:
  Use if(riscv_has_extension_likely) instead of asm goto (Eric). It 
  looks like both versions generate the same assembly. Perhaps we should 
  do the same change in other places such as arch/riscv/include/asm/bitops.h

- Link to v2: https://lore.kernel.org/r/20250319-riscv-swab-v2-0-d53b6d6ab915@iencinas.com

Arnd, I tried your suggestion but couldn't make it work. Let me know if
I missed something in my response.

Changes in v2:
- Introduce first patch factoring out the default implementation into
  asm-generic
- Remove blank line to make checkpatch happy
- Link to v1: https://lore.kernel.org/r/20250310-riscv-swab-v1-1-34652ef1ee96@iencinas.com

---
Ignacio Encinas (2):
      include/uapi/linux/swab.h: move default implementation for swab macros into asm-generic
      riscv: introduce asm/swab.h

 arch/riscv/include/asm/swab.h   | 43 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/asm-generic/swab.h | 32 ++++++++++++++++++++++++++++++
 include/uapi/linux/swab.h       | 33 +------------------------------
 3 files changed, 76 insertions(+), 32 deletions(-)
---
base-commit: a7f2e10ecd8f18b83951b0bab47ddaf48f93bf47
change-id: 20250307-riscv-swab-b81b94a9ac1b

Best regards,
-- 
Ignacio Encinas <ignacio@iencinas.com>

[PATCH v3 1/2] include/uapi/linux/swab.h: move default implementation for swab macros into asm-generic

[Ignacio Encinas]

Move the default byteswap implementation into asm-generic so that it can
be included from arch code.

This is required by RISC-V in order to have a fallback implementation
without duplicating it.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 include/uapi/asm-generic/swab.h | 32 ++++++++++++++++++++++++++++++++
 include/uapi/linux/swab.h       | 33 +--------------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/include/uapi/asm-generic/swab.h b/include/uapi/asm-generic/swab.h
index f2da4e4fd4d1..43d83df007a6 100644
--- a/include/uapi/asm-generic/swab.h
+++ b/include/uapi/asm-generic/swab.h
@@ -16,4 +16,36 @@
 #endif
 #endif
 
+/*
+ * 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 ___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)(x) & (__u64)0x00000000000000ffULL) << 56) |	\
+	(((__u64)(x) & (__u64)0x000000000000ff00ULL) << 40) |	\
+	(((__u64)(x) & (__u64)0x0000000000ff0000ULL) << 24) |	\
+	(((__u64)(x) & (__u64)0x00000000ff000000ULL) <<  8) |	\
+	(((__u64)(x) & (__u64)0x000000ff00000000ULL) >>  8) |	\
+	(((__u64)(x) & (__u64)0x0000ff0000000000ULL) >> 24) |	\
+	(((__u64)(x) & (__u64)0x00ff000000000000ULL) >> 40) |	\
+	(((__u64)(x) & (__u64)0xff00000000000000ULL) >> 56)))
+
+#define ___constant_swahw32(x) ((__u32)(			\
+	(((__u32)(x) & (__u32)0x0000ffffUL) << 16) |		\
+	(((__u32)(x) & (__u32)0xffff0000UL) >> 16)))
+
+#define ___constant_swahb32(x) ((__u32)(			\
+	(((__u32)(x) & (__u32)0x00ff00ffUL) << 8) |		\
+	(((__u32)(x) & (__u32)0xff00ff00UL) >> 8)))
+
 #endif /* _ASM_GENERIC_SWAB_H */
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 01717181339e..ca808c492996 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -6,38 +6,7 @@
 #include <linux/stddef.h>
 #include <asm/bitsperlong.h>
 #include <asm/swab.h>
-
-/*
- * 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 ___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)(x) & (__u64)0x00000000000000ffULL) << 56) |	\
-	(((__u64)(x) & (__u64)0x000000000000ff00ULL) << 40) |	\
-	(((__u64)(x) & (__u64)0x0000000000ff0000ULL) << 24) |	\
-	(((__u64)(x) & (__u64)0x00000000ff000000ULL) <<  8) |	\
-	(((__u64)(x) & (__u64)0x000000ff00000000ULL) >>  8) |	\
-	(((__u64)(x) & (__u64)0x0000ff0000000000ULL) >> 24) |	\
-	(((__u64)(x) & (__u64)0x00ff000000000000ULL) >> 40) |	\
-	(((__u64)(x) & (__u64)0xff00000000000000ULL) >> 56)))
-
-#define ___constant_swahw32(x) ((__u32)(			\
-	(((__u32)(x) & (__u32)0x0000ffffUL) << 16) |		\
-	(((__u32)(x) & (__u32)0xffff0000UL) >> 16)))
-
-#define ___constant_swahb32(x) ((__u32)(			\
-	(((__u32)(x) & (__u32)0x00ff00ffUL) << 8) |		\
-	(((__u32)(x) & (__u32)0xff00ff00UL) >> 8)))
+#include <asm-generic/swab.h>
 
 /*
  * Implement the following as inlines, but define the interface using

-- 
2.49.0

[PATCH v3 2/2] riscv: introduce asm/swab.h

[Ignacio Encinas]

Implement endianness swap macros for RISC-V.

Use the rev8 instruction when Zbb is available. Otherwise, rely on the
default mask-and-shift implementation.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 arch/riscv/include/asm/swab.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/riscv/include/asm/swab.h b/arch/riscv/include/asm/swab.h
new file mode 100644
index 000000000000..7352e8405a99
--- /dev/null
+++ b/arch/riscv/include/asm/swab.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASM_RISCV_SWAB_H
+#define _ASM_RISCV_SWAB_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <asm/cpufeature-macros.h>
+#include <asm/hwcap.h>
+#include <asm-generic/swab.h>
+
+#if defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE)
+
+#define ARCH_SWAB(size) \
+static __always_inline unsigned long __arch_swab##size(__u##size value) \
+{									\
+	unsigned long x = value;					\
+									\
+	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
+		asm volatile (".option push\n"				\
+			      ".option arch,+zbb\n"			\
+			      "rev8 %0, %1\n"				\
+			      ".option pop\n"				\
+			      : "=r" (x) : "r" (x));			\
+		return x >> (BITS_PER_LONG - size);			\
+	}                                                               \
+	return  ___constant_swab##size(value);				\
+}
+
+#ifdef CONFIG_64BIT
+ARCH_SWAB(64)
+#define __arch_swab64 __arch_swab64
+#endif
+
+ARCH_SWAB(32)
+#define __arch_swab32 __arch_swab32
+
+ARCH_SWAB(16)
+#define __arch_swab16 __arch_swab16
+
+#undef ARCH_SWAB
+
+#endif /* defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE) */
+#endif /* _ASM_RISCV_SWAB_H */

-- 
2.49.0

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Arnd Bergmann]

On Thu, Apr 3, 2025, at 22:34, Ignacio Encinas wrote:
> +#define ARCH_SWAB(size) \
> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
> +{									\
> +	unsigned long x = value;					\
> +									\
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
> +		asm volatile (".option push\n"				\
> +			      ".option arch,+zbb\n"			\
> +			      "rev8 %0, %1\n"				\
> +			      ".option pop\n"				\
> +			      : "=r" (x) : "r" (x));			\
> +		return x >> (BITS_PER_LONG - size);			\
> +	}                                                               \
> +	return  ___constant_swab##size(value);				\
> +}

I think the fallback should really just use the __builtin_bswap
helpers instead of the ___constant_swab variants. The output
would be the same, but you can skip patch 1/2.

I would also suggest dumbing down the macro a bit so you can
still find the definition with 'git grep __arch_swab64'. Ideally
just put the function body into a macro but leave the three
separate inline function definitions.

     Arnd

Re: [PATCH v3 1/2] include/uapi/linux/swab.h: move default implementation for swab macros into asm-generic

[kernel test robot]

Hi Ignacio,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a7f2e10ecd8f18b83951b0bab47ddaf48f93bf47]

url:    https://github.com/intel-lab-lkp/linux/commits/Ignacio-Encinas/include-uapi-linux-swab-h-move-default-implementation-for-swab-macros-into-asm-generic/20250404-051744
base:   a7f2e10ecd8f18b83951b0bab47ddaf48f93bf47
patch link:    https://lore.kernel.org/r/20250403-riscv-swab-v3-1-3bf705d80e33%40iencinas.com
patch subject: [PATCH v3 1/2] include/uapi/linux/swab.h: move default implementation for swab macros into asm-generic
config: i386-buildonly-randconfig-004-20250404 (https://download.01.org/0day-ci/archive/20250404/202504042300.it9RcOSt-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250404/202504042300.it9RcOSt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504042300.it9RcOSt-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> usr/include/asm-generic/swab.h:21: found __[us]{8,16,32,64} type without #include <linux/types.h>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ben Dooks]

On 03/04/2025 21:34, Ignacio Encinas wrote:
> Implement endianness swap macros for RISC-V.
> 
> Use the rev8 instruction when Zbb is available. Otherwise, rely on the
> default mask-and-shift implementation.
> 
> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
> ---
>   arch/riscv/include/asm/swab.h | 43 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/swab.h b/arch/riscv/include/asm/swab.h
> new file mode 100644
> index 000000000000..7352e8405a99
> --- /dev/null
> +++ b/arch/riscv/include/asm/swab.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _ASM_RISCV_SWAB_H
> +#define _ASM_RISCV_SWAB_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <asm/cpufeature-macros.h>
> +#include <asm/hwcap.h>
> +#include <asm-generic/swab.h>
> +
> +#if defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE)
> +
> +#define ARCH_SWAB(size) \
> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
> +{									\
> +	unsigned long x = value;					\
> +									\
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
> +		asm volatile (".option push\n"				\
> +			      ".option arch,+zbb\n"			\
> +			      "rev8 %0, %1\n"				\
> +			      ".option pop\n"				\
> +			      : "=r" (x) : "r" (x));			\
> +		return x >> (BITS_PER_LONG - size);			\
> +	}                                                               \
> +	return  ___constant_swab##size(value);				\
> +}
> +
> +#ifdef CONFIG_64BIT
> +ARCH_SWAB(64)
> +#define __arch_swab64 __arch_swab64
> +#endif
> +
> +ARCH_SWAB(32)
> +#define __arch_swab32 __arch_swab32
> +
> +ARCH_SWAB(16)
> +#define __arch_swab16 __arch_swab16
> +
> +#undef ARCH_SWAB
> +
> +#endif /* defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE) */
> +#endif /* _ASM_RISCV_SWAB_H */
> 

I was having a look at this as well, using the alternatives macros.

It would be nice to have a __zbb_swab defined so that you could do some
time checks with this, because it would be interesting to see the
benchmark of how much these improve byteswapping.

How about:

#define ARCH_SWAB(size) \
static __always_inline unsigned long __zbb_swab##size(__u##size value) \
{								\
	unsigned long x = value;				\
								\
	asm volatile (".option push\n"				\
		      ".option arch,+zbb\n"			\
		      "rev8 %0, %1\n"				\
		      ".option pop\n"				\
		      : "=r" (x) : "r" (x));			\
	return x >> (BITS_PER_LONG - size);			\
}                                                               \
								\
static __always_inline unsigned long __arch_swab##size(__u##size value) \
	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB))       \
		return  __zbb_swab##size(value)			\
	return ___constant_swab##size(value);			\	


We might need to define  __zbb_swab##size to BUG or something if it
isn't available.

Also, I wonder if it is possible to say to the build system we must
have ZBB therefore only emit ZBB for cases where you are building a
kernel for an known ZBB system.



-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ben Dooks]

On 04/04/2025 06:58, Arnd Bergmann wrote:
> On Thu, Apr 3, 2025, at 22:34, Ignacio Encinas wrote:
>> +#define ARCH_SWAB(size) \
>> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
>> +{									\
>> +	unsigned long x = value;					\
>> +									\
>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
>> +		asm volatile (".option push\n"				\
>> +			      ".option arch,+zbb\n"			\
>> +			      "rev8 %0, %1\n"				\
>> +			      ".option pop\n"				\
>> +			      : "=r" (x) : "r" (x));			\
>> +		return x >> (BITS_PER_LONG - size);			\
>> +	}                                                               \
>> +	return  ___constant_swab##size(value);				\
>> +}
> 
> I think the fallback should really just use the __builtin_bswap
> helpers instead of the ___constant_swab variants. The output
> would be the same, but you can skip patch 1/2.
> 
> I would also suggest dumbing down the macro a bit so you can
> still find the definition with 'git grep __arch_swab64'. Ideally
> just put the function body into a macro but leave the three
> separate inline function definitions.

I thought we explicitly disabled the builtin from gc... I tried doing
this and just ended up with undefined calls from these sites.,

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ben Dooks]

On 03/04/2025 21:34, Ignacio Encinas wrote:
> Implement endianness swap macros for RISC-V.
> 
> Use the rev8 instruction when Zbb is available. Otherwise, rely on the
> default mask-and-shift implementation.
> 
> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
> ---
>   arch/riscv/include/asm/swab.h | 43 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/swab.h b/arch/riscv/include/asm/swab.h
> new file mode 100644
> index 000000000000..7352e8405a99
> --- /dev/null
> +++ b/arch/riscv/include/asm/swab.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _ASM_RISCV_SWAB_H
> +#define _ASM_RISCV_SWAB_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <asm/cpufeature-macros.h>
> +#include <asm/hwcap.h>
> +#include <asm-generic/swab.h>
> +
> +#if defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE)
> +
> +#define ARCH_SWAB(size) \
> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
> +{									\
> +	unsigned long x = value;					\
> +									\
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
> +		asm volatile (".option push\n"				\
> +			      ".option arch,+zbb\n"			\
> +			      "rev8 %0, %1\n"				\
> +			      ".option pop\n"				\
> +			      : "=r" (x) : "r" (x));			\
> +		return x >> (BITS_PER_LONG - size);			\
> +	}                                                               \
> +	return  ___constant_swab##size(value);				\
> +}
> +
> +#ifdef CONFIG_64BIT
> +ARCH_SWAB(64)
> +#define __arch_swab64 __arch_swab64
> +#endif

I suppose if we're 64bit we can't just rely on values being in one
register so this'd need special casing here?

> +ARCH_SWAB(32)
> +#define __arch_swab32 __arch_swab32
> +
> +ARCH_SWAB(16)
> +#define __arch_swab16 __arch_swab16
> +
> +#undef ARCH_SWAB
> +
> +#endif /* defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE) */
> +#endif /* _ASM_RISCV_SWAB_H */
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v3 0/2] Implement endianess swap macros for RISC-V

[Ben Dooks]

On 03/04/2025 21:34, Ignacio Encinas wrote:
> Motivated by [1]. A couple of things to note:
> 
> RISC-V needs a default implementation to fall back on. There is one
> available in include/uapi/linux/swab.h but that header can't be included
> from arch/riscv/include/asm/swab.h. Therefore, the first patch in this
> series moves the default implementation into asm-generic.
> 
> Tested with crc_kunit as pointed out here [2]. I can't provide
> performance numbers as I don't have RISC-V hardware yet.
> 
> [1] https://lore.kernel.org/all/20250302220426.GC2079@quark.localdomain/
> [2] https://lore.kernel.org/all/20250216225530.306980-1-ebiggers@kernel.org/
> 
> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>

I'll try and get these tested with my big-endian riscv qemu and verify
if they work there.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ignacio Encinas]

On 4/4/25 7:58, Arnd Bergmann wrote:
> On Thu, Apr 3, 2025, at 22:34, Ignacio Encinas wrote:
>> +#define ARCH_SWAB(size) \
>> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
>> +{									\
>> +	unsigned long x = value;					\
>> +									\
>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
>> +		asm volatile (".option push\n"				\
>> +			      ".option arch,+zbb\n"			\
>> +			      "rev8 %0, %1\n"				\
>> +			      ".option pop\n"				\
>> +			      : "=r" (x) : "r" (x));			\
>> +		return x >> (BITS_PER_LONG - size);			\
>> +	}                                                               \
>> +	return  ___constant_swab##size(value);				\
>> +}

Hello Arnd!

> I think the fallback should really just use the __builtin_bswap
> helpers instead of the ___constant_swab variants. The output
> would be the same, but you can skip patch 1/2.

I tried, but that change causes build errors:

```
undefined reference to `__bswapsi2'

[...]

undefined reference to `__bswapdi2
```

I tried working around those, but couldn't find a good solution. I'm a 
bit out of my depth here, but I "summarized" everything here [1]. Let me
know if I'm missing something.

[1] https://lore.kernel.org/linux-riscv/b3b59747-0484-4042-bdc4-c067688e3bfe@iencinas.com/

> I would also suggest dumbing down the macro a bit so you can
> still find the definition with 'git grep __arch_swab64'. Ideally
> just put the function body into a macro but leave the three
> separate inline function definitions.

Good point, thanks for bringing it up. Just to be sure, is this what you
had in mind? (Give or take formatting + naming of variables)

#define arch_swab(size, value) 						\
({                      						\
	unsigned long x = value;					\
									\
	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
		asm volatile (".option push\n"				\
			      ".option arch,+zbb\n"			\
			      "rev8 %0, %1\n"				\
			      ".option pop\n"				\
			      : "=r" (x) : "r" (x));			\
		x = x >> (BITS_PER_LONG - size);			\
	} else {                                                        \
		x = ___constant_swab##size(value);                      \
	}								\
	x;								\
})

static __always_inline unsigned long __arch_swab64(__u64 value) {
	return arch_swab(64, value);
}

Thanks!

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ignacio Encinas]

On 4/4/25 17:47, Ben Dooks wrote:
> I was having a look at this as well, using the alternatives macros.
> 
> It would be nice to have a __zbb_swab defined so that you could do some
> time checks with this, because it would be interesting to see the
> benchmark of how much these improve byteswapping.

I get your point, but isn't what you propose equivalent to benchmarking 
__arch_swab vs ___constant_swab? 
 
> Also, I wonder if it is possible to say to the build system we must
> have ZBB therefore only emit ZBB for cases where you are building a
> kernel for an known ZBB system.

You can disable ZBB instructions if you do RISCV_ISA_ZBB=n. 

config RISCV_ISA_ZBB
	bool "Zbb extension support for bit manipulation instructions"
	depends on TOOLCHAIN_HAS_ZBB
	depends on RISCV_ALTERNATIVE
	default y
	help
	   Adds support to dynamically detect the presence of the ZBB
	   extension (basic bit manipulation) and enable its usage.

	   The Zbb extension provides instructions to accelerate a number
	   of bit-specific operations (count bit population, sign extending,
	   bitrotation, etc).

	   If you don't know what to do here, say Y.

However, statically assuming ZBB instruction support is another beast
and I don't really have an informed opinion about this. Perhaps in a few
years?

Thanks for the review!

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ignacio Encinas]

On 4/4/25 17:55, Ben Dooks wrote:
> On 03/04/2025 21:34, Ignacio Encinas wrote:
>> +#ifdef CONFIG_64BIT
>> +ARCH_SWAB(64)
>> +#define __arch_swab64 __arch_swab64
>> +#endif
> 
> I suppose if we're 64bit we can't just rely on values being in one
> register so this'd need special casing here?

Oops... I somehow decided that __arch_swab64 wasn't worth having for
CONFIG_32BIT. I can't tell how useful it is to have it, but it is
doable and already present in the codebase (include/uapi/linux/swab.h):

	__u32 h = val >> 32;
	__u32 l = val & ((1ULL << 32) - 1);
	return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h)));

I'll excuse myself on this one because I'm not sure I have ever used a
32 bit CPU (other than the very occasional and quick school project)

Thanks for catching this one! I'll make sure to add __arch_swab64 for
the 32BIT version mimicking the snippet from above.

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Eric Biggers]

On Fri, Apr 04, 2025 at 04:47:52PM +0100, Ben Dooks wrote:
> On 03/04/2025 21:34, Ignacio Encinas wrote:
> > Implement endianness swap macros for RISC-V.
> > 
> > Use the rev8 instruction when Zbb is available. Otherwise, rely on the
> > default mask-and-shift implementation.
> > 
> > Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
> > ---
> >   arch/riscv/include/asm/swab.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/swab.h b/arch/riscv/include/asm/swab.h
> > new file mode 100644
> > index 000000000000..7352e8405a99
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/swab.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _ASM_RISCV_SWAB_H
> > +#define _ASM_RISCV_SWAB_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/compiler.h>
> > +#include <asm/cpufeature-macros.h>
> > +#include <asm/hwcap.h>
> > +#include <asm-generic/swab.h>
> > +
> > +#if defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE)
> > +
> > +#define ARCH_SWAB(size) \
> > +static __always_inline unsigned long __arch_swab##size(__u##size value) \
> > +{									\
> > +	unsigned long x = value;					\
> > +									\
> > +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
> > +		asm volatile (".option push\n"				\
> > +			      ".option arch,+zbb\n"			\
> > +			      "rev8 %0, %1\n"				\
> > +			      ".option pop\n"				\
> > +			      : "=r" (x) : "r" (x));			\
> > +		return x >> (BITS_PER_LONG - size);			\
> > +	}                                                               \
> > +	return  ___constant_swab##size(value);				\
> > +}
> > +
> > +#ifdef CONFIG_64BIT
> > +ARCH_SWAB(64)
> > +#define __arch_swab64 __arch_swab64
> > +#endif
> > +
> > +ARCH_SWAB(32)
> > +#define __arch_swab32 __arch_swab32
> > +
> > +ARCH_SWAB(16)
> > +#define __arch_swab16 __arch_swab16
> > +
> > +#undef ARCH_SWAB
> > +
> > +#endif /* defined(CONFIG_RISCV_ISA_ZBB) && !defined(NO_ALTERNATIVE) */
> > +#endif /* _ASM_RISCV_SWAB_H */
> > 
> 
> I was having a look at this as well, using the alternatives macros.
> 
> It would be nice to have a __zbb_swab defined so that you could do some
> time checks with this, because it would be interesting to see the
> benchmark of how much these improve byteswapping.

FYI if you missed the previous discussion
(https://lore.kernel.org/linux-riscv/20250302220426.GC2079@quark.localdomain/),
currently the overhead caused by the slow generic byte-swapping on RISC-V is
easily visible in the CRC benchmark.  For example compare:

    crc32_le_benchmark: len=16384: 2440 MB/s

to
    
    crc32_be_benchmark: len=16384: 674 MB/s

But the main loops of crc32_le and crc32_be are basically the same, except
crc32_le does le64_to_cpu() (or le32_to_cpu()) on the data whereas crc32_be does
be64_to_cpu() (or be32_to_cpu()).  The above numbers came from a little-endian
CPU, where le*_to_cpu() is a no-op and be*_to_cpu() is a byte-swap.

To reproduce this, build a kernel from the latest upstream with
CONFIG_CRC_KUNIT_TEST=y and CONFIG_CRC_BENCHMARK=y, boot it on a CPU that has
the Zbc extension, and check dmesg for the benchmark results.

This patch should mostly close the difference, though I don't currently have
hardware to confirm that myself.

- Eric

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Alexandre Ghiti]

Hi Ignacio,

On 04/04/2025 19:35, Ignacio Encinas wrote:
>
> On 4/4/25 7:58, Arnd Bergmann wrote:
>> On Thu, Apr 3, 2025, at 22:34, Ignacio Encinas wrote:
>>> +#define ARCH_SWAB(size) \
>>> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
>>> +{									\
>>> +	unsigned long x = value;					\
>>> +									\
>>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
>>> +		asm volatile (".option push\n"				\
>>> +			      ".option arch,+zbb\n"			\
>>> +			      "rev8 %0, %1\n"				\
>>> +			      ".option pop\n"				\
>>> +			      : "=r" (x) : "r" (x));			\
>>> +		return x >> (BITS_PER_LONG - size);			\
>>> +	}                                                               \
>>> +	return  ___constant_swab##size(value);				\
>>> +}
> Hello Arnd!
>
>> I think the fallback should really just use the __builtin_bswap
>> helpers instead of the ___constant_swab variants. The output
>> would be the same, but you can skip patch 1/2.
> I tried, but that change causes build errors:
>
> ```
> undefined reference to `__bswapsi2'
>
> [...]
>
> undefined reference to `__bswapdi2
> ```
>
> I tried working around those, but couldn't find a good solution. I'm a
> bit out of my depth here, but I "summarized" everything here [1]. Let me
> know if I'm missing something.
>
> [1] https://lore.kernel.org/linux-riscv/b3b59747-0484-4042-bdc4-c067688e3bfe@iencinas.com/

Note that I only encountered those issues in the purgatory.

So to me we have 3 solutions:

- either implementing both __bswapsi2 and __bswapdi2

- or linking against libgcc

- or merging patch 1.

Given the explanation in commit d67703a8a69e ("arm64: kill off the 
libgcc dependency"), I would not use libgcc.

The less intrusive solution (for us riscv) is merging patch 1.

But please let me know if I missed another solution or if I'm wrong.

Thanks,

Alex


>
>> I would also suggest dumbing down the macro a bit so you can
>> still find the definition with 'git grep __arch_swab64'. Ideally
>> just put the function body into a macro but leave the three
>> separate inline function definitions.
> Good point, thanks for bringing it up. Just to be sure, is this what you
> had in mind? (Give or take formatting + naming of variables)
>
> #define arch_swab(size, value) 						\
> ({                      						\
> 	unsigned long x = value;					\
> 									\
> 	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
> 		asm volatile (".option push\n"				\
> 			      ".option arch,+zbb\n"			\
> 			      "rev8 %0, %1\n"				\
> 			      ".option pop\n"				\
> 			      : "=r" (x) : "r" (x));			\
> 		x = x >> (BITS_PER_LONG - size);			\
> 	} else {                                                        \
> 		x = ___constant_swab##size(value);                      \
> 	}								\
> 	x;								\
> })
>
> static __always_inline unsigned long __arch_swab64(__u64 value) {
> 	return arch_swab(64, value);
> }
>
> Thanks!
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 2/2] riscv: introduce asm/swab.h

[Ignacio Encinas Rubio]

Hello!

On 23/4/25 13:08, Alexandre Ghiti wrote:
> Hi Ignacio,
> 
> On 04/04/2025 19:35, Ignacio Encinas wrote:
>>
>> On 4/4/25 7:58, Arnd Bergmann wrote:
>>> On Thu, Apr 3, 2025, at 22:34, Ignacio Encinas wrote:
>>>> +#define ARCH_SWAB(size) \
>>>> +static __always_inline unsigned long __arch_swab##size(__u##size value) \
>>>> +{                                    \
>>>> +    unsigned long x = value;                    \
>>>> +                                    \
>>>> +    if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
>>>> +        asm volatile (".option push\n"                \
>>>> +                  ".option arch,+zbb\n"            \
>>>> +                  "rev8 %0, %1\n"                \
>>>> +                  ".option pop\n"                \
>>>> +                  : "=r" (x) : "r" (x));            \
>>>> +        return x >> (BITS_PER_LONG - size);            \
>>>> +    }                                                               \
>>>> +    return  ___constant_swab##size(value);                \
>>>> +}
>> Hello Arnd!
>>
>>> I think the fallback should really just use the __builtin_bswap
>>> helpers instead of the ___constant_swab variants. The output
>>> would be the same, but you can skip patch 1/2.
>> I tried, but that change causes build errors:
>>
>> ```
>> undefined reference to `__bswapsi2'
>>
>> [...]
>>
>> undefined reference to `__bswapdi2
>> ```
>>
>> I tried working around those, but couldn't find a good solution. I'm a
>> bit out of my depth here, but I "summarized" everything here [1]. Let me
>> know if I'm missing something.
>>
>> [1] https://lore.kernel.org/linux-riscv/b3b59747-0484-4042-bdc4-c067688e3bfe@iencinas.com/
> 
> Note that I only encountered those issues in the purgatory.
> 
> So to me we have 3 solutions:
> 
> - either implementing both __bswapsi2 and __bswapdi2

Which would be fine but seems a bit pointless to me: these __bswapsi2
and __bswapdi2 implementations will be ___constant_swab with a different
name: same code or hardcoded (and equivalent) assembly

> - or linking against libgcc
>
> - or merging patch 1.
>
> Given the explanation in commit d67703a8a69e ("arm64: kill off the libgcc dependency"), I would not use libgcc.
> 
> The less intrusive solution (for us riscv) is merging patch 1.

I also think this seems the best solution. I'll send a v4 taking the
tips that were pointed during review (and fixing the issue pointed out
by the kernel test robot) keeping patch 1.

> But please let me know if I missed another solution or if I'm wrong.
> 
> Thanks,
> 
> Alex
> 
> 
>>
>>> I would also suggest dumbing down the macro a bit so you can
>>> still find the definition with 'git grep __arch_swab64'. Ideally
>>> just put the function body into a macro but leave the three
>>> separate inline function definitions.
>> Good point, thanks for bringing it up. Just to be sure, is this what you
>> had in mind? (Give or take formatting + naming of variables)
>>
>> #define arch_swab(size, value)                         \
>> ({                                              \
>>     unsigned long x = value;                    \
>>                                     \
>>     if (riscv_has_extension_likely(RISCV_ISA_EXT_ZBB)) {            \
>>         asm volatile (".option push\n"                \
>>                   ".option arch,+zbb\n"            \
>>                   "rev8 %0, %1\n"                \
>>                   ".option pop\n"                \
>>                   : "=r" (x) : "r" (x));            \
>>         x = x >> (BITS_PER_LONG - size);            \
>>     } else {                                                        \
>>         x = ___constant_swab##size(value);                      \
>>     }                                \
>>     x;                                \
>> })
>>
>> static __always_inline unsigned long __arch_swab64(__u64 value) {
>>     return arch_swab(64, value);
>> }
>>
>> Thanks!
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv