Re: [PATCH v4 1/4] RISC-V: Add Bitmanip/Scalar Crypto parsing from DT

[Conor Dooley]

On Thu, Jul 13, 2023 at 02:45:57PM +0200, Andrew Jones wrote:
> On Thu, Jul 13, 2023 at 12:27:24PM +0100, Conor Dooley wrote:

> > +#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) {	\
> > +	.name = #_name,					\
> > +	.property = #_name,				\
> > +	.bundle_ids = _bundled_exts,			\
> > +	.bundle_size = ARRAY_SIZE(_bundled_exts)	\
> > +}
> > +
> > +static const unsigned int riscv_zk_bundled_exts[] = {
> > +	RISCV_ISA_EXT_ZBKB,
> > +	RISCV_ISA_EXT_ZBKC,
> > +	RISCV_ISA_EXT_ZBKX,
> > +	RISCV_ISA_EXT_ZKND,
> > +	RISCV_ISA_EXT_ZKNE,
> > +	RISCV_ISA_EXT_ZKR,
> > +	RISCV_ISA_EXT_ZKT,
> 
> I think RISCV_ISA_EXT_ZKNH also belongs in this bundle,
> since the spec says zk is the zkn bundle plus zkr and zkt.
> 
> > +};
> > +
> > +static const unsigned int riscv_zkn_bundled_exts[] = {
> > +	RISCV_ISA_EXT_ZBKB,
> > +	RISCV_ISA_EXT_ZBKC,
> > +	RISCV_ISA_EXT_ZBKX,
> > +	RISCV_ISA_EXT_ZKND,
> > +	RISCV_ISA_EXT_ZKNE,
> > +	RISCV_ISA_EXT_ZKNH,
> > +};
> > +
> > +static const unsigned int riscv_zks_bundled_exts[] = {
> > +	RISCV_ISA_EXT_ZBKB,
> > +	RISCV_ISA_EXT_ZBKC,
> > +	RISCV_ISA_EXT_ZKSED,
> > +	RISCV_ISA_EXT_ZKSH
> 
> And, per the spec, this one appears to be missing RISCV_ISA_EXT_ZBKX.

Yeah, these do look wrong. I should've cross-checked it.

> I found [1] which calls these shorthands "group names", so maybe we should
> use the term "group" instead of "bundle"?

WFM at least.

> I'm tempted to try to directly
> code that graphic in [1] with something like...
> 
> #define Zks_group1 \
> 	RISCV_ISA_EXT_ZKSED, \
> 	RISCV_ISA_EXT_ZKSH
> 
> #define Zks_group2 \
> 	RISCV_ISA_EXT_ZBKB, \
> 	RISCV_ISA_EXT_ZBKC, \
> 	RISCV_ISA_EXT_ZBKX
> 
> #define Zks_group \
> 	Zks_group1, \
> 	Zks_group2
> 
> #define Zkn_group1 \
> 	RISCV_ISA_EXT_ZKND, \
> 	RISCV_ISA_EXT_ZKNE, \
> 	RISCV_ISA_EXT_ZKNH
> 
> #define Zkn_group2 \
> 	Zks_group2
> 
> #define Zkn_group \
> 	Zkn_group1, \
> 	Zkn_group2
> 
> static const unsigned int riscv_zks_group[] = {
> 	Zks_group,
> };
> 
> static const unsigned int riscv_zkn_group[] = {
> 	Zkn_group,
> };
> 
> static const unsigned int riscv_zk_group[] = {
> 	Zks_group,
> 	Zkn_group,
> 	RISCV_ISA_EXT_ZKR,
> 	RISCV_ISA_EXT_ZKT,
> };
> 
> ...but now that I have, I'm not sure I like the looks of it...

If you called them RISCV_ISA_EXT_GROUP_ZKN (or similar) it would look a
lot less out of place IMO. I'd probably drop the "group2" dance & pick a
better name for "Zks_group2", maybe just do something like
RISCV_ISA_EXT_GROUP_SCALAR_CRYPTO_BITMANIP? Mouthful, but seemed better
than trying to be clever with ZBK or something.

> [1] https://wiki.riscv.org/display/HOME/Scalar+Cryptography+Instruction+Set+Extension+Group+Names+Diagram

Attachment: signature.asc
Description: PGP signature