Message-ID: <20230518-retrial-remindful-ef21e3669c70@wendy>
Date:   Thu, 18 May 2023 12:15:52 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Andrew Jones <ajones@...tanamicro.com>
CC:     <palmer@...belt.com>, <conor@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Alistair Francis <alistair.francis@....com>,
        Anup Patel <apatel@...tanamicro.com>,
        Atish Patra <atishp@...shpatra.org>,
        Jessica Clarke <jrtc27@...c27.com>,
        Rick Chen <rick@...estech.com>, Leo <ycliang@...estech.com>,
        <linux-riscv@...ts.infradead.org>, <qemu-riscv@...gnu.org>,
        <u-boot@...ts.denx.de>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] dt-bindings: riscv: deprecate riscv,isa

Hey Drew,

On Thu, May 18, 2023 at 12:31:51PM +0200, Andrew Jones wrote:
> On Thu, May 18, 2023 at 09:58:30AM +0100, Conor Dooley wrote:

> > -  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
> > -      Zicsr and Zifencei extensions and thus "i" implies
> > -      "zicsr_zifencei".
> > -
> > -      While the isa strings in ISA specification are case
> > -      insensitive, letters in the riscv,isa string must be all
> > -      lowercase to simplify parsing.
> > -    $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 requires 'timebase-frequency' in /cpus, so disallow it here
> >    timebase-frequency: false
> >  
> > @@ -133,8 +117,13 @@ properties:
> >        DMIPS/MHz, relative to highest capacity-dmips-mhz
> >        in the system.
> >  
> > +oneOf:
> > +  - required:
> > +      - riscv,isa
> 
> This is the part Anup keeps reminding me about. We can create better ways
> to handle extensions in DT and ACPI, but we'll still need to parse ISA
> strings to handle legacy DTs and holdouts that keep creating ISA strings,
> at least during the deprecation period,

Yeah, we cannot remove the string parser from the kernel as we will
break existing users.
I don't see keeping it as a problem, we should be nice to people rather
than intentionally trip them up. Let them trip themselves up when their
implicit meaning doesn't match whatever bit of software they are
running's.

> since ISA strings are still "the
> way to do it" according to the spec.

I am not sure what this means, dt-bindings determine the DT ABI, not
what RVI puts in specs.

> Also, if we assume the wording in the spec does get shored up, then,
> unless I'm missing something, the list of advantages for this boolean
> proposal from your commit message would be

Well, shored up _AND_ adhered to. Actions speak far, far louder than
words in that respect unfortunately!

> * More character choices for name -- probably not a huge gain for ratified
>   extensions, since the boolean properties will likely still use the same
>   name as the ISA string (riscv,isa-extension-<name>). But, for vendor
>   extensions, this is indeed a major improvement, since vendor extension
>   boolean property names may need to be extended in unambiguous ways to
>   handle changes in the extension.
> 
> * Simpler, more complete DT validation (but we still need a best effort
>   for legacy ISA strings)

The "best effort" validation via dt-bindings is the current regex. I've
intentionally marked it deprecated, rather than delete it partly for
that reason & partly out of consideration for other users.

> * Simpler DT parsing (but we still need the current parser for legacy ISA
>   strings)

Speaking only about Linux, we can use these meanings here for interpreting
the strings and then apply the fixups that correspond to the difference
between the defined meanings & those at the time of the dt-binding
originally being merged - unconditionally setting zifencei, zicsr, zicntr,
etc. If you don't implement those things, then expect to fall over.
Other operating systems etc may have different implicit meanings and
their own decisions to make!
Oh, and if the "foo" bit of "riscv,isa-extension-foo" doesn't match what
you put in riscv,isa then you keep the pieces. For example, if a vendor
has a vendor extension Xconor where version 1.0.1 is incompatible with
v1.0.0, the properties may be "riscv,isa-extension-xconor" and
"riscv,isa-extension-xconor-no-insnx". In that case, for Linux, I would
assert that there should/would be no way to get the later version of
that extension via riscv,isa.
There are no existing situations like this, so no backwards
compatibility is broken here. If/when it happens, the property is
deprecated and we can direct such cases to the new interface :)
Basically the same as:
https://lore.kernel.org/linux-riscv/20230504-oncoming-antihero-1ed69bb8f57d@spud/

	That reminds me, I need to re-spin that with more extensions set
	unconditionally.

If some new OS comes along, they don't need to implement riscv,isa at
all as it's deprecated.

I'd like to add another one:
* Unified meaning of extensions across bits of software. I actually have
  no idea what versions of things other OSes map the characters to.

> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > new file mode 100644
> > index 000000000000..1b4d726f7174
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -0,0 +1,259 @@
> > +# 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@...ive.com>
> > +  - Palmer Dabbelt <palmer@...ive.com>
> > +  - Conor Dooley <conor@...nel.org>
> > +
> > +description: |
> > +  RISC-V has large number of extensions, some of which "standard" extensions,
>                ^ a                                       ^ are
> 
> > +  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 extensions.
> 
> drop 'a' or depluralize 'extensions'
> 
> > +
> > +  Once a standard extension has been ratified, no features can be added or
> 
> I'd change 'features' to 'changes in behavior', and then...
> 
> > +  removed without the creation of a new extension for that sub- or super-set.
> 
> ...drop 'for that sub- or super-set'
> 
> > +  The properties for standard extensions therefore map to their originally
> > +  ratified states, with the exception of the I, Zicntr & Zihpm extensions.
> 
> Can you elaborate on the exceptions? Or, if the exceptions are described
> below, maybe a '(see below)' here would help ease the reader's
> insecurities about their lack of knowledge about these exceptions, as
> they'll see that the education is coming :-)

Ah crap, I meant to note in the I section that timers were moved to
their own home. Since we are defining this stuff now, I felt that it'd
make sense to define riscv,isa-extension-i as meaning the ratified spec,
sans the counters.

> > +  riscv,isa-extension-f:
> > +    type: boolean
> > +    description:
> > +      The standard M extension for single-precision floating point, as
>                       ^ F

Whoops. All of these are me realising this morning, after checking
everything a few times last night, that there was a dt_binding_check
complaint which prevented me implementing non-patterns as
patternProperties. As a result, I split everything back out into single
properties. It's always the last minute bits...

I could have had a mix of properties and pattern properties, but I
preferred to keep the ordering as something that people could more
easily use to locate properties.

> > +      20191213 version of the unprivileged ISA specification.
> > +
> > +  riscv,isa-extension-v:
> > +    type: boolean
> > +    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.
> > +
> > +  riscv,isa-extension-h:
> > +    type: boolean
> > +    description:
> > +      The standard h extension for hypervisors as ratified in the 20191213
>                       ^ H (might as well keep the case consistent)
> 
> > +      version of the privileged ISA specification.
> > +
> > +  # Additional Standard Extensions, sorted by category then alphabetically
> 
> Can we just do pure alphabetically? And the single-letter extensions above
> don't have a "sorted by" comment above them. I guess they need one, or
> maybe they can also be alphabetical?

I don't really have a preference for ordering. I'm happy to do pure
alphabetical.

> > +  riscv,isa-extension-zicboz:
> > +    type: boolean
> > +    description:
> > +      The standard  Zicbomz extension for cache-block zeroing as ratified in
>                      ^ ^Zicboz
>                      ^ extra space
> 
> (The repetition is making my vision blur, so I'm feeling like I should
> write a script to compare $a and $b, where $a is riscv,isa-extension-$a
> and $b is 'The standard $b' to make sure they match :-) But I probably
> won't...

Again, same excuse! Silly me and all that.

> I'll take your word for it on the versions/dates/commit hashes referenced,
> at least until we get closer to actually merging this.

Aye & I am hoping that by the time that this could actually be
considered for merging that perhaps some of the items will migrate to
the riscv-isa-manual, instead of being flung to the four winds.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists