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

[Robin Murphy <robin.murphy@arm.com>]

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);
> +}
> +