Messages in this thread Patch in this message | | | From | Kai-Heng Feng <> | Date | Wed, 7 Jul 2021 17:06:35 +0800 | Subject | Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 |
| |
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, > >>>
| |