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

[Conor Dooley]

Hey Atish, Stefan,

On Mon, Jun 26, 2023 at 11:35:10PM -0700, Atish Patra wrote:
> On Mon, Jun 26, 2023 at 5:40 PM Stefan O'Rear <sorear@xxxxxxxxxxxx> wrote:
> > On Mon, Jun 26, 2023, at 6:10 AM, Conor Dooley wrote:

> > > 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.

Yes, I think that this all makes sense from a compatibility point of
view. Splitting it up:

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

This makes total sense from a "being nice to" software point of view.

> > but need not also be present if Y was ratified after
> > or at the same time as X.

It may make sense to reduce this to only after, or not permit the
supersets at all where they are ratified alongside their subsets.
Cross that bridge when we come to it perhaps.

> > If X "depends on" Y, then Y must be present in
> > riscv,isa-extensions even if X and Y were ratified at the same tim

For Linux, this is already the case for F & D. I think that's a good
policy to follow.

> > >
> > > 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?

The same way any other dt-binding works, it's no different in that
respect to compatible strings.

> > Is it
> > worth creating an experimental-* namespace for prototype implementations
> > of unreleased extensions?

IMO, people are free to do whatever they like in their own development
trees. I don't really know why we'd introduce stuff in dt-bindings for
things that are only in an experimental state.

> > > +  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.

Sorry, this is a bit unclear. Do you mean that the sentence you have
provided here should be in the Zicsr entry?

> > > +        - 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.

If it is not compatible, then it should not claim to be :)
We currently report the thing as implementing the same F extension as
anything else that claims to support F, but I don't think we should be
adding a new strictly defined property if it does not apply. Kinda
defeats the purpose I think. I'm not sure whether it should get a new
property, or continue (mis)using riscv,isa, in that case.

> > #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;
> > }

As an aside, if you are building software for a k210, you probably know
pretty damn well what your target system is, given the constraints of
the platform!

> > > +        - 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 ?

Iff it is the first time of appearance, yes.

> > 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?

I only added what was already in use, adding new stuff can be done
subsequently.

> > 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.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature