Messages in this thread | | | Subject | Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension | From | Lu Baolu <> | Date | Wed, 8 Jul 2020 10:53:12 +0800 |
| |
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@linux.intel.com> 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. */ 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);
Best regards, baolu
| |