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@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.
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
