Re: [PATCH 1/3] RISC-V: add Bitmanip/Scalar Crypto parsing from DT

[Hongren (Zenithal) Zheng]

On Wed, May 04, 2022 at 01:21:23AM +0200, Heiko Stuebner wrote:
> Am Samstag, 30. April 2022, 15:50:22 CEST schrieb Hongren (Zenithal) Zheng:
> > This commit parses Zb/Zk related string from DT and
> > output them in cpuinfo
> > 
> > One thing worth noting is that if DT provides zk,
> > all zbkb, zbkc, zbkx and zkn, zkr, zkt would be enabled.
> > 
> > Note that zk is a valid extension name and the current
> > DT binding spec allows this.
> > 
> > There currently lacks a mechanism to merge them when
> > producing cpuinfo. Namely if you provide a riscv,isa
> > "rv64imafdc_zk_zks", the cpuinfo output would be
> > "rv64imafdc_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zksed
> > _zksh_zkt"
> > 
> > Tested-by: Jiatai He <jiatai2021@xxxxxxxxxxx>
> > Signed-off-by: Hongren (Zenithal) Zheng <i@xxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/hwcap.h | 14 ++++++++++++++
> >  arch/riscv/kernel/cpu.c        | 14 ++++++++++++++
> >  arch/riscv/kernel/cpufeature.c | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 0734e42f74f2..199eda39e0b8 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -52,6 +52,20 @@ extern unsigned long elf_hwcap;
> >   */
> >  enum riscv_isa_ext_id {
> >  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> > +	RISCV_ISA_EXT_ZBA,
> > +	RISCV_ISA_EXT_ZBB,
> > +	RISCV_ISA_EXT_ZBC,
> > +	RISCV_ISA_EXT_ZBS,
> > +	RISCV_ISA_EXT_ZBKB,
> > +	RISCV_ISA_EXT_ZBKC,
> > +	RISCV_ISA_EXT_ZBKX,
> > +	RISCV_ISA_EXT_ZKNE,
> > +	RISCV_ISA_EXT_ZKND,
> > +	RISCV_ISA_EXT_ZKNH,
> > +	RISCV_ISA_EXT_ZKSED,
> > +	RISCV_ISA_EXT_ZKSH,
> > +	RISCV_ISA_EXT_ZKR,
> > +	RISCV_ISA_EXT_ZKT,
> >  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> >  };
> >  
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index ccb617791e56..7251336969c1 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -87,6 +87,20 @@ int riscv_of_parent_hartid(struct device_node *node)
> >   *    extensions by an underscore.
> >   */
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > +	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> > +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +	__RISCV_ISA_EXT_DATA(zbc, RISCV_ISA_EXT_ZBC),
> > +	__RISCV_ISA_EXT_DATA(zbkb, RISCV_ISA_EXT_ZBKB),
> > +	__RISCV_ISA_EXT_DATA(zbkc, RISCV_ISA_EXT_ZBKC),
> > +	__RISCV_ISA_EXT_DATA(zbkx, RISCV_ISA_EXT_ZBKX),
> > +	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> > +	__RISCV_ISA_EXT_DATA(zknd, RISCV_ISA_EXT_ZKND),
> > +	__RISCV_ISA_EXT_DATA(zkne, RISCV_ISA_EXT_ZKNE),
> > +	__RISCV_ISA_EXT_DATA(zknh, RISCV_ISA_EXT_ZKNH),
> > +	__RISCV_ISA_EXT_DATA(zkr, RISCV_ISA_EXT_ZKR),
> > +	__RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED),
> > +	__RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH),
> > +	__RISCV_ISA_EXT_DATA(zkt, RISCV_ISA_EXT_ZKT),
> >  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> 
> I guess a bit of sorting rule might be helpful, here it's the additions
> above sscofpmf while in the enum it's the other way around.

Here the order is defined by the rule that
"Standard supervisor-level extensions (starts with 'S')
should be listed after standard unprivileged extensions."
You can see the comment above this snippet.
You can also see the for loop in print_isa_ext.

Although I do not know should I place 'Zbs' before 'Zbk*'
or after them as 'Zbk*' belong to scalar crypto but
they are prefixed with "b". It seems it is undecided now.

In the enum it is sorted according to the logical id.
As "This enum represent the logical ID" in
arch/riscv/include/asm/hwcap.h said.

> As the list will get a long longer over time, I guess consistency
> might improve readability.

Agreed. I think changing the internal logical id is
acceptable for me and it brings no API change.

I need feedback on whether I could change the logical id
of the existing sscofpmf extension.

> 
> >  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> >  };
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 1b2d42d7f589..10f9daf3734e 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -192,6 +192,39 @@ void __init riscv_fill_hwcap(void)
> >  				set_bit(*ext - 'a', this_isa);
> >  			} else {
> >  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> > +				SET_ISA_EXT_MAP("zba"     , RISCV_ISA_EXT_ZBA     );
> 
> not sure if the additional whitespaces are really necessary? [especially the ones at the end]
> What did checkpatch have to say about them?

checkpatch did complain about them. Also checkpatch complaint
about the style issue for PATCH 3/3.
It's my fault not to check style before sending.
Will fix the style issue in the next version.
I think I'll send the next version after getting more feedbacks.

> 
> > +				SET_ISA_EXT_MAP("zbb"     , RISCV_ISA_EXT_ZBB     );
> > +				SET_ISA_EXT_MAP("zbc"     , RISCV_ISA_EXT_ZBC     );
> > +				SET_ISA_EXT_MAP("zbs"     , RISCV_ISA_EXT_ZBS     );
> > +				SET_ISA_EXT_MAP("zbkb"    , RISCV_ISA_EXT_ZBKB    );
> > +				SET_ISA_EXT_MAP("zbkc"    , RISCV_ISA_EXT_ZBKC    );
> > +				SET_ISA_EXT_MAP("zbks"    , RISCV_ISA_EXT_ZBKX    );
> > +				SET_ISA_EXT_MAP("zknd"    , RISCV_ISA_EXT_ZKND    );
> > +				SET_ISA_EXT_MAP("zkne"    , RISCV_ISA_EXT_ZKNE    );
> > +				SET_ISA_EXT_MAP("zknh"    , RISCV_ISA_EXT_ZKNH    );
> > +				SET_ISA_EXT_MAP("zksed"   , RISCV_ISA_EXT_ZKSED   );
> > +				SET_ISA_EXT_MAP("zksh"    , RISCV_ISA_EXT_ZKSH    );
> > +				SET_ISA_EXT_MAP("zkr"     , RISCV_ISA_EXT_ZKR     );
> > +				SET_ISA_EXT_MAP("zkt"     , RISCV_ISA_EXT_ZKT     );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZBKB    );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZBKC    );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZBKX    );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZKND    );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZKNE    );
> > +				SET_ISA_EXT_MAP("zkn"     , RISCV_ISA_EXT_ZKNH    );
> > +				SET_ISA_EXT_MAP("zks"     , RISCV_ISA_EXT_ZBKB    );
> > +				SET_ISA_EXT_MAP("zks"     , RISCV_ISA_EXT_ZBKC    );
> > +				SET_ISA_EXT_MAP("zks"     , RISCV_ISA_EXT_ZBKX    );
> > +				SET_ISA_EXT_MAP("zks"     , RISCV_ISA_EXT_ZKSED   );
> > +				SET_ISA_EXT_MAP("zks"     , RISCV_ISA_EXT_ZKSH    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZBKB    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZBKC    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZBKX    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZKND    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZKNE    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZKNH    );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZKR     );
> > +				SET_ISA_EXT_MAP("zk"      , RISCV_ISA_EXT_ZKT     );
> >  			}
> >  #undef SET_ISA_EXT_MAP
> >  		}
> > 
> 
> 
> 
>