From: "Hongren (Zenithal) Zheng" <[email protected]>
This patch 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.
This patch also changes the logical id of
existing multi-letter extensions and adds a statement
that instead of logical id compatibility, the order
is needed.
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 <[email protected]>
Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 11 +++++++++++
arch/riscv/kernel/cpu.c | 11 +++++++++++
arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index f041bfa7f6a0..b80ca6e77088 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -53,6 +53,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
#define RISCV_ISA_EXT_NAME_LEN_MAX 32
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index a2fc952318e9..10524322a4c0 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
__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_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(smaia, RISCV_ISA_EXT_SMAIA),
__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index bdcf460ea53d..447f853a5a4c 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
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("zicbom", RISCV_ISA_EXT_ZICBOM);
SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
+ 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("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("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
}
--
2.41.0
On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
>
> From: "Hongren (Zenithal) Zheng" <[email protected]>
>
> This patch 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.
>
> This patch also changes the logical id of
> existing multi-letter extensions and adds a statement
> that instead of logical id compatibility, the order
> is needed.
>
> 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 <[email protected]>
> Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
> ---
> arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> arch/riscv/kernel/cpu.c | 11 +++++++++++
> arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index f041bfa7f6a0..b80ca6e77088 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -53,6 +53,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
> #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..10524322a4c0 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..447f853a5a4c 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> 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);
Should "zbks" be "zbkx"?
> SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> + 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("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("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);
It would be nice to consolidate the ones together that search for a
single string and set multiple bits, though I don't have any super
elegant ideas for how off the top of my head.
On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > >
> > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > >
> > > This patch parses Zb/Zk related string from DT and
>
> %s/This patch//
>
> > > 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.
>
> Please explain why this is okay.
From riscv scalar crypto spec, zk is a shorthand
for zkn, zkr and zkt and zkn also includes zbkb, zbkc
and zbkx.
>
> > > Note that zk is a valid extension name and the current
> > > DT binding spec allows this.
> > >
> > > This patch also changes the logical id of
> > > existing multi-letter extensions and adds a statement
> > > that instead of logical id compatibility, the order
> > > is needed.
>
> Does it?
That is in the old version of this patch,
should be removed now
see https://lore.kernel.org/linux-riscv/YqY0aSngjI0Hc5d4@Sun/
>
> > > 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"
>
> I think this is fine.
>
> Please re-wrap this all to 72 characters.
>
> > >
> > > Tested-by: Jiatai He <[email protected]>
> > > Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
>
> This is missing your SoB Samuel.
>
> > > ---
> > > arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> > > arch/riscv/kernel/cpu.c | 11 +++++++++++
> > > arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> > > 3 files changed, 52 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index f041bfa7f6a0..b80ca6e77088 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -53,6 +53,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
> > > #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index a2fc952318e9..10524322a4c0 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> > > __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > > __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index bdcf460ea53d..447f853a5a4c 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> > > SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > > SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> > > 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);
>
> This order does not look correct, please add them in alphanumerical
> order as the comment these SET_ISA_EXT_MAP()s requests. Ditto below.
Agreed. Seems that I did not worked carefully for this part.
>
> > > + SET_ISA_EXT_MAP("zbkc", RISCV_ISA_EXT_ZBKC);
> > > + SET_ISA_EXT_MAP("zbks", RISCV_ISA_EXT_ZBKX);
> >
> > Should "zbks" be "zbkx"?
> >
> > > SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> > > SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> > > SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > > + 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("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("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);
> >
> > It would be nice to consolidate the ones together that search for a
> > single string and set multiple bits, though I don't have any super
> > elegant ideas for how off the top of my head.
>
> I've got a refactor of this code in progress, dropping all of these
> copy-paste in place of a loop. It certainly looks more elegant than
> this, but it will fall over a bit for these "one string matches many
> extensions" cases. See here:
> https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> My immediate thought is to add another element to riscv_isa_ext_data,
> that contains "parent" extensions to check for. Should be fairly doable,
> I'll whip something up on top of that...
>
> Cheers,
> Conor.
On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> >
> > From: "Hongren (Zenithal) Zheng" <[email protected]>
> >
> > This patch parses Zb/Zk related string from DT and
%s/This patch//
> > 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.
Please explain why this is okay.
> > Note that zk is a valid extension name and the current
> > DT binding spec allows this.
> >
> > This patch also changes the logical id of
> > existing multi-letter extensions and adds a statement
> > that instead of logical id compatibility, the order
> > is needed.
Does it?
> > 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"
I think this is fine.
Please re-wrap this all to 72 characters.
> >
> > Tested-by: Jiatai He <[email protected]>
> > Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
This is missing your SoB Samuel.
> > ---
> > arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> > arch/riscv/kernel/cpu.c | 11 +++++++++++
> > arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> > 3 files changed, 52 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index f041bfa7f6a0..b80ca6e77088 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -53,6 +53,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
> > #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index a2fc952318e9..10524322a4c0 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> > __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> > __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index bdcf460ea53d..447f853a5a4c 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> > SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> > 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);
This order does not look correct, please add them in alphanumerical
order as the comment these SET_ISA_EXT_MAP()s requests. Ditto below.
> > + SET_ISA_EXT_MAP("zbkc", RISCV_ISA_EXT_ZBKC);
> > + SET_ISA_EXT_MAP("zbks", RISCV_ISA_EXT_ZBKX);
>
> Should "zbks" be "zbkx"?
>
> > SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> > SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> > SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > + 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("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("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);
>
> It would be nice to consolidate the ones together that search for a
> single string and set multiple bits, though I don't have any super
> elegant ideas for how off the top of my head.
I've got a refactor of this code in progress, dropping all of these
copy-paste in place of a loop. It certainly looks more elegant than
this, but it will fall over a bit for these "one string matches many
extensions" cases. See here:
https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
My immediate thought is to add another element to riscv_isa_ext_data,
that contains "parent" extensions to check for. Should be fairly doable,
I'll whip something up on top of that...
Cheers,
Conor.
On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> >
> > From: "Hongren (Zenithal) Zheng" <[email protected]>
> >
> > This patch 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.
> >
> > This patch also changes the logical id of
> > existing multi-letter extensions and adds a statement
> > that instead of logical id compatibility, the order
> > is needed.
> >
> > 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 <[email protected]>
> > Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
I think an extra line of your own signed-off-by is needed
> > ---
> > arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> > arch/riscv/kernel/cpu.c | 11 +++++++++++
> > arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> > 3 files changed, 52 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index f041bfa7f6a0..b80ca6e77088 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -53,6 +53,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
> > #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index a2fc952318e9..10524322a4c0 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> > __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> > __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index bdcf460ea53d..447f853a5a4c 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> > SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> > 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);
>
> Should "zbks" be "zbkx"?
Yes that is a nice catch!
>
> > SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> > SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> > SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > + 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("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("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);
>
> It would be nice to consolidate the ones together that search for a
> single string and set multiple bits, though I don't have any super
> elegant ideas for how off the top of my head.
On Wed, Jun 28, 2023 at 03:03:58AM +0800, Hongren (Zenithal) Zheng wrote:
> On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > > >
> > > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > > >
> > > > This patch parses Zb/Zk related string from DT and
> >
> > %s/This patch//
> >
> > > > 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.
> >
> > Please explain why this is okay.
>
> From riscv scalar crypto spec, zk is a shorthand
> for zkn, zkr and zkt and zkn also includes zbkb, zbkc
> and zbkx.
Hmm, seems you misunderstood, sorry about that.
What I was looking for is an explanation of this in the commit message.
Hope that helps,
Conor.
On Wed, Jun 28, 2023 at 03:03:58AM +0800, Hongren (Zenithal) Zheng wrote:
> On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > > >
> > > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > > >
> > > > This patch parses Zb/Zk related string from DT and
> >
> > %s/This patch//
> >
> > > > 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.
> >
> > Please explain why this is okay.
>
> From riscv scalar crypto spec, zk is a shorthand
> for zkn, zkr and zkt and zkn also includes zbkb, zbkc
> and zbkx.
>
> >
> > > > Note that zk is a valid extension name and the current
> > > > DT binding spec allows this.
> > > >
> > > > This patch also changes the logical id of
> > > > existing multi-letter extensions and adds a statement
> > > > that instead of logical id compatibility, the order
> > > > is needed.
> >
> > Does it?
>
> That is in the old version of this patch,
> should be removed now
> see https://lore.kernel.org/linux-riscv/YqY0aSngjI0Hc5d4@Sun/
>
> >
> > > > 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"
> >
> > I think this is fine.
> >
> > Please re-wrap this all to 72 characters.
> >
> > > >
> > > > Tested-by: Jiatai He <[email protected]>
> > > > Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
> >
> > This is missing your SoB Samuel.
> >
> > > > ---
> > > > arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> > > > arch/riscv/kernel/cpu.c | 11 +++++++++++
> > > > arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> > > > 3 files changed, 52 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > index f041bfa7f6a0..b80ca6e77088 100644
> > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > @@ -53,6 +53,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
> > > > #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > index a2fc952318e9..10524322a4c0 100644
> > > > --- a/arch/riscv/kernel/cpu.c
> > > > +++ b/arch/riscv/kernel/cpu.c
> > > > @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > > __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> > > > __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > > > __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index bdcf460ea53d..447f853a5a4c 100644
> > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> > > > SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > > > SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> > > > 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);
> >
> > This order does not look correct, please add them in alphanumerical
> > order as the comment these SET_ISA_EXT_MAP()s requests. Ditto below.
>
> Agreed. Seems that I did not worked carefully for this part.
Or it could be me when rebasing that patch. In any case, it will be
fixed with v2 of the patch.
Cheers,
Samuel.
On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > >
> > > From: "Hongren (Zenithal) Zheng" <[email protected]>
> > >
> > > This patch parses Zb/Zk related string from DT and
>
> %s/This patch//
>
> > > 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.
>
> Please explain why this is okay.
>
> > > Note that zk is a valid extension name and the current
> > > DT binding spec allows this.
> > >
> > > This patch also changes the logical id of
> > > existing multi-letter extensions and adds a statement
> > > that instead of logical id compatibility, the order
> > > is needed.
>
> Does it?
>
> > > 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"
>
> I think this is fine.
>
> Please re-wrap this all to 72 characters.
>
> > >
> > > Tested-by: Jiatai He <[email protected]>
> > > Signed-off-by: Hongren (Zenithal) Zheng <[email protected]>
>
> This is missing your SoB Samuel.
>
> > > ---
> > > arch/riscv/include/asm/hwcap.h | 11 +++++++++++
> > > arch/riscv/kernel/cpu.c | 11 +++++++++++
> > > arch/riscv/kernel/cpufeature.c | 30 ++++++++++++++++++++++++++++++
> > > 3 files changed, 52 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index f041bfa7f6a0..b80ca6e77088 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -53,6 +53,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
> > > #define RISCV_ISA_EXT_NAME_LEN_MAX 32
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index a2fc952318e9..10524322a4c0 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -215,7 +215,18 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > __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_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(smaia, RISCV_ISA_EXT_SMAIA),
> > > __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > > __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index bdcf460ea53d..447f853a5a4c 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -309,10 +309,40 @@ void __init riscv_fill_hwcap(void)
> > > SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > > SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> > > 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);
>
> This order does not look correct, please add them in alphanumerical
> order as the comment these SET_ISA_EXT_MAP()s requests. Ditto below.
>
> > > + SET_ISA_EXT_MAP("zbkc", RISCV_ISA_EXT_ZBKC);
> > > + SET_ISA_EXT_MAP("zbks", RISCV_ISA_EXT_ZBKX);
> >
> > Should "zbks" be "zbkx"?
> >
> > > SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> > > SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> > > SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > > + 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("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("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);
> >
> > It would be nice to consolidate the ones together that search for a
> > single string and set multiple bits, though I don't have any super
> > elegant ideas for how off the top of my head.
>
> I've got a refactor of this code in progress, dropping all of these
> copy-paste in place of a loop. It certainly looks more elegant than
> this, but it will fall over a bit for these "one string matches many
> extensions" cases. See here:
> https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> My immediate thought is to add another element to riscv_isa_ext_data,
> that contains "parent" extensions to check for. Should be fairly doable,
> I'll whip something up on top of that...
Nice, and thanks for the review.
Should I wait for your refactor to be merged before pushing this one?
Cheers,
Samuel.
On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > > It would be nice to consolidate the ones together that search for a
> > > single string and set multiple bits, though I don't have any super
> > > elegant ideas for how off the top of my head.
> >
> > I've got a refactor of this code in progress, dropping all of these
> > copy-paste in place of a loop. It certainly looks more elegant than
> > this, but it will fall over a bit for these "one string matches many
> > extensions" cases. See here:
> > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > My immediate thought is to add another element to riscv_isa_ext_data,
> > that contains "parent" extensions to check for. Should be fairly doable,
> > I'll whip something up on top of that...
>
> Nice, and thanks for the review.
> Should I wait for your refactor to be merged before pushing this one?
I don't know. I think that you should continue on with your series here,
and whichever goes in second gets rebased on top of the other.
I don't think it makes material difference to review of this patchset as
to whether you rebase on top of what I'm working on, so I wouldn't
bother until it gets merged.
Rather hacky, had less time than expected this morning:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
repurposed Zicsr for the sake of testing something in the time I had.
Evan, at a high level, does that look more elegant to you, or have I made
things worse?
On Wed, Jun 28, 2023 at 12:10:11PM +0100, Conor Dooley wrote:
> On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
>
> > > > It would be nice to consolidate the ones together that search for a
> > > > single string and set multiple bits, though I don't have any super
> > > > elegant ideas for how off the top of my head.
> > >
> > > I've got a refactor of this code in progress, dropping all of these
> > > copy-paste in place of a loop. It certainly looks more elegant than
> > > this, but it will fall over a bit for these "one string matches many
> > > extensions" cases. See here:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > that contains "parent" extensions to check for. Should be fairly doable,
> > > I'll whip something up on top of that...
> >
> > Nice, and thanks for the review.
>
> > Should I wait for your refactor to be merged before pushing this one?
>
> I don't know. I think that you should continue on with your series here,
> and whichever goes in second gets rebased on top of the other.
Sounds good to me, thanks.
Cheers,
Samuel.
On Wed, Jun 28, 2023 at 01:21:01PM +0200, Markus Elfring wrote:
> …
> > This patch also changes the logical id of
> …
>
> Does such a wording really fit to the known requirement “Solve only one problem per patch.”?
That comment is inaccurate anyways. I removed it from v2 of the patch
set.
Cheers,
Samuel.
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81
>
>
> Please choose an imperative change suggestion.
>
> Regards,
> Markus
On Wed, Jun 28, 2023 at 12:10:11PM +0100, Conor Dooley wrote:
> On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
>
> > > > It would be nice to consolidate the ones together that search for a
> > > > single string and set multiple bits, though I don't have any super
> > > > elegant ideas for how off the top of my head.
> > >
> > > I've got a refactor of this code in progress, dropping all of these
> > > copy-paste in place of a loop. It certainly looks more elegant than
> > > this, but it will fall over a bit for these "one string matches many
> > > extensions" cases. See here:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > that contains "parent" extensions to check for. Should be fairly doable,
> > > I'll whip something up on top of that...
> >
> > Nice, and thanks for the review.
>
> > Should I wait for your refactor to be merged before pushing this one?
>
> I don't know. I think that you should continue on with your series here,
> and whichever goes in second gets rebased on top of the other.
> I don't think it makes material difference to review of this patchset as
> to whether you rebase on top of what I'm working on, so I wouldn't
> bother until it gets merged.
>
> Rather hacky, had less time than expected this morning:
> https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
> Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
> repurposed Zicsr for the sake of testing something in the time I had.
>
> Evan, at a high level, does that look more elegant to you, or have I made
> things worse?
Got some more time this afternoon, cleaned it up a bit. On top of what
is in
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/log/?h=riscv-extensions-strings
IOW, the not-yet-sent v2 of
https://lore.kernel.org/all/20230626-provable-angrily-81760e8c3cc6@wendy/
here's some infra for doing superset stuff...
Going to send my v2 without this, as there's nothing in tree right now
that actually needs this sort of thing.
-- >8 --
From 0875e1aa2bf773b53cae02490ebc69e798e491c4 Mon Sep 17 00:00:00 2001
From: Conor Dooley <[email protected]>
Date: Wed, 28 Jun 2023 12:01:40 +0100
Subject: [PATCH] RISC-V: detect extension support from superset extensions
Some extensions, for example scalar crypto's Zk extension, are comprised
of anumber of sub-extensions that may be implemented independently.
Provide some infrastructure for detecting support for an extension where
only its superset appears in DT or ACPI.
SET_ISA_EXT_MAP() already served little purpose, move the code into an
inline function instead, where the code can be reused more easily by the
riscv_try_set_ext_from_supersets().
Signed-off-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 3 +
arch/riscv/kernel/cpufeature.c | 107 ++++++++++++++++++++++++++++-----
2 files changed, 96 insertions(+), 14 deletions(-)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index c4d6faa5cdf8..5b41a7aa9ec5 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -73,11 +73,14 @@
unsigned long riscv_get_elf_hwcap(void);
+#define RISCV_ISA_MAX_SUPERSETS 1
struct riscv_isa_ext_data {
const unsigned int id;
const char *name;
const char *property;
const bool multi_letter;
+ const unsigned int num_supersets;
+ const char *supersets[RISCV_ISA_MAX_SUPERSETS];
};
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 53b38f6c0562..0d56f4a11a3e 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -104,6 +104,16 @@ static bool riscv_isa_extension_check(int id)
.property = #_name, \
.id = _id, \
.multi_letter = _multi, \
+ .num_supersets = 0, \
+}
+
+#define __RISCV_ISA_EXT_DATA_SUBSET(_name, _id, _multi, _num_supersets, ...) { \
+ .name = #_name, \
+ .property = #_name, \
+ .id = _id, \
+ .multi_letter = _multi, \
+ .num_supersets = _num_supersets, \
+ .supersets = {__VA_ARGS__}, \
}
/*
@@ -180,6 +190,39 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
+static inline int __init riscv_try_set_ext(const char *name, const unsigned int bit,
+ const char *ext, const char *ext_end,
+ struct riscv_isainfo *isainfo)
+{
+ if ((ext_end - ext == strlen(name)) &&
+ !strncasecmp(ext, name, strlen(name)) &&
+ riscv_isa_extension_check(bit)) {
+ set_bit(bit, isainfo->isa);
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static inline void __init riscv_try_set_ext_from_supersets(struct riscv_isa_ext_data ext_data,
+ const char *ext, const char *ext_end,
+ struct riscv_isainfo *isainfo)
+{
+ for (int i = 0; i < ext_data.num_supersets; i++) {
+ const char *superset = ext_data.supersets[i];
+ const int bit = ext_data.id;
+ int ret;
+
+ /*
+ * If the extension that's been found is a superset, there's no
+ * reason to keep looking for other supersets.
+ */
+ ret = riscv_try_set_ext(superset, bit, ext, ext_end, isainfo);
+ if (!ret)
+ return;
+ }
+}
+
static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
unsigned long *isa2hwcap, const char *isa)
{
@@ -311,14 +354,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) {
@@ -328,12 +363,27 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
*this_hwcap |= isa2hwcap[nr];
set_bit(nr, isainfo->isa);
}
- } 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);
+ riscv_try_set_ext_from_supersets(riscv_isa_ext[i],
+ ext, ext_end, isainfo);
+ } else {
+ for (int i = 0; i < riscv_isa_ext_count; i++) {
+ const char *name = riscv_isa_ext[i].name;
+ const int bit = riscv_isa_ext[i].id;
+ int ret;
+
+ ret = riscv_try_set_ext(name, bit, ext, ext_end, isainfo);
+
+ /*
+ * There's no point checking if the extension that the parser has
+ * just found is a superset, if it is the extension itself...
+ */
+ if (!ret)
+ riscv_try_set_ext_from_supersets(riscv_isa_ext[i],
+ ext, ext_end, isainfo);
+ }
}
-#undef SET_ISA_EXT_MAP
}
}
@@ -416,6 +466,28 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
acpi_put_table((struct acpi_table_header *)rhct);
}
+static inline bool riscv_ext_superset_present(struct riscv_isa_ext_data ext_data,
+ struct device_node *cpu_node)
+{
+ if (!ext_data.num_supersets)
+ return false;
+
+ for (int i = 0; i < ext_data.num_supersets; i++) {
+ const char *superset = ext_data.supersets[i];
+ int ret;
+
+ /*
+ * Once a single superset is found, there's no point looking
+ * for any other ones.
+ */
+ ret = of_property_match_string(cpu_node,"riscv,isa-extensions", superset);
+ if (ret >= 0)
+ return true;
+ }
+
+ return false;
+}
+
static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
{
unsigned int cpu;
@@ -435,8 +507,15 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
continue;
for (int i = 0; i < riscv_isa_ext_count; i++) {
- if (of_property_match_string(cpu_node, "riscv,isa-extensions",
- riscv_isa_ext[i].name) < 0)
+ int ret = of_property_match_string(cpu_node, "riscv,isa-extensions",
+ riscv_isa_ext[i].name);
+
+ /*
+ * If the extension itself is not found it could be a
+ * subset of another extension, so the supersets need to
+ * be checked for too.
+ */
+ if (ret < 0 && !riscv_ext_superset_present(riscv_isa_ext[i], cpu_node))
continue;
if (!riscv_isa_extension_check(riscv_isa_ext[i].id))
--
2.39.2
On Wed, Jun 28, 2023 at 4:10 AM Conor Dooley <[email protected]> wrote:
>
> On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
>
> > > > It would be nice to consolidate the ones together that search for a
> > > > single string and set multiple bits, though I don't have any super
> > > > elegant ideas for how off the top of my head.
> > >
> > > I've got a refactor of this code in progress, dropping all of these
> > > copy-paste in place of a loop. It certainly looks more elegant than
> > > this, but it will fall over a bit for these "one string matches many
> > > extensions" cases. See here:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > that contains "parent" extensions to check for. Should be fairly doable,
> > > I'll whip something up on top of that...
> >
> > Nice, and thanks for the review.
>
> > Should I wait for your refactor to be merged before pushing this one?
>
> I don't know. I think that you should continue on with your series here,
> and whichever goes in second gets rebased on top of the other.
> I don't think it makes material difference to review of this patchset as
> to whether you rebase on top of what I'm working on, so I wouldn't
> bother until it gets merged.
>
> Rather hacky, had less time than expected this morning:
> https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
> Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
> repurposed Zicsr for the sake of testing something in the time I had.
>
> Evan, at a high level, does that look more elegant to you, or have I made
> things worse?
>
I see what you're going for at least. It's unfortunate that when
someone bumps up RISCV_ISA_MAX_SUPERSETS it squares the whole array.
Another way to go might be to define the elements in a separate array,
like:
unsigned int riscv_zks_exts[] = {
RISCV_ISA_EXT_ZBKB,
RISCV_ISA_EXT_ZBKC,
....
};
then the macro entry looks like:
SET_ISA_EXT_MAP_MULTI("zks", riscv_zks_exts),
where the SET_ISA_EXT_MAP_MULTI() could use ARRAY_SIZE() to stash both
the pointer to the array and the number of elements.
-Evan
On Wed, Jun 28, 2023 at 10:18:34AM -0700, Evan Green wrote:
> On Wed, Jun 28, 2023 at 4:10 AM Conor Dooley <[email protected]> wrote:
> >
> > On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> >
> > > > > It would be nice to consolidate the ones together that search for a
> > > > > single string and set multiple bits, though I don't have any super
> > > > > elegant ideas for how off the top of my head.
> > > >
> > > > I've got a refactor of this code in progress, dropping all of these
> > > > copy-paste in place of a loop. It certainly looks more elegant than
> > > > this, but it will fall over a bit for these "one string matches many
> > > > extensions" cases. See here:
> > > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > > that contains "parent" extensions to check for. Should be fairly doable,
> > > > I'll whip something up on top of that...
> > >
> > > Nice, and thanks for the review.
> >
> > > Should I wait for your refactor to be merged before pushing this one?
> >
> > I don't know. I think that you should continue on with your series here,
> > and whichever goes in second gets rebased on top of the other.
> > I don't think it makes material difference to review of this patchset as
> > to whether you rebase on top of what I'm working on, so I wouldn't
> > bother until it gets merged.
> >
> > Rather hacky, had less time than expected this morning:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
> > Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
> > repurposed Zicsr for the sake of testing something in the time I had.
> >
> > Evan, at a high level, does that look more elegant to you, or have I made
> > things worse?
> >
>
> I see what you're going for at least. It's unfortunate that when
> someone bumps up RISCV_ISA_MAX_SUPERSETS it squares the whole array.
> Another way to go might be to define the elements in a separate array,
> like:
>
> unsigned int riscv_zks_exts[] = {
> RISCV_ISA_EXT_ZBKB,
> RISCV_ISA_EXT_ZBKC,
> ....
> };
>
> then the macro entry looks like:
>
> SET_ISA_EXT_MAP_MULTI("zks", riscv_zks_exts),
>
> where the SET_ISA_EXT_MAP_MULTI() could use ARRAY_SIZE() to stash both
> the pointer to the array and the number of elements.
Yup, I like the sound of that. I like the variadic stuff as it'd not
require defining a bunch of sub-arrays of supersets. I guess if it grows
too badly, we can just dump it off into another file or w/e.
On Wed, Jun 28, 2023 at 06:24:40PM +0100, Conor Dooley wrote:
> On Wed, Jun 28, 2023 at 10:18:34AM -0700, Evan Green wrote:
> > On Wed, Jun 28, 2023 at 4:10 AM Conor Dooley <[email protected]> wrote:
> > >
> > > On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > > > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <[email protected]> wrote:
> > >
> > > > > > It would be nice to consolidate the ones together that search for a
> > > > > > single string and set multiple bits, though I don't have any super
> > > > > > elegant ideas for how off the top of my head.
> > > > >
> > > > > I've got a refactor of this code in progress, dropping all of these
> > > > > copy-paste in place of a loop. It certainly looks more elegant than
> > > > > this, but it will fall over a bit for these "one string matches many
> > > > > extensions" cases. See here:
> > > > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > > > that contains "parent" extensions to check for. Should be fairly doable,
> > > > > I'll whip something up on top of that...
> > > >
> > > > Nice, and thanks for the review.
> > >
> > > > Should I wait for your refactor to be merged before pushing this one?
> > >
> > > I don't know. I think that you should continue on with your series here,
> > > and whichever goes in second gets rebased on top of the other.
> > > I don't think it makes material difference to review of this patchset as
> > > to whether you rebase on top of what I'm working on, so I wouldn't
> > > bother until it gets merged.
> > >
> > > Rather hacky, had less time than expected this morning:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
> > > Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
> > > repurposed Zicsr for the sake of testing something in the time I had.
> > >
> > > Evan, at a high level, does that look more elegant to you, or have I made
> > > things worse?
> > >
> >
> > I see what you're going for at least. It's unfortunate that when
> > someone bumps up RISCV_ISA_MAX_SUPERSETS it squares the whole array.
> > Another way to go might be to define the elements in a separate array,
> > like:
> >
> > unsigned int riscv_zks_exts[] = {
> > RISCV_ISA_EXT_ZBKB,
> > RISCV_ISA_EXT_ZBKC,
> > ....
> > };
> >
> > then the macro entry looks like:
> >
> > SET_ISA_EXT_MAP_MULTI("zks", riscv_zks_exts),
> >
> > where the SET_ISA_EXT_MAP_MULTI() could use ARRAY_SIZE() to stash both
> > the pointer to the array and the number of elements.
>
> Yup, I like the sound of that. I like the variadic stuff as it'd not
> require defining a bunch of sub-arrays of supersets. I guess if it grows
> too badly, we can just dump it off into another file or w/e.
Also, I realised the other day that I had a bug in my series - I was using
"name" to read the property, not "property", which is what required the
extra "supersets" property.
The simplest thing to do actually seems to be to expand the "property"
member to an array of strings named "properties", rather than
introducing a "supersets" or similar.
Perhaps I am forgetting a good reason for why I had it split, but I'll
give it a whirl and see what I think...
Cheers,
Conor.