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

Ignacio Encinas Rubio
2025-04-24T17:27:20Z

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