Received-SPF: pass (google.com: domain of linux-kernel+bounces-147071-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 15:54:32 +0100
From: Conor Dooley <conor@kernel.org>
To: =?iso-8859-1?Q?Cl=E9ment_L=E9ger?= <cleger@rivosinc.com>
Cc: Conor Dooley <conor.dooley@microchip.com>,
	Deepak Gupta <debug@rivosinc.com>, Jonathan Corbet <corbet@lwn.net>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Anup Patel <anup@brainfault.org>, Shuah Khan <shuah@kernel.org>,
	Atish Patra <atishp@atishpatra.org>, linux-doc@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, kvm@vger.kernel.org,
	kvm-riscv@lists.infradead.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 07/10] riscv: add ISA extension parsing for Zcmop
Message-ID: <20240416-gave-apron-3234098ce416@spud>
References: <20240410091106.749233-8-cleger@rivosinc.com>
 <ZhcFeVYUQJmBAKuv@debug.ba.rivosinc.com>
 <20240410-jawless-cavalry-a3eaf9c562a4@spud>
 <20240410-judgingly-appease-5df493852b70@spud>
 <ZhcTiakvfbjb2hon@debug.ba.rivosinc.com>
 <1287e6e9-cb8e-4a78-9195-ce29f1c4bace@rivosinc.com>
 <20240411-superglue-errant-b32e5118695f@wendy>
 <c86f9fa8-e273-4509-83fa-f21d3265d5c9@rivosinc.com>
 <20240411-backwater-opal-00c9aed2231e@wendy>
 <5eda3278-24bc-4c17-a741-523ad5ff79f7@rivosinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="o4oWfyukzOIBXo40"
Content-Disposition: inline
In-Reply-To: <5eda3278-24bc-4c17-a741-523ad5ff79f7@rivosinc.com>


--o4oWfyukzOIBXo40
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024 at 11:10:24AM +0200, Cl=E9ment L=E9ger wrote:
>=20
>=20
> On 11/04/2024 13:53, Conor Dooley wrote:
> > On Thu, Apr 11, 2024 at 11:08:21AM +0200, Cl=E9ment L=E9ger wrote:
> >>>> If we consider to have potentially broken isa string (ie extensions
> >>>> dependencies not correctly handled), then we'll need some way to
> >>>> validate this within the kernel.
> >>>
> >>> No, the DT passed to the kernel should be correct and we by and large=
 we
> >>> should not have to do validation of it. What I meant above was writing
> >>> the binding so that something invalid will not pass dtbs_check.
> >>
> >> Acked, I was mainly answering Deepak question about dependencies wrt to
> >> using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant
> >> since we expect a correct isa string to be passed.
> >=20
> > Ahh, okay.
> >=20
> >> But as you stated, DT
> >> validation clearly make sense. I think a lot of extensions strings wou=
ld
> >> benefit such support (All the Zv* depends on V, etc).
> >=20
> > I think it is actually as simple something like this, which makes it
> > invalid to have "d" without "f":
> >=20
> > | diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml =
b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > | index 468c646247aa..594828700cbe 100644
> > | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > | @@ -484,5 +484,20 @@ properties:
> > |              Registers in the AX45MP datasheet.
> > |              https://www.andestech.com/wp-content/uploads/AX45MP-1C-R=
ev.-5.0.0-Datasheet.pdf
> > | =20
> > | +allOf:
> > | +  - if:
> > | +      properties:
> > | +        riscv,isa-extensions:
> > | +          contains:
> > | +            const: "d"
> > | +          not:
> > | +            contains:
> > | +              const: "f"
> > | +    then:
> > | +      properties:
> > | +        riscv,isa-extensions:
> > | +          false
> > | +
> > | +
> > |  additionalProperties: true
> > |  ...
> >=20
> > If you do have d without f, the checker will say:
> > cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a'=
, 'd', 'c']
> >=20
> > At least that's readable, even though not clear about what to do. I wish
>=20
> That looks really readable indeed but the messages that result from
> errors are not so informative.
>=20
> It tried playing with various constructs and found this one to yield a
> comprehensive message:
>=20
> +allOf:
> +  - if:
> +      properties:
> +        riscv,isa-extensions:
> +          contains:
> +            const: zcf
> +          not:
> +            contains:
> +              const: zca
> +    then:
> +      properties:
> +        riscv,isa-extensions:
> +          items:
> +            anyOf:
> +              - const: zca
>=20
> arch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0:
> riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed:
>         'zca' was expected
>         from schema $id: http://devicetree.org/schemas/riscv/extensions.y=
aml
>=20
> Even though dt-bindings-check passed, not sure if this is totally a
> valid construct though...

I asked Rob about this yesterday, he suggested adding:
riscv,isa-extensions:
  if:
    contains:
      const: zcf
  then:
    contains:
      const: zca
to the existing property, not in an allOf. I think that is by far the
most readable version in terms of what goes into the binding. The output
would look like:
cpu@0: riscv,isa-extensions: ['i', 'm', 'a', 'd', 'c'] does not contain ite=
ms matching the given schema
(for d requiring f cos I am lazy)

Also, his comment about your one that gives the nice message was that it
would wrong as the anyOf was pointless and it says all items must be
"zca". I didn't try it, but I have a feeling your nice output will be
rather less nice if several different deps are unmet - but hey, probably
will still be better than having an undocumented extension!


--o4oWfyukzOIBXo40
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZh6RJwAKCRB4tDGHoIJi
0gKiAPwO95yHRMreVZI1sGkG9R4vNU1EfJdXv3oNOqMEeB8Z7wEA/fI5rZ01fYH6
ZiMSKf0BThc7H4AEo7Lo2lqLwVoBngw=
=iSce
-----END PGP SIGNATURE-----

--o4oWfyukzOIBXo40--