iommu_aux_at(de)tach_device() enhancement

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 <[email protected]>

Cc Alex Williamson <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

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

drivers/iommu/iommu.c +76 -10

index 1ed1e14a1f0c..435835058209 100644
@@ -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);
 

drivers/vfio/vfio_iommu_type1.c +3 -2

index 5e556ac9102a..d3be45dfa58e 100644
@@ -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);
 	}

include/linux/iommu.h +8 -4

index 5657d4fef9f2..7da5e67bf7dc 100644
@@ -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)
 {
 }
 

Quoted 31 lines · On Tue, 7 Jul 2020 09:39:56 +0800 Lu Baolu<[email protected]> 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;

Quoted 5 lines

         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,

Hi Alex,

Thanks a lot for your comments. Please check my reply inline.

Quoted 34 lines · Alex Williamson

Quoted 31 lines · On Tue, 7 Jul 2020 09:39:56 +0800 Lu Baolu<[email protected]> 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;

Quoted 7 lines

Quoted 5 lines

         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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 65 lines · On Wed, 8 Jul 2020 10:53:12 +0800 Lu Baolu <[email protected]> wrote:

Hi Alex,

Thanks a lot for your comments. Please check my reply inline.

Quoted 34 lines · Alex Williamson

Quoted 31 lines · On Tue, 7 Jul 2020 09:39:56 +0800 Lu Baolu<[email protected]> 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;

Quoted 7 lines

Quoted 5 lines

          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.

Quoted 7 lines

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.

Quoted 4 lines

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,

Quoted 8 lines

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.

Quoted 14 lines

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

Quoted 5 lines

          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

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 <[email protected]>

Cc Alex Williamson <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

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

drivers/iommu/iommu.c +0 -0

index 1ed1e14a1f0c..435835058209 100644

drivers/iommu/iommu.c +74 -9

  {
-	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);
 

drivers/vfio/vfio_iommu_type1.c +3 -2

index 5e556ac9102a..d3be45dfa58e 100644
@@ -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);
  	}

include/linux/iommu.h +6 -3

index 5657d4fef9f2..7da5e67bf7dc 100644
@@ -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)
  {