From: Robin Murphy <robin.murphy@arm.com> To: Joerg Roedel <joro@8bytes.org> Cc: iommu@lists.linux-foundation.org, jroedel@suse.de, linux-kernel@vger.kernel.org, Tom Murphy <murphyt7@tcd.ie>, jsnitsel@redhat.com Subject: Re: [PATCH] iommu: Implement deferred domain attachment Date: Fri, 15 May 2020 17:28:53 +0100 [thread overview] Message-ID: <e7bdcbf1-a713-618d-3e02-037f509a17e9@arm.com> (raw) In-Reply-To: <20200515161400.GZ18353@8bytes.org> 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;
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Joerg Roedel <joro@8bytes.org> Cc: iommu@lists.linux-foundation.org, jroedel@suse.de, linux-kernel@vger.kernel.org, Tom Murphy <murphyt7@tcd.ie> Subject: Re: [PATCH] iommu: Implement deferred domain attachment Date: Fri, 15 May 2020 17:28:53 +0100 [thread overview] Message-ID: <e7bdcbf1-a713-618d-3e02-037f509a17e9@arm.com> (raw) In-Reply-To: <20200515161400.GZ18353@8bytes.org> 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
next prev parent reply other threads:[~2020-05-15 16:28 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-15 9:45 [PATCH] iommu: Implement deferred domain attachment Joerg Roedel 2020-05-15 9:45 ` Joerg Roedel 2020-05-15 13:51 ` Lu Baolu 2020-05-15 13:51 ` Lu Baolu 2020-05-15 14:20 ` Joerg Roedel 2020-05-15 14:20 ` Joerg Roedel 2020-05-15 15:42 ` Robin Murphy 2020-05-15 15:42 ` Robin Murphy 2020-05-15 16:14 ` Joerg Roedel 2020-05-15 16:14 ` Joerg Roedel 2020-05-15 16:28 ` Robin Murphy [this message] 2020-05-15 16:28 ` Robin Murphy 2020-05-15 18:26 ` Joerg Roedel 2020-05-15 18:26 ` Joerg Roedel 2020-05-15 19:23 ` Robin Murphy 2020-05-15 19:23 ` Robin Murphy 2020-05-18 13:26 ` Joerg Roedel 2020-05-18 13:26 ` Joerg Roedel 2020-05-18 22:15 ` Jerry Snitselaar 2020-05-18 22:15 ` Jerry Snitselaar 2020-05-19 7:09 ` Jerry Snitselaar 2020-05-19 7:09 ` Jerry Snitselaar
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=e7bdcbf1-a713-618d-3e02-037f509a17e9@arm.com \ --to=robin.murphy@arm.com \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=jroedel@suse.de \ --cc=jsnitsel@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=murphyt7@tcd.ie \ /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.