[PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[James Dong]

With specific Linux command line option such as "iommu=pt", some types
of hotplug devices will be assigned to the static 1:1 iommu domain by
default during kernel init.

If such a hotplug device is rescanned after being removed, for example,
with following sample commands:
  echo 1 > /sys/bus/pci/devices/0000\:03\:00.1/remove
  echo 1 > /sys/bus/pci/rescan
, an iommu group for this hotplug device will be added without attaching
it to the default static 1:1 domain. As a result, this device's I/O
operation won't work.

Keep hotplug devices with default static 1:1 iommu mapping in their default
domain after such hotplug devices are rescanned.

Signed-off-by: James Dong <xmdong@google.com>
Reported-by: Jis Ben <jisben@google.com>
---
 drivers/iommu/intel-iommu.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 78188bf7e90d..4b02949e58ca 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5230,6 +5230,7 @@ static int intel_iommu_add_device(struct device *dev)
 	struct intel_iommu *iommu;
 	struct iommu_group *group;
 	u8 bus, devfn;
+	int ret = 0;
 
 	iommu = device_to_iommu(dev, &bus, &devfn);
 	if (!iommu)
@@ -5242,8 +5243,19 @@ static int intel_iommu_add_device(struct device *dev)
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
+	if (!iommu_group_default_domain(group) &&
+	    !find_domain(dev) && iommu_should_identity_map(dev, 0)) {
+		ret = domain_add_dev_info(si_domain, dev);
+		if (!ret)
+			pr_info("identity mapping for device %s\n",
+				dev_name(dev));
+		else
+			pr_info("identity mapping failed (%d) for device %s\n",
+				ret, dev_name(dev));
+	}
+
 	iommu_group_put(group);
-	return 0;
+	return ret;
 }
 
 static void intel_iommu_remove_device(struct device *dev)
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Lu Baolu]

Hi,

On 2/22/19 10:44 AM, James Dong wrote:
> With specific Linux command line option such as "iommu=pt", some types
> of hotplug devices will be assigned to the static 1:1 iommu domain by
> default during kernel init.
> 
> If such a hotplug device is rescanned after being removed, for example,
> with following sample commands:
>    echo 1 > /sys/bus/pci/devices/0000\:03\:00.1/remove
>    echo 1 > /sys/bus/pci/rescan
> , an iommu group for this hotplug device will be added without attaching
> it to the default static 1:1 domain. As a result, this device's I/O
> operation won't work.

What does "I/O operation won't work" exactly mean here? Do you see any
IOMMU fault message? Or, something doesn't work as expected?

> 
> Keep hotplug devices with default static 1:1 iommu mapping in their default
> domain after such hotplug devices are rescanned.
> 
> Signed-off-by: James Dong <xmdong@google.com>
> Reported-by: Jis Ben <jisben@google.com>
> ---
>   drivers/iommu/intel-iommu.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 78188bf7e90d..4b02949e58ca 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5230,6 +5230,7 @@ static int intel_iommu_add_device(struct device *dev)
>   	struct intel_iommu *iommu;
>   	struct iommu_group *group;
>   	u8 bus, devfn;
> +	int ret = 0;
>   
>   	iommu = device_to_iommu(dev, &bus, &devfn);
>   	if (!iommu)
> @@ -5242,8 +5243,19 @@ static int intel_iommu_add_device(struct device *dev)
>   	if (IS_ERR(group))
>   		return PTR_ERR(group);
>   
> +	if (!iommu_group_default_domain(group) &&
> +	    !find_domain(dev) && iommu_should_identity_map(dev, 0)) {
> +		ret = domain_add_dev_info(si_domain, dev);
> +		if (!ret)
> +			pr_info("identity mapping for device %s\n",
> +				dev_name(dev));
> +		else
> +			pr_info("identity mapping failed (%d) for device %s\n",
> +				ret, dev_name(dev));
> +	}
> +

I am not sure about whether this is a good place to fix this problem
before Intel IOMMU driver switches to default domain.

Do you mind checking this?

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6ecdcf8fc8c0..f62f30bc1339 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2632,6 +2632,9 @@ static struct dmar_domain 
*find_or_alloc_domain(struct device *dev, int gaw)
                         goto out;
         }

+       if (!iommu_should_identity_map(dev, 0))
+               return si_domain;
+
         /* Allocate and initialize new domain for the device */
         domain = alloc_domain(0);
         if (!domain)

Best regards,
Lu Baolu

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Lu Baolu]

Hi

On 2/22/19 1:06 PM, Lu Baolu wrote:

> Do you mind checking this?
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6ecdcf8fc8c0..f62f30bc1339 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2632,6 +2632,9 @@ static struct dmar_domain 
> *find_or_alloc_domain(struct device *dev, int gaw)
>                          goto out;
>          }
> 
> +       if (!iommu_should_identity_map(dev, 0))
> +               return si_domain;
> +
>          /* Allocate and initialize new domain for the device */
>          domain = alloc_domain(0);
>          if (!domain)

Oops! This can't be compiled. Please try below instead if you'd like to.
Sorry about it.

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6ecdcf8fc8c0..b89f5ba6a3c8 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -174,6 +174,8 @@ static inline unsigned long virt_to_dma_pfn(void *p)
         return page_to_dma_pfn(virt_to_page(p));
  }

+static int iommu_should_identity_map(struct device *dev, int startup);
+
  /* global iommu list, set NULL for ignored DMAR units */
  static struct intel_iommu **g_iommus;

@@ -2632,6 +2634,9 @@ static struct dmar_domain 
*find_or_alloc_domain(struct device *dev, int gaw)
                         goto out;
         }

+       if (iommu_should_identity_map(dev, 0))
+               return si_domain;
+
         /* Allocate and initialize new domain for the device */
         domain = alloc_domain(0);
         if (!domain)

Best regards,
Lu Baolu

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[James Dong via iommu]

[-- Attachment #1.1: Type: text/plain, Size: 3528 bytes --]

Baolu:

Thanks for your comments and your patch. Please find below our responses to
each of your comments:

> What does "I/O operation won't work" exactly mean here? Do you see any
> IOMMU fault message? Or, something doesn't work as expected?

Yes, DMAR fault messages as following came out:
[  354.939896] DMAR: DMAR:[DMA Read] Request device [03:00.1]fault addr
1fdfe80000
[  354.939896] DMAR:[fault reason 02] Present bit in context entry is clear


> Do you mind checking this?
>
> index 6ecdcf8fc8c0..f62f30bc1339 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2632,6 +2632,9 @@ static struct dmar_domain
> *find_or_alloc_domain(struct device *dev, int gaw)
>                          goto out;
>          }
>
> +       if (!iommu_should_identity_map(dev, 0))
> +               return si_domain;
> +
>          /* Allocate and initialize new domain for the device */
>          domain = alloc_domain(0);
>          if (!domain)

Tried this patch, and the same DMAR fault message came out.

Guess it is because of the iommu code path for hotplug devices. If a hotplug
device is rescanned after removal, iommu_bus_notifier will be called as part
of the notifier chains to handle BUS_NOTIFY_ADD_DEVICE event. Along the code
path, intel_iommu_ops->add_device() created an iommu group for this hotplug
device, but failed to create an iommu domain because of the default domain
type IOMMU_DOMAIN_IDENTITY imposed by current IOMMU command line option got
declined by intel_iommu_ops->domain_alloc().

In your patch, function find_or_alloc_domain() is not even in the code path
of BUS_NOTIFY_ADD_DEVICE event notifier chain.

Please let us know if your have more concerns and suggestions.

Best Regards,
James

On Thu, Feb 21, 2019 at 9:35 PM Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:

> Hi
>
> On 2/22/19 1:06 PM, Lu Baolu wrote:
>
> > Do you mind checking this?
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 6ecdcf8fc8c0..f62f30bc1339 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2632,6 +2632,9 @@ static struct dmar_domain
> > *find_or_alloc_domain(struct device *dev, int gaw)
> >                          goto out;
> >          }
> >
> > +       if (!iommu_should_identity_map(dev, 0))
> > +               return si_domain;
> > +
> >          /* Allocate and initialize new domain for the device */
> >          domain = alloc_domain(0);
> >          if (!domain)
>
> Oops! This can't be compiled. Please try below instead if you'd like to.
> Sorry about it.
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6ecdcf8fc8c0..b89f5ba6a3c8 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -174,6 +174,8 @@ static inline unsigned long virt_to_dma_pfn(void *p)
>          return page_to_dma_pfn(virt_to_page(p));
>   }
>
> +static int iommu_should_identity_map(struct device *dev, int startup);
> +
>   /* global iommu list, set NULL for ignored DMAR units */
>   static struct intel_iommu **g_iommus;
>
> @@ -2632,6 +2634,9 @@ static struct dmar_domain
> *find_or_alloc_domain(struct device *dev, int gaw)
>                          goto out;
>          }
>
> +       if (iommu_should_identity_map(dev, 0))
> +               return si_domain;
> +
>          /* Allocate and initialize new domain for the device */
>          domain = alloc_domain(0);
>          if (!domain)
>
> Best regards,
> Lu Baolu
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 5330 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[James Dong]

Baolu:

Sorry that my last reply email seems not text format. Resend it now.

Thanks for your comments and your patch. Please find below our responses to
each of your comments:

> What does "I/O operation won't work" exactly mean here? Do you see any
> IOMMU fault message? Or, something doesn't work as expected?

Yes, DMAR fault messages as following came out:
[  354.939896] DMAR: DMAR:[DMA Read] Request device [03:00.1]fault addr 1fdfe80000 
[  354.939896] DMAR:[fault reason 02] Present bit in context entry is clear


> Do you mind checking this?
>
> index 6ecdcf8fc8c0..f62f30bc1339 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2632,6 +2632,9 @@ static struct dmar_domain 
> *find_or_alloc_domain(struct device *dev, int gaw)
>                          goto out;
>          }
> 
> +       if (!iommu_should_identity_map(dev, 0))
> +               return si_domain;
> +
>          /* Allocate and initialize new domain for the device */
>          domain = alloc_domain(0);
>          if (!domain)

Tried this patch, and the same DMAR fault message came out.

Guess it is because of the iommu code path for hotplug devices. If a hotplug
device is rescanned after removal, iommu_bus_notifier will be called as part
of the notifier chains to handle BUS_NOTIFY_ADD_DEVICE event. Along the code
path, intel_iommu_ops->add_device() created an iommu group for this hotplug
device, but failed to create an iommu domain because of the default domain
type IOMMU_DOMAIN_IDENTITY imposed by current IOMMU command line option got
declined by intel_iommu_ops->domain_alloc().

Since si_domain is type of "struct dmar_domain", which is platform dependent,
it is hard to make this change in intel_iommu_ops->domain_alloc().

In your patch, function find_or_alloc_domain() is not in the code path of
BUS_NOTIFY_ADD_DEVICE event notifier chain.

Please let us know if your have more concerns and suggestions.

Best Regards,
James

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Lu Baolu]

Hi James,

On 2/22/19 2:38 PM, James Dong wrote:
> 
> Tried this patch, and the same DMAR fault message came out.
> 
> Guess it is because of the iommu code path for hotplug devices. If a hotplug
> device is rescanned after removal, iommu_bus_notifier will be called as part
> of the notifier chains to handle BUS_NOTIFY_ADD_DEVICE event. Along the code
> path, intel_iommu_ops->add_device() created an iommu group for this hotplug
> device, but failed to create an iommu domain because of the default domain
> type IOMMU_DOMAIN_IDENTITY imposed by current IOMMU command line option got
> declined by intel_iommu_ops->domain_alloc().
> 
> In your patch, function find_or_alloc_domain() is not even in the code path
> of BUS_NOTIFY_ADD_DEVICE event notifier chain.
> 
> Please let us know if your have more concerns and suggestions.

Can I reproduce this with a local machine? If so, how should I do?

> 
> Best Regards,
> James

Best regards,
Lu Baolu

RE: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[James Dong]

Baolu:

The reproduction depends on devices. HW passthrough PCIe devices with default
identity map could have the issue. Make sure that messages like following came
out in dmesg and their mapping does not change after booting:
  [   10.167809] DMAR: Hardware identity mapping for device 0000:30:00.0
  [   10.167823] DMAR: Hardware identity mapping for device 0000:30:00.1

Devices which make following true could also be used for the experiment:
  > static int iommu_should_identity_map(struct device *dev, int startup)
  > {
  >		if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
  >			return 1;
  >		if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
  >			return 1;

Once they are up, remove them first by following command:
  echo 1 > /sys/bus/pci/devices/0000\:03\:00.1/remove

Then trigger the hotplug device rescanning:
  echo 1 > /sys/bus/pci/rescan

To provide an example of specific devices on the market, I need to try out.
Or, if it is fine with you, forcing a PCIe NIC card to be default hardware
passthrough by changing the intel-iommu.c is another easy way to reproduce
this issue.

Best Regards,
James

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Jis Ben via iommu]

[-- Attachment #1.1: Type: text/plain, Size: 1586 bytes --]

An NVMe device configured in static identity mapping should also cause
this error when removed and rescanned. Essentially, a device that does
a DMA when its driver inits, or one that you can force a DMA from.



On Fri, Feb 22, 2019 at 12:28 AM James Dong <xmdong-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> Baolu:
>
> The reproduction depends on devices. HW passthrough PCIe devices with
> default
> identity map could have the issue. Make sure that messages like following
> came
> out in dmesg and their mapping does not change after booting:
>   [   10.167809] DMAR: Hardware identity mapping for device 0000:30:00.0
>   [   10.167823] DMAR: Hardware identity mapping for device 0000:30:00.1
>
> Devices which make following true could also be used for the experiment:
>   > static int iommu_should_identity_map(struct device *dev, int startup)
>   > {
>   >             if ((iommu_identity_mapping & IDENTMAP_AZALIA) &&
> IS_AZALIA(pdev))
>   >                     return 1;
>   >             if ((iommu_identity_mapping & IDENTMAP_GFX) &&
> IS_GFX_DEVICE(pdev))
>   >                     return 1;
>
> Once they are up, remove them first by following command:
>   echo 1 > /sys/bus/pci/devices/0000\:03\:00.1/remove
>
> Then trigger the hotplug device rescanning:
>   echo 1 > /sys/bus/pci/rescan
>
> To provide an example of specific devices on the market, I need to try out.
> Or, if it is fine with you, forcing a PCIe NIC card to be default hardware
> passthrough by changing the intel-iommu.c is another easy way to reproduce
> this issue.
>
> Best Regards,
> James
>

[-- Attachment #1.2: Type: text/html, Size: 2092 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Jis Ben]

An NVMe device configured in static identity mapping should also cause
this error when removed and rescanned. Essentially, a device that does
a DMA when its driver inits, or one that you can force a DMA from.

-Jis


On Fri, Feb 22, 2019 at 12:28 AM James Dong <xmdong@google.com> wrote:
>
> Baolu:
>
> The reproduction depends on devices. HW passthrough PCIe devices with default
> identity map could have the issue. Make sure that messages like following came
> out in dmesg and their mapping does not change after booting:
>   [   10.167809] DMAR: Hardware identity mapping for device 0000:30:00.0
>   [   10.167823] DMAR: Hardware identity mapping for device 0000:30:00.1
>
> Devices which make following true could also be used for the experiment:
>   > static int iommu_should_identity_map(struct device *dev, int startup)
>   > {
>   >             if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
>   >                     return 1;
>   >             if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
>   >                     return 1;
>
> Once they are up, remove them first by following command:
>   echo 1 > /sys/bus/pci/devices/0000\:03\:00.1/remove
>
> Then trigger the hotplug device rescanning:
>   echo 1 > /sys/bus/pci/rescan
>
> To provide an example of specific devices on the market, I need to try out.
> Or, if it is fine with you, forcing a PCIe NIC card to be default hardware
> passthrough by changing the intel-iommu.c is another easy way to reproduce
> this issue.
>
> Best Regards,
> James

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Lu Baolu]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

Hi James,

On 2/22/19 3:36 PM, James Dong wrote:
> Baolu:
> 
> Sorry that my last reply email seems not text format. Resend it now.
> 
> Thanks for your comments and your patch. Please find below our responses to
> each of your comments:
> 
>> What does "I/O operation won't work" exactly mean here? Do you see any
>> IOMMU fault message? Or, something doesn't work as expected?
> 
> Yes, DMAR fault messages as following came out:
> [  354.939896] DMAR: DMAR:[DMA Read] Request device [03:00.1]fault addr 1fdfe80000
> [  354.939896] DMAR:[fault reason 02] Present bit in context entry is clear
> 
> 
>> Do you mind checking this?
>>
>> index 6ecdcf8fc8c0..f62f30bc1339 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2632,6 +2632,9 @@ static struct dmar_domain
>> *find_or_alloc_domain(struct device *dev, int gaw)
>>                           goto out;
>>           }
>>
>> +       if (!iommu_should_identity_map(dev, 0))
>> +               return si_domain;
>> +
>>           /* Allocate and initialize new domain for the device */
>>           domain = alloc_domain(0);
>>           if (!domain)
> 
> Tried this patch, and the same DMAR fault message came out.

Thank you!

> 
> Guess it is because of the iommu code path for hotplug devices. If a hotplug
> device is rescanned after removal, iommu_bus_notifier will be called as part
> of the notifier chains to handle BUS_NOTIFY_ADD_DEVICE event. Along the code
> path, intel_iommu_ops->add_device() created an iommu group for this hotplug
> device, but failed to create an iommu domain because of the default domain
> type IOMMU_DOMAIN_IDENTITY imposed by current IOMMU command line option got
> declined by intel_iommu_ops->domain_alloc().

The Intel IOMMU driver hasn't switched to default domain yet although
it's in the pipe line. So, there should be no domain allocated when a
group is allocated for the device.

The problem is we need to check whether a hot-added device requires
identity map instead of allocating a normal domain blindly.

> 
> Since si_domain is type of "struct dmar_domain", which is platform dependent,
> it is hard to make this change in intel_iommu_ops->domain_alloc().
> 
> In your patch, function find_or_alloc_domain() is not in the code path of
> BUS_NOTIFY_ADD_DEVICE event notifier chain.
> 
> Please let us know if your have more concerns and suggestions.

Can you please try the patch attached? I think this is a generic issue
as I described in the commit message.

Best regards,
Lu Baolu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iommu-vt-d-Check-identity-map-for-hot-added-devices.patch --]
[-- Type: text/x-patch; name="0001-iommu-vt-d-Check-identity-map-for-hot-added-devices.patch", Size: 2584 bytes --]

From d942e60557fc7ea6fee535fb9a0a7d334d65b636 Mon Sep 17 00:00:00 2001
From: Lu Baolu <baolu.lu@linux.intel.com>
Date: Sun, 24 Feb 2019 10:01:03 +0800
Subject: [PATCH 1/1] iommu/vt-d: Check identity map for hot-added devices

The Intel IOMMU driver will put devices into a static identity
mapped domain during boot if the kernel parameter "iommu=pt" is
used. That means the IOMMU hardware will translate a DMA address
into the same memory address.

Unfortunately, a hot-added device doesn't subject to this. That
results in some devices not working properly after hot added. A
quick way to reproduce this issue is to boot a system with

    iommu=pt

and, remove then readd the pci device with

    echo 1 > /sys/bus/pci/devices/[pci_source_id]/remove
    echo 1 > /sys/bus/pci/rescan

You will find the identity mapped domain was replaced with a
normal domain.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6ecdcf8fc8c0..730ee29d561b 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3001,9 +3001,9 @@ static int iommu_should_identity_map(struct device *dev, int startup)
 	}
 
 	/*
-	 * At boot time, we don't yet know if devices will be 64-bit capable.
-	 * Assume that they will — if they turn out not to be, then we can
-	 * take them out of the 1:1 domain later.
+	 * At boot time or hot added, we don't yet know if devices will be
+	 * 64-bit capable. Assume that they will — if they turn out not to
+	 * be, then we can take them out of the 1:1 domain later.
 	 */
 	if (!startup) {
 		/*
@@ -4807,16 +4807,19 @@ static int device_notifier(struct notifier_block *nb,
 	if (iommu_dummy(dev))
 		return 0;
 
-	if (action != BUS_NOTIFY_REMOVED_DEVICE)
-		return 0;
-
-	domain = find_domain(dev);
-	if (!domain)
-		return 0;
+	if (action == BUS_NOTIFY_REMOVED_DEVICE) {
+		domain = find_domain(dev);
+		if (!domain)
+			return 0;
 
-	dmar_remove_one_dev_info(dev);
-	if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
-		domain_exit(domain);
+		dmar_remove_one_dev_info(dev);
+		if (!domain_type_is_vm_or_si(domain) &&
+		    list_empty(&domain->devices))
+			domain_exit(domain);
+	} else if (action == BUS_NOTIFY_ADD_DEVICE) {
+		if (iommu_should_identity_map(dev, 1))
+			domain_add_dev_info(si_domain, dev);
+	}
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[Lu Baolu]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

Hi James,

On 2/22/19 3:36 PM, James Dong wrote:
> Baolu:
> 
> Sorry that my last reply email seems not text format. Resend it now.
> 
> Thanks for your comments and your patch. Please find below our responses to
> each of your comments:
> 
>> What does "I/O operation won't work" exactly mean here? Do you see any
>> IOMMU fault message? Or, something doesn't work as expected?
> 
> Yes, DMAR fault messages as following came out:
> [  354.939896] DMAR: DMAR:[DMA Read] Request device [03:00.1]fault addr 1fdfe80000
> [  354.939896] DMAR:[fault reason 02] Present bit in context entry is clear
> 
> 
>> Do you mind checking this?
>>
>> index 6ecdcf8fc8c0..f62f30bc1339 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2632,6 +2632,9 @@ static struct dmar_domain
>> *find_or_alloc_domain(struct device *dev, int gaw)
>>                           goto out;
>>           }
>>
>> +       if (!iommu_should_identity_map(dev, 0))
>> +               return si_domain;
>> +
>>           /* Allocate and initialize new domain for the device */
>>           domain = alloc_domain(0);
>>           if (!domain)
> 
> Tried this patch, and the same DMAR fault message came out.

Thank you!

> 
> Guess it is because of the iommu code path for hotplug devices. If a hotplug
> device is rescanned after removal, iommu_bus_notifier will be called as part
> of the notifier chains to handle BUS_NOTIFY_ADD_DEVICE event. Along the code
> path, intel_iommu_ops->add_device() created an iommu group for this hotplug
> device, but failed to create an iommu domain because of the default domain
> type IOMMU_DOMAIN_IDENTITY imposed by current IOMMU command line option got
> declined by intel_iommu_ops->domain_alloc().

The Intel IOMMU driver hasn't switched to default domain yet although
it's in the pipe line. So, there should be no domain allocated when a
group is allocated for the device.

The problem is we need to check whether a hot-added device requires
identity map instead of allocating a normal domain blindly.

> 
> Since si_domain is type of "struct dmar_domain", which is platform dependent,
> it is hard to make this change in intel_iommu_ops->domain_alloc().
> 
> In your patch, function find_or_alloc_domain() is not in the code path of
> BUS_NOTIFY_ADD_DEVICE event notifier chain.
> 
> Please let us know if your have more concerns and suggestions.

Can you please try the patch attached? I think this is a generic issue
as I described in the commit message.

Best regards,
Lu Baolu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iommu-vt-d-Check-identity-map-for-hot-added-devices.patch --]
[-- Type: text/x-patch; name="0001-iommu-vt-d-Check-identity-map-for-hot-added-devices.patch", Size: 2687 bytes --]

>From d942e60557fc7ea6fee535fb9a0a7d334d65b636 Mon Sep 17 00:00:00 2001
From: Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Date: Sun, 24 Feb 2019 10:01:03 +0800
Subject: [PATCH 1/1] iommu/vt-d: Check identity map for hot-added devices

The Intel IOMMU driver will put devices into a static identity
mapped domain during boot if the kernel parameter "iommu=pt" is
used. That means the IOMMU hardware will translate a DMA address
into the same memory address.

Unfortunately, a hot-added device doesn't subject to this. That
results in some devices not working properly after hot added. A
quick way to reproduce this issue is to boot a system with

    iommu=pt

and, remove then readd the pci device with

    echo 1 > /sys/bus/pci/devices/[pci_source_id]/remove
    echo 1 > /sys/bus/pci/rescan

You will find the identity mapped domain was replaced with a
normal domain.

Cc: Ashok Raj <ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/intel-iommu.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6ecdcf8fc8c0..730ee29d561b 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3001,9 +3001,9 @@ static int iommu_should_identity_map(struct device *dev, int startup)
 	}
 
 	/*
-	 * At boot time, we don't yet know if devices will be 64-bit capable.
-	 * Assume that they will — if they turn out not to be, then we can
-	 * take them out of the 1:1 domain later.
+	 * At boot time or hot added, we don't yet know if devices will be
+	 * 64-bit capable. Assume that they will — if they turn out not to
+	 * be, then we can take them out of the 1:1 domain later.
 	 */
 	if (!startup) {
 		/*
@@ -4807,16 +4807,19 @@ static int device_notifier(struct notifier_block *nb,
 	if (iommu_dummy(dev))
 		return 0;
 
-	if (action != BUS_NOTIFY_REMOVED_DEVICE)
-		return 0;
-
-	domain = find_domain(dev);
-	if (!domain)
-		return 0;
+	if (action == BUS_NOTIFY_REMOVED_DEVICE) {
+		domain = find_domain(dev);
+		if (!domain)
+			return 0;
 
-	dmar_remove_one_dev_info(dev);
-	if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
-		domain_exit(domain);
+		dmar_remove_one_dev_info(dev);
+		if (!domain_type_is_vm_or_si(domain) &&
+		    list_empty(&domain->devices))
+			domain_exit(domain);
+	} else if (action == BUS_NOTIFY_ADD_DEVICE) {
+		if (iommu_should_identity_map(dev, 1))
+			domain_add_dev_info(si_domain, dev);
+	}
 
 	return 0;
 }
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

RE: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting

[James Dong]

Baolu:

Yes, it is a generic issue for hotplug devices with current Intel IOMMU driver,
as reported in this thread as well.

The patch you provided does the job in our case. Please update this thread once
your patch is merged. Thanks.

Best Regards,
James

On 2/23/19 12:56 AM, Lu Baolu wrote
>
> @@ -4807,16 +4807,19 @@ static int device_notifier(struct notifier_block *nb,
> 	if (iommu_dummy(dev))
> 		return 0;
> 
>-	if (action != BUS_NOTIFY_REMOVED_DEVICE)
>-		return 0;
>-
>-	domain = find_domain(dev);
>-	if (!domain)
>-		return 0;
>+	if (action == BUS_NOTIFY_REMOVED_DEVICE) {
>+		domain = find_domain(dev);
>+		if (!domain)
>+			return 0;
> 
>-	dmar_remove_one_dev_info(dev);
>-	if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
>-		domain_exit(domain);
>+		dmar_remove_one_dev_info(dev);
>+		if (!domain_type_is_vm_or_si(domain) &&
>+		    list_empty(&domain->devices))
>+			domain_exit(domain);
>+	} else if (action == BUS_NOTIFY_ADD_DEVICE) {
>+		if (iommu_should_identity_map(dev, 1))
>+			domain_add_dev_info(si_domain, dev);
>+	}