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

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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,