diff mbox series

[v2,03/12] dt-bindings: riscv: add Zc* extension rules implied by C extension

Message ID 20240418124300.1387978-4-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 18, 2024, 12:42 p.m. UTC
As stated by Zc* spec:

"As C defines the same instructions as Zca, Zcf and Zcd, the rule is that:
 - C always implies Zca
 - C+F implies Zcf (RV32 only)
 - C+D implies Zcd"

Add additionnal validation rules to enforce this in dts.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 .../devicetree/bindings/riscv/cpus.yaml       |  8 +++--
 .../devicetree/bindings/riscv/extensions.yaml | 34 +++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

Comments

Conor Dooley April 19, 2024, 3:49 p.m. UTC | #1
On Thu, Apr 18, 2024 at 02:42:26PM +0200, Clément Léger wrote:
> As stated by Zc* spec:
> 
> "As C defines the same instructions as Zca, Zcf and Zcd, the rule is that:
>  - C always implies Zca
>  - C+F implies Zcf (RV32 only)
>  - C+D implies Zcd"
> 
> Add additionnal validation rules to enforce this in dts.

I'll get it out of the way: NAK, and the dts patch is the perfect
example of why. I don't want us to have to continually update
devicetrees. If these are implied due to being subsets of other
extensions, then software should be able to enable them when that
other extension is present.

My fear is that, and a quick look at the "add probing" commit seemed to
confirm it, new subsets would require updates to the dts, even though
the existing extension is perfectly sufficient to determine presence.

I definitely want to avoid continual updates to the devicetree for churn
reasons whenever subsets are added, but not turning on the likes of Zca
when C is present because "the bindings were updated to enforce this"
is a complete blocker. I do concede that having two parents makes that
more difficult and will likely require some changes to how we probe - do
we need to have a "second round" type thing?
Taking Zcf as an example, maybe something like making both of C and F into
"standard" supersets and adding a case to riscv_isa_extension_check()
that would mandate that Zca and F are enabled before enabling it, and we
would ensure that C implies Zca before it implies Zcf?

Given we'd be relying on ordering, we have to perform the same implication
for both F and C and make sure that the "implies" struct has Zca before Zcf.
I don't really like that suggestion, hopefully there's a nicer way of doing
that, but I don't like the dt stuff here.

Thanks,
Conor.

> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  .../devicetree/bindings/riscv/cpus.yaml       |  8 +++--
>  .../devicetree/bindings/riscv/extensions.yaml | 34 +++++++++++++++++++
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index d87dd50f1a4b..c4e2c65437b1 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -168,7 +168,7 @@ examples:
>                  i-cache-size = <16384>;
>                  reg = <0>;
>                  riscv,isa-base = "rv64i";
> -                riscv,isa-extensions = "i", "m", "a", "c";
> +                riscv,isa-extensions = "i", "m", "a", "c", "zca";
>  
>                  cpu_intc0: interrupt-controller {
>                          #interrupt-cells = <1>;
> @@ -194,7 +194,8 @@ examples:
>                  reg = <1>;
>                  tlb-split;
>                  riscv,isa-base = "rv64i";
> -                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
> +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
> +                                       "zcd";
>  
>                  cpu_intc1: interrupt-controller {
>                          #interrupt-cells = <1>;
> @@ -215,7 +216,8 @@ examples:
>                  compatible = "riscv";
>                  mmu-type = "riscv,sv48";
>                  riscv,isa-base = "rv64i";
> -                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
> +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
> +                                       "zcd";
>  
>                  interrupt-controller {
>                          #interrupt-cells = <1>;
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index db7daf22b863..0172cbaa13ca 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -549,6 +549,23 @@ properties:
>                  const: zca
>              - contains:
>                  const: f
> +      # C extension implies Zca
> +      - if:
> +          contains:
> +            const: c
> +        then:
> +          contains:
> +            const: zca
> +      # C extension implies Zcd if d
> +      - if:
> +          allOf:
> +            - contains:
> +                const: c
> +            - contains:
> +                const: d
> +        then:
> +          contains:
> +            const: zcd
>  
>  allOf:
>    # Zcf extension does not exists on rv64
> @@ -566,6 +583,23 @@ allOf:
>            not:
>              contains:
>                const: zcf
> +  # C extension implies Zcf if f on rv32 only
> +  - if:
> +      properties:
> +        riscv,isa-extensions:
> +          allOf:
> +            - contains:
> +                const: c
> +            - contains:
> +                const: f
> +        riscv,isa-base:
> +          contains:
> +            const: rv32i
> +    then:
> +      properties:
> +        riscv,isa-extensions:
> +          contains:
> +            const: zcf
>  
>  additionalProperties: true
>  ...
> -- 
> 2.43.0
>
Clément Léger April 22, 2024, 8:53 a.m. UTC | #2
On 19/04/2024 17:49, Conor Dooley wrote:
> On Thu, Apr 18, 2024 at 02:42:26PM +0200, Clément Léger wrote:
>> As stated by Zc* spec:
>>
>> "As C defines the same instructions as Zca, Zcf and Zcd, the rule is that:
>>  - C always implies Zca
>>  - C+F implies Zcf (RV32 only)
>>  - C+D implies Zcd"
>>
>> Add additionnal validation rules to enforce this in dts.
> 
> I'll get it out of the way: NAK, and the dts patch is the perfect
> example of why. I don't want us to have to continually update
> devicetrees. If these are implied due to being subsets of other
> extensions, then software should be able to enable them when that
> other extension is present.

Acked.

> 
> My fear is that, and a quick look at the "add probing" commit seemed to
> confirm it, new subsets would require updates to the dts, even though
> the existing extension is perfectly sufficient to determine presence.
> 
> I definitely want to avoid continual updates to the devicetree for churn
> reasons whenever subsets are added, but not turning on the likes of Zca
> when C is present because "the bindings were updated to enforce this"
> is a complete blocker. I do concede that having two parents makes that
> more difficult and will likely require some changes to how we probe - do
> we need to have a "second round" type thing?

Yeah, I understand. At first, I actually did the modifications in the
ISA probing loop with some dependency probing (ie loop while we don't
have a stable extension state). But I thought that it was not actually
our problem but rather the ISA string provider. For instance, Qemu
provides them.


> Taking Zcf as an example, maybe something like making both of C and F into
> "standard" supersets and adding a case to riscv_isa_extension_check()
> that would mandate that Zca and F are enabled before enabling it, and we
> would ensure that C implies Zca before it implies Zcf?

I'm afraid that riscv_isa_extension_check() will become a rat nest so
rather than going that way, I would be in favor of adding a validation
callback for the extensions if needed.

> 
> Given we'd be relying on ordering, we have to perform the same implication
> for both F and C and make sure that the "implies" struct has Zca before Zcf.
> I don't really like that suggestion, hopefully there's a nicer way of doing
> that, but I don't like the dt stuff here.

I guess the "cleanest" way would be to have some "defered-like"
mechanism in ISA probing which would allow to handle ordering as well as
dependencies/implies for extensions. For Zca, Zcf, we actually do not
have ordering problems but I think it would be a bit broken not to
support that as well.

I can actually revive the work mentioned above to handle that and see if
it works ok.

Clément

> 
> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  .../devicetree/bindings/riscv/cpus.yaml       |  8 +++--
>>  .../devicetree/bindings/riscv/extensions.yaml | 34 +++++++++++++++++++
>>  2 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> index d87dd50f1a4b..c4e2c65437b1 100644
>> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> @@ -168,7 +168,7 @@ examples:
>>                  i-cache-size = <16384>;
>>                  reg = <0>;
>>                  riscv,isa-base = "rv64i";
>> -                riscv,isa-extensions = "i", "m", "a", "c";
>> +                riscv,isa-extensions = "i", "m", "a", "c", "zca";
>>  
>>                  cpu_intc0: interrupt-controller {
>>                          #interrupt-cells = <1>;
>> @@ -194,7 +194,8 @@ examples:
>>                  reg = <1>;
>>                  tlb-split;
>>                  riscv,isa-base = "rv64i";
>> -                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
>> +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
>> +                                       "zcd";
>>  
>>                  cpu_intc1: interrupt-controller {
>>                          #interrupt-cells = <1>;
>> @@ -215,7 +216,8 @@ examples:
>>                  compatible = "riscv";
>>                  mmu-type = "riscv,sv48";
>>                  riscv,isa-base = "rv64i";
>> -                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
>> +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
>> +                                       "zcd";
>>  
>>                  interrupt-controller {
>>                          #interrupt-cells = <1>;
>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> index db7daf22b863..0172cbaa13ca 100644
>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> @@ -549,6 +549,23 @@ properties:
>>                  const: zca
>>              - contains:
>>                  const: f
>> +      # C extension implies Zca
>> +      - if:
>> +          contains:
>> +            const: c
>> +        then:
>> +          contains:
>> +            const: zca
>> +      # C extension implies Zcd if d
>> +      - if:
>> +          allOf:
>> +            - contains:
>> +                const: c
>> +            - contains:
>> +                const: d
>> +        then:
>> +          contains:
>> +            const: zcd
>>  
>>  allOf:
>>    # Zcf extension does not exists on rv64
>> @@ -566,6 +583,23 @@ allOf:
>>            not:
>>              contains:
>>                const: zcf
>> +  # C extension implies Zcf if f on rv32 only
>> +  - if:
>> +      properties:
>> +        riscv,isa-extensions:
>> +          allOf:
>> +            - contains:
>> +                const: c
>> +            - contains:
>> +                const: f
>> +        riscv,isa-base:
>> +          contains:
>> +            const: rv32i
>> +    then:
>> +      properties:
>> +        riscv,isa-extensions:
>> +          contains:
>> +            const: zcf
>>  
>>  additionalProperties: true
>>  ...
>> -- 
>> 2.43.0
>>
Conor Dooley April 22, 2024, 11:19 a.m. UTC | #3
On Mon, Apr 22, 2024 at 10:53:04AM +0200, Clément Léger wrote:
> On 19/04/2024 17:49, Conor Dooley wrote:
> > On Thu, Apr 18, 2024 at 02:42:26PM +0200, Clément Léger wrote:
> >> As stated by Zc* spec:
> >>
> >> "As C defines the same instructions as Zca, Zcf and Zcd, the rule is that:
> >>  - C always implies Zca
> >>  - C+F implies Zcf (RV32 only)
> >>  - C+D implies Zcd"
> >>
> >> Add additionnal validation rules to enforce this in dts.
> > 
> > I'll get it out of the way: NAK, and the dts patch is the perfect
> > example of why. I don't want us to have to continually update
> > devicetrees. If these are implied due to being subsets of other
> > extensions, then software should be able to enable them when that
> > other extension is present.
> 
> Acked.
> 
> > 
> > My fear is that, and a quick look at the "add probing" commit seemed to
> > confirm it, new subsets would require updates to the dts, even though
> > the existing extension is perfectly sufficient to determine presence.
> > 
> > I definitely want to avoid continual updates to the devicetree for churn
> > reasons whenever subsets are added, but not turning on the likes of Zca
> > when C is present because "the bindings were updated to enforce this"
> > is a complete blocker. I do concede that having two parents makes that
> > more difficult and will likely require some changes to how we probe - do
> > we need to have a "second round" type thing?
> 
> Yeah, I understand. At first, I actually did the modifications in the
> ISA probing loop with some dependency probing (ie loop while we don't
> have a stable extension state). But I thought that it was not actually
> our problem but rather the ISA string provider. For instance, Qemu
> provides them.


A newer version of QEMU might, but not all do, so I'm not sure that using
it is a good example. My expectations is that a devicetree will be written
to the standards of the day and not be updated as subsets are released.

If this were the first instance of a superset/bundle I'd be prepared to
accept an argument that we should not infer anything - but it's not and
we'd be introducing inconsistency with the crypto stuff. I know that both
scenarios are different in terms of extension history given that this is
splitting things into a subset and that was a superset/bundle created at
the same time, but they're not really that different in terms of the
DT/ACPI to user "interface".

> > Taking Zcf as an example, maybe something like making both of C and F into
> > "standard" supersets and adding a case to riscv_isa_extension_check()
> > that would mandate that Zca and F are enabled before enabling it, and we
> > would ensure that C implies Zca before it implies Zcf?
> 
> I'm afraid that riscv_isa_extension_check() will become a rat nest so
> rather than going that way, I would be in favor of adding a validation
> callback for the extensions if needed.

IOW, extension check split out per extension moving to be a callback?

> > Given we'd be relying on ordering, we have to perform the same implication
> > for both F and C and make sure that the "implies" struct has Zca before Zcf.
> > I don't really like that suggestion, hopefully there's a nicer way of doing
> > that, but I don't like the dt stuff here.
> 
> I guess the "cleanest" way would be to have some "defered-like"
> mechanism in ISA probing which would allow to handle ordering as well as
> dependencies/implies for extensions. For Zca, Zcf, we actually do not
> have ordering problems but I think it would be a bit broken not to
> support that as well.

We could, I suppose, enable all detected extensions on a CPU and run the
aforemention callback, disabling them if conditions are not met?

Is that something like what you're suggesting?
Clément Léger April 22, 2024, 11:40 a.m. UTC | #4
On 22/04/2024 13:19, Conor Dooley wrote:
> On Mon, Apr 22, 2024 at 10:53:04AM +0200, Clément Léger wrote:
>> On 19/04/2024 17:49, Conor Dooley wrote:
>>> On Thu, Apr 18, 2024 at 02:42:26PM +0200, Clément Léger wrote:
>>>> As stated by Zc* spec:
>>>>
>>>> "As C defines the same instructions as Zca, Zcf and Zcd, the rule is that:
>>>>  - C always implies Zca
>>>>  - C+F implies Zcf (RV32 only)
>>>>  - C+D implies Zcd"
>>>>
>>>> Add additionnal validation rules to enforce this in dts.
>>>
>>> I'll get it out of the way: NAK, and the dts patch is the perfect
>>> example of why. I don't want us to have to continually update
>>> devicetrees. If these are implied due to being subsets of other
>>> extensions, then software should be able to enable them when that
>>> other extension is present.
>>
>> Acked.
>>
>>>
>>> My fear is that, and a quick look at the "add probing" commit seemed to
>>> confirm it, new subsets would require updates to the dts, even though
>>> the existing extension is perfectly sufficient to determine presence.
>>>
>>> I definitely want to avoid continual updates to the devicetree for churn
>>> reasons whenever subsets are added, but not turning on the likes of Zca
>>> when C is present because "the bindings were updated to enforce this"
>>> is a complete blocker. I do concede that having two parents makes that
>>> more difficult and will likely require some changes to how we probe - do
>>> we need to have a "second round" type thing?
>>
>> Yeah, I understand. At first, I actually did the modifications in the
>> ISA probing loop with some dependency probing (ie loop while we don't
>> have a stable extension state). But I thought that it was not actually
>> our problem but rather the ISA string provider. For instance, Qemu
>> provides them.
> 
> 
> A newer version of QEMU might, but not all do, so I'm not sure that using
> it is a good example. My expectations is that a devicetree will be written
> to the standards of the day and not be updated as subsets are released.
> 
> If this were the first instance of a superset/bundle I'd be prepared to
> accept an argument that we should not infer anything - but it's not and
> we'd be introducing inconsistency with the crypto stuff. I know that both
> scenarios are different in terms of extension history given that this is
> splitting things into a subset and that was a superset/bundle created at
> the same time, but they're not really that different in terms of the
> DT/ACPI to user "interface".
> 
>>> Taking Zcf as an example, maybe something like making both of C and F into
>>> "standard" supersets and adding a case to riscv_isa_extension_check()
>>> that would mandate that Zca and F are enabled before enabling it, and we
>>> would ensure that C implies Zca before it implies Zcf?
>>
>> I'm afraid that riscv_isa_extension_check() will become a rat nest so
>> rather than going that way, I would be in favor of adding a validation
>> callback for the extensions if needed.
> 
> IOW, extension check split out per extension moving to be a callback?
> 
>>> Given we'd be relying on ordering, we have to perform the same implication
>>> for both F and C and make sure that the "implies" struct has Zca before Zcf.
>>> I don't really like that suggestion, hopefully there's a nicer way of doing
>>> that, but I don't like the dt stuff here.
>>
>> I guess the "cleanest" way would be to have some "defered-like"
>> mechanism in ISA probing which would allow to handle ordering as well as
>> dependencies/implies for extensions. For Zca, Zcf, we actually do not
>> have ordering problems but I think it would be a bit broken not to
>> support that as well.
> 
> We could, I suppose, enable all detected extensions on a CPU and run the
> aforemention callback, disabling them if conditions are not met?
> 
> Is that something like what you're suggesting?

Yep, exactly. First parse the ISA blindly in a bitmap, (either from
riscv,isa string, riscv,isa-extensions, or ACPI). Then in a second time,
verify the ISA extensions by validating extension and looping until we
reach a stable set.

Clément
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index d87dd50f1a4b..c4e2c65437b1 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -168,7 +168,7 @@  examples:
                 i-cache-size = <16384>;
                 reg = <0>;
                 riscv,isa-base = "rv64i";
-                riscv,isa-extensions = "i", "m", "a", "c";
+                riscv,isa-extensions = "i", "m", "a", "c", "zca";
 
                 cpu_intc0: interrupt-controller {
                         #interrupt-cells = <1>;
@@ -194,7 +194,8 @@  examples:
                 reg = <1>;
                 tlb-split;
                 riscv,isa-base = "rv64i";
-                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
+                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
+                                       "zcd";
 
                 cpu_intc1: interrupt-controller {
                         #interrupt-cells = <1>;
@@ -215,7 +216,8 @@  examples:
                 compatible = "riscv";
                 mmu-type = "riscv,sv48";
                 riscv,isa-base = "rv64i";
-                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
+                riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca",
+                                       "zcd";
 
                 interrupt-controller {
                         #interrupt-cells = <1>;
diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index db7daf22b863..0172cbaa13ca 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -549,6 +549,23 @@  properties:
                 const: zca
             - contains:
                 const: f
+      # C extension implies Zca
+      - if:
+          contains:
+            const: c
+        then:
+          contains:
+            const: zca
+      # C extension implies Zcd if d
+      - if:
+          allOf:
+            - contains:
+                const: c
+            - contains:
+                const: d
+        then:
+          contains:
+            const: zcd
 
 allOf:
   # Zcf extension does not exists on rv64
@@ -566,6 +583,23 @@  allOf:
           not:
             contains:
               const: zcf
+  # C extension implies Zcf if f on rv32 only
+  - if:
+      properties:
+        riscv,isa-extensions:
+          allOf:
+            - contains:
+                const: c
+            - contains:
+                const: f
+        riscv,isa-base:
+          contains:
+            const: rv32i
+    then:
+      properties:
+        riscv,isa-extensions:
+          contains:
+            const: zcf
 
 additionalProperties: true
 ...