Re: [PATCH v15 05/27] riscv: usercfi state for task and save/restore of CSR_SSP on trap entry/exit

[Deepak Gupta <debug@rivosinc.com>]

On Thu, May 15, 2025 at 10:48:35AM +0200, Radim Krčmář wrote:
>2025-05-15T09:28:25+02:00, Alexandre Ghiti <alex@ghiti.fr>:
>> On 06/05/2025 12:10, Radim Krčmář wrote:
>>> 2025-05-02T16:30:36-07:00, Deepak Gupta <debug@rivosinc.com>:
>>>> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>>>> @@ -91,6 +91,32 @@
>>>> +.macro restore_userssp tmp
>>>> +	ALTERNATIVE("nops(2)",
>>>> +		__stringify(				\
>>>> +		REG_L \tmp, TASK_TI_USER_SSP(tp);	\
>>>> +		csrw CSR_SSP, \tmp),
>>>> +		0,
>>>> +		RISCV_ISA_EXT_ZICFISS,
>>>> +		CONFIG_RISCV_USER_CFI)
>>>> +.endm
>>> Do we need to emit the nops when CONFIG_RISCV_USER_CFI isn't selected?
>>>
>>> (Why not put #ifdef CONFIG_RISCV_USER_CFI around the ALTERNATIVES?)
>>
>> The alternatives are used to create a generic kernel that contains the
>> code for a large number of extensions and only enable it at runtime
>> depending on the platform capabilities. This way distros can ship a
>> single kernel that works on all platforms.
>
>Yup, and if a kernel is compiled without CONFIG_RISCV_USER_CFI, the nops
>will only enlarge the binary and potentially slow down execution.
>In other words, why we don't do something like this
>
> (!CONFIG_RISCV_USER_CFI ? "" :
>   (RISCV_ISA_EXT_ZICFISS ? __stringify(...) : "nops(x)"))
>
>instead of the current
>
> (CONFIG_RISCV_USER_CFI &&
>    RISCV_ISA_EXT_ZICFISS ? __stringify(...) : "nops(x)")
>
>It could be a new preprocessor macro in case we wanted to make it nice,
>but it's probably not a common case, so an ifdef could work as well.
>
>Do we just generally not care about such minor optimizations?

On its own just for this series, I am not sure if I would call it even a
minor optimization.

But sure, it may (or may not) have noticeable effect if someone were
to go around and muck with ALTERNATIVES macro and emit `old_c` only
if config were selected. That should be a patch set on its own with
data providing benefits from it.

>
>(If we wanted to go an extra mile, we could also keep the nops when both
> CONFIG_RISCV_USER_CFI and RISCV_ISA_EXT_ZICFISS are present, but
> command line riscv_nousercfi disabled backward cfi.)
>
>Thanks.