linux-kernel - RE: [PATCH v2 0/5] Add Tegra194 Dual ARM SMMU driver

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]

Date:   Tue, 27 Nov 2018 02:30:05 +0000
From:   Krishna Reddy <vdumpa@...dia.com>
To:     Olof Johansson <olof@...om.net>
CC:     "will.deacon@....com" <will.deacon@....com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "joro@...tes.org" <joro@...tes.org>,
        Sachin Nikam <Snikam@...dia.com>,
        Pritesh Raithatha <praithatha@...dia.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Timo Alho <talho@...dia.com>, Yu-Huan Hsu <YHsu@...dia.com>,
        Nicolin Chen <nicolinc@...dia.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        Thierry Reding <treding@...dia.com>,
        Alexander Van Brunt <avanbrunt@...dia.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Juha Tukkinen <jtukkinen@...dia.com>
Subject: RE: [PATCH v2 0/5] Add Tegra194 Dual ARM SMMU driver

Hi Olof,
Thanks for the comments!

>>Based on what I can see, it seems that you're trying to describe two
pieces of hardware with only one device in the DT. That seems like an
odd choice. 

T194 SOC HW is designed to use Two ARM SMMU's together like one SMMU.
The IOVA accesses from the HW controllers clients is interleaved across these Two
SMMU's by Memory controller HW.
Even though,  there are two ARM SMMU's,  HW design requires SW programming
these two ARM SMMU instances identically.

>>Also, it seems like all three SMMUs share the same interrupt
line? That's somewhat suboptimal IMHO, but harder to change.

This is by HW design.  SMMU interrupts are only expected if there is an illegal IOVA
access, which shouldn't happen in practical. The SMMU interrupts are only used
to log the faults and for debug purpose on T194.

>>Why not instantiate both of them and create a reference between then
such that the TLB and sync ops are done on both of them in the native
driver? I.e. two arm_smmu structs and a pointer between then (i.e. add a
"next shared smmu" pointer in the struct arm_smmu).

DMA framework supports only one SMMU driver instance per device.
SMMU driver setup iommu_ops pointer in the device struct during the boot when
the platform device is added to the bus.
There is no support to allow a platform device register with two different SMMU
devices.
Two different  SMMU instances is 2x page table memory and 2x map/unmap overhead.
With SMMU driver knowing about two smmu instances, page table memory usage, map/unmap
Overhead would be efficient.

>>As long as devices only references one of them, locking only that one should
provide suitable protection as well.

Earlier proposal on creating one Master SMMU node and slave SMMU nodes within isn't liked. 
Will has proposed creating a library and reusing the code. I have made the patches
to share the code.
https://www.spinics.net/lists/linux-tegra/msg35539.html
https://www.spinics.net/lists/linux-tegra/msg35494.html

>>Seems like a simpler approach than adding a new layer to the driver, but maybe
>>I am missing some complexity here?

Programming both SMMU instances identically with existing ARM SMMU driver is not
possible with multiple DT nodes. 
In the following patch, by writel* macro override, both ARM SMMU instances are
programmed identically in  Tegra194 SMMU driver transparently. Only tlb sync and
interrupt handling need to know about specific instance.

https://lkml.org/lkml/2018/11/1/573
+#define writel_relaxed writel_relaxed_all
+#define writeq_relaxed writeq_relaxed_all
+#define writel writel_all

-KR