| Message ID | 20240410091106.749233-8-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for a few Zc* extensions as well as Zcmop | expand |
On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: >Add parsing for Zcmop ISA extension which was ratified in commit >b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. > >Signed-off-by: Clément Léger <cleger@rivosinc.com> >--- > arch/riscv/include/asm/hwcap.h | 1 + > arch/riscv/kernel/cpufeature.c | 1 + > 2 files changed, 2 insertions(+) > >diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h >index b7551bad341b..cff7660de268 100644 >--- a/arch/riscv/include/asm/hwcap.h >+++ b/arch/riscv/include/asm/hwcap.h >@@ -86,6 +86,7 @@ > #define RISCV_ISA_EXT_ZCB 77 > #define RISCV_ISA_EXT_ZCD 78 > #define RISCV_ISA_EXT_ZCF 79 >+#define RISCV_ISA_EXT_ZCMOP 80 > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 > >diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >index 09dee071274d..f1450cd7231e 100644 >--- a/arch/riscv/kernel/cpufeature.c >+++ b/arch/riscv/kernel/cpufeature.c >@@ -265,6 +265,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), >+ __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), As per spec zcmop is dependent on zca. So perhaps below ? __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, RISCV_ISA_EXT_ZCA) > __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), >-- >2.43.0 > >
On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: > On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: > > Add parsing for Zcmop ISA extension which was ratified in commit > > b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > --- > > arch/riscv/include/asm/hwcap.h | 1 + > > arch/riscv/kernel/cpufeature.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index b7551bad341b..cff7660de268 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -86,6 +86,7 @@ > > #define RISCV_ISA_EXT_ZCB 77 > > #define RISCV_ISA_EXT_ZCD 78 > > #define RISCV_ISA_EXT_ZCF 79 > > +#define RISCV_ISA_EXT_ZCMOP 80 > > > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index 09dee071274d..f1450cd7231e 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), > > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), > > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), > > + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), > > As per spec zcmop is dependent on zca. So perhaps below ? > > __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, RISCV_ISA_EXT_ZCA) What's zicboz got to do with it, copy-pasto I guess? If we're gonna imply stuff like this though I think we need some comments explaining why it's okay. > > __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), > > -- > > 2.43.0 > > > >
On Wed, Apr 10, 2024 at 11:16:11PM +0100, Conor Dooley wrote: > On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: > > On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: > > > Add parsing for Zcmop ISA extension which was ratified in commit > > > b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. > > > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > > --- > > > arch/riscv/include/asm/hwcap.h | 1 + > > > arch/riscv/kernel/cpufeature.c | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > index b7551bad341b..cff7660de268 100644 > > > --- a/arch/riscv/include/asm/hwcap.h > > > +++ b/arch/riscv/include/asm/hwcap.h > > > @@ -86,6 +86,7 @@ > > > #define RISCV_ISA_EXT_ZCB 77 > > > #define RISCV_ISA_EXT_ZCD 78 > > > #define RISCV_ISA_EXT_ZCF 79 > > > +#define RISCV_ISA_EXT_ZCMOP 80 > > > > > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index 09dee071274d..f1450cd7231e 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), > > > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), > > > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), > > > + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), > > > > As per spec zcmop is dependent on zca. So perhaps below ? > > > > __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, RISCV_ISA_EXT_ZCA) > > What's zicboz got to do with it, copy-pasto I guess? > If we're gonna imply stuff like this though I think we need some > comments explaining why it's okay. Also, I'm inclined to call that out specifically in the binding, I've not yet checked if dependencies actually work for elements of a string array like the do for individual properties. I'll todo list that..
On Wed, Apr 10, 2024 at 11:27:16PM +0100, Conor Dooley wrote: >On Wed, Apr 10, 2024 at 11:16:11PM +0100, Conor Dooley wrote: >> On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: >> > On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: >> > > Add parsing for Zcmop ISA extension which was ratified in commit >> > > b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. >> > > >> > > Signed-off-by: Clément Léger <cleger@rivosinc.com> >> > > --- >> > > arch/riscv/include/asm/hwcap.h | 1 + >> > > arch/riscv/kernel/cpufeature.c | 1 + >> > > 2 files changed, 2 insertions(+) >> > > >> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h >> > > index b7551bad341b..cff7660de268 100644 >> > > --- a/arch/riscv/include/asm/hwcap.h >> > > +++ b/arch/riscv/include/asm/hwcap.h >> > > @@ -86,6 +86,7 @@ >> > > #define RISCV_ISA_EXT_ZCB 77 >> > > #define RISCV_ISA_EXT_ZCD 78 >> > > #define RISCV_ISA_EXT_ZCF 79 >> > > +#define RISCV_ISA_EXT_ZCMOP 80 >> > > >> > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 >> > > >> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> > > index 09dee071274d..f1450cd7231e 100644 >> > > --- a/arch/riscv/kernel/cpufeature.c >> > > +++ b/arch/riscv/kernel/cpufeature.c >> > > @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { >> > > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), >> > > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), >> > > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), >> > > + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), >> > >> > As per spec zcmop is dependent on zca. So perhaps below ? >> > >> > __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, RISCV_ISA_EXT_ZCA) >> >> What's zicboz got to do with it, copy-pasto I guess? Yes, copy-pasta :-) >> If we're gonna imply stuff like this though I think we need some >> comments explaining why it's okay. > >Also, I'm inclined to call that out specifically in the binding, I've >not yet checked if dependencies actually work for elements of a string >array like the do for individual properties. I'll todo list that.. Earlier examples of specifying dependency on envcfg actually had functional use case. So you are right, I am not sure if its actually needed in this particular case. And yes definitley, dependency should be mentioned in binding.
On 11/04/2024 00:32, Deepak Gupta wrote: > On Wed, Apr 10, 2024 at 11:27:16PM +0100, Conor Dooley wrote: >> On Wed, Apr 10, 2024 at 11:16:11PM +0100, Conor Dooley wrote: >>> On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: >>> > On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: >>> > > Add parsing for Zcmop ISA extension which was ratified in commit >>> > > b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. >>> > > >>> > > Signed-off-by: Clément Léger <cleger@rivosinc.com> >>> > > --- >>> > > arch/riscv/include/asm/hwcap.h | 1 + >>> > > arch/riscv/kernel/cpufeature.c | 1 + >>> > > 2 files changed, 2 insertions(+) >>> > > >>> > > diff --git a/arch/riscv/include/asm/hwcap.h >>> b/arch/riscv/include/asm/hwcap.h >>> > > index b7551bad341b..cff7660de268 100644 >>> > > --- a/arch/riscv/include/asm/hwcap.h >>> > > +++ b/arch/riscv/include/asm/hwcap.h >>> > > @@ -86,6 +86,7 @@ >>> > > #define RISCV_ISA_EXT_ZCB 77 >>> > > #define RISCV_ISA_EXT_ZCD 78 >>> > > #define RISCV_ISA_EXT_ZCF 79 >>> > > +#define RISCV_ISA_EXT_ZCMOP 80 >>> > > >>> > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 >>> > > >>> > > diff --git a/arch/riscv/kernel/cpufeature.c >>> b/arch/riscv/kernel/cpufeature.c >>> > > index 09dee071274d..f1450cd7231e 100644 >>> > > --- a/arch/riscv/kernel/cpufeature.c >>> > > +++ b/arch/riscv/kernel/cpufeature.c >>> > > @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data >>> riscv_isa_ext[] = { >>> > > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), >>> > > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), >>> > > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), >>> > > + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), >>> > >>> > As per spec zcmop is dependent on zca. So perhaps below ? >>> > >>> > __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, >>> RISCV_ISA_EXT_ZCA) >>> >>> What's zicboz got to do with it, copy-pasto I guess? > > Yes, copy-pasta :-) > >>> If we're gonna imply stuff like this though I think we need some >>> comments explaining why it's okay. >> >> Also, I'm inclined to call that out specifically in the binding, I've >> not yet checked if dependencies actually work for elements of a string >> array like the do for individual properties. I'll todo list that.. > > Earlier examples of specifying dependency on envcfg actually had functional > use case. > So you are right, I am not sure if its actually needed in this > particular case. I actually saw that and think about addressing it but AFAICT, this should be handled by the machine firmware passing the isa string to the kernel (ie, it should be valid). In the case of QEMU, it takes care of setting the extension that are required by this extension itself. If we consider to have potentially broken isa string (ie extensions dependencies not correctly handled), then we'll need some way to validate this within the kernel. Clément > > And yes definitley, dependency should be mentioned in binding. > >
On Thu, Apr 11, 2024 at 09:25:06AM +0200, Clément Léger wrote: > > > On 11/04/2024 00:32, Deepak Gupta wrote: > > On Wed, Apr 10, 2024 at 11:27:16PM +0100, Conor Dooley wrote: > >> On Wed, Apr 10, 2024 at 11:16:11PM +0100, Conor Dooley wrote: > >>> On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: > >>> > On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: > >>> > > Add parsing for Zcmop ISA extension which was ratified in commit > >>> > > b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. > >>> > > > >>> > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > >>> > > --- > >>> > > arch/riscv/include/asm/hwcap.h | 1 + > >>> > > arch/riscv/kernel/cpufeature.c | 1 + > >>> > > 2 files changed, 2 insertions(+) > >>> > > > >>> > > diff --git a/arch/riscv/include/asm/hwcap.h > >>> b/arch/riscv/include/asm/hwcap.h > >>> > > index b7551bad341b..cff7660de268 100644 > >>> > > --- a/arch/riscv/include/asm/hwcap.h > >>> > > +++ b/arch/riscv/include/asm/hwcap.h > >>> > > @@ -86,6 +86,7 @@ > >>> > > #define RISCV_ISA_EXT_ZCB 77 > >>> > > #define RISCV_ISA_EXT_ZCD 78 > >>> > > #define RISCV_ISA_EXT_ZCF 79 > >>> > > +#define RISCV_ISA_EXT_ZCMOP 80 > >>> > > > >>> > > #define RISCV_ISA_EXT_XLINUXENVCFG 127 > >>> > > > >>> > > diff --git a/arch/riscv/kernel/cpufeature.c > >>> b/arch/riscv/kernel/cpufeature.c > >>> > > index 09dee071274d..f1450cd7231e 100644 > >>> > > --- a/arch/riscv/kernel/cpufeature.c > >>> > > +++ b/arch/riscv/kernel/cpufeature.c > >>> > > @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data > >>> riscv_isa_ext[] = { > >>> > > __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), > >>> > > __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), > >>> > > __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), > >>> > > + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), > >>> > > >>> > As per spec zcmop is dependent on zca. So perhaps below ? > >>> > > >>> > __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, > >>> RISCV_ISA_EXT_ZCA) > >>> > >>> What's zicboz got to do with it, copy-pasto I guess? > > > > Yes, copy-pasta :-) > > > >>> If we're gonna imply stuff like this though I think we need some > >>> comments explaining why it's okay. > >> > >> Also, I'm inclined to call that out specifically in the binding, I've > >> not yet checked if dependencies actually work for elements of a string > >> array like the do for individual properties. I'll todo list that.. > > > > Earlier examples of specifying dependency on envcfg actually had functional > > use case. > > So you are right, I am not sure if its actually needed in this > > particular case. > > I actually saw that and think about addressing it but AFAICT, this > should be handled by the machine firmware passing the isa string to the > kernel (ie, it should be valid). In the case of QEMU, it takes care of > setting the extension that are required by this extension itself. > > If we consider to have potentially broken isa string (ie extensions > dependencies not correctly handled), then we'll need some way to > validate this within the kernel. No, the DT passed to the kernel should be correct and we by and large we should not have to do validation of it. What I meant above was writing the binding so that something invalid will not pass dtbs_check.
On 11/04/2024 11:03, Conor Dooley wrote: > On Thu, Apr 11, 2024 at 09:25:06AM +0200, Clément Léger wrote: >> >> >> On 11/04/2024 00:32, Deepak Gupta wrote: >>> On Wed, Apr 10, 2024 at 11:27:16PM +0100, Conor Dooley wrote: >>>> On Wed, Apr 10, 2024 at 11:16:11PM +0100, Conor Dooley wrote: >>>>> On Wed, Apr 10, 2024 at 02:32:41PM -0700, Deepak Gupta wrote: >>>>>> On Wed, Apr 10, 2024 at 11:11:00AM +0200, Clément Léger wrote: >>>>>>> Add parsing for Zcmop ISA extension which was ratified in commit >>>>>>> b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. >>>>>>> >>>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>>>>> --- >>>>>>> arch/riscv/include/asm/hwcap.h | 1 + >>>>>>> arch/riscv/kernel/cpufeature.c | 1 + >>>>>>> 2 files changed, 2 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/riscv/include/asm/hwcap.h >>>>> b/arch/riscv/include/asm/hwcap.h >>>>>>> index b7551bad341b..cff7660de268 100644 >>>>>>> --- a/arch/riscv/include/asm/hwcap.h >>>>>>> +++ b/arch/riscv/include/asm/hwcap.h >>>>>>> @@ -86,6 +86,7 @@ >>>>>>> #define RISCV_ISA_EXT_ZCB 77 >>>>>>> #define RISCV_ISA_EXT_ZCD 78 >>>>>>> #define RISCV_ISA_EXT_ZCF 79 >>>>>>> +#define RISCV_ISA_EXT_ZCMOP 80 >>>>>>> >>>>>>> #define RISCV_ISA_EXT_XLINUXENVCFG 127 >>>>>>> >>>>>>> diff --git a/arch/riscv/kernel/cpufeature.c >>>>> b/arch/riscv/kernel/cpufeature.c >>>>>>> index 09dee071274d..f1450cd7231e 100644 >>>>>>> --- a/arch/riscv/kernel/cpufeature.c >>>>>>> +++ b/arch/riscv/kernel/cpufeature.c >>>>>>> @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data >>>>> riscv_isa_ext[] = { >>>>>>> __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), >>>>>>> __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), >>>>>>> __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), >>>>>>> + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), >>>>>> >>>>>> As per spec zcmop is dependent on zca. So perhaps below ? >>>>>> >>>>>> __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZCMOP, >>>>> RISCV_ISA_EXT_ZCA) >>>>> >>>>> What's zicboz got to do with it, copy-pasto I guess? >>> >>> Yes, copy-pasta :-) >>> >>>>> If we're gonna imply stuff like this though I think we need some >>>>> comments explaining why it's okay. >>>> >>>> Also, I'm inclined to call that out specifically in the binding, I've >>>> not yet checked if dependencies actually work for elements of a string >>>> array like the do for individual properties. I'll todo list that.. >>> >>> Earlier examples of specifying dependency on envcfg actually had functional >>> use case. >>> So you are right, I am not sure if its actually needed in this >>> particular case. >> >> I actually saw that and think about addressing it but AFAICT, this >> should be handled by the machine firmware passing the isa string to the >> kernel (ie, it should be valid). In the case of QEMU, it takes care of >> setting the extension that are required by this extension itself. >> >> If we consider to have potentially broken isa string (ie extensions >> dependencies not correctly handled), then we'll need some way to >> validate this within the kernel. > > No, the DT passed to the kernel should be correct and we by and large we > should not have to do validation of it. What I meant above was writing > the binding so that something invalid will not pass dtbs_check. Acked, I was mainly answering Deepak question about dependencies wrt to using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant since we expect a correct isa string to be passed. But as you stated, DT validation clearly make sense. I think a lot of extensions strings would benefit such support (All the Zv* depends on V, etc). Clément
On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote: > >> If we consider to have potentially broken isa string (ie extensions > >> dependencies not correctly handled), then we'll need some way to > >> validate this within the kernel. > > > > No, the DT passed to the kernel should be correct and we by and large we > > should not have to do validation of it. What I meant above was writing > > the binding so that something invalid will not pass dtbs_check. > > Acked, I was mainly answering Deepak question about dependencies wrt to > using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant > since we expect a correct isa string to be passed. Ahh, okay. > But as you stated, DT > validation clearly make sense. I think a lot of extensions strings would > benefit such support (All the Zv* depends on V, etc). I think it is actually as simple something like this, which makes it invalid to have "d" without "f": | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml | index 468c646247aa..594828700cbe 100644 | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml | @@ -484,5 +484,20 @@ properties: | Registers in the AX45MP datasheet. | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf | | +allOf: | + - if: | + properties: | + riscv,isa-extensions: | + contains: | + const: "d" | + not: | + contains: | + const: "f" | + then: | + properties: | + riscv,isa-extensions: | + false | + | + | additionalProperties: true | ... If you do have d without f, the checker will say: cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c'] At least that's readable, even though not clear about what to do. I wish the former could be said about the wall of text you get for /each/ undocumented entry in the string.
On 11/04/2024 13:53, Conor Dooley wrote: > On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote: >>>> If we consider to have potentially broken isa string (ie extensions >>>> dependencies not correctly handled), then we'll need some way to >>>> validate this within the kernel. >>> >>> No, the DT passed to the kernel should be correct and we by and large we >>> should not have to do validation of it. What I meant above was writing >>> the binding so that something invalid will not pass dtbs_check. >> >> Acked, I was mainly answering Deepak question about dependencies wrt to >> using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant >> since we expect a correct isa string to be passed. > > Ahh, okay. > >> But as you stated, DT >> validation clearly make sense. I think a lot of extensions strings would >> benefit such support (All the Zv* depends on V, etc). > > I think it is actually as simple something like this, which makes it > invalid to have "d" without "f": > > | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml > | index 468c646247aa..594828700cbe 100644 > | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml > | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml > | @@ -484,5 +484,20 @@ properties: > | Registers in the AX45MP datasheet. > | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf > | > | +allOf: > | + - if: > | + properties: > | + riscv,isa-extensions: > | + contains: > | + const: "d" > | + not: > | + contains: > | + const: "f" > | + then: > | + properties: > | + riscv,isa-extensions: > | + false > | + > | + > | additionalProperties: true > | ... > > If you do have d without f, the checker will say: > cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c'] > > At least that's readable, even though not clear about what to do. I wish That looks really readable indeed but the messages that result from errors are not so informative. It tried playing with various constructs and found this one to yield a comprehensive message: +allOf: + - if: + properties: + riscv,isa-extensions: + contains: + const: zcf + not: + contains: + const: zca + then: + properties: + riscv,isa-extensions: + items: + anyOf: + - const: zca arch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0: riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed: 'zca' was expected from schema $id: http://devicetree.org/schemas/riscv/extensions.yaml Even though dt-bindings-check passed, not sure if this is totally a valid construct though... Thanks, Clément > the former could be said about the wall of text you get for /each/ > undocumented entry in the string.
On Mon, Apr 15, 2024 at 11:10:24AM +0200, Clément Léger wrote: > > > On 11/04/2024 13:53, Conor Dooley wrote: > > On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote: > >>>> If we consider to have potentially broken isa string (ie extensions > >>>> dependencies not correctly handled), then we'll need some way to > >>>> validate this within the kernel. > >>> > >>> No, the DT passed to the kernel should be correct and we by and large we > >>> should not have to do validation of it. What I meant above was writing > >>> the binding so that something invalid will not pass dtbs_check. > >> > >> Acked, I was mainly answering Deepak question about dependencies wrt to > >> using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant > >> since we expect a correct isa string to be passed. > > > > Ahh, okay. > > > >> But as you stated, DT > >> validation clearly make sense. I think a lot of extensions strings would > >> benefit such support (All the Zv* depends on V, etc). > > > > I think it is actually as simple something like this, which makes it > > invalid to have "d" without "f": > > > > | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml > > | index 468c646247aa..594828700cbe 100644 > > | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml > > | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml > > | @@ -484,5 +484,20 @@ properties: > > | Registers in the AX45MP datasheet. > > | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf > > | > > | +allOf: > > | + - if: > > | + properties: > > | + riscv,isa-extensions: > > | + contains: > > | + const: "d" > > | + not: > > | + contains: > > | + const: "f" > > | + then: > > | + properties: > > | + riscv,isa-extensions: > > | + false > > | + > > | + > > | additionalProperties: true > > | ... > > > > If you do have d without f, the checker will say: > > cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c'] > > > > At least that's readable, even though not clear about what to do. I wish > > That looks really readable indeed but the messages that result from > errors are not so informative. > > It tried playing with various constructs and found this one to yield a > comprehensive message: > > +allOf: > + - if: > + properties: > + riscv,isa-extensions: > + contains: > + const: zcf > + not: > + contains: > + const: zca > + then: > + properties: > + riscv,isa-extensions: > + items: > + anyOf: > + - const: zca > > arch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0: > riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed: > 'zca' was expected > from schema $id: http://devicetree.org/schemas/riscv/extensions.yaml > > Even though dt-bindings-check passed, not sure if this is totally a > valid construct though... I asked Rob about this yesterday, he suggested adding: riscv,isa-extensions: if: contains: const: zcf then: contains: const: zca to the existing property, not in an allOf. I think that is by far the most readable version in terms of what goes into the binding. The output would look like: cpu@0: riscv,isa-extensions: ['i', 'm', 'a', 'd', 'c'] does not contain items matching the given schema (for d requiring f cos I am lazy) Also, his comment about your one that gives the nice message was that it would wrong as the anyOf was pointless and it says all items must be "zca". I didn't try it, but I have a feeling your nice output will be rather less nice if several different deps are unmet - but hey, probably will still be better than having an undocumented extension!
On 16/04/2024 16:54, Conor Dooley wrote: > On Mon, Apr 15, 2024 at 11:10:24AM +0200, Clément Léger wrote: >> >> >> On 11/04/2024 13:53, Conor Dooley wrote: >>> On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote: >>>>>> If we consider to have potentially broken isa string (ie extensions >>>>>> dependencies not correctly handled), then we'll need some way to >>>>>> validate this within the kernel. >>>>> >>>>> No, the DT passed to the kernel should be correct and we by and large we >>>>> should not have to do validation of it. What I meant above was writing >>>>> the binding so that something invalid will not pass dtbs_check. >>>> >>>> Acked, I was mainly answering Deepak question about dependencies wrt to >>>> using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant >>>> since we expect a correct isa string to be passed. >>> >>> Ahh, okay. >>> >>>> But as you stated, DT >>>> validation clearly make sense. I think a lot of extensions strings would >>>> benefit such support (All the Zv* depends on V, etc). >>> >>> I think it is actually as simple something like this, which makes it >>> invalid to have "d" without "f": >>> >>> | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml >>> | index 468c646247aa..594828700cbe 100644 >>> | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml >>> | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml >>> | @@ -484,5 +484,20 @@ properties: >>> | Registers in the AX45MP datasheet. >>> | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf >>> | >>> | +allOf: >>> | + - if: >>> | + properties: >>> | + riscv,isa-extensions: >>> | + contains: >>> | + const: "d" >>> | + not: >>> | + contains: >>> | + const: "f" >>> | + then: >>> | + properties: >>> | + riscv,isa-extensions: >>> | + false >>> | + >>> | + >>> | additionalProperties: true >>> | ... >>> >>> If you do have d without f, the checker will say: >>> cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c'] >>> >>> At least that's readable, even though not clear about what to do. I wish >> >> That looks really readable indeed but the messages that result from >> errors are not so informative. >> >> It tried playing with various constructs and found this one to yield a >> comprehensive message: >> >> +allOf: >> + - if: >> + properties: >> + riscv,isa-extensions: >> + contains: >> + const: zcf >> + not: >> + contains: >> + const: zca >> + then: >> + properties: >> + riscv,isa-extensions: >> + items: >> + anyOf: >> + - const: zca >> >> arch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0: >> riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed: >> 'zca' was expected >> from schema $id: http://devicetree.org/schemas/riscv/extensions.yaml >> >> Even though dt-bindings-check passed, not sure if this is totally a >> valid construct though... > > I asked Rob about this yesterday, he suggested adding: > riscv,isa-extensions: > if: > contains: > const: zcf > then: > contains: > const: zca That is way more readable and concise ! > to the existing property, not in an allOf. I think that is by far the > most readable version in terms of what goes into the binding. The output > would look like: > cpu@0: riscv,isa-extensions: ['i', 'm', 'a', 'd', 'c'] does not contain items matching the given schema > (for d requiring f cos I am lazy) Than fine by me. The error is at least a bit more understandable than the one with the false schema ;) > > Also, his comment about your one that gives the nice message was that it > would wrong as the anyOf was pointless and it says all items must be > "zca". That's what I understood also. > I didn't try it, but I have a feeling your nice output will be > rather less nice if several different deps are unmet - but hey, probably > will still be better than having an undocumented extension! > If you are ok with that, let's go with Rob suggestion. I'll resubmit a V2 with validation for these extensions and probably a followup for the other ones lacking dependency checking. Thanks, Clément
On Tue, Apr 16, 2024 at 05:23:51PM +0200, Clément Léger wrote: > > > On 16/04/2024 16:54, Conor Dooley wrote: > > On Mon, Apr 15, 2024 at 11:10:24AM +0200, Clément Léger wrote: > >> > >> > >> On 11/04/2024 13:53, Conor Dooley wrote: > >>> On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote: > >>>>>> If we consider to have potentially broken isa string (ie extensions > >>>>>> dependencies not correctly handled), then we'll need some way to > >>>>>> validate this within the kernel. > >>>>> > >>>>> No, the DT passed to the kernel should be correct and we by and large we > >>>>> should not have to do validation of it. What I meant above was writing > >>>>> the binding so that something invalid will not pass dtbs_check. > >>>> > >>>> Acked, I was mainly answering Deepak question about dependencies wrt to > >>>> using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant > >>>> since we expect a correct isa string to be passed. > >>> > >>> Ahh, okay. > >>> > >>>> But as you stated, DT > >>>> validation clearly make sense. I think a lot of extensions strings would > >>>> benefit such support (All the Zv* depends on V, etc). > >>> > >>> I think it is actually as simple something like this, which makes it > >>> invalid to have "d" without "f": > >>> > >>> | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml > >>> | index 468c646247aa..594828700cbe 100644 > >>> | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml > >>> | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml > >>> | @@ -484,5 +484,20 @@ properties: > >>> | Registers in the AX45MP datasheet. > >>> | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf > >>> | > >>> | +allOf: > >>> | + - if: > >>> | + properties: > >>> | + riscv,isa-extensions: > >>> | + contains: > >>> | + const: "d" > >>> | + not: > >>> | + contains: > >>> | + const: "f" > >>> | + then: > >>> | + properties: > >>> | + riscv,isa-extensions: > >>> | + false > >>> | + > >>> | + > >>> | additionalProperties: true > >>> | ... > >>> > >>> If you do have d without f, the checker will say: > >>> cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c'] > >>> > >>> At least that's readable, even though not clear about what to do. I wish > >> > >> That looks really readable indeed but the messages that result from > >> errors are not so informative. > >> > >> It tried playing with various constructs and found this one to yield a > >> comprehensive message: > >> > >> +allOf: > >> + - if: > >> + properties: > >> + riscv,isa-extensions: > >> + contains: > >> + const: zcf > >> + not: > >> + contains: > >> + const: zca > >> + then: > >> + properties: > >> + riscv,isa-extensions: > >> + items: > >> + anyOf: > >> + - const: zca > >> > >> arch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0: > >> riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed: > >> 'zca' was expected > >> from schema $id: http://devicetree.org/schemas/riscv/extensions.yaml > >> > >> Even though dt-bindings-check passed, not sure if this is totally a > >> valid construct though... > > > > I asked Rob about this yesterday, he suggested adding: > > riscv,isa-extensions: > > if: > > contains: > > const: zcf > > then: > > contains: > > const: zca > > That is way more readable and concise ! > > > to the existing property, not in an allOf. I think that is by far the > > most readable version in terms of what goes into the binding. The output > > would look like: > > cpu@0: riscv,isa-extensions: ['i', 'm', 'a', 'd', 'c'] does not contain items matching the given schema > > (for d requiring f cos I am lazy) > > Than fine by me. The error is at least a bit more understandable than > the one with the false schema ;) > > > > > Also, his comment about your one that gives the nice message was that it > > would wrong as the anyOf was pointless and it says all items must be > > "zca". > > That's what I understood also. > > > I didn't try it, but I have a feeling your nice output will be > > rather less nice if several different deps are unmet - but hey, probably > > will still be better than having an undocumented extension! > > > > If you are ok with that, let's go with Rob suggestion. I'll resubmit a > V2 with validation for these extensions and probably a followup for the > other ones lacking dependency checking. Also, Rob made some modifications to dt-schema yesterday, so now the report about an undocumented extension looks like: cpu@1: riscv,isa-extensions:3: '0' is not one of ['i', 'm', 'a', 'f', 'd', 'q', 'c', 'v', 'h', 'smaia', 'smstateen', 'ssaia', 'sscofpmf', 'sstc', 'svinval', 'svnapot', 'svpbmt', 'zacas', 'zba', 'zbb', 'zbc', 'zbkb', 'zbkc', 'zbkx', 'zbs', 'zfa', 'zfh', 'zfhmin', 'zk', 'zkn', 'zknd', 'zkne', 'zknh', 'zkr', 'zks', 'zksed', 'zksh', 'zkt', 'zicbom', 'zicbop', 'zicboz', 'zicntr', 'zicond', 'zicsr', 'zifencei', 'zihintpause', 'zihintntl', 'zihpm', 'ztso', 'zvbb', 'zvbc', 'zvfh', 'zvfhmin', 'zvkb', 'zvkg', 'zvkn', 'zvknc', 'zvkned', 'zvkng', 'zvknha', 'zvknhb', 'zvks', 'zvksc', 'zvksed', 'zvksh', 'zvksg', 'zvkt', 'xandespmu'] instead of cpu@0: riscv,isa-extensions:4: 'anyOf' conditional failed, one must be fixed: 'i' was expected 'm' was expected 'a' was expected 'f' was expected 'd' was expected 'q' was expected 'c' was expected 'v' was expected 'h' was expected 'smaia' was expected 'smstateen' was expected 'ssaia' was expected 'sscofpmf' was expected 'sstc' was expected 'svinval' was expected 'svnapot' was expected 'svpbmt' was expected 'zacas' was expected 'zba' was expected 'zbb' was expected 'zbc' was expected 'zbkb' was expected 'zbkc' was expected 'zbkx' was expected 'zbs' was expected 'zfa' was expected 'zfh' was expected 'zfhmin' was expected 'zk' was expected 'zkn' was expected 'zknd' was expected 'zkne' was expected 'zknh' was expected 'zkr' was expected 'zks' was expected 'zksed' was expected 'zksh' was expected 'zkt' was expected 'zicbom' was expected 'zicbop' was expected 'zicboz' was expected 'zicntr' was expected 'zicond' was expected 'zicsr' was expected 'zifencei' was expected 'zihintpause' was expected 'zihintntl' was expected 'zihpm' was expected 'ztso' was expected 'zvbb' was expected 'zvbc' was expected 'zvfh' was expected 'zvfhmin' was expected 'zvkb' was expected 'zvkg' was expected 'zvkn' was expected 'zvknc' was expected 'zvkned' was expected 'zvkng' was expected 'zvknha' was expected 'zvknhb' was expected 'zvks' was expected 'zvksc' was expected 'zvksed' was expected 'zvksh' was expected 'zvksg' was expected 'zvkt' was expected 'xandespmu' was expected from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml# Which is really great from a readability pov. Not only is it compressed to a single line, it actually points out which extension is the offender. Thanks, Conor.
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index b7551bad341b..cff7660de268 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -86,6 +86,7 @@ #define RISCV_ISA_EXT_ZCB 77 #define RISCV_ISA_EXT_ZCD 78 #define RISCV_ISA_EXT_ZCF 79 +#define RISCV_ISA_EXT_ZCMOP 80 #define RISCV_ISA_EXT_XLINUXENVCFG 127 diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 09dee071274d..f1450cd7231e 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -265,6 +265,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { __RISCV_ISA_EXT_DATA(zcb, RISCV_ISA_EXT_ZCB), __RISCV_ISA_EXT_DATA(zcd, RISCV_ISA_EXT_ZCD), __RISCV_ISA_EXT_DATA(zcf, RISCV_ISA_EXT_ZCF), + __RISCV_ISA_EXT_DATA(zcmop, RISCV_ISA_EXT_ZCMOP), __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),
Add parsing for Zcmop ISA extension which was ratified in commit b854a709c00 ("Zcmop is ratified/1.0") of the riscv-isa-manual. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- arch/riscv/include/asm/hwcap.h | 1 + arch/riscv/kernel/cpufeature.c | 1 + 2 files changed, 2 insertions(+)