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 16:42:23 +0100 [thread overview] Message-ID: <d4e1cd9e-fc83-d41a-49c0-8f14f44b2701@arm.com> (raw) In-Reply-To: <20200515094519.20338-1-joro@8bytes.org> 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; > } > > /* >
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 16:42:23 +0100 [thread overview] Message-ID: <d4e1cd9e-fc83-d41a-49c0-8f14f44b2701@arm.com> (raw) In-Reply-To: <20200515094519.20338-1-joro@8bytes.org> 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
next prev parent reply other threads:[~2020-05-15 15:42 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 [this message] 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 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=d4e1cd9e-fc83-d41a-49c0-8f14f44b2701@arm.com \ --to=robin.murphy@arm.com \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=jroedel@suse.de \ --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.