LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups
@ 2015-01-27  0:08 Joerg Roedel
  2015-01-27  0:08 ` [PATCH 1/5] iommu: Add default domain to iommu-groups Joerg Roedel
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] iommu: Add default domain to iommu-groups
  2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
@ 2015-01-27  0:08 ` Joerg Roedel
  2015-01-27  0:08 ` [PATCH 2/5] iommu: Allocate a default domain for iommu groups Joerg Roedel
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/5] iommu: Allocate a default domain for iommu groups
  2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
  2015-01-27  0:08 ` [PATCH 1/5] iommu: Add default domain to iommu-groups Joerg Roedel
@ 2015-01-27  0:08 ` Joerg Roedel
  2015-01-28 14:30   ` Will Deacon
  2015-01-27  0:08 ` [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group Joerg Roedel
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
  2015-01-27  0:08 ` [PATCH 1/5] iommu: Add default domain to iommu-groups Joerg Roedel
  2015-01-27  0:08 ` [PATCH 2/5] iommu: Allocate a default domain for iommu groups Joerg Roedel
@ 2015-01-27  0:08 ` Joerg Roedel
  2015-01-28 14:35   ` Will Deacon
  2015-02-03 12:25   ` Thierry Reding
  2015-01-27  0:08 ` [PATCH 4/5] iommu: Make sure a device is always attached to a domain Joerg Roedel
  2015-01-27  0:08 ` [PATCH 5/5] iommu: Add iommu_get_domain_for_dev function Joerg Roedel
  4 siblings, 2 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/5] iommu: Make sure a device is always attached to a domain
  2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
                   ` (2 preceding siblings ...)
  2015-01-27  0:08 ` [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group Joerg Roedel
@ 2015-01-27  0:08 ` Joerg Roedel
  2015-01-28 14:38   ` Will Deacon
  2015-01-27  0:08 ` [PATCH 5/5] iommu: Add iommu_get_domain_for_dev function Joerg Roedel
  4 siblings, 1 reply; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/5] iommu: Add iommu_get_domain_for_dev function
  2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
                   ` (3 preceding siblings ...)
  2015-01-27  0:08 ` [PATCH 4/5] iommu: Make sure a device is always attached to a domain Joerg Roedel
@ 2015-01-27  0:08 ` Joerg Roedel
  4 siblings, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-27  0:08 UTC (permalink / raw)
  To: iommu
  Cc: Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Thierry Reding, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart, Joerg Roedel,
	jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups
  2015-01-27  0:08 ` [PATCH 2/5] iommu: Allocate a default domain for iommu groups Joerg Roedel
@ 2015-01-28 14:30   ` Will Deacon
  2015-01-28 15:11     ` Robin Murphy
  2015-01-30 12:25     ` Joerg Roedel
  0 siblings, 2 replies; 16+ messages in thread
From: Will Deacon @ 2015-01-28 14:30 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-01-27  0:08 ` [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group Joerg Roedel
@ 2015-01-28 14:35   ` Will Deacon
  2015-01-30 12:28     ` Joerg Roedel
  2015-02-03 12:25   ` Thierry Reding
  1 sibling, 1 reply; 16+ messages in thread
From: Will Deacon @ 2015-01-28 14:35 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain
  2015-01-27  0:08 ` [PATCH 4/5] iommu: Make sure a device is always attached to a domain Joerg Roedel
@ 2015-01-28 14:38   ` Will Deacon
  2015-01-30 12:29     ` Joerg Roedel
  0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2015-01-28 14:38 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups
  2015-01-28 14:30   ` Will Deacon
@ 2015-01-28 15:11     ` Robin Murphy
  2015-01-30 12:25     ` Joerg Roedel
  1 sibling, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2015-01-28 15:11 UTC (permalink / raw)
  To: Will Deacon, Joerg Roedel
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Laurent Pinchart, jroedel

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] iommu: Allocate a default domain for iommu groups
  2015-01-28 14:30   ` Will Deacon
  2015-01-28 15:11     ` Robin Murphy
@ 2015-01-30 12:25     ` Joerg Roedel
  1 sibling, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-30 12:25 UTC (permalink / raw)
  To: Will Deacon
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-01-28 14:35   ` Will Deacon
@ 2015-01-30 12:28     ` Joerg Roedel
  2015-02-02 16:45       ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Joerg Roedel @ 2015-01-30 12:28 UTC (permalink / raw)
  To: Will Deacon
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] iommu: Make sure a device is always attached to a domain
  2015-01-28 14:38   ` Will Deacon
@ 2015-01-30 12:29     ` Joerg Roedel
  0 siblings, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-01-30 12:29 UTC (permalink / raw)
  To: Will Deacon
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-01-30 12:28     ` Joerg Roedel
@ 2015-02-02 16:45       ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2015-02-02 16:45 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, Kukjin Kim, David Woodhouse, Heiko Stuebner, Hiroshi Doyu,
	Thierry Reding, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-01-27  0:08 ` [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group Joerg Roedel
  2015-01-28 14:35   ` Will Deacon
@ 2015-02-03 12:25   ` Thierry Reding
  2015-02-03 12:59     ` Joerg Roedel
  1 sibling, 1 reply; 16+ messages in thread
From: Thierry Reding @ 2015-02-03 12:25 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, Will Deacon, Kukjin Kim, David Woodhouse, Heiko Stuebner,
	Hiroshi Doyu, Alex Williamson, Arnd Bergmann, linux-kernel,
	Robin Murphy, Laurent Pinchart, jroedel

[-- 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 --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group
  2015-02-03 12:25   ` Thierry Reding
@ 2015-02-03 12:59     ` Joerg Roedel
  0 siblings, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2015-02-03 12:59 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Joerg Roedel, iommu, Will Deacon, Kukjin Kim, David Woodhouse,
	Heiko Stuebner, Hiroshi Doyu, Alex Williamson, Arnd Bergmann,
	linux-kernel, Robin Murphy, Laurent Pinchart

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-02-03 12:59 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27  0:08 [RFC PATCH 0/5] iommu: Introduce default domains for iommu groups Joerg Roedel
2015-01-27  0:08 ` [PATCH 1/5] iommu: Add default domain to iommu-groups Joerg Roedel
2015-01-27  0:08 ` [PATCH 2/5] iommu: Allocate a default domain for iommu groups Joerg Roedel
2015-01-28 14:30   ` Will Deacon
2015-01-28 15:11     ` Robin Murphy
2015-01-30 12:25     ` Joerg Roedel
2015-01-27  0:08 ` [PATCH 3/5] iommu: Limit iommu_attach/detach_device to devices with their own group Joerg Roedel
2015-01-28 14:35   ` Will Deacon
2015-01-30 12:28     ` Joerg Roedel
2015-02-02 16:45       ` Will Deacon
2015-02-03 12:25   ` Thierry Reding
2015-02-03 12:59     ` Joerg Roedel
2015-01-27  0:08 ` [PATCH 4/5] iommu: Make sure a device is always attached to a domain Joerg Roedel
2015-01-28 14:38   ` Will Deacon
2015-01-30 12:29     ` Joerg Roedel
2015-01-27  0:08 ` [PATCH 5/5] iommu: Add iommu_get_domain_for_dev function Joerg Roedel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).