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

From: Robin Murphy
Date: Tue May 24 2022 - 10:36:25 EST


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