Re: [PATCH v7 03/10] iommu/sva: Add iommu_sva_domain support

Robin Murphy
2022-05-24T14:36:25Z

On 2022-05-19 08:20, Lu Baolu wrote:
[...]
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 106506143896..210c376f6043 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -69,3 +69,51 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
> return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> }
> EXPORT_SYMBOL_GPL(iommu_sva_find);
> +
> +/*
> + * IOMMU SVA driver-oriented interfaces
> + */
> +struct iommu_domain *
> +iommu_sva_alloc_domain(struct bus_type *bus, struct mm_struct *mm)

Argh, please no new bus-based external interfaces! Domain allocation
needs to resolve to the right IOMMU instance to solve a number of
issues, and cleaning up existing users of iommu_domain_alloc() to
prepare for that is already hard enough. This is arguably even more
relevant here than for other domain types, since SVA support is more
likely to depend on specific features that can vary between IOMMU
instances even with the same driver. Please make the external interface
take a struct device, then resolve the ops through dev->iommu.

Further nit: the naming inconsistency bugs me a bit -
iommu_sva_domain_alloc() seems more natural. Also I'd question the
symmetry vs. usability dichotomy of whether we *really* want two
different free functions for a struct iommu_domain pointer, where any
caller which might mix SVA and non-SVA usage then has to remember how
they allocated any particular domain :/

> +{
> + struct iommu_sva_domain *sva_domain;
> + struct iommu_domain *domain;
> +
> + if (!bus->iommu_ops || !bus->iommu_ops->sva_domain_ops)
> + return ERR_PTR(-ENODEV);
> +
> + sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
> + if (!sva_domain)
> + return ERR_PTR(-ENOMEM);
> +
> + mmgrab(mm);
> + sva_domain->mm = mm;
> +
> + domain = &sva_domain->domain;
> + domain->type = IOMMU_DOMAIN_SVA;
> + domain->ops = bus->iommu_ops->sva_domain_ops;

I'd have thought it would be logical to pass IOMMU_DOMAIN_SVA to the
normal domain_alloc call, so that driver-internal stuff like context
descriptors can be still be hung off the domain as usual (rather than
all drivers having to implement some extra internal lookup mechanism to
handle all the SVA domain ops), but that's something we're free to come
back and change later. FWIW I'd just stick the mm pointer in struct
iommu_domain, in a union with the fault handler stuff and/or iova_cookie
- those are mutually exclusive with SVA, right?

Cheers,
Robin.

> +
> + return domain;
> +}
> +
> +void iommu_sva_free_domain(struct iommu_domain *domain)
> +{
> + struct iommu_sva_domain *sva_domain = to_sva_domain(domain);
> +
> + mmdrop(sva_domain->mm);
> + kfree(sva_domain);
> +}
> +