[PATCH] iommu: Implement deferred domain attachment

[PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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 <jsnitsel@redhat.com>
Cc: Tom Murphy <murphyt7@tcd.ie>
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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

[PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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 <jsnitsel@redhat.com>
Cc: Tom Murphy <murphyt7@tcd.ie>
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Lu Baolu]

Hi Joerg,

On 2020/5/15 17:45, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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

Re: [PATCH] iommu: Implement deferred domain attachment

[Lu Baolu]

Hi Joerg,

On 2020/5/15 17:45, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

On 2020-05-15 10:45, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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;
>   }
>   
>   /*
> 

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

On 2020-05-15 10:45, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-iommu api")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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;
>   }
>   
>   /*
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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 <jroedel@suse.de>
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 <jsnitsel@redhat.com>
Cc: Tom Murphy <murphyt7@tcd.ie>
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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 <jroedel@suse.de>
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 <jsnitsel@redhat.com>
Cc: Tom Murphy <murphyt7@tcd.ie>
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

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 <jroedel@suse.de>
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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;

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

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 <jroedel@suse.de>
> 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 <jsnitsel@redhat.com>
> Cc: Tom Murphy <murphyt7@tcd.ie>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   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;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

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.

Re: [PATCH] iommu: Implement deferred domain attachment

[Robin Murphy]

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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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))

Re: [PATCH] iommu: Implement deferred domain attachment

[Joerg Roedel]

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Jerry Snitselaar]

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.

Re: [PATCH] iommu: Implement deferred domain attachment

[Jerry Snitselaar]

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.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Implement deferred domain attachment

[Jerry Snitselaar]

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
>iommu@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

This worked for me as well.

Re: [PATCH] iommu: Implement deferred domain attachment

[Jerry Snitselaar]

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
>iommu@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

This worked for me as well.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu