RE: [RFC PATCH v2 08/10] vfio/type1: Add domain at(de)taching group helpers

[Tian, Kevin]

> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@xxxxxxx]
> Sent: Thursday, September 13, 2018 1:54 AM
> 
> On 12/09/2018 06:02, Lu Baolu wrote:
> > Hi,
> >
> > On 09/11/2018 12:23 AM, Jean-Philippe Brucker wrote:
> >> On 30/08/2018 05:09, Lu Baolu wrote:
> >>> +static int vfio_detach_aux_domain(struct device *dev, void *data)
> >>> +{
> >>> +	struct iommu_domain *domain = data;
> >>> +
> >>> +	vfio_mdev_set_aux_domain(dev, NULL);
> >>> +	iommu_detach_device(domain, dev->parent);
> >>
> >> I think that's only going to work for vt-d, which doesn't use a
> >> default_domain. For other drivers, iommu.c ends up calling
> >> domain->ops->attach_dev(default_domain, dev) here, which isn't what
> we want.
> >
> > This doesn't break any functionality. It takes effect only if iommu
> > hardware supports finer granular translation and advertises it through
> > the API.>
> >> That was my concern with reusing attach/detach_dev callbacks for
> PASID
> >> management. The attach_dev code of IOMMU drivers already has to
> deal
> >> with toggling between default and unmanaged domain. Dealing with
> more
> >> state transitions in the same path is going to be difficult.
> >
> > Let's consider it from the API point of view.
> >
> > We have iommu_a(de)ttach_device() APIs to attach or detach a domain
> > to/from a device. We should avoid applying a limitation of "these are
> > only for single domain case, for multiple domains, use another API".
> 
> That's an idealized view of the API, the actual semantics aren't as
> simple. For IOMMU drivers that implement IOMMU_DOMAIN_DMA in their
> domain_alloc operation (Arm SMMU, AMD IOMMU, ...), attach_dev
> attaches
> an unmanaged domain, detach_dev reattaches the default DMA domain,
> and
> the detach_dev IOMMU op is not called. We can't change that now, so it's
> difficult to add more functionality to attach_dev and detach_dev.
> 

Now we have four possible usages on a(de)ttach_device:

1) Normal DMA API path i.e. IOMMU_DOMAIN_DMA, for DMA operations
in host/parent device driver;

2) VFIO passthru path, when the physical device is assigned to user space
or guest driver

3) mdev passthru path 1, when mdev is assigned to user space or guest
driver. Here mdev is a wrap on random PCI device

4) mdev passthru path 2, when mdev is assigned to user space or guest
driver. Here mdev is in a smaller granularity (e.g. tagged by PASID) as
supported by vt-d scalable mode

1) and 2) are existing usages. What you described (unmanaged vs. default)
falls into this category.

3) is actually same as 2) in IOMMU layer. sort of passing through DMA
capability to guest. Though there is still a parent driver, the parent driver
itself should not do DMA - i.e. unmanaged in host side.

4) is a new code path introduced in IOMMU layer, which is what
vfio_a(de)tach_aux_domain is trying to address. In this case parent device
can still have its own DMA capability, using existing code path 1) (thus
default domain still applies). and the parent device can have multiple 
aux domains (and associated structures), using code path 4).

I didn't see how this new code path interferes with default domain 
concept in existing path. In the future if ARM or other architectures
plan to support similar scalable mode capability, only the new code
path should be tweaked.

Thanks
Kevin