Re: [PATCH v3] dt-bindings: riscv: deprecate riscv,isa

[Atish Patra]

On Mon, Jun 26, 2023 at 5:40 PM Stefan O'Rear <sorear@fastmail.com> wrote:
>
> On Mon, Jun 26, 2023, at 6:10 AM, Conor Dooley wrote:
> > intro
> > =====
> >
> > When the RISC-V dt-bindings were accepted upstream in Linux, the base
> > ISA etc had yet to be ratified. By the ratification of the base ISA,
> > incompatible changes had snuck into the specifications - for example the
> > Zicsr and Zifencei extensions were spun out of the base ISA.
> >
> > Fast forward to today, and the reason for this patch.
> > Currently the riscv,isa dt property permits only a specific subset of
> > the ISA string - in particular it excludes version numbering.
> > With the current constraints, it is not possible to discern whether
> > "rv64i" means that the hart supports the fence.i instruction, for
> > example.
> > Future systems may choose to implement their own instruction fencing,
> > perhaps using a vendor extension, or they may not implement the optional
> > counter extensions. Software needs a way to determine this.
> >
> > versioning schemes
> > ==================
> >
> > "Use the extension versions that are described in the ISA manual" you
> > may say, and it's not like this has not been considered.
> > Firstly, software that parses the riscv,isa property at runtime will
> > need to contain a lookup table of some sort that maps arbitrary versions
> > to versions it understands. There is not a consistent application of
> > version number applied to extensions, with a higgledy-piggledy
> > collection of tags, "bare" and versioned documents awaiting the reader
> > on the "recently ratified extensions" page:
> > https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
> >
> >       As an aside, and this is reflected in the patch too, since many
> >       extensions have yet to appear in a release of the ISA specs,
> >       they are defined by commits in their respective "working draft"
> >       repositories.
> >
> > Secondly, there is an issue of backwards compatibility, whereby allowing
> > numbers in the ISA string, some parsers may be broken. This would
> > require an additional property to be created to even use the versions in
> > this manner.
> >
> > ~boolean properties~ string array property
> > ==========================================
> >
> > If a new property is needed, the whole approach may as well be looked at
> > from the bottom up. A string with limited character choices etc is
> > hardly the best approach for communicating extension information to
> > software.
> >
> > Switching to using properties that are defined on a per extension basis,
> > allows us to define explicit meanings for the DT representation of each
> > extension - rather than the current situation where different operating
> > systems or other bits of software may impart different meanings to
> > characters in the string.
> > Clearly the best source of meanings is the specifications themselves,
> > this just provides us the ability to choose at what point in time the
> > meaning is set. If an extension changes incompatibility in the future,
> > a new property will be required.
> >
> > Off-list, some of the RVI folks have committed to shoring up the wording
> > in either the ISA specifications, the riscv-isa-manual or
> > so that in the future, modifications to and additions or removals of
> > features will require a new extension. Codifying that assertion
> > somewhere would make it quite unlikely that compatibility would be
> > broken, but we have the tools required to deal with it, if & when it
> > crops up.
> > It is in our collective interest, as consumers of extension meanings, to
> > define a scheme that enforces compatibility.
> >
> > The use of individual properties, rather than elements in a single
>
> no longer individual properties
>
> > string, will also permit validation that the properties have a meaning,
> > as well as potentially reject mutually exclusive combinations, or
> > enforce dependencies between extensions. That would not have be possible
>
> Under what circumstances is a device tree which declares support for a
> superset extension (e.g. m) required to also declare support for its subsets
> (e.g. zmmul)?  There are compatibility issues in both directions.
>
> Proposal: If an extension X is a superset of an extension Y and X is present
> in riscv,isa-extensions, Y must also be present if Y was ratified or added
> to the schema before X, but need not also be present if Y was ratified after
> or at the same time as X.  If X "depends on" Y, then Y must be present in
> riscv,isa-extensions even if X and Y were ratified at the same time.
>
> > with the current dt-schema infrastructure for arbitrary strings, as we
> > would need to add a riscv,isa parser to dt-validate!
> > That's not implemented in this patch, but rather left as future work (for
> > the brave, or the foolish).
> >
> > acpi
> > ====
> >
> > The current ACPI ECR is based on having a single ISA string unfortunately,
> > but ideally ACPI will move to another method, perhaps GUIDs, that give
> > explicit meaning to extensions.
> >
> > parser simplicity
> > =================
> >
> > Many systems that parse DT at runtime already implement an function that
> > can check for the presence of a string in an array of string, as it is
> > similar to the process for parsing a list of compatible strings, so a
> > bunch of new, custom, DT parsing should not be needed.
> > Getting rid of "riscv,isa" parsing would be a nice simplification, but
> > unfortunately for backwards compatibility with old dtbs, existing
> > parsers may not be removable - which may greatly simplify
> > dt parsing code. In Linux, for example, checking for whether a hart
> > supports an extension becomes as simple as:
> >       of_property_match_string(node, "riscv,isa-extensions", "zicbom")
> >
> > vendor extensions
> > =================
> >
> > Compared to riscv,isa, this proposed scheme promotes vendor extensions,
> > oft touted as the strength of RISC-V, to first-class citizens.
> > At present, extensions are defined as meaning what the RISC-V ISA
> > specifications say they do. There is no realistic way of using that
> > interface to provide cross-platform definitions for what vendor
> > extensions mean. Vendor extensions may also have even less consistency
> > than RVI do in terms of versioning, or no care about backwards
> > compatibility.
> > The new property allows us to assign explicit meanings on a per vendor
> > extension basis, backed up by a description of their meanings.
>
> How are vendor extension names allocated?  Will any proposed name for a
> vendor extension pass through linux-riscv@ before it shows up in the wild,
> or are vendors expected to allocate extension names unilaterally?  Is it
> worth creating an experimental-* namespace for prototype implementations
> of unreleased extensions?
>
> > fin
> > ===
> >
> > Create a new file to store the extension meanings and a new
> > riscv,isa-base property to replace the aspect of riscv,isa that is
> > not represented by the new property - the base ISA implemented by a hart.
> >
> > As a starting point, add properties for extensions currently used in
> > Linux.
> >
> > Finally, mark riscv,isa as deprecated, as removing support for it in
> > existing programs would be an ABI break.
> >
> > CC: Palmer Dabbelt <palmer@dabbelt.com>
> > CC: Paul Walmsley <paul.walmsley@sifive.com>
> > CC: Rob Herring <robh+dt@kernel.org>
> > CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > CC: Alistair Francis <alistair.francis@wdc.com>
> > CC: Andrew Jones <ajones@ventanamicro.com>
> > CC: Anup Patel <apatel@ventanamicro.com>
> > CC: Atish Patra <atishp@atishpatra.org>
> > CC: Jessica Clarke <jrtc27@jrtc27.com>
> > CC: Rick Chen <rick@andestech.com>
> > CC: Leo <ycliang@andestech.com>
> > CC: Oleksii <oleksii.kurochko@gmail.com>
> > CC: linux-riscv@lists.infradead.org
> > CC: qemu-riscv@nongnu.org
> > CC: u-boot@lists.denx.de
> > CC: devicetree@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> > Changes in v3:
> > - Per Rob's suggestion, switch to an array of strings. Cuts down on the
> >   size, compared to booleans. It has a standard mechanism for parsing
> >   (you need to parse arrays of strings for compatibles). It still allows
> >   for having a limited set of explicitly defined properties - so the
> >   advantages over a free-form string still apply.
> > - Pick up Palmer's Ack and Review (although I expect that he will be the
> >   one to apply this).
> > ---
> >  .../devicetree/bindings/riscv/cpus.yaml       |  43 ++-
> >  .../devicetree/bindings/riscv/extensions.yaml | 245 ++++++++++++++++++
> >  2 files changed, 265 insertions(+), 23 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/riscv/extensions.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > index 67bd239ead0b..74bc92591086 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -25,6 +25,7 @@ description: |
> >
> >  allOf:
> >    - $ref: /schemas/cpu.yaml#
> > +  - $ref: extensions.yaml
> >
> >  properties:
> >    compatible:
> > @@ -82,25 +83,6 @@ properties:
> >      description:
> >        The blocksize in bytes for the Zicboz cache operations.
> >
> > -  riscv,isa:
> > -    description:
> > -      Identifies the specific RISC-V instruction set architecture
> > -      supported by the hart.  These are documented in the RISC-V
> > -      User-Level ISA document, available from
> > -      https://riscv.org/specifications/
> > -
> > -      Due to revisions of the ISA specification, some deviations
> > -      have arisen over time.
> > -      Notably, riscv,isa was defined prior to the creation of the
> > -      Zicntr, Zicsr, Zifencei and Zihpm extensions and thus "i"
> > -      implies "zicntr_zicsr_zifencei_zihpm".
> > -
> > -      While the isa strings in ISA specification are case
> > -      insensitive, letters in the riscv,isa string must be all
> > -      lowercase.
> > -    $ref: /schemas/types.yaml#/definitions/string
> > -    pattern:
> > ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
> > -
> >    # RISC-V has multiple properties for cache op block sizes as the
> > sizes
> >    # differ between individual CBO extensions
> >    cache-op-block-size: false
> > @@ -139,8 +121,17 @@ properties:
> >        DMIPS/MHz, relative to highest capacity-dmips-mhz
> >        in the system.
> >
> > +oneOf:
> > +  - required:
> > +      - riscv,isa
> > +  - required:
> > +      - riscv,isa-base
> > +
> > +dependencies:
> > +  riscv,isa-base: [ "riscv,isa-extensions" ]
> > +  riscv,isa-extensions: [ "riscv,isa-base" ]
> > +
> >  required:
> > -  - riscv,isa
> >    - interrupt-controller
> >
> >  unevaluatedProperties: false
> > @@ -160,7 +151,9 @@ examples:
> >                  i-cache-sets = <128>;
> >                  i-cache-size = <16384>;
> >                  reg = <0>;
> > -                riscv,isa = "rv64imac";
> > +                riscv,isa-base = "rv64i";
> > +                riscv,isa-extensions = "i", "m", "a", "c";
> > +
> >                  cpu_intc0: interrupt-controller {
> >                          #interrupt-cells = <1>;
> >                          compatible = "riscv,cpu-intc";
> > @@ -183,8 +176,10 @@ examples:
> >                  i-tlb-size = <32>;
> >                  mmu-type = "riscv,sv39";
> >                  reg = <1>;
> > -                riscv,isa = "rv64imafdc";
> >                  tlb-split;
> > +                riscv,isa-base = "rv64i";
> > +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
> > +
> >                  cpu_intc1: interrupt-controller {
> >                          #interrupt-cells = <1>;
> >                          compatible = "riscv,cpu-intc";
> > @@ -202,8 +197,10 @@ examples:
> >                  device_type = "cpu";
> >                  reg = <0>;
> >                  compatible = "riscv";
> > -                riscv,isa = "rv64imafdc";
> >                  mmu-type = "riscv,sv48";
> > +                riscv,isa-base = "rv64i";
> > +                riscv,isa-extensions = "i", "m", "a", "f", "d", "c";
> > +
> >                  interrupt-controller {
> >                          #interrupt-cells = <1>;
> >                          interrupt-controller;
> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > new file mode 100644
> > index 000000000000..af98307f2c2c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -0,0 +1,245 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/riscv/extensions.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V ISA extensions
> > +
> > +maintainers:
> > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > +  - Palmer Dabbelt <palmer@sifive.com>
> > +  - Conor Dooley <conor@kernel.org>
> > +
> > +description: |
> > +  RISC-V has a large number of extensions, some of which are "standard"
> > +  extensions, meaning they are ratified by RISC-V International, and
> > others
> > +  are "vendor" extensions.
> > +  This document defines properties that indicate whether a hart
> > supports a
> > +  given extension.
> > +
> > +  Once a standard extension has been ratified, no changes in behaviour
> > can be
> > +  made without the creation of a new extension.
> > +  The properties for standard extensions therefore map to their
> > originally
> > +  ratified states, with the exception of the I, Zicntr & Zihpm
> > extensions.
> > +  See the "i" property for more information.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: riscv
> > +
> > +properties:
> > +  riscv,isa:
> > +    description:
> > +      Identifies the specific RISC-V instruction set architecture
> > +      supported by the hart.  These are documented in the RISC-V
> > +      User-Level ISA document, available from
> > +      https://riscv.org/specifications/
> > +
> > +      Due to revisions of the ISA specification, some deviations
> > +      have arisen over time.
> > +      Notably, riscv,isa was defined prior to the creation of the
> > +      Zicntr, Zicsr, Zifencei and Zihpm extensions and thus "i"
> > +      implies "zicntr_zicsr_zifencei_zihpm".
> > +
> > +      While the isa strings in ISA specification are case
> > +      insensitive, letters in the riscv,isa string must be all
> > +      lowercase.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    pattern:
> > ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
> > +    deprecated: true
> > +
> > +  riscv,isa-base:
> > +    description:
> > +      The base ISA implemented by this hart, as described by the
> > 20191213
> > +      version of the unprivileged ISA specification.
> > +    enum:
> > +      - rv32i
> > +      - rv64i
> > +
> > +  riscv,isa-extensions:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +    minItems: 1
> > +    description: Extensions supported by the hart.
> > +    items:
> > +      anyOf:
> > +        # single letter extensions, in canonical order
> > +        - const: i
> > +          description: |
> > +            The base integer instruction set, as ratified in the
> > 20191213
> > +            version of the unprivileged ISA specification, with the
> > exception of
> > +            counter access.
> > +            Counter access was removed after the ratification of the
> > 20191213
> > +            version of the unprivileged specification and shunted into
> > the
> > +            Zicntr and Zihpm extensions.
>
> I think this may belong in the description of zicsr?  rdcycle in 20191213
> is a special case of csrrs, which is in zicsr not the base.
>
> > +
> > +        - const: m
> > +          description:
> > +            The standard M extension for integer multiplication and
> > division, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: a
> > +          description:
> > +            The standard A extension for atomic instructions, as
> > ratified in the
> > +            20191213 version of the unprivileged ISA specification.
> > +
> > +        - const: f
> > +          description:
> > +            The standard F extension for single-precision floating
> > point, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
>
> Do we want to be able to describe the K210 in the new schema?  I believe
> that it implements the 2.0 F and D extensions, which are neither forward
> nor backward compatible with the ratified ones.
>
> #include <stdio.h>
> int main() {
>     long a,b;
>     asm("fsub.s fa0,fa0,fa0\n"
>         "fdiv.s fa0,fa0,fa0\n"
>         "fmv.x.d %0,fa0\n"
>         "fcvt.s.w fa1,x0\n"
>         "fmax.s fa1,fa1,fa0\n"
>         "fmv.x.d %1,fa1\n" : "=r" (a), "=r" (b));
>     printf("box(nan) = %lx\nmax(0,nan) = %lx\n", a, b);
>     return 0;
> }
>
> > +
> > +        - const: d
> > +          description:
> > +            The standard D extension for double-precision
> > floating-point, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: q
> > +          description:
> > +            The standard Q extension for quad-precision
> > floating-point, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: c
> > +          description:
> > +            The standard C extension for compressed instructions, as
> > ratified in
> > +            the 20191213 version of the unprivileged ISA specification.
> > +
> > +        - const: v
> > +          description:
> > +            The standard V extension for vector operations, as ratified
> > +            in-and-around commit 7a6c8ae ("Fix text that describes
> > vfmv.v.f
> > +            encoding") of the riscv-v-spec.
> > +
> > +        - const: h
> > +          description:
> > +            The standard H extension for hypervisors as ratified in
> > the 20191213
> > +            version of the privileged ISA specification.
> > +
> > +        # multi-letter extensions, sorted alphanumerically

The multi-letter extensions name should match(ignoring case) the name
of the frozen/ratified or
vendor specific extension name. Correct ?

> > +        - const: smaia
> > +          description: |
> > +            The standard Smaia supervisor-level extension for the
> > advanced
> > +            interrupt architecture for machine-mode-visible csr and
> > behavioural
> > +            changes to interrupts as frozen at commit ccbddab ("Merge
> > pull
> > +            request #42 from riscv/jhauser-2023-RC4") of riscv-aia.
> > +
> > +        - const: ssaia
> > +          description: |
> > +            The standard Ssaia supervisor-level extension for the
> > advanced
> > +            interrupt architecture for supervisor-mode-visible csr and
> > +            behavioural changes to interrupts as frozen at commit
> > ccbddab
> > +            ("Merge pull request #42 from riscv/jhauser-2023-RC4") of
> > riscv-aia.
> > +
> > +        - const: sscofpmf
> > +          description: |
> > +            The standard Sscofpmf supervisor-level extension for count
> > overflow
> > +            and mode-based filtering as ratified at commit 01d1df0
> > ("Add ability
> > +            to manually trigger workflow. (#2)") of
> > riscv-count-overflow.
> > +
> > +        - const: sstc
> > +          description: |
> > +            The standard Sstc supervisor-level extension for time
> > compare as
> > +            ratified at commit 3f9ed34 ("Add ability to manually
> > trigger
> > +            workflow. (#2)") of riscv-time-compare.
> > +
> > +        - const: svinval
> > +          description:
> > +            The standard Svinval supervisor-level extension for
> > fine-grained
> > +            address-translation cache invalidation as ratified in the
> > 20191213
> > +            version of the privileged ISA specification.
> > +
> > +        - const: svnapot
> > +          description:
> > +            The standard Svnapot supervisor-level extensions for napot
> > +            translation contiguity as ratified in the 20191213 version
> > of the
> > +            privileged ISA specification.
> > +
> > +        - const: svpbmt
> > +          description:
> > +            The standard Svpbmt supervisor-level extensions for
> > page-based
> > +            memory types as ratified in the 20191213 version of the
> > privileged
> > +            ISA specification.
> > +
> > +        - const: zba
> > +          description: |
> > +            The standard Zba bit-manipulation extension for address
> > generation
> > +            acceleration instructions as ratified at commit 6d33919
> > ("Merge pull
> > +            request #158 from hirooih/clmul-fix-loop-end-condition") of
> > +            riscv-bitmanip.
> > +
> > +        - const: zbb
> > +          description: |
> > +            The standard Zbb bit-manipulation extension for basic
> > bit-manipulation
> > +            as ratified at commit 6d33919 ("Merge pull request #158
> > from
> > +            hirooih/clmul-fix-loop-end-condition") of riscv-bitmanip.
> > +
> > +        - const: zbc
> > +          description: |
> > +            The standard Zbc bit-manipulation extension for carry-less
> > +            multiplication as ratified at commit 6d33919 ("Merge pull
> > request
> > +            #158 from hirooih/clmul-fix-loop-end-condition") of
> > riscv-bitmanip.
> > +
> > +        - const: zbs
> > +          description: |
> > +            The standard Zbs bit-manipulation extension for single-bit
> > +            instructions as ratified at commit 6d33919 ("Merge pull
> > request #158
> > +            from hirooih/clmul-fix-loop-end-condition") of
> > riscv-bitmanip.
> > +
> > +        - const: zicbom
> > +          description:
> > +            The standard Zicbom extension for base cache management
> > operations as
> > +            ratified in commit 3dd606f ("Create cmobase-v1.0.pdf") of
> > riscv-CMOs.
> > +
> > +        - const: zicbop
> > +          description:
> > +            The standard Zicbop extension for cache-block prefetch
> > instructions
> > +            as ratified in commit 3dd606f ("Create cmobase-v1.0.pdf")
> > of
> > +            riscv-CMOs.
> > +
> > +        - const: zicboz
> > +          description:
> > +            The standard Zicboz extension for cache-block zeroing as
> > ratified
> > +            in commit 3dd606f ("Create cmobase-v1.0.pdf") of
> > riscv-CMOs.
> > +
> > +        - const: zicntr
> > +          description:
> > +            The standard Zicntr extension for base counters and
> > timers, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: zicsr
> > +          description:
> > +            The standard Zicsr extension for control and status
> > register
> > +            instructions, as ratified in the 20191213 version of the
> > +            unprivileged ISA specification.
> > +
> > +        - const: zifencei
> > +          description:
> > +            The standard Zifencei extension for instruction-fetch
> > fence, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: zihintpause
> > +          description:
> > +            The standard Zihintpause extension for pause hints, as
> > ratified in
> > +            commit d8ab5c7 ("Zihintpause is ratified") of the
> > riscv-isa-manual.
> > +
> > +        - const: zihpm
> > +          description:
> > +            The standard Zihpm extension for hardware performance
> > counters, as
> > +            ratified in the 20191213 version of the unprivileged ISA
> > +            specification.
> > +
> > +        - const: ztso
> > +          description:
> > +            The standard Ztso extension for total store ordering, as
> > ratified
> > +            in commit 2e5236 ("Ztso is now ratified.") of the
> > +            riscv-isa-manual.
>
> There are quite a few extension names defined in ratified specifications
> that aren't in that list yet.  Would there be interest in adding them or
> are we waiting for specific conditions to be met?
>
> In particular several subsystems depend on "ziccif" from the profiles
> spec but we haven't previously had a way to check or document that
> dependency.
>
> > +additionalProperties: true
> > +...
> > --
> > 2.40.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish