Messages in this thread | | | Subject | Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension | From | Lu Baolu <> | Date | Thu, 9 Jul 2020 08:37:08 +0800 |
| |
Hi Alex,
On 7/9/20 3:07 AM, Alex Williamson wrote: > On Wed, 8 Jul 2020 10:53:12 +0800 > Lu Baolu <baolu.lu@linux.intel.com> 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@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. > > 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.
Yes. Fair enough. This piece of comments will be removed.
> >> */ >> 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, >
Okay, thank you!
> Alex
Best regards, baolu
| |