diff mbox series

[07/10] riscv: add ISA extension parsing for Zcmop

Message ID 20240410091106.749233-8-cleger@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series Add support for a few Zc* extensions as well as Zcmop | expand

Commit Message

Clément Léger April 10, 2024, 9:11 a.m. UTC
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(+)

Comments

Deepak Gupta April 10, 2024, 9:32 p.m. UTC | #1
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
>
>
Conor Dooley April 10, 2024, 10:16 p.m. UTC | #2
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
> > 
> >
Conor Dooley April 10, 2024, 10:27 p.m. UTC | #3
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..
Deepak Gupta April 10, 2024, 10:32 p.m. UTC | #4
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.
Clément Léger April 11, 2024, 7:25 a.m. UTC | #5
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.
> 
>
Conor Dooley April 11, 2024, 9:03 a.m. UTC | #6
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.
Clément Léger April 11, 2024, 9:08 a.m. UTC | #7
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
Conor Dooley April 11, 2024, 11:53 a.m. UTC | #8
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.
Clément Léger April 15, 2024, 9:10 a.m. UTC | #9
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.
Conor Dooley April 16, 2024, 2:54 p.m. UTC | #10
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!
Clément Léger April 16, 2024, 3:23 p.m. UTC | #11
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
Conor Dooley April 17, 2024, 1:11 p.m. UTC | #12
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 mbox series

Patch

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),