Re: [PATCH 0/7] iommu cleanup and refactoring

[Robin Murphy]

On 2022-01-26 13:27, Jason Gunthorpe via iommu wrote:
> On Wed, Jan 26, 2022 at 09:51:36AM +0800, Lu Baolu wrote:
>
> > > > they are fundamentally different things in their own right, and the ideal
> > > > API should give us the orthogonality to also bind a device to an SVA domain
> > > > without PASID (e.g. for KVM stage 2, or userspace assignment of simpler
> > > > fault/stall-tolerant devices), or attach PASIDs to regular iommu_domains.
> > >
> > > Yes, these are orthogonal things. A iommu driver that supports PASID
> > > ideally should support PASID enabled attach/detatch for every
> > > iommu_domain type it supports.
> > >
> > > SVA should not be entangled with PASID beyond that SVA is often used
> > > with PASID - a SVA iommu_domain should be fully usable with a RID too.
> >
> > The prototype of PASID enabled attach/detach ops could look like:
> >
> >         int (*attach_dev_pasid)(struct iommu_domain *domain,
> >                                 struct device *dev, ioasid_t id);
> >         void (*detach_dev_pasid)(struct iommu_domain *domain,
> >                                  struct device *dev, ioasid_t id);
>
> It seems reasonable and straightforward to me..
>
> These would be domain ops?
>   
> > But the iommu driver should implement different callbacks for
> >
> > 1) attaching an IOMMU DMA domain to a PASID on device;
> >     - kernel DMA with PASID
> >     - mdev-like device passthrough
> >     - etc.
> > 2) attaching a CPU-shared domain to a PASID on device;
> >     - SVA
> >     - guest PASID
> >     - etc.
>
> But this you mean domain->ops would be different? Seems fine, up to
> the driver.

Indeed, it makes little practical difference whether the driver provides 
distinct sets of ops or just common callbacks which switch on the domain 
type internally. I expect that's largely going to come down to personal 
preference and how much internal driver code is common between the paths.

> I'd hope to see some flow like:
>
>   domain = device->bus->iommu_ops->alloc_sva_domain(dev)
>   domain->ops->attach_dev_pasid(domain, dev, current->pasid)
>
> To duplicate the current SVA APIs

+1 - I'd concur that attach/detach belong as domain ops in general. 
There's quite a nice logical split forming here where domain ops are the 
ones that make sense for external subsystems and endpoint drivers to use 
too, while device ops, with the sole exception of domain_alloc, are 
IOMMU API internals. By that reasoning, attach/bind/etc. are firmly in 
the former category.

Thanks,
Robin.