On Tue, Oct 17, 2023 at 12:58:39PM -0700, Nicolin Chen wrote:
On Tue, Oct 17, 2023 at 12:53:01PM -0300, Jason Gunthorpe wrote:
On Tue, Oct 17, 2023 at 08:52:49AM +0000, Tian, Kevin wrote:
I prefer to removing enforce_cc in attach fn completely then no parent trick in this patch. Just keep it in hwpt_alloc and leave to iommu driver to figure out the attaching compatibility:
You are basically saying to set the cc mode during creation because we know the idev at that moment and can tell if it should be on/off?
It seems reasonable, but I can't remember why it is in the attach path at the moment.
This was the commit adding it in the alloc path: https://lore.kernel.org/linux-iommu/8e897628-61fa-b3fb-b609-44eeda11b45e@arm...
The older code was doing a hwpt "upgrade" from !cc to cc:
/** Try to upgrade the domain we have, it is an iommu driver bug to* report IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail* enforce_cache_coherency when there are no devices attached to the* domain.*/if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) {if (hwpt->domain->ops->enforce_cache_coherency)hwpt->enforce_cache_coherency =hwpt->domain->ops->enforce_cache_coherency(hwpt->domain);If we remove the enforce_cc call in the attach path and let the driver decide whether to enforce or reject in attach_dev calls, there seems to be no point in tracking an enforce_cache_coherency flag in the IOMMUFD pathway but only for the VFIO_DMA_CC_IOMMU extension check in the vfio-compat pathway?
I think the purpose of this code is to try to minimize the number of domains.
Otherwise we have a problem where the order devices are attached to the domain decides how many domains you get. ie the first device attached does not want CC (but is compatible with it) so we create a non-CC domain
Then later we attach a device that does want CC and now we are forced to create a second iommu domain when upgrading the first domain would have been fine.
Hotplug is another scenario (though Intel driver does not support it, and it looks broken)
Really, I hate this CC mechanism. It is only for Intel, can we just punt it to userspace and have an intel 'want cc flag' for the entire nesting path and forget about it?
Jason