Re: [PATCH 1/3] RISC-V: Add Zicclsm to cpufeature and hwprobe

[Charlie Jenkins]

On Thu, Jun 06, 2024 at 07:43:52PM +0100, Conor Dooley wrote:
> On Thu, Jun 06, 2024 at 02:32:13PM -0400, Jesse Taube wrote:
> > > Zicclsm Misaligned loads and stores to main memory regions with both
> > > the cacheability and coherence PMAs must be supported.
> > > Note:
> > > This introduces a new extension name for this feature.
> > > This requires misaligned support for all regular load and store
> > > instructions (including scalar and vector) but not AMOs or other
> > > specialized forms of memory access. Even though mandated, misaligned
> > > loads and stores might execute extremely slowly. Standard software
> > > distributions should assume their existence only for correctness,
> > > not for performance.
> > 
> > Detecing zicclsm allows the kernel to report if the
> > hardware supports misaligned accesses even if support wasn't probed.
> > 
> > This is useful for usermode to know if vector misaligned accesses are
> > supported.
> > 
> > Signed-off-by: Jesse Taube <jesse@xxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/hwcap.h        | 1 +
> >  arch/riscv/include/uapi/asm/hwprobe.h | 1 +
> >  arch/riscv/kernel/cpufeature.c        | 1 +
> >  arch/riscv/kernel/sys_hwprobe.c       | 1 +
> >  4 files changed, 4 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index e17d0078a651..8c0d0b555a8e 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -81,6 +81,7 @@
> >  #define RISCV_ISA_EXT_ZTSO		72
> >  #define RISCV_ISA_EXT_ZACAS		73
> >  #define RISCV_ISA_EXT_XANDESPMU		74
> > +#define RISCV_ISA_EXT_ZICCLSM		75
> >  
> >  #define RISCV_ISA_EXT_XLINUXENVCFG	127
> >  
> > diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > index 2902f68dc913..060212331a03 100644
> > --- a/arch/riscv/include/uapi/asm/hwprobe.h
> > +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> > @@ -59,6 +59,7 @@ struct riscv_hwprobe {
> >  #define		RISCV_HWPROBE_EXT_ZTSO		(1ULL << 33)
> >  #define		RISCV_HWPROBE_EXT_ZACAS		(1ULL << 34)
> >  #define		RISCV_HWPROBE_EXT_ZICOND	(1ULL << 35)
> > +#define		RISCV_HWPROBE_EXT_ZICCLSM	(1ULL << 36)
> 
> Missing an update to hwprobe.rst.
> 

"RISCV_HWPROBE_EXT_ZIHINTPAUSE	(1ULL << 36)" was also defined here in
6.10 so this key needs to be bumped down one.

- Charlie

> >  #define RISCV_HWPROBE_KEY_CPUPERF_0	5
> >  #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
> >  #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 3ed2359eae35..863c708f2f2e 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -305,6 +305,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> >  	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> >  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> >  	__RISCV_ISA_EXT_DATA(xandespmu, RISCV_ISA_EXT_XANDESPMU),
> > +	__RISCV_ISA_EXT_DATA(zicclsm, RISCV_ISA_EXT_ZICCLSM),
> 
> Please read the ordering comment above this structure!
> Also, you're missing dt-binding documentation for the extension.
> 
> >  };
> >  
> >  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > index 8cae41a502dd..b286b73e763e 100644
> > --- a/arch/riscv/kernel/sys_hwprobe.c
> > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > @@ -125,6 +125,7 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >  			EXT_KEY(ZVKT);
> >  			EXT_KEY(ZVFH);
> >  			EXT_KEY(ZVFHMIN);
> > +			EXT_KEY(ZICCLSM);
> 
> Order looks off here too, I think this should be added in in the same
> order as to riscv_isa_ext, although the requirement isn't hard here,
> just that adding to the end of a list means it's annoying to check for
> what's missing.
> 
> Thanks,
> Conor.
> 
> >  		}
> >  
> >  		if (has_fpu()) {
> > -- 
> > 2.43.0
> > 
> >