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

Alexandre Ghiti
2025-04-23T11:08:48Z

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