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

From: Lu Baolu
Date: Mon Jun 29 2020 - 21:07:40 EST


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.