[PATCH v7 10/28] iommufd/access: Bypass access->ops->unmap for internal use

[Tian, Kevin]

> From: Nicolin Chen <nicolinc at nvidia.com>
> Sent: Thursday, July 3, 2025 4:12 AM
> 
> On Wed, Jul 02, 2025 at 09:45:26AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc at nvidia.com>
> > > Sent: Friday, June 27, 2025 3:35 AM
> > >
> > > +int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned
> long
> > > iova,
> > > +				unsigned long length)
> > >  {
> > >  	struct iommufd_ioas *ioas =
> > >  		container_of(iopt, struct iommufd_ioas, iopt);
> > >  	struct iommufd_access *access;
> > >  	unsigned long index;
> > > +	int ret = 0;
> > >
> > >  	xa_lock(&ioas->iopt.access_list);
> > > +	/* Bypass any unmap if there is an internal access */
> > > +	xa_for_each(&ioas->iopt.access_list, index, access) {
> > > +		if (iommufd_access_is_internal(access)) {
> > > +			ret = -EBUSY;
> > > +			goto unlock;
> > > +		}
> > > +	}
> > > +
> >
> > hmm all those checks are per iopt. Could do one-off check in
> > iopt_unmap_iova_range() and store the result in a local flag.
> >
> > Then use that flag to decide whether to return -EBUSY if
> > area->num_accesses is true in the loop.
> 
> I don't quite follow this...
> 
> Do you suggest to move this xa_for_each to iopt_unmap_iova_range?

yes

> 
> What's that local flag used for?
> 

I meant something like below:

iopt_unmap_iova_range()
{
	bool internal_access = false;

	down_read(&iopt->domains_rwsem);
	down_write(&iopt->iova_rwsem);
	/* Bypass any unmap if there is an internal access */
	xa_for_each(&iopt->access_list, index, access) {
		if (iommufd_access_is_internal(access)) {
			internal_access = true;
			break;
		}
	}

	while ((area = iopt_area_iter_first(iopt, start, last))) {
		if (area->num_access) {
			if (internal_access) {
				rc = -EBUSY;
				goto out_unlock_iova;
			}
			up_write(&iopt->iova_rwsem);
			up_read(&iopt->domains_rwsem);
			iommufd_access_notify_unmap(iopt, area_first, length);	
		}
	}
}

it checks the access_list in the common path, but the cost should be
negligible when there is no access attached to this iopt. The upside
is that now unmap is denied explicitly in the area loop instead of 
still trying to unmap and then handling errors.

but the current way is also fine. After another thought I'm neutral
to it. 😊