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

Fri May 12 11:08:58 PDT 2023

On Thu, 11 May 2023 14:27:44 PDT (-0700), atishp at atishpatra.org wrote:
> On Mon, May 8, 2023 at 11:20 AM Conor Dooley <conor at kernel.org> wrote:
>>
>> From: Conor Dooley <conor.dooley at microchip.com>
>>
>> Yo,
>>
>> So here's some bits that I have been poking at on top of the recent bits
>> of ISA string parser work:
>> https://lore.kernel.org/linux-riscv/20230504-divisive-unsavory-5a2ff0c3c2d1@spud/
>>
>> TL;DR is that I do not trust the riscv,isa property to carry sufficient
>> information to not cause us problems in the future.
>>
>> Note that this is a very very early RFC, and the implementation etc is
>> intended to be *demonstrative* rather than acceptable.
>>
>> Problem
>> =======
>>
>> I've been kinda triggered by the whole "Zicsr and Zifencei are not part
>> of i" thing, where the dt-binding was defined prior to that split and
>> thus `i` means `zicsr_zifencei` without a real way to differentiate
>> between the two. From a Linux kernel point of view, it's "fine" because
>> we require require Zicsr and Zifencei, so a system without them will not
>> get far enough along for this problem to even manifest - but that's just
>> the example that we already have in front of us & we don't know what
>> might be done in the future when it comes to backwards-compatilibty
>> issues.
>>
>> Yes you might say, expand the dt-binding to allow the version numbers,
>> as the Linux kernel's parser already supports strings containing the
>> version number (although it just ignores them). But that may not be the
>> case for any other consumer of the riscv,isa property - and such an
>> expansion of the dt-binding may actually cause them problems. A valid
>> parser for the current dt-binding may very well fall over if it is
>> expanded to allow free-form numbering.
>>
>> Secondly, it is not realistic to maintain a list of every possible
>> version that someone may insert for every extension to do an explicit
>> comparison, nor can we rely on RVI interpreting "backwards compatible"
>> in a way that means software intended for the older version will still
>> run. (Or for that matter, can we rely on vendors *at all*).
>> If we could rely on that, then we could at least read "x2p2" and realise
>> that we can fall back to "x2p0", but I don't think we have that luxury.
>>
>> The other thought I had was that perhaps some software may choose not to
>> implement version x.y.0 of an extension and only support x.z.0, z > y
>> for some reason. We'd want to refuse that extension if the extension is
>> found, but the version is not listed as being something compatible with
>> x.z.0, and while the ISA spec does say that the default assumption is
>> 2p0 for unversioned extensions in its current form, I struggle to
>> extrapolate that to extensions not currently part of the unpriv spec,
>> but rather defined on their own.
>>
>
> That's a fair point. However, any new RVI ISA extension will only have v1.0
> as per my knowledge. Any new feature will have to be part of a
> different extension.
> At least that was the plan discussed last year.
>
> https://github.com/riscv/riscv-isa-manual/issues/781#issuecomment-983222655
>
> Are you aware of any discussion that changes this ?

That comment was made in November 2021.  The recently ratified list 
<https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions> has a 
bunch in November that I'm going to skip, but since then there's been:

* Zmmul: ratified at version 2.0
* Zawrs: ratified at version 1.01
* Ztso: ratified at version 1.0
* RV32E/RV64E: ratified at version 2.0
* Zicntr: ratified without a version
* Zihpm: ratified without a version
* Zc*: ratified at version 1.0 (which is newer than v1.0.1, v1.0.2, 
  v1.0.3, and v1.0.3-1).

So I think it doesn't really matter what some comment in a github issues 
says, there's still no consistent versioning scheme for the extensions.  

>
>> Proposal
>> ========
>>
>> Instead, I propose a per-extension key/value property, for example
>> riscv,isa-extension-v = "v1.0.0"
>> in the style of compatible strings - so the value is not what hardware
>> implements, but rather the minimum-known version for which the
>> programming model is compatible.
>> Until something comes along that is not compatible with v1.0.0 that we
>> want to support in the kernel, no new keys need to be added to the
>> kernel, just changes to the dt-binding.
>>
>> The binding for it is currently set up so that either you need to
>> be compatible version with v1.0.0, or add a special case. Although
>> v1.0.0 in this case is just a placeholder number, it could be v2.0.0 or
>> any other number. It could even be "initial" to something like that, to
>> match against whatever the first released spec version is.
>>
>>         As an aside, the dt-binding doesn't actually work properly for
>>         enforcement etc at present, but I wanted to get some feedback
>>         before going too far down the rabbit hole there.
>>
>> This method gives us the implemented version -> compatible version "for
>> free", as it is done by the creator of the DT, rather than software
>> running on the platform.
>> We can hopefully be strict about what people are inserting version wise,
>> using dt-validate, rather than it being pot-luck as to what gets filled in,
>> if anything.
>> I'm very reluctant to add more complexity to the property, and therefore
>> parsers, when a key-value type interface is more easily used with
>> standard OF functions - of_property_present(), of_property_read_string()
>> etc to use the Linux kernel's examples.
>>
>> Another benefit of this approach is that we, by way of the dt-binding,
>> control the meaning of the versions.
>> If a vendor decides to release something using Xfoo, but provides no
>> version information, we can then assign one ourselves in case Xfoo in
>> their next SoC is not quite the same. Something similar came up this
>> morning, talking with Heiko about the TH1520, and how to explain the
>> meaning of "_xfoo" properties in "riscv,isa". The ISA spec documentation
>> is pointed to by the binding for that, but vendor properties are
>> obviously not described there. At the expensive of bloating the binding
>> file, the proposed scheme would provide a way to stably document vendor
>> properties.
>>
>> I guess I am trying to design in some flexibility rather than two years
>> down the line realise that the isa string is a source of problems, and
>> have to try and retrofit something in.
>>
>> I would like to encourage people to populate their DT with every
>> extension under the sun that they support, even if software doesn't use
>> it right now (look at the starfive folk that didn't add the bitmanip
>> until told to) so that if/when it is used in the future these boards
>> will pick up the support automagically.
>>
>> ACPI
>> ====
>>
>> This whole proposal is written for a pre-ACPI world, and I have yet to
>> give any thought to how such a key-value interface would work there.
>> I'm not really sure how to deal with that, given they have some ECR
>> process yada yada, but thoughts on that side of things would be very
>> much appreciated.
>>
>> Why x.y.z rather than x.y per the ISA specs?
>> ============================================
>>
>> I said the same, Palmer wanted x.y.z. For example, the T-HEAD vector stuff
>> is 0.7.1 & he cited an example (that now eludes me) of a breaking change
>> in an extension between 1.0 and 1.0.1. God knows how vendors will choose
>> to version things, so having the extra level is likely advantageous.
>>
>> Other stuff
>> ===========
>>
>> The code here is very much in an RFC state. I tested it on an Icicle kit
>> as a PoC - and it does work, but I have not even remotely tested it
>> sufficiently.
>>
>> The dt-binding changes need to be worked on as they do not actually
>> enforce anything!
>>
>> I've intentionally only send this to the linux lists, despite this
>> having wider impact, as it is in a very early state & there's no point
>> involving all & sundry if the idea is hated.
>> If it is not universally derided, I will send the binding patches to
>> various other lists also.
>>
>> What do I hate about this?
>> ==========================
>>
>> I fear bloat in the dt-binding and devicetrees as properties are added
>> mostly. Depending on what I have to do to get enforcement with
>> dt-validate, a complicated binding is also a concern.
>>
>> Suggestions etc very much welcome :)
>>
>> Cheers,
>> Conor.
>>
>> CC: Rob Herring <robh+dt at kernel.org>
>> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt at linaro.org>
>> CC: Conor Dooley <conor+dt at kernel.org>
>> CC: Palmer Dabbelt <palmer at dabbelt.com>
>> CC: Paul Walmsley <paul.walmsley at sifive.com>
>> CC: Heiko Stuebner <heiko at sntech.de>
>> CC: Andrew Jones <ajones at ventanamicro.com>
>> CC: Sunil V L <sunilvl at ventanamicro.com>
>> CC: Yangyu Chen <cyy at cyyself.name>
>> CC: devicetree at vger.kernel.org
>> CC: linux-riscv at lists.infradead.org
>>
>> Conor Dooley (6):
>>   dt-bindings: riscv: clarify what an unversioned extension means
>>   dt-bindings: riscv: add riscv,isa-extension-* property and
>>     incompatible example
>>   RISC-V: deprecate riscv,isa & replace it with per-extension properties
>>   RISC-V: add support for riscv,isa-base property
>>   RISC-V: drop a needless check in print_isa_ext()
>>   riscv: dts: microchip: use new riscv,isa-extension-* properties for
>>     mpfs
>>
>>  .../devicetree/bindings/riscv/cpus.yaml       |  64 +++++-
>>  arch/riscv/boot/dts/microchip/mpfs.dtsi       |  42 +++-
>>  arch/riscv/include/asm/hwcap.h                |  29 ++-
>>  arch/riscv/kernel/cpu.c                       | 124 +++---------
>>  arch/riscv/kernel/cpufeature.c                | 188 +++++++++++++++---
>>  5 files changed, 316 insertions(+), 131 deletions(-)
>>
>> --
>> 2.39.2
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> -- 
> Regards,
> Atish