Re: [PATCH 1/2] iommu: Add iommu_group_get/set_domain()

[Lu Baolu]

Hi Robin,

On 6/29/20 7:56 PM, Robin Murphy wrote:
> On 2020-06-27 04:15, Lu Baolu wrote:
> > The hardware assistant vfio mediated device is a use case of iommu
> > aux-domain. The interactions between vfio/mdev and iommu during mdev
> > creation and passthr are:
> >
> > - Create a group for mdev with iommu_group_alloc();
> > - Add the device to the group with
> > ÂÂÂÂÂÂÂÂ group = iommu_group_alloc();
> > ÂÂÂÂÂÂÂÂ if (IS_ERR(group))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return PTR_ERR(group);
> >
> > ÂÂÂÂÂÂÂÂ ret = iommu_group_add_device(group, &mdev->dev);
> > ÂÂÂÂÂÂÂÂ if (!ret)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_info(&mdev->dev, "MDEV: group_id = %d\n",
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iommu_group_id(group));
> > - Allocate an aux-domain
> > ÂÂÂÂiommu_domain_alloc()
> > - Attach the aux-domain to the physical device from which the mdev is
> > ÂÂ created.
> > ÂÂÂÂiommu_aux_attach_device()
> >
> > In the whole process, an iommu group was allocated for the mdev and an
> > iommu domain was attached to the group, but the group->domain leaves
> > NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
> >
> > This adds iommu_group_get/set_domain() so that group->domain could be
> > managed whenever a domain is attached or detached through the aux-domain
> > api's.
>
> Letting external callers poke around directly in the internals of 
> iommu_group doesn't look right to me.

Unfortunately, it seems that the vifo iommu abstraction is deeply bound
to the IOMMU subsystem. We can easily find other examples:

iommu_group_get/set_iommudata()
iommu_group_get/set_name()
...

> If a regular device is attached to one or more aux domains for PASID 
> use, iommu_get_domain_for_dev() is still going to return the primary 
> domain, so why should it be expected to behave differently for mediated

Unlike the normal device attach, we will encounter two devices when it
comes to aux-domain.

- Parent physical device - this might be, for example, a PCIe device
with PASID feature support, hence it is able to tag an unique PASID
for DMA transfers originated from its subset. The device driver hence
is able to wrapper this subset into an isolated:

- Mediated device - a fake device created by the device driver mentioned
above.

Yes. All you mentioned are right for the parent device. But for mediated
device, iommu_get_domain_for_dev() doesn't work even it has an valid
iommu_group and iommu_domain.

iommu_get_domain_for_dev() is a necessary interface for device drivers
which want to support aux-domain. For example,

          struct iommu_domain *domain;
          struct device *dev = mdev_dev(mdev);
	  unsigned long pasid;

          domain = iommu_get_domain_for_dev(dev);
          if (!domain)
                  return -ENODEV;

          pasid = iommu_aux_get_pasid(domain, dev->parent);
	  if (pasid == IOASID_INVALID)
		  return -EINVAL;

	  /* Program the device context with the PASID value */
	  ....

Without this fix, iommu_get_domain_for_dev() always returns NULL and the
device driver has no means to support aux-domain.

Best regards,
baolu

> devices? AFAICS it's perfectly legitimate to have no primary domain if 
> traffic-without-PASID is invalid.
>
> Robin.