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

Message ID	20230508-hypnotic-phobia-99598439d828@spud (mailing list archive)
Headers	show Return-Path: <linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org> From: Conor Dooley <conor@kernel.org> To: linux-riscv@lists.infradead.org Subject: [RFC 0/6] Deprecate riscv,isa DT property? Date: Mon, 8 May 2023 19:16:20 +0100 Message-Id: <20230508-hypnotic-phobia-99598439d828@spud> MIME-Version: 1.0 Precedence: list Cc: devicetree@vger.kernel.org, Conor Dooley <conor+dt@kernel.org>, Heiko Stuebner <heiko@sntech.de>, conor@kernel.org, Yangyu Chen <cyy@cyyself.name>, Conor Dooley <conor.dooley@microchip.com>, Rob Herring <robh+dt@kernel.org>, Palmer Dabbelt <palmer@dabbelt.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Paul Walmsley <paul.walmsley@sifive.com>, Andrew Jones <ajones@ventanamicro.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" <linux-riscv-bounces@lists.infradead.org> Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org
Series	Deprecate riscv,isa DT property? \| expand [RFC,0/6] Deprecate riscv,isa DT property? [RFC,1/6] dt-bindings: riscv: clarify what an unversioned extension means [RFC,2/6] dt-bindings: riscv: add riscv,isa-extension-* property and incompatible example [RFC,3/6] RISC-V: deprecate riscv,isa & replace it with per-extension properties [RFC,4/6] RISC-V: add support for riscv,isa-base property [RFC,5/6] RISC-V: drop a needless check in print_isa_ext() [RFC,6/6] riscv: dts: microchip: use new riscv,isa-extension-* properties for mpfs

Conor Dooley May 8, 2023, 6:16 p.m. UTC

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

Atish Patra May 11, 2023, 9:27 p.m. UTC | #1

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

Conor Dooley May 11, 2023, 9:47 p.m. UTC | #2

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?

Atish Patra May 11, 2023, 10:34 p.m. UTC | #3

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.

Conor Dooley May 11, 2023, 10:38 p.m. UTC | #4

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.

Palmer Dabbelt May 12, 2023, 6:01 p.m. UTC | #5

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.

Palmer Dabbelt May 12, 2023, 6:08 p.m. UTC | #6

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

Conor Dooley May 12, 2023, 7:40 p.m. UTC | #7

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

Conor Dooley May 12, 2023, 10:05 p.m. UTC | #8

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

Atish Patra May 12, 2023, 11:20 p.m. UTC | #9

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.

Conor Dooley May 12, 2023, 11:52 p.m. UTC | #10

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.

Palmer Dabbelt May 12, 2023, 11:55 p.m. UTC | #11

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.

Conor Dooley May 13, 2023, 12:09 a.m. UTC | #12

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.

Palmer Dabbelt May 13, 2023, 12:38 a.m. UTC | #13

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.

Anup Patel May 13, 2023, 7:47 a.m. UTC | #14

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

Jessica Clarke May 13, 2023, 9:34 p.m. UTC | #15

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

Conor Dooley May 13, 2023, 9:54 p.m. UTC | #16

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.

Sunil V L May 15, 2023, 4:38 a.m. UTC | #17

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

Conor Dooley May 15, 2023, 7:52 a.m. UTC | #18

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.

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

Message

Comments