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