[RFC PATCH 0/5] iommu: Introduce default domains for iommu groups

[RFC PATCH 0/5] iommu: Introduce default domains for iommu groups

[Joerg Roedel]

Hi,

this is a patch-set to add a default domain to each
iommu-group present in the system. The default domain is
used for devices that are not assigned to any other domain.

This means, when a device is detached from a domain, it
is automatically re-assigned to its default domain. These
changes make use of the domain types introduced in another
patch-set. The default domain is allocated with type
IOMMU_DOMAIN_DMA to be used in a common DMA-API implemention
later.

For now the default-domain handling is an opt-in for iommu
drivers, as some changes are required in them. First of all,
the semantics of the attach_domain callback has changed.
This callback must not fail if a device is already attached
to a domain, but just overwrite the existing attachment.
This semantic change allows to get rid of the detach_domain
callback when all drivers are converted.

A driver can enable default domains for itself by
implementing a domain_alloc callback that can return domains
of the IOMMU_DOMAIN_DMA type. But these patches only
implement domains in the core code and do not convert any
driver yet.

This patch-set also makes the difference between
attach/detach_device and attach/detach_group more explict.
With patch 3 applied, attach/detach_device will only succeed
when the device is the only one in its group. This probably
requires changes in the callers, which are not included
here.

Any useful comments and feedback appreciated.

Thanks,

	Joerg

Joerg Roedel (5):
  iommu: Add default domain to iommu-groups
  iommu: Allocate a default domain for iommu groups
  iommu: Limit iommu_attach/detach_device to devices with their own
    group
  iommu: Make sure a device is always attached to a domain
  iommu: Add iommu_get_domain_for_dev function

 drivers/iommu/iommu.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/iommu.h |   6 +++
 2 files changed, 138 insertions(+), 10 deletions(-)

-- 
1.9.1

[RFC PATCH 0/5] iommu: Introduce default domains for iommu groups

[Joerg Roedel]

Hi,

this is a patch-set to add a default domain to each
iommu-group present in the system. The default domain is
used for devices that are not assigned to any other domain.

This means, when a device is detached from a domain, it
is automatically re-assigned to its default domain. These
changes make use of the domain types introduced in another
patch-set. The default domain is allocated with type
IOMMU_DOMAIN_DMA to be used in a common DMA-API implemention
later.

For now the default-domain handling is an opt-in for iommu
drivers, as some changes are required in them. First of all,
the semantics of the attach_domain callback has changed.
This callback must not fail if a device is already attached
to a domain, but just overwrite the existing attachment.
This semantic change allows to get rid of the detach_domain
callback when all drivers are converted.

A driver can enable default domains for itself by
implementing a domain_alloc callback that can return domains
of the IOMMU_DOMAIN_DMA type. But these patches only
implement domains in the core code and do not convert any
driver yet.

This patch-set also makes the difference between
attach/detach_device and attach/detach_group more explict.
With patch 3 applied, attach/detach_device will only succeed
when the device is the only one in its group. This probably
requires changes in the callers, which are not included
here.

Any useful comments and feedback appreciated.

Thanks,

	Joerg

Joerg Roedel (5):
  iommu: Add default domain to iommu-groups
  iommu: Allocate a default domain for iommu groups
  iommu: Limit iommu_attach/detach_device to devices with their own
    group
  iommu: Make sure a device is always attached to a domain
  iommu: Add iommu_get_domain_for_dev function

 drivers/iommu/iommu.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/iommu.h |   6 +++
 2 files changed, 138 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/5] iommu: Add default domain to iommu-groups

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e66cc08..34636eb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -51,6 +51,7 @@ struct iommu_group {
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;
 	int id;
+	struct iommu_domain *default_domain;
 };
 
 struct iommu_device {
-- 
1.9.1

[PATCH 1/5] iommu: Add default domain to iommu-groups

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e66cc08..34636eb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -51,6 +51,7 @@ struct iommu_group {
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;
 	int id;
+	struct iommu_domain *default_domain;
 };
 
 struct iommu_device {
-- 
1.9.1

[PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The default domain will be used (if supported by the iommu
driver) when the devices in the iommu group are not attached
to any other domain.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 34636eb..8f33ddd3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 #define to_iommu_group(_kobj)		\
 	container_of(_kobj, struct iommu_group, kobj)
 
+static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 enum iommu_domain_type type);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -362,6 +365,17 @@ rename:
 
 	kobject_get(group->devices_kobj);
 
+	/*
+	 * Try to allocate a default domain for the group, if this
+	 * hasn't happened yet. This is not the best place to do that,
+	 * it should happen in iommu_group_alloc(). But we have no
+	 * iommu_ops there yet, so the allocation has to happen here for
+	 * now.
+	 */
+	if (group->default_domain == NULL)
+		group->default_domain = __iommu_domain_alloc(dev->bus,
+							     IOMMU_DOMAIN_DMA);
+
 	dev->iommu_group = group;
 
 	mutex_lock(&group->mutex);
@@ -899,22 +913,28 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
-struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
+static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 enum iommu_domain_type type)
 {
 	struct iommu_domain *domain;
 
 	if (bus == NULL || bus->iommu_ops == NULL)
 		return NULL;
 
-	domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
+	domain = bus->iommu_ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
 
 	domain->ops  = bus->iommu_ops;
-	domain->type = IOMMU_DOMAIN_UNMANAGED;
+	domain->type = type;
 
 	return domain;
 }
+
+struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
+{
+	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+}
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
 void iommu_domain_free(struct iommu_domain *domain)
-- 
1.9.1

[PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

The default domain will be used (if supported by the iommu
driver) when the devices in the iommu group are not attached
to any other domain.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/iommu.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 34636eb..8f33ddd3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 #define to_iommu_group(_kobj)		\
 	container_of(_kobj, struct iommu_group, kobj)
 
+static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 enum iommu_domain_type type);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -362,6 +365,17 @@ rename:
 
 	kobject_get(group->devices_kobj);
 
+	/*
+	 * Try to allocate a default domain for the group, if this
+	 * hasn't happened yet. This is not the best place to do that,
+	 * it should happen in iommu_group_alloc(). But we have no
+	 * iommu_ops there yet, so the allocation has to happen here for
+	 * now.
+	 */
+	if (group->default_domain == NULL)
+		group->default_domain = __iommu_domain_alloc(dev->bus,
+							     IOMMU_DOMAIN_DMA);
+
 	dev->iommu_group = group;
 
 	mutex_lock(&group->mutex);
@@ -899,22 +913,28 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
-struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
+static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 enum iommu_domain_type type)
 {
 	struct iommu_domain *domain;
 
 	if (bus == NULL || bus->iommu_ops == NULL)
 		return NULL;
 
-	domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
+	domain = bus->iommu_ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
 
 	domain->ops  = bus->iommu_ops;
-	domain->type = IOMMU_DOMAIN_UNMANAGED;
+	domain->type = type;
 
 	return domain;
 }
+
+struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
+{
+	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+}
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
 void iommu_domain_free(struct iommu_domain *domain)
-- 
1.9.1

[PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This patch changes the behavior of the iommu_attach_device
and iommu_detach_device functions. With this change these
functions only work on devices that have their own group.
For all other devices the iommu_group_attach/detach
functions must be used.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8f33ddd3..b63a550 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -51,6 +51,7 @@ struct iommu_group {
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;
 	int id;
+	unsigned dev_cnt;
 	struct iommu_domain *default_domain;
 };
 
@@ -380,6 +381,7 @@ rename:
 
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
+	group->dev_cnt += 1;
 	mutex_unlock(&group->mutex);
 
 	/* Notify any listeners about change to group. */
@@ -408,6 +410,7 @@ void iommu_group_remove_device(struct device *dev)
 				     IOMMU_GROUP_NOTIFY_DEL_DEVICE, dev);
 
 	mutex_lock(&group->mutex);
+	group->dev_cnt -= 1;
 	list_for_each_entry(tmp_device, &group->devices, list) {
 		if (tmp_device->dev == dev) {
 			device = tmp_device;
@@ -943,7 +946,8 @@ void iommu_domain_free(struct iommu_domain *domain)
 }
 EXPORT_SYMBOL_GPL(iommu_domain_free);
 
-int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
+static int __iommu_attach_device(struct iommu_domain *domain,
+				 struct device *dev)
 {
 	int ret;
 	if (unlikely(domain->ops->attach_dev == NULL))
@@ -954,9 +958,38 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 		trace_attach_device_to_domain(dev);
 	return ret;
 }
+
+int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return __iommu_attach_device(domain, dev);
+
+	/*
+	 * We have a group - lock it to make sure the device-count doesn't
+	 * change while we are attaching
+	 */
+	mutex_lock(&group->mutex);
+	ret = -EINVAL;
+	if (group->dev_cnt != 1)
+		goto out_unlock;
+
+	ret = iommu_attach_group(domain, group);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
-void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
+static void __iommu_detach_device(struct iommu_domain *domain,
+				  struct device *dev)
 {
 	if (unlikely(domain->ops->detach_dev == NULL))
 		return;
@@ -964,6 +997,28 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
+
+void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
+{
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return __iommu_detach_device(domain, dev);
+
+	mutex_lock(&group->mutex);
+	if (group->dev_cnt != 1) {
+		WARN_ON(1);
+		goto out_unlock;
+	}
+
+	iommu_detach_group(domain, group);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+}
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
 /*
@@ -980,7 +1035,7 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
 
-	return iommu_attach_device(domain, dev);
+	return __iommu_attach_device(domain, dev);
 }
 
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
@@ -994,7 +1049,7 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
 
-	iommu_detach_device(domain, dev);
+	__iommu_detach_device(domain, dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

This patch changes the behavior of the iommu_attach_device
and iommu_detach_device functions. With this change these
functions only work on devices that have their own group.
For all other devices the iommu_group_attach/detach
functions must be used.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8f33ddd3..b63a550 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -51,6 +51,7 @@ struct iommu_group {
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;
 	int id;
+	unsigned dev_cnt;
 	struct iommu_domain *default_domain;
 };
 
@@ -380,6 +381,7 @@ rename:
 
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
+	group->dev_cnt += 1;
 	mutex_unlock(&group->mutex);
 
 	/* Notify any listeners about change to group. */
@@ -408,6 +410,7 @@ void iommu_group_remove_device(struct device *dev)
 				     IOMMU_GROUP_NOTIFY_DEL_DEVICE, dev);
 
 	mutex_lock(&group->mutex);
+	group->dev_cnt -= 1;
 	list_for_each_entry(tmp_device, &group->devices, list) {
 		if (tmp_device->dev == dev) {
 			device = tmp_device;
@@ -943,7 +946,8 @@ void iommu_domain_free(struct iommu_domain *domain)
 }
 EXPORT_SYMBOL_GPL(iommu_domain_free);
 
-int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
+static int __iommu_attach_device(struct iommu_domain *domain,
+				 struct device *dev)
 {
 	int ret;
 	if (unlikely(domain->ops->attach_dev == NULL))
@@ -954,9 +958,38 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 		trace_attach_device_to_domain(dev);
 	return ret;
 }
+
+int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return __iommu_attach_device(domain, dev);
+
+	/*
+	 * We have a group - lock it to make sure the device-count doesn't
+	 * change while we are attaching
+	 */
+	mutex_lock(&group->mutex);
+	ret = -EINVAL;
+	if (group->dev_cnt != 1)
+		goto out_unlock;
+
+	ret = iommu_attach_group(domain, group);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
-void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
+static void __iommu_detach_device(struct iommu_domain *domain,
+				  struct device *dev)
 {
 	if (unlikely(domain->ops->detach_dev == NULL))
 		return;
@@ -964,6 +997,28 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
+
+void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
+{
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return __iommu_detach_device(domain, dev);
+
+	mutex_lock(&group->mutex);
+	if (group->dev_cnt != 1) {
+		WARN_ON(1);
+		goto out_unlock;
+	}
+
+	iommu_detach_group(domain, group);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+}
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
 /*
@@ -980,7 +1035,7 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
 
-	return iommu_attach_device(domain, dev);
+	return __iommu_attach_device(domain, dev);
 }
 
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
@@ -994,7 +1049,7 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
 
-	iommu_detach_device(domain, dev);
+	__iommu_detach_device(domain, dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Make use of the default domain and re-attach a device to it
when it is detached from another domain. Also enforce that a
device has to be in the default domain before it can be
attached to a different domain.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b63a550..5080759 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -53,6 +53,7 @@ struct iommu_group {
 	int id;
 	unsigned dev_cnt;
 	struct iommu_domain *default_domain;
+	struct iommu_domain *domain;
 };
 
 struct iommu_device {
@@ -1040,8 +1041,17 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
-	return iommu_group_for_each_dev(group, domain,
-					iommu_group_do_attach_device);
+	int ret;
+
+	if (group->default_domain && group->domain != group->default_domain)
+		return -EBUSY;
+
+	ret = iommu_group_for_each_dev(group, domain,
+				       iommu_group_do_attach_device);
+	if (ret == 0)
+		group->domain = domain;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
@@ -1056,7 +1066,25 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
-	iommu_group_for_each_dev(group, domain, iommu_group_do_detach_device);
+	int ret;
+
+	if (!group->default_domain) {
+		iommu_group_for_each_dev(group, domain,
+					 iommu_group_do_detach_device);
+		group->domain = NULL;
+		return;
+	}
+
+	if (group->domain == group->default_domain)
+		return;
+
+	/* Detach by re-attaching to the default domain */
+	ret = iommu_group_for_each_dev(group, group->default_domain,
+				       iommu_group_do_attach_device);
+	if (ret != 0)
+		WARN_ON(1);
+	else
+		group->domain = group->default_domain;
 }
 EXPORT_SYMBOL_GPL(iommu_detach_group);
 
-- 
1.9.1

[PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Make use of the default domain and re-attach a device to it
when it is detached from another domain. Also enforce that a
device has to be in the default domain before it can be
attached to a different domain.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b63a550..5080759 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -53,6 +53,7 @@ struct iommu_group {
 	int id;
 	unsigned dev_cnt;
 	struct iommu_domain *default_domain;
+	struct iommu_domain *domain;
 };
 
 struct iommu_device {
@@ -1040,8 +1041,17 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
-	return iommu_group_for_each_dev(group, domain,
-					iommu_group_do_attach_device);
+	int ret;
+
+	if (group->default_domain && group->domain != group->default_domain)
+		return -EBUSY;
+
+	ret = iommu_group_for_each_dev(group, domain,
+				       iommu_group_do_attach_device);
+	if (ret == 0)
+		group->domain = domain;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
@@ -1056,7 +1066,25 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
-	iommu_group_for_each_dev(group, domain, iommu_group_do_detach_device);
+	int ret;
+
+	if (!group->default_domain) {
+		iommu_group_for_each_dev(group, domain,
+					 iommu_group_do_detach_device);
+		group->domain = NULL;
+		return;
+	}
+
+	if (group->domain == group->default_domain)
+		return;
+
+	/* Detach by re-attaching to the default domain */
+	ret = iommu_group_for_each_dev(group, group->default_domain,
+				       iommu_group_do_attach_device);
+	if (ret != 0)
+		WARN_ON(1);
+	else
+		group->domain = group->default_domain;
 }
 EXPORT_SYMBOL_GPL(iommu_detach_group);
 
-- 
1.9.1

[PATCH 5/5] iommu: Add iommu_get_domain_for_dev function

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This function can be used to request the current domain a
device is attached to.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5080759..824442e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1022,6 +1022,24 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
+struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
+{
+	struct iommu_domain *domain;
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return NULL;
+
+	domain = group->domain;
+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
+
 /*
  * IOMMU groups are really the natrual working unit of the IOMMU, but
  * the IOMMU API works on domains and devices.  Bridge that gap by
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2951dca..dc102ad 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -177,6 +177,7 @@ extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
+extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);
 extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,
@@ -316,6 +317,11 @@ static inline void iommu_detach_device(struct iommu_domain *domain,
 {
 }
 
+static inline struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline int iommu_map(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, int gfp_order, int prot)
 {
-- 
1.9.1

[PATCH 5/5] iommu: Add iommu_get_domain_for_dev function

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

This function can be used to request the current domain a
device is attached to.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5080759..824442e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1022,6 +1022,24 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
+struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
+{
+	struct iommu_domain *domain;
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	/* FIXME: Remove this when groups a mandatory for iommu drivers */
+	if (group == NULL)
+		return NULL;
+
+	domain = group->domain;
+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
+
 /*
  * IOMMU groups are really the natrual working unit of the IOMMU, but
  * the IOMMU API works on domains and devices.  Bridge that gap by
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2951dca..dc102ad 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -177,6 +177,7 @@ extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
+extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);
 extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,
@@ -316,6 +317,11 @@ static inline void iommu_detach_device(struct iommu_domain *domain,
 {
 }
 
+static inline struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline int iommu_map(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, int gfp_order, int prot)
 {
-- 
1.9.1

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The default domain will be used (if supported by the iommu
> driver) when the devices in the iommu group are not attached
> to any other domain.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/iommu.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 34636eb..8f33ddd3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
>  #define to_iommu_group(_kobj)		\
>  	container_of(_kobj, struct iommu_group, kobj)
>  
> +static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> +						 enum iommu_domain_type type);
> +
>  static ssize_t iommu_group_attr_show(struct kobject *kobj,
>  				     struct attribute *__attr, char *buf)
>  {
> @@ -362,6 +365,17 @@ rename:
>  
>  	kobject_get(group->devices_kobj);
>  
> +	/*
> +	 * Try to allocate a default domain for the group, if this
> +	 * hasn't happened yet. This is not the best place to do that,
> +	 * it should happen in iommu_group_alloc(). But we have no
> +	 * iommu_ops there yet, so the allocation has to happen here for
> +	 * now.
> +	 */
> +	if (group->default_domain == NULL)
> +		group->default_domain = __iommu_domain_alloc(dev->bus,
> +							     IOMMU_DOMAIN_DMA);

Having a per-group domain is wasteful for IOMMUs that only support a modest
number of concurrent domains, so in reality I think we need to have one
domain per IOMMU instance. Is that possible?

One problem with the current per-bus approach is that __iommu_domain_alloc
can't figure out which IOMMU instance corresponds to the group.

Will

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> The default domain will be used (if supported by the iommu
> driver) when the devices in the iommu group are not attached
> to any other domain.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 34636eb..8f33ddd3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
>  #define to_iommu_group(_kobj)		\
>  	container_of(_kobj, struct iommu_group, kobj)
>  
> +static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> +						 enum iommu_domain_type type);
> +
>  static ssize_t iommu_group_attr_show(struct kobject *kobj,
>  				     struct attribute *__attr, char *buf)
>  {
> @@ -362,6 +365,17 @@ rename:
>  
>  	kobject_get(group->devices_kobj);
>  
> +	/*
> +	 * Try to allocate a default domain for the group, if this
> +	 * hasn't happened yet. This is not the best place to do that,
> +	 * it should happen in iommu_group_alloc(). But we have no
> +	 * iommu_ops there yet, so the allocation has to happen here for
> +	 * now.
> +	 */
> +	if (group->default_domain == NULL)
> +		group->default_domain = __iommu_domain_alloc(dev->bus,
> +							     IOMMU_DOMAIN_DMA);

Having a per-group domain is wasteful for IOMMUs that only support a modest
number of concurrent domains, so in reality I think we need to have one
domain per IOMMU instance. Is that possible?

One problem with the current per-bus approach is that __iommu_domain_alloc
can't figure out which IOMMU instance corresponds to the group.

Will

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This patch changes the behavior of the iommu_attach_device
> and iommu_detach_device functions. With this change these
> functions only work on devices that have their own group.
> For all other devices the iommu_group_attach/detach
> functions must be used.

I like this a lot. Currently, if somebody detaches a device from the ARM
SMMU, I end up detaching its group as well, which I've always found slightly
odd.

> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8f33ddd3..b63a550 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -51,6 +51,7 @@ struct iommu_group {
>  	void (*iommu_data_release)(void *iommu_data);
>  	char *name;
>  	int id;
> +	unsigned dev_cnt;

Is this actually used on a fast path, or can we just inspect the list of
devices on the group instead?

Will

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> This patch changes the behavior of the iommu_attach_device
> and iommu_detach_device functions. With this change these
> functions only work on devices that have their own group.
> For all other devices the iommu_group_attach/detach
> functions must be used.

I like this a lot. Currently, if somebody detaches a device from the ARM
SMMU, I end up detaching its group as well, which I've always found slightly
odd.

> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8f33ddd3..b63a550 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -51,6 +51,7 @@ struct iommu_group {
>  	void (*iommu_data_release)(void *iommu_data);
>  	char *name;
>  	int id;
> +	unsigned dev_cnt;

Is this actually used on a fast path, or can we just inspect the list of
devices on the group instead?

Will

Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:58AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Make use of the default domain and re-attach a device to it
> when it is detached from another domain. Also enforce that a
> device has to be in the default domain before it can be
> attached to a different domain.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index b63a550..5080759 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -53,6 +53,7 @@ struct iommu_group {
>  	int id;
>  	unsigned dev_cnt;
>  	struct iommu_domain *default_domain;
> +	struct iommu_domain *domain;
>  };
>  
>  struct iommu_device {
> @@ -1040,8 +1041,17 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
>  
>  int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
>  {
> -	return iommu_group_for_each_dev(group, domain,
> -					iommu_group_do_attach_device);
> +	int ret;
> +
> +	if (group->default_domain && group->domain != group->default_domain)
> +		return -EBUSY;

I think this is now racy with itself and detach, whereas before we always
held the group->mutex by virtue of iommu_group_for_each_dev.

Will

Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Will Deacon]

On Tue, Jan 27, 2015 at 12:08:58AM +0000, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Make use of the default domain and re-attach a device to it
> when it is detached from another domain. Also enforce that a
> device has to be in the default domain before it can be
> attached to a different domain.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index b63a550..5080759 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -53,6 +53,7 @@ struct iommu_group {
>  	int id;
>  	unsigned dev_cnt;
>  	struct iommu_domain *default_domain;
> +	struct iommu_domain *domain;
>  };
>  
>  struct iommu_device {
> @@ -1040,8 +1041,17 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
>  
>  int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
>  {
> -	return iommu_group_for_each_dev(group, domain,
> -					iommu_group_do_attach_device);
> +	int ret;
> +
> +	if (group->default_domain && group->domain != group->default_domain)
> +		return -EBUSY;

I think this is now racy with itself and detach, whereas before we always
held the group->mutex by virtue of iommu_group_for_each_dev.

Will

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Robin Murphy]

On 28/01/15 14:30, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
>> From: Joerg Roedel <jroedel@suse.de>
>>
>> The default domain will be used (if supported by the iommu
>> driver) when the devices in the iommu group are not attached
>> to any other domain.
>>
>> Signed-off-by: Joerg Roedel <jroedel@suse.de>
>> ---
>>   drivers/iommu/iommu.c | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 34636eb..8f33ddd3 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
>>   #define to_iommu_group(_kobj)		\
>>   	container_of(_kobj, struct iommu_group, kobj)
>>
>> +static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>> +						 enum iommu_domain_type type);
>> +
>>   static ssize_t iommu_group_attr_show(struct kobject *kobj,
>>   				     struct attribute *__attr, char *buf)
>>   {
>> @@ -362,6 +365,17 @@ rename:
>>
>>   	kobject_get(group->devices_kobj);
>>
>> +	/*
>> +	 * Try to allocate a default domain for the group, if this
>> +	 * hasn't happened yet. This is not the best place to do that,
>> +	 * it should happen in iommu_group_alloc(). But we have no
>> +	 * iommu_ops there yet, so the allocation has to happen here for
>> +	 * now.
>> +	 */
>> +	if (group->default_domain == NULL)
>> +		group->default_domain = __iommu_domain_alloc(dev->bus,
>> +							     IOMMU_DOMAIN_DMA);
>
> Having a per-group domain is wasteful for IOMMUs that only support a modest
> number of concurrent domains, so in reality I think we need to have one
> domain per IOMMU instance. Is that possible?

Strictly speaking, it is, provided you can identify instances (I've 
hacked up such a thing controlled from the DMA mapping side), but 
there's a question of how to handle devices with differing DMA ranges. 
The Intel IOVA allocator could actually handle them sharing one address 
space, since you can perform individual allocations with different 
constraints, but I'm not sure if that really makes sense. Perhaps one 
domain per dma-ranges variation per instance?

> One problem with the current per-bus approach is that __iommu_domain_alloc
> can't figure out which IOMMU instance corresponds to the group.

Indeed - I think it might make sense to pass devices around instead of 
buses, and for now stick in an abstraction like:

static const struct iommu_ops *get_iommu_for_dev(struct device *dev)
{
	return dev->bus->iommu_ops;
}

in which we can then later hook up some sort of of_iommu_get_ops-based 
lookup for non-PCI devices. Which ends up more or less looking like 
Thierry's original idea, but kept private to the IOMMU API internals.

Robin.

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Robin Murphy]

On 28/01/15 14:30, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
>> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
>>
>> The default domain will be used (if supported by the iommu
>> driver) when the devices in the iommu group are not attached
>> to any other domain.
>>
>> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
>> ---
>>   drivers/iommu/iommu.c | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 34636eb..8f33ddd3 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -76,6 +76,9 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
>>   #define to_iommu_group(_kobj)		\
>>   	container_of(_kobj, struct iommu_group, kobj)
>>
>> +static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>> +						 enum iommu_domain_type type);
>> +
>>   static ssize_t iommu_group_attr_show(struct kobject *kobj,
>>   				     struct attribute *__attr, char *buf)
>>   {
>> @@ -362,6 +365,17 @@ rename:
>>
>>   	kobject_get(group->devices_kobj);
>>
>> +	/*
>> +	 * Try to allocate a default domain for the group, if this
>> +	 * hasn't happened yet. This is not the best place to do that,
>> +	 * it should happen in iommu_group_alloc(). But we have no
>> +	 * iommu_ops there yet, so the allocation has to happen here for
>> +	 * now.
>> +	 */
>> +	if (group->default_domain == NULL)
>> +		group->default_domain = __iommu_domain_alloc(dev->bus,
>> +							     IOMMU_DOMAIN_DMA);
>
> Having a per-group domain is wasteful for IOMMUs that only support a modest
> number of concurrent domains, so in reality I think we need to have one
> domain per IOMMU instance. Is that possible?

Strictly speaking, it is, provided you can identify instances (I've 
hacked up such a thing controlled from the DMA mapping side), but 
there's a question of how to handle devices with differing DMA ranges. 
The Intel IOVA allocator could actually handle them sharing one address 
space, since you can perform individual allocations with different 
constraints, but I'm not sure if that really makes sense. Perhaps one 
domain per dma-ranges variation per instance?

> One problem with the current per-bus approach is that __iommu_domain_alloc
> can't figure out which IOMMU instance corresponds to the group.

Indeed - I think it might make sense to pass devices around instead of 
buses, and for now stick in an abstraction like:

static const struct iommu_ops *get_iommu_for_dev(struct device *dev)
{
	return dev->bus->iommu_ops;
}

in which we can then later hook up some sort of of_iommu_get_ops-based 
lookup for non-PCI devices. Which ends up more or less looking like 
Thierry's original idea, but kept private to the IOMMU API internals.

Robin.

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:30:06PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
> > +	if (group->default_domain == NULL)
> > +		group->default_domain = __iommu_domain_alloc(dev->bus,
> > +							     IOMMU_DOMAIN_DMA);
> 
> Having a per-group domain is wasteful for IOMMUs that only support a modest
> number of concurrent domains, so in reality I think we need to have one
> domain per IOMMU instance. Is that possible?

Well, you could make sure that there are no more groups behind one IOMMU
than the number of concurent domains it can handle. But that would be
too static. But once we have an per-iommu-descriptor in the IOMMU core I
see no reason to allocate a default domain per iommu only, based on a
policy exported by the driver.


	Joerg

Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:30:06PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:56AM +0000, Joerg Roedel wrote:
> > +	if (group->default_domain == NULL)
> > +		group->default_domain = __iommu_domain_alloc(dev->bus,
> > +							     IOMMU_DOMAIN_DMA);
> 
> Having a per-group domain is wasteful for IOMMUs that only support a modest
> number of concurrent domains, so in reality I think we need to have one
> domain per IOMMU instance. Is that possible?

Well, you could make sure that there are no more groups behind one IOMMU
than the number of concurent domains it can handle. But that would be
too static. But once we have an per-iommu-descriptor in the IOMMU core I
see no reason to allocate a default domain per iommu only, based on a
policy exported by the driver.


	Joerg

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:35:24PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> > @@ -51,6 +51,7 @@ struct iommu_group {
> >  	void (*iommu_data_release)(void *iommu_data);
> >  	char *name;
> >  	int id;
> > +	unsigned dev_cnt;
> 
> Is this actually used on a fast path, or can we just inspect the list of
> devices on the group instead?

Not really a fast path, but we have to hold the group mutex while
traversing the list, which could hurt performance somewhere else. Are
these 4 bytes a problem?


	Joerg

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:35:24PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> > @@ -51,6 +51,7 @@ struct iommu_group {
> >  	void (*iommu_data_release)(void *iommu_data);
> >  	char *name;
> >  	int id;
> > +	unsigned dev_cnt;
> 
> Is this actually used on a fast path, or can we just inspect the list of
> devices on the group instead?

Not really a fast path, but we have to hold the group mutex while
traversing the list, which could hurt performance somewhere else. Are
these 4 bytes a problem?


	Joerg

Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:38:52PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:58AM +0000, Joerg Roedel wrote:
> > -	return iommu_group_for_each_dev(group, domain,
> > -					iommu_group_do_attach_device);
> > +	int ret;
> > +
> > +	if (group->default_domain && group->domain != group->default_domain)
> > +		return -EBUSY;
> 
> I think this is now racy with itself and detach, whereas before we always
> held the group->mutex by virtue of iommu_group_for_each_dev.

You are right, thanks. I will update the code with correct locking.


	Joerg

Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain

[Joerg Roedel]

On Wed, Jan 28, 2015 at 02:38:52PM +0000, Will Deacon wrote:
> On Tue, Jan 27, 2015 at 12:08:58AM +0000, Joerg Roedel wrote:
> > -	return iommu_group_for_each_dev(group, domain,
> > -					iommu_group_do_attach_device);
> > +	int ret;
> > +
> > +	if (group->default_domain && group->domain != group->default_domain)
> > +		return -EBUSY;
> 
> I think this is now racy with itself and detach, whereas before we always
> held the group->mutex by virtue of iommu_group_for_each_dev.

You are right, thanks. I will update the code with correct locking.


	Joerg

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Will Deacon]

On Fri, Jan 30, 2015 at 12:28:14PM +0000, Joerg Roedel wrote:
> On Wed, Jan 28, 2015 at 02:35:24PM +0000, Will Deacon wrote:
> > On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> > > @@ -51,6 +51,7 @@ struct iommu_group {
> > >  	void (*iommu_data_release)(void *iommu_data);
> > >  	char *name;
> > >  	int id;
> > > +	unsigned dev_cnt;
> > 
> > Is this actually used on a fast path, or can we just inspect the list of
> > devices on the group instead?
> 
> Not really a fast path, but we have to hold the group mutex while
> traversing the list, which could hurt performance somewhere else. Are
> these 4 bytes a problem?

No problem, it just seemed a bit redundant to have two ways of describing
the same thing and having the pain of keeping them in sync with each other.

Will

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Will Deacon]

On Fri, Jan 30, 2015 at 12:28:14PM +0000, Joerg Roedel wrote:
> On Wed, Jan 28, 2015 at 02:35:24PM +0000, Will Deacon wrote:
> > On Tue, Jan 27, 2015 at 12:08:57AM +0000, Joerg Roedel wrote:
> > > @@ -51,6 +51,7 @@ struct iommu_group {
> > >  	void (*iommu_data_release)(void *iommu_data);
> > >  	char *name;
> > >  	int id;
> > > +	unsigned dev_cnt;
> > 
> > Is this actually used on a fast path, or can we just inspect the list of
> > devices on the group instead?
> 
> Not really a fast path, but we have to hold the group mutex while
> traversing the list, which could hurt performance somewhere else. Are
> these 4 bytes a problem?

No problem, it just seemed a bit redundant to have two ways of describing
the same thing and having the pain of keeping them in sync with each other.

Will

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Tue, Jan 27, 2015 at 01:08:57AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This patch changes the behavior of the iommu_attach_device
> and iommu_detach_device functions. With this change these
> functions only work on devices that have their own group.
> For all other devices the iommu_group_attach/detach
> functions must be used.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)

Sorry for my ignorance, but I don't understand what IOMMU groups are
supposed to be or how to make use of them. It seems like a common idiom
is to simply allocate a new group and add a device to it in the IOMMU's
->add_device() callback, but I fail to see the reason for that.

Can anybody point me to documentation about this? I've looked and I did
not find anything.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]

On Tue, Jan 27, 2015 at 01:08:57AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> This patch changes the behavior of the iommu_attach_device
> and iommu_detach_device functions. With this change these
> functions only work on devices that have their own group.
> For all other devices the iommu_group_attach/detach
> functions must be used.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)

Sorry for my ignorance, but I don't understand what IOMMU groups are
supposed to be or how to make use of them. It seems like a common idiom
is to simply allocate a new group and add a device to it in the IOMMU's
->add_device() callback, but I fail to see the reason for that.

Can anybody point me to documentation about this? I've looked and I did
not find anything.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

On Tue, Feb 03, 2015 at 01:25:07PM +0100, Thierry Reding wrote:
> Sorry for my ignorance, but I don't understand what IOMMU groups are
> supposed to be or how to make use of them. It seems like a common idiom
> is to simply allocate a new group and add a device to it in the IOMMU's
> ->add_device() callback, but I fail to see the reason for that.
> 
> Can anybody point me to documentation about this? I've looked and I did
> not find anything.

An iommu group is a set of devices that the iommu hardware can isolate
from other devices. So devices in one group can't be isolated from each
other and have to share the same iommu page table.

This happens for example on legac 32 bit PCI buses. All devices on such
a bus use the request-id of the PCIe-PCI bridge, so they can't be
isolated from each other from the IOMMU and have to be in one iommu
group.


	Joerg

Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group

[Joerg Roedel]

On Tue, Feb 03, 2015 at 01:25:07PM +0100, Thierry Reding wrote:
> Sorry for my ignorance, but I don't understand what IOMMU groups are
> supposed to be or how to make use of them. It seems like a common idiom
> is to simply allocate a new group and add a device to it in the IOMMU's
> ->add_device() callback, but I fail to see the reason for that.
> 
> Can anybody point me to documentation about this? I've looked and I did
> not find anything.

An iommu group is a set of devices that the iommu hardware can isolate
from other devices. So devices in one group can't be isolated from each
other and have to share the same iommu page table.

This happens for example on legac 32 bit PCI buses. All devices on such
a bus use the request-id of the PCIe-PCI bridge, so they can't be
isolated from each other from the IOMMU and have to be in one iommu
group.


	Joerg