[PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement

[PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement

[Lu Baolu]

This series aims to enhance the iommu_aux_at(de)tach_device() api's
so that some generic iommu api's like iommu_get_domain_for_dev() could
also work for vfio/mdev device.

The initial version of this series was post at

https://lkml.org/lkml/2020/6/26/1118

This version is evolved according to Robin's feedback.

Your comments are very appreciated.

Best regards,
baolu

Lu Baolu (2):
  iommu: iommu_aux_at(de)tach_device() extension
  iommu: Add aux_domain_attached flag to iommu_group

 drivers/iommu/iommu.c           | 98 +++++++++++++++++++++++++++++----
 drivers/vfio/vfio_iommu_type1.c |  5 +-
 include/linux/iommu.h           | 12 ++--
 3 files changed, 99 insertions(+), 16 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Lu Baolu]

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);
        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@arm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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)
 {
 }
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/2] iommu: Add aux_domain_attached flag to iommu_group

[Lu Baolu]

The normal domain at(de)tach is parallel with aux-domain at(de)tach. In
another word, once an iommu_group is attached through the normal domain
attach api's, it should not go through the aux-domain at(de)tach api's
until the domain is detached. And, vice versa.

Currently, we prohibit an iommu_group to go through aux-domain api's if
group->domain != NULL; but we don't check aux-domain attachment in the
normal attach api's. This marks an iommu_group after an aux-domain is
attached, so that normal domain at(de)tach api's should never be used
after that.

Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 435835058209..3e7489ea2010 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,7 @@ struct iommu_group {
 	struct iommu_domain *default_domain;
 	struct iommu_domain *domain;
 	struct list_head entry;
+	unsigned int aux_domain_attached:1;
 };
 
 struct group_device {
@@ -2074,6 +2075,9 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 {
 	int ret;
 
+	if (group->aux_domain_attached)
+		return -EINVAL;
+
 	if (group->default_domain && group->domain != group->default_domain)
 		return -EBUSY;
 
@@ -2111,6 +2115,9 @@ static void __iommu_detach_group(struct iommu_domain *domain,
 {
 	int ret;
 
+	if (WARN_ON(group->aux_domain_attached))
+		return;
+
 	if (!group->default_domain) {
 		__iommu_group_for_each_dev(group, domain,
 					   iommu_group_do_detach_device);
@@ -2769,6 +2776,7 @@ int iommu_aux_attach_device(struct iommu_domain *domain,
 	if (!ret) {
 		trace_attach_device_to_domain(phys_dev);
 		group->domain = domain;
+		group->aux_domain_attached = true;
 	}
 
 out_unlock:
@@ -2802,8 +2810,12 @@ void iommu_aux_detach_device(struct iommu_domain *domain,
 	if (WARN_ON(iommu_group_device_count(group) != 1))
 		goto out_unlock;
 
+	if (WARN_ON(!group->aux_domain_attached))
+		goto out_unlock;
+
 	domain->ops->aux_detach_dev(domain, phys_dev);
 	group->domain = NULL;
+	group->aux_domain_attached = false;
 	trace_detach_device_from_domain(phys_dev);
 
 out_unlock:
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

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@arm.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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)
>  {
>  }
>  

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Lu Baolu]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Alex Williamson]

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.

>   */
> 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,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Lu Baolu]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Lu Baolu]

On 2020/7/7 9:39, Lu Baolu 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);
>          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@arm.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   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)

I hit a lock issue during internal test. Will fix it in the next
version.

Best regards,
baolu

>   {
> -	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)
>   {
>   }
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu