linux-kernel - RE: [PATCH v3 2/6] iommu/arm-smmu: Add support to program multiple ARM SMMU's identically

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: <BYAPR12MB2759C4AB588260ECB46046E3B3BE0@BYAPR12MB2759.namprd12.prod.outlook.com>
Date:   Wed, 19 Dec 2018 20:36:54 +0000
From:   Krishna Reddy <vdumpa@...dia.com>
To:     Robin Murphy <robin.murphy@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "joro@...tes.org" <joro@...tes.org>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        Thierry Reding <treding@...dia.com>,
        "Yu-Huan Hsu" <YHsu@...dia.com>, Sachin Nikam <Snikam@...dia.com>,
        Pritesh Raithatha <praithatha@...dia.com>,
        Timo Alho <talho@...dia.com>,
        Alexander Van Brunt <avanbrunt@...dia.com>,
        "Thomas Zeng (SW-TEGRA)" <thomasz@...dia.com>,
        "olof@...om.net" <olof@...om.net>,
        Juha Tukkinen <jtukkinen@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        "jon.he@....com" <jon.he@....com>
Subject: RE: [PATCH v3 2/6] iommu/arm-smmu: Add support to program multiple
 ARM SMMU's identically

Hi Robin,
Thanks for the feedback :)

>The whole point of the library idea was to factor out the code in such a way that all the details
>specific to a particular implementation can be kept together. But what this patch does is insert
>Tegra194-specific handling all through the 'common' code, which is the exact opposite of that concept
>and just makes more hard-to-maintain mess.

In an attempt to reuse most of the ARM SMMU implementation, which heavily relies on data from arm_smmu_device,
The library code has been added with some functionality only usable by Tegra194 SMMU driver. 
In V4 patches, I am working on to add a mechanism to override writel/readl functions in library so that
Tegra smmu driver can override read/write functions and handle programming of multiple instances on its own. 

>The amount of copy-paste duplication in patch #4 has the opposite problem - about 95% of that isn't 
>Tegra194-specific at all (I mean, how many fsl_mc instances does it have?), and having multiple copies of
> generic code with the potential to diverge is also not what anyone wants.

I have split the code in a way that library only contains the code that deals with register programming.
And avoided platform driver code and DT parsing code getting into library, which can allow drivers changing
Independently if necessary in future.

> Plus I don't think ending up building 
> multiple separate drivers will even work in general - thanks to the current state of
>bus_set_iommu() etc., you can't use the regular driver for your third SMMU at the same time.

Good point!
From code, platform_dma_configure/of_dma_configure/of_iommu_configure takes care
of setting right iommu_ops for devices based on the iommu DT node they have in iommus=<> entry.

If iommu.c is updated to use dev->bus->dma_configure(),  then it doesn't really need to use dev->bus->iommu_ops.
dev->bus->dma_configure() can be used to set dev->dma_ops to the right one, if dev->dma_ops is not
already set. 
If this approach looks good, I can make a patch to clean up bus->iommu_ops usage related code to allow
devices to use specific SMMU instance as they need.

>I think what really needs to be done is to conceptually split the driver into "architecture" and "implementation"
> layers - at some point after the holidays we're probably going to sit down and go through all the various quirks and
> specifics we know about to try and figure out what that should actually look like.

If you can provide some high level details on what to keep in library vs implementation after holidays, I would be
 happy to rework the patches.  Will look forward for further discussions on this.

-KR