Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension

[Lu Baolu <baolu.lu@linux.intel.com>]

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