linux-kernel - Re: [PATCH 1/2] RISC-V: sbi: turn sbi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DAKNYLI1ZWNU.2RGRA0ONMCIYL@ventanamicro.com>
Date: Thu, 12 Jun 2025 17:35:30 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Cc: <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, "Paul
 Walmsley" <paul.walmsley@...ive.com>, "Palmer Dabbelt"
 <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>, "Alexandre
 Ghiti" <alex@...ti.fr>, "Atish Patra" <atishp@...osinc.com>, "Andrew Jones"
 <ajones@...tanamicro.com>, Clément Léger
 <cleger@...osinc.com>, "Anup Patel" <apatel@...tanamicro.com>
Subject: Re: [PATCH 1/2] RISC-V: sbi: turn sbi_ecall into variadic macro

2025-06-12T17:14:09+02:00, Thomas Weißschuh <thomas.weissschuh@...utronix.de>:
> On Thu, Jun 12, 2025 at 04:57:54PM +0200, Radim Krčmář wrote:
>> Counting the arguments to sbi_ecall() and padding with zeros gets old
>> pretty quick.  It's also harder to distinguish a tailing 0 argument and
>> the padding.  The patch changes sbi_ecall() to accept anything between 1
>> and 8 integer arguments.
>> 
>> Those who can count are also given sbi_ecall1() to sbi_ecall8(), which
>> the variadic magic uses under the hood.  The error messages upon a
>> programmer error are a bit hairy, as expected of macros, and the
>> static_assert is there to improve them a bit.
>> 
>> The final goal would be avoid clobbering registers that are not used in
>> the ecall, but we first have to fix the code generation around
>> tracepoints if sbi_ecall is expected to be used in paths where
>> performance is critical.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@...tanamicro.com>
>> ---
>>  arch/riscv/include/asm/sbi.h | 34 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 33 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> index 341e74238aa0..c62db61bd018 100644
>> --- a/arch/riscv/include/asm/sbi.h
>> +++ b/arch/riscv/include/asm/sbi.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/types.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/jump_label.h>
>> +#include <linux/build_bug.h>
>>  
>>  #ifdef CONFIG_RISCV_SBI
>>  enum sbi_ext_id {
>> @@ -465,9 +466,40 @@ struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
>>  			  unsigned long arg2, unsigned long arg3,
>>  			  unsigned long arg4, unsigned long arg5,
>>  			  int fid, int ext);
>> -#define sbi_ecall(e, f, a0, a1, a2, a3, a4, a5)	\
>> +
>> +#define sbi_ecall1(e) \
>> +		__sbi_ecall(0, 0, 0, 0, 0, 0, 0, e)
>> +#define sbi_ecall2(e, f) \
>> +		__sbi_ecall(0, 0, 0, 0, 0, 0, f, e)
>> +#define sbi_ecall3(e, f, a0) \
>> +		__sbi_ecall(a0, 0, 0, 0, 0, 0, f, e)
>> +#define sbi_ecall4(e, f, a0, a1) \
>> +		__sbi_ecall(a0, a1, 0, 0, 0, 0, f, e)
>> +#define sbi_ecall5(e, f, a0, a1, a2) \
>> +		__sbi_ecall(a0, a1, a2, 0, 0, 0, f, e)
>> +#define sbi_ecall6(e, f, a0, a1, a2, a3) \
>> +		__sbi_ecall(a0, a1, a2, a3, 0, 0, f, e)
>> +#define sbi_ecall7(e, f, a0, a1, a2, a3, a4) \
>> +		__sbi_ecall(a0, a1, a2, a3, a4, 0, f, e)
>> +#define sbi_ecall8(e, f, a0, a1, a2, a3, a4, a5) \
>>  		__sbi_ecall(a0, a1, a2, a3, a4, a5, f, e)
>>  
>> +#define __sbi_count_args_magic(_, a, b, c, d, e, f, g, h, N, ...) N
>> +#define __sbi_count_args(...) \
>> +	__sbi_count_args_magic(_, ## __VA_ARGS__, 8, 7, 6, 5, 4, 3, 2, 1, 0)
>
> This looks a lot like COUNT_ARGS() from include/linux/args.h.
>
>> +#define __sbi_count_args2(...) \
>> +	(sizeof((unsigned long[]){0, ## __VA_ARGS__}) / sizeof(unsigned long) - 1)
>> +#define __sbi_concat_expanded(a, b) a ## b
>
> ... and CONCATENATE()

Thanks for pointing them out, I will use them in v2.

>> +#define __sbi_concat(n) __sbi_concat_expanded(sbi_ecall, n)
>> +
>> +/* sbi_ecall selects the appropriate sbi_ecall1 to sbi_ecall8 */
>> +#define sbi_ecall(...)  \
>> +	({ \
>> +		static_assert(__sbi_count_args2(__VA_ARGS__) <= 8); \
>
> Why does this need to use __sbi_count_args2() ?

When you go over the hardcoded maximum count in COUNT_ARGS, the macro
stops counting and returns the arguments instead.
The array approach in __sbi_count_args2() counts any number of
arguments, so it gives a much more understandable error.

I guess using COUNT_ARGS() and condemning programmers that overshoot the
15 counted arguments is better.  We don't really need a static_assert
then either, and v2 coud be just:

#define sbi_ecall(...) \
	CONCATENATE(sbi_ecall, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__);