Hey Will,
On Fri, 2015-01-16 at 20:33 +0000, Will Deacon wrote:
> Hi Alex,
>
> Thanks for having a look.
>
> 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.dea...@arm.com>
> > > ---
> > > 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.
> > 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.
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.
> > 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,
Alex
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index f7718d73e984..5300b6507481 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -616,6 +616,7 @@ static struct iommu_group *get_pci_alias_group(struct
> > > pci_dev *pdev,
> > > struct group_for_pci_data {
> > > struct pci_dev *pdev;
> > > struct iommu_group *group;
> > > + u16 alias;
> > > };
> > >
> > > /*
> > > @@ -628,6 +629,7 @@ static int get_pci_alias_or_group(struct pci_dev
> > > *pdev, u16 alias, void *opaque)
> > >
> > > data->pdev = pdev;
> > > data->group = iommu_group_get(&pdev->dev);
> > > + data->alias = alias;
> > >
> > > return data->group != NULL;
> > > }
> > > @@ -636,7 +638,8 @@ static int get_pci_alias_or_group(struct pci_dev
> > > *pdev, u16 alias, void *opaque)
> > > * Use standard PCI bus topology, isolation features, and DMA alias
> > > quirks
> > > * to find or create an IOMMU group for a device.
> > > */
> > > -static struct iommu_group *iommu_group_get_for_pci_dev(struct pci_dev
> > > *pdev)
> > > +struct iommu_group *iommu_group_get_for_pci_dev(struct pci_dev *pdev,
> > > + u16 *alias)
> > > {
> > > struct group_for_pci_data data;
> > > struct pci_bus *bus;
> > > @@ -653,6 +656,13 @@ static struct iommu_group
> > > *iommu_group_get_for_pci_dev(struct pci_dev *pdev)
> > > return data.group;
> > >
> > > pdev = data.pdev;
> > > + if (alias) {
> > > + u8 busn = PCI_BUS_NUM(pdev->bus->number);
> > > + if (pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> > > + *alias = PCI_DEVID(busn, pdev->dma_alias_devfn);
> > > + else
> > > + *alias = data.alias;
> > > + }
> > >
> > > /*
> > > * Continue upstream from the point of minimum IOMMU granularity
> > > @@ -717,7 +727,7 @@ struct iommu_group *iommu_group_get_for_dev(struct
> > > device *dev)
> > > if (!dev_is_pci(dev))
> > > return ERR_PTR(-EINVAL);
> > >
> > > - group = iommu_group_get_for_pci_dev(to_pci_dev(dev));
> > > + group = iommu_group_get_for_pci_dev(to_pci_dev(dev), NULL);
> > >
> > > if (IS_ERR(group))
> > > return group;
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 38daa453f2e5..1fe97d62a286 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -199,6 +199,8 @@ extern int iommu_group_register_notifier(struct
> > > iommu_group *group,
> > > extern int iommu_group_unregister_notifier(struct iommu_group *group,
> > > struct notifier_block *nb);
> > > extern int iommu_group_id(struct iommu_group *group);
> > > +extern struct iommu_group *iommu_group_get_for_pci_dev(struct pci_dev
> > > *dev,
> > > + u16 *alias);
> > > extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> > >
> > > extern int iommu_domain_get_attr(struct iommu_domain *domain, enum
> > > iommu_attr,
> >
> >
> >
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
