Message-ID: <2e082beb-63dd-4395-a3ba-1be4acc28910@ghiti.fr>
Date: Tue, 10 Jun 2025 10:15:22 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
 Xi Ruoyao <xry111@...111.site>
Cc: Vineet Gupta <vineetg@...osinc.com>, Nathan Chancellor
 <nathan@...nel.org>, "Jason A . Donenfeld" <Jason@...c4.com>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Guo Ren <guoren@...nel.org>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] RISC-V: vDSO: Correct inline assembly constraints in
 the getrandom syscall wrapper

Hi,

On 6/10/25 09:11, Thomas Weißschuh wrote:
> On Sat, Jun 07, 2025 at 10:16:34PM +0800, Xi Ruoyao wrote:
>> On Fri, 2025-06-06 at 15:01 -0700, Vineet Gupta wrote:
>>> On 6/6/25 02:24, Xi Ruoyao wrote:
>>>> As recently pointed out by Thomas, if a register is forced for two
>>>> different register variables, among them one is used as "+" (both input
>>>> and output) and another is only used as input, Clang would treat the
>>>> conflicting input parameters as undefined behaviour and optimize away
>>>> the argument assignment.
>>>>
>>>> Per an example in the GCC documentation, for this purpose we can use "="
>>>> (only output) for the output, and "0" for the input for that we must
>>>> reuse the same register as the output.  And GCC developers have
>>>> confirmed using a simple "r" (that we use for most vDSO implementations)
>>>> instead of "0" is also fine.
>>>>
>>>> Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
>>>> Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
>>>> Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
>>>> Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
>>>> Cc: Nathan Chancellor <nathan@...nel.org>
>>>> Signed-off-by: Xi Ruoyao <xry111@...111.site>
>>>> ---
>>>>
>>>> v1 -> v2: Keep using "r" for buffer to follow the existing convention
>>>> (that the GCC developers have confirmed fine).
>>>>
>>>>   arch/riscv/include/asm/vdso/getrandom.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
>>>> index 8dc92441702a..c6d66895c1f5 100644
>>>> --- a/arch/riscv/include/asm/vdso/getrandom.h
>>>> +++ b/arch/riscv/include/asm/vdso/getrandom.h
>>>> @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
>>>>   	register unsigned int flags asm("a2") = _flags;
>>>>   
>>>>   	asm volatile ("ecall\n"
>>>> -		      : "+r" (ret)
>>>> +		      : "=r" (ret)
>>>>   		      : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
>>>>   		      : "memory");
>>> My 2 cents as I've dabbled into this for ARC glibc syscall macros [1] where r0
>>> is both the first syscall/function arg and also the function/syscall return.
>>>
>>> The v2 approach still keeps 2 different variables in same local reg which has
>>> potential for any future compiler shenanigans.
>>> Segher's example avoided specifying the same reg.
>>> What about something like the following: seems to generate the right code (with
>>> gcc 15)
>>>
>>>         register long ret asm("a0");
>> Then it would be better to rename this variable to just "a0".  And I
>> guess Thomas doesn't want a new convention different from all other
>> syscall wrappers in vDSO...
> Indeed. I want to keep it consistent. Especially for a bugfix.
> Speaking of which, IMO this patch should have a Fixes tag.


Yes, here it is:

Fixes: ee0d03053e70 ("RISC-V: vDSO: Wire up getrandom() vDSO 
implementation")


>
> Then we could start a new discussion about changing it to something else everywhere.
> Although I don't think that the single-variable variant is better.


Vineet feel free to propose something for all architectures if you think 
that's better.

For now, I'll merge this version for inclusion in -rc2,

Thanks,

Alex


>
>>>         register long nr asm("a7") = __NR_getrandom;
>>>         register size_t len asm("a1") = _len;
>>>         register unsigned int flags asm("a2") = _flags;
>>>         ret = (unsigned long) _buffer;
>>>
>>>         asm volatile ("ecall\n"
>>>                       : "+r" (ret)                                 // keep "+r"
>>> for input _buffer / output ret
>>>                       : "r" (nr), "r" (len), "r" (flags)
>>>                       : "memory");
>>>
>>>         return ret;
>>>
>>> Thx,
>>> -Vineet
>>>
>>> [1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/arc/sysdep.h
>> -- 
>> Xi Ruoyao <xry111@...111.site>
>> School of Aerospace Science and Technology, Xidian University
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists