Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 15, 2024 at 08:24:44AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Thursday, September 12, 2024 12:05 PM
> > 
> > On 9/11/24 6:19 PM, Vasant Hegde wrote:
> > >   /**
> > > - * iommu_paging_domain_alloc() - Allocate a paging domain
> > > + * iommu_paging_domain_alloc_flags() - Allocate a paging domain
> > >    * @dev: device for which the domain is allocated
> > > + * @flags: Bitmap of iommufd_hwpt_alloc_flags
> > >    *
> > >    * Allocate a paging domain which will be managed by a kernel driver.
> > Return
> > >    * allocated domain if successful, or a ERR pointer for failure.
> > >    */
> > > -struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
> > > +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device
> > *dev,
> > > +						     unsigned int flags)
> > >   {
> > > +	const struct iommu_ops *ops;
> > > +	struct iommu_domain *domain;
> > > +
> > >   	if (!dev_has_iommu(dev))
> > >   		return ERR_PTR(-ENODEV);
> > >
> > > -	return __iommu_domain_alloc(dev_iommu_ops(dev), dev,
> > IOMMU_DOMAIN_UNMANAGED);
> > > +	ops = dev_iommu_ops(dev);
> > > +
> > > +	if (ops->domain_alloc_paging && !flags)
> > > +		domain = ops->domain_alloc_paging(dev);
> > > +	else if (ops->domain_alloc_user)
> > > +		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> > > +	else if (ops->domain_alloc && !flags)
> > > +		domain = ops-
> > >domain_alloc(IOMMU_DOMAIN_UNMANAGED);
> > > +	else
> > > +		return ERR_PTR(-EOPNOTSUPP);
> > 
> > How about
> > 
> > 	if (flags) {
> > 		if (!ops->domain_alloc_user)
> > 			return ERR_PTR(-EOPNOTSUPP);
> > 		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> > 	} else if (ops->domain_alloc_paging) {
> > 		domain = ops->domain_alloc_paging(dev);
> > 	} else if (ops->domain_alloc) {
> > 		domain = ops-
> > >domain_alloc(IOMMU_DOMAIN_UNMANAGED);
> > 	} else {
> > 		return ERR_PTR(-EOPNOTSUPP);
> > 	}
> > 
> 
> There is slight semantics difference.
> 
> Original code picks domain_alloc_user() if !domain_alloc_paging
> and !flags.
> 
> but your code will pick domain_alloc().
> 
> Based on discussions in this thread domain_alloc_user/ext is the
> preferred one. Then below is the right order?
> 
> 	If (ops->domain_alloc_user) {
> 		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> 	} else if (flags) {
> 		return ERR_PTR(-EOPNOTSUPP);
> 	} else if (ops->domain_alloc_paging) {
> 		domain = ops->domain_alloc_paging(dev);
>  	} else if (ops->domain_alloc) {
>  		domain = ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
>  	} else {
>  		return ERR_PTR(-EOPNOTSUPP);
>  	}

This can work too, but I think we need a few more patches still to get
there?

The order above makes sense if drivers implement both
ops->domain_alloc_paging/user, then you'd want to prefer the more
narrow one.

Once drivers don't implement both then it would make sense to switch
to your version

Jason