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

* [RFC 0/6] Deprecate riscv,isa DT property?
@ 2023-05-08 18:16 ` Conor Dooley
  0 siblings, 0 replies; 56+ messages in thread
From: Conor Dooley @ 2023-05-08 18:16 UTC (permalink / raw)
  To: linux-riscv
  Cc: conor, Conor Dooley, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Palmer Dabbelt, Paul Walmsley, Heiko Stuebner,
	Andrew Jones, Sunil V L, Yangyu Chen, devicetree

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

-- 
2.39.2

^ permalink raw reply	[flat|nested] 56+ messages in thread