From: Lu Baolu <baolu.lu@linux.intel.com> To: James Dong <xmdong@google.com> Cc: baolu.lu@linux.intel.com, David Woodhouse <dwmw2@infradead.org>, Joerg Roedel <joro@8bytes.org>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Jis Ben <jisben@google.com> Subject: Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting Date: Sun, 24 Feb 2019 16:51:06 +0800 [thread overview] Message-ID: <897e8383-ad75-35d0-5ad9-3d3a8b46b6ba@linux.intel.com> (raw) In-Reply-To: <20190222073658.66798-1-xmdong@google.com> [-- 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
WARNING: multiple messages have this Message-ID (diff)
From: Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> To: James Dong <xmdong-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Jis Ben <jisben-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Subject: Re: [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting Date: Sun, 24 Feb 2019 16:51:06 +0800 [thread overview] Message-ID: <897e8383-ad75-35d0-5ad9-3d3a8b46b6ba@linux.intel.com> (raw) In-Reply-To: <20190222073658.66798-1-xmdong-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> [-- 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 --]
next prev parent reply other threads:[~2019-02-24 9:03 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-22 2:44 [PATCH] iommu/vt-d: Handle hotplug devices' default identity mapping setting James Dong 2019-02-22 5:06 ` Lu Baolu 2019-02-22 5:30 ` Lu Baolu [not found] ` <fe6dcb4e-03c4-0bbf-c51f-5964b20593dd-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 2019-02-22 6:38 ` James Dong via iommu 2019-02-22 7:42 ` Lu Baolu 2019-02-22 8:28 ` James Dong [not found] ` <20190222082829.75567-1-xmdong-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2019-02-22 18:07 ` Jis Ben via iommu 2019-02-22 18:10 ` Jis Ben 2019-02-22 7:36 ` James Dong 2019-02-24 8:51 ` Lu Baolu [this message] 2019-02-24 8:51 ` Lu Baolu 2019-02-24 16:10 ` James Dong
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=897e8383-ad75-35d0-5ad9-3d3a8b46b6ba@linux.intel.com \ --to=baolu.lu@linux.intel.com \ --cc=dwmw2@infradead.org \ --cc=iommu@lists.linux-foundation.org \ --cc=jisben@google.com \ --cc=joro@8bytes.org \ --cc=linux-kernel@vger.kernel.org \ --cc=xmdong@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.