Messages in this thread | | | Date | Wed, 25 May 2022 14:20:29 +0800 | Subject | Re: [PATCH v7 03/10] iommu/sva: Add iommu_sva_domain support | From | Baolu Lu <> |
| |
Hi Robin,
On 2022/5/24 22:36, Robin Murphy wrote: > 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
Agreed with above comments. Thanks! I will post an additional patch for review later.
> 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?
"iova_cookie" is mutually exclusive with SVA, but I am not sure about the fault handler stuff.
Did you mean @handler and @handler_token staffs below?
struct iommu_domain { unsigned type; const struct iommu_domain_ops *ops; unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ iommu_fault_handler_t handler; void *handler_token; struct iommu_domain_geometry geometry; struct iommu_dma_cookie *iova_cookie; };
Is it only for DMA domains? From the point view of IOMMU faults, it seems to be generic.
Best regards, baolu
| |