Re: [PATCH v5 2/3] arm64: Add IOMMU dma_ops

[Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]

On Fri, Aug 07, 2015 at 04:27:56PM +0100, Robin Murphy wrote:
> As per the comments, the issue here lies in the order in which the
> OF/driver core code currently calls things for platform devices: as
> it stands we can't attach the device to a domain in
> arch_setup_dma_ops() because it doesn't have a group, and we can't
> even add it to a group ourselves because it isn't fully created and
> doesn't exist in sysfs yet. The only reason arch/arm is currently
> getting away without this workaround is that the few IOMMU drivers
> there hooked up to the generic infrastructure don't actually mind
> that they get an attach_device from arch_setup_dma_ops() before
> they've even seen an add_device (largely because they don't care
> about groups).

Hmm, what about just registering the iommu-driver in arch_setup_dma_ops
with bus_set_iommu and not care about devices? The code will iterate
over the devices already on the bus and tries to attach them to the
iommu driver. But as you said, the devices are not yet on the bus, so
when a device is later added by the OF/driver core code you can do
everything needed for the device in the add_device call-back.

This might include initializing the hardware iommu needed for the
device and setting its per-device dma_ops.

> Laurent's probe-deferral series largely solves these problems in the
> right place - adding identical boilerplate code to every IOMMU
> driver to do something they shouldn't have to know about (and don't
> necessarily have all the right information for) is exactly what we
> don't want to do. As discussed over on another thread, I'm planning
> to pick that series up and polish it off after this, but my top
> priority is getting the basic dma_ops functionality into arm64 that
> people need right now. I will be only too happy when I can submit
> the patch removing this notifier workaround ;)

I've experienced it often that someone promises me to fix things later,
but that it doesn't happen then, so please understand that I am pretty
cautious about adding such hacks ;)

> No driver other than the AMD IOMMU has any support yet. Support for
> IOMMU_DOMAIN_DMA can easily be added to existing drivers based on
> patch 1 of this series, but more generally it's not entirely clear
> how default domains are going to work beyond x86. On systems like
> Juno or Seattle with different sets of masters behind different
> IOMMU instances (with limited domain contexts each), the most
> sensible approach would be to have a single default domain per IOMMU
> (spanning domains across instances would need some hideous
> synchronisation logic for some implementations), but the current
> domain_alloc interface gives us no way to do that. On something like
> RK3288 with two different types of IOMMU on the platform "bus", it
> breaks down even further as there's no way to guarantee that
> iommu_domain_alloc() even gives you a domain from the right *driver*
> (hence bypassing it by going through ops directly here).

Default domain allocation comes down to how the bus organizes its
iommu-groups. For every group (at least in its current design) a default
domain is allocated. An a group is typically only behind a single iommu
instance.

> Only for PCI devices, via iommu_group_get_for_pci_dev(). The code
> here, however, only runs for platform devices - ops will be always
> null for a PCI device since of_iommu_configure() will have bailed
> out (see the silly warning removed by my patch you picked up the
> other day). Once iommu_group_get_for_dev() supports platform
> devices, this too can go away. In the meantime if someone adds PCI
> support to of_iommu_configure() and IOMMU_DOMAIN_DMA support to
> their IOMMU driver, then we'll get a domain back from
> iommu_get_domain_for_dev() and just use that.

What is the plan for getting an iommu-groups implementation for the
platform bus?


	Joerg