Re: [PATCH 1/2] iommu: return dma alias from iommu_group_get_for_pci_dev()

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

On Fri, Jan 16, 2015 at 09:11:03PM +0000, Alex Williamson wrote:
> On Fri, 2015-01-16 at 20:33 +0000, Will Deacon wrote:
> > On Fri, Jan 16, 2015 at 05:41:51PM +0000, Alex Williamson wrote:
> > > On Fri, 2015-01-16 at 16:58 +0000, Will Deacon wrote:
> > > > Some IOMMU drivers (e.g. the ARM SMMU) require not only the IOMMU group
> > > > for a PCI device, but also the effective alias of the device (as seen by
> > > > the IOMMU) in order to program the translations correctly.
> > > > 
> > > > This patch extends iommu_group_get_for_pci_dev to return the DMA alias
> > > > of the device as well as the group, which can potentially be overridden
> > > > by quirks in the PCI layer. The function is also made visible to drivers
> > > > so that the new functionality can be used.
> > > > 
> > > > Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > > > ---
> > > >  drivers/iommu/iommu.c | 14 ++++++++++++--
> > > >  include/linux/iommu.h |  2 ++
> > > >  2 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > This would seem to promote the idea that an IOMMU group for a PCI device
> > > has a single bdf alias, which is not necessarily true.
> > 
> > True, but you can deal with that in the caller by ensuring you get the alias
> > for each member of the group, no?
> 
> If the caller needs to do that anyway (which they do), what's the point
> of this addition?  What does this alias that we're returning here
> represent versus the other aliases available?  Doesn't the alias
> returned depend on the IOMMU group member that it's called from?  Or
> even whether the group has already been established?  So even if it did
> make sense to return an alias, the answer depends on the caller's
> perspective.  Sort of a Schrödinger's cat function.

I'm trying to use this function from the ->add_device IOMMU callback, so
that I can figure out (a) which IOMMU group the new device is in and (b)
which aliases are contained in that group.

I achieve (b) not by walking the group (iommu_group_for_each_dev) on each
->add_device, but by simply adding the alias for the new device to a data
structure stashed via iommu_group_set_iommudata.

Anyway, you have some good points, more below...

> > > It also only returns the alias based on PCI topology, which can easily be
> > > discovered by the caller using pci_for_each_dma_alias() directly.  So I
> > > don't really see the benefit of this versus using
> > > iommu_group_for_each_dev() and/or pci_for_each_dma_alias().
> > 
> > The problem with using pci_for_each_dma_alias is that I end up duplicating
> > the remainder of iommu_group_get_for_pci_dev in the SMMU driver in order
> > to handle PCI_DEV_FLAGS_DMA_ALIAS_DEVFN set by PCI quirks and
> > aliasing due to ACS constraints. Furthermore, I end up walking the PCI
> > topology twice: once for the group and a second time for the aliases.
> 
> But the patch is taking the alias only after the
> pci_for_each_dma_alias() call, not after the ACS constraints, so I don't
> see how the duplicated code is any more than another
> pci_for_each_dma_alias() plus callback.  intel-iommu does this plenty.

Right, I think this is what I was missing. Given that the ACS stuff is all
about getting the right group, then I should already have seen any other
group members in ->add_device and therefore using pci_for_each_dma_alias
will work fine providing that I obtain the group using
iommu_group_get_for_dev. Any further alias transformations will be described
by the firmware. Sound sensible?

> Is walking the PCI topology a second time really an issue?  IOMMU group
> setup should be done at boot, and really how deep do you expect a PCI
> topology to be?  A "complex" device will have a switch/bridge and
> endpoints, so we're talking about up to 3 bus levels.  In theory, yes we
> could walk a monster topology, in practice, notsomuch.

Ok, I guess we can revisit this if it does ever become an issue.

> > > It's also intentional that while we have PCI specific code in iommu.c,
> > > the exposed interfaces are device agnostic.  I'm not seeing enough
> > > reason here to change that.  Thanks,
> > 
> > Hmm, I'd really rather have *something* in the core to avoid duplication
> > between all IOMMU drivers sitting on PCI without ACPI tables to describe
> > the IDs. How about a method to extract the IDs from an IOMMU group?
> 
> Well, we do have stuff in the core.  We can get an IOMMU group for a
> device, we can iterate the devices within an IOMMU group, and we can get
> alias for devices that are PCI.  The IOMMU & PCI cores facilitates all
> of that.  I don't get why we need to incorporate some combination of
> that into a new or existing interface when it implies a relationship
> that doesn't necessarily exist and biases the IOMMU interfaces towards a
> particular device type.  Thanks,

Combining the interfaces would help to reduce some redundant walking of
device topologies, but since that's not a practical problem at the moment
then I'll make do with what we have.

Will