Message ID | 20230508-hypnotic-phobia-99598439d828@spud (mailing list archive) |
---|---|
Headers | show |
Series | Deprecate riscv,isa DT property? | expand |
On Mon, May 8, 2023 at 11:20 AM Conor Dooley <conor@kernel.org> wrote: > > From: Conor Dooley <conor.dooley@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 ? > 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@kernel.org> > CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > CC: Conor Dooley <conor+dt@kernel.org> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Heiko Stuebner <heiko@sntech.de> > CC: Andrew Jones <ajones@ventanamicro.com> > CC: Sunil V L <sunilvl@ventanamicro.com> > CC: Yangyu Chen <cyy@cyyself.name> > CC: devicetree@vger.kernel.org > CC: linux-riscv@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, May 11, 2023 at 02:27:44PM -0700, Atish Patra wrote: > > 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. That's more than last year at this point, and nothing has changed in the documentation! Talk's cheap, ehh? > https://github.com/riscv/riscv-isa-manual/issues/781#issuecomment-983222655 > > Are you aware of any discussion that changes this ? It's called "trust issues". I am far less worried about the addition of new features though than the removal of existing ones. Part of me fears for fence.i-less systems for example, but there would be other ways to bodge around the mess if it comes to pass. If we are *sure* that no extensions will modify features additively or subtractively, then this may not be needed at all & I can avoid having to bend dt-validate to my will. We have no guarantees for vendor extensions on that front either, they're free to do what they like w.r.t. versioning, no?
On Thu, May 11, 2023 at 2:47 PM Conor Dooley <conor@kernel.org> wrote: > > On Thu, May 11, 2023 at 02:27:44PM -0700, Atish Patra wrote: > > > > 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. > > 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 !! > > https://github.com/riscv/riscv-isa-manual/issues/781#issuecomment-983222655 > > > > Are you aware of any discussion that changes this ? > > It's called "trust issues". I am far less worried about the addition of > new features though than the removal of existing ones. > Part of me fears for fence.i-less systems for example, but there would > be other ways to bodge around the mess if it comes to pass. > If we are *sure* that no extensions will modify features additively or > subtractively, then this may not be needed at all & I can avoid having > to bend dt-validate to my will. Fair enough. Let's get some clarification first from RVI. It must be documented in unpriv spec. Otherwise, there is no point of promise :) > We have no guarantees for vendor extensions on that front either, > they're free to do what they like w.r.t. versioning, no? Vendor extensions are wild west. Who knows what scheme they will use. We will likely have a vendor specific string parsing logic. They can do whatever in that to figure out the version if they require it.
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@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! > 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.
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@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! > >> 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.
On Thu, 11 May 2023 14:27:44 PDT (-0700), atishp@atishpatra.org wrote: > On Mon, May 8, 2023 at 11:20 AM Conor Dooley <conor@kernel.org> wrote: >> >> From: Conor Dooley <conor.dooley@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@kernel.org> >> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> >> CC: Conor Dooley <conor+dt@kernel.org> >> CC: Palmer Dabbelt <palmer@dabbelt.com> >> CC: Paul Walmsley <paul.walmsley@sifive.com> >> CC: Heiko Stuebner <heiko@sntech.de> >> CC: Andrew Jones <ajones@ventanamicro.com> >> CC: Sunil V L <sunilvl@ventanamicro.com> >> CC: Yangyu Chen <cyy@cyyself.name> >> CC: devicetree@vger.kernel.org >> CC: linux-riscv@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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > -- > Regards, > Atish
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@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: > > > 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") 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".
+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@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. 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! (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. Cheers, Conor.
On Fri, May 12, 2023 at 3:05 PM Conor Dooley <conor@kernel.org> 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@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 The only problem with boolean properties is you lose the ability to add extra information about an ISA extension in case we require it. One of the examples is CMO extensions. The current riscv,isa string parsing scheme that doesn't have infrastructure to do that either. We had some related discussions in the past about how to extend the key-value pair to include that value. https://lore.kernel.org/lkml/CAOnJCUKgt1+SVXTBmGChJf74JrsqeqACXbjQAXnhFALkXhPFew@mail.gmail.com/ > 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. > > 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! > > (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. > > Cheers, > Conor.
On Fri, May 12, 2023 at 04:20:43PM -0700, Atish Patra wrote: > On Fri, May 12, 2023 at 3:05 PM Conor Dooley <conor@kernel.org> 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@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 > > The only problem with boolean properties is you lose the ability to > add extra information > about an ISA extension in case we require it. One of the examples is > CMO extensions. > The current riscv,isa string parsing scheme that doesn't have > infrastructure to do that either. I think that is a different problem entirely. Every extension has totally different requirements for what information needs to be passed via DT/ACPI to the kernel (or onwards to userspace). I'm not sure whether creating child nodes for these things makes sense, rather than the current scheme of having "riscv,cbom-block-size" etc. Both are much of a muchness to me, and we have already set out on the path of inserting these properties at the cpu node level. I don't see all that much benefit of deviating from that course ¯\_(ツ)_/¯ However, if you consider it a *presence* test, rather than specifically a boolean property, the probing strategy remains the same for both cases, and it is in the enablement stage that that aspect comes into play. For the sake of this discussion, the probing/detection strategy is what I think is important to consider, IOW do we replace "riscv,isa" at all, and whether it is a child node or boolean property is an implementation detail. By removing the value, I meant removing the extension version, since that will no longer be needed. > We had some related discussions in the past about how to extend the > key-value pair to include > that value. > > https://lore.kernel.org/lkml/CAOnJCUKgt1+SVXTBmGChJf74JrsqeqACXbjQAXnhFALkXhPFew@mail.gmail.com/ Yeah, I had done some looking back at the previous lots of changes for these things while trying to redo the comments on the existing parser. I do dislike that scheme, with the | mmu { | riscv,isa-ext-foo; | }; way of doing things, as the different node types need to be individually probed for. In a scheme where the riscv,isa-ext-foo properties are moved up to the cpu node, possibly as child nodes containing extension specific properties, doing presence detection is uniform across extension types: | for (int k = 0; k < ARRAY_SIZE(riscv_isa_extensions) - 1; k++) { | const char *tmp; | | /* | * I need to double check that of_property_present() works for | * children and "regular" properties. | */ | ret = of_property_present(node, riscv_isa_extensions[k].prop_name); | | if (ret && riscv_isa_extension_check(riscv_isa_extensions[k].key)) { | if (!riscv_isa_extensions[k].multi_letter) | this_hwcap |= isa2hwcap[riscv_isa_extensions[k].key]; | | set_bit(riscv_isa_extensions[k].key, this_isa); | } | } LMK if I missed another proposal in those threads (they mostly seemed focused on /proc/cpuinfo), but the lack of a unified probe/detection mechanism in the scheme I did see suggested put me off. Thanks, Conor. > > 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. > > > > 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! > > > > (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.
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@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. > 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. > Cheers, > Conor.
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@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. 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. > > 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. I'll give it a wee bit for the RVI lads to figure out what they are doing. Thanks, Conor.
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@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.
On Sat, May 13, 2023 at 3:35 AM Conor Dooley <conor@kernel.org> 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@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. ISA string parsed for both DT and ACPI. For ACPI, moving to a per-extension bit in a bitmap and defining a new bit with every ISA extension will be very very inconvenient for updating the ACPI specs. We should continue the ISA string parsing at least for ACPI. For DT, users can either use "riscv,isa" DT property or use boolean DT properties. > > 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! > > (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. > > Cheers, > Conor. Regards, Anup
On 13 May 2023, at 08:47, Anup Patel <apatel@ventanamicro.com> wrote: > > On Sat, May 13, 2023 at 3:35 AM Conor Dooley <conor@kernel.org> 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@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. > > ISA string parsed for both DT and ACPI. > > For ACPI, moving to a per-extension bit in a bitmap and defining > a new bit with every ISA extension will be very very inconvenient > for updating the ACPI specs. We should continue the ISA string > parsing at least for ACPI. > > For DT, users can either use "riscv,isa" DT property or use boolean > DT properties. Can we please not gratuitously have two ways of doing the same thing. I say this as a non-Linux OS that has to deal with whatever Linux decides to do with device trees. It is a total nuisance when you flip flop on things and we have to follow suit. Please consider the breakage very carefully. Jess >> 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! >> >> (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. >> >> Cheers, >> Conor. > > Regards, > Anup > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Sat, May 13, 2023 at 10:34:15PM +0100, Jessica Clarke wrote: > On 13 May 2023, at 08:47, Anup Patel <apatel@ventanamicro.com> wrote: > > For DT, users can either use "riscv,isa" DT property or use boolean > > DT properties. > > Can we please not gratuitously have two ways of doing the same thing. My intention, iff this goes ahead, is to deprecate that property, not have some 'you can use "riscv,isa" or boolean, whichever you choose' situation. Obviously for backwards compatibility reasons parsing it as a fallback would have be kept in Linux, so in theory a DT based Linux system "can use either". It would be up to other platforms to decide whether they would also like to do such a thing. > I say this as a non-Linux OS that has to deal with whatever Linux > decides to do with device trees. It is a total nuisance when you flip > flop on things and we have to follow suit. Please consider the breakage > very carefully. I think I said it in my cover & in a later message, that I sent it here only for first thoughts and my intention is to "send a non-RFC version of this that also CCs the likes of QEMU, U-Boot & the BSD folk". It's clearly not something that could be done unilaterally.
On Sat, May 13, 2023 at 01:17:03PM +0530, Anup Patel wrote: > On Sat, May 13, 2023 at 3:35 AM Conor Dooley <conor@kernel.org> 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@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. > > ISA string parsed for both DT and ACPI. > > For ACPI, moving to a per-extension bit in a bitmap and defining > a new bit with every ISA extension will be very very inconvenient > for updating the ACPI specs. We should continue the ISA string > parsing at least for ACPI. > > For DT, users can either use "riscv,isa" DT property or use boolean > DT properties. > From ACPI perspective, the format better be backed by unpriv (or any other) spec from RVI considering it is a standard across OSs and to avoid any maintenance issues. Thanks, Sunil
On Mon, May 15, 2023 at 10:08:35AM +0530, Sunil V L wrote: > On Sat, May 13, 2023 at 01:17:03PM +0530, Anup Patel wrote: > > ISA string parsed for both DT and ACPI. > > > > For ACPI, moving to a per-extension bit in a bitmap and defining > > a new bit with every ISA extension will be very very inconvenient > > for updating the ACPI specs. We should continue the ISA string > > parsing at least for ACPI. > > > > For DT, users can either use "riscv,isa" DT property or use boolean > > DT properties. > > > From ACPI perspective, the format better be backed by unpriv (or any > other) spec from RVI considering it is a standard across OSs and to > avoid any maintenance issues. DT is also used across multiple OSes, I am not sure what your point is here. The problem, for DT in particular, is defining __which__ version of the unpriv spec meaning is derived from, not whether to use definitions from the specifications.
From: Conor Dooley <conor.dooley@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. 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@kernel.org> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> CC: Conor Dooley <conor+dt@kernel.org> CC: Palmer Dabbelt <palmer@dabbelt.com> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: Heiko Stuebner <heiko@sntech.de> CC: Andrew Jones <ajones@ventanamicro.com> CC: Sunil V L <sunilvl@ventanamicro.com> CC: Yangyu Chen <cyy@cyyself.name> CC: devicetree@vger.kernel.org CC: linux-riscv@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(-)