[PATCH 0/2] Reuse generic PCI DMA alias parsing code for the ARM SMMU

[PATCH 0/2] Reuse generic PCI DMA alias parsing code for the ARM SMMU

[Will Deacon]

Hi all,

This series of two patches removes the (incomplete) PCI DMA alias parsing
code from the ARM SMMU driver and instead modifies the generic
iommu_group_get_for_pci_dev implementation to return the alias as well as
the group.

One snag is that iommu_group_get_for_pci_dev is no longer static after
the change, but returning the alias from iommu_group_get_for_dev doesn't
feel right either, as we don't currently have any generic IOMMU grouping
for the platform bus.

Feedback welcome,

Will

--->8

Will Deacon (2):
  iommu: return dma alias from iommu_group_get_for_pci_dev()
  iommu/arm-smmu: remove homebrew PCI dma alias parsing

 drivers/iommu/arm-smmu.c | 58 ++++++++++++++++++++++++++----------------------
 drivers/iommu/iommu.c    | 14 ++++++++++--
 include/linux/iommu.h    |  2 ++
 3 files changed, 45 insertions(+), 29 deletions(-)

-- 
2.1.4

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

[Will Deacon]

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(-)

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,
-- 
2.1.4

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

[Alex Williamson]

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.  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().

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,

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,

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

[Will Deacon]

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.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?

> 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.

> 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?

Will

> 
> 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,
> 
> 
> 
> 

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

[Alex Williamson]

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.deacon@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

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

[Will Deacon]

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

[PATCH 2/2] iommu/arm-smmu: remove homebrew PCI dma alias parsing

[Will Deacon]

Core code can walk the PCI topology and extract the DMA alias for us
whilst retrieving the IOMMU group for a device, so use that instead.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 58 ++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 006f006c35e9..779c248a140d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1255,12 +1255,6 @@ static bool arm_smmu_capable(enum iommu_cap cap)
 	}
 }
 
-static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data)
-{
-	*((u16 *)data) = alias;
-	return 0; /* Continue walking */
-}
-
 static void __arm_smmu_release_pci_iommudata(void *data)
 {
 	kfree(data);
@@ -1269,56 +1263,66 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
-	struct arm_smmu_master_cfg *cfg;
 	struct iommu_group *group;
-	void (*releasefn)(void *) = NULL;
 	int ret;
 
 	smmu = find_smmu_for_device(dev);
 	if (!smmu)
 		return -ENODEV;
 
-	group = iommu_group_alloc();
-	if (IS_ERR(group)) {
-		dev_err(dev, "Failed to allocate IOMMU group\n");
-		return PTR_ERR(group);
-	}
-
 	if (dev_is_pci(dev)) {
+		u16 sid;
+		struct arm_smmu_master_cfg *cfg;
 		struct pci_dev *pdev = to_pci_dev(dev);
+		void (*releasefn)(void *) = __arm_smmu_release_pci_iommudata;
 
-		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+		group = iommu_group_get_for_pci_dev(pdev, &sid);
+		if (IS_ERR(group))
+			goto out_no_group;
+
+		cfg = iommu_group_get_iommudata(group);
 		if (!cfg) {
-			ret = -ENOMEM;
+			cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+			if (!cfg) {
+				ret = -ENOMEM;
+				goto out_put_group;
+			}
+
+			iommu_group_set_iommudata(group, cfg, releasefn);
+		}
+
+		if (cfg->num_streamids >= MAX_MASTER_STREAMIDS) {
+			ret = -ENOSPC;
 			goto out_put_group;
 		}
 
-		cfg->num_streamids = 1;
 		/*
 		 * Assume Stream ID == Requester ID for now.
 		 * We need a way to describe the ID mappings in FDT.
 		 */
-		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
-				       &cfg->streamids[0]);
-		releasefn = __arm_smmu_release_pci_iommudata;
+		cfg->streamids[cfg->num_streamids++] = sid;
 	} else {
 		struct arm_smmu_master *master;
 
 		master = find_smmu_master(smmu, dev->of_node);
-		if (!master) {
-			ret = -ENODEV;
-			goto out_put_group;
-		}
+		if (!master)
+			return -ENODEV;
+
+		group = iommu_group_alloc();
+		if (IS_ERR(group))
+			goto out_no_group;
 
-		cfg = &master->cfg;
+		iommu_group_set_iommudata(group, &master->cfg, NULL);
 	}
 
-	iommu_group_set_iommudata(group, cfg, releasefn);
-	ret = iommu_group_add_device(group, dev);
+	return iommu_group_add_device(group, dev);
 
 out_put_group:
 	iommu_group_put(group);
 	return ret;
+out_no_group:
+	dev_err(dev, "Failed to allocate IOMMU group\n");
+	return PTR_ERR(group);
 }
 
 static void arm_smmu_remove_device(struct device *dev)
-- 
2.1.4

RE: [PATCH 2/2] iommu/arm-smmu: remove homebrew PCI dma alias parsing

[Varun Sethi]

Hi Will,
Please find my query  inline.

Regards
Varun

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Will Deacon
> Sent: Friday, January 16, 2015 10:29 PM
> To: alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Cc: m-karicheri2-l0cyMroinI0@public.gmane.org; Will Deacon; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org;
> arnd-r2nGTMty4D4@public.gmane.org
> Subject: [PATCH 2/2] iommu/arm-smmu: remove homebrew PCI dma alias
> parsing
> 
> Core code can walk the PCI topology and extract the DMA alias for us whilst
> retrieving the IOMMU group for a device, so use that instead.
> 
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 58 ++++++++++++++++++++++++++-----------
> -----------
>  1 file changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> 006f006c35e9..779c248a140d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1255,12 +1255,6 @@ static bool arm_smmu_capable(enum iommu_cap
> cap)
>  	}
>  }
> 
> -static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void
> *data) -{
> -	*((u16 *)data) = alias;
> -	return 0; /* Continue walking */
> -}
> -
>  static void __arm_smmu_release_pci_iommudata(void *data)  {
>  	kfree(data);
> @@ -1269,56 +1263,66 @@ static void
> __arm_smmu_release_pci_iommudata(void *data)  static int
> arm_smmu_add_device(struct device *dev)  {
>  	struct arm_smmu_device *smmu;
> -	struct arm_smmu_master_cfg *cfg;
>  	struct iommu_group *group;
> -	void (*releasefn)(void *) = NULL;
>  	int ret;
> 
>  	smmu = find_smmu_for_device(dev);
>  	if (!smmu)
>  		return -ENODEV;
> 
> -	group = iommu_group_alloc();
> -	if (IS_ERR(group)) {
> -		dev_err(dev, "Failed to allocate IOMMU group\n");
> -		return PTR_ERR(group);
> -	}
> -
>  	if (dev_is_pci(dev)) {
> +		u16 sid;
> +		struct arm_smmu_master_cfg *cfg;
>  		struct pci_dev *pdev = to_pci_dev(dev);
> +		void (*releasefn)(void *) =
> __arm_smmu_release_pci_iommudata;
> 
> -		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +		group = iommu_group_get_for_pci_dev(pdev, &sid);
> +		if (IS_ERR(group))
> +			goto out_no_group;
> +
> +		cfg = iommu_group_get_iommudata(group);
>  		if (!cfg) {
> -			ret = -ENOMEM;
> +			cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +			if (!cfg) {
> +				ret = -ENOMEM;
> +				goto out_put_group;
> +			}
> +
> +			iommu_group_set_iommudata(group, cfg,
> releasefn);
> +		}
> +
> +		if (cfg->num_streamids >= MAX_MASTER_STREAMIDS) {
> +			ret = -ENOSPC;
>  			goto out_put_group;
>  		}
> 
> -		cfg->num_streamids = 1;
>  		/*
>  		 * Assume Stream ID == Requester ID for now.
>  		 * We need a way to describe the ID mappings in FDT.
>  		 */
> -		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
> -				       &cfg->streamids[0]);
> -		releasefn = __arm_smmu_release_pci_iommudata;
> +		cfg->streamids[cfg->num_streamids++] = sid;


[varun] Couldn't there be an issue, when a single stream id is shared by multiple devices  (in case of non transparent bridge). Wouldn't that be an issue here? When devices are attached to the domain SMR would be allocated per stream id, this could lead to duplicate stream ids.

Re: [PATCH 2/2] iommu/arm-smmu: remove homebrew PCI dma alias parsing

[Will Deacon]

On Mon, Jan 19, 2015 at 01:43:51PM +0000, Varun Sethi wrote:
> Hi Will,

Hey Varun,

> >  		/*
> >  		 * Assume Stream ID == Requester ID for now.
> >  		 * We need a way to describe the ID mappings in FDT.
> >  		 */
> > -		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
> > -				       &cfg->streamids[0]);
> > -		releasefn = __arm_smmu_release_pci_iommudata;
> > +		cfg->streamids[cfg->num_streamids++] = sid;
> 
> [varun] Couldn't there be an issue, when a single stream id is shared by
> multiple devices  (in case of non transparent bridge). Wouldn't that be an
> issue here? When devices are attached to the domain SMR would be allocated
> per stream id, this could lead to duplicate stream ids.

Yup. Since I'm reworking this code after Alex's feedback, I'll add some
code to check for duplicates before adding the new SID.

Thanks,

Will