Message-ID: <f5464e26-faa0-48f1-8585-9ce52c8c9f5f@iencinas.com>
Date: Thu, 24 Apr 2025 19:27:09 +0200
From: Ignacio Encinas Rubio <ignacio@...cinas.com>
To: Alexandre Ghiti <alex@...ti.fr>, Arnd Bergmann <arnd@...db.de>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>
Cc: Eric Biggers <ebiggers@...nel.org>, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linux.dev,
 Shuah Khan <skhan@...uxfoundation.org>,
 Zhihang Shao <zhihang.shao.iscas@...il.com>, Björn Töpel
 <bjorn@...nel.org>, Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] riscv: introduce asm/swab.h



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@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists