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

On Wed, Jul 12, 2023 at 3:39 AM Conor Dooley <[email protected]> wrote:
>
> Hey Samuel, Evan,
>
> On Wed, Jul 12, 2023 at 10:41:17AM +0200, Samuel Ortiz wrote:
> > From: "Hongren (Zenithal) Zheng" <[email protected]>
> >
> > Parse Zb/Zk related string from DT and output them to cpuinfo.
>
> One thing that has sprung to mind is that this is not limited to DT
> anymore, since the information could in theory come from ACPI too.
> Ditto the title I guess.
>
> > It is worth noting that the Scalar Crypto extension defines "zk" as a
> > shorthand for the Zkn, Zkr and Zkt extensions. Since the Zkn one also
> > implies the Zbkb, Zbkc and Zbkx extensions, simply passing the valid
> > "zk" extension name through a DT will enable all of the Zbkb, Zbkc,
> > Zbkx, Zkn, Zkr and Zkt extensions.
> >
> > Also, since there currently is no mechanism to merge all enabled
> > extensions, the generated cpuinfo output could be relatively large.
> > For example, setting the "riscv,isa" DT property to "rv64imafdc_zk_zks"
> > will generate the following cpuinfo output:
> > "rv64imafdc_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zksed_zksh_zkt".
>
> On that note, I've created another version of what checking for
> supersets could look like, since it'll be needed either by my series or
> this one, depending on what gets merged first. I've yet to test the
> dedicated extensions part of it, but I wanted to get this out before I
> went looking at other fixes in the area.
>
> Evan, since it was you that commented on this stuff last time around,
> could you take another look? I'm still not keen on the "subset_of"
> arrays, but they're an improvement on what I had last time around for
> sure.
>

This looks alright to me. At the risk of getting into bikeshedding
territory, the only awkward bit of it is it composes the extensions in
sort of the opposite way you'd expect. I tend to think of Zks as being
comprised of {zbkb, zbkc, zksed, zksh}, rather than zbkb being a part
of {zks, zkn, zk}, though both are of course correct. Here's an
untested version of the other way. You can decide if you like it
better or worse than what you've got, and I'm fine either way. Sorry
gmail mangles it, if you want the patch for real I can get it to you:

From e201c34c05cd82812b5b3f47ccdd7d5909259f07 Mon Sep 17 00:00:00 2001
From: Evan Green <[email protected]>
Date: Wed, 12 Jul 2023 10:36:15 -0700
Subject: [PATCH] WIP: RISC-V: Allow support for bundled extensions, and add Zk*

---
arch/riscv/include/asm/hwcap.h | 13 ++++++
arch/riscv/kernel/cpufeature.c | 82 +++++++++++++++++++++++++++++-----
2 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index b7b58258f6c7..7d2d10b42cf3 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -58,6 +58,17 @@
#define RISCV_ISA_EXT_ZICSR 40
#define RISCV_ISA_EXT_ZIFENCEI 41
#define RISCV_ISA_EXT_ZIHPM 42
+#define RISCV_ISA_EXT_ZBC 43
+#define RISCV_ISA_EXT_ZBKB 44
+#define RISCV_ISA_EXT_ZBKC 45
+#define RISCV_ISA_EXT_ZBKX 46
+#define RISCV_ISA_EXT_ZKND 47
+#define RISCV_ISA_EXT_ZKNE 48
+#define RISCV_ISA_EXT_ZKNH 49
+#define RISCV_ISA_EXT_ZKR 50
+#define RISCV_ISA_EXT_ZKSED 51
+#define RISCV_ISA_EXT_ZKSH 52
+#define RISCV_ISA_EXT_ZKT 53

#define RISCV_ISA_EXT_MAX 64

@@ -77,6 +88,8 @@ struct riscv_isa_ext_data {
const unsigned int id;
const char *name;
const char *property;
+ const unsigned int *bundled_exts;
+ const unsigned int bundle_size;
};

extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 5945dfc5f806..2a1f958c1777 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -105,6 +105,39 @@ static bool riscv_isa_extension_check(int id)
.id = _id, \
}

+#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) { \
+ .name = #_name, \
+ .property = #_name, \
+ .bundled_exts = _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,
+};
+
+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
+};
+
/*
* The canonical order of ISA extension names in the ISA string is defined in
* chapter 27 of the unprivileged specification.
@@ -167,7 +200,20 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
__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_BUNDLE(zk, riscv_zk_bundled_exts),
+ __RISCV_ISA_EXT_BUNDLE(zkn, riscv_zkn_bundled_exts),
+ __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_BUNDLE(zks, riscv_zks_bundled_exts),
+ __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED),
+ __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH),
__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),
@@ -179,6 +225,30 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {

const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);

+static void match_isa_ext(const struct riscv_isa_ext_data *ext, const
char *name,
+ const char *name_end, struct riscv_isainfo *isainfo)
+{
+ if ((name_end - name == strlen(ext->name)) &&
+ !strncasecmp(name, ext->name, name_end - name)) {
+
+ /*
+ * If this is a bundle, enable all the ISA extensions that
+ * comprise the bundle.
+ */
+ if (ext->bundle_size) {
+ unsigned int i;
+ for (i = 0; i < ext->bundle_size; i++) {
+ if
(riscv_isa_extension_check(ext->bundled_exts[i]))
+ set_bit(ext->bundled_exts[i],
isainfo->isa);
+ }
+
+
+ } else if (riscv_isa_extension_check(ext->id)) {
+ set_bit(ext->id, isainfo->isa);
+ }
+ }
+}
+
static void __init riscv_parse_isa_string(unsigned long *this_hwcap,
struct riscv_isainfo *isainfo,
unsigned long *isa2hwcap,
const char *isa)
{
@@ -310,14 +380,6 @@ static void __init
riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
if (*isa == '_')
++isa;

-#define SET_ISA_EXT_MAP(name, bit)
\
- do {
\
- if ((ext_end - ext == sizeof(name) - 1) &&
\
- !strncasecmp(ext, name, sizeof(name) - 1)
&& \
- riscv_isa_extension_check(bit))
\
- set_bit(bit, isainfo->isa);
\
- } while (false)
\
-
if (unlikely(ext_err))
continue;
if (!ext_long) {
@@ -329,10 +391,8 @@ static void __init
riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
}
} else {
for (int i = 0; i < riscv_isa_ext_count; i++)
- SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
- riscv_isa_ext[i].id);
+ match_isa_ext(&riscv_isa_ext[i], ext,
ext_end, isainfo);
}
-#undef SET_ISA_EXT_MAP
}
}

2023-07-13 09:06:49

by Andrew Jones

[permalink] [raw]

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

On Wed, Jul 12, 2023 at 10:43:33AM -0700, Evan Green wrote:
> On Wed, Jul 12, 2023 at 3:39 AM Conor Dooley <[email protected]> wrote:
> >
> > Hey Samuel, Evan,
> >
> > On Wed, Jul 12, 2023 at 10:41:17AM +0200, Samuel Ortiz wrote:
> > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > >
> > > Parse Zb/Zk related string from DT and output them to cpuinfo.
> >
> > One thing that has sprung to mind is that this is not limited to DT
> > anymore, since the information could in theory come from ACPI too.
> > Ditto the title I guess.
> >
> > > It is worth noting that the Scalar Crypto extension defines "zk" as a
> > > shorthand for the Zkn, Zkr and Zkt extensions. Since the Zkn one also
> > > implies the Zbkb, Zbkc and Zbkx extensions, simply passing the valid
> > > "zk" extension name through a DT will enable all of the Zbkb, Zbkc,
> > > Zbkx, Zkn, Zkr and Zkt extensions.
> > >
> > > Also, since there currently is no mechanism to merge all enabled
> > > extensions, the generated cpuinfo output could be relatively large.
> > > For example, setting the "riscv,isa" DT property to "rv64imafdc_zk_zks"
> > > will generate the following cpuinfo output:
> > > "rv64imafdc_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zksed_zksh_zkt".
> >
> > On that note, I've created another version of what checking for
> > supersets could look like, since it'll be needed either by my series or
> > this one, depending on what gets merged first. I've yet to test the
> > dedicated extensions part of it, but I wanted to get this out before I
> > went looking at other fixes in the area.
> >
> > Evan, since it was you that commented on this stuff last time around,
> > could you take another look? I'm still not keen on the "subset_of"
> > arrays, but they're an improvement on what I had last time around for
> > sure.
> >
>
> This looks alright to me. At the risk of getting into bikeshedding
> territory, the only awkward bit of it is it composes the extensions in
> sort of the opposite way you'd expect. I tend to think of Zks as being
> comprised of {zbkb, zbkc, zksed, zksh},

This is also the way I think of it, so, FWIW, I prefer the approach below,
where bundles are expanded.

Thanks,
drew

> rather than zbkb being a part
> of {zks, zkn, zk}, though both are of course correct. Here's an
> untested version of the other way. You can decide if you like it
> better or worse than what you've got, and I'm fine either way. Sorry
> gmail mangles it, if you want the patch for real I can get it to you:
>
> From e201c34c05cd82812b5b3f47ccdd7d5909259f07 Mon Sep 17 00:00:00 2001
> From: Evan Green <[email protected]>
> Date: Wed, 12 Jul 2023 10:36:15 -0700
> Subject: [PATCH] WIP: RISC-V: Allow support for bundled extensions, and add Zk*
>
> ---
> arch/riscv/include/asm/hwcap.h | 13 ++++++
> arch/riscv/kernel/cpufeature.c | 82 +++++++++++++++++++++++++++++-----
> 2 files changed, 84 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index b7b58258f6c7..7d2d10b42cf3 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -58,6 +58,17 @@
> #define RISCV_ISA_EXT_ZICSR 40
> #define RISCV_ISA_EXT_ZIFENCEI 41
> #define RISCV_ISA_EXT_ZIHPM 42
> +#define RISCV_ISA_EXT_ZBC 43
> +#define RISCV_ISA_EXT_ZBKB 44
> +#define RISCV_ISA_EXT_ZBKC 45
> +#define RISCV_ISA_EXT_ZBKX 46
> +#define RISCV_ISA_EXT_ZKND 47
> +#define RISCV_ISA_EXT_ZKNE 48
> +#define RISCV_ISA_EXT_ZKNH 49
> +#define RISCV_ISA_EXT_ZKR 50
> +#define RISCV_ISA_EXT_ZKSED 51
> +#define RISCV_ISA_EXT_ZKSH 52
> +#define RISCV_ISA_EXT_ZKT 53
>
> #define RISCV_ISA_EXT_MAX 64
>
> @@ -77,6 +88,8 @@ struct riscv_isa_ext_data {
> const unsigned int id;
> const char *name;
> const char *property;
> + const unsigned int *bundled_exts;
> + const unsigned int bundle_size;
> };
>
> extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5945dfc5f806..2a1f958c1777 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -105,6 +105,39 @@ static bool riscv_isa_extension_check(int id)
> .id = _id, \
> }
>
> +#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) { \
> + .name = #_name, \
> + .property = #_name, \
> + .bundled_exts = _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,
> +};
> +
> +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
> +};
> +
> /*
> * The canonical order of ISA extension names in the ISA string is defined in
> * chapter 27 of the unprivileged specification.
> @@ -167,7 +200,20 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> __RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> __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_BUNDLE(zk, riscv_zk_bundled_exts),
> + __RISCV_ISA_EXT_BUNDLE(zkn, riscv_zkn_bundled_exts),
> + __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_BUNDLE(zks, riscv_zks_bundled_exts),
> + __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED),
> + __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH),
> __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),
> @@ -179,6 +225,30 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>
> const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>
> +static void match_isa_ext(const struct riscv_isa_ext_data *ext, const
> char *name,
> + const char *name_end, struct riscv_isainfo *isainfo)
> +{
> + if ((name_end - name == strlen(ext->name)) &&
> + !strncasecmp(name, ext->name, name_end - name)) {
> +
> + /*
> + * If this is a bundle, enable all the ISA extensions that
> + * comprise the bundle.
> + */
> + if (ext->bundle_size) {
> + unsigned int i;
> + for (i = 0; i < ext->bundle_size; i++) {
> + if
> (riscv_isa_extension_check(ext->bundled_exts[i]))
> + set_bit(ext->bundled_exts[i],
> isainfo->isa);
> + }
> +
> +
> + } else if (riscv_isa_extension_check(ext->id)) {
> + set_bit(ext->id, isainfo->isa);
> + }
> + }
> +}
> +
> static void __init riscv_parse_isa_string(unsigned long *this_hwcap,
> struct riscv_isainfo *isainfo,
> unsigned long *isa2hwcap,
> const char *isa)
> {
> @@ -310,14 +380,6 @@ static void __init
> riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
> if (*isa == '_')
> ++isa;
>
> -#define SET_ISA_EXT_MAP(name, bit)
> \
> - do {
> \
> - if ((ext_end - ext == sizeof(name) - 1) &&
> \
> - !strncasecmp(ext, name, sizeof(name) - 1)
> && \
> - riscv_isa_extension_check(bit))
> \
> - set_bit(bit, isainfo->isa);
> \
> - } while (false)
> \
> -
> if (unlikely(ext_err))
> continue;
> if (!ext_long) {
> @@ -329,10 +391,8 @@ static void __init
> riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
> }
> } else {
> for (int i = 0; i < riscv_isa_ext_count; i++)
> - SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> - riscv_isa_ext[i].id);
> + match_isa_ext(&riscv_isa_ext[i], ext,
> ext_end, isainfo);
> }
> -#undef SET_ISA_EXT_MAP
> }
> }

2023-07-13 11:42:32

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

Attachments:

(No filename) (2.71 kB)
signature.asc (235.00 B)
Download all attachments

2023-10-12 16:28:35

by Evan Green

[permalink] [raw]

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

On Thu, Jul 13, 2023 at 4:28 AM Conor Dooley <[email protected]> wrote:
>
> On Thu, Jul 13, 2023 at 10:46:14AM +0200, Andrew Jones wrote:
> > On Wed, Jul 12, 2023 at 10:43:33AM -0700, Evan Green wrote:
> > > On Wed, Jul 12, 2023 at 3:39 AM Conor Dooley <[email protected]> wrote:
> > > > On Wed, Jul 12, 2023 at 10:41:17AM +0200, Samuel Ortiz wrote:
> > > > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > > > >
> > > > > Parse Zb/Zk related string from DT and output them to cpuinfo.
> > > >
> > > > One thing that has sprung to mind is that this is not limited to DT
> > > > anymore, since the information could in theory come from ACPI too.
> > > > Ditto the title I guess.
> > > >
> > > > > It is worth noting that the Scalar Crypto extension defines "zk" as a
> > > > > shorthand for the Zkn, Zkr and Zkt extensions. Since the Zkn one also
> > > > > implies the Zbkb, Zbkc and Zbkx extensions, simply passing the valid
> > > > > "zk" extension name through a DT will enable all of the Zbkb, Zbkc,
> > > > > Zbkx, Zkn, Zkr and Zkt extensions.
> > > > >
> > > > > Also, since there currently is no mechanism to merge all enabled
> > > > > extensions, the generated cpuinfo output could be relatively large.
> > > > > For example, setting the "riscv,isa" DT property to "rv64imafdc_zk_zks"
> > > > > will generate the following cpuinfo output:
> > > > > "rv64imafdc_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zksed_zksh_zkt".
> > > >
> > > > On that note, I've created another version of what checking for
> > > > supersets could look like, since it'll be needed either by my series or
> > > > this one, depending on what gets merged first. I've yet to test the
> > > > dedicated extensions part of it, but I wanted to get this out before I
> > > > went looking at other fixes in the area.
> > > >
> > > > Evan, since it was you that commented on this stuff last time around,
> > > > could you take another look? I'm still not keen on the "subset_of"
> > > > arrays, but they're an improvement on what I had last time around for
> > > > sure.
> > > >
> > >
> > > This looks alright to me. At the risk of getting into bikeshedding
> > > territory, the only awkward bit of it is it composes the extensions in
> > > sort of the opposite way you'd expect. I tend to think of Zks as being
> > > comprised of {zbkb, zbkc, zksed, zksh},
> >
> > This is also the way I think of it, so, FWIW, I prefer the approach below,
> > where bundles are expanded.
>
> I took the patch Evan sent me off-list & have spun it into a "real"
> patch. I did some minor changes (c99 loop, __init marking) to match what
> the code was already doing and expanded it to work for the new property
> stuff.
> I'm not sure that match_isa_ext() needs to be a function, given the
> single user - it might be better refactored to drop the outer if & return
> whether a match was found.
> If done that would allow sharing the same function between isa string and
> dedicated property parsing. I left it intact for now. Might also be
> worth adding kerneldoc for that struct.
>
> -- >8 --
> From 8300b98e487e57d192bc9581c6a3639c40c52623 Mon Sep 17 00:00:00 2001
> From: Evan Green <[email protected]>
> Date: Wed, 12 Jul 2023 10:36:15 -0700
> Subject: [PATCH] RISC-V: Add support Scalar Crypto using "bundled extensions"
>
> The Scalar Crypto specification defines Zk as a shorthand for the
> Zkn, Zkr and Zkt extensions. The same follows for both Zkn, Zks and Zbk,
> which are all shorthands for various other extensions. The detailed
> breakdown can be found in their dt-binding entries.
>
> Since Zkn also implies the Zbkb, Zbkc and Zbkx extensions, simply passing
> "zk" through a DT should enable all of Zbkb, Zbkc, Zbkx, Zkn, Zkr and Zkt.
> For example, setting the "riscv,isa" DT property to "rv64imafdc_zk"
> should generate the following cpuinfo output:
> "rv64imafdc_zicntr_zicsr_zifencei_zihpm_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zkt"
>
> riscv_isa_ext_data grows a pair of new members, to permit setting the
> relevant bits for "bundled" extensions, both while parsing the ISA string
> and the new dedicated extension properties
>
> Co-developed-by: Conor Dooley <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> There's basically none of the original patch left, so I have dropped the
> co-developed-bys and signed-off-bys from before. It does need one from
> you now though Evan!

Oops, I hadn't realized this was blocked on me!

Signed-off-by: Evan Green <[email protected]>