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

[Deepak Gupta]

On Thu, May 15, 2025 at 10:48:35AM +0200, Radim Krčmář wrote:
> 2025-05-15T09:28:25+02:00, Alexandre Ghiti <alex@xxxxxxxx>:
> > On 06/05/2025 12:10, Radim Krčmář wrote:
> > > 2025-05-02T16:30:36-07:00, Deepak Gupta <debug@xxxxxxxxxxxx>:
> > > > 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.