From: Joerg Roedel <[email protected]>
The IOMMU core code has support for deferring the attachment of a domain
to a device. This is needed in kdump kernels where the new domain must
not be attached to a device before the device driver takes it over.
But this needs support from the dma-ops code too, to actually do the
late attachment when there are DMA-API calls for the device. This got
lost in the AMD IOMMU driver after converting it to the dma-iommu code.
Do the late attachment in the dma-iommu code-path to fix the issue.
Cc: Jerry Snitselaar <[email protected]>
Cc: Tom Murphy <[email protected]>
Reported-by: Jerry Snitselaar <[email protected]>
Tested-by: Jerry Snitselaar <[email protected]>
Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/iommu.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4050569188be..f54ebb964271 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1889,13 +1889,19 @@ void iommu_domain_free(struct iommu_domain *domain)
}
EXPORT_SYMBOL_GPL(iommu_domain_free);
-static int __iommu_attach_device(struct iommu_domain *domain,
- struct device *dev)
+static bool __iommu_is_attach_deferred(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (!domain->ops->is_attach_deferred)
+ return false;
+
+ return domain->ops->is_attach_deferred(domain, dev);
+}
+
+static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
@@ -1903,9 +1909,19 @@ static int __iommu_attach_device(struct iommu_domain *domain,
ret = domain->ops->attach_dev(domain, dev);
if (!ret)
trace_attach_device_to_domain(dev);
+
return ret;
}
+static int __iommu_attach_device(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (__iommu_is_attach_deferred(domain, dev))
+ return 0;
+
+ return __iommu_attach_device_no_defer(domain, dev);
+}
+
int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
{
struct iommu_group *group;
@@ -2023,7 +2039,12 @@ EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
*/
struct iommu_domain *iommu_get_dma_domain(struct device *dev)
{
- return dev->iommu_group->default_domain;
+ struct iommu_domain *domain = dev->iommu_group->default_domain;
+
+ if (__iommu_is_attach_deferred(domain, dev))
+ __iommu_attach_device_no_defer(domain, dev);
+
+ return domain;
}
/*
--
2.25.1
Hi Joerg,
On 2020/5/15 17:45, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> The IOMMU core code has support for deferring the attachment of a domain
> to a device. This is needed in kdump kernels where the new domain must
> not be attached to a device before the device driver takes it over.
>
> But this needs support from the dma-ops code too, to actually do the
> late attachment when there are DMA-API calls for the device. This got
> lost in the AMD IOMMU driver after converting it to the dma-iommu code.
>
> Do the late attachment in the dma-iommu code-path to fix the issue.
>
> Cc: Jerry Snitselaar <[email protected]>
> Cc: Tom Murphy <[email protected]>
> Reported-by: Jerry Snitselaar <[email protected]>
> Tested-by: Jerry Snitselaar <[email protected]>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/iommu.c | 33 +++++++++++++++++++++++++++------
> 1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4050569188be..f54ebb964271 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1889,13 +1889,19 @@ void iommu_domain_free(struct iommu_domain *domain)
> }
> EXPORT_SYMBOL_GPL(iommu_domain_free);
>
> -static int __iommu_attach_device(struct iommu_domain *domain,
> - struct device *dev)
> +static bool __iommu_is_attach_deferred(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + if (!domain->ops->is_attach_deferred)
> + return false;
> +
> + return domain->ops->is_attach_deferred(domain, dev);
> +}
> +
> +static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
> + struct device *dev)
> {
> int ret;
> - if ((domain->ops->is_attach_deferred != NULL) &&
> - domain->ops->is_attach_deferred(domain, dev))
> - return 0;
>
> if (unlikely(domain->ops->attach_dev == NULL))
> return -ENODEV;
> @@ -1903,9 +1909,19 @@ static int __iommu_attach_device(struct iommu_domain *domain,
> ret = domain->ops->attach_dev(domain, dev);
> if (!ret)
> trace_attach_device_to_domain(dev);
> +
> return ret;
> }
>
> +static int __iommu_attach_device(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + if (__iommu_is_attach_deferred(domain, dev))
> + return 0;
> +
> + return __iommu_attach_device_no_defer(domain, dev);
> +}
> +
> int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> {
> struct iommu_group *group;
> @@ -2023,7 +2039,12 @@ EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
> */
> struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> {
> - return dev->iommu_group->default_domain;
> + struct iommu_domain *domain = dev->iommu_group->default_domain;
> +
> + if (__iommu_is_attach_deferred(domain, dev))
> + __iommu_attach_device_no_defer(domain, dev);
It seems that the return value needs to be checked. The default domain
is invalid if attach() failed.
Best regards,
baolu
On Fri, May 15, 2020 at 09:51:03PM +0800, Lu Baolu wrote:
> On 2020/5/15 17:45, Joerg Roedel wrote:
> > struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> > {
> > - return dev->iommu_group->default_domain;
> > + struct iommu_domain *domain = dev->iommu_group->default_domain;
> > +
> > + if (__iommu_is_attach_deferred(domain, dev))
> > + __iommu_attach_device_no_defer(domain, dev);
>
> It seems that the return value needs to be checked. The default domain
> is invalid if attach() failed.
True, I looked at that, the callers can't handle returning NULL here, so
I kept it this way for now. The outcome is that DMA will fail, but
otherwise we'd see a NULL-ptr dereference really quickly after returning
from that function.
Bottom line: This needs to be cleaned up separatly.
Regards,
Joerg
On 2020-05-15 10:45, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> The IOMMU core code has support for deferring the attachment of a domain
> to a device. This is needed in kdump kernels where the new domain must
> not be attached to a device before the device driver takes it over.
>
> But this needs support from the dma-ops code too, to actually do the
> late attachment when there are DMA-API calls for the device. This got
> lost in the AMD IOMMU driver after converting it to the dma-iommu code.
>
> Do the late attachment in the dma-iommu code-path to fix the issue.
>
> Cc: Jerry Snitselaar <[email protected]>
> Cc: Tom Murphy <[email protected]>
> Reported-by: Jerry Snitselaar <[email protected]>
> Tested-by: Jerry Snitselaar <[email protected]>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/iommu.c | 33 +++++++++++++++++++++++++++------
> 1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4050569188be..f54ebb964271 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1889,13 +1889,19 @@ void iommu_domain_free(struct iommu_domain *domain)
> }
> EXPORT_SYMBOL_GPL(iommu_domain_free);
>
> -static int __iommu_attach_device(struct iommu_domain *domain,
> - struct device *dev)
> +static bool __iommu_is_attach_deferred(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + if (!domain->ops->is_attach_deferred)
> + return false;
> +
> + return domain->ops->is_attach_deferred(domain, dev);
> +}
> +
> +static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
> + struct device *dev)
> {
> int ret;
> - if ((domain->ops->is_attach_deferred != NULL) &&
> - domain->ops->is_attach_deferred(domain, dev))
> - return 0;
>
> if (unlikely(domain->ops->attach_dev == NULL))
> return -ENODEV;
> @@ -1903,9 +1909,19 @@ static int __iommu_attach_device(struct iommu_domain *domain,
> ret = domain->ops->attach_dev(domain, dev);
> if (!ret)
> trace_attach_device_to_domain(dev);
> +
> return ret;
> }
>
> +static int __iommu_attach_device(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + if (__iommu_is_attach_deferred(domain, dev))
> + return 0;
> +
> + return __iommu_attach_device_no_defer(domain, dev);
> +}
> +
> int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> {
> struct iommu_group *group;
> @@ -2023,7 +2039,12 @@ EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
> */
> struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> {
> - return dev->iommu_group->default_domain;
> + struct iommu_domain *domain = dev->iommu_group->default_domain;
> +
> + if (__iommu_is_attach_deferred(domain, dev))
> + __iommu_attach_device_no_defer(domain, dev);
This raises a red flag, since iommu-dma already has explicit deferred
attach handling where it should need it, immediately after this is
called to retrieve the domain. The whole thing smells to me like we
should have an explicit special-case in iommu_probe_device() rather than
hooking __iommu_attach_device() in general then having to bodge around
the fallout elsewhere.
Robin.
> +
> + return domain;
> }
>
> /*
>
On Fri, May 15, 2020 at 04:42:23PM +0100, Robin Murphy wrote:
> > struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> > {
> > - return dev->iommu_group->default_domain;
> > + struct iommu_domain *domain = dev->iommu_group->default_domain;
> > +
> > + if (__iommu_is_attach_deferred(domain, dev))
> > + __iommu_attach_device_no_defer(domain, dev);
>
> This raises a red flag, since iommu-dma already has explicit deferred attach
> handling where it should need it, immediately after this is called to
> retrieve the domain. The whole thing smells to me like we should have an
> explicit special-case in iommu_probe_device() rather than hooking
> __iommu_attach_device() in general then having to bodge around the fallout
> elsewhere.
Good point, I missed that. But it didn't work for its only user, the
AMD IOMMU driver, the reason is that it calls iommu_attach_device(),
which in its code-path checks for deferred attaching again and bails
out, without do the real attachment.
But below updated fix should work. Jerry, could you please test it
again?
From 4e262dedcd36c7572312c65e66416da74fc78047 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Fri, 15 May 2020 11:25:03 +0200
Subject: [PATCH] iommu: Fix deferred domain attachment
The IOMMU core code has support for deferring the attachment of a domain
to a device. This is needed in kdump kernels where the new domain must
not be attached to a device before the device driver takes it over.
When the AMD IOMMU driver got converted to use the dma-iommu
implementation, the deferred attaching got lost. The code in
dma-iommu.c has support for deferred attaching, but it calls into
iommu_attach_device() to actually do it. But iommu_attach_device()
will check if the device should be deferred in it code-path and do
nothing, breaking deferred attachment.
Provide a function in IOMMU core code to reliably attach a device to a
domain without any deferred checks and also without other safe-guards.
Cc: Jerry Snitselaar <[email protected]>
Cc: Tom Murphy <[email protected]>
Reported-by: Jerry Snitselaar <[email protected]>
Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/dma-iommu.c | 4 ++--
drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++++++++++-----
include/linux/iommu.h | 2 ++
3 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index ba128d1cdaee..403fda04ea98 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -362,8 +362,8 @@ static int iommu_dma_deferred_attach(struct device *dev,
return 0;
if (unlikely(ops->is_attach_deferred &&
- ops->is_attach_deferred(domain, dev)))
- return iommu_attach_device(domain, dev);
+ ops->is_attach_deferred(domain, dev)))
+ return iommu_attach_device_no_defer(domain, dev);
return 0;
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4050569188be..91dbdbc6d640 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -23,6 +23,7 @@
#include <linux/property.h>
#include <linux/fsl/mc.h>
#include <linux/module.h>
+#include <linux/crash_dump.h>
#include <trace/events/iommu.h>
static struct kset *iommu_group_kset;
@@ -1889,13 +1890,19 @@ void iommu_domain_free(struct iommu_domain *domain)
}
EXPORT_SYMBOL_GPL(iommu_domain_free);
-static int __iommu_attach_device(struct iommu_domain *domain,
- struct device *dev)
+static bool __iommu_is_attach_deferred(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (!domain->ops->is_attach_deferred)
+ return false;
+
+ return domain->ops->is_attach_deferred(domain, dev);
+}
+
+static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
@@ -1903,9 +1910,29 @@ static int __iommu_attach_device(struct iommu_domain *domain,
ret = domain->ops->attach_dev(domain, dev);
if (!ret)
trace_attach_device_to_domain(dev);
+
return ret;
}
+static int __iommu_attach_device(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (__iommu_is_attach_deferred(domain, dev))
+ return 0;
+
+ return __iommu_attach_device_no_defer(domain, dev);
+}
+
+int iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev)
+{
+ /* Safe-Guard to only call this when needed */
+ if (!is_kdump_kernel())
+ return -ENODEV;
+
+ return __iommu_attach_device_no_defer(domain, dev);
+}
+
int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
{
struct iommu_group *group;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7cfd2dddb49d..f82b20a61d0b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -449,6 +449,8 @@ extern struct iommu_group *iommu_group_get_by_id(int id);
extern void iommu_domain_free(struct iommu_domain *domain);
extern int iommu_attach_device(struct iommu_domain *domain,
struct device *dev);
+extern int iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev);
extern void iommu_detach_device(struct iommu_domain *domain,
struct device *dev);
extern int iommu_cache_invalidate(struct iommu_domain *domain,
--
2.25.1
On 2020-05-15 17:14, Joerg Roedel wrote:
> On Fri, May 15, 2020 at 04:42:23PM +0100, Robin Murphy wrote:
>>> struct iommu_domain *iommu_get_dma_domain(struct device *dev)
>>> {
>>> - return dev->iommu_group->default_domain;
>>> + struct iommu_domain *domain = dev->iommu_group->default_domain;
>>> +
>>> + if (__iommu_is_attach_deferred(domain, dev))
>>> + __iommu_attach_device_no_defer(domain, dev);
>>
>> This raises a red flag, since iommu-dma already has explicit deferred attach
>> handling where it should need it, immediately after this is called to
>> retrieve the domain. The whole thing smells to me like we should have an
>> explicit special-case in iommu_probe_device() rather than hooking
>> __iommu_attach_device() in general then having to bodge around the fallout
>> elsewhere.
>
> Good point, I missed that. But it didn't work for its only user, the
> AMD IOMMU driver, the reason is that it calls iommu_attach_device(),
> which in its code-path checks for deferred attaching again and bails
> out, without do the real attachment.
>
> But below updated fix should work. Jerry, could you please test it
> again?
>
> From 4e262dedcd36c7572312c65e66416da74fc78047 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <[email protected]>
> Date: Fri, 15 May 2020 11:25:03 +0200
> Subject: [PATCH] iommu: Fix deferred domain attachment
>
> The IOMMU core code has support for deferring the attachment of a domain
> to a device. This is needed in kdump kernels where the new domain must
> not be attached to a device before the device driver takes it over.
>
> When the AMD IOMMU driver got converted to use the dma-iommu
> implementation, the deferred attaching got lost. The code in
> dma-iommu.c has support for deferred attaching, but it calls into
> iommu_attach_device() to actually do it. But iommu_attach_device()
> will check if the device should be deferred in it code-path and do
> nothing, breaking deferred attachment.
>
> Provide a function in IOMMU core code to reliably attach a device to a
> domain without any deferred checks and also without other safe-guards.
>
> Cc: Jerry Snitselaar <[email protected]>
> Cc: Tom Murphy <[email protected]>
> Reported-by: Jerry Snitselaar <[email protected]>
> Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 4 ++--
> drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++++++++++-----
> include/linux/iommu.h | 2 ++
> 3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index ba128d1cdaee..403fda04ea98 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -362,8 +362,8 @@ static int iommu_dma_deferred_attach(struct device *dev,
> return 0;
>
> if (unlikely(ops->is_attach_deferred &&
> - ops->is_attach_deferred(domain, dev)))
> - return iommu_attach_device(domain, dev);
> + ops->is_attach_deferred(domain, dev)))
> + return iommu_attach_device_no_defer(domain, dev);
Wouldn't it be simpler to just invoke ops->attach_dev directly and avoid
having to formalise a public interface that nobody else should ever use
anyway?
That said, unless I've entirely misunderstood the situation I still
think that something like the below makes more sense (apologies for
broken whitespace).
Robin.
----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2b471419e26c..1a52e530774c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -704,6 +704,7 @@ int iommu_group_add_device(struct iommu_group
*group, struct device *dev)
{
int ret, i = 0;
struct group_device *device;
+ struct iommu_domain *domain;
device = kzalloc(sizeof(*device), GFP_KERNEL);
if (!device)
@@ -746,8 +747,11 @@ int iommu_group_add_device(struct iommu_group
*group, struct device *dev)
mutex_lock(&group->mutex);
list_add_tail(&device->list, &group->devices);
- if (group->domain)
- ret = __iommu_attach_device(group->domain, dev);
+ domain = group->domain;
+ if (domain && (!domain->ops->is_attach_deferred ||
+ !domain->ops->is_attach_deferred(domain, dev)))
+ ret = __iommu_attach_device(domain, dev);
+ }
mutex_unlock(&group->mutex);
if (ret)
goto err_put_group;
@@ -1652,9 +1656,6 @@ static int __iommu_attach_device(struct
iommu_domain *domain,
struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
On Fri, May 15, 2020 at 05:28:53PM +0100, Robin Murphy wrote:
> On 2020-05-15 17:14, Joerg Roedel wrote:
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index ba128d1cdaee..403fda04ea98 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -362,8 +362,8 @@ static int iommu_dma_deferred_attach(struct device *dev,
> > return 0;
> > if (unlikely(ops->is_attach_deferred &&
> > - ops->is_attach_deferred(domain, dev)))
> > - return iommu_attach_device(domain, dev);
> > + ops->is_attach_deferred(domain, dev)))
> > + return iommu_attach_device_no_defer(domain, dev);
>
> Wouldn't it be simpler to just invoke ops->attach_dev directly and avoid
> having to formalise a public interface that nobody else should ever use
> anyway?
That would omit the ops->attach_dev != NULL check and the trace-point on
device attach. Besides that, it would be a layering violation. But the
function is of course entirely internal to the iommu subsytem and is a
good canditate to be moved to a header file in drivers/iommu.
> @@ -746,8 +747,11 @@ int iommu_group_add_device(struct iommu_group *group,
> struct device *dev)
>
> mutex_lock(&group->mutex);
> list_add_tail(&device->list, &group->devices);
> - if (group->domain)
> - ret = __iommu_attach_device(group->domain, dev);
> + domain = group->domain;
> + if (domain && (!domain->ops->is_attach_deferred ||
> + !domain->ops->is_attach_deferred(domain, dev)))
> + ret = __iommu_attach_device(domain, dev);
> + }
> mutex_unlock(&group->mutex);
> if (ret)
> goto err_put_group;
No, doing this in iommu_group_add_device() doesn't solve the problem.
The attach must not happen before a device driver took control of the
device and silenced any DMA initiated by the old kernel. At probe time
this isn't guaranteed.
Joerg
On 2020-05-15 19:26, Joerg Roedel wrote:
> On Fri, May 15, 2020 at 05:28:53PM +0100, Robin Murphy wrote:
>> On 2020-05-15 17:14, Joerg Roedel wrote:
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index ba128d1cdaee..403fda04ea98 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -362,8 +362,8 @@ static int iommu_dma_deferred_attach(struct device *dev,
>>> return 0;
>>> if (unlikely(ops->is_attach_deferred &&
>>> - ops->is_attach_deferred(domain, dev)))
>>> - return iommu_attach_device(domain, dev);
>>> + ops->is_attach_deferred(domain, dev)))
>>> + return iommu_attach_device_no_defer(domain, dev);
>>
>> Wouldn't it be simpler to just invoke ops->attach_dev directly and avoid
>> having to formalise a public interface that nobody else should ever use
>> anyway?
>
> That would omit the ops->attach_dev != NULL check and the trace-point on
> device attach. Besides that, it would be a layering violation. But the
> function is of course entirely internal to the iommu subsytem and is a
> good canditate to be moved to a header file in drivers/iommu.
Sure, checking the pointer before calling was implied, but the
tracepoint is a good argument, I'd forgotten about that :)
>> @@ -746,8 +747,11 @@ int iommu_group_add_device(struct iommu_group *group,
>> struct device *dev)
>>
>> mutex_lock(&group->mutex);
>> list_add_tail(&device->list, &group->devices);
>> - if (group->domain)
>> - ret = __iommu_attach_device(group->domain, dev);
>> + domain = group->domain;
>> + if (domain && (!domain->ops->is_attach_deferred ||
>> + !domain->ops->is_attach_deferred(domain, dev)))
>> + ret = __iommu_attach_device(domain, dev);
>> + }
>> mutex_unlock(&group->mutex);
>> if (ret)
>> goto err_put_group;
>
> No, doing this in iommu_group_add_device() doesn't solve the problem.
> The attach must not happen before a device driver took control of the
> device and silenced any DMA initiated by the old kernel. At probe time
> this isn't guaranteed.
But that's not what this is; this is (supposed to be) the exact same
"don't actually perform the attach yet" logic as before, just
restricting it to default domains in the one place that it actually
needs to be, so as not to fundamentally bugger up iommu_attach_device()
in a way that prevents it from working as expected at the correct point
later.
Thinking a bit more, consider if the driver resets the device then
attaches it straight to its own unmanaged domain rather than calling any
DMA ops (e.g. VFIO?) - it looks like that would also be totally broken
right now, and no amount of bodges in iommu-dma is going to help there.
Robin.
On Fri, May 15, 2020 at 08:23:13PM +0100, Robin Murphy wrote:
> But that's not what this is; this is (supposed to be) the exact same "don't
> actually perform the attach yet" logic as before, just restricting it to
> default domains in the one place that it actually needs to be, so as not to
> fundamentally bugger up iommu_attach_device() in a way that prevents it from
> working as expected at the correct point later.
You are right, that is better. I tested it and it seems to work. Updated
diff attached, with a minor cleanup included. Mind sending it as a
proper patch I can send upstream?
Thanks,
Joerg
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7b375421afba..a9d02bc3ab5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -693,6 +693,15 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
return ret;
}
+static bool iommu_is_attach_deferred(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (domain->ops->is_attach_deferred)
+ return domain->ops->is_attach_deferred(domain, dev);
+
+ return false;
+}
+
/**
* iommu_group_add_device - add a device to an iommu group
* @group: the group into which to add the device (reference should be held)
@@ -705,6 +714,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
{
int ret, i = 0;
struct group_device *device;
+ struct iommu_domain *domain;
device = kzalloc(sizeof(*device), GFP_KERNEL);
if (!device)
@@ -747,7 +757,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
mutex_lock(&group->mutex);
list_add_tail(&device->list, &group->devices);
- if (group->domain)
+ domain = group->domain;
+ if (domain && !iommu_is_attach_deferred(domain, dev))
ret = __iommu_attach_device(group->domain, dev);
mutex_unlock(&group->mutex);
if (ret)
@@ -1653,9 +1664,6 @@ static int __iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
@@ -1727,8 +1735,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
static void __iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
+ if (iommu_is_attach_deferred(domain, dev))
return;
if (unlikely(domain->ops->detach_dev == NULL))
On Mon May 18 20, Joerg Roedel wrote:
>On Fri, May 15, 2020 at 08:23:13PM +0100, Robin Murphy wrote:
>> But that's not what this is; this is (supposed to be) the exact same "don't
>> actually perform the attach yet" logic as before, just restricting it to
>> default domains in the one place that it actually needs to be, so as not to
>> fundamentally bugger up iommu_attach_device() in a way that prevents it from
>> working as expected at the correct point later.
>
>You are right, that is better. I tested it and it seems to work. Updated
>diff attached, with a minor cleanup included. Mind sending it as a
>proper patch I can send upstream?
>
>Thanks,
>
> Joerg
>
I should have this tested this afternoon.
On Mon May 18 20, Joerg Roedel wrote:
>On Fri, May 15, 2020 at 08:23:13PM +0100, Robin Murphy wrote:
>> But that's not what this is; this is (supposed to be) the exact same "don't
>> actually perform the attach yet" logic as before, just restricting it to
>> default domains in the one place that it actually needs to be, so as not to
>> fundamentally bugger up iommu_attach_device() in a way that prevents it from
>> working as expected at the correct point later.
>
>You are right, that is better. I tested it and it seems to work. Updated
>diff attached, with a minor cleanup included. Mind sending it as a
>proper patch I can send upstream?
>
>Thanks,
>
> Joerg
>
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index 7b375421afba..a9d02bc3ab5b 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -693,6 +693,15 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
> return ret;
> }
>
>+static bool iommu_is_attach_deferred(struct iommu_domain *domain,
>+ struct device *dev)
>+{
>+ if (domain->ops->is_attach_deferred)
>+ return domain->ops->is_attach_deferred(domain, dev);
>+
>+ return false;
>+}
>+
> /**
> * iommu_group_add_device - add a device to an iommu group
> * @group: the group into which to add the device (reference should be held)
>@@ -705,6 +714,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
> {
> int ret, i = 0;
> struct group_device *device;
>+ struct iommu_domain *domain;
>
> device = kzalloc(sizeof(*device), GFP_KERNEL);
> if (!device)
>@@ -747,7 +757,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>
> mutex_lock(&group->mutex);
> list_add_tail(&device->list, &group->devices);
>- if (group->domain)
>+ domain = group->domain;
>+ if (domain && !iommu_is_attach_deferred(domain, dev))
> ret = __iommu_attach_device(group->domain, dev);
> mutex_unlock(&group->mutex);
> if (ret)
>@@ -1653,9 +1664,6 @@ static int __iommu_attach_device(struct iommu_domain *domain,
> struct device *dev)
> {
> int ret;
>- if ((domain->ops->is_attach_deferred != NULL) &&
>- domain->ops->is_attach_deferred(domain, dev))
>- return 0;
>
> if (unlikely(domain->ops->attach_dev == NULL))
> return -ENODEV;
>@@ -1727,8 +1735,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
> static void __iommu_detach_device(struct iommu_domain *domain,
> struct device *dev)
> {
>- if ((domain->ops->is_attach_deferred != NULL) &&
>- domain->ops->is_attach_deferred(domain, dev))
>+ if (iommu_is_attach_deferred(domain, dev))
> return;
>
> if (unlikely(domain->ops->detach_dev == NULL))
>_______________________________________________
>iommu mailing list
>[email protected]
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
This worked for me as well.