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

[Alex Williamson]

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()?

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

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,

Alex

>         if (pasid == IOASID_INVALID)
>                 return -EINVAL;
> 
>          /* Program the device context with the PASID value */
>          ....
> 
> This extends iommu_aux_at(de)tach_device() so that the users could pass
> in an optional device pointer (struct device for vfio/mdev for example),
> and the necessary check and data link could be done.
> 
> Fixes: a3a195929d40b ("iommu: Add APIs for multiple domains per device")
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/iommu.c           | 86 +++++++++++++++++++++++++++++----
>  drivers/vfio/vfio_iommu_type1.c |  5 +-
>  include/linux/iommu.h           | 12 +++--
>  3 files changed, 87 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1ed1e14a1f0c..435835058209 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2723,26 +2723,92 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
>   * This should make us safe against a device being attached to a guest as a
>   * whole while there are still pasid users on it (aux and sva).
>   */
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> +			    struct device *phys_dev, struct device *dev)
>  {
> -	int ret = -ENODEV;
> +	struct iommu_group *group;
> +	int ret;
>  
> -	if (domain->ops->aux_attach_dev)
> -		ret = domain->ops->aux_attach_dev(domain, dev);
> +	if (!domain->ops->aux_attach_dev ||
> +	    !iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
> +		return -ENODEV;
>  
> -	if (!ret)
> -		trace_attach_device_to_domain(dev);
> +	/* Bare use only. */
> +	if (!dev) {
> +		ret = domain->ops->aux_attach_dev(domain, phys_dev);
> +		if (!ret)
> +			trace_attach_device_to_domain(phys_dev);
> +
> +		return ret;
> +	}
> +
> +	/*
> +	 * The caller has created a made-up device (for example, vfio/mdev)
> +	 * and allocated an iommu_group for user level direct assignment.
> +	 * Make sure that the group has only single device and hasn't been
> +	 * attached by any other domain.
> +	 */
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return -ENODEV;
> +
> +	/*
> +	 * Lock the group to make sure the device-count doesn't change while
> +	 * we are attaching.
> +	 */
> +	mutex_lock(&group->mutex);
> +	ret = -EINVAL;
> +	if ((iommu_group_device_count(group) != 1) || group->domain)
> +		goto out_unlock;
> +
> +	ret = -EBUSY;
> +	if (group->default_domain && group->domain != group->default_domain)
> +		goto out_unlock;
> +
> +	ret = domain->ops->aux_attach_dev(domain, phys_dev);
> +	if (!ret) {
> +		trace_attach_device_to_domain(phys_dev);
> +		group->domain = domain;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	iommu_group_put(group);
>  
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
>  
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> +			     struct device *phys_dev, struct device *dev)
>  {
> -	if (domain->ops->aux_detach_dev) {
> -		domain->ops->aux_detach_dev(domain, dev);
> -		trace_detach_device_from_domain(dev);
> +	struct iommu_group *group;
> +
> +	if (WARN_ON_ONCE(!domain->ops->aux_detach_dev))
> +		return;
> +
> +	if (!dev) {
> +		domain->ops->aux_detach_dev(domain, phys_dev);
> +		trace_detach_device_from_domain(phys_dev);
> +
> +		return;
>  	}
> +
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return;
> +
> +	mutex_lock(&group->mutex);
> +	if (WARN_ON(iommu_group_device_count(group) != 1))
> +		goto out_unlock;
> +
> +	domain->ops->aux_detach_dev(domain, phys_dev);
> +	group->domain = NULL;
> +	trace_detach_device_from_domain(phys_dev);
> +
> +out_unlock:
> +	mutex_unlock(&group->mutex);
> +	iommu_group_put(group);
>  }
>  EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
>  
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..d3be45dfa58e 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1635,7 +1635,8 @@ static int vfio_mdev_attach_domain(struct device *dev, void *data)
>  	iommu_device = vfio_mdev_get_iommu_device(dev);
>  	if (iommu_device) {
>  		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> -			return iommu_aux_attach_device(domain, iommu_device);
> +			return iommu_aux_attach_device(domain,
> +						       iommu_device, dev);
>  		else
>  			return iommu_attach_device(domain, iommu_device);
>  	}
> @@ -1651,7 +1652,7 @@ static int vfio_mdev_detach_domain(struct device *dev, void *data)
>  	iommu_device = vfio_mdev_get_iommu_device(dev);
>  	if (iommu_device) {
>  		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> -			iommu_aux_detach_device(domain, iommu_device);
> +			iommu_aux_detach_device(domain, iommu_device, dev);
>  		else
>  			iommu_detach_device(domain, iommu_device);
>  	}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5657d4fef9f2..7da5e67bf7dc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -632,8 +632,10 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features f);
>  int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
>  int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
>  bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> +			    struct device *phys_dev, struct device *dev);
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> +			     struct device *phys_dev, struct device *dev);
>  int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
>  
>  struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> @@ -1007,13 +1009,15 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
>  }
>  
>  static inline int
> -iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_attach_device(struct iommu_domain *domain,
> +			struct device *phys_dev, struct device *dev)
>  {
>  	return -ENODEV;
>  }
>  
>  static inline void
> -iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_detach_device(struct iommu_domain *domain,
> +			struct device *phys_dev, struct device *dev)
>  {
>  }
>