Re: [PATCH 02/11] iommu/vt-d: Optimize iotlb_sync_map for non-caching/non-RWBF modes

[Baolu Lu <baolu.lu@linux.intel.com>]

On 7/16/25 22:12, Jason Gunthorpe wrote:
> On Mon, Jul 14, 2025 at 12:50:19PM +0800, Lu Baolu wrote:
>> @@ -1833,6 +1845,8 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
>>   	if (ret)
>>   		goto out_block_translation;
>>   
>> +	domain->iotlb_sync_map |= domain_need_iotlb_sync_map(domain, iommu);
> 
> This has no locking and is in the wrong order anyhow :(
> 
> Any change to how invalidation works has to be done before attaching
> the HW so that the required invalidations are already happening before
> the HW can walk the page table.
> 
> And you need to serialize somehow with concurrent map/unmap as iommufd
> doesn't prevent userspace from racing attach with map/unmap.

domain->iotlb_sync_map does not change the driver's behavior. It simply
indicates that there's no need to waste time calling
cache_tag_flush_range_np(), as it's just a no-op.

> 
> The cache_tag_assign_domain() looks similarly wrong too, it needs to
> start invalidating the cache tag of the new domain, then change the
> context then stop invalidating the cache tag of the old
> domain. Otherwise there are invalidation races.
> 
> Finally, if the HW needs RWBF then this also needs to do the buffer
> flush in this thread before installing the context to prevent a race.
> 
> Overall this dynamic behavior may just be a bad idea, and perhaps you
> can live with domains having the domain->iotlb_sync_map as a static
> property set once during paging domain allocation.
> 
> If the iommu requires iotlb_sync_map but the domain does not have it
> then the attach is rejected. This reduces domain sharing
> possibilities, but maybe that is just fine??

I previously discussed this with Kevin, and we agreed on a phase-by-
phase approach. As I mentioned, domain->iotlb_sync_map is merely a hint
for the driver, preventing it from looping through all cache tags to
determine if any cache invalidation work needs to be performed. We
already know it's predetermined that no work needs to be done.

RWBF is only required on some early implementations where memory
coherence was not yet implemented by the VT-d engine. It should be
difficult to find such systems in modern environments. Thus,
iotlb_sync_map is primarily relevant for nested translation that
utilizes S2 shadowing page tables. This, too, is a legacy feature, as
Intel has supported hardware-assisted nested translation for years.

Making iotlb_sync_map static is a feature, not an optimization. We are
still evaluating the value of this, as it's only truly helpful if there
are real use cases.

Thanks,
baolu