Re: [PATCH v1 02/13] riscv: add ISA extension probing for Zv* extensions

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7853 bytes --]

Hey,

On Thu, Oct 12, 2023 at 05:15:45PM +0200, Clément Léger wrote:
> On 12/10/2023 16:10, Conor Dooley wrote:
> > On Thu, Oct 12, 2023 at 03:17:14PM +0200, Clément Léger wrote:
> >> On 11/10/2023 13:14, Clément Léger wrote:
> >>> Add probing of some Zv* ISA extensions that are mentioned in "RISC-V
> >>> Cryptography Extensions Volume II" [1]. These ISA extensions are the
> >>> following:
> >>>
> >>> - Zvbb: Vector Basic Bit-manipulation
> >>> - Zvbc: Vector Carryless Multiplication
> >>> - Zvkb: Vector Cryptography Bit-manipulation
> >>> - Zvkg: Vector GCM/GMAC.
> >>> - Zvkned: NIST Suite: Vector AES Block Cipher
> >>> - Zvknh[ab]: NIST Suite: Vector SHA-2 Secure Hash
> >>> - Zvksed: ShangMi Suite: SM4 Block Cipher
> >>> - Zvksh: ShangMi Suite: SM3 Secure Hash
> >>> - Zvkn: NIST Algorithm Suite
> >>> - Zvknc: NIST Algorithm Suite with carryless multiply
> >>> - Zvkng: NIST Algorithm Suite with GCM.
> >>> - Zvks: ShangMi Algorithm Suite
> >>> - Zvksc: ShangMi Algorithm Suite with carryless multiplication
> >>> - Zvksg: ShangMi Algorithm Suite with GCM.
> >>> - Zvkt: Vector Data-Independent Execution Latency.
> >>>
> >>> [1] https://drive.google.com/file/d/1gb9OLH-DhbCgWp7VwpPOVrrY6f3oSJLL/view
> >>>
> >>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >>> ---
> >>>  arch/riscv/include/asm/hwcap.h | 16 ++++++++++++++++
> >>>  arch/riscv/kernel/cpufeature.c | 16 ++++++++++++++++
> >>>  2 files changed, 32 insertions(+)
> >>>
> >>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> >>> index b7b58258f6c7..4e46981ac6c8 100644
> >>> --- a/arch/riscv/include/asm/hwcap.h
> >>> +++ b/arch/riscv/include/asm/hwcap.h
> >>> @@ -58,6 +58,22 @@
> >>>  #define RISCV_ISA_EXT_ZICSR		40
> >>>  #define RISCV_ISA_EXT_ZIFENCEI		41
> >>>  #define RISCV_ISA_EXT_ZIHPM		42
> >>> +#define RISCV_ISA_EXT_ZVBB		43
> >>> +#define RISCV_ISA_EXT_ZVBC		44
> >>> +#define RISCV_ISA_EXT_ZVKB		45
> >>> +#define RISCV_ISA_EXT_ZVKG		46
> >>> +#define RISCV_ISA_EXT_ZVKN		47
> >>> +#define RISCV_ISA_EXT_ZVKNC		48
> >>> +#define RISCV_ISA_EXT_ZVKNED		49
> >>> +#define RISCV_ISA_EXT_ZVKNG		50
> >>> +#define RISCV_ISA_EXT_ZVKNHA		51
> >>> +#define RISCV_ISA_EXT_ZVKNHB		52
> >>> +#define RISCV_ISA_EXT_ZVKS		53
> >>> +#define RISCV_ISA_EXT_ZVKSC		54
> >>> +#define RISCV_ISA_EXT_ZVKSED		55
> >>> +#define RISCV_ISA_EXT_ZVKSH		56
> >>> +#define RISCV_ISA_EXT_ZVKSG		57
> >>
> >> About Zvks/Zvkn, these extensions are actually shorthand for a few other
> >> sub-extensions, it is still not clear if it should be parsed as is.
> >> There are multiple solutions:
> >>
> >> - Handle them as-is, simply enable the extension, if reported through
> >> hwprobe, userspace will be responsible to detect the sub-extensions
> >> (current approach)
> > 
> > I dislike this, since in-kernel users will have to check for "parent" &
> > "child" extensions.
> > 
> >> - "Unfold" the extension in order to enable all the sub-extensions and
> >> keep the main one (for instance for Zvkn, enable Zvkned, Zvknhb, Zvkb,
> >> Zvkt, Zvkn)
> > 
> > We threw together some code for this a few months ago after some
> > discussion with some of your Rivos colleagues. The initial version of it
> > was in this thread with Evan:
> > https://lore.kernel.org/all/20230703-mangle-panning-75909ebbe30c@spud/
> > and in a later iteration there was some more done by myself and Drew:
> > https://lore.kernel.org/all/20230713-bootleg-tray-c5bfe58b5673@wendy/
> > One of the versions ended up as the riscv-extensions-strings-scalar-crypto
> > branch in my k.org repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/log/?h=riscv-extensions-strings-scalar-crypto
> > 
> 
> Thanks for these information ! I think your version to handle extension
> group is pretty clean. Are you waiting for anything in particular except
> a Signed-off: from Evan to submit that patch ?

Lack of a user. I was hoping that it'd go alongside the crypto extension
stuff that needed it.

> If so, can I backport
> this patch in my branch, gather Evan SoB and rebase my series on top of it ?

For sure.

> > That crypto stuff has all gone quiet of late unfortunately. I wonder if
> > Samuel is still working on it.
> 
> I talked with Samuel and we agreed on the following plan: I'll actually
> carry on the bitmanip ISA part and he will resubmit the Zkr with
> archrandom part.

:+1:

> >> - "Unfold" but don't keep the extension "shorthand" in the ISA extension
> >> list (for instance for Zvkn, enable Zvkned, Zvknhb, Zvkb, Zvkt)
> > 
> > But I would also be fine with this one from a pure in-kernel PoV.
> 
> Which is the case with your version FWIU (ie, only the child extensions
> are visible).

Yeah, I think I might've done it intentionally so that the same thing
appeared in /proc/cpuinfo whether the "parent" or all the "children"
were provided in DT.

(I wrote that stuff before hwprobe got merge I think)

I think users would probably appreciate being able to poll for the whole
extension, rather than the component parts. There's probably also some
thoughts expressed in the two threads I linked, IIRC Evan and I did
discuss some of the behaviour there. What to do if an extension later
grows a subset springs to mind.

Conor.

> > I think it's likely to be annoying for users though, since they won't be
> > able to poll for the "parent" unless we re-assemble the parents in
> > hwprobe etc (eugh).
> 
> Indeed, and re-assembling the parent is IMHO duplication of the existing
> information. Checking that the needed ISA extensions are present will be
> simple enough (simple bitmask) so I'm not sure that re-assembling the
> parents is necessary (But that's a personal statement and I'm pretty
> sure others will like it to be provided directly).
> 
> Thanks,
> 
> Clément
> 
> > 
> > - don't permit passing the "parents" at all, and only deal with the
> >   "children". We can enforce this for DT, but not for ACPI, so probably
> >   not a runner>
> > Thanks,
> > Conor.
> > 
> >>
> >> Thanks,
> >>
> >> Clément
> >>
> >>> +#define RISCV_ISA_EXT_ZVKT		58
> >>>  
> >>>  #define RISCV_ISA_EXT_MAX		64
> >>>  
> >>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >>> index 1cfbba65d11a..859d647f3ced 100644
> >>> --- a/arch/riscv/kernel/cpufeature.c
> >>> +++ b/arch/riscv/kernel/cpufeature.c
> >>> @@ -174,6 +174,22 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> >>>  	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> >>>  	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> >>>  	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> >>> +	__RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB),
> >>> +	__RISCV_ISA_EXT_DATA(zvbc, RISCV_ISA_EXT_ZVBC),
> >>> +	__RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB),
> >>> +	__RISCV_ISA_EXT_DATA(zvkg, RISCV_ISA_EXT_ZVKG),
> >>> +	__RISCV_ISA_EXT_DATA(zvkn, RISCV_ISA_EXT_ZVKN),
> >>> +	__RISCV_ISA_EXT_DATA(zvknc, RISCV_ISA_EXT_ZVKNC),
> >>> +	__RISCV_ISA_EXT_DATA(zvkned, RISCV_ISA_EXT_ZVKNED),
> >>> +	__RISCV_ISA_EXT_DATA(zvkng, RISCV_ISA_EXT_ZVKNG),
> >>> +	__RISCV_ISA_EXT_DATA(zvknha, RISCV_ISA_EXT_ZVKNHA),
> >>> +	__RISCV_ISA_EXT_DATA(zvknhb, RISCV_ISA_EXT_ZVKNHB),
> >>> +	__RISCV_ISA_EXT_DATA(zvks, RISCV_ISA_EXT_ZVKS),
> >>> +	__RISCV_ISA_EXT_DATA(zvksc, RISCV_ISA_EXT_ZVKSC),
> >>> +	__RISCV_ISA_EXT_DATA(zvksed, RISCV_ISA_EXT_ZVKSED),
> >>> +	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
> >>> +	__RISCV_ISA_EXT_DATA(zvksg, RISCV_ISA_EXT_ZVKSG),
> >>> +	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
> >>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> >>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> >>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]