[RFC 0/6] Deprecate riscv,isa DT property?

[Palmer Dabbelt]

On Fri, 12 May 2023 17:09:00 PDT (-0700), Conor Dooley wrote:
> On Fri, May 12, 2023 at 04:55:50PM -0700, Palmer Dabbelt wrote:
>> On Fri, 12 May 2023 15:05:24 PDT (-0700), Conor Dooley wrote:
>> > +CC Greg, Mark, Krste, Philipp, Andrew,
>> > 
>> > (this is LKML now, no top posting or html replies)
>> > 
>> > On Fri, May 12, 2023 at 08:40:10PM +0100, Conor Dooley wrote:
>> > > On Fri, May 12, 2023 at 11:01:09AM -0700, Palmer Dabbelt wrote:
>> > > > On Thu, 11 May 2023 15:38:10 PDT (-0700), Conor Dooley wrote:
>> > > > > On Thu, May 11, 2023 at 03:34:24PM -0700, Atish Patra wrote:
>> > > > > > On Thu, May 11, 2023 at 2:47 PM Conor Dooley <conor at kernel.org> wrote:
>> > > > > > > On Thu, May 11, 2023 at 02:27:44PM -0700, Atish Patra wrote:
>> > > > > > > > > That's more than last year at this point, and nothing
>> > > has changed in the
>> > > > > > > documentation! Talk's cheap, ehh?
>> > > > > > >
>> > > > > > > > > Yup. I will poke RVI folks to check if it still is the
>> > > plan or changed !!
>> > > > > > > Sounds good, thanks!
>> > > 
>> > > There has been some movement on that front, shall see where it goes
>> > > :upsidedown_smile:
>> > 
>> > There's been some off-list discussion prompted by Atish with some of the
>> > RVI spec folk, from which the upshot __appears__ to be an understanding
>> > that using version numbering to indicate removal of ISA features is a bad
>> > idea.
>> > I'm hoping that this results in the enshrinement of this in the ISA
>> > specs, so that we have something concrete to point to as the basis for
>> > not needing to handle version numbering. Certainly that'd be great for
>> > ACPI and remove concerns there.
>> > 
>> > > > > > We will likely have a vendor specific string parsing logic.
>> > > > > > > Complicating the parsing logic is the exact sort of crap
>> > > that I want
>> > > > > to avoid.
>> > > > > Ya, I think we're reallly overcomplicating things with the ISA
>> > > strings.
>> > > > Let's just deprecate them and move to something that doesn't need all the
>> > > > bespoke string parsing.
>> > > 
>> > > Versioning aside, although that removes a large part of the motivation,
>> > > the interface becomes quite nice:
>> > > of_property_present(node, "riscv,isa-extension-zicbom")
>> > 
>> > My current feeling is that, rather than introducing a key-value type of
>> > property, adding boolean properties for extensions is the way to go
>> > here. The "riscv,isa" part of the DT ABI pre-dates even the ratification
>> > of the base extensions (and thus the removal of some features...).
>> > Starting again with a new property would allow us to define extensions
>> > as per their ratified state, rather than the intermediate & incompatible
>> > states that we have currently got with "riscv,isa".
>> > Such a model does rely on the strengthening of the wording in the
>> > specification.
>> 
>> IMO the important part is that we encode an exact version (or commit if
>> they're going to stop doing versions) of the spec in the DT.  We've gotten
>> burned enough times by just trying to point at the latest spec and trusting
>> that compatibility will be handled in the specs, in practice that doesn't
>> work.
>> 
>> Given how inconsistent the RISC-V version schemes have been, we really can't
>> assign any semantic meaning to version numbers.  So I don't think it matters
>> all that much if we encode this as
>> 
>>    riscv,$SPEC = ["v1.0", "v1.1"]
>> 
>> or
>> 
>>    riscv,$SPEC = true // with the binding saying v1.0 or $SHA...
>>    riscv,$SPEC-1.1 = true // with the binding saying v1.1 or $SHA...
>> 
>> Since we can't ascribe any meaning to those version numbers there's nothing
>> to parse in them, so it's just going to plumb into some lookup table in the
>> kernel either way.  The important part is just that we document exactly what
>> spec version we're encoding, as that way we can avoid getting burned by
>> these changes again in the future.
>
> What I was envisioning was something like:
>
> property:
>   riscv,isa-extension-i:
>     description:
>       This hart implements I, as per version 20191213 of the unpriv
>       spec.
>
> If you don't implement that, then don't populate it. If you do, and
> things break, you keep both pieces.

That seems reasonable to me, I guess the "true" was entirely redundant 
there.  I don't think that makes much of a difference in the rest of the 
discussion, it's just a bit cleaner in the encoding.

> The current:
>
>   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/
>
> Is, IMO, utterly unhelpful. My recent addition:
>
>       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".
>
> Is accurate, but is a symptom of the problem rather than a solution.

Ya, it's like the GCC docs that say "this is like an ISA string, but 
different".  Good to note, but not that helpful for users.  Though I 
suppose the fact that we can never seem to document how ISA strings work 
is a pretty good sign they're not a good interface...

>> > This had the advantage of being, as I mention above, even easier to
>> > parse in software than the key-value pair business - but also is
>> > trivially implemented in a dt-binding. What I have been trying to do
>> > with the validation of the key-value stuff does not appear to be readily
>> > doable!
>> 
>> IMO that's the most important deciding factor on how these should be
>> encoded.  We're not tracking the ISA string any more, so it doesn't matter
>> how closely those line up.  We do have a chance to actually validate the
>> interface, though, which was a big problem with the ISA strings.
>> 
>> From talking it sounds like the form you're proposing is easier to encode in
>> dt-schema than what I'd proposed.  I didn't look at dt-schema at all before
>> thinking up the interface, so you're probably right ;).
>> 
>> Assuming that's the case it seems like the way to go as for as I'm
>> concerned.
>> 
>> > (Another drawback that has come to mind is that we have no way to
>> > validate whether mutually exclusive extensions have been added with
>> > "riscv,isa")
>> > 
>> > > That also gives us the ability to define what supported vendor
>> > > extensions actually mean in a dt-binding, which to me is a big win in
>> > > terms of the aforementioned "wild west".
>> > 
>> > Vendor extensions etc are oft quoted as one of the strengths of RISC-V,
>> > and my feeling is that "riscv,isa" is not a mechanism where we can
>> > easily handle meanings - especially for vendor stuff where there is
>> > otherwise no centralised location for _xfoo -> feature mappings.
>> 
>> IMO there's not any fundamental difference: it's not like the standard
>> extensions have any meaningful naming/versioning scheme, so it's still all
>> just lookup tables.
>> 
>> We do get the same benefits from schema validation that we'd get for
>> standard extensions, though.  That's probably a way bigger win for vendor
>> extensions, as it'll close a big loophole in our DT validation right now
>> where users can cram arbitrary stuff into "riscv,isa" and then we have to
>> just deal with it.
>
> TL;DR appears to be that I should revise this in a way that functions
> in a way that is compatible with dt-schema & send a non-RFC version of
> this that also CCs the likes of QEMU, U-Boot & the BSD folk.

Sounds good, thanks for picking this up.

> I'll give it a wee bit for the RVI lads to figure out what they are
> doing.
>
> Thanks,
> Conor.