* [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 @ 2021-07-06 6:51 Kai-Heng Feng 2021-07-06 9:27 ` Robin Murphy 0 siblings, 1 reply; 5+ messages in thread From: Kai-Heng Feng @ 2021-07-06 6:51 UTC (permalink / raw) To: joro, will; +Cc: Kai-Heng Feng, Lu Baolu, open list:IOMMU DRIVERS, open list Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") not only moved the check for untrusted device to IOMMU core, it also introduced a behavioral change by returning def_domain_type() directly without checking its return value. That makes many devices no longer use the default IOMMU setting. So revert back to the old behavior which defaults to iommu_def_domain_type when driver callback returns 0. Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/iommu/iommu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 5419c4b9f27a..faac4f795025 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); static int iommu_get_def_domain_type(struct device *dev) { const struct iommu_ops *ops = dev->bus->iommu_ops; + unsigned int type = 0; if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) return IOMMU_DOMAIN_DMA; if (ops->def_domain_type) - return ops->def_domain_type(dev); + type = ops->def_domain_type(dev); - return 0; + return (type == 0) ? iommu_def_domain_type : type; } static int iommu_group_alloc_default_domain(struct bus_type *bus, -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 2021-07-06 6:51 [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 Kai-Heng Feng @ 2021-07-06 9:27 ` Robin Murphy 2021-07-06 16:21 ` Kai-Heng Feng 0 siblings, 1 reply; 5+ messages in thread From: Robin Murphy @ 2021-07-06 9:27 UTC (permalink / raw) To: Kai-Heng Feng, joro, will; +Cc: open list:IOMMU DRIVERS, open list On 2021-07-06 07:51, Kai-Heng Feng wrote: > Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted > device into core") not only moved the check for untrusted device to > IOMMU core, it also introduced a behavioral change by returning > def_domain_type() directly without checking its return value. That makes > many devices no longer use the default IOMMU setting. > > So revert back to the old behavior which defaults to > iommu_def_domain_type when driver callback returns 0. > > Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") Are you sure about that? From that same commit: @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct iommu_group *group, if (group->default_domain) return 0; - type = iommu_get_def_domain_type(dev); + type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; return iommu_group_alloc_default_domain(dev->bus, group, type); } AFAICS the other two callers should also handle 0 correctly. Have you seen a problem in practice? Robin. > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/iommu/iommu.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 5419c4b9f27a..faac4f795025 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); > static int iommu_get_def_domain_type(struct device *dev) > { > const struct iommu_ops *ops = dev->bus->iommu_ops; > + unsigned int type = 0; > > if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) > return IOMMU_DOMAIN_DMA; > > if (ops->def_domain_type) > - return ops->def_domain_type(dev); > + type = ops->def_domain_type(dev); > > - return 0; > + return (type == 0) ? iommu_def_domain_type : type; > } > > static int iommu_group_alloc_default_domain(struct bus_type *bus, > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 2021-07-06 9:27 ` Robin Murphy @ 2021-07-06 16:21 ` Kai-Heng Feng 2021-07-06 18:02 ` Robin Murphy 0 siblings, 1 reply; 5+ messages in thread From: Kai-Heng Feng @ 2021-07-06 16:21 UTC (permalink / raw) To: Robin Murphy; +Cc: Joerg Roedel, will, open list:IOMMU DRIVERS, open list On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-07-06 07:51, Kai-Heng Feng wrote: > > Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted > > device into core") not only moved the check for untrusted device to > > IOMMU core, it also introduced a behavioral change by returning > > def_domain_type() directly without checking its return value. That makes > > many devices no longer use the default IOMMU setting. > > > > So revert back to the old behavior which defaults to > > iommu_def_domain_type when driver callback returns 0. > > > > Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") > > Are you sure about that? From that same commit: > > @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct > iommu_group *group, > if (group->default_domain) > return 0; > > - type = iommu_get_def_domain_type(dev); > + type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; > > return iommu_group_alloc_default_domain(dev->bus, group, type); > } > > AFAICS the other two callers should also handle 0 correctly. Have you > seen a problem in practice? Thanks for pointing out how the return value is being handled by the callers. However, the same check is missing in probe_get_default_domain_type(): static int probe_get_default_domain_type(struct device *dev, void *data) { struct __group_domain_type *gtype = data; unsigned int type = iommu_get_def_domain_type(dev); ... } I personally prefer the old way instead of open coding with ternary operator, so I'll do that in v2. In practice, this causes a kernel panic when probing Realtek WiFi. Because of the bug, dma_ops isn't set by probe_finalize(), dma_map_single() falls back to swiotlb which isn't set and caused a kernel panic. I didn't attach the panic log because the system simply is frozen at that point so the message is not logged to the storage. I'll see if I can find another way to collect the log and attach it in v2. Kai-Heng > > Robin. > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/iommu/iommu.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index 5419c4b9f27a..faac4f795025 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); > > static int iommu_get_def_domain_type(struct device *dev) > > { > > const struct iommu_ops *ops = dev->bus->iommu_ops; > > + unsigned int type = 0; > > > > if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) > > return IOMMU_DOMAIN_DMA; > > > > if (ops->def_domain_type) > > - return ops->def_domain_type(dev); > > + type = ops->def_domain_type(dev); > > > > - return 0; > > + return (type == 0) ? iommu_def_domain_type : type; > > } > > > > static int iommu_group_alloc_default_domain(struct bus_type *bus, > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 2021-07-06 16:21 ` Kai-Heng Feng @ 2021-07-06 18:02 ` Robin Murphy 2021-07-07 9:06 ` Kai-Heng Feng 0 siblings, 1 reply; 5+ messages in thread From: Robin Murphy @ 2021-07-06 18:02 UTC (permalink / raw) To: Kai-Heng Feng; +Cc: Joerg Roedel, will, open list:IOMMU DRIVERS, open list On 2021-07-06 17:21, Kai-Heng Feng wrote: > On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 2021-07-06 07:51, Kai-Heng Feng wrote: >>> Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted >>> device into core") not only moved the check for untrusted device to >>> IOMMU core, it also introduced a behavioral change by returning >>> def_domain_type() directly without checking its return value. That makes >>> many devices no longer use the default IOMMU setting. >>> >>> So revert back to the old behavior which defaults to >>> iommu_def_domain_type when driver callback returns 0. >>> >>> Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") >> >> Are you sure about that? From that same commit: >> >> @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct >> iommu_group *group, >> if (group->default_domain) >> return 0; >> >> - type = iommu_get_def_domain_type(dev); >> + type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; >> >> return iommu_group_alloc_default_domain(dev->bus, group, type); >> } >> >> AFAICS the other two callers should also handle 0 correctly. Have you >> seen a problem in practice? > > Thanks for pointing out how the return value is being handled by the callers. > However, the same check is missing in probe_get_default_domain_type(): > static int probe_get_default_domain_type(struct device *dev, void *data) > { > struct __group_domain_type *gtype = data; > unsigned int type = iommu_get_def_domain_type(dev); > ... > } I'm still not following - the next line right after that is "if (type)", which means it won't touch gtype, and if that happens for every iteration, probe_alloc_default_domain() subsequently hits its "if (!gtype.type)" condition and still ends up with iommu_def_domain_type. This *was* one of the other two callers I was talking about (the second being iommu_change_dev_def_domain()), and in fact on second look I think your proposed change will actually break this logic, since it's necessary to differentiate between a specific type being requested for the given device, and a "don't care" response which only implies to use the global default type if it's still standing after *all* the appropriate devices have been queried. > I personally prefer the old way instead of open coding with ternary > operator, so I'll do that in v2. > > In practice, this causes a kernel panic when probing Realtek WiFi. > Because of the bug, dma_ops isn't set by probe_finalize(), > dma_map_single() falls back to swiotlb which isn't set and caused a > kernel panic. Hmm, but if that's the case, wouldn't it still be a problem anyway if the end result was IOMMU_DOMAIN_IDENTITY? I can't claim to fully understand the x86 swiotlb and no_iommu dance, but nothing really stands out to give me confidence that it handles the passthrough options correctly. Robin. > I didn't attach the panic log because the system simply is frozen at > that point so the message is not logged to the storage. > I'll see if I can find another way to collect the log and attach it in v2. > > Kai-Heng > >> >> Robin. >> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> --- >>> drivers/iommu/iommu.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 5419c4b9f27a..faac4f795025 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); >>> static int iommu_get_def_domain_type(struct device *dev) >>> { >>> const struct iommu_ops *ops = dev->bus->iommu_ops; >>> + unsigned int type = 0; >>> >>> if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) >>> return IOMMU_DOMAIN_DMA; >>> >>> if (ops->def_domain_type) >>> - return ops->def_domain_type(dev); >>> + type = ops->def_domain_type(dev); >>> >>> - return 0; >>> + return (type == 0) ? iommu_def_domain_type : type; >>> } >>> >>> static int iommu_group_alloc_default_domain(struct bus_type *bus, >>> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 2021-07-06 18:02 ` Robin Murphy @ 2021-07-07 9:06 ` Kai-Heng Feng 0 siblings, 0 replies; 5+ messages in thread From: Kai-Heng Feng @ 2021-07-07 9:06 UTC (permalink / raw) To: Robin Murphy; +Cc: Joerg Roedel, will, open list:IOMMU DRIVERS, open list On Wed, Jul 7, 2021 at 2:03 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-07-06 17:21, Kai-Heng Feng wrote: > > On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy <robin.murphy@arm.com> wrote: > >> > >> On 2021-07-06 07:51, Kai-Heng Feng wrote: > >>> Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted > >>> device into core") not only moved the check for untrusted device to > >>> IOMMU core, it also introduced a behavioral change by returning > >>> def_domain_type() directly without checking its return value. That makes > >>> many devices no longer use the default IOMMU setting. > >>> > >>> So revert back to the old behavior which defaults to > >>> iommu_def_domain_type when driver callback returns 0. > >>> > >>> Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") > >> > >> Are you sure about that? From that same commit: > >> > >> @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct > >> iommu_group *group, > >> if (group->default_domain) > >> return 0; > >> > >> - type = iommu_get_def_domain_type(dev); > >> + type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; > >> > >> return iommu_group_alloc_default_domain(dev->bus, group, type); > >> } > >> > >> AFAICS the other two callers should also handle 0 correctly. Have you > >> seen a problem in practice? > > > > Thanks for pointing out how the return value is being handled by the callers. > > However, the same check is missing in probe_get_default_domain_type(): > > static int probe_get_default_domain_type(struct device *dev, void *data) > > { > > struct __group_domain_type *gtype = data; > > unsigned int type = iommu_get_def_domain_type(dev); > > ... > > } > > I'm still not following - the next line right after that is "if (type)", > which means it won't touch gtype, and if that happens for every > iteration, probe_alloc_default_domain() subsequently hits its "if > (!gtype.type)" condition and still ends up with iommu_def_domain_type. > This *was* one of the other two callers I was talking about (the second > being iommu_change_dev_def_domain()), and in fact on second look I think > your proposed change will actually break this logic, since it's > necessary to differentiate between a specific type being requested for > the given device, and a "don't care" response which only implies to use > the global default type if it's still standing after *all* the > appropriate devices have been queried. You are right, what I am seeing here is that the AMD GFX is using IOMMU_DOMAIN_IDENTITY, and makes other devices in the same group, including the Realtek WiFi, to use IOMMU_DOMAIN_IDENTITY as well. > > > I personally prefer the old way instead of open coding with ternary > > operator, so I'll do that in v2. > > > > In practice, this causes a kernel panic when probing Realtek WiFi. > > Because of the bug, dma_ops isn't set by probe_finalize(), > > dma_map_single() falls back to swiotlb which isn't set and caused a > > kernel panic. > > Hmm, but if that's the case, wouldn't it still be a problem anyway if > the end result was IOMMU_DOMAIN_IDENTITY? I can't claim to fully > understand the x86 swiotlb and no_iommu dance, but nothing really stands > out to give me confidence that it handles the passthrough options correctly. Yes, I think AMD IOMMU driver needs more thourough check on static void __init amd_iommu_init_dma_ops(void) { swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; ... } Since swiotlb gets disabled by the check, but the Realtek WiFi is only capable of doing 32bit DMA, the kernel panics because io_tlb_default_mem is NULL. Thanks again for pointing to the right direction, this patch is indeed incorrect. Kai-Heng > > Robin. > > > I didn't attach the panic log because the system simply is frozen at > > that point so the message is not logged to the storage. > > I'll see if I can find another way to collect the log and attach it in v2. > > > > Kai-Heng > > > >> > >> Robin. > >> > >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >>> --- > >>> drivers/iommu/iommu.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >>> index 5419c4b9f27a..faac4f795025 100644 > >>> --- a/drivers/iommu/iommu.c > >>> +++ b/drivers/iommu/iommu.c > >>> @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); > >>> static int iommu_get_def_domain_type(struct device *dev) > >>> { > >>> const struct iommu_ops *ops = dev->bus->iommu_ops; > >>> + unsigned int type = 0; > >>> > >>> if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) > >>> return IOMMU_DOMAIN_DMA; > >>> > >>> if (ops->def_domain_type) > >>> - return ops->def_domain_type(dev); > >>> + type = ops->def_domain_type(dev); > >>> > >>> - return 0; > >>> + return (type == 0) ? iommu_def_domain_type : type; > >>> } > >>> > >>> static int iommu_group_alloc_default_domain(struct bus_type *bus, > >>> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-07 9:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-06 6:51 [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 Kai-Heng Feng 2021-07-06 9:27 ` Robin Murphy 2021-07-06 16:21 ` Kai-Heng Feng 2021-07-06 18:02 ` Robin Murphy 2021-07-07 9:06 ` Kai-Heng Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).