[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <66a5aba9-2a32-4ef9-a839-a389b975757d@ghiti.fr>
Date: Wed, 23 Apr 2025 13:08:42 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Ignacio Encinas <ignacio@...cinas.com>, 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
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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Powered by blists - more mailing lists