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

[Ignacio Encinas <ignacio@iencinas.com>]

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!