linux-kernel - Re: [PATCH 07/10] riscv: add ISA extension parsing for Zcmop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240416-gave-apron-3234098ce416@spud>
Date: Tue, 16 Apr 2024 15:54:32 +0100
From: Conor Dooley <conor@...nel.org>
To: Clément Léger <cleger@...osinc.com>
Cc: Conor Dooley <conor.dooley@...rochip.com>,
	Deepak Gupta <debug@...osinc.com>, Jonathan Corbet <corbet@....net>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Anup Patel <anup@...infault.org>, Shuah Khan <shuah@...nel.org>,
	Atish Patra <atishp@...shpatra.org>, linux-doc@...r.kernel.org,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, kvm@...r.kernel.org,
	kvm-riscv@...ts.infradead.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 07/10] riscv: add ISA extension parsing for Zcmop

On Mon, Apr 15, 2024 at 11:10:24AM +0200, Clément Léger wrote:
> 
> 
> On 11/04/2024 13:53, Conor Dooley wrote:
> > On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger 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.
> > 
> > Ahh, okay.
> > 
> >> But as you stated, DT
> >> validation clearly make sense. I think a lot of extensions strings would
> >> benefit such support (All the Zv* depends on V, etc).
> > 
> > I think it is actually as simple something like this, which makes it
> > invalid to have "d" without "f":
> > 
> > | 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-Rev.-5.0.0-Datasheet.pdf
> > |  
> > | +allOf:
> > | +  - if:
> > | +      properties:
> > | +        riscv,isa-extensions:
> > | +          contains:
> > | +            const: "d"
> > | +          not:
> > | +            contains:
> > | +              const: "f"
> > | +    then:
> > | +      properties:
> > | +        riscv,isa-extensions:
> > | +          false
> > | +
> > | +
> > |  additionalProperties: true
> > |  ...
> > 
> > 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']
> > 
> > At least that's readable, even though not clear about what to do. I wish
> 
> That looks really readable indeed but the messages that result from
> errors are not so informative.
> 
> It tried playing with various constructs and found this one to yield a
> comprehensive message:
> 
> +allOf:
> +  - if:
> +      properties:
> +        riscv,isa-extensions:
> +          contains:
> +            const: zcf
> +          not:
> +            contains:
> +              const: zca
> +    then:
> +      properties:
> +        riscv,isa-extensions:
> +          items:
> +            anyOf:
> +              - const: zca
> 
> 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.yaml
> 
> 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 items 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!


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)