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
