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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 17, 2025 at 10:40:01AM +0800, Baolu Lu wrote:
> 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.

Of course it changes the behavior, it changes what the invalidation
callback does.

Without locking you have a race situation where a PGD is visible to HW
that requires extra flushing and the SW is not doing the extra
flushing.

Before any PGD is made visible to the HW the software must ensure all
the required invalidations are happening.

> 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.

The iteration though the cache tags is done inside a lock so it
doesn't have this race (it has the issue I mentioned setting up the
cache tage list though).

> 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.

Then I would set it at domain creation time, check it during attach,
and remove this race.

Jason