On Wed, 8 Jul 2020 10:53:12 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> Hi Alex,
>
> Thanks a lot for your comments. Please check my reply inline.
>
> On 7/8/20 5:04 AM, Alex Williamson wrote:
> > On Tue, 7 Jul 2020 09:39:56 +0800
> > Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx> 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.
> >>
> >> The iommu_get_domain_for_dev() is a necessary interface for device
> >> drivers that 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);
> > How did we know this was an aux domain? ie. How did we know we could
> > use it with iommu_aux_get_pasid()?
>
> Yes. It's a bit confusing if iommu_get_domain_for_dev() is reused here
> for aux-domain.
>
> >
> > Why did we assume the parent device is the iommu device for the aux
> > domain? Should that level of detail be already known by the aux domain?
> >
> > Nits - The iomu device of an mdev device is found via
> > mdev_get_iommu_device(dev), it should not be assumed to be the parent.
> > The parent of an mdev device is found via mdev_parent_dev(mdev).
>
> My bad. The driver should use mdev_get_iommu_device() instead.
>
> >
> > The leaps in logic here make me wonder if we should instead be exposing
> > more of an aux domain API rather than blurring the differences between
> > these domains. Thanks,
>
> How about add below API?
>
> /**
> * iommu_aux_get_domain_for_dev - get aux domain for a device
> * @dev: the accessory device
> *
> * The caller should pass a valid @dev to iommu_aux_attach_device() before
> * calling this api. Return an attached aux-domain, or NULL otherwise.
That's not necessarily the caller's responsibility, that might happen
elsewhere, this function simply returns an aux domain for the device if
it's attached to one.
> */
> struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
> {
> struct iommu_domain *domain = NULL;
> struct iommu_group *group;
>
> group = iommu_group_get(dev);
> if (!group)
> return NULL;
>
> if (group->aux_domain_attached)
> domain = group->domain;
>
> iommu_group_put(group);
>
> return domain;
> }
> EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
For your example use case, this seems more clear to me. Thanks,
Alex
