On Mon, May 8, 2023 at 11:20 AM Conor Dooley <conor@xxxxxxxxxx> wrote: > > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > > 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 ? > 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@xxxxxxxxxx> > CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx> > CC: Conor Dooley <conor+dt@xxxxxxxxxx> > CC: Palmer Dabbelt <palmer@xxxxxxxxxxx> > CC: Paul Walmsley <paul.walmsley@xxxxxxxxxx> > CC: Heiko Stuebner <heiko@xxxxxxxxx> > CC: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> > CC: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> > CC: Yangyu Chen <cyy@xxxxxxxxxxxx> > CC: devicetree@xxxxxxxxxxxxxxx > CC: linux-riscv@xxxxxxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-riscv -- Regards, Atish
