Re: [PATCH v2] devicetree: Add generic IOMMU device tree bindings

[Thierry Reding]

On Sat, Jun 07, 2014 at 03:22:13PM +0200, Arnd Bergmann wrote:
> On Saturday 07 June 2014 00:45:45 Thierry Reding wrote:
> > This is somewhat off-topic, but given the various concepts discussed in
> > this thread I'm beginning to wonder how they will be implemented.
> 
> I think it's good you raised the question.
> 
> > The
> > current implementation hooks the IOMMU API into the DMA mapping API, and
> > the way this is done is by setting a single IOMMU (or rather a set of
> > IOMMU operations) globally per bus type.
> 
> I hadn't realized that we have a per-bus iommu_ops pointer. I agree
> this will become a limitation as soon as we have a soc with two different
> IOMMUs that have platform devices attached, and it has to be moved into
> the device or a structure related to that.
> 
> If that turns out controversial, we can probably have a set of pseudo
> iommu ops that just call into dev->archdata->iommu_ops->function()
> for ARM.
> 
> > There are two issues that I can see with that: one is that we can't
> > support multiple IOMMUs in the system at all, and the other is that
> > there is no context associated with the IOMMU ops, and therefore there
> > is no way to differentiate between two instances of the same IOMMU. A
> > few drivers use global variables to keep track of context information
> > but that won't work with multiple instances, unless they keep a global
> > list of all instances and then require explicit lookup in each of the
> > IOMMU operations. That sounds more like a workaround rather than a
> > proper solution to me.
> 
> Supporting multiple iommus that share one iommu driver should work
> without such hacks, as you can put the per-device information into
> dev->device_dma_parameters (this works only for very simple IOMMUs)
> or dev->archdata->iommu

I was talking about the lack of a place to store context for the IOMMU
itself. Currently none of the functions in iommu_ops have a way to get
access to the IOMMU context itself. In fact there's not even a common
structure that could be used for this purpose. I have a couple of local
patches that try to add something like that, along with functions to
more explicitly hook up a device with it's IOMMU(s). It looks somewhat
like this:

	struct iommu {
		struct device *dev;

		struct list_head list;

		const struct iommu_ops *ops;
	};

	/* register and unregister IOMMU device with core */
	int iommu_add(struct iommu *iommu);
	void iommu_remove(struct iommu *iommu);

	/*
	 * Attach a device to one or more IOMMUs (according to the
	 * iommus property).
	 */
	int iommu_attach(struct device *dev);
	int iommu_detach(struct device *dev);

Does that look like a direction that we would want to pursue?

> (we may want to generalize that, I think someone just posted patches
> for it).

Perhaps you mean this:

	[PATCHv3 1/3] device.h: arm dma-iommu: Move out dma_iommu_mapping struct

? From a quick glance that indeed looks like a promising step towards
unifying this across architectures.

> > Discussion in this thread indicates that both of the above will be
> > required at some point. Have I completely missed something or will we
> > have to rework (parts of) the IOMMU API to make this work?
> > 
> > One other thing that I have some difficulty understanding is how we can
> > support things like process isolation using the current IOMMU API. Since
> > a device will be statically assigned to one IOMMU domain at probe time
> > there is no way we can change the address space upon a context switch.
> 
> We have just introduced a way to parse dma-ranges in of_dma_configure().
> 
> The only way I see this done for platform devices is to do the IOMMU
> configuration in the same place: if an iommus property is found there,
> we call out to the iommu driver that matches the respective iommu device
> and let it configure the master device.
> 
> The device already has multiple properties related to iommus:
> 'struct device_dma_parameters', 'archdata', 'iommu_group', and
> pdev_archdata for platform devices. This should be enough to set up
> the default iommu dma_map_ops so we can have non-isolated DMA using
> dma_map_* and dma_alloc_coherent.

Right, I think up to that point things should be fine with the existing
IOMMU API and using only DMA mapping functions.

> I haven't given much thought to devices that want to use the IOMMU
> API directly so they can have multiple domains rather than rely on
> the dma-mapping abstraction.

I'm specifically thinking about cases where we want to use the IOMMU to
isolate processes from each other. This is probably most relevant for
GPUs, since they are driven to a large degree from userspace. Other
peripherals are mostly services in kernel space exclusively, so I don't
think the issue is as relevant there.

On Tegra there's two IOMMUs, one system-wide and another one directly
used by the GPU (and programmed by the GPU driver (nouveau)). For the
latter it probably doesn't make sense to expose it via the IOMMU API,
and we may be able to get sufficient process isolation using only it
rather than in combination with the SMMU. However some of the graphics
components don't master through the GPU's IOMMU, so if we want any kind
of process isolation there we may need to control the IOMMU more
explicitly. I'm still trying to get a more full understanding of this,
but it would seem to me that we'd need some way to use different domains
(which I think is the proper abstraction for what the Tegra SMMU calls
an Address Space ID (ASID)) in one driver, depending on the current
process.

Thierry

Attachment: pgpZwKnDO1PrV.pgp
Description: PGP signature