linux-kernel - Re: [PATCH 1/6] riscv: Introduce use_alternative

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: <544577b2-6810-4bef-b588-e1c662d5be13@iscas.ac.cn>
Date: Wed, 20 Aug 2025 23:25:30 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Yury Norov <yury.norov@...il.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Rasmus Villemoes
 <linux@...musvillemoes.dk>, Vivian Wang <uwu@...m.page>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Aydın Mercan <aydin@...can.dev>
Subject: Re: [PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

Hi Yury,

Thanks for the review.

On 8/20/25 22:56, Yury Norov wrote:
> On Wed, Aug 20, 2025 at 09:44:45PM +0800, Vivian Wang wrote:
>> Introduce convenience helpers use_alternative_likely() and
>> use_alternative_unlikely() to implement the pattern of using asm goto to
>> check if an alternative is selected. Existing code will be converted in
>> subsequent patches.
>>
>> Similar to arm64 alternative_has_cap_{likely,unlikely}, but for riscv,
>> alternatives are not all CPU capabilities.
>>
>> Suggested-by: Aydın Mercan <aydin@...can.dev>
>> Signed-off-by: Vivian Wang <wangruikang@...as.ac.cn>
>> ---
>>  arch/riscv/include/asm/alternative-macros.h | 73 +++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
>> index 231d777d936c2d29c858decaa9a3fa5f172efbb8..be9835b5e4eba03d76db3a73da19ac9e2981c4db 100644
>> --- a/arch/riscv/include/asm/alternative-macros.h
>> +++ b/arch/riscv/include/asm/alternative-macros.h
>> @@ -158,4 +158,77 @@
>>  	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,	\
>>  					new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)
>>  
>> +/*
>> + * use_alternative_{likely,unlikely}() returns true if the alternative is
>> + * applied and false otherwise, but in a way where the compiler can optimize
>> + * this check down to a nop instruction that's patched into a jump, or vice
>> + * versa.
>> + *
>> + * Always returns false if the alternatives mechanism is not available.
>> + *
>> + * Usage example:
>> + *   if (use_alternative_likely(0, RISCV_ISA_ZBB))
>> + *
>> + * Similar to static keys, "likely" means use a nop if the alternative is
>> + * selected, and jump if unselected; "unlikely" is the other way around.
>> + */
>> +
>> +#ifndef __ASSEMBLER__
>> +
>> +#include <linux/types.h>
>> +
>> +#ifdef CONFIG_RISCV_ALTERNATIVE
>> +
>> +static __always_inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
>> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
>> +
>> +	asm goto(ALTERNATIVE("j %l[no_alt]", "nop", %[vendor_id], %[patch_id], 1)
>> +		 :
>> +		 : [vendor_id] "i"(vendor_id),
>> +		   [patch_id] "i"(patch_id)
>> +		 :
>> +		 : no_alt);
>> +
>> +	return true;
>> +
>> +no_alt:
>> +	return false;
>> +}
> Apart from those BUILD_BUG_ON()s, it looks similar to
> __riscv_has_extension_likely(). Can you make sure you don't duplicate
> it?
>
> If so, can you describe what's the difference between those two in the
> commit message?

Whoops, *completely* missed that. Thanks for the catch.

It turns out I was trying to find uses of this pattern by searching for
"j<space>%l[...]". The block in __riscv_has_extension_{likely,unlikely}
uses "j<tab>%l[...]".

I'll just use __riscv_has_extension_{likely,unlikely} in v2 and drop this.

>> +static __always_inline bool use_alternative_unlikely(u16 vendor_id, u32 patch_id)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
>> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
>> +
>> +	asm goto(ALTERNATIVE("nop", "j %l[alt]", %[vendor_id], %[patch_id], 1)
>> +		 :
>> +		 : [vendor_id] "i"(vendor_id),
>> +		   [patch_id] "i"(patch_id)
>> +		 :
>> +		 : alt);
>> +
>> +	return false;
>> +
>> +alt:
>> +	return true;
>> +}
> This 'unlikely' version is just an negation of 'likely' one, and it
> looks like an attempt to save on one negation. On the other hand, the
> function is __always_inline, which means that compiler should normally
> take care of it. Can you prove with objdump that it really works as
> intended? I mean that 
>
>         if (use_alternative_unlikely())
>                 do_something();
>
> generates a better code than 
>         
>         if (!use_alternative_likely())
>                 do_something();

use_alternative_likely() and use_alternative_unlikely() are not
negations of each other and in fact should be functionally equivalent. I
also briefly explained the difference in the comment, but the difference
is which case is nop i.e. fallthrough, and which case requires a jump
instruction. The likely case should get a "nop", and the unlikely case
should get a "j %l[...]". This choice does work as intended [1].

I don't think it is possible to give both options to the compiler, so at
least for now AIUI users have to pick one.

The same applies to __riscv_has_extension_{likely,unlikely}.

Vivian "dramforever" Wang

[1]: https://godbolt.org/z/v8zTEhzTx