| Message ID | 8-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023/8/3 8:07, Jason Gunthorpe wrote: > Except for dart every driver returns 0 or IDENTITY from def_domain_type(). > > The drivers that return IDENTITY have some kind of good reason, typically > that quirky hardware really can't support anything other than IDENTITY. > > Arrange things so that if the driver says it needs IDENTITY then > iommu_get_default_domain_type() either fails or returns IDENTITY. It will > never reject the driver's override to IDENTITY. > > The only real functional difference is that the PCI untrusted flag is now > ignored for quirky HW instead of overriding the IOMMU driver. > > This makes the next patch cleaner that wants to force IDENTITY always for > ARM_IOMMU because there is no support for DMA. > > Tested-by: Steven Price <steven.price@arm.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 66 +++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 33 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index c64365169b678d..53174179102d17 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1669,19 +1669,6 @@ struct iommu_group *fsl_mc_device_group(struct device *dev) > } > EXPORT_SYMBOL_GPL(fsl_mc_device_group); > > -static int iommu_get_def_domain_type(struct device *dev) > -{ > - const struct iommu_ops *ops = dev_iommu_ops(dev); > - > - 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); > - > - return 0; > -} > - > static struct iommu_domain * > __iommu_group_alloc_default_domain(const struct bus_type *bus, > struct iommu_group *group, int req_type) > @@ -1775,36 +1762,49 @@ static int iommu_bus_notifier(struct notifier_block *nb, > static int iommu_get_default_domain_type(struct iommu_group *group, > int target_type) > { > + const struct iommu_ops *ops = dev_iommu_ops( > + list_first_entry(&group->devices, struct group_device, list) > + ->dev); How about consolidating above into a single helper? --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1787,6 +1787,21 @@ __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) return __iommu_group_domain_alloc(group, req_type); } +/* + * Returns the iommu_ops for the devices in an iommu group. + * + * It is assumed that all devices in an iommu group are managed by a single + * IOMMU unit. Therefore, this returns the dev_iommu_ops of the first device + * in the group. + */ +static const struct iommu_ops *group_iommu_ops(struct iommu_group *group) +{ + struct group_device *device; + + device = list_first_entry(&group->devices, struct group_device, list); + return dev_iommu_ops(device->dev); +} + /* * req_type of 0 means "auto" which means to select a domain based on * iommu_def_domain_type or what the driver actually supports. @@ -1794,10 +1809,7 @@ __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) static struct iommu_domain * iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) { - struct device *dev = - list_first_entry(&group->devices, struct group_device, list) - ->dev; - const struct iommu_ops *ops = dev_iommu_ops(dev); + const struct iommu_ops *ops = group_iommu_ops(group); struct iommu_domain *dom; lockdep_assert_held(&group->mutex); @@ -1888,9 +1900,7 @@ static int iommu_bus_notifier(struct notifier_block *nb, static int iommu_get_default_domain_type(struct iommu_group *group, int target_type) { - const struct iommu_ops *ops = dev_iommu_ops( - list_first_entry(&group->devices, struct group_device, list) - ->dev); + const struct iommu_ops *ops = group_iommu_ops(group); int best_type = target_type; struct group_device *gdev; struct device *last_dev; @@ -2124,13 +2134,9 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops, static struct iommu_domain * __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type) { - struct device *dev = - list_first_entry(&group->devices, struct group_device, list) - ->dev; - lockdep_assert_held(&group->mutex); - return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type); + return __iommu_domain_alloc(group_iommu_ops(group), dev, type); } struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus) > int best_type = target_type; > struct group_device *gdev; > struct device *last_dev; > + int type; > > lockdep_assert_held(&group->mutex); > - > for_each_group_device(group, gdev) { > - unsigned int type = iommu_get_def_domain_type(gdev->dev); > - > - if (best_type && type && best_type != type) { > - if (target_type) { > - dev_err_ratelimited( > - gdev->dev, > - "Device cannot be in %s domain\n", > - iommu_domain_type_str(target_type)); > - return -1; > - } > - > - dev_warn( > - gdev->dev, > - "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", > - iommu_domain_type_str(type), dev_name(last_dev), > - iommu_domain_type_str(best_type)); > - return 0; > + type = best_type; > + if (ops->def_domain_type) { > + type = ops->def_domain_type(gdev->dev); > + if (best_type && type && best_type != type) > + goto err; > } > - if (!best_type) > - best_type = type; > + > + if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) { > + type = IOMMU_DOMAIN_DMA; > + if (best_type && type && best_type != type) > + goto err; > + } > + best_type = type; > last_dev = gdev->dev; > } > return best_type; > + > +err: > + if (target_type) { > + dev_err_ratelimited( > + gdev->dev, > + "Device cannot be in %s domain - it is forcing %s\n", > + iommu_domain_type_str(target_type), > + iommu_domain_type_str(type)); > + return -1; > + } > + > + dev_warn( > + gdev->dev, > + "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", > + iommu_domain_type_str(type), dev_name(last_dev), > + iommu_domain_type_str(best_type)); > + return 0; This doesn't match the commit message, where it states: "Arrange things so that if the driver says it needs IDENTITY then iommu_get_default_domain_type() either fails or returns IDENTITY. " I saw that this change was made in the sequential patch. It is probably better to put that here? > } > > static void iommu_group_do_probe_finalize(struct device *dev) Best regards, baolu
On Sat, Aug 12, 2023 at 10:15:42AM +0800, Baolu Lu wrote: > > How about consolidating above into a single helper? > > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1787,6 +1787,21 @@ __iommu_group_alloc_default_domain(struct iommu_group > *group, int req_type) > return __iommu_group_domain_alloc(group, req_type); > } > > +/* > + * Returns the iommu_ops for the devices in an iommu group. > + * > + * It is assumed that all devices in an iommu group are managed by a single > + * IOMMU unit. Therefore, this returns the dev_iommu_ops of the first > device > + * in the group. > + */ > +static const struct iommu_ops *group_iommu_ops(struct iommu_group *group) > +{ > + struct group_device *device; > + > + device = list_first_entry(&group->devices, struct group_device, list); > + return dev_iommu_ops(device->dev); > +} Okay I did this, but it doesn't help as much.. > @@ -2124,13 +2134,9 @@ static struct iommu_domain > *__iommu_domain_alloc(const struct iommu_ops *ops, > static struct iommu_domain * > __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type) > { > - struct device *dev = > - list_first_entry(&group->devices, struct group_device, list) > - ->dev; > - > lockdep_assert_held(&group->mutex); > > - return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type); > + return __iommu_domain_alloc(group_iommu_ops(group), dev, type); > } Since this is all still needed to calculate dev > > +err: > > + if (target_type) { > > + dev_err_ratelimited( > > + gdev->dev, > > + "Device cannot be in %s domain - it is forcing %s\n", > > + iommu_domain_type_str(target_type), > > + iommu_domain_type_str(type)); > > + return -1; > > + } > > + > > + dev_warn( > > + gdev->dev, > > + "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", > > + iommu_domain_type_str(type), dev_name(last_dev), > > + iommu_domain_type_str(best_type)); > > + return 0; > > This doesn't match the commit message, where it states: > > "Arrange things so that if the driver says it needs IDENTITY then > iommu_get_default_domain_type() either fails or returns IDENTITY. > " > > I saw that this change was made in the sequential patch. It is probably > better to put that here? Ah, I went over all this again and decided to try again, it is too complicated. This patch can do what the commit message says and the following patches are even simpler: /* * Combine the driver's choosen def_domain_type across all the devices in a * group. Drivers must give a consistent result. */ static int iommu_get_def_domain_type(struct iommu_group *group, struct device *dev, int cur_type) { const struct iommu_ops *ops = group_iommu_ops(group); int type; if (!ops->def_domain_type) return cur_type; type = ops->def_domain_type(dev); if (!type || cur_type == type) return cur_type; if (!cur_type) return type; dev_err_ratelimited( dev, "IOMMU driver error, requesting conflicting def_domain_type, %s and %s, for devices in group %u.\n", iommu_domain_type_str(cur_type), iommu_domain_type_str(type), group->id); /* * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY * takes precedence. */ if (cur_type || type == IOMMU_DOMAIN_IDENTITY) return IOMMU_DOMAIN_IDENTITY; return cur_type; } /* * A target_type of 0 will select the best domain type. 0 can be returned in * this case meaning the global default should be used. */ static int iommu_get_default_domain_type(struct iommu_group *group, int target_type) { struct device *untrusted = NULL; struct group_device *gdev; int driver_type = 0; lockdep_assert_held(&group->mutex); /* * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an * identity_domain and it will automatically become their default * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain. * Override the selection to IDENTITY. */ if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && IS_ENABLED(CONFIG_IOMMU_DMA))); driver_type = IOMMU_DOMAIN_IDENTITY; } for_each_group_device(group, gdev) { driver_type = iommu_get_def_domain_type(group, gdev->dev, driver_type); if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) { /* * No ARM32 using systems will set untrusted, it cannot * work. */ if (WARN_ON(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))) return -1; untrusted = gdev->dev; } } if (untrusted) { if (driver_type && driver_type != IOMMU_DOMAIN_DMA) { dev_err_ratelimited( untrusted, "Device is not trusted, but driver is overriding group %u to %s, refusing to probe.\n", group->id, iommu_domain_type_str(driver_type)); return -1; } driver_type = IOMMU_DOMAIN_DMA; } if (target_type) { if (driver_type && target_type != driver_type) return -1; return target_type; } return driver_type; } Thanks, Jason
On 2023/8/15 1:25, Jason Gunthorpe wrote: > Ah, I went over all this again and decided to try again, it is too > complicated. This patch can do what the commit message says and the > following patches are even simpler: Yes. This method is more concise. Some nits below. > > /* > * Combine the driver's choosen def_domain_type across all the devices in a > * group. Drivers must give a consistent result. > */ > static int iommu_get_def_domain_type(struct iommu_group *group, > struct device *dev, int cur_type) > { > const struct iommu_ops *ops = group_iommu_ops(group); > int type; > > if (!ops->def_domain_type) > return cur_type; > > type = ops->def_domain_type(dev); > if (!type || cur_type == type) > return cur_type; > if (!cur_type) > return type; > > dev_err_ratelimited( > dev, > "IOMMU driver error, requesting conflicting def_domain_type, %s and %s, for devices in group %u.\n", > iommu_domain_type_str(cur_type), iommu_domain_type_str(type), > group->id); > > /* > * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY > * takes precedence. > */ > if (cur_type || type == IOMMU_DOMAIN_IDENTITY) > return IOMMU_DOMAIN_IDENTITY; No need to check cur_type. It already returned if cur_type is 0. > return cur_type; > } > > /* > * A target_type of 0 will select the best domain type. 0 can be returned in > * this case meaning the global default should be used. > */ > static int iommu_get_default_domain_type(struct iommu_group *group, > int target_type) > { > struct device *untrusted = NULL; > struct group_device *gdev; > int driver_type = 0; > > lockdep_assert_held(&group->mutex); > > /* > * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an > * identity_domain and it will automatically become their default > * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain. > * Override the selection to IDENTITY. > */ > if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { > static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && > IS_ENABLED(CONFIG_IOMMU_DMA))); IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) is duplicate with the condition in the if statement. So only static_assert(!IS_ENABLED(CONFIG_IOMMU_DMA)); ? > driver_type = IOMMU_DOMAIN_IDENTITY; > } > > for_each_group_device(group, gdev) { > driver_type = iommu_get_def_domain_type(group, gdev->dev, > driver_type); No need to call this in the loop body? > > if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) { > /* > * No ARM32 using systems will set untrusted, it cannot > * work. > */ > if (WARN_ON(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))) > return -1; > untrusted = gdev->dev; > } > } > > if (untrusted) { > if (driver_type && driver_type != IOMMU_DOMAIN_DMA) { > dev_err_ratelimited( > untrusted, > "Device is not trusted, but driver is overriding group %u to %s, refusing to probe.\n", > group->id, iommu_domain_type_str(driver_type)); > return -1; > } > driver_type = IOMMU_DOMAIN_DMA; > } > > if (target_type) { > if (driver_type && target_type != driver_type) > return -1; > return target_type; > } > return driver_type; > } Best regards, baolu
On Tue, Aug 15, 2023 at 09:18:59AM +0800, Baolu Lu wrote: > > /* > > * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY > > * takes precedence. > > */ > > if (cur_type || type == IOMMU_DOMAIN_IDENTITY) > > return IOMMU_DOMAIN_IDENTITY; > > No need to check cur_type. It already returned if cur_type is 0. Yep > > return cur_type; > > } > > > > /* > > * A target_type of 0 will select the best domain type. 0 can be returned in > > * this case meaning the global default should be used. > > */ > > static int iommu_get_default_domain_type(struct iommu_group *group, > > int target_type) > > { > > struct device *untrusted = NULL; > > struct group_device *gdev; > > int driver_type = 0; > > > > lockdep_assert_held(&group->mutex); > > > > /* > > * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an > > * identity_domain and it will automatically become their default > > * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain. > > * Override the selection to IDENTITY. > > */ > > if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { > > static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && > > IS_ENABLED(CONFIG_IOMMU_DMA))); > > IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) is duplicate with the condition in the > if statement. So only > static_assert(!IS_ENABLED(CONFIG_IOMMU_DMA)); > ? static_assert doesn't work that way, it ignores its calling context and always checks during compilation, so the duplication is required > > > > for_each_group_device(group, gdev) { > > driver_type = iommu_get_def_domain_type(group, gdev->dev, > > driver_type); > > No need to call this in the loop body? Do need it, this only gets the def_domain_type of a single device so we have to iterate over all the devices in the group to 'reduce' the type for the group. Thanks, Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index c64365169b678d..53174179102d17 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1669,19 +1669,6 @@ struct iommu_group *fsl_mc_device_group(struct device *dev) } EXPORT_SYMBOL_GPL(fsl_mc_device_group); -static int iommu_get_def_domain_type(struct device *dev) -{ - const struct iommu_ops *ops = dev_iommu_ops(dev); - - 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); - - return 0; -} - static struct iommu_domain * __iommu_group_alloc_default_domain(const struct bus_type *bus, struct iommu_group *group, int req_type) @@ -1775,36 +1762,49 @@ static int iommu_bus_notifier(struct notifier_block *nb, static int iommu_get_default_domain_type(struct iommu_group *group, int target_type) { + const struct iommu_ops *ops = dev_iommu_ops( + list_first_entry(&group->devices, struct group_device, list) + ->dev); int best_type = target_type; struct group_device *gdev; struct device *last_dev; + int type; lockdep_assert_held(&group->mutex); - for_each_group_device(group, gdev) { - unsigned int type = iommu_get_def_domain_type(gdev->dev); - - if (best_type && type && best_type != type) { - if (target_type) { - dev_err_ratelimited( - gdev->dev, - "Device cannot be in %s domain\n", - iommu_domain_type_str(target_type)); - return -1; - } - - dev_warn( - gdev->dev, - "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", - iommu_domain_type_str(type), dev_name(last_dev), - iommu_domain_type_str(best_type)); - return 0; + type = best_type; + if (ops->def_domain_type) { + type = ops->def_domain_type(gdev->dev); + if (best_type && type && best_type != type) + goto err; } - if (!best_type) - best_type = type; + + if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) { + type = IOMMU_DOMAIN_DMA; + if (best_type && type && best_type != type) + goto err; + } + best_type = type; last_dev = gdev->dev; } return best_type; + +err: + if (target_type) { + dev_err_ratelimited( + gdev->dev, + "Device cannot be in %s domain - it is forcing %s\n", + iommu_domain_type_str(target_type), + iommu_domain_type_str(type)); + return -1; + } + + dev_warn( + gdev->dev, + "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", + iommu_domain_type_str(type), dev_name(last_dev), + iommu_domain_type_str(best_type)); + return 0; } static void iommu_group_do_probe_finalize(struct device *dev)