[PATCH v6 8/8] arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

[Laurent Pinchart]

Hi Alexandre,

On Sunday 18 January 2015 15:54:34 Alexandre Courbot wrote:
> On 01/16/2015 08:18 AM, Laurent Pinchart wrote:
> > On Thursday 15 January 2015 11:12:17 Will Deacon wrote:
> >> On Thu, Jan 15, 2015 at 08:28:44AM +0000, Thierry Reding wrote:
> >>> On Wed, Jan 14, 2015 at 10:46:10AM +0000, Will Deacon wrote:
> >>>> On Wed, Jan 14, 2015 at 09:00:24AM +0000, Alexandre Courbot wrote:
> >>> [...]
> >>> 
> >>>>> 2) Say you want to use the IOMMU API in your driver, and have an iommu
> >>>>> property in your device's DT node. If by chance your IOMMU is
> >>>>> registered early, you will already have a mapping automatically
> >>>>> created even before your probe function is called. Can this be
> >>>>> avoided? Is it even safe?
> >>>> 
> >>>> Currently, I think you have to either teardown the ops manually or
> >>>> return an error from of_xlate. Thierry was also looking at this sort of
> >>>> thing, so it might be worth talking to him.
> >>> 
> >>> I already explained in earlier threads why I think this is a bad idea.
> >>> It's completely unnatural for any driver to manually tear down something
> >>> that it didn't want set up in the first place. It also means that you
> >>> have to carefully audit any users of these IOMMU APIs to make sure that
> >>> they do tear down. That doesn't sound like a good incremental approach,
> >>> as evidenced by the breakage that Alex and Heiko have encountered.
> >> 
> >> Well, perhaps we hide that behind a get_iommu API or something. We *do*
> >> need this manual teardown step to support things like VFIO, so it makes
> >> sense to reuse it for other users too imo.
> >> 
> >>> The solution for me has been to completely side-step the issue and not
> >>> register the IOMMU with the new mechanism at all. That is, there's no
> >>> .of_xlate() implementation, which means that the ARM DMA API glue won't
> >>> try to be smart and use the IOMMU in ways it's not meant to be used.
> > 
> > That will break when someone will want to use the same IOMMU type for
> > devices that use the DMA mapping API to hide the IOMMU. That might not be
> > the case for your IOMMU today, but it's pretty fragile, we need to fix
> > it.
> > 
> >>> This has several advantages, such as that I can also use the regular
> >>> driver model for suspend/resume of the IOMMU, and I get to enjoy the
> >>> benefits of devres in the IOMMU driver. Probe ordering is still a tiny
> >>> issue, but we can easily solve that using explicit initcall ordering
> >>> (which really isn't any worse than IOMMU_OF_DECLARE()).
> >> 
> >> That's a pity. I'd much rather extend what we currently have to satisfy
> >> your use-case. Ho-hum.
> > 
> > Assuming we want the IOMMU to be handled transparently for the majority of
> > devices I only see two ways to fix this,
> > 
> > The first way is to create a default DMA mapping unconditionally and let
> > drivers that can't live with it tear it down. That's what is implemented
> > today.
> 
> I strongly support Thierry's point that drivers should not have to tear
> down things they don't need. The issue we are facing today is a very
> good illustration of why one should not have to do this.
> 
> Everybody hates to receive unsollicited email with a link that says "to
> unsubscribe, click here". Let's not import that unpleasant culture into
> the kernel.
> 
> I am arriving late in this discussion, but what is wrong with asking
> drivers to explicitly state that they want the DMA API to be backed by
> the IOMMU instead of forcibly making it work that way?

The vast majority of the drivers are not IOMMU-aware. We would thus need to 
add a call at the beginning of the probe function of nearly every driver that 
can perform DMA to state that the driver doesn't need to handle any IOMMU that 
might be present in the system itself. I don't think that's a better solution.

Explicitly tearing down mappings in drivers that want to manage IOMMUs isn't a 
solution I like either. A possibly better solution would be to call a function 
to state that the DMA mapping API shouldn't not handle IOMMUs. Something like

	dma_mapping_ignore_iommu(dev);

at the beginning of the probe function of such drivers could do. The function 
would perform behind the scene all operations needed to tear down everything 
that shouldn't have been set up.

> > The second way is to implement a mechanism to let drivers signal that they
> > want to handle DMA mappings themselves. As the mappings need in the
> > general case to be created before the probe function  is called
> 
> Sorry for being ignorant here, but why is that?

Because a driver can call the DMA mapping API in its probe function, to 
allocate DMA coherent memory for instance. We need to ensure that the DMA 
mapping IOMMU has set up the required IOMMU ops by that time. As explained 
above I don't like the idea of sprinkling explicit calls to initialize IOMMU 
support in the vast majority of drivers, especially when they shouldn't be 
IOMMU-aware, so we then need to initialize everything that is needed before 
the first call to the DMA mapping API.

> > we can't signal this by calling a function in probe(). A new flag field
> > for struct device_driver is a possible solution. This would however
> > require delaying the creation of DMA mappings until right before probe
> > time. Attaching to the IOMMU could be pushed to right before probe() as
> > well, which would have the added benefit of making IOMMU driver
> > implementable as real platform drivers.
> 
> Keeping the ability to write IOMMU drivers as platform drivers would be
> nice indeed.
> 
> The problem with the opt-out flag though is that one will need to check
> every single driver out there to see whether it stills behave correctly
> if its hardware is suddently put behind a IOMMU.

That's actually my default assumption :-) On ARM platforms at least, for many 
devices, whether an IOMMU is present or not is an integration property, not a 
property of the device. The same way a device shouldn't care about the exact 
bus topology that connects it to the CPU and memory, it shouldn't care about 
whether an IOMMU is present on that bus, except in special cases.

> Doing it the other way (a flag that enables IOMMU if available) sounds safer
> to me.
>
> What we have right now is a mechanism that basically makes it impossible
> to use the DMA API on many ARM platforms if ARM_DMA_USE_IOMMU is set
> (and I suspect it would also make the IOMMU unusable as well, without
> any way to fix things). This is quite concerning.
> 
> Even more concerning is that -rc5 is about to be released and we have
> in-tree drivers (Rockchip DRM) that are not working as they should
> because of this patch. Will, what is your plan to fix this? Do we have
> stuff that absolutely depends on this patch? If not, can we just revert
> it until all these issues are solved?

-- 
Regards,

Laurent Pinchart