Messages in this thread | | | Date | Tue, 25 Jan 2022 11:00:46 -0400 | From | Jason Gunthorpe <> | Subject | Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops |
| |
On Tue, Jan 25, 2022 at 02:23:52PM +0000, Robin Murphy wrote: > On 2022-01-25 06:27, Lu Baolu wrote:
> Where it's just about which operations are valid for which domains, it's > even simpler for the core interface wrappers to validate the domain type, > rather than forcing drivers to implement multiple ops structures purely for > the sake of having different callbacks populated. We already have this in > places, e.g. where iommu_map() checks for __IOMMU_DOMAIN_PAGING.
In my experience it is usually much clearer to directly test the op for NULL to know if a feature is supported than to invent flags to do the same test. eg ops->map/etc == NULL means no paging.
I think we should not be afraid to have multiple ops in drivers for things that are actually different in the driver. This is usually a net win vs tying to handle all the cases with different 'if' flows.
eg identity domains and others really would ideally eventually have a NULL ops for map/unmap too.
The 'type' should conceptually be part of the ops, not the mutable struct - but we don't have to get there all at once.
> Paging domains are also effectively the baseline level of IOMMU API > functionality. All drivers support them, and for the majority of drivers > it's all they will ever support. Those drivers really don't benefit from any > of the churn and boilerplate in this patch as-is, and it's so easy to > compromise with a couple of lines of core code to handle the common case by > default when the driver *isn't* one of the handful which ever actually cares > to install their own per-domain ops. Consider how much cleaner this patch > would look if the typical driver diff could be something completely minimal > like this:
It is clever, but I'm not sure if hoisting a single assignment out of the driver is worth the small long term complexity of having different driver flows?
Jason
| |