iommu aux-domain APIs extensions

PATCH Superseded Lu Baolu 6 participants 36 messages Jul 14, 2020 – Aug 3, 2020

Subsystems (3)

KVM IOMMU VFIO driver

lore

Share & tools

lore

Lu Baolu

author

2020-07-14 06:01 UTC #0 lore raw

v3 cover iommu aux-domain APIs extensions

This series aims to extend the IOMMU aux-domain API set so that it could be more friendly to vfio/mdev usage. The interactions between vfio/mdev and iommu during mdev creation and passthr are:

1. Create a group for mdev with iommu_group_alloc();
2. 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));

3. Allocate an aux-domain with iommu_domain_alloc();
4. Attach the aux-domain to the iommu_group.

       iommu_group_for_each_dev {
               if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
                       return iommu_aux_attach_device(domain, iommu_device);
               else
                       return iommu_attach_device(domain, iommu_device);
        }

where, iommu_device is the aux-domain-capable device. The mdev's in the group are all derived from it.

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() (or other similar interfaces) doesn't work anymore.

The iommu_get_domain_for_dev() is a necessary interface for device drivers that want to support vfio/mdev based aux-domain. For example,

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

We tried to address this by extending iommu_aux_at(de)tach_device() so that the users could pass in an optional device pointer (for example vfio/mdev). (v2 of this series)

https://lore.kernel.org/linux-iommu/[email protected]/

But that will cause a lock issue as group->mutex has been applied in iommu_group_for_each_dev(), but has to be reapplied again in the iommu_aux_attach_device().

This version tries to address this by introducing two new APIs into the aux-domain API set:

/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group which
* contains sub-devices (for example mdevs)
* derived from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)

/**
* iommu_aux_detach_group - detach an aux-domain from an iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
* iommu_aux_attach_group().
*/
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)

This version is evolved according to feedbacks from Robin(v1) and Alex(v2). Your comments are very appreciated.

Best regards,
baolu

---
Change log:
- v1->v2:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Suggested by Robin.

- v2->v3:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Suggested by Alex

Lu Baolu (4):
iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
iommu: Add iommu_aux_at(de)tach_group()
iommu: Add iommu_aux_get_domain_for_dev() vfio/type1: Use iommu_aux_at(de)tach_group() APIs

 drivers/iommu/iommu.c           | 92 ++++++++++++++++++++++++++++++---
 drivers/vfio/vfio_iommu_type1.c | 44 +++-------------
 include/linux/iommu.h           | 24 +++++++++
 3 files changed, 116 insertions(+), 44 deletions(-)

Signature · 1 line

2.17.1

Lu Baolu

author

2020-07-14 06:01 UTC #1 lore raw

PATCH v3 1/4 iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's

The iommu aux-domain api's work only when IOMMU_DEV_FEAT_AUX is enabled for the device. Add this check to avoid misuse.

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

---
 drivers/iommu/iommu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/iommu/iommu.c +9 -7

index 1ed1e14a1f0c..e1fdd3531d65 100644
@@ -2725,11 +2725,13 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
  */
 int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
 {
-	int ret = -ENODEV;
+	int ret;
 
-	if (domain->ops->aux_attach_dev)
-		ret = domain->ops->aux_attach_dev(domain, dev);
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_attach_dev)
+		return -ENODEV;
 
+	ret = domain->ops->aux_attach_dev(domain, dev);
 	if (!ret)
 		trace_attach_device_to_domain(dev);

@@ -2748,12 +2750,12 @@ EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
 
 int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 {
-	int ret = -ENODEV;
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_get_pasid)
+		return -ENODEV;
 
-	if (domain->ops->aux_get_pasid)
-		ret = domain->ops->aux_get_pasid(domain, dev);
+	return domain->ops->aux_get_pasid(domain, dev);
 
-	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);

Signature · 1 line

2.17.1

Alex Williamson

2020-07-29 20:03 UTC #2 lore raw

Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's

Quoted 2 lines · On Tue, 14 Jul 2020 13:57:00 +0800 Lu Baolu <[email protected]> wrote:

The iommu aux-domain api's work only when IOMMU_DEV_FEAT_AUX is enabled for the device. Add this check to avoid misuse.

Shouldn't this really be the IOMMU driver's responsibility to test? If nothing else, iommu_dev_feature_enabled() needs to get the iommu_ops from dev->bus->iommu_ops, which is presumably the same iommu_ops we're then calling from domain->ops to attach/detach the device, so it'd be more efficient for the IOMMU driver to error on devices that don't support aux. Thanks,

Alex

Quoted 43 lines

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

---
 drivers/iommu/iommu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/iommu/iommu.c +9 -7

index 1ed1e14a1f0c..e1fdd3531d65 100644
@@ -2725,11 +2725,13 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
  */
 int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
 {
-	int ret = -ENODEV;
+	int ret;
 
-	if (domain->ops->aux_attach_dev)
-		ret = domain->ops->aux_attach_dev(domain, dev);
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_attach_dev)
+		return -ENODEV;
 
+	ret = domain->ops->aux_attach_dev(domain, dev);
 	if (!ret)
 		trace_attach_device_to_domain(dev);

@@ -2748,12 +2750,12 @@ EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
 
 int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 {
-	int ret = -ENODEV;
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_get_pasid)
+		return -ENODEV;
 
-	if (domain->ops->aux_get_pasid)
-		ret = domain->ops->aux_get_pasid(domain, dev);
+	return domain->ops->aux_get_pasid(domain, dev);
 
-	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);

Lu Baolu

author

2020-07-30 01:51 UTC #3 lore raw

Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's

Hi Alex,

Quoted 6 lines · Alex Williamson

Quoted 2 lines · On Tue, 14 Jul 2020 13:57:00 +0800 Lu Baolu <[email protected]> wrote:
The iommu aux-domain api's work only when IOMMU_DEV_FEAT_AUX is enabled for the device. Add this check to avoid misuse.

Quoted 6 lines

Shouldn't this really be the IOMMU driver's responsibility to test? If nothing else, iommu_dev_feature_enabled() needs to get the iommu_ops from dev->bus->iommu_ops, which is presumably the same iommu_ops we're then calling from domain->ops to attach/detach the device, so it'd be more efficient for the IOMMU driver to error on devices that don't support aux. Thanks,

Fair enough. The vendor iommu driver always knows the status of aux- domain support. So this check is duplicated. I will drop this patch.

Best regards,
baolu

Quoted 47 lines

Alex

Quoted 43 lines

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

---
  drivers/iommu/iommu.c | 16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)

drivers/iommu/iommu.c +9 -7

index 1ed1e14a1f0c..e1fdd3531d65 100644
@@ -2725,11 +2725,13 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
   */
  int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
  {
-	int ret = -ENODEV;
+	int ret;
 
-	if (domain->ops->aux_attach_dev)
-		ret = domain->ops->aux_attach_dev(domain, dev);
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_attach_dev)
+		return -ENODEV;
 
+	ret = domain->ops->aux_attach_dev(domain, dev);
  	if (!ret)
  		trace_attach_device_to_domain(dev);

@@ -2748,12 +2750,12 @@ EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
 
  int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
  {
-	int ret = -ENODEV;
+	if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    !domain->ops->aux_get_pasid)
+		return -ENODEV;
 
-	if (domain->ops->aux_get_pasid)
-		ret = domain->ops->aux_get_pasid(domain, dev);
+	return domain->ops->aux_get_pasid(domain, dev);
 
-	return ret;
  }
  EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);

Lu Baolu

author

2020-07-14 06:02 UTC #4 lore raw

PATCH v3 2/4 iommu: Add iommu_aux_at(de)tach_group()

In reply to Lu Baolu

This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.

/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group which
* contains sub-devices (for example mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group,
struct device *dev)

/**
* iommu_aux_detach_group - detach an aux-domain from an iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group,
struct device *dev)

It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.

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

---
 drivers/iommu/iommu.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h | 17 +++++++++++++
 2 files changed, 75 insertions(+)

drivers/iommu/iommu.c +58 -0

index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {

@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group which
+ *                          contains sub-devices (for example mdevs) derived
+ *                          from @dev.
+ * @domain: an aux-domain;
+ * @group:  an iommu_group which contains sub-devices derived from @dev;
+ * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct device *dev)
+{
+	int ret = -EBUSY;
+
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
+	ret = iommu_aux_attach_device(domain, dev);
+	if (!ret) {
+		group->domain = domain;
+		group->aux_domain_attached = true;
+	}
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_attach_group);
+
+/**
+ * iommu_aux_detach_group - detach an aux-domain from an iommu_group
+ *
+ * @domain: an aux-domain;
+ * @group:  an iommu_group which contains sub-devices derived from @dev;
+ * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * @domain must have been attached to @group via iommu_aux_attach_group().
+ */
+void iommu_aux_detach_group(struct iommu_domain *domain,
+			    struct iommu_group *group, struct device *dev)
+{
+	mutex_lock(&group->mutex);
+
+	if (WARN_ON(!group->aux_domain_attached || group->domain != domain))
+		goto out_unlock;
+
+	iommu_aux_detach_device(domain, dev);
+	group->aux_domain_attached = false;
+	group->domain = NULL;
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+}
+EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device

include/linux/iommu.h +17 -0

index 5657d4fef9f2..9506551139ab 100644
@@ -635,6 +635,10 @@ 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_get_pasid(struct iommu_domain *domain, struct device *dev);
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct device *dev);
+void iommu_aux_detach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct device *dev);
 
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,

@@ -1023,6 +1027,19 @@ iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 	return -ENODEV;
 }
 
+static inline int
+iommu_aux_attach_group(struct iommu_domain *domain,
+		       struct iommu_group *group, struct device *dev)
+{
+	return -ENODEV;
+}
+
+static inline void
+iommu_aux_detach_group(struct iommu_domain *domain,
+		       struct iommu_group *group, struct device *dev)
+{
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
 {

Signature · 1 line

2.17.1

Jacob Pan

2020-07-14 16:32 UTC #5 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 76 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu <[email protected]> wrote:

This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
* contains sub-devices (for example mdevs)
derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +3 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) }
 EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
+ * contains sub-devices (for example mdevs)
derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived from @dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct device
*dev) +{

drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781

Quoted 6 lines

drivers/iommu/iommu.c +6 -0

+	int ret = -EBUSY;
+
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+

Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()

Quoted 70 lines

drivers/iommu/iommu.c +17 -0

+	ret = iommu_aux_attach_device(domain, dev);
+	if (!ret) {
+		group->domain = domain;
+		group->aux_domain_attached = true;
+	}
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_attach_group);
+
+/**
+ * iommu_aux_detach_group - detach an aux-domain from an iommu_group
+ *
+ * @domain: an aux-domain;
+ * @group:  an iommu_group which contains sub-devices derived from

@dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * @domain must have been attached to @group via
iommu_aux_attach_group().
+ */

drivers/iommu/iommu.c +2 -0

+void iommu_aux_detach_group(struct iommu_domain *domain,
+			    struct iommu_group *group, struct device

*dev) +{

drivers/iommu/iommu.c +3 -0

+	mutex_lock(&group->mutex);
+
+	if (WARN_ON(!group->aux_domain_attached || group->domain !=

domain))

drivers/iommu/iommu.c +11 -0

+		goto out_unlock;
+
+	iommu_aux_detach_device(domain, dev);
+	group->aux_domain_attached = false;
+	group->domain = NULL;
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+}
+EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device

include/linux/iommu.h +0 -0

index 5657d4fef9f2..9506551139ab 100644
@@ -635,6 +635,10 @@ 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_get_pasid(struct iommu_domain *domain, struct device *dev); +int iommu_aux_attach_group(struct iommu_domain *domain,

include/linux/iommu.h +1 -0

+			   struct iommu_group *group, struct device

*dev); +void iommu_aux_detach_group(struct iommu_domain *domain,

include/linux/iommu.h +1 -0

+			   struct iommu_group *group, struct device

*dev);
struct iommu_sva *iommu_sva_bind_device(struct device *dev,

include/linux/iommu.h +6 -0

 					struct mm_struct *mm,

@@ -1023,6 +1027,19 @@ iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) return -ENODEV;
 }
 
+static inline int
+iommu_aux_attach_group(struct iommu_domain *domain,
+		       struct iommu_group *group, struct device *dev)
+{
+	return -ENODEV;
+}

include/linux/iommu.h iommu_aux_get_pasid(struct iommu_domain · line 1045

Quoted 10 lines

include/linux/iommu.h +7 -0

+
+static inline void
+iommu_aux_detach_group(struct iommu_domain *domain,
+		       struct iommu_group *group, struct device *dev)
+{
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
 *drvdata) {

[Jacob Pan]

Lu Baolu

author

2020-07-15 00:52 UTC #6 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Hi Jacob,

Quoted 81 lines · Jacob Pan

Quoted 78 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
* contains sub-devices (for example mdevs)
derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
  drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +3 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) }
  EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
+ * contains sub-devices (for example mdevs)
derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived from @dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct device
*dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+

Quoted 9 lines

drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()

It has been changed in PATCH 4/4:

static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)

{
        if (group->mdev_group)
                return iommu_aux_attach_group(domain->domain,
                                              group->iommu_group,
                                              group->iommu_device);
        else
                return iommu_attach_group(domain->domain,

group->iommu_group);
}

So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.

Best regards,
baolu

Jacob Pan

2020-07-15 15:54 UTC #7 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 105 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:

Hi Jacob,
Quoted 81 lines · Jacob Pan
Quoted 78 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
  drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +3 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) }
  EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived from @dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+  
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
         if (group->mdev_group)
                 return iommu_aux_attach_group(domain->domain,
                                               group->iommu_group,
                                               group->iommu_device);
         else
                 return iommu_attach_group(domain->domain, 

Quoted 6 lines

group->iommu_group);
}
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.

I get that, but don't you have to attach all the devices within the group? Here you see the group already has a domain and exit.

Quoted 2 lines

Best regards,
baolu

[Jacob Pan]

Lu Baolu

author

2020-07-16 01:12 UTC #8 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Hi Jacob,

Quoted 111 lines · Jacob Pan

Quoted 108 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 81 lines · Jacob Pan
Quoted 78 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
   drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +3 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) }
   EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived from @dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
          if (group->mdev_group)
                  return iommu_aux_attach_group(domain->domain,
                                                group->iommu_group,
                                                group->iommu_device);
          else
                  return iommu_attach_group(domain->domain,
group->iommu_group);
}

Quoted 4 lines

Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the

This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from iommu_attach_group(), iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.

Quoted 1 line

group? Here you see the group already has a domain and exit.

If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.

Best regards,
baolu

Alex Williamson

2020-07-29 20:03 UTC #9 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 125 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:

Hi Jacob,
Quoted 111 lines · Jacob Pan
Quoted 108 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 81 lines · Jacob Pan
Quoted 78 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from
@dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
   drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +3 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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 {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
 *domain, struct device *dev) }
   EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an iommu_group
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived from @dev;
+ * @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+  
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
          if (group->mdev_group)
                  return iommu_aux_attach_group(domain->domain,
                                                group->iommu_group,
                                                group->iommu_device);
          else
                  return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from iommu_attach_group(), iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.

Quoted 4 lines

Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.

I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,

Alex

Tian, Kevin

2020-07-29 23:34 UTC #10 lore raw

RE: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 147 lines

From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:04 AM
Quoted 18 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 15 lines · Jacob Pan
Quoted 12 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 9 lines · Jacob Pan
Quoted 6 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
   *
   * Returns 0 on success, or an error value.
   */
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
   drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h |
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +1 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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;
   };
 
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct
Quoted 2 lines
drivers/iommu/iommu.c +0 -0
   struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
iommu_domain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
Quoted 5 lines
drivers/iommu/iommu.c +2 -0
 *domain, struct device *dev) }
   EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
+ * @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 8 lines
Quoted 8 lines
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2781
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
          if (group->mdev_group)
                  return iommu_aux_attach_group(domain->domain,
                                                group->iommu_group,
                                                group->iommu_device);
          else
                  return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
Quoted 8 lines
iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.
Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.

Quoted 7 lines

I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,
Alex

Baolu and I discussed about this assumption before. The assumption is not based on singleton groups. We do consider multiple mdevs in one group. But our feeling at the moment is that all mdevs (or other AUX derivatives) in the same group should come from the same parent device, thus comes with above design. Does it sound a reasonable assumption to you?

Thanks
Keivn
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Alex Williamson

2020-07-30 19:47 UTC #11 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 153 lines · Kevin

Quoted 153 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:04 AM
Quoted 18 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 15 lines · Jacob Pan
Quoted 12 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 9 lines · Jacob Pan
Quoted 6 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
   *
   * Returns 0 on success, or an error value.
   */
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
   drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h |
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +1 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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;
   };
 
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct
Quoted 2 lines
drivers/iommu/iommu.c +0 -0
   struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct  
iommu_domain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct · line 2764
Quoted 5 lines
drivers/iommu/iommu.c +2 -0
 *domain, struct device *dev) }
   EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an  
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
+ * @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 8 lines
Quoted 8 lines
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+  
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
          if (group->mdev_group)
                  return iommu_aux_attach_group(domain->domain,
                                                group->iommu_group,
                                                group->iommu_device);
          else
                  return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
Quoted 8 lines
iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.
Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.
I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,

Quoted 8 lines

Quoted 1 line
Alex
Baolu and I discussed about this assumption before. The assumption is not based on singleton groups. We do consider multiple mdevs in one group. But our feeling at the moment is that all mdevs (or other AUX derivatives) in the same group should come from the same parent device, thus comes with above design. Does it sound a reasonable assumption to you?

No, the approach in this series doesn't really make sense to me. We currently have the following workflow as Baolu notes in the cover letter:

	domain = iommu_domain_alloc(bus);

	iommu_group_for_each_dev(group... 

		iommu_device = mdev-magic()

		if (iommu_dev_feature_enabled(iommu_device,
						IOMMU_DEV_FEAT_AUX))
			iommu_aux_attach_device(domain, iommu_device);

And we want to convert this to a group function, like we have for non-aux domains:

	domain = iommu_domain_alloc(bus);

	iommu_device = mdev-magic()

	iommu_aux_attach_group(domain, group, iommu_device);

And I think we want to do that largely because iommu_group.domain is private to iommu.c (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain. Passing an iommu_device avoids the problem that IOMMU API code doesn't know how to derive an iommu_device for each device in the group, but while doing so it ignores the fundamental nature of a group as being a set of one or more devices. Even if we can make the leap that all devices within the group would use the same iommu_device, an API that sets and aux domain for a group while entirely ignoring the devices within the group seems very broken.

So, barring adding an abstraction at struct device where an IOMMU API could retrieve the iommu_device backing anther device (which seems a very abstract concept for the base class), why not have the caller provide a lookup function? Ex:

int iommu_aux_attach_group(struct iommu_domain *domain,

			   struct iommu_group *group,
			   struct device *(*iommu_device_lookup)(
				struct device *dev));

Thus vfio could could simply provide &vfio_mdev_get_iommu_device and we'd have equivalent functionality to what we have currently, but with the domain pointer set in the iommu_group.

This also however highlights that our VF backed mdevs will have the same issue, so maybe this new IOMMU API interface should mimic vfio_mdev_attach_domain() more directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux attach. I'm not sure what the name of this combined function should be, iommu_attach_group_with_lookup()? This could be the core implementation of iommu_attach_group() where the existing function simply wraps the call with a NULL function pointer.

Anyway, I think there are ways to implement this that are more in line with the spirit of groups. Thanks,

Alex

Lu Baolu

author

2020-07-31 05:48 UTC #12 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Hi Alex,

Quoted 188 lines · Alex Williamson

Quoted 153 lines · Kevin
Quoted 153 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:04 AM
Quoted 18 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 15 lines · Jacob Pan
Quoted 12 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 9 lines · Jacob Pan
Quoted 6 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
    *
    * Returns 0 on success, or an error value.
    */
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
    drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h |
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +1 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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;
    };
 
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct
Quoted 2 lines
drivers/iommu/iommu.c +0 -0
    struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
iommu_domain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct · line 2764
Quoted 5 lines
drivers/iommu/iommu.c +2 -0
 *domain, struct device *dev) }
    EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
+ * @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 8 lines
Quoted 8 lines
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
           if (group->mdev_group)
                   return iommu_aux_attach_group(domain->domain,
                                                 group->iommu_group,
                                                 group->iommu_device);
           else
                   return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
Quoted 8 lines
iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.
Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.
I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,
Quoted 8 lines
Quoted 1 line
Alex
Baolu and I discussed about this assumption before. The assumption is not based on singleton groups. We do consider multiple mdevs in one group. But our feeling at the moment is that all mdevs (or other AUX derivatives) in the same group should come from the same parent device, thus comes with above design. Does it sound a reasonable assumption to you?
No, the approach in this series doesn't really make sense to me. We currently have the following workflow as Baolu notes in the cover letter:
	domain = iommu_domain_alloc(bus);

	iommu_group_for_each_dev(group...

		iommu_device = mdev-magic()

		if (iommu_dev_feature_enabled(iommu_device,
						IOMMU_DEV_FEAT_AUX))
			iommu_aux_attach_device(domain, iommu_device);
And we want to convert this to a group function, like we have for non-aux domains:
	domain = iommu_domain_alloc(bus);

	iommu_device = mdev-magic()

	iommu_aux_attach_group(domain, group, iommu_device);

Quoted 10 lines

And I think we want to do that largely because iommu_group.domain is private to iommu.c (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain. Passing an iommu_device avoids the problem that IOMMU API code doesn't know how to derive an iommu_device for each device in the group, but while doing so it ignores the fundamental nature of a group as being a set of one or more devices. Even if we can make the leap that all devices within the group would use the same iommu_device, an API that sets and aux domain for a group while entirely ignoring the devices within the group seems very broken.

Agreed. We couldn't assume that all devices in an iommu group shares a same iommu_device, especially when it comes to PF/VF wrapped mediated device case.

Quoted 6 lines

So, barring adding an abstraction at struct device where an IOMMU API could retrieve the iommu_device backing anther device (which seems a very abstract concept for the base class), why not have the caller provide a lookup function? Ex:

Quoted 8 lines

int iommu_aux_attach_group(struct iommu_domain *domain,
			   struct iommu_group *group,
			   struct device *(*iommu_device_lookup)(
				struct device *dev));
Thus vfio could could simply provide &vfio_mdev_get_iommu_device and we'd have equivalent functionality to what we have currently, but with the domain pointer set in the iommu_group.

This looks good to me.

Quoted 10 lines

This also however highlights that our VF backed mdevs will have the same issue, so maybe this new IOMMU API interface should mimic vfio_mdev_attach_domain() more directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux attach. I'm not sure what the name of this combined function should be, iommu_attach_group_with_lookup()? This could be the core implementation of iommu_attach_group() where the existing function simply wraps the call with a NULL function pointer.

Quoted 2 lines

Anyway, I think there are ways to implement this that are more in line with the spirit of groups.

Another possible implementation, just for discussion purpose:

1. Add a member in group_device to save the iommu_device if it exists:

drivers/iommu/iommu.c +7 -0

index b6858adc4f17..6474e82cf4b4 100644
@@ -47,9 +47,16 @@ struct iommu_group {
        struct list_head entry;
 };
 
+/*
+ * dma_alias: The device put in this group might depends on another
+ *            physical device to do the DMA remapping. At(de)taching
+ *            the domain to/from @dma_alias instead of @dev if
+ *            @dma_alias is set.
+ */
 struct group_device {
        struct list_head list;
        struct device *dev;
+       struct device *dma_alias;
        char *name;
 };

2. Pass in the iommu_device when calling iommu_group_add_device().

int iommu_group_add_device(struct iommu_group *group,
struct device *dev, struct device *dma_alias)

Hence, the iommu core could get a chance to set the iommu_device in the group device.

3. Mimic vfio_mdev_attach_domain() logic in iommu_group_do_attach_device():

if (group->dma_alias) {
	if (iommu_dev_feature_enabled(group->dma_alias, IOMMU_DEV_FEAT_AUX))
		iommu_aux_attach_device(domain, group->dma_alias);
	else
		__iommu_attach_device(domain, group->dma_alias);
} else {
	__iommu_attach_device(domain, dev);
}

One limitation is that the driver should call mdev_set_iommu_device() before the mdev_probe() get called.

Best regards,
baolu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Alex Williamson

2020-07-31 18:05 UTC #13 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Quoted 281 lines · On Fri, 31 Jul 2020 13:47:57 +0800 Lu Baolu <[email protected]> wrote:

Hi Alex,
Quoted 188 lines · Alex Williamson
Quoted 153 lines · Kevin
Quoted 153 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:04 AM
Quoted 18 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 15 lines · Jacob Pan
Quoted 12 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 9 lines · Jacob Pan
Quoted 6 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
    *
    * Returns 0 on success, or an error value.
    */
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
    drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h |
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +1 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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;
    };
 
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct
Quoted 2 lines
drivers/iommu/iommu.c +0 -0
    struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct  
iommu_domain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct · line 2764
Quoted 5 lines
drivers/iommu/iommu.c +2 -0
 *domain, struct device *dev) }
    EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an  
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
+ * @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 8 lines
Quoted 8 lines
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+  
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
           if (group->mdev_group)
                   return iommu_aux_attach_group(domain->domain,
                                                 group->iommu_group,
                                                 group->iommu_device);
           else
                   return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
Quoted 8 lines
iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.
Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.
I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,
Quoted 8 lines
Quoted 1 line
Alex
Baolu and I discussed about this assumption before. The assumption is not based on singleton groups. We do consider multiple mdevs in one group. But our feeling at the moment is that all mdevs (or other AUX derivatives) in the same group should come from the same parent device, thus comes with above design. Does it sound a reasonable assumption to you?
No, the approach in this series doesn't really make sense to me. We currently have the following workflow as Baolu notes in the cover letter:
	domain = iommu_domain_alloc(bus);

	iommu_group_for_each_dev(group...

		iommu_device = mdev-magic()

		if (iommu_dev_feature_enabled(iommu_device,
						IOMMU_DEV_FEAT_AUX))
			iommu_aux_attach_device(domain, iommu_device);
And we want to convert this to a group function, like we have for non-aux domains:
	domain = iommu_domain_alloc(bus);

	iommu_device = mdev-magic()

	iommu_aux_attach_group(domain, group, iommu_device);
Quoted 10 lines
And I think we want to do that largely because iommu_group.domain is private to iommu.c (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain. Passing an iommu_device avoids the problem that IOMMU API code doesn't know how to derive an iommu_device for each device in the group, but while doing so it ignores the fundamental nature of a group as being a set of one or more devices. Even if we can make the leap that all devices within the group would use the same iommu_device, an API that sets and aux domain for a group while entirely ignoring the devices within the group seems very broken.
Agreed. We couldn't assume that all devices in an iommu group shares a same iommu_device, especially when it comes to PF/VF wrapped mediated device case.
Quoted 6 lines
So, barring adding an abstraction at struct device where an IOMMU API could retrieve the iommu_device backing anther device (which seems a very abstract concept for the base class), why not have the caller provide a lookup function? Ex:
Quoted 8 lines
int iommu_aux_attach_group(struct iommu_domain *domain,
			   struct iommu_group *group,
			   struct device *(*iommu_device_lookup)(
				struct device *dev));
Thus vfio could could simply provide &vfio_mdev_get_iommu_device and we'd have equivalent functionality to what we have currently, but with the domain pointer set in the iommu_group.
This looks good to me.
Quoted 10 lines
This also however highlights that our VF backed mdevs will have the same issue, so maybe this new IOMMU API interface should mimic vfio_mdev_attach_domain() more directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux attach. I'm not sure what the name of this combined function should be, iommu_attach_group_with_lookup()? This could be the core implementation of iommu_attach_group() where the existing function simply wraps the call with a NULL function pointer.
Quoted 2 lines
Anyway, I think there are ways to implement this that are more in line with the spirit of groups.
Another possible implementation, just for discussion purpose:
1. Add a member in group_device to save the iommu_device if it exists:
drivers/iommu/iommu.c +7 -0
index b6858adc4f17..6474e82cf4b4 100644
@@ -47,9 +47,16 @@ struct iommu_group {
         struct list_head entry;
  };
 
+/*
+ * dma_alias: The device put in this group might depends on another
+ *            physical device to do the DMA remapping. At(de)taching
+ *            the domain to/from @dma_alias instead of @dev if
+ *            @dma_alias is set.
+ */
  struct group_device {
         struct list_head list;
         struct device *dev;
+       struct device *dma_alias;
         char *name;
  };
 
2. Pass in the iommu_device when calling iommu_group_add_device().
int iommu_group_add_device(struct iommu_group *group,
struct device *dev, struct device *dma_alias)
Hence, the iommu core could get a chance to set the iommu_device in the group device.
3. Mimic vfio_mdev_attach_domain() logic in iommu_group_do_attach_device():
if (group->dma_alias) {
	if (iommu_dev_feature_enabled(group->dma_alias, IOMMU_DEV_FEAT_AUX))
		iommu_aux_attach_device(domain, group->dma_alias);
	else
		__iommu_attach_device(domain, group->dma_alias);
} else {
	__iommu_attach_device(domain, dev);
}

Quoted 2 lines

One limitation is that the driver should call mdev_set_iommu_device() before the mdev_probe() get called.

That's an option, but DMA aliases are an existing thing within our IOMMU/PCI constructs, so I'd steer away from "dma_alias" terminology. I thought the callback was a little less invasive to the IOMMU layer for now as aux domains are still a rather unique use case, and I'm not sure we can justify the hack of otherwise IOMMU backed mdevs formally within the IOMMU API. Thanks,

Alex

Lu Baolu

author

2020-08-03 02:02 UTC #14 lore raw

Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

Hi Alex,

Quoted 284 lines · Alex Williamson

Quoted 281 lines · On Fri, 31 Jul 2020 13:47:57 +0800 Lu Baolu <[email protected]> wrote:
Hi Alex,
Quoted 188 lines · Alex Williamson
Quoted 153 lines · Kevin
Quoted 153 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:04 AM
Quoted 18 lines · On Thu, 16 Jul 2020 09:07:46 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 15 lines · Jacob Pan
Quoted 12 lines · On Wed, 15 Jul 2020 08:47:36 +0800 Lu Baolu <[email protected]> wrote:
Hi Jacob,
Quoted 9 lines · Jacob Pan
Quoted 6 lines · On Tue, 14 Jul 2020 13:57:01 +0800 Lu Baolu<[email protected]> wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev where sub-devices derived from an aux-domain capable device are created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
     *
     * Returns 0 on success, or an error value.
     */
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
* @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
It also adds a flag in the iommu_group data structure to identify an iommu_group with aux-domain attached from those normal ones.
Signed-off-by Lu Baolu<[email protected]>
---
     drivers/iommu/iommu.c | 58
+++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h |
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
17 +++++++++++++ 2 files changed, 75 insertions(+)
drivers/iommu/iommu.c +1 -0
index e1fdd3531d65..cad5a19ebf22 100644
@@ -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;
     };
 
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct
Quoted 2 lines
drivers/iommu/iommu.c +0 -0
     struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
iommu_domain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/iommu.c int iommu_aux_get_pasid(struct · line 2764
Quoted 5 lines
drivers/iommu/iommu.c +2 -0
 *domain, struct device *dev) }
     EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an
iommu_group
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
@dev;
+ * @dev: the physical device which supports
IOMMU_DEV_FEAT_AUX.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 8 lines
Quoted 8 lines
+ *
+ * Returns 0 on success, or an error value.
+ */
drivers/iommu/iommu.c +2 -0
+int iommu_aux_attach_group(struct iommu_domain *domain,
+			   struct iommu_group *group, struct
device *dev) +{
drivers/iommu/iommu.c +2 -0
+	int ret = -EBUSY;
+
Quoted 9 lines
drivers/iommu/iommu.c
Quoted 4 lines
drivers/iommu/iommu.c +4 -0
+	mutex_lock(&group->mutex);
+	if (group->domain)
+		goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
            if (group->mdev_group)
                    return iommu_aux_attach_group(domain->domain,
                                                  group->iommu_group,
                                                  group->iommu_device);
            else
                    return iommu_attach_group(domain->domain,
group->iommu_group);
}
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
Quoted 8 lines
iommu_aux_attach_group() doesn't need to attach the domain to each device in group, instead it only needs to attach the domain to the physical device where the mdev's were created from.
Quoted 1 line
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already attached to the group, so it returns -EBUSY.
I agree with Jacob, singleton groups should not be built into the IOMMU API, we're not building an interface just for mdevs or current limitations of mdevs. This also means that setting a flag on the group and passing a device that's assumed to be common for all devices within the group, don't really make sense here. Thanks,
Quoted 8 lines
Quoted 1 line
Alex
Baolu and I discussed about this assumption before. The assumption is not based on singleton groups. We do consider multiple mdevs in one group. But our feeling at the moment is that all mdevs (or other AUX derivatives) in the same group should come from the same parent device, thus comes with above design. Does it sound a reasonable assumption to you?
No, the approach in this series doesn't really make sense to me. We currently have the following workflow as Baolu notes in the cover letter:
	domain = iommu_domain_alloc(bus);

	iommu_group_for_each_dev(group...

		iommu_device = mdev-magic()

		if (iommu_dev_feature_enabled(iommu_device,
						IOMMU_DEV_FEAT_AUX))
			iommu_aux_attach_device(domain, iommu_device);
And we want to convert this to a group function, like we have for non-aux domains:
	domain = iommu_domain_alloc(bus);

	iommu_device = mdev-magic()

	iommu_aux_attach_group(domain, group, iommu_device);
Quoted 10 lines
And I think we want to do that largely because iommu_group.domain is private to iommu.c (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain. Passing an iommu_device avoids the problem that IOMMU API code doesn't know how to derive an iommu_device for each device in the group, but while doing so it ignores the fundamental nature of a group as being a set of one or more devices. Even if we can make the leap that all devices within the group would use the same iommu_device, an API that sets and aux domain for a group while entirely ignoring the devices within the group seems very broken.
Agreed. We couldn't assume that all devices in an iommu group shares a same iommu_device, especially when it comes to PF/VF wrapped mediated device case.
Quoted 6 lines
So, barring adding an abstraction at struct device where an IOMMU API could retrieve the iommu_device backing anther device (which seems a very abstract concept for the base class), why not have the caller provide a lookup function? Ex:
Quoted 8 lines
int iommu_aux_attach_group(struct iommu_domain *domain,
			   struct iommu_group *group,
			   struct device *(*iommu_device_lookup)(
				struct device *dev));
Thus vfio could could simply provide &vfio_mdev_get_iommu_device and we'd have equivalent functionality to what we have currently, but with the domain pointer set in the iommu_group.
This looks good to me.
Quoted 10 lines
This also however highlights that our VF backed mdevs will have the same issue, so maybe this new IOMMU API interface should mimic vfio_mdev_attach_domain() more directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux attach. I'm not sure what the name of this combined function should be, iommu_attach_group_with_lookup()? This could be the core implementation of iommu_attach_group() where the existing function simply wraps the call with a NULL function pointer.
Quoted 2 lines
Anyway, I think there are ways to implement this that are more in line with the spirit of groups.
Another possible implementation, just for discussion purpose:
1. Add a member in group_device to save the iommu_device if it exists:
drivers/iommu/iommu.c +7 -0
index b6858adc4f17..6474e82cf4b4 100644
@@ -47,9 +47,16 @@ struct iommu_group {
          struct list_head entry;
   };
 
+/*
+ * dma_alias: The device put in this group might depends on another
+ *            physical device to do the DMA remapping. At(de)taching
+ *            the domain to/from @dma_alias instead of @dev if
+ *            @dma_alias is set.
+ */
   struct group_device {
          struct list_head list;
          struct device *dev;
+       struct device *dma_alias;
          char *name;
   };
 
2. Pass in the iommu_device when calling iommu_group_add_device().
int iommu_group_add_device(struct iommu_group *group,
struct device *dev, struct device *dma_alias)
Hence, the iommu core could get a chance to set the iommu_device in the group device.
3. Mimic vfio_mdev_attach_domain() logic in iommu_group_do_attach_device():
if (group->dma_alias) {
	if (iommu_dev_feature_enabled(group->dma_alias, IOMMU_DEV_FEAT_AUX))
		iommu_aux_attach_device(domain, group->dma_alias);
	else
		__iommu_attach_device(domain, group->dma_alias);
} else {
	__iommu_attach_device(domain, dev);
}

Quoted 9 lines

Quoted 2 lines
One limitation is that the driver should call mdev_set_iommu_device() before the mdev_probe() get called.
That's an option, but DMA aliases are an existing thing within our IOMMU/PCI constructs, so I'd steer away from "dma_alias" terminology. I thought the callback was a little less invasive to the IOMMU layer for now as aux domains are still a rather unique use case, and I'm not sure we can justify the hack of otherwise IOMMU backed mdevs formally within the IOMMU API.

Fair enough. I will start with lookup callback solution.

Best regards,
baolu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Lu Baolu

author

2020-07-14 06:02 UTC #15 lore raw

PATCH v3 3/4 iommu: Add iommu_aux_get_domain_for_dev()

In reply to Lu Baolu

The device driver needs an API to get its aux-domain. A typical usage scenario is:

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

This adds an API for such use case.

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

---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)

drivers/iommu/iommu.c +18 -0

index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+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);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device

include/linux/iommu.h +7 -0

index 9506551139ab..cda6cef7579e 100644
@@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
 void iommu_aux_detach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev);
 
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,

@@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct iommu_domain *domain,
 {
 }
 
+static inline struct iommu_domain *
+iommu_aux_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
 {

Signature · 1 line

2.17.1

Alex Williamson

2020-07-29 20:25 UTC #16 lore raw

Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Quoted 37 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <[email protected]> wrote:

The device driver needs an API to get its aux-domain. A typical usage scenario is:

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

This adds an API for such use case.

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

---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)

drivers/iommu/iommu.c +1 -0

index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)

drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830

Quoted 10 lines

drivers/iommu/iommu.c +10 -0

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

Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.

If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too. Thanks,

Alex

Quoted 35 lines

drivers/iommu/iommu.c +7 -0

+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device

include/linux/iommu.h +7 -0

index 9506551139ab..cda6cef7579e 100644
@@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
 void iommu_aux_detach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev);
 
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,

@@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct iommu_domain *domain,
 {
 }
 
+static inline struct iommu_domain *
+iommu_aux_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
 {

Tian, Kevin

2020-07-29 23:49 UTC #17 lore raw

RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Quoted 60 lines

From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:25 AM
drivers/iommu/iommu.c void iommu_aux_detach_group(struct
Quoted 33 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....
This adds an API for such use case.
Suggested-by Alex Williamson <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
iommu_domain *domain,
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 4 lines
drivers/iommu/iommu.c +1 -0
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
*dev)
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+{
+	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;
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.

Quoted 3 lines

If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would

IOMMU layer manages domains per parent device. Here given a dev (of mdev), we need a way to find its associated domain under its parent device. And we cannot simply use iommu_get_domain_for_dev on the parent device of the mdev, as it will give us the primary domain of parent device.

Thanks
Kevin

Quoted 45 lines

resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too. Thanks,
Alex
include/linux/iommu.h int iommu_aux_attach_group(struct
Quoted 15 lines
drivers/iommu/iommu.c +7 -0
+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device
include/linux/iommu.h +0 -0
index 9506551139ab..cda6cef7579e 100644
@@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct
iommu_domain *domain,
include/linux/iommu.h int iommu_aux_attach_group(struct iommu_domain *domain,
Quoted 4 lines
include/linux/iommu.h +1 -0
 			   struct iommu_group *group, struct device *dev);
 void iommu_aux_detach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
*dev);
include/linux/iommu.h iommu_aux_detach_group(struct
Quoted 4 lines
include/linux/iommu.h +0 -0
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,
@@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct
iommu_domain *domain,
include/linux/iommu.h iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 11 lines
include/linux/iommu.h +6 -0
 {
 }
 
+static inline struct iommu_domain *
+iommu_aux_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
*drvdata)
Quoted 1 line
include/linux/iommu.h +0 -0
 {

Alex Williamson

2020-07-30 20:17 UTC #18 lore raw

Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Quoted 60 lines · Kevin

Quoted 60 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:25 AM
drivers/iommu/iommu.c void iommu_aux_detach_group(struct
Quoted 33 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....
This adds an API for such use case.
Suggested-by Alex Williamson <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct  
iommu_domain *domain,
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 4 lines
drivers/iommu/iommu.c +1 -0
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device  
*dev)
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+{
+	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;  
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.

Quoted 5 lines

Quoted 3 lines
If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would
IOMMU layer manages domains per parent device. Here given a

Is this the IOMMU layer or the VT-d driver? I don't see any notion of managing domains relative to a parent in the IOMMU layer. Please point to something more specific if I'm wrong here.

Quoted 4 lines

dev (of mdev), we need a way to find its associated domain under its parent device. And we cannot simply use iommu_get_domain_for_dev on the parent device of the mdev, as it will give us the primary domain of parent device.

Not the parent device of the mdev, but the mdev_dev(mdev) device. Isn't that what this series is enabling, being able to return the domain from the group that contains the mdev_dev? We shouldn't need to leave breadcrumbs on the group to know about the domain, the domain itself should be the source of knowledge, or provide a mechanism/ops to learn that knowledge. Thanks,

Alex

Quoted 46 lines

Quoted 45 lines
resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too. Thanks,
Alex
include/linux/iommu.h int iommu_aux_attach_group(struct
Quoted 15 lines
drivers/iommu/iommu.c +7 -0
+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device
include/linux/iommu.h +0 -0
index 9506551139ab..cda6cef7579e 100644
@@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct  
iommu_domain *domain,
include/linux/iommu.h int iommu_aux_attach_group(struct iommu_domain *domain,
Quoted 4 lines
include/linux/iommu.h +1 -0
 			   struct iommu_group *group, struct device *dev);
 void iommu_aux_detach_group(struct iommu_domain *domain,
 			   struct iommu_group *group, struct device *dev);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device  
*dev);
include/linux/iommu.h iommu_aux_detach_group(struct
Quoted 4 lines
include/linux/iommu.h +0 -0
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,
@@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct  
iommu_domain *domain,
include/linux/iommu.h iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 11 lines
include/linux/iommu.h +6 -0
 {
 }
 
+static inline struct iommu_domain *
+iommu_aux_get_domain_for_dev(struct device *dev)
+{
+	return NULL;
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void  
*drvdata)
Quoted 1 line
include/linux/iommu.h +0 -0
 {  

Tian, Kevin

2020-07-31 00:26 UTC #19 lore raw

RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Quoted 71 lines

From: Alex Williamson <[email protected]>
Sent: Friday, July 31, 2020 4:17 AM
Quoted 40 lines · Kevin
Quoted 40 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:25 AM
drivers/iommu/iommu.c void iommu_aux_detach_group(struct
Quoted 33 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....
This adds an API for such use case.
Suggested-by Alex Williamson <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
iommu_domain *domain,
Quoted 4 lines
Quoted 4 lines
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 4 lines
drivers/iommu/iommu.c +1 -0
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct
device
Quoted 20 lines
Quoted 19 lines
*dev)
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+{
+	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;
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.
If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would

Quoted 5 lines

Quoted 1 line
IOMMU layer manages domains per parent device. Here given a
Is this the IOMMU layer or the VT-d driver? I don't see any notion of managing domains relative to a parent in the IOMMU layer. Please point to something more specific if I'm wrong here.

it's maintained in VT-d driver (include/linux/intel-iommu.h)

struct device_domain_info {
struct list_head link; /* link to domain siblings */ struct list_head global; /* link to global list */ struct list_head table; /* link to pasid table */ struct list_head auxiliary_domains; /* auxiliary domains
* attached to this device */
...

Quoted 6 lines

Quoted 5 lines
dev (of mdev), we need a way to find its associated domain under its parent device. And we cannot simply use iommu_get_domain_for_dev on the parent device of the mdev, as it will give us the primary domain of parent device.

Quoted 8 lines

Not the parent device of the mdev, but the mdev_dev(mdev) device. Isn't that what this series is enabling, being able to return the domain from the group that contains the mdev_dev? We shouldn't need to leave breadcrumbs on the group to know about the domain, the domain itself should be the source of knowledge, or provide a mechanism/ops to learn that knowledge. Thanks,
Alex

It's the tradeoff between leaving breadcrumb in domain or in group. Today the domain has no knowledge of mdev. It just includes a list of physical devices which are attached to the domain (either due to the device is assigned in a whole or as the parent device of an assigned mdev). Then we have two choices. One is to save the mdev_dev info in device_domain_info and maintain a mapping between mdev_dev and related aux domain. The other is to record the domain info directly in group. Earlier we choose the latter one as it looks simpler. If you prefer to the former one, we can think more and have a try.

Thanks
Kevin
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Tian, Kevin

2020-07-31 02:17 UTC #20 lore raw

RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

In reply to Alex Williamson

Quoted 109 lines

From: Tian, Kevin
Sent: Friday, July 31, 2020 8:26 AM
Quoted 19 lines
From: Alex Williamson <[email protected]>
Sent: Friday, July 31, 2020 4:17 AM
Quoted 13 lines · Kevin
Quoted 13 lines
From: Alex Williamson <[email protected]>
Sent: Thursday, July 30, 2020 4:25 AM
Quoted 7 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device =
vfio_mdev_get_iommu_device(dev);
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/iommu/iommu.c void iommu_aux_detach_group(struct
Quoted 26 lines
        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....
This adds an API for such use case.
Suggested-by Alex Williamson <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
iommu_domain *domain,
Quoted 23 lines
Quoted 4 lines
Quoted 4 lines
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain,
Quoted 4 lines
drivers/iommu/iommu.c +1 -0
 }
 EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct
device
Quoted 18 lines
Quoted 18 lines
*dev)
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+{
+	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;
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.
If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test
that
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
it's an aux domain in the example use case. It seems like that would
IOMMU layer manages domains per parent device. Here given a
Is this the IOMMU layer or the VT-d driver? I don't see any notion of managing domains relative to a parent in the IOMMU layer. Please point to something more specific if I'm wrong here.
it's maintained in VT-d driver (include/linux/intel-iommu.h)
struct device_domain_info {
struct list_head link; /* link to domain siblings */ struct list_head global; /* link to global list */ struct list_head table; /* link to pasid table */ struct list_head auxiliary_domains; /* auxiliary domains
* attached to this device */
...
Quoted 9 lines
Quoted 5 lines
dev (of mdev), we need a way to find its associated domain under its parent device. And we cannot simply use iommu_get_domain_for_dev on the parent device of the mdev, as it will give us the primary domain of parent device.
Not the parent device of the mdev, but the mdev_dev(mdev) device. Isn't that what this series is enabling, being able to return the domain from the group that contains the mdev_dev? We shouldn't need
to
Quoted 5 lines
leave breadcrumbs on the group to know about the domain, the domain itself should be the source of knowledge, or provide a mechanism/ops to learn that knowledge. Thanks,
Alex

Quoted 10 lines

It's the tradeoff between leaving breadcrumb in domain or in group. Today the domain has no knowledge of mdev. It just includes a list of physical devices which are attached to the domain (either due to the device is assigned in a whole or as the parent device of an assigned mdev). Then we have two choices. One is to save the mdev_dev info in device_domain_info and maintain a mapping between mdev_dev and related aux domain. The other is to record the domain info directly in group. Earlier we choose the latter one as it looks simpler. If you prefer to the former one, we can think more and have a try.

Please skip this comment. I have a wrong understanding of the problem and is discussing with Baolu. He will reply with our conclusion later.

Thanks
Kevin

Lu Baolu

author

2020-07-31 06:30 UTC #21 lore raw

Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

In reply to Alex Williamson

Hi Alex,

Quoted 39 lines · Alex Williamson

Quoted 36 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu<[email protected]> wrote:

The device driver needs an API to get its aux-domain. A typical usage scenario is:

         unsigned long pasid;
         struct iommu_domain *domain;
         struct device *dev = mdev_dev(mdev);
         struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

         domain = iommu_aux_get_domain_for_dev(dev);
         if (!domain)
                 return -ENODEV;

         pasid = iommu_aux_get_pasid(domain, iommu_device);
         if (pasid <= 0)
                 return -EINVAL;

          /* Program the device context */
          ....

This adds an API for such use case.

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

---
  drivers/iommu/iommu.c | 18 ++++++++++++++++++
  include/linux/iommu.h |  7 +++++++
  2 files changed, 25 insertions(+)

drivers/iommu/iommu.c +0 -0

index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
  }
  EXPORT_SYMBOL_GPL(iommu_aux_detach_group);

Quoted 14 lines

drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 11 lines
drivers/iommu/iommu.c +11 -0
+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;
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.

Agreed. Given that a group may contain both non-aux and aux devices, adding such flag in iommu_group doesn't make sense.

Quoted 7 lines

If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too.

Let's rehearsal our use case.

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

[1]     domain = iommu_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

[2]     pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

The reason why I add this iommu_aux_get_domain_for_dev() is that we need to make sure the domain got at [1] is valid to be used at [2].

https://lore.kernel.org/linux-iommu/[email protected]/

When calling into iommu_aux_get_pasid(), the iommu driver should make sure that @domain is a valid aux-domain for @iommu_device. Hence, for our use case, it seems that there's no need for a is_aux_domain() api.

Anyway, I'm not against adding a new is_aux_domain() api if there's a need elsewhere.

Best regards,
baolu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Alex Williamson

2020-07-31 18:14 UTC #22 lore raw

Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Quoted 86 lines · On Fri, 31 Jul 2020 14:30:03 +0800 Lu Baolu <[email protected]> wrote:

Hi Alex,
Quoted 39 lines · Alex Williamson
Quoted 36 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu<[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
         unsigned long pasid;
         struct iommu_domain *domain;
         struct device *dev = mdev_dev(mdev);
         struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

         domain = iommu_aux_get_domain_for_dev(dev);
         if (!domain)
                 return -ENODEV;

         pasid = iommu_aux_get_pasid(domain, iommu_device);
         if (pasid <= 0)
                 return -EINVAL;

          /* Program the device context */
          ....
This adds an API for such use case.
Suggested-by Alex Williamson<[email protected]>
Signed-off-by Lu Baolu<[email protected]>
---
  drivers/iommu/iommu.c | 18 ++++++++++++++++++
  include/linux/iommu.h |  7 +++++++
  2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
  }
  EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
Quoted 14 lines
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 11 lines
drivers/iommu/iommu.c +11 -0
+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;  
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.
Agreed. Given that a group may contain both non-aux and aux devices, adding such flag in iommu_group doesn't make sense.
Quoted 7 lines
If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too.
Let's rehearsal our use case.
         unsigned long pasid;
         struct iommu_domain *domain;
         struct device *dev = mdev_dev(mdev);
         struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
[1]     domain = iommu_get_domain_for_dev(dev);
         if (!domain)
                 return -ENODEV;
[2]     pasid = iommu_aux_get_pasid(domain, iommu_device);
         if (pasid <= 0)
                 return -EINVAL;

          /* Program the device context */
          ....

Quoted 4 lines

The reason why I add this iommu_aux_get_domain_for_dev() is that we need to make sure the domain got at [1] is valid to be used at [2].
https://lore.kernel.org/linux-iommu/[email protected]/

Yep, I thought that was a bit of a leap in logic.

Quoted 6 lines

When calling into iommu_aux_get_pasid(), the iommu driver should make sure that @domain is a valid aux-domain for @iommu_device. Hence, for our use case, it seems that there's no need for a is_aux_domain() api.
Anyway, I'm not against adding a new is_aux_domain() api if there's a need elsewhere.

I think it could work either way, we could have an iommu_get_aux_domain_for_dev() which returns NULL if the domain is not an aux domain, or we could use iommu_get_domain_for_dev() and the caller could test the domain with iommu_is_aux_domain() if they need to confirm if it's an aux domain. The former could even be written using the latter, a wrapper of iommu_get_domain_for_dev() that checks aux property before returning. Thanks,

Alex

Lu Baolu

author

2020-08-03 02:20 UTC #23 lore raw

Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

Hi Alex,

Quoted 103 lines · Alex Williamson

Quoted 86 lines · On Fri, 31 Jul 2020 14:30:03 +0800 Lu Baolu <[email protected]> wrote:
Hi Alex,
Quoted 39 lines · Alex Williamson
Quoted 36 lines · On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu<[email protected]> wrote:
The device driver needs an API to get its aux-domain. A typical usage scenario is:
          unsigned long pasid;
          struct iommu_domain *domain;
          struct device *dev = mdev_dev(mdev);
          struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

          domain = iommu_aux_get_domain_for_dev(dev);
          if (!domain)
                  return -ENODEV;

          pasid = iommu_aux_get_pasid(domain, iommu_device);
          if (pasid <= 0)
                  return -EINVAL;

           /* Program the device context */
           ....
This adds an API for such use case.
Suggested-by Alex Williamson<[email protected]>
Signed-off-by Lu Baolu<[email protected]>
---
   drivers/iommu/iommu.c | 18 ++++++++++++++++++
   include/linux/iommu.h |  7 +++++++
   2 files changed, 25 insertions(+)
drivers/iommu/iommu.c +0 -0
index cad5a19ebf22..434bf42b6b9b 100644
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
   }
   EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
 
Quoted 14 lines
drivers/iommu/iommu.c void iommu_aux_detach_group(struct iommu_domain *domain, · line 2830
Quoted 11 lines
drivers/iommu/iommu.c +11 -0
+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;
Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided.
Agreed. Given that a group may contain both non-aux and aux devices, adding such flag in iommu_group doesn't make sense.
Quoted 7 lines
If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too.
Let's rehearsal our use case.
          unsigned long pasid;
          struct iommu_domain *domain;
          struct device *dev = mdev_dev(mdev);
          struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
[1]     domain = iommu_get_domain_for_dev(dev);
          if (!domain)
                  return -ENODEV;
[2]     pasid = iommu_aux_get_pasid(domain, iommu_device);
          if (pasid <= 0)
                  return -EINVAL;

           /* Program the device context */
           ....
Quoted 4 lines
The reason why I add this iommu_aux_get_domain_for_dev() is that we need to make sure the domain got at [1] is valid to be used at [2].
https://lore.kernel.org/linux-iommu/[email protected]/
Yep, I thought that was a bit of a leap in logic.
Quoted 6 lines
When calling into iommu_aux_get_pasid(), the iommu driver should make sure that @domain is a valid aux-domain for @iommu_device. Hence, for our use case, it seems that there's no need for a is_aux_domain() api.
Anyway, I'm not against adding a new is_aux_domain() api if there's a need elsewhere.

Quoted 7 lines

I think it could work either way, we could have an iommu_get_aux_domain_for_dev() which returns NULL if the domain is not an aux domain, or we could use iommu_get_domain_for_dev() and the caller could test the domain with iommu_is_aux_domain() if they need to confirm if it's an aux domain. The former could even be written using the latter, a wrapper of iommu_get_domain_for_dev() that checks aux property before returning.

Okay. So iommu_get_domain_for_dev() and iommu_is_aux_domain() are what we wanted. The iommu_get_domain_for_dev() could be a simple wrapper of them.

Thanks a lot for the guide. I will implement a new version according to the feedbacks.

Best regards,
baolu

Lu Baolu

author

2020-07-14 06:02 UTC #24 lore raw

PATCH v3 4/4 vfio/type1: Use iommu_aux_at(de)tach_group() APIs

In reply to Lu Baolu

Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.

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

---
 drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
 1 file changed, 7 insertions(+), 37 deletions(-)

drivers/vfio/vfio_iommu_type1.c +7 ~~-37~~

index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
 struct vfio_group {
 	struct iommu_group	*iommu_group;
 	struct list_head	next;
+	struct device		*iommu_device;
 	bool			mdev_group;	/* An mdev group */
 	bool			pinned_page_dirty_scope;
 };

@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
 	return NULL;
 }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
 static int vfio_iommu_attach_group(struct vfio_domain *domain,
 				   struct vfio_group *group)
 {
 	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);
 	else
 		return iommu_attach_group(domain->domain, group->iommu_group);
 }

@@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain,
 				    struct vfio_group *group)
 {
 	if (group->mdev_group)
-		iommu_group_for_each_dev(group->iommu_group, domain->domain,
-					 vfio_mdev_detach_domain);
+		iommu_aux_detach_group(domain->domain, group->iommu_group,
+				       group->iommu_device);
 	else
 		iommu_detach_group(domain->domain, group->iommu_group);
 }

@@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			return 0;
 		}
 
+		group->iommu_device = iommu_device;
 		bus = iommu_device->bus;
 	}

Signature · 1 line

2.17.1

Christoph Hellwig

2020-07-14 08:25 UTC #25 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Quoted 3 lines · Lu Baolu

Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.

This removes the last user of iommu_aux_attach_device and iommu_aux_detach_device, which can be removed now.

Jacob Pan

2020-07-14 16:22 UTC #26 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Quoted 8 lines · On Tue, 14 Jul 2020 09:25:14 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 4 lines · Lu Baolu
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
This removes the last user of iommu_aux_attach_device and iommu_aux_detach_device, which can be removed now.

it is still used in patch 2/4 inside iommu_aux_attach_group(), right?

Quoted 4 lines

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[Jacob Pan]

Lu Baolu

author

2020-07-15 01:04 UTC #27 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Hi Christoph and Jacob,

Quoted 12 lines · Jacob Pan

Quoted 7 lines · On Tue, 14 Jul 2020 09:25:14 +0100 Christoph Hellwig<[email protected]> wrote:
Quoted 4 lines · Lu Baolu
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
This removes the last user of iommu_aux_attach_device and iommu_aux_detach_device, which can be removed now.
it is still used in patch 2/4 inside iommu_aux_attach_group(), right?

There is a need to use this interface. For example, an aux-domain is attached to a subset of a physical device and used in the kernel. In this usage scenario, there's no need to use vfio/mdev. The device driver could just allocate an aux-domain and call iommu_aux_attach_device() to setup the iommu.

Best regards,
baolu

Tian, Kevin

2020-07-15 01:23 UTC #28 lore raw

RE: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Quoted 16 lines

From: Lu Baolu <[email protected]>
Sent: Wednesday, July 15, 2020 9:00 AM
Hi Christoph and Jacob,
Quoted 10 lines · Jacob Pan
Quoted 7 lines · On Tue, 14 Jul 2020 09:25:14 +0100 Christoph Hellwig<[email protected]> wrote:
Quoted 4 lines · Lu Baolu
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
This removes the last user of iommu_aux_attach_device and iommu_aux_detach_device, which can be removed now.

Quoted 9 lines

Quoted 2 lines
it is still used in patch 2/4 inside iommu_aux_attach_group(), right?
There is a need to use this interface. For example, an aux-domain is attached to a subset of a physical device and used in the kernel. In this usage scenario, there's no need to use vfio/mdev. The device driver could just allocate an aux-domain and call iommu_aux_attach_device() to setup the iommu.

and here is one example usage for adding per-instance pagetables for drm/msm:
https://lore.kernel.org/lkml/[email protected]/

Thanks
Kevin

Alex Williamson

2020-07-29 20:33 UTC #29 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

In reply to Lu Baolu

Quoted 57 lines · On Tue, 14 Jul 2020 13:57:03 +0800 Lu Baolu <[email protected]> wrote:

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

---
 drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
 1 file changed, 7 insertions(+), 37 deletions(-)

drivers/vfio/vfio_iommu_type1.c +1 ~~-32~~

index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
 struct vfio_group {
 	struct iommu_group	*iommu_group;
 	struct list_head	next;
+	struct device		*iommu_device;
 	bool			mdev_group;	/* An mdev group */
 	bool			pinned_page_dirty_scope;
 };

@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
 	return NULL;
 }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-

drivers/vfio/vfio_iommu_type1.c static struct device *vfio_mdev_get_iommu_device(struct device *dev) · line 1637

Quoted 10 lines

drivers/vfio/vfio_iommu_type1.c +3 -3

 static int vfio_iommu_attach_group(struct vfio_domain *domain,
 				   struct vfio_group *group)
 {
 	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);

No, we previously iterated all devices in the group and used the aux interface only when we have an iommu_device supporting aux. If we simply assume an mdev group only uses an aux domain we break existing users, ex. SR-IOV VF backed mdevs. Thanks,

Alex

Quoted 22 lines

drivers/vfio/vfio_iommu_type1.c +3 -2

 	else
 		return iommu_attach_group(domain->domain, group->iommu_group);
 }

@@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain,
 				    struct vfio_group *group)
 {
 	if (group->mdev_group)
-		iommu_group_for_each_dev(group->iommu_group, domain->domain,
-					 vfio_mdev_detach_domain);
+		iommu_aux_detach_group(domain->domain, group->iommu_group,
+				       group->iommu_device);
 	else
 		iommu_detach_group(domain->domain, group->iommu_group);
 }

@@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			return 0;
 		}
 
+		group->iommu_device = iommu_device;
 		bus = iommu_device->bus;
 	}

Lu Baolu

author

2020-07-30 02:46 UTC #30 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Hi Alex,

Quoted 67 lines · Alex Williamson

Quoted 64 lines · On Tue, 14 Jul 2020 13:57:03 +0800 Lu Baolu <[email protected]> wrote:

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

---
  drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
  1 file changed, 7 insertions(+), 37 deletions(-)

drivers/vfio/vfio_iommu_type1.c +1 ~~-35~~

index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
  struct vfio_group {
  	struct iommu_group	*iommu_group;
  	struct list_head	next;
+	struct device		*iommu_device;
  	bool			mdev_group;	/* An mdev group */
  	bool			pinned_page_dirty_scope;
  };

@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
  	return NULL;
  }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
  static int vfio_iommu_attach_group(struct vfio_domain *domain,
  				   struct vfio_group *group)
  {
  	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);

Quoted 8 lines

drivers/vfio/vfio_iommu_type1.c static struct device *vfio_mdev_get_iommu_device(struct device *dev) · line 1637
Quoted 3 lines
drivers/vfio/vfio_iommu_type1.c +3 -0
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);
No, we previously iterated all devices in the group and used the aux interface only when we have an iommu_device supporting aux. If we simply assume an mdev group only uses an aux domain we break existing users, ex. SR-IOV VF backed mdevs. Thanks,

Oh, yes. Sorry! I didn't consider the physical device backed mdevs cases.

Looked into this part of code, it seems that there's a lock issue here. The group->mutex is held in iommu_group_for_each_dev() and will be acquired again in iommu_attach_device().

How about making it like:

static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)

{
        if (group->mdev_group) {
                struct device *iommu_device = group->iommu_device;

                if (WARN_ON(!iommu_device))
                        return -EINVAL;

                if (iommu_dev_feature_enabled(iommu_device, 
IOMMU_DEV_FEAT_AUX))
                        return iommu_aux_attach_device(domain->domain, 
iommu_device);
                else
                        return iommu_attach_device(domain->domain, 
iommu_device);
        } else {
                return iommu_attach_group(domain->domain, 
group->iommu_group);
        }
}

The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs in an iommu group should be derived from a same physical device.

Any thoughts?

Quoted 2 lines

Alex

Best regards,
baolu

Quoted 25 lines

Quoted 24 lines

drivers/vfio/vfio_iommu_type1.c +3 -2

  	else
  		return iommu_attach_group(domain->domain, group->iommu_group);
  }

@@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain,
  				    struct vfio_group *group)
  {
  	if (group->mdev_group)
-		iommu_group_for_each_dev(group->iommu_group, domain->domain,
-					 vfio_mdev_detach_domain);
+		iommu_aux_detach_group(domain->domain, group->iommu_group,
+				       group->iommu_device);
  	else
  		iommu_detach_group(domain->domain, group->iommu_group);
  }

@@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
  			return 0;
  		}
 
+		group->iommu_device = iommu_device;
  		bus = iommu_device->bus;
  	}

Alex Williamson

2020-07-30 21:17 UTC #31 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Quoted 79 lines · On Thu, 30 Jul 2020 10:41:32 +0800 Lu Baolu <[email protected]> wrote:

Hi Alex,

Quoted 67 lines · Alex Williamson

Quoted 64 lines · On Tue, 14 Jul 2020 13:57:03 +0800 Lu Baolu <[email protected]> wrote:

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

---
  drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
  1 file changed, 7 insertions(+), 37 deletions(-)

drivers/vfio/vfio_iommu_type1.c +1 ~~-35~~

index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
  struct vfio_group {
  	struct iommu_group	*iommu_group;
  	struct list_head	next;
+	struct device		*iommu_device;
  	bool			mdev_group;	/* An mdev group */
  	bool			pinned_page_dirty_scope;
  };

@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
  	return NULL;
  }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
  static int vfio_iommu_attach_group(struct vfio_domain *domain,
  				   struct vfio_group *group)
  {
  	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);

Quoted 8 lines

drivers/vfio/vfio_iommu_type1.c static struct device *vfio_mdev_get_iommu_device(struct device *dev) · line 1637
Quoted 3 lines
drivers/vfio/vfio_iommu_type1.c +3 -0
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);  
No, we previously iterated all devices in the group and used the aux interface only when we have an iommu_device supporting aux. If we simply assume an mdev group only uses an aux domain we break existing users, ex. SR-IOV VF backed mdevs. Thanks,

Quoted 6 lines

Oh, yes. Sorry! I didn't consider the physical device backed mdevs cases.
Looked into this part of code, it seems that there's a lock issue here. The group->mutex is held in iommu_group_for_each_dev() and will be acquired again in iommu_attach_device().

These are two different groups. We walk the devices in the mdev's group with iommu_group_for_each_dev(), holding the mdev's group lock, but we call iommu_attach_device() with iommu_device, which results in acquiring the lock for the iommu_device's group.

Quoted 24 lines

How about making it like:

static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)

{
         if (group->mdev_group) {
                 struct device *iommu_device = group->iommu_device;

                 if (WARN_ON(!iommu_device))
                         return -EINVAL;

                 if (iommu_dev_feature_enabled(iommu_device, 
IOMMU_DEV_FEAT_AUX))
                         return iommu_aux_attach_device(domain->domain, 
iommu_device);
                 else
                         return iommu_attach_device(domain->domain, 
iommu_device);
         } else {
                 return iommu_attach_group(domain->domain, 
group->iommu_group);
         }
}

Quoted 2 lines

The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs in an iommu group should be derived from a same physical device.

Have we? iommu_attach_device() will fail if the group is not singleton, but that's just encouraging us to use the _attach_group() interface where the _attach_device() interface is relegated to special cases. Ideally we'd get out of those special cases and create an _attach_group() for aux that doesn't further promote these notions.

Quoted 1 line

Any thoughts?

See my reply to Kevin, I'm thinking we need to provide a callback that can enlighten the IOMMU layer to be able to do _attach_group() with aux or separate IOMMU backed devices. Thanks,

Alex

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Lu Baolu

author

2020-07-31 01:42 UTC #32 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Hi Alex,

Quoted 85 lines · Alex Williamson

Quoted 82 lines · On Thu, 30 Jul 2020 10:41:32 +0800 Lu Baolu <[email protected]> wrote:
Hi Alex,
Quoted 67 lines · Alex Williamson
Quoted 64 lines · On Tue, 14 Jul 2020 13:57:03 +0800 Lu Baolu <[email protected]> wrote:
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
Signed-off-by Lu Baolu <[email protected]>
---
   drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
   1 file changed, 7 insertions(+), 37 deletions(-)
drivers/vfio/vfio_iommu_type1.c +1 ~~-35~~
index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
   struct vfio_group {
   	struct iommu_group	*iommu_group;
   	struct list_head	next;
+	struct device		*iommu_device;
   	bool			mdev_group;	/* An mdev group */
   	bool			pinned_page_dirty_scope;
   };
@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
   	return NULL;
   }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
   static int vfio_iommu_attach_group(struct vfio_domain *domain,
   				   struct vfio_group *group)
   {
   	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);
Quoted 8 lines
drivers/vfio/vfio_iommu_type1.c static struct device *vfio_mdev_get_iommu_device(struct device *dev) · line 1637
Quoted 3 lines
drivers/vfio/vfio_iommu_type1.c +3 -0
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);
No, we previously iterated all devices in the group and used the aux interface only when we have an iommu_device supporting aux. If we simply assume an mdev group only uses an aux domain we break existing users, ex. SR-IOV VF backed mdevs. Thanks,
Oh, yes. Sorry! I didn't consider the physical device backed mdevs cases.

Quoted 8 lines

Quoted 3 lines
Looked into this part of code, it seems that there's a lock issue here. The group->mutex is held in iommu_group_for_each_dev() and will be acquired again in iommu_attach_device().
These are two different groups. We walk the devices in the mdev's group with iommu_group_for_each_dev(), holding the mdev's group lock, but we call iommu_attach_device() with iommu_device, which results in acquiring the lock for the iommu_device's group.

You are right. Sorry for the noise. Please ignore it.

Quoted 25 lines

How about making it like:

static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)

{
          if (group->mdev_group) {
                  struct device *iommu_device = group->iommu_device;

                  if (WARN_ON(!iommu_device))
                          return -EINVAL;

                  if (iommu_dev_feature_enabled(iommu_device,
IOMMU_DEV_FEAT_AUX))
                          return iommu_aux_attach_device(domain->domain,
iommu_device);
                  else
                          return iommu_attach_device(domain->domain,
iommu_device);
          } else {
                  return iommu_attach_group(domain->domain,
group->iommu_group);
          }
}

Quoted 4 lines

Quoted 2 lines
The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs in an iommu group should be derived from a same physical device.
Have we?

We have done this with below.

static int vfio_mdev_iommu_device(struct device *dev, void *data)

{
        struct device **old = data, *new;

        new = vfio_mdev_get_iommu_device(dev);
        if (!new || (*old && *old != new))
                return -EINVAL;

        *old = new;

        return 0;
}

But I agree that as a generic iommu aux-domain api, we shouldn't put this limited assumption in it.

Quoted 5 lines

iommu_attach_device() will fail if the group is not singleton, but that's just encouraging us to use the _attach_group() interface where the _attach_device() interface is relegated to special cases. Ideally we'd get out of those special cases and create an _attach_group() for aux that doesn't further promote these notions.

Yes. Fair enough.

Quoted 6 lines

Quoted 2 lines
Any thoughts?
See my reply to Kevin, I'm thinking we need to provide a callback that can enlighten the IOMMU layer to be able to do _attach_group() with aux or separate IOMMU backed devices.

Thanks for the guide. I will check your reply.

Best regards,
baolu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Liu, Yi L

2020-07-30 09:37 UTC #33 lore raw

RE: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

In reply to Lu Baolu

Quoted 17 lines

From: Lu Baolu <[email protected]>
Sent: Tuesday, July 14, 2020 1:57 PM
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
 1 file changed, 7 insertions(+), 37 deletions(-)
drivers/vfio/vfio_iommu_type1.c +0 -0
index 5e556ac9102a..f8812e68de77 100644

drivers/vfio/vfio_iommu_type1.c struct vfio_dma { · line 103

Quoted 4 lines

drivers/vfio/vfio_iommu_type1.c +1 -0

@@ -100,6 +100,7 @@ struct vfio_dma {
 struct vfio_group {
 	struct iommu_group	*iommu_group;
 	struct list_head	next;
+	struct device		*iommu_device;

I know mdev group has only one device, so such a group has a single iommu_device. But I guess may be helpful to add a comment here or in commit message. Otherwise, it looks weird that a group structure contains a single iommu_device field instead of a list of iommu_device.

Regards,
Yi Liu

Quoted 74 lines

drivers/vfio/vfio_iommu_type1.c +3 ~~-33~~

 	bool			mdev_group;	/* An mdev group */
 	bool			pinned_page_dirty_scope;
 };

@@ -1627,45 +1628,13 @@ static struct device
 *vfio_mdev_get_iommu_device(struct device *dev)
 	return NULL;
 }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data) -{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data) -{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	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);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
 static int vfio_iommu_attach_group(struct vfio_domain *domain,
 				   struct vfio_group *group)
 {
 	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);
+		return iommu_aux_attach_group(domain->domain,
+					      group->iommu_group,
+					      group->iommu_device);
 	else
 		return iommu_attach_group(domain->domain, group-

Quoted 1 line

iommu_group); } @@ -1674,8 +1643,8 @@ static void

vfio_iommu_detach_group(struct vfio_domain *domain,

drivers/vfio/vfio_iommu_type1.c +0 -1

 				    struct vfio_group *group)
 {
 	if (group->mdev_group)
-		iommu_group_for_each_dev(group->iommu_group, domain-

Quoted 1 line

domain,

drivers/vfio/vfio_iommu_type1.c +3 -2

-					 vfio_mdev_detach_domain);
+		iommu_aux_detach_group(domain->domain, group->iommu_group,
+				       group->iommu_device);
 	else
 		iommu_detach_group(domain->domain, group->iommu_group);  }

@@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void
 *iommu_data,
 			return 0;
 		}
 
+		group->iommu_device = iommu_device;
 		bus = iommu_device->bus;
 	}
 
--

Quoted 1 line

2.17.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Lu Baolu

author

2020-07-31 01:44 UTC #34 lore raw

Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

Hi Yi,

Quoted 12 lines · Yi L

Quoted 12 lines
From: Lu Baolu<[email protected]>
Sent: Tuesday, July 14, 2020 1:57 PM
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places.
Signed-off-by Lu Baolu<[email protected]>
---
  drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
  1 file changed, 7 insertions(+), 37 deletions(-)

Quoted 14 lines

drivers/vfio/vfio_iommu_type1.c struct vfio_dma { · line 103
Quoted 9 lines
drivers/vfio/vfio_iommu_type1.c +1 -0
index 5e556ac9102a..f8812e68de77 100644
@@ -100,6 +100,7 @@ struct vfio_dma {
  struct vfio_group {
  	struct iommu_group	*iommu_group;
  	struct list_head	next;
+	struct device		*iommu_device;
I know mdev group has only one device, so such a group has a single iommu_device. But I guess may be helpful to add a comment here or in commit message. Otherwise, it looks weird that a group structure contains a single iommu_device field instead of a list of iommu_device.

Right! I will add some comments if this is still needed in the next version.

Best regards,
baolu

Lu Baolu

author

2020-07-23 13:56 UTC #35 lore raw

In reply to Lu Baolu

Hi Joerg and Alex,

Any comments for this series?

Just check to see whether we could make it for v5.9. The first aux- domain capable device driver has been posted [1].

[1] https://lore.kernel.org/linux-pci/159534667974.28840.2045034360240786644.stgit@djiang5-desk3.ch.intel.com/

Best regards,
baolu

Quoted 119 lines · Lu Baolu

This series aims to extend the IOMMU aux-domain API set so that it could be more friendly to vfio/mdev usage. The interactions between vfio/mdev and iommu during mdev creation and passthr are:
1. Create a group for mdev with iommu_group_alloc();
2. 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));
3. Allocate an aux-domain with iommu_domain_alloc();
4. Attach the aux-domain to the iommu_group.
        iommu_group_for_each_dev {
                if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
                        return iommu_aux_attach_device(domain, iommu_device);
                else
                        return iommu_attach_device(domain, iommu_device);
         }

    where, iommu_device is the aux-domain-capable device. The mdev's in
    the group are all derived from it.
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() (or other similar interfaces) doesn't work anymore.
The iommu_get_domain_for_dev() is a necessary interface for device drivers that want to support vfio/mdev based aux-domain. For example,
         unsigned long pasid;
         struct iommu_domain *domain;
         struct device *dev = mdev_dev(mdev);
         struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

         domain = iommu_get_domain_for_dev(dev);
         if (!domain)
                 return -ENODEV;

         pasid = iommu_aux_get_pasid(domain, iommu_device);
         if (pasid <= 0)
                 return -EINVAL;

          /* Program the device context */
          ....
We tried to address this by extending iommu_aux_at(de)tach_device() so that the users could pass in an optional device pointer (for example vfio/mdev). (v2 of this series)
https://lore.kernel.org/linux-iommu/[email protected]/
But that will cause a lock issue as group->mutex has been applied in iommu_group_for_each_dev(), but has to be reapplied again in the iommu_aux_attach_device().
This version tries to address this by introducing two new APIs into the aux-domain API set:
/**
* iommu_aux_attach_group - attach an aux-domain to an iommu_group which
* contains sub-devices (for example mdevs)
* derived from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived from @dev;
* @dev: the physical device which supports IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
* iommu_aux_attach_group().
*/
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group, struct device *dev)
This version is evolved according to feedbacks from Robin(v1) and Alex(v2). Your comments are very appreciated.
Best regards,
baolu
---
Change log:
- v1->v2:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Suggested by Robin.
- v2->v3:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Suggested by Alex
Lu Baolu (4):
iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
iommu: Add iommu_aux_at(de)tach_group()
iommu: Add iommu_aux_get_domain_for_dev() vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  drivers/iommu/iommu.c           | 92 ++++++++++++++++++++++++++++++---
  drivers/vfio/vfio_iommu_type1.c | 44 +++-------------
  include/linux/iommu.h           | 24 +++++++++
  3 files changed, 116 insertions(+), 44 deletions(-)