Date:   Mon, 7 Nov 2022 10:39:04 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Robin Murphy <robin.murphy@....com>,
        Prathamesh Shete <pshete@...dia.com>, joro@...tes.org,
        adrian.hunter@...el.com, jonathanh@...dia.com,
        p.zabel@...gutronix.de, linux-mmc@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
        will@...nel.org, iommu@...ts.linux.dev, anrao@...dia.com,
        smangipudi@...dia.com, kyarlagadda@...dia.com,
        Thierry Reding <treding@...dia.com>
Subject: Re: [PATCH v10 1/4] iommu: Always define struct iommu_fwspec

On Fri, Nov 04, 2022 at 03:35:32PM +0100, Ulf Hansson wrote:
> On Thu, 3 Nov 2022 at 18:35, Robin Murphy <robin.murphy@....com> wrote:
> >
> > On 2022-11-03 14:55, Ulf Hansson wrote:
> > > On Thu, 3 Nov 2022 at 15:01, Thierry Reding <thierry.reding@...il.com> wrote:
> > >>
> > >> On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> > >>> On 2022-11-03 04:38, Prathamesh Shete wrote:
> > >>>> In order to fully make use of the !IOMMU_API stub functions, make the
> > >>>> struct iommu_fwspec always available so that users of the stubs can keep
> > >>>> using the structure's internals without causing compile failures.
> > >>>
> > >>> I'm really in two minds about this... fwspecs are an internal detail of the
> > >>> IOMMU API that are meant to be private between individual drivers and
> > >>> firmware code, so anything poking at them arguably does and should depend on
> > >>> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> > >>> added for the sake of one driver that was misusing it where it really wanted
> > >>> device_iommu_mapped(), and has since been fixed, so if anything my
> > >>> preference would be to remove that stub :/
> > >>
> > >> Tegra has been using this type of weak dependency on IOMMU_API mainly in
> > >> order to allow building without the IOMMU support on some old platforms
> > >> where people may actually care about the kernel size (Tegra20 systems
> > >> were sometimes severely constrained and don't have anything that we'd
> > >> call an IOMMU today).
> > >>
> > >> We have similar stubs in place for most other major subsystems in order
> > >> to allow code to simply compile out if the subsystem is disabled, which
> > >> is quite convenient for sharing code between platforms that may want a
> > >> given feature and other platforms that may not want it, without causing
> > >> too much of a hassle with compile-testing.
> > >
> > > I agree with the above.
> > >
> > > Moreover, the stubs make the code more portable/scalable and so it
> > > becomes easier to maintain.
> >
> > Are you suggesting that having the same thing open-coded slightly
> > differently (with bugs) in 8 different places is somehow more
> > maintainable than abstracting it into a single centralised implementation?
> >
> > Is it "easier to maintain" when already seemingly every thing I try to
> > clean up or refactor in the IOMMU API at the moment is stymied by
> > finding Tegra drivers doing unexpected (and often questionable) things?
> > Is it "more scalable" to make it even easier for people to copy
> > questionable code without a second thought, leaving API maintainers to
> > play an ever-expanding game of whack-a-mole to clean it up? No. No it
> > chuffing well isn't :(
> 
> Ohh, I wasn't aware of these kinds of issues for the IOMMU interface.
> 
> Abusing interfaces is an orthogonal problem to what I was suggesting
> to solve here. The main problem I was trying to address was to prevent
> sprinkling subsystems/drivers with "#ifdefs" all over the place, as
> that doesn't scale.
> 
> >
> > >>> I don't technically have much objection to this patch in isolation, but what
> > >>> I don't like is the direction of travel it implies. I see the anti-pattern
> > >>> is only spread across Tegra drivers, making Tegra-specific assumptions, so
> > >>> in my view the best answer would be to abstract that fwpsec dependency into
> > >>> a single Tegra-specific helper, which would better represent the nature of
> > >>> what's really going on here.
> > >>
> > >> I don't see how this is an anti-pattern. It might not be common for
> > >> drivers to need to reach into iommu_fwspec, so that might indeed be
> > >> specific to Tegra (for whatever reason our IP seems to want extra
> > >> flexibility), but the general pattern of using stubs is wide-spread,
> > >> so I don't see why IOMMU_API would need to be special.
> > >
> > > Again, I agree.
> >
> > The anti-pattern is reaching into some other driver's private data
> > assuming a particular format, with zero indication of the huge degree of
> > assumption involved, and half the time not even checking that what's
> > being dereferenced is valid.
> 
> I see.
> 
> >
> > > Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
> > > isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
> > > sprinkled across the kernel. I think it would be nice if we could
> > > improve the situation. So far, using stubs along with what the
> > > $subject patch proposes, seems to me to be the best approach.
> >
> > Yes, there is plenty of code through the tree that is only relevant to
> > the IOMMU API and would be a complete waste of space without it, that is
> > not the point in question here. Grep for dev_iommu_fwspec_get; outside
> > drivers/iommu, the only users are IOMMU-API-specific parts of ACPI code,
> > as intended, plus 8 random Tegra drivers.
> >
> > Now, there does happen to be a tacit contract between the ACPI IORT code
> > and the Arm SMMU drivers for how SMMU StreamIDs are encoded in their
> > respective fwspecs, but it was never intended for wider consumption. If
> > Tegra drivers want to have a special relationship with arm-smmu then
> > fair enough, but they can do the same as MSM and formalise it somewhere
> > that the SMMU driver maintainers are at least aware of, rather than
> > holding the whole generic IOMMU API hostage.
> 
> Thanks for clarifying this. I certainly understand your concern better now.
> 
> >
> > Since apparently it wasn't clear, what I was proposing is a driver
> > helper at least something like this:
> >
> > int tegra_arm_smmu_streamid(struct device *dev)
> > {
> > #ifdef CONFIG_IOMMU_API
> >         struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
> >
> >         if (fwspec && fwspec->num_ids == 1)
> >                 return fwspec->ids[0] & 0xffff;
> > #endif
> >         return -EINVAL;
> > }
> >
> > Now THAT is scalable and maintainable; any number of random drivers can
> > call it without any preconditions, it's a lot clearer what's going on,
> > and I won't have to swear profusely while herding patches through half a
> > dozen different trees if, when my ops rework gets to the point of
> > refactoring iommu_fwspec with dev_iommu, it ends up changing anything
> > significant.
> 
> It sure sounds like we need another level of abstraction for iommus,
> to avoid the interface from being abused. Perhaps something along the
> lines of what we have for clocks (providers and consumers use
> different interfaces).

Yeah, I was thinking along the same lines. It seems a bit odd to me that
Tegra would be the only chip that ever needs access to the stream IDs
outside of the IOMMU driver), so a generic function that would allow a
device to retrieve its stream ID seems like it would be useful.

We have a few cases where a device can have multiple stream IDs where we
currently force them all to be the same for simplicity. One case where
this can happen is when a device is both a DMA engine and a DMA-capable
microcontroller in one, where the microcontroller can then use a stream
ID separate from that of the DMA engine.

We have another case where a device has multiple contexts for isolation
where things are a bit trickier, but we already have an implementation
using multiple logical devices and iommu-map to take care of that, so it
basically boils down to the above use-case.

> Anyway, to simplify future potential rework in this direction, I can
> agree and understand your points.
> 
> What you propose above, with one or a few Tegra specific helper
> functions, seems like the best we can do for now.

If this is really a Tegra-specific need, I guess we can start with a
Tegra-specific helper. We could always generalize from that at a later
point.

Thierry

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

Powered by blists - more mailing lists