[PATCH 0/5] iommu/vt-d: Fix kdump boot with VT-d enabled

[PATCH 0/5] iommu/vt-d: Fix kdump boot with VT-d enabled

[Joerg Roedel]

Hi,

booting into a crashdump kernel with Intel IOMMU enabled and
configured into passthrough mode does not succeed with the current
kernel. The reason is that the check for identity mappings happen
before the check for deferred device attachments. That results in
wrong results returned from iommu_need_mapping() and subsequently in a
wrong domain-type used in __intel_map_single(). A stripped oops is in
the commit-message of patch 3.

The patch-set fixes the issue and does a few code cleanups along the
way. I have not yet researched the stable and fixes tags, but when the
patches are fine I will add the tags before applying the patches.

Please review.

Thanks,

	Joerg

Joerg Roedel (5):
  iommu/vt-d: Add attach_deferred() helper
  iommu/vt-d: Move deferred device attachment into helper function
  iommu/vt-d: Do deferred attachment in iommu_need_mapping()
  iommu/vt-d: Remove deferred_attach_domain()
  iommu/vt-d: Simplify check in identity_mapping()

 drivers/iommu/intel-iommu.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/5] iommu/vt-d: Add attach_deferred() helper

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Implement a helper function to check whether a device's attach process
is deferred.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9dc37672bf89..80f2332a5466 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -762,6 +762,11 @@ static int iommu_dummy(struct device *dev)
 	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
 }
 
+static bool attach_deferred(struct device *dev)
+{
+	return dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO;
+}
+
 /**
  * is_downstream_to_pci_bridge - test if a device belongs to the PCI
  *				 sub-hierarchy of a candidate PCI-PCI bridge
@@ -2510,8 +2515,7 @@ struct dmar_domain *find_domain(struct device *dev)
 {
 	struct device_domain_info *info;
 
-	if (unlikely(dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO ||
-		     dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO))
+	if (unlikely(attach_deferred(dev) || iommu_dummy(dev)))
 		return NULL;
 
 	if (dev_is_pci(dev))
@@ -2527,7 +2531,7 @@ struct dmar_domain *find_domain(struct device *dev)
 
 static struct dmar_domain *deferred_attach_domain(struct device *dev)
 {
-	if (unlikely(dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO)) {
+	if (unlikely(attach_deferred(dev))) {
 		struct iommu_domain *domain;
 
 		dev->archdata.iommu = NULL;
@@ -6133,7 +6137,7 @@ intel_iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 static bool intel_iommu_is_attach_deferred(struct iommu_domain *domain,
 					   struct device *dev)
 {
-	return dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO;
+	return attach_deferred(dev);
 }
 
 static int
-- 
2.17.1

[PATCH 2/5] iommu/vt-d: Move deferred device attachment into helper function

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Move the code that does the deferred device attachment into a separate
helper function.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 80f2332a5466..42cdcce1602e 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2529,16 +2529,20 @@ struct dmar_domain *find_domain(struct device *dev)
 	return NULL;
 }
 
-static struct dmar_domain *deferred_attach_domain(struct device *dev)
+static void do_deferred_attach(struct device *dev)
 {
-	if (unlikely(attach_deferred(dev))) {
-		struct iommu_domain *domain;
+	struct iommu_domain *domain;
 
-		dev->archdata.iommu = NULL;
-		domain = iommu_get_domain_for_dev(dev);
-		if (domain)
-			intel_iommu_attach_device(domain, dev);
-	}
+	dev->archdata.iommu = NULL;
+	domain = iommu_get_domain_for_dev(dev);
+	if (domain)
+		intel_iommu_attach_device(domain, dev);
+}
+
+static struct dmar_domain *deferred_attach_domain(struct device *dev)
+{
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
 
 	return find_domain(dev);
 }
-- 
2.17.1

[PATCH 3/5] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The attachment of deferred devices needs to happen before the check
whether the device is identity mapped or not. Otherwise the check will
return wrong results, cause warnings boot failures in kdump kernels, like

	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70

	[...]

	 Call Trace:
	  __intel_map_single+0x55/0x190
	  intel_alloc_coherent+0xac/0x110
	  dmam_alloc_attrs+0x50/0xa0
	  ahci_port_start+0xfb/0x1f0 [libahci]
	  ata_host_start.part.39+0x104/0x1e0 [libata]

With the earlier check the kdump boot succeeds and a crashdump is written.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 42cdcce1602e..32f43695a22b 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
 
 static struct dmar_domain *deferred_attach_domain(struct device *dev)
 {
-	if (unlikely(attach_deferred(dev)))
-		do_deferred_attach(dev);
-
 	return find_domain(dev);
 }
 
@@ -3595,6 +3592,9 @@ static bool iommu_need_mapping(struct device *dev)
 	if (iommu_dummy(dev))
 		return false;
 
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
+
 	ret = identity_mapping(dev);
 	if (ret) {
 		u64 dma_mask = *dev->dma_mask;
-- 
2.17.1

[PATCH 4/5] iommu/vt-d: Remove deferred_attach_domain()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The function is now only a wrapper around find_domain(). Remove the
function and call find_domain() directly at the call-sites.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 32f43695a22b..16e47ca505eb 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2539,11 +2539,6 @@ static void do_deferred_attach(struct device *dev)
 		intel_iommu_attach_device(domain, dev);
 }
 
-static struct dmar_domain *deferred_attach_domain(struct device *dev)
-{
-	return find_domain(dev);
-}
-
 static inline struct device_domain_info *
 dmar_search_domain_by_dev_info(int segment, int bus, int devfn)
 {
@@ -3643,7 +3638,7 @@ static dma_addr_t __intel_map_single(struct device *dev, phys_addr_t paddr,
 
 	BUG_ON(dir == DMA_NONE);
 
-	domain = deferred_attach_domain(dev);
+	domain = find_domain(dev);
 	if (!domain)
 		return DMA_MAPPING_ERROR;
 
@@ -3863,7 +3858,7 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
 	if (!iommu_need_mapping(dev))
 		return dma_direct_map_sg(dev, sglist, nelems, dir, attrs);
 
-	domain = deferred_attach_domain(dev);
+	domain = find_domain(dev);
 	if (!domain)
 		return 0;
 
@@ -3958,7 +3953,7 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size,
 	int prot = 0;
 	int ret;
 
-	domain = deferred_attach_domain(dev);
+	domain = find_domain(dev);
 	if (WARN_ON(dir == DMA_NONE || !domain))
 		return DMA_MAPPING_ERROR;
 
-- 
2.17.1

[PATCH 5/5] iommu/vt-d: Simplify check in identity_mapping()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The function only has one call-site and there it is never called with
dummy or deferred devices. Simplify the check in the function to
account for that.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 16e47ca505eb..0b5a0fadbc0c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2916,7 +2916,7 @@ static int identity_mapping(struct device *dev)
 	struct device_domain_info *info;
 
 	info = dev->archdata.iommu;
-	if (info && info != DUMMY_DEVICE_DOMAIN_INFO && info != DEFER_DEVICE_DOMAIN_INFO)
+	if (info)
 		return (info->domain == si_domain);
 
 	return 0;
-- 
2.17.1

Re: [PATCH 1/5] iommu/vt-d: Add attach_deferred() helper

[Jerry Snitselaar]

On Mon Feb 17 20, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>Implement a helper function to check whether a device's attach process
>is deferred.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 2/5] iommu/vt-d: Move deferred device attachment into helper function

[Jerry Snitselaar]

On Mon Feb 17 20, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>Move the code that does the deferred device attachment into a separate
>helper function.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 3/5] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Jerry Snitselaar]

On Mon Feb 17 20, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>The attachment of deferred devices needs to happen before the check
>whether the device is identity mapped or not. Otherwise the check will
>return wrong results, cause warnings boot failures in kdump kernels, like
>
>	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70
>
>	[...]
>
>	 Call Trace:
>	  __intel_map_single+0x55/0x190
>	  intel_alloc_coherent+0xac/0x110
>	  dmam_alloc_attrs+0x50/0xa0
>	  ahci_port_start+0xfb/0x1f0 [libahci]
>	  ata_host_start.part.39+0x104/0x1e0 [libata]
>
>With the earlier check the kdump boot succeeds and a crashdump is written.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 4/5] iommu/vt-d: Remove deferred_attach_domain()

[Jerry Snitselaar]

On Mon Feb 17 20, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>The function is now only a wrapper around find_domain(). Remove the
>function and call find_domain() directly at the call-sites.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 5/5] iommu/vt-d: Simplify check in identity_mapping()

[Jerry Snitselaar]

On Mon Feb 17 20, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>The function only has one call-site and there it is never called with
>dummy or deferred devices. Simplify the check in the function to
>account for that.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 3/5] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Lu Baolu]

Hi Joerg,

Thanks for doing this.

On 2020/2/18 3:38, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The attachment of deferred devices needs to happen before the check
> whether the device is identity mapped or not. Otherwise the check will
> return wrong results, cause warnings boot failures in kdump kernels, like
> 
> 	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70
> 
> 	[...]
> 
> 	 Call Trace:
> 	  __intel_map_single+0x55/0x190
> 	  intel_alloc_coherent+0xac/0x110
> 	  dmam_alloc_attrs+0x50/0xa0
> 	  ahci_port_start+0xfb/0x1f0 [libahci]
> 	  ata_host_start.part.39+0x104/0x1e0 [libata]
> 
> With the earlier check the kdump boot succeeds and a crashdump is written.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/intel-iommu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 42cdcce1602e..32f43695a22b 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
>   
>   static struct dmar_domain *deferred_attach_domain(struct device *dev)
>   {
> -	if (unlikely(attach_deferred(dev)))
> -		do_deferred_attach(dev);
> -

This should also be moved to the call place of deferred_attach_domain()
in bounce_map_single().

bounce_map_single() assumes that devices always use DMA domain, so it
doesn't call iommu_need_mapping(). We could do_deferred_attach() there
manually.

Best regards,
baolu

>   	return find_domain(dev);
>   }
>   
> @@ -3595,6 +3592,9 @@ static bool iommu_need_mapping(struct device *dev)
>   	if (iommu_dummy(dev))
>   		return false;
>   
> +	if (unlikely(attach_deferred(dev)))
> +		do_deferred_attach(dev);
> +
>   	ret = identity_mapping(dev);
>   	if (ret) {
>   		u64 dma_mask = *dev->dma_mask;
> 

[PATCH 3/5 v2] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Joerg Roedel]

Hi Baolu,

On Tue, Feb 18, 2020 at 10:38:14AM +0800, Lu Baolu wrote:
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 42cdcce1602e..32f43695a22b 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
> >   static struct dmar_domain *deferred_attach_domain(struct device *dev)
> >   {
> > -	if (unlikely(attach_deferred(dev)))
> > -		do_deferred_attach(dev);
> > -
> 
> This should also be moved to the call place of deferred_attach_domain()
> in bounce_map_single().
> 
> bounce_map_single() assumes that devices always use DMA domain, so it
> doesn't call iommu_need_mapping(). We could do_deferred_attach() there
> manually.

Good point, thanks for your review. Updated patch below.

From 3a5b8a66d288d86ac1fd45092e7d96f842d0cccf Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
Date: Mon, 17 Feb 2020 17:20:59 +0100
Subject: [PATCH 3/5] iommu/vt-d: Do deferred attachment in
 iommu_need_mapping()

The attachment of deferred devices needs to happen before the check
whether the device is identity mapped or not. Otherwise the check will
return wrong results, cause warnings boot failures in kdump kernels, like

	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70

	[...]

	 Call Trace:
	  __intel_map_single+0x55/0x190
	  intel_alloc_coherent+0xac/0x110
	  dmam_alloc_attrs+0x50/0xa0
	  ahci_port_start+0xfb/0x1f0 [libahci]
	  ata_host_start.part.39+0x104/0x1e0 [libata]

With the earlier check the kdump boot succeeds and a crashdump is written.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 42cdcce1602e..723f615c6e84 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
 
 static struct dmar_domain *deferred_attach_domain(struct device *dev)
 {
-	if (unlikely(attach_deferred(dev)))
-		do_deferred_attach(dev);
-
 	return find_domain(dev);
 }
 
@@ -3595,6 +3592,9 @@ static bool iommu_need_mapping(struct device *dev)
 	if (iommu_dummy(dev))
 		return false;
 
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
+
 	ret = identity_mapping(dev);
 	if (ret) {
 		u64 dma_mask = *dev->dma_mask;
@@ -3958,7 +3958,11 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size,
 	int prot = 0;
 	int ret;
 
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
+
 	domain = deferred_attach_domain(dev);
+
 	if (WARN_ON(dir == DMA_NONE || !domain))
 		return DMA_MAPPING_ERROR;
 
-- 
2.25.0

Re: [PATCH 3/5 v2] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Lu Baolu]

Hi Joerg,

On 2020/2/18 17:28, Joerg Roedel wrote:
> Hi Baolu,
> 
> On Tue, Feb 18, 2020 at 10:38:14AM +0800, Lu Baolu wrote:
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 42cdcce1602e..32f43695a22b 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
>>>    static struct dmar_domain *deferred_attach_domain(struct device *dev)
>>>    {
>>> -	if (unlikely(attach_deferred(dev)))
>>> -		do_deferred_attach(dev);
>>> -
>>
>> This should also be moved to the call place of deferred_attach_domain()
>> in bounce_map_single().
>>
>> bounce_map_single() assumes that devices always use DMA domain, so it
>> doesn't call iommu_need_mapping(). We could do_deferred_attach() there
>> manually.
> 
> Good point, thanks for your review. Updated patch below.
> 

Looks good to me now. For all patches in this series,

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

>  From 3a5b8a66d288d86ac1fd45092e7d96f842d0cccf Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel@suse.de>
> Date: Mon, 17 Feb 2020 17:20:59 +0100
> Subject: [PATCH 3/5] iommu/vt-d: Do deferred attachment in
>   iommu_need_mapping()
> 
> The attachment of deferred devices needs to happen before the check
> whether the device is identity mapped or not. Otherwise the check will
> return wrong results, cause warnings boot failures in kdump kernels, like
> 
> 	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70
> 
> 	[...]
> 
> 	 Call Trace:
> 	  __intel_map_single+0x55/0x190
> 	  intel_alloc_coherent+0xac/0x110
> 	  dmam_alloc_attrs+0x50/0xa0
> 	  ahci_port_start+0xfb/0x1f0 [libahci]
> 	  ata_host_start.part.39+0x104/0x1e0 [libata]
> 
> With the earlier check the kdump boot succeeds and a crashdump is written.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/intel-iommu.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 42cdcce1602e..723f615c6e84 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
>   
>   static struct dmar_domain *deferred_attach_domain(struct device *dev)
>   {
> -	if (unlikely(attach_deferred(dev)))
> -		do_deferred_attach(dev);
> -
>   	return find_domain(dev);
>   }
>   
> @@ -3595,6 +3592,9 @@ static bool iommu_need_mapping(struct device *dev)
>   	if (iommu_dummy(dev))
>   		return false;
>   
> +	if (unlikely(attach_deferred(dev)))
> +		do_deferred_attach(dev);
> +
>   	ret = identity_mapping(dev);
>   	if (ret) {
>   		u64 dma_mask = *dev->dma_mask;
> @@ -3958,7 +3958,11 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size,
>   	int prot = 0;
>   	int ret;
>   
> +	if (unlikely(attach_deferred(dev)))
> +		do_deferred_attach(dev);
> +
>   	domain = deferred_attach_domain(dev);
> +
>   	if (WARN_ON(dir == DMA_NONE || !domain))
>   		return DMA_MAPPING_ERROR;
>   
> 

Re: [PATCH 3/5 v2] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Jerry Snitselaar]

On Tue Feb 18 20, Joerg Roedel wrote:
>Hi Baolu,
>
>On Tue, Feb 18, 2020 at 10:38:14AM +0800, Lu Baolu wrote:
>> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> > index 42cdcce1602e..32f43695a22b 100644
>> > --- a/drivers/iommu/intel-iommu.c
>> > +++ b/drivers/iommu/intel-iommu.c
>> > @@ -2541,9 +2541,6 @@ static void do_deferred_attach(struct device *dev)
>> >   static struct dmar_domain *deferred_attach_domain(struct device *dev)
>> >   {
>> > -	if (unlikely(attach_deferred(dev)))
>> > -		do_deferred_attach(dev);
>> > -
>>
>> This should also be moved to the call place of deferred_attach_domain()
>> in bounce_map_single().
>>
>> bounce_map_single() assumes that devices always use DMA domain, so it
>> doesn't call iommu_need_mapping(). We could do_deferred_attach() there
>> manually.
>
>Good point, thanks for your review. Updated patch below.
>
>From 3a5b8a66d288d86ac1fd45092e7d96f842d0cccf Mon Sep 17 00:00:00 2001
>From: Joerg Roedel <jroedel@suse.de>
>Date: Mon, 17 Feb 2020 17:20:59 +0100
>Subject: [PATCH 3/5] iommu/vt-d: Do deferred attachment in
> iommu_need_mapping()
>
>The attachment of deferred devices needs to happen before the check
>whether the device is identity mapped or not. Otherwise the check will
>return wrong results, cause warnings boot failures in kdump kernels, like
>
>	WARNING: CPU: 0 PID: 318 at ../drivers/iommu/intel-iommu.c:592 domain_get_iommu+0x61/0x70
>
>	[...]
>
>	 Call Trace:
>	  __intel_map_single+0x55/0x190
>	  intel_alloc_coherent+0xac/0x110
>	  dmam_alloc_attrs+0x50/0xa0
>	  ahci_port_start+0xfb/0x1f0 [libahci]
>	  ata_host_start.part.39+0x104/0x1e0 [libata]
>
>With the earlier check the kdump boot succeeds and a crashdump is written.
>
>Signed-off-by: Joerg Roedel <jroedel@suse.de>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH 3/5 v2] iommu/vt-d: Do deferred attachment in iommu_need_mapping()

[Joerg Roedel]

On Tue, Feb 18, 2020 at 07:54:52PM +0800, Lu Baolu wrote:
> Looks good to me now. For all patches in this series,
> 
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Thanks, queued the fixes for v5.6.

Re: [PATCH 1/5] iommu/vt-d: Add attach_deferred() helper

[Christoph Hellwig]

> +static bool attach_deferred(struct device *dev)
> +{
> +	return dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO;
> +}

This is not a very useful helper.

> +
>  /**
>   * is_downstream_to_pci_bridge - test if a device belongs to the PCI
>   *				 sub-hierarchy of a candidate PCI-PCI bridge
> @@ -2510,8 +2515,7 @@ struct dmar_domain *find_domain(struct device *dev)
>  {
>  	struct device_domain_info *info;
>  
> -	if (unlikely(dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO ||
> -		     dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO))
> +	if (unlikely(attach_deferred(dev) || iommu_dummy(dev)))
>  		return NULL;

I'd rather kill the iommu_dummy helper as well.  And IIRC I have an
outstanding series somewhere that does just that..

Re: [PATCH 2/5] iommu/vt-d: Move deferred device attachment into helper function

[Christoph Hellwig]

> +static void do_deferred_attach(struct device *dev)
>  {
> +	struct iommu_domain *domain;
>  
> +	dev->archdata.iommu = NULL;
> +	domain = iommu_get_domain_for_dev(dev);
> +	if (domain)
> +		intel_iommu_attach_device(domain, dev);
> +}
> +
> +static struct dmar_domain *deferred_attach_domain(struct device *dev)
> +{
> +	if (unlikely(attach_deferred(dev)))
> +		do_deferred_attach(dev);
>  
>  	return find_domain(dev);
>  }

Can we start using proper sybmbol prefixes and avoid do_* names where
possible?

Re: [PATCH 1/5] iommu/vt-d: Add attach_deferred() helper

[Joerg Roedel]

On Tue, Feb 18, 2020 at 09:14:54AM -0800, Christoph Hellwig wrote:
> > +static bool attach_deferred(struct device *dev)
> > +{
> > +	return dev->archdata.iommu == DEFER_DEVICE_DOMAIN_INFO;
> > +}
> 
> This is not a very useful helper.

Depends on what one considers useful. I think such helpers make the code
better readable, which is pretty useful to me.

Regards,

	Joerg