[PATCH v8 3/5] iommu/of: Add msi address regions reservation helper

[Marc Zyngier]

On 27/09/17 14:32, Shameer Kolothum wrote:
> From: John Garry <john.garry at huawei.com>
> 
> On some platforms msi-controller address regions have to be excluded
> from normal IOVA allocation in that they are detected and decoded in
> a HW specific way by system components and so they cannot be considered
> normal IOVA address space.
> 
> Add a helper function that retrieves msi address regions through device
> tree msi mapping, so that these regions will not be translated by IOMMU
> and will be excluded from IOVA allocations.
> 
> Signed-off-by: John Garry <john.garry at huawei.com>
> [Shameer: Modified msi-parent retrieval logic]
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi at huawei.com>
> ---
>  drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h | 10 +++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e60e3db..ffd7fb7 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -21,6 +21,7 @@
>  #include <linux/iommu.h>
>  #include <linux/limits.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_iommu.h>
>  #include <linux/of_pci.h>
>  #include <linux/slab.h>
> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,
>  	return ops->of_xlate(dev, iommu_spec);
>  }
>  
> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> +{
> +	u32 *rid = data;
> +
> +	*rid = alias;
> +	return 0;
> +}
> +
>  struct of_pci_iommu_alias_info {
>  	struct device *dev;
>  	struct device_node *np;
> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
>  	return info->np == pdev->bus->dev.of_node;
>  }
>  
> +static int of_iommu_alloc_resv_region(struct device_node *np,
> +				      struct list_head *head)
> +{
> +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +	struct iommu_resv_region *region;
> +	struct resource res;
> +	int err;
> +
> +	err = of_address_to_resource(np, 0, &res);

What is the rational for registering the first region only? Surely you
can have more than one region in an MSI controller (yes, your particular
case is the ITS which has only one, but we're trying to do something
generic here).

Another issue I have with this code is that it inserts all of the ITS
MMIO in the RESV_MSI range. As long as we don't generate any page tables
for this, we're fine. But if we ever change this, we'll end-up with the
ITS programming interface being exposed to a device, which wouldn't be
acceptable.

Why can't we have some generic property instead, that would describe the
actual ranges that cannot be translated? That way, no random insertion
of a random range, and relatively future proof.

I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel
like a much nicer and simpler solution to this problem.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...