Re: [PATCH V4 3/4] iommufd: Add IOMMU_IOAS_CHANGE_PROCESS

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Nov 13, 2024 at 08:25:45AM +0000, Tian, Kevin wrote:
> > From: Steve Sistare <steven.sistare@oracle.com>
> > Sent: Friday, November 8, 2024 5:47 AM
> > 
> > +int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
> > +{
> > +	struct iommu_ioas_change_process *cmd = ucmd->cmd;
> > +	struct iommufd_ctx *ictx = ucmd->ictx;
> > +	unsigned long all_npinned[IOPT_PAGES_ACCOUNT_MODE_NUM] = {};
> > +	struct iommufd_ioas *ioas;
> > +	struct iopt_area *area;
> > +	struct iopt_pages *pages;
> > +	struct xarray ioas_list;
> > +	unsigned long index;
> > +	int rc;
> > +
> > +	if (cmd->__reserved)
> > +		return -EOPNOTSUPP;
> > +
> > +	xa_init(&ioas_list);
> > +	rc = iommufd_take_all_iova_rwsem(ictx, &ioas_list);
> > +	if (rc)
> > +		return rc;
> > +
> > +	for_each_ioas_area(&ioas_list, index, ioas, area)  {
> > +		if (area->pages->type != IOPT_ADDRESS_FILE) {
> > +			rc = -EINVAL;
> > +			goto out;
> > +		}
> > +	}
> 
> this and next loop could be merged into one. It's not a bad idea
> to add cost to a wrong operation while making it slightly more
> efficient in a normal path.

The next loop messes up pages->last_npinned, so if you merge them then
you also have to do a partial restoration of last_npinned and when I
wrote this I thought that restoration was complex enough to be not
worth the optimization..

Jason