[PATCH 0/8] iommu: Make core iommu-groups code more generic

[PATCH 0/8] iommu: Make core iommu-groups code more generic

[Joerg Roedel]

Hi,

this patch-set makes the core code for managing iommu-groups
more generic by lifting its dependencies on PCI. The core
function iommu_group_get_for_dev() had a hard dev_is_pci()
check in it, followed by PCI specific handling.

This check is removed in favour of the the revived
device_group() iommu-ops call-back. With this call-back an
IOMMU driver can define how the devices it manages are
grouped together. Two functions, one generic and one for PCI
devices, are provided that can be uses as a device_group()
call-back or might be used in such a call-back.

The existing drivers making use of the iommu_group_get_for_dev()
function are converted to this call-back.

	
	Joerg

Joerg Roedel (8):
  iommu: Revive device_group iommu-ops call-back
  iommu: Export and rename iommu_group_get_for_pci_dev()
  iommu: Add generic_device_group() function
  iommu: Add device_group call-back to x86 iommu drivers
  iommu/fsl: Convert to device_group call-back
  iommu/arm-smmu: Switch to device_group call-back
  iommu: Remove is_pci_dev() fall-back from iommu_group_get_for_dev
  iommu: Move default domain allocation to iommu_group_get_for_dev()

 drivers/iommu/amd_iommu.c       |  1 +
 drivers/iommu/arm-smmu-v3.c     |  1 +
 drivers/iommu/arm-smmu.c        | 77 ++++++++++++++++++++++++-----------------
 drivers/iommu/fsl_pamu_domain.c | 41 +++++++++-------------
 drivers/iommu/intel-iommu.c     |  1 +
 drivers/iommu/iommu.c           | 46 +++++++++++++++++-------
 include/linux/iommu.h           |  7 +++-
 7 files changed, 105 insertions(+), 69 deletions(-)

-- 
1.9.1

[PATCH 1/8] iommu: Revive device_group iommu-ops call-back

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

That call-back is currently unused, change it into a
call-back function for finding the right IOMMU group for a
device.
This is a first step to remove the hard-coded PCI dependency
in the iommu-group code.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 9 ++++++---
 include/linux/iommu.h | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 049df49..5637463 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -814,6 +814,7 @@ static struct iommu_group *iommu_group_get_for_pci_dev(struct pci_dev *pdev)
  */
 struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 {
+	const struct iommu_ops *ops = dev->bus->iommu_ops;
 	struct iommu_group *group;
 	int ret;
 
@@ -821,10 +822,12 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	if (group)
 		return group;
 
-	if (!dev_is_pci(dev))
-		return ERR_PTR(-EINVAL);
+	group = ERR_PTR(-EINVAL);
 
-	group = iommu_group_get_for_pci_dev(to_pci_dev(dev));
+	if (ops && ops->device_group)
+		group = ops->device_group(dev);
+	else if (dev_is_pci(dev))
+		group = iommu_group_get_for_pci_dev(to_pci_dev(dev));
 
 	if (IS_ERR(group))
 		return group;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f9c1b6d..5a19952 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -167,7 +167,7 @@ struct iommu_ops {
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
 	int (*add_device)(struct device *dev);
 	void (*remove_device)(struct device *dev);
-	int (*device_group)(struct device *dev, unsigned int *groupid);
+	struct iommu_group *(*device_group)(struct device *dev);
 	int (*domain_get_attr)(struct iommu_domain *domain,
 			       enum iommu_attr attr, void *data);
 	int (*domain_set_attr)(struct iommu_domain *domain,
-- 
1.9.1

[PATCH 2/8] iommu: Export and rename iommu_group_get_for_pci_dev()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Rename that function to pci_device_group() and export it, so
that IOMMU drivers can use it as their device_group
call-back.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 8 ++++++--
 include/linux/iommu.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5637463..fdea700 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -731,13 +731,17 @@ 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 *pci_device_group(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct group_for_pci_data data;
 	struct pci_bus *bus;
 	struct iommu_group *group = NULL;
 	u64 devfns[4] = { 0 };
 
+	if (WARN_ON(!dev_is_pci(dev)))
+		return ERR_PTR(-EINVAL);
+
 	/*
 	 * Find the upstream DMA alias for the device.  A device must not
 	 * be aliased due to topology in order to have its own IOMMU group.
@@ -827,7 +831,7 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	if (ops && ops->device_group)
 		group = ops->device_group(dev);
 	else if (dev_is_pci(dev))
-		group = iommu_group_get_for_pci_dev(to_pci_dev(dev));
+		group = pci_device_group(dev);
 
 	if (IS_ERR(group))
 		return group;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5a19952..8026e6a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -316,6 +316,9 @@ static inline size_t iommu_map_sg(struct iommu_domain *domain,
 	return domain->ops->map_sg(domain, iova, sg, nents, prot);
 }
 
+/* PCI device grouping function */
+extern struct iommu_group *pci_device_group(struct device *dev);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
-- 
1.9.1

[PATCH 3/8] iommu: Add generic_device_group() function

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This function can be used as a device_group call-back and
just allocates one iommu-group per device.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 15 +++++++++++++++
 include/linux/iommu.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fdea700..a80c9c5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -728,6 +728,21 @@ static int get_pci_alias_or_group(struct pci_dev *pdev, u16 alias, void *opaque)
 }
 
 /*
+ * Generic device_group call-back function. It just allocates one
+ * iommu-group per device.
+ */
+struct iommu_group *generic_device_group(struct device *dev)
+{
+	struct iommu_group *group;
+
+	group = iommu_group_alloc();
+	if (IS_ERR(group))
+		return NULL;
+
+	return group;
+}
+
+/*
  * Use standard PCI bus topology, isolation features, and DMA alias quirks
  * to find or create an IOMMU group for a device.
  */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 8026e6a..c73824f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -318,6 +318,8 @@ static inline size_t iommu_map_sg(struct iommu_domain *domain,
 
 /* PCI device grouping function */
 extern struct iommu_group *pci_device_group(struct device *dev);
+/* Generic device grouping function */
+extern struct iommu_group *generic_device_group(struct device *dev);
 
 #else /* CONFIG_IOMMU_API */
 
-- 
1.9.1

[PATCH 4/8] iommu: Add device_group call-back to x86 iommu drivers

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Set the device_group call-back to pci_device_group() for the
Intel VT-d and the AMD IOMMU driver.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c   | 1 +
 drivers/iommu/intel-iommu.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 08d2775..a76a3e5 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3198,6 +3198,7 @@ static const struct iommu_ops amd_iommu_ops = {
 	.iova_to_phys = amd_iommu_iova_to_phys,
 	.add_device = amd_iommu_add_device,
 	.remove_device = amd_iommu_remove_device,
+	.device_group = pci_device_group,
 	.get_dm_regions = amd_iommu_get_dm_regions,
 	.put_dm_regions = amd_iommu_put_dm_regions,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 35365f0..f2b276d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4873,6 +4873,7 @@ static const struct iommu_ops intel_iommu_ops = {
 	.iova_to_phys	= intel_iommu_iova_to_phys,
 	.add_device	= intel_iommu_add_device,
 	.remove_device	= intel_iommu_remove_device,
+	.device_group   = pci_device_group,
 	.pgsize_bitmap	= INTEL_IOMMU_PGSIZES,
 };
 
-- 
1.9.1

[PATCH 5/8] iommu/fsl: Convert to device_group call-back

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Convert the fsl pamu driver to make use of the new
device_group call-back.

Cc: Varun Sethi <Varun.Sethi@freescale.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/fsl_pamu_domain.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 1d45293..da0e1e3 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -923,7 +923,7 @@ static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
 	pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
 	/* We can partition PCIe devices so assign device group to the device */
 	if (pci_endpt_partioning) {
-		group = iommu_group_get_for_dev(&pdev->dev);
+		group = pci_device_group(&pdev->dev);
 
 		/*
 		 * PCIe controller is not a paritionable entity
@@ -956,44 +956,34 @@ static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
 	return group;
 }
 
-static int fsl_pamu_add_device(struct device *dev)
+static struct iommu_group *fsl_pamu_device_group(struct device *dev)
 {
 	struct iommu_group *group = ERR_PTR(-ENODEV);
-	struct pci_dev *pdev;
-	const u32 *prop;
-	int ret = 0, len;
+	int len;
 
 	/*
 	 * For platform devices we allocate a separate group for
 	 * each of the devices.
 	 */
-	if (dev_is_pci(dev)) {
-		pdev = to_pci_dev(dev);
-		/* Don't create device groups for virtual PCI bridges */
-		if (pdev->subordinate)
-			return 0;
+	if (dev_is_pci(dev))
+		group = get_pci_device_group(to_pci_dev(dev));
+	else if (of_get_property(dev->of_node, "fsl,liodn", &len))
+		group = get_device_iommu_group(dev);
 
-		group = get_pci_device_group(pdev);
+	return group;
+}
 
-	} else {
-		prop = of_get_property(dev->of_node, "fsl,liodn", &len);
-		if (prop)
-			group = get_device_iommu_group(dev);
-	}
+static int fsl_pamu_add_device(struct device *dev)
+{
+	struct iommu_group *group;
 
+	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
-	/*
-	 * Check if device has already been added to an iommu group.
-	 * Group could have already been created for a PCI device in
-	 * the iommu_group_get_for_dev path.
-	 */
-	if (!dev->iommu_group)
-		ret = iommu_group_add_device(group, dev);
-
 	iommu_group_put(group);
-	return ret;
+
+	return 0;
 }
 
 static void fsl_pamu_remove_device(struct device *dev)
@@ -1072,6 +1062,7 @@ static const struct iommu_ops fsl_pamu_ops = {
 	.domain_get_attr = fsl_pamu_get_domain_attr,
 	.add_device	= fsl_pamu_add_device,
 	.remove_device	= fsl_pamu_remove_device,
+	.device_group   = fsl_pamu_device_group,
 };
 
 int __init pamu_domain_init(void)
-- 
1.9.1

[PATCH 6/8] iommu/arm-smmu: Switch to device_group call-back

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This converts the ARM SMMU and the SMMUv3 driver to use the
new device_group call-back.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/arm-smmu-v3.c |  1 +
 drivers/iommu/arm-smmu.c    | 77 +++++++++++++++++++++++++++------------------
 2 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 286e890..e9e591c 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1902,6 +1902,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.iova_to_phys		= arm_smmu_iova_to_phys,
 	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
+	.device_group		= pci_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 48a39df..b4c0629 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1292,33 +1292,25 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }
 
-static int arm_smmu_add_pci_device(struct pci_dev *pdev)
+static int arm_smmu_init_pci_device(struct pci_dev *pdev,
+				    struct iommu_group *group)
 {
-	int i, ret;
-	u16 sid;
-	struct iommu_group *group;
 	struct arm_smmu_master_cfg *cfg;
-
-	group = iommu_group_get_for_dev(&pdev->dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
+	u16 sid;
+	int i;
 
 	cfg = iommu_group_get_iommudata(group);
 	if (!cfg) {
 		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
-		if (!cfg) {
-			ret = -ENOMEM;
-			goto out_put_group;
-		}
+		if (!cfg)
+			return -ENOMEM;
 
 		iommu_group_set_iommudata(group, cfg,
 					  __arm_smmu_release_pci_iommudata);
 	}
 
-	if (cfg->num_streamids >= MAX_MASTER_STREAMIDS) {
-		ret = -ENOSPC;
-		goto out_put_group;
-	}
+	if (cfg->num_streamids >= MAX_MASTER_STREAMIDS)
+		return -ENOSPC;
 
 	/*
 	 * Assume Stream ID == Requester ID for now.
@@ -1334,16 +1326,13 @@ static int arm_smmu_add_pci_device(struct pci_dev *pdev)
 		cfg->streamids[cfg->num_streamids++] = sid;
 
 	return 0;
-out_put_group:
-	iommu_group_put(group);
-	return ret;
 }
 
-static int arm_smmu_add_platform_device(struct device *dev)
+static int arm_smmu_init_platform_device(struct device *dev,
+					 struct iommu_group *group)
 {
-	struct iommu_group *group;
-	struct arm_smmu_master *master;
 	struct arm_smmu_device *smmu = find_smmu_for_device(dev);
+	struct arm_smmu_master *master;
 
 	if (!smmu)
 		return -ENODEV;
@@ -1352,21 +1341,20 @@ static int arm_smmu_add_platform_device(struct device *dev)
 	if (!master)
 		return -ENODEV;
 
-	/* No automatic group creation for platform devices */
-	group = iommu_group_alloc();
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
 	iommu_group_set_iommudata(group, &master->cfg, NULL);
-	return iommu_group_add_device(group, dev);
+
+	return 0;
 }
 
 static int arm_smmu_add_device(struct device *dev)
 {
-	if (dev_is_pci(dev))
-		return arm_smmu_add_pci_device(to_pci_dev(dev));
+	struct iommu_group *group;
 
-	return arm_smmu_add_platform_device(dev);
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	return 0;
 }
 
 static void arm_smmu_remove_device(struct device *dev)
@@ -1374,6 +1362,32 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_group_remove_device(dev);
 }
 
+static struct iommu_group *arm_smmu_device_group(struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	if (dev_is_pci(dev))
+		group = pci_device_group(dev);
+	else
+		group = generic_device_group(dev);
+
+	if (IS_ERR(group))
+		return group;
+
+	if (dev_is_pci(dev))
+		ret = arm_smmu_init_pci_device(to_pci_dev(dev), group);
+	else
+		ret = arm_smmu_init_platform_device(dev, group);
+
+	if (ret) {
+		iommu_group_put(group);
+		group = ERR_PTR(ret);
+	}
+
+	return group;
+}
+
 static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 				    enum iommu_attr attr, void *data)
 {
@@ -1430,6 +1444,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.iova_to_phys		= arm_smmu_iova_to_phys,
 	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
+	.device_group		= arm_smmu_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
-- 
1.9.1

[PATCH 7/8] iommu: Remove is_pci_dev() fall-back from iommu_group_get_for_dev

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

All callers of iommu_group_get_for_dev() provide a
device_group call-back now, so this fall-back is no longer
needed.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a80c9c5..e2b5526 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -845,8 +845,6 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 
 	if (ops && ops->device_group)
 		group = ops->device_group(dev);
-	else if (dev_is_pci(dev))
-		group = pci_device_group(dev);
 
 	if (IS_ERR(group))
 		return group;
-- 
1.9.1

[PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Now that the iommu core support for iommu groups is not
pci-centric anymore, we can move default domain allocation
to the bus independent iommu_group_get_for_dev() function.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e2b5526..abae363 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -810,14 +810,6 @@ struct iommu_group *pci_device_group(struct device *dev)
 	if (IS_ERR(group))
 		return NULL;
 
-	/*
-	 * Try to allocate a default domain - needs support from the
-	 * IOMMU driver.
-	 */
-	group->default_domain = __iommu_domain_alloc(pdev->dev.bus,
-						     IOMMU_DOMAIN_DMA);
-	group->domain = group->default_domain;
-
 	return group;
 }
 
@@ -849,6 +841,16 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	if (IS_ERR(group))
 		return group;
 
+	/*
+	 * Try to allocate a default domain - needs support from the
+	 * IOMMU driver.
+	 */
+	if (!group->default_domain) {
+		group->default_domain = __iommu_domain_alloc(dev->bus,
+							     IOMMU_DOMAIN_DMA);
+		group->domain = group->default_domain;
+	}
+
 	ret = iommu_group_add_device(group, dev);
 	if (ret) {
 		iommu_group_put(group);
-- 
1.9.1

Re: [PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()

[Will Deacon]

Hi Joerg,

Looking at this from an SMMU perspective, I think there's an issue
with attaching to the default domain like this.

On Wed, Oct 21, 2015 at 11:51:43PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Now that the iommu core support for iommu groups is not
> pci-centric anymore, we can move default domain allocation
> to the bus independent iommu_group_get_for_dev() function.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/iommu.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e2b5526..abae363 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -810,14 +810,6 @@ struct iommu_group *pci_device_group(struct device *dev)
>  	if (IS_ERR(group))
>  		return NULL;
>  
> -	/*
> -	 * Try to allocate a default domain - needs support from the
> -	 * IOMMU driver.
> -	 */
> -	group->default_domain = __iommu_domain_alloc(pdev->dev.bus,
> -						     IOMMU_DOMAIN_DMA);
> -	group->domain = group->default_domain;
> -
>  	return group;
>  }
>  
> @@ -849,6 +841,16 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>  	if (IS_ERR(group))
>  		return group;
>  
> +	/*
> +	 * Try to allocate a default domain - needs support from the
> +	 * IOMMU driver.
> +	 */
> +	if (!group->default_domain) {
> +		group->default_domain = __iommu_domain_alloc(dev->bus,
> +							     IOMMU_DOMAIN_DMA);
> +		group->domain = group->default_domain;
> +	}
> +
>  	ret = iommu_group_add_device(group, dev);
>  	if (ret) {
>  		iommu_group_put(group);

The call to iommu_group_get_for_dev in arm_smmu_add_device will end up
calling __iommu_attach_device, since group->domain will now be initialised
by the code above. This means the SMMU driver will see an ->attach_dev
call for a device that is part-way through an ->add_device callback and
will be missing the initialisation necessary for us to idenfity the SMMU
instance to which is corresponds. In fact, the iommudata for the group
won't be initialised at all, so the whole thing will fail afaict.

Note that I haven't actually taken this for a spin, so I could be missing
something.

Will

Re: [PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()

[Joerg Roedel]

Hi Will,

On Thu, Oct 29, 2015 at 06:22:49PM +0000, Will Deacon wrote:
> The call to iommu_group_get_for_dev in arm_smmu_add_device will end up
> calling __iommu_attach_device, since group->domain will now be initialised
> by the code above. This means the SMMU driver will see an ->attach_dev
> call for a device that is part-way through an ->add_device callback and
> will be missing the initialisation necessary for us to idenfity the SMMU
> instance to which is corresponds. In fact, the iommudata for the group
> won't be initialised at all, so the whole thing will fail afaict.
> 
> Note that I haven't actually taken this for a spin, so I could be missing
> something.

Yeah, I havn't looked at how to convert the ARM-SMMU drivers to default
domains yet, so the issue you describe above is totally possible.

But there is no way to trigger it yet, because your domain_alloc
function can not yet allocate IOMMU_DOMAIN_DMA domains. While converting
the issue must be fixed, of course.

I tested this patch-set on an AMD Seattle system and it worked fine
there.


	Joerg

Re: [PATCH 0/8] iommu: Make core iommu-groups code more generic

[Yong Wu]

On Wed, 2015-10-21 at 23:51 +0200, Joerg Roedel wrote:
> Hi,
> 
> this patch-set makes the core code for managing iommu-groups
> more generic by lifting its dependencies on PCI. The core
> function iommu_group_get_for_dev() had a hard dev_is_pci()
> check in it, followed by PCI specific handling.
> 
> This check is removed in favour of the the revived
> device_group() iommu-ops call-back. With this call-back an
> IOMMU driver can define how the devices it manages are
> grouped together. Two functions, one generic and one for PCI
> devices, are provided that can be uses as a device_group()
> call-back or might be used in such a call-back.
> 
> The existing drivers making use of the iommu_group_get_for_dev()
> function are converted to this call-back.
> 
> 	
> 	Joerg
> 
(Resend since some mail servers reject this.)

Hi Joerg,
    Thanks for this patch-set. With this patch-set we can implement our
owner device_group(), then we could achieve all the iommu devices into
the same m4u iommu-group easily.
    
    This patch-set have been merged into v4.4-rc1 where I will get a
WARN while I test iommu_detach_device, below is the warning log:

(151119_13:39:37.472)WARNING:
at /proj/mtk40525/upstreamdev/v4.4/kernel/mediatek/drivers/iommu/iommu.c:1154
(151119_13:39:37.472)Modules linked in:
(151119_13:39:37.472)CPU: 1 PID: 731 Comm: sh Not tainted 4.4.0-rc1+
#37
(151119_13:39:37.472)Hardware name: MediaTek MT8173 evaluation board
(DT)
(151119_13:39:37.472)task: ffffffc076bb4d00 ti: ffffffc076bdc000
task.ti: ffffffc076bdc000
(151119_13:39:37.472)PC is at iommu_detach_device+0x5c/0xb0
(151119_13:39:37.472)LR is at iommu_detach_device+0x30/0xb0
xxx

>From the code, there are 2 places confuse me:
1. It seems that the iommu core has supposed that there is only one
device in each a group. In the iommu_detach_device/iommu_attach_device
there is a checking like this:

        if (iommu_group_device_count(group) != 1) {
                WARN_ON(1);
                goto out_unlock;
        }

   Then for our case(there are many devices in a group), Could we delete
this checking?

2. iommu_detach_device will call __iommu_detach_group in which it will
return directly if group->iommu==group->default_iommu, Unfortunately
both will be the same if we call iommu_group_get_for_dev(). then
iommu_detach_device cann't finish the detach operation.
   And if there are many devices in a group, It seems that we can not
detach whole the group in the iommu_detach_device.
 
/* ============================= */
Below is our add_device() and device_group(), if we call the wrong iommu
interface, please also tell me. Thanks.

static int mtk_iommu_add_device(struct device *dev)
{
        struct iommu_group *group;

        if (!dev->archdata.iommu) /* Not a iommu client device */
                return -ENODEV;

        group = iommu_group_get_for_dev(dev);
        if (IS_ERR(group))
                return PTR_ERR(group);

        iommu_group_put(group);
        return 0;
}

static struct iommu_group *mtk_iommu_device_group(struct device *dev)
{
        struct mtk_iommu_data *data;
        struct mtk_iommu_client_priv *priv;

        priv = dev->archdata.iommu;
        if (!priv)
                return ERR_PTR(-ENODEV);

        /* All the client devices are in the same m4u iommu-group */
        data = dev_get_drvdata(priv->m4udev);
        if (!data->m4ugroup) {
                data->m4ugroup = iommu_group_alloc();
                if (IS_ERR(data->m4ugroup))
                        dev_err(dev, "Failed to allocate M4U IOMMU group
\n");
        }
        return data->m4ugroup;
}
/* ============================= */

Re: [PATCH 0/8] iommu: Make core iommu-groups code more generic

[Joerg Roedel]

Hi Yong Wu,

On Thu, Nov 19, 2015 at 04:13:21PM +0800, Yong Wu wrote:
> (151119_13:39:37.472)WARNING:
> at /proj/mtk40525/upstreamdev/v4.4/kernel/mediatek/drivers/iommu/iommu.c:1154
> (151119_13:39:37.472)Modules linked in:
> (151119_13:39:37.472)CPU: 1 PID: 731 Comm: sh Not tainted 4.4.0-rc1+
> #37
> (151119_13:39:37.472)Hardware name: MediaTek MT8173 evaluation board
> (DT)
> (151119_13:39:37.472)task: ffffffc076bb4d00 ti: ffffffc076bdc000
> task.ti: ffffffc076bdc000
> (151119_13:39:37.472)PC is at iommu_detach_device+0x5c/0xb0
> (151119_13:39:37.472)LR is at iommu_detach_device+0x30/0xb0
> xxx

This warning triggers when you use iommu_detach_device() with a device
that is in a group with other devices. For those devices you must
attach/detach the whole group of devices.

For that you have to use iommu_detach_group() and iommu_attach_group().


	Joerg

Re: [PATCH 0/8] iommu: Make core iommu-groups code more generic

[Yong Wu]

On Thu, 2015-11-19 at 14:41 +0100, Joerg Roedel wrote:
> Hi Yong Wu,
> 
> On Thu, Nov 19, 2015 at 04:13:21PM +0800, Yong Wu wrote:
> > (151119_13:39:37.472)WARNING:
> > at /proj/mtk40525/upstreamdev/v4.4/kernel/mediatek/drivers/iommu/iommu.c:1154
> > (151119_13:39:37.472)Modules linked in:
> > (151119_13:39:37.472)CPU: 1 PID: 731 Comm: sh Not tainted 4.4.0-rc1+
> > #37
> > (151119_13:39:37.472)Hardware name: MediaTek MT8173 evaluation board
> > (DT)
> > (151119_13:39:37.472)task: ffffffc076bb4d00 ti: ffffffc076bdc000
> > task.ti: ffffffc076bdc000
> > (151119_13:39:37.472)PC is at iommu_detach_device+0x5c/0xb0
> > (151119_13:39:37.472)LR is at iommu_detach_device+0x30/0xb0
> > xxx
> 
> This warning triggers when you use iommu_detach_device() with a device
> that is in a group with other devices. For those devices you must
> attach/detach the whole group of devices.
> 
> For that you have to use iommu_detach_group() and iommu_attach_group().
> 
> 
> 	Joerg
> 

     Currently We have to use iommu_detach_group and iommu_attach_group.
Is there any plan for adding detach/attach the device within a iommu
group?

Why I ask this is because that:

    There are 2 interfaces in MTK-SMI patch, named by
mtk_smi_larb_get/mtk_smi_larb_put which help enable/disable the
power-domain and clocks for each a device(local arbiter).
Take a example, VIDEO have to call mtk_smi_larb_get to enable its power
and clocks before it works and call mtk_smi_larb_put to disable them
after it work done.

    If the iommu support attach/detach the device in a iommu-group, then
we could request VIDEO to call iommu_attach_device before it works(the
mtk_iommu_attach_device will call the smi interface to enable its power,
clocks and enable iommu).and call iommu_detach_device after it work
done. then the VIDEO could call iommu_attach_device/iommu_detach_device
instead of talking with SMI directly.

Re: [PATCH 0/8] iommu: Make core iommu-groups code more generic

[Joerg Roedel]

Hi Yong Wu,

On Tue, Dec 01, 2015 at 07:29:51PM +0800, Yong Wu wrote:
>      Currently We have to use iommu_detach_group and iommu_attach_group.
> Is there any plan for adding detach/attach the device within a iommu
> group?

No, there is no such plan. The reason is using attach/detach_device
always also affects the other devices in the iommu-group. Therefore
attach/detach_device was limited to devices which have no others in
their group.
Having attach/detach_group makes it more obvious to the user that the
call affects more than one device.


	Joerg