Re: [PATCH v9 03/26] riscv: zicfiss / zicfilp enumeration

[Deepak Gupta]

On Thu, Feb 06, 2025 at 02:50:29PM +0100, Clément Léger wrote:
> On 05/02/2025 02:21, Deepak Gupta wrote:
> > This patch adds support for detecting zicfiss and zicfilp. zicfiss and
> > zicfilp stands for unprivleged integer spec extension for shadow stack
> > and branch tracking on indirect branches, respectively.
> >
> > This patch looks for zicfiss and zicfilp in device tree and accordinlgy
> > lights up bit in cpu feature bitmap. Furthermore this patch adds detection
> > utility functions to return whether shadow stack or landing pads are
> > supported by cpu.
> >
> > Signed-off-by: Deepak Gupta <debug@xxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/cpufeature.h | 13 +++++++++++++
> >  arch/riscv/include/asm/hwcap.h      |  2 ++
> >  arch/riscv/include/asm/processor.h  |  1 +
> >  arch/riscv/kernel/cpufeature.c      |  2 ++
> >  4 files changed, 18 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > index 569140d6e639..69007b8100ca 100644
> > --- a/arch/riscv/include/asm/cpufeature.h
> > +++ b/arch/riscv/include/asm/cpufeature.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/kconfig.h>
> >  #include <linux/percpu-defs.h>
> >  #include <linux/threads.h>
> > +#include <linux/smp.h>
> >  #include <asm/hwcap.h>
> >  #include <asm/cpufeature-macros.h>
> >
> > @@ -137,4 +138,16 @@ static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsi
> >  	return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> >  }
> >
> > +static inline bool cpu_supports_shadow_stack(void)
> > +{
> > +	return (IS_ENABLED(CONFIG_RISCV_USER_CFI) &&
> > +		riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICFISS));
> > +}
> > +
> > +static inline bool cpu_supports_indirect_br_lp_instr(void)
> > +{
> > +	return (IS_ENABLED(CONFIG_RISCV_USER_CFI) &&
> > +		riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICFILP));
> > +}
> > +
> >  #endif
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 869da082252a..2dc4232bdb3e 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -100,6 +100,8 @@
> >  #define RISCV_ISA_EXT_ZICCRSE		91
> >  #define RISCV_ISA_EXT_SVADE		92
> >  #define RISCV_ISA_EXT_SVADU		93
> > +#define RISCV_ISA_EXT_ZICFILP		94
> > +#define RISCV_ISA_EXT_ZICFISS		95
> >
> >  #define RISCV_ISA_EXT_XLINUXENVCFG	127
> >
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index 5f56eb9d114a..e3aba3336e63 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -13,6 +13,7 @@
> >  #include <vdso/processor.h>
> >
> >  #include <asm/ptrace.h>
> > +#include <asm/hwcap.h>
> >
> >  #define arch_get_mmap_end(addr, len, flags)			\
> >  ({								\
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index c6ba750536c3..e72de12e5b99 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -333,6 +333,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> >  	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
> >  					  riscv_ext_zicboz_validate),
> >  	__RISCV_ISA_EXT_DATA(ziccrse, RISCV_ISA_EXT_ZICCRSE),
> > +	__RISCV_ISA_EXT_SUPERSET(zicfilp, RISCV_ISA_EXT_ZICFILP, riscv_xlinuxenvcfg_exts),
> > +	__RISCV_ISA_EXT_SUPERSET(zicfiss, RISCV_ISA_EXT_ZICFISS, riscv_xlinuxenvcfg_exts),
>
> Hey Deepak,
>
> I think these definitions can benefit from using a validation callback:
>
> static int riscv_cfi_validate(const struct riscv_isa_ext_data *data,
> 				  const unsigned long *isa_bitmap)
> {
> 	if (!IS_ENABLED(CONFIG_RISCV_USER_CFI)
> 		return -EINVAL;
> 		
> 	return 0;
> }

Yes this is a good idea.
I'll add that.

> __RISCV_ISA_EXT_SUPERSET_VALIDATE(zicfilp, RISCV_ISA_EXT_ZICFILP,
> riscv_xlinuxenvcfg_exts, riscv_cfi_validate),
> __RISCV_ISA_EXT_SUPERSET_VALIDATE(zicfiss, RISCV_ISA_EXT_ZICFISS,
> riscv_xlinuxenvcfg_exts, riscv_cfi_validate),
>
> That way, ZICFISS/ZICFILP wont be enable if the kernel does not have
> builtin support for them. Additionally, this solve a bug you have with
> your hwprobe patch (19/26) that exposes ZICFILP/ZICFISS unconditionally
> (ie, even if the kernel does not have CONFIG_RISCV_USER_CFI).

Yes good catch.

> BTW, patch 23/26 introduce CONFIG_RISCV_USER_CFI but it is used in that
> patch.
> Thanks,
>
> Clément
>
> >  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> >  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
> >  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),