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 _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
