[PATCH 0/2] iommu: fix handling of attach/detach for default domains

[PATCH 0/2] iommu: fix handling of attach/detach for default domains

[Marek Szyprowski]

Hello,

This patchset fixes attaching and detaching devices to a group, which
has default domain configured. With both patches applied all calls to
iommu's attach/detach functions will be balanced, whenever caller
attaches device to the default domain or the custom one.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (2):
  iommu: call detach also for default_domain before attaching to new one
  iommu: fix default domain handling in __iommu_detach_group()

 drivers/iommu/iommu.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

-- 
1.9.2

[PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Marek Szyprowski]

This patch ensures that devices attached to the default_domain will be
first detached from it before attaching to new domain. To avoid forward
declaration, __iommu_attach_group() function has been moved to new place
in the source code.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/iommu.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0e3b009..db231ad 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1198,22 +1198,6 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 	return __iommu_attach_device(domain, dev);
 }
 
-static int __iommu_attach_group(struct iommu_domain *domain,
-				struct iommu_group *group)
-{
-	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;
-}
-
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	int ret;
@@ -1235,6 +1219,28 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 	return 0;
 }
 
+static int __iommu_attach_group(struct iommu_domain *domain,
+				struct iommu_group *group)
+{
+	int ret;
+
+	if (group->default_domain && group->domain != group->default_domain)
+		return -EBUSY;
+
+	if (group->default_domain && group->default_domain == group->domain) {
+		__iommu_group_for_each_dev(group, group->domain,
+					   iommu_group_do_detach_device);
+		group->domain = NULL;
+	}
+
+	ret = __iommu_group_for_each_dev(group, domain,
+					 iommu_group_do_attach_device);
+	if (ret == 0)
+		group->domain = domain;
+
+	return ret;
+}
+
 static void __iommu_detach_group(struct iommu_domain *domain,
 				 struct iommu_group *group)
 {
-- 
1.9.2

Re: [PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Joerg Roedel]

On Tue, Feb 16, 2016 at 03:40:31PM +0100, Marek Szyprowski wrote:
> This patch ensures that devices attached to the default_domain will be
> first detached from it before attaching to new domain. To avoid forward
> declaration, __iommu_attach_group() function has been moved to new place
> in the source code.

Actually it was intentional to not invoke the detach_device call-back in
the attach_device path.

The reason is that detaching first and than attaching again leaves the
device without a domain for a short period of time, until it is attached
to the new domain.

The attach_device call-back is supposed to handle this situation and
just silently overwrite any other domain->device binding it finds for
the device.

This allows to do re-attachment with less iommu flushes and to get rid
of the detach_device call-back at some point.



	Joerg

Re: [PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Marek Szyprowski]

Hello,

On 2016-02-16 16:59, Joerg Roedel wrote:
> On Tue, Feb 16, 2016 at 03:40:31PM +0100, Marek Szyprowski wrote:
>> This patch ensures that devices attached to the default_domain will be
>> first detached from it before attaching to new domain. To avoid forward
>> declaration, __iommu_attach_group() function has been moved to new place
>> in the source code.
> Actually it was intentional to not invoke the detach_device call-back in
> the attach_device path.
>
> The reason is that detaching first and than attaching again leaves the
> device without a domain for a short period of time, until it is attached
> to the new domain.
>
> The attach_device call-back is supposed to handle this situation and
> just silently overwrite any other domain->device binding it finds for
> the device.
>
> This allows to do re-attachment with less iommu flushes and to get rid
> of the detach_device call-back at some point.

Huh, I wasn't aware of this change in the iommu drivers api. For some
drivers attach/detach callbacks does something more than just programming
page table base register, like for example in case of exynos iommu it is
enabling runtime power management and clocks. The code is really much 
simpler
if those calls are balanced, but if the goal is to allow multiple 
unballanced
attach calls, I will try to fix this in our driver.

Maybe it should be documented somewhere, that attach calls can be 
unbalanced?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Joerg Roedel]

On Wed, Feb 17, 2016 at 08:35:10AM +0100, Marek Szyprowski wrote:
> Huh, I wasn't aware of this change in the iommu drivers api. For some
> drivers attach/detach callbacks does something more than just programming
> page table base register, like for example in case of exynos iommu it is
> enabling runtime power management and clocks. The code is really
> much simpler
> if those calls are balanced, but if the goal is to allow multiple
> unballanced
> attach calls, I will try to fix this in our driver.
> 
> Maybe it should be documented somewhere, that attach calls can be
> unbalanced?

Well, when your driver uses default-domains, the detach_dev call-back is
not used anymore. The attach_dev call-back is supposed to just overwrite
any existing binding that may exist for the device. So the calls are not
unbalanced, the detach_dev calls just don't happen anymore.



	Joerg

Re: [PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Marek Szyprowski]

Hello,

On 2016-02-17 12:14, Joerg Roedel wrote:
> On Wed, Feb 17, 2016 at 08:35:10AM +0100, Marek Szyprowski wrote:
>> Huh, I wasn't aware of this change in the iommu drivers api. For some
>> drivers attach/detach callbacks does something more than just programming
>> page table base register, like for example in case of exynos iommu it is
>> enabling runtime power management and clocks. The code is really
>> much simpler
>> if those calls are balanced, but if the goal is to allow multiple
>> unballanced
>> attach calls, I will try to fix this in our driver.
>>
>> Maybe it should be documented somewhere, that attach calls can be
>> unbalanced?
> Well, when your driver uses default-domains, the detach_dev call-back is
> not used anymore. The attach_dev call-back is supposed to just overwrite
> any existing binding that may exist for the device. So the calls are not
> unbalanced, the detach_dev calls just don't happen anymore.

 From driver perspective the default_domains don't really differ from the
'other' domains. They are just allocated from the IOMMU core and used by
the IOMMU/DMA-mapping glue code. That's what I got from reading the code.

There should be also a way to detach the driver even from the default domain
to implement the arch_tear_down_dma_ops function.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] iommu: call detach also for default_domain before attaching to new one

[Joerg Roedel]

Hi Marek,

On Wed, Feb 17, 2016 at 03:42:54PM +0100, Marek Szyprowski wrote:
> From driver perspective the default_domains don't really differ from the
> 'other' domains. They are just allocated from the IOMMU core and used by
> the IOMMU/DMA-mapping glue code. That's what I got from reading the code.
> 
> There should be also a way to detach the driver even from the default domain
> to implement the arch_tear_down_dma_ops function.

The attaching/detaching in the iommu-core only manages software state.
The iommu-driver can power-down the iommu device when the default-domain
is attached and it does not expect any map/unmap calls.

All it has to make sure is that it wakes up the iommu device again when
or before these calls are made again.


	Joerg

[PATCH 2/2] iommu: fix default domain handling in __iommu_detach_group()

[Marek Szyprowski]

This patch ensures that all devices will be first detached from the
provided domain and then attached to the default_domain if such has been
provided.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index db231ad..045efb0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1246,14 +1246,14 @@ static void __iommu_detach_group(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (!group->default_domain) {
-		__iommu_group_for_each_dev(group, domain,
-					   iommu_group_do_detach_device);
-		group->domain = NULL;
+	if (group->domain == group->default_domain)
 		return;
-	}
 
-	if (group->domain == group->default_domain)
+	__iommu_group_for_each_dev(group, domain,
+				   iommu_group_do_detach_device);
+	group->domain = NULL;
+
+	if (!group->default_domain)
 		return;
 
 	/* Detach by re-attaching to the default domain */
-- 
1.9.2