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

[Alexandre Ghiti <alex@ghiti.fr>]

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