linux-kernel - Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach

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: <20201002012919.GB26971@Asurada-Nvidia>
Date:   Thu, 1 Oct 2020 18:29:20 -0700
From:   Nicolin Chen <nicoleotsuka@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     joro@...tes.org, krzk@...nel.org, digetx@...il.com,
        vdumpa@...dia.com, jonathanh@...dia.com,
        linux-tegra@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and
 .attach_dev

On Thu, Oct 01, 2020 at 12:46:14PM +0200, Thierry Reding wrote:
> > > > -	/*
> > > > -	 * This is a bit of a hack. Ideally we'd want to simply return this
> > > > -	 * value. However the IOMMU registration process will attempt to add
> > > > -	 * all devices to the IOMMU when bus_set_iommu() is called. In order
> > > > -	 * not to rely on global variables to track the IOMMU instance, we
> > > > -	 * set it here so that it can be looked up from the .probe_device()
> > > > -	 * callback via the IOMMU device's .drvdata field.
> > > > -	 */
> > > > -	mc->smmu = smmu;
> > > 
> > > I don't think this is going to work. I distinctly remember putting this
> > > here because we needed access to this before ->probe_device() had been
> > > called for any of the devices.
> > 
> > Do you remember which exact part of code needs to access mc->smmu
> > before ->probe_device() is called?
> > 
> > What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> > return value from ->probe_device(), previously ->add_device(), to
> > carry on when you added this code/driver:
> >     commit 8918465163171322c77a19d5258a95f56d89d2e4
> >     Author: Thierry Reding <treding@...dia.com>
> >     Date:   Wed Apr 16 09:24:44 2014 +0200
> >         memory: Add NVIDIA Tegra memory controller support
> > 
> > ..until the core had a change one year later:
> >     commit 38667f18900afe172a4fe44279b132b4140f920f
> >     Author: Joerg Roedel <jroedel@...e.de>
> >     Date:   Mon Jun 29 10:16:08 2015 +0200
> >         iommu: Ignore -ENODEV errors from add_device call-back
> > 
> > As my commit message of this change states, ->probe_device() will
> > be called in from both bus_set_iommu() and really_probe() of each
> > device through of_iommu_configure() -- the later one initializes
> > an fwspec by polling the iommus property in the IOMMU core, same
> > as what we do here in tegra-smmu. If this works, we can probably
> > drop the hack here and get rid of tegra_smmu_configure().
> 
> Looking at this a bit more, I notice that tegra_smmu_configure() does a
> lot of what's already done during of_iommu_configure(), so it'd indeed
> be nice if we could somehow get rid of that. However, like I said, I do
> recall that for DMA/IOMMU we need this prior to ->probe_device(), so it
> isn't clear to me if we can do that.
> 
> So I think in order to make progress we need to check that dropping this
> does indeed still work when we enable DMA/IOMMU (and the preliminary
> patches to pass 1:1 mappings via reserved-memory regions). If so, I
> think it should be safe to remove this.

I am attaching a patch that works with both IOMMU_DOMAIN_UNMANAGED
and IOMMU_DOMAIN_DMA. Would it be possible for you to give a test?

The implementation of getting mc->smmu is using a parent_driver as
I was asking you in the other reply. Yet, it could let us give it a
try.

View attachment "0001-iommu-tegra-smmu-Test.patch" of type "text/x-diff" (7822 bytes)