linux-kernel - Re: [PATCH v2] dt-bindings: iommu: Convert Arm SMMUv3 to DT schema

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] [day] [month] [year] [list]

Message-ID: <CAL_JsqLd52KfOc62b6Lg0nwy=xYqyJvM5Nqu5QR_2tYVvZbWOA@mail.gmail.com>
Date:   Fri, 1 Nov 2019 15:14:03 -0500
From:   Rob Herring <robh@...nel.org>
To:     Will Deacon <will@...nel.org>
Cc:     devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        Mark Rutland <mark.rutland@....com>,
        Robin Murphy <Robin.Murphy@....com>,
        Linux IOMMU <iommu@...ts.linux-foundation.org>
Subject: Re: [PATCH v2] dt-bindings: iommu: Convert Arm SMMUv3 to DT schema

On Fri, Nov 1, 2019 at 12:08 PM Will Deacon <will@...nel.org> wrote:
>
> Hi Rob,
>
> On Mon, Oct 14, 2019 at 02:12:56PM -0500, Rob Herring wrote:
> > Convert the Arm SMMv3 binding to the DT schema format.
> >
> > Cc: Joerg Roedel <joro@...tes.org>
> > Cc: Mark Rutland <mark.rutland@....com>
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Robin Murphy <Robin.Murphy@....com>
> > Cc: iommu@...ts.linux-foundation.org
> > Signed-off-by: Rob Herring <robh@...nel.org>
> > ---
> > v2:
> > - Refine interrupt definition based on Robin's comments
> >
> >  .../devicetree/bindings/iommu/arm,smmu-v3.txt |  77 --------------
> >  .../bindings/iommu/arm,smmu-v3.yaml           | 100 ++++++++++++++++++
> >  2 files changed, 100 insertions(+), 77 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >  create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
>
> [...]
>
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
> > new file mode 100644
> > index 000000000000..662cbc4592c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
> > @@ -0,0 +1,100 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iommu/arm,smmu-v3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM SMMUv3 Architecture Implementation
> > +
> > +maintainers:
> > +  - Will Deacon <will@...nel.org>
> > +  - Robin Murphy <Robin.Murphy@....com>
> > +
> > +description: |+
> > +  The SMMUv3 architecture is a significant departure from previous
> > +  revisions, replacing the MMIO register interface with in-memory command
> > +  and event queues and adding support for the ATS and PRI components of
> > +  the PCIe specification.
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^iommu@[0-9a-f]*"
> > +  compatible:
> > +    const: arm,smmu-v3
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 4
> > +
> > +  interrupt-names:
> > +    oneOf:
> > +      - const: combined
> > +        description:
> > +          The combined interrupt is optional, and should only be provided if the
> > +          hardware supports just a single, combined interrupt line.
> > +          If provided, then the combined interrupt will be used in preference to
> > +          any others.
> > +      - items:
> > +          - const: eventq     # Event Queue not empt
> > +          - const: priq       # PRI Queue not empty
> > +          - const: cmdq-sync  # CMD_SYNC complete
> > +          - const: gerror     # Global Error activated
> > +      - minItems: 2
> > +        maxItems: 4
> > +        items:
> > +          - const: eventq
> > +          - const: gerror
> > +          - const: priq
> > +          - const: cmdq-sync
>
> I find it a bit odd to say "minItems: 2" here since, for example, if you
> have an SMMU that supports PRI then you really want the PRIQ interrupt
> hooked up. The only one never care about in the current driver is cmdq-sync,
> but that's just a driver quirk.

I don't know. I'm just documenting what exists and doesn't seem like
an outright error. The one case is TI:

arch/arm64/boot/dts/ti/k3-j721e-main.dtsi:
interrupt-names = "eventq", "gerror";

If we want to make priq conditionally required, then I need to know
which compatibles would imply supporting PRI. If that's discoverable,
then we can't really enforce the interrupt being present in the
schema.

> Also, if the thing supports MSIs then it might not have any wired interrupts
> at all. Hmm.

That would be why 'interrupts' is optional. The schema only applies if
a property is present.

> > +  '#iommu-cells':
> > +    const: 1
> > +
> > +  dma-coherent:
> > +    description: |
> > +      Present if page table walks made by the SMMU are cache coherent with the
> > +      CPU.
>
> This looks like you've taken the text from SMMUv2 by accident. For SMMUv3,
> it's not just about page table walks, but *any* DMA operations made by the
> SMMU (e.g. STE lookup). I don't see the need to change the current text tbh.

Indeed. I'll do a follow-up as I've already applied these 2 patches.

Rob