Re: [PATCH v2 23/25] iommu: Add ops->domain_alloc_paging()

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jun 01, 2023 at 08:17:56PM +0100, Robin Murphy wrote:
> On 2023-05-16 01:00, Jason Gunthorpe wrote:
> > This callback requests the driver to create only a __IOMMU_DOMAIN_PAGING
> > domain, so it saves a few lines in a lot of drivers needlessly checking
> > the type.
> > 
> > More critically, this allows us to sweep out all the
> > IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA checks from a lot of the
> > drivers, simplifying what is going on in the code and ultimately removing
> > the now-unused special cases in drivers where they did not support
> > IOMMU_DOMAIN_DMA.
> > 
> > domain_alloc_paging() should return a struct iommu_domain that is
> > functionally compatible with ARM_DMA_USE_IOMMU, dma-iommu.c and iommufd.
> > 
> > Be forwards looking and pass in a 'struct device *' argument. We can
> > provide this when allocating the default_domain. No drivers will look at
> > this.
> 
> As mentioned before, we already know we're going to need additional flags
> (and possibly data) to cover the existing set_pgtable_quirks use-case plus
> new stuff like the proposed dirty-tracking enable, so I'd be inclined to
> either add an extensible structure argument now to avoid future
> churn,

I think I said this doesn't really work out because you still have to
go into every driver and code up a 'does not support' check and fail
if any new extension is added. Basically the same churn as adding a
function argument.

Adding more ops is a possible smoother way to support this. Keep
alloc_paging() for these simple drivers and we can add an op with a
big signature and structure or whatever for the fewer fully featured
drivers.

This way the core code can do the 'do not support' checks in one place
based on the set ops and we keep boiler plate code out of alot of
drivers.

> or just not bother adding the device argument either until drivers
> can actually use it.

I debated this, I figured we'd use it pretty quickly, but certainly it
can be taken out.

> > @@ -1995,14 +1995,25 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
> >   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
> >   static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
> > +						 struct device *dev,
> >   						 unsigned int type)
> >   {
> >   	struct iommu_domain *domain;
> >   	if (type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
> >   		return ops->identity_domain;
> > +	else if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
> > +		 ops->domain_alloc_paging) {
> > +		/*
> > +		 * For now exclude DMA_FQ since it is still a driver policy
> > +		 * decision through domain_alloc() if we can use FQ mode.
> > +		 */
> 
> That's sorted now, so the type test can neatly collapse down to "type &
> __IOMMU_DOMAIN_PAGING".

Thanks, I rebase it on Joerg's tree with that series and fix it up

Jason