Re: [PATCH v6 08/25] iommu: Reorganize iommu_get_default_domain_type() to respect def_domain_type()

Baolu Lu
2023-08-15T01:19:58Z

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