iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] iommu aux-domain APIs extensions
@ 2020-07-14  5:56 Lu Baolu
  2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
                   ` (4 more replies)
  0 siblings, 5 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-14  5:56 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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/20200707013957.23672-1-baolu.lu@linux.intel.com/

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/20200627031532.28046-1-baolu.lu@linux.intel.com/
   - Suggested by Robin.

 - v2->v3:
   - https://lore.kernel.org/linux-iommu/20200707013957.23672-1-baolu.lu@linux.intel.com/
   - 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(-)

-- 
2.17.1

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
  2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
@ 2020-07-14  5:57 ` Lu Baolu
  2020-07-29 20:03   ` Alex Williamson
  2020-07-14  5:57 ` [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group() Lu Baolu
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-14  5:57 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1ed1e14a1f0c..e1fdd3531d65 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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);
 
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
  2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
@ 2020-07-14  5:57 ` Lu Baolu
  2020-07-14 16:39   ` Jacob Pan
  2020-07-14  5:57 ` [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-14  5:57 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h | 17 +++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e1fdd3531d65..cad5a19ebf22 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,7 @@ struct iommu_group {
 	struct iommu_domain *default_domain;
 	struct iommu_domain *domain;
 	struct list_head entry;
+	unsigned int aux_domain_attached:1;
 };
 
 struct group_device {
@@ -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
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5657d4fef9f2..9506551139ab 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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)
 {
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
  2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
  2020-07-14  5:57 ` [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group() Lu Baolu
@ 2020-07-14  5:57 ` Lu Baolu
  2020-07-29 20:25   ` Alex Williamson
  2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
  2020-07-23 13:55 ` [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
  4 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-14  5:57 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <alex.williamson@redhat.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h |  7 +++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cad5a19ebf22..434bf42b6b9b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9506551139ab..cda6cef7579e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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)
 {
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
                   ` (2 preceding siblings ...)
  2020-07-14  5:57 ` [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
@ 2020-07-14  5:57 ` Lu Baolu
  2020-07-14  8:25   ` Christoph Hellwig
                     ` (2 more replies)
  2020-07-23 13:55 ` [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
  4 siblings, 3 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-14  5:57 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <baolu.lu@linux.intel.com>
---
 drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
 1 file changed, 7 insertions(+), 37 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..f8812e68de77 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -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;
 	}
 
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
@ 2020-07-14  8:25   ` Christoph Hellwig
  2020-07-14 16:29     ` Jacob Pan
  2020-07-29 20:32   ` Alex Williamson
  2020-07-30  9:36   ` Liu, Yi L
  2 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2020-07-14  8:25 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu 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.

This removes the last user of iommu_aux_attach_device and
iommu_aux_detach_device, which can be removed now.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14  8:25   ` Christoph Hellwig
@ 2020-07-14 16:29     ` Jacob Pan
  2020-07-15  1:00       ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Jacob Pan @ 2020-07-14 16:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

On Tue, 14 Jul 2020 09:25:14 +0100
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu 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.  
> 
> 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?

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-14  5:57 ` [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group() Lu Baolu
@ 2020-07-14 16:39   ` Jacob Pan
  2020-07-15  0:47     ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Jacob Pan @ 2020-07-14 16:39 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

On Tue, 14 Jul 2020 13:57:01 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 58
> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e1fdd3531d65..cad5a19ebf22 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -45,6 +45,7 @@ struct iommu_group {
>  	struct iommu_domain *default_domain;
>  	struct iommu_domain *domain;
>  	struct list_head entry;
> +	unsigned int aux_domain_attached:1;
>  };
>  
>  struct group_device {
> @@ -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;
> +
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()

> +	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
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5657d4fef9f2..9506551139ab 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -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) {

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-14 16:39   ` Jacob Pan
@ 2020-07-15  0:47     ` Lu Baolu
  2020-07-15 16:01       ` Jacob Pan
  0 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-15  0:47 UTC (permalink / raw)
  To: Jacob Pan
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

Hi Jacob,

On 7/15/20 12:39 AM, Jacob Pan wrote:
> On Tue, 14 Jul 2020 13:57:01 +0800
> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 58
>> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index e1fdd3531d65..cad5a19ebf22 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -45,6 +45,7 @@ struct iommu_group {
>>   	struct iommu_domain *default_domain;
>>   	struct iommu_domain *domain;
>>   	struct list_head entry;
>> +	unsigned int aux_domain_attached:1;
>>   };
>>   
>>   struct group_device {
>> @@ -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;
>> +
> 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14 16:29     ` Jacob Pan
@ 2020-07-15  1:00       ` Lu Baolu
  2020-07-15  1:23         ` Tian, Kevin
  0 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-15  1:00 UTC (permalink / raw)
  To: Jacob Pan, Christoph Hellwig
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

Hi Christoph and Jacob,

On 7/15/20 12:29 AM, Jacob Pan wrote:
> On Tue, 14 Jul 2020 09:25:14 +0100
> Christoph Hellwig<hch@infradead.org>  wrote:
> 
>> On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu 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.
>> 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-15  1:00       ` Lu Baolu
@ 2020-07-15  1:23         ` Tian, Kevin
  0 siblings, 0 replies; 36+ messages in thread
From: Tian, Kevin @ 2020-07-15  1:23 UTC (permalink / raw)
  To: Lu Baolu, Jacob Pan, Christoph Hellwig
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, July 15, 2020 9:00 AM
> 
> Hi Christoph and Jacob,
> 
> On 7/15/20 12:29 AM, Jacob Pan wrote:
> > On Tue, 14 Jul 2020 09:25:14 +0100
> > Christoph Hellwig<hch@infradead.org>  wrote:
> >
> >> On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu 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.
> >> 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.
> 

and here is one example usage for adding per-instance pagetables for drm/msm:
https://lore.kernel.org/lkml/20200626200414.14382-5-jcrouse@codeaurora.org/

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-15  0:47     ` Lu Baolu
@ 2020-07-15 16:01       ` Jacob Pan
  2020-07-16  1:07         ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Jacob Pan @ 2020-07-15 16:01 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

On Wed, 15 Jul 2020 08:47:36 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Jacob,
> 
> On 7/15/20 12:39 AM, Jacob Pan wrote:
> > On Tue, 14 Jul 2020 13:57:01 +0800
> > Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/iommu.c | 58
> >> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> >> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index e1fdd3531d65..cad5a19ebf22 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -45,6 +45,7 @@ struct iommu_group {
> >>   	struct iommu_domain *default_domain;
> >>   	struct iommu_domain *domain;
> >>   	struct list_head entry;
> >> +	unsigned int aux_domain_attached:1;
> >>   };
> >>   
> >>   struct group_device {
> >> @@ -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;
> >> +  
> > 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.
> 
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.

> Best regards,
> baolu

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-15 16:01       ` Jacob Pan
@ 2020-07-16  1:07         ` Lu Baolu
  2020-07-29 20:03           ` Alex Williamson
  0 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-16  1:07 UTC (permalink / raw)
  To: Jacob Pan
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Alex Williamson,
	Robin Murphy

Hi Jacob,

On 7/16/20 12:01 AM, Jacob Pan wrote:
> On Wed, 15 Jul 2020 08:47:36 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> Hi Jacob,
>>
>> On 7/15/20 12:39 AM, Jacob Pan wrote:
>>> On Tue, 14 Jul 2020 13:57:01 +0800
>>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 58
>>>> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
>>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index e1fdd3531d65..cad5a19ebf22 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -45,6 +45,7 @@ struct iommu_group {
>>>>    	struct iommu_domain *default_domain;
>>>>    	struct iommu_domain *domain;
>>>>    	struct list_head entry;
>>>> +	unsigned int aux_domain_attached:1;
>>>>    };
>>>>    
>>>>    struct group_device {
>>>> @@ -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;
>>>> +
>>> 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.
>>
> 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.

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 0/4] iommu aux-domain APIs extensions
  2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
                   ` (3 preceding siblings ...)
  2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
@ 2020-07-23 13:55 ` Lu Baolu
  4 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-23 13:55 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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

On 2020/7/14 13:56, Lu Baolu wrote:
> 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/20200707013957.23672-1-baolu.lu@linux.intel.com/
> 
> 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/20200627031532.28046-1-baolu.lu@linux.intel.com/
>     - Suggested by Robin.
> 
>   - v2->v3:
>     - https://lore.kernel.org/linux-iommu/20200707013957.23672-1-baolu.lu@linux.intel.com/
>     - 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(-)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-16  1:07         ` Lu Baolu
@ 2020-07-29 20:03           ` Alex Williamson
  2020-07-29 23:34             ` Tian, Kevin
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-29 20:03 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Thu, 16 Jul 2020 09:07:46 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Jacob,
> 
> On 7/16/20 12:01 AM, Jacob Pan wrote:
> > On Wed, 15 Jul 2020 08:47:36 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >   
> >> Hi Jacob,
> >>
> >> On 7/15/20 12:39 AM, Jacob Pan wrote:  
> >>> On Tue, 14 Jul 2020 13:57:01 +0800
> >>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
> >>>> ---
> >>>>    drivers/iommu/iommu.c | 58
> >>>> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> >>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> >>>>
> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >>>> index e1fdd3531d65..cad5a19ebf22 100644
> >>>> --- a/drivers/iommu/iommu.c
> >>>> +++ b/drivers/iommu/iommu.c
> >>>> @@ -45,6 +45,7 @@ struct iommu_group {
> >>>>    	struct iommu_domain *default_domain;
> >>>>    	struct iommu_domain *domain;
> >>>>    	struct list_head entry;
> >>>> +	unsigned int aux_domain_attached:1;
> >>>>    };
> >>>>    
> >>>>    struct group_device {
> >>>> @@ -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;
> >>>> +  
> >>> 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.
> >>  
> > 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.
> 
> > 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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
  2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
@ 2020-07-29 20:03   ` Alex Williamson
  2020-07-30  1:46     ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-29 20:03 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue, 14 Jul 2020 13:57:00 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1ed1e14a1f0c..e1fdd3531d65 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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);
>  

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-14  5:57 ` [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
@ 2020-07-29 20:25   ` Alex Williamson
  2020-07-29 23:49     ` Tian, Kevin
  2020-07-31  6:30     ` Lu Baolu
  0 siblings, 2 replies; 36+ messages in thread
From: Alex Williamson @ 2020-07-29 20:25 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue, 14 Jul 2020 13:57:02 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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 <alex.williamson@redhat.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 18 ++++++++++++++++++
>  include/linux/iommu.h |  7 +++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cad5a19ebf22..434bf42b6b9b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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;

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

> +
> +	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
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9506551139ab..cda6cef7579e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -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)
>  {

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
  2020-07-14  8:25   ` Christoph Hellwig
@ 2020-07-29 20:32   ` Alex Williamson
  2020-07-30  2:41     ` Lu Baolu
  2020-07-30  9:36   ` Liu, Yi L
  2 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-29 20:32 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue, 14 Jul 2020 13:57:03 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
>  1 file changed, 7 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..f8812e68de77 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -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);

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


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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-29 20:03           ` Alex Williamson
@ 2020-07-29 23:34             ` Tian, Kevin
  2020-07-30 19:46               ` Alex Williamson
  0 siblings, 1 reply; 36+ messages in thread
From: Tian, Kevin @ 2020-07-29 23:34 UTC (permalink / raw)
  To: Alex Williamson, Lu Baolu
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Thursday, July 30, 2020 4:04 AM
> 
> On Thu, 16 Jul 2020 09:07:46 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> > Hi Jacob,
> >
> > On 7/16/20 12:01 AM, Jacob Pan wrote:
> > > On Wed, 15 Jul 2020 08:47:36 +0800
> > > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> > >
> > >> Hi Jacob,
> > >>
> > >> On 7/15/20 12:39 AM, Jacob Pan wrote:
> > >>> On Tue, 14 Jul 2020 13:57:01 +0800
> > >>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
> > >>>> ---
> > >>>>    drivers/iommu/iommu.c | 58
> > >>>> +++++++++++++++++++++++++++++++++++++++++++
> include/linux/iommu.h |
> > >>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > >>>> index e1fdd3531d65..cad5a19ebf22 100644
> > >>>> --- a/drivers/iommu/iommu.c
> > >>>> +++ b/drivers/iommu/iommu.c
> > >>>> @@ -45,6 +45,7 @@ struct iommu_group {
> > >>>>    	struct iommu_domain *default_domain;
> > >>>>    	struct iommu_domain *domain;
> > >>>>    	struct list_head entry;
> > >>>> +	unsigned int aux_domain_attached:1;
> > >>>>    };
> > >>>>
> > >>>>    struct group_device {
> > >>>> @@ -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;
> > >>>> +
> > >>> 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.
> > >>
> > > 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.
> >
> > > 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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-29 20:25   ` Alex Williamson
@ 2020-07-29 23:49     ` Tian, Kevin
  2020-07-30 20:17       ` Alex Williamson
  2020-07-31  6:30     ` Lu Baolu
  1 sibling, 1 reply; 36+ messages in thread
From: Tian, Kevin @ 2020-07-29 23:49 UTC (permalink / raw)
  To: Alex Williamson, Lu Baolu
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Thursday, July 30, 2020 4:25 AM
> 
> On Tue, 14 Jul 2020 13:57:02 +0800
> Lu Baolu <baolu.lu@linux.intel.com> 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 <alex.williamson@redhat.com>
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  drivers/iommu/iommu.c | 18 ++++++++++++++++++
> >  include/linux/iommu.h |  7 +++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index cad5a19ebf22..434bf42b6b9b 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -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;
> 
> 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

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

> 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
> 
> > +
> > +	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
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 9506551139ab..cda6cef7579e 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -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)
> >  {

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
  2020-07-29 20:03   ` Alex Williamson
@ 2020-07-30  1:46     ` Lu Baolu
  0 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-30  1:46 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 7/30/20 4:03 AM, Alex Williamson wrote:
> On Tue, 14 Jul 2020 13:57:00 +0800
> Lu Baolu <baolu.lu@linux.intel.com> 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,

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

> 
> Alex
> 
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 1ed1e14a1f0c..e1fdd3531d65 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -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);
>>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-29 20:32   ` Alex Williamson
@ 2020-07-30  2:41     ` Lu Baolu
  2020-07-30 21:17       ` Alex Williamson
  0 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-30  2:41 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 7/30/20 4:32 AM, Alex Williamson wrote:
> On Tue, 14 Jul 2020 13:57:03 +0800
> Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
>> ---
>>   drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
>>   1 file changed, 7 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 5e556ac9102a..f8812e68de77 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -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);
> 
> 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?

> 
> Alex

Best regards,
baolu

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
  2020-07-14  8:25   ` Christoph Hellwig
  2020-07-29 20:32   ` Alex Williamson
@ 2020-07-30  9:36   ` Liu, Yi L
  2020-07-31  1:39     ` Lu Baolu
  2 siblings, 1 reply; 36+ messages in thread
From: Liu, Yi L @ 2020-07-30  9:36 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Tian, Kevin, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 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 <baolu.lu@linux.intel.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
>  1 file changed, 7 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..f8812e68de77 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -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

>  	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;
>  	}
> 
> --
> 2.17.1

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-29 23:34             ` Tian, Kevin
@ 2020-07-30 19:46               ` Alex Williamson
  2020-07-31  5:47                 ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-30 19:46 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Wed, 29 Jul 2020 23:34:40 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Thursday, July 30, 2020 4:04 AM
> > 
> > On Thu, 16 Jul 2020 09:07:46 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >   
> > > Hi Jacob,
> > >
> > > On 7/16/20 12:01 AM, Jacob Pan wrote:  
> > > > On Wed, 15 Jul 2020 08:47:36 +0800
> > > > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> > > >  
> > > >> Hi Jacob,
> > > >>
> > > >> On 7/15/20 12:39 AM, Jacob Pan wrote:  
> > > >>> On Tue, 14 Jul 2020 13:57:01 +0800
> > > >>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
> > > >>>> ---
> > > >>>>    drivers/iommu/iommu.c | 58
> > > >>>> +++++++++++++++++++++++++++++++++++++++++++  
> > include/linux/iommu.h |  
> > > >>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > >>>> index e1fdd3531d65..cad5a19ebf22 100644
> > > >>>> --- a/drivers/iommu/iommu.c
> > > >>>> +++ b/drivers/iommu/iommu.c
> > > >>>> @@ -45,6 +45,7 @@ struct iommu_group {
> > > >>>>    	struct iommu_domain *default_domain;
> > > >>>>    	struct iommu_domain *domain;
> > > >>>>    	struct list_head entry;
> > > >>>> +	unsigned int aux_domain_attached:1;
> > > >>>>    };
> > > >>>>
> > > >>>>    struct group_device {
> > > >>>> @@ -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;
> > > >>>> +  
> > > >>> 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.
> > > >>  
> > > > 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.
> > >  
> > > > 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  
> 
> 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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-29 23:49     ` Tian, Kevin
@ 2020-07-30 20:17       ` Alex Williamson
  2020-07-31  0:26         ` Tian, Kevin
  2020-07-31  2:17         ` Tian, Kevin
  0 siblings, 2 replies; 36+ messages in thread
From: Alex Williamson @ 2020-07-30 20:17 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Wed, 29 Jul 2020 23:49:20 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Thursday, July 30, 2020 4:25 AM
> > 
> > On Tue, 14 Jul 2020 13:57:02 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> 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 <alex.williamson@redhat.com>
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > ---
> > >  drivers/iommu/iommu.c | 18 ++++++++++++++++++
> > >  include/linux/iommu.h |  7 +++++++
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index cad5a19ebf22..434bf42b6b9b 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -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;  
> > 
> > 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  
> 
> 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.

> 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


> > 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
> >   
> > > +
> > > +	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
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 9506551139ab..cda6cef7579e 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -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)  
> > >  {  
> 

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-30  2:41     ` Lu Baolu
@ 2020-07-30 21:17       ` Alex Williamson
  2020-07-31  1:37         ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-30 21:17 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Thu, 30 Jul 2020 10:41:32 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Alex,
> 
> On 7/30/20 4:32 AM, Alex Williamson wrote:
> > On Tue, 14 Jul 2020 13:57:03 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
> >>   1 file changed, 7 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 5e556ac9102a..f8812e68de77 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -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);  
> > 
> > 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().

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.

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

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.

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-30 20:17       ` Alex Williamson
@ 2020-07-31  0:26         ` Tian, Kevin
  2020-07-31  2:17         ` Tian, Kevin
  1 sibling, 0 replies; 36+ messages in thread
From: Tian, Kevin @ 2020-07-31  0:26 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, July 31, 2020 4:17 AM
> 
> On Wed, 29 Jul 2020 23:49:20 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Alex Williamson <alex.williamson@redhat.com>
> > > Sent: Thursday, July 30, 2020 4:25 AM
> > >
> > > On Tue, 14 Jul 2020 13:57:02 +0800
> > > Lu Baolu <baolu.lu@linux.intel.com> 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 <alex.williamson@redhat.com>
> > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > > ---
> > > >  drivers/iommu/iommu.c | 18 ++++++++++++++++++
> > > >  include/linux/iommu.h |  7 +++++++
> > > >  2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index cad5a19ebf22..434bf42b6b9b 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -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;
> > >
> > > 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
> >
> > 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
                                             */
	...

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-30 21:17       ` Alex Williamson
@ 2020-07-31  1:37         ` Lu Baolu
  0 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-31  1:37 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 7/31/20 5:17 AM, Alex Williamson wrote:
> On Thu, 30 Jul 2020 10:41:32 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> Hi Alex,
>>
>> On 7/30/20 4:32 AM, Alex Williamson wrote:
>>> On Tue, 14 Jul 2020 13:57:03 +0800
>>> Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
>>>>    1 file changed, 7 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>>> index 5e556ac9102a..f8812e68de77 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -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);
>>>
>>> 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().
> 
> 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.

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

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-07-30  9:36   ` Liu, Yi L
@ 2020-07-31  1:39     ` Lu Baolu
  0 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-07-31  1:39 UTC (permalink / raw)
  To: Liu, Yi L, Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Tian, Kevin, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Yi,

On 7/30/20 5:36 PM, Liu, Yi L wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> 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<baolu.lu@linux.intel.com>
>> ---
>>   drivers/vfio/vfio_iommu_type1.c | 44 ++++++---------------------------
>>   1 file changed, 7 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 5e556ac9102a..f8812e68de77 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-30 20:17       ` Alex Williamson
  2020-07-31  0:26         ` Tian, Kevin
@ 2020-07-31  2:17         ` Tian, Kevin
  1 sibling, 0 replies; 36+ messages in thread
From: Tian, Kevin @ 2020-07-31  2:17 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

> From: Tian, Kevin
> Sent: Friday, July 31, 2020 8:26 AM
> 
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Friday, July 31, 2020 4:17 AM
> >
> > On Wed, 29 Jul 2020 23:49:20 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Thursday, July 30, 2020 4:25 AM
> > > >
> > > > On Tue, 14 Jul 2020 13:57:02 +0800
> > > > Lu Baolu <baolu.lu@linux.intel.com> 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 <alex.williamson@redhat.com>
> > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > > > ---
> > > > >  drivers/iommu/iommu.c | 18 ++++++++++++++++++
> > > > >  include/linux/iommu.h |  7 +++++++
> > > > >  2 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > index cad5a19ebf22..434bf42b6b9b 100644
> > > > > --- a/drivers/iommu/iommu.c
> > > > > +++ b/drivers/iommu/iommu.c
> > > > > @@ -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;
> > > >
> > > > 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
> > >
> > > 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
>                                              */
> 	...
> 
> >
> > > 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
> 
> 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-30 19:46               ` Alex Williamson
@ 2020-07-31  5:47                 ` Lu Baolu
  2020-07-31 18:05                   ` Alex Williamson
  0 siblings, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-31  5:47 UTC (permalink / raw)
  To: Alex Williamson, Tian, Kevin
  Cc: Jean-Philippe Brucker, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 2020/7/31 3:46, Alex Williamson wrote:
> On Wed, 29 Jul 2020 23:34:40 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
>>> From: Alex Williamson <alex.williamson@redhat.com>
>>> Sent: Thursday, July 30, 2020 4:04 AM
>>>
>>> On Thu, 16 Jul 2020 09:07:46 +0800
>>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>    
>>>> Hi Jacob,
>>>>
>>>> On 7/16/20 12:01 AM, Jacob Pan wrote:
>>>>> On Wed, 15 Jul 2020 08:47:36 +0800
>>>>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>>   
>>>>>> Hi Jacob,
>>>>>>
>>>>>> On 7/15/20 12:39 AM, Jacob Pan wrote:
>>>>>>> On Tue, 14 Jul 2020 13:57:01 +0800
>>>>>>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
>>>>>>>> ---
>>>>>>>>     drivers/iommu/iommu.c | 58
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/iommu.h |
>>>>>>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>>>>> index e1fdd3531d65..cad5a19ebf22 100644
>>>>>>>> --- a/drivers/iommu/iommu.c
>>>>>>>> +++ b/drivers/iommu/iommu.c
>>>>>>>> @@ -45,6 +45,7 @@ struct iommu_group {
>>>>>>>>     	struct iommu_domain *default_domain;
>>>>>>>>     	struct iommu_domain *domain;
>>>>>>>>     	struct list_head entry;
>>>>>>>> +	unsigned int aux_domain_attached:1;
>>>>>>>>     };
>>>>>>>>
>>>>>>>>     struct group_device {
>>>>>>>> @@ -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;
>>>>>>>> +
>>>>>>> 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.
>>>>>>   
>>>>> 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.
>>>>   
>>>>> 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
>>
>> 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.

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.

> 
> 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 looks good to me.

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

Another possible implementation, just for discussion purpose:

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b6858adc4f17..6474e82cf4b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-29 20:25   ` Alex Williamson
  2020-07-29 23:49     ` Tian, Kevin
@ 2020-07-31  6:30     ` Lu Baolu
  2020-07-31 18:14       ` Alex Williamson
  1 sibling, 1 reply; 36+ messages in thread
From: Lu Baolu @ 2020-07-31  6:30 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 2020/7/30 4:25, Alex Williamson wrote:
> On Tue, 14 Jul 2020 13:57:02 +0800
> Lu Baolu<baolu.lu@linux.intel.com>  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<alex.williamson@redhat.com>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 18 ++++++++++++++++++
>>   include/linux/iommu.h |  7 +++++++
>>   2 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cad5a19ebf22..434bf42b6b9b 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -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;
> 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.

> 
> 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/20200707150408.474d81f1@x1.home/

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-31  5:47                 ` Lu Baolu
@ 2020-07-31 18:05                   ` Alex Williamson
  2020-08-03  1:57                     ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-31 18:05 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Tian, Kevin, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Fri, 31 Jul 2020 13:47:57 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Alex,
> 
> On 2020/7/31 3:46, Alex Williamson wrote:
> > On Wed, 29 Jul 2020 23:34:40 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> >>> From: Alex Williamson <alex.williamson@redhat.com>
> >>> Sent: Thursday, July 30, 2020 4:04 AM
> >>>
> >>> On Thu, 16 Jul 2020 09:07:46 +0800
> >>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >>>      
> >>>> Hi Jacob,
> >>>>
> >>>> On 7/16/20 12:01 AM, Jacob Pan wrote:  
> >>>>> On Wed, 15 Jul 2020 08:47:36 +0800
> >>>>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >>>>>     
> >>>>>> Hi Jacob,
> >>>>>>
> >>>>>> On 7/15/20 12:39 AM, Jacob Pan wrote:  
> >>>>>>> On Tue, 14 Jul 2020 13:57:01 +0800
> >>>>>>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>>     drivers/iommu/iommu.c | 58
> >>>>>>>> +++++++++++++++++++++++++++++++++++++++++++  
> >>> include/linux/iommu.h |  
> >>>>>>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >>>>>>>> index e1fdd3531d65..cad5a19ebf22 100644
> >>>>>>>> --- a/drivers/iommu/iommu.c
> >>>>>>>> +++ b/drivers/iommu/iommu.c
> >>>>>>>> @@ -45,6 +45,7 @@ struct iommu_group {
> >>>>>>>>     	struct iommu_domain *default_domain;
> >>>>>>>>     	struct iommu_domain *domain;
> >>>>>>>>     	struct list_head entry;
> >>>>>>>> +	unsigned int aux_domain_attached:1;
> >>>>>>>>     };
> >>>>>>>>
> >>>>>>>>     struct group_device {
> >>>>>>>> @@ -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;
> >>>>>>>> +  
> >>>>>>> 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.
> >>>>>>     
> >>>>> 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.
> >>>>     
> >>>>> 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  
> >>
> >> 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.  
> 
> 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.
> 
> > 
> > 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 looks good to me.
> 
> > 
> > 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.  
> 
> Another possible implementation, just for discussion purpose:
> 
> 1. Add a member in group_device to save the iommu_device if it exists:
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index b6858adc4f17..6474e82cf4b4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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.

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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-31  6:30     ` Lu Baolu
@ 2020-07-31 18:14       ` Alex Williamson
  2020-08-03  2:15         ` Lu Baolu
  0 siblings, 1 reply; 36+ messages in thread
From: Alex Williamson @ 2020-07-31 18:14 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Fri, 31 Jul 2020 14:30:03 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Alex,
> 
> On 2020/7/30 4:25, Alex Williamson wrote:
> > On Tue, 14 Jul 2020 13:57:02 +0800
> > Lu Baolu<baolu.lu@linux.intel.com>  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<alex.williamson@redhat.com>
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/iommu.c | 18 ++++++++++++++++++
> >>   include/linux/iommu.h |  7 +++++++
> >>   2 files changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index cad5a19ebf22..434bf42b6b9b 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -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;  
> > 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.
> 
> > 
> > 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/20200707150408.474d81f1@x1.home/

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

> 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

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
  2020-07-31 18:05                   ` Alex Williamson
@ 2020-08-03  1:57                     ` Lu Baolu
  0 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-08-03  1:57 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Tian, Kevin, Jiang, Dave, Raj, Ashok, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 8/1/20 2:05 AM, Alex Williamson wrote:
> On Fri, 31 Jul 2020 13:47:57 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> Hi Alex,
>>
>> On 2020/7/31 3:46, Alex Williamson wrote:
>>> On Wed, 29 Jul 2020 23:34:40 +0000
>>> "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>>    
>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>> Sent: Thursday, July 30, 2020 4:04 AM
>>>>>
>>>>> On Thu, 16 Jul 2020 09:07:46 +0800
>>>>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>>       
>>>>>> Hi Jacob,
>>>>>>
>>>>>> On 7/16/20 12:01 AM, Jacob Pan wrote:
>>>>>>> On Wed, 15 Jul 2020 08:47:36 +0800
>>>>>>> Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>>>>      
>>>>>>>> Hi Jacob,
>>>>>>>>
>>>>>>>> On 7/15/20 12:39 AM, Jacob Pan wrote:
>>>>>>>>> On Tue, 14 Jul 2020 13:57:01 +0800
>>>>>>>>> Lu Baolu<baolu.lu@linux.intel.com>  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<baolu.lu@linux.intel.com>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/iommu/iommu.c | 58
>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/iommu.h |
>>>>>>>>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>>>>>>> index e1fdd3531d65..cad5a19ebf22 100644
>>>>>>>>>> --- a/drivers/iommu/iommu.c
>>>>>>>>>> +++ b/drivers/iommu/iommu.c
>>>>>>>>>> @@ -45,6 +45,7 @@ struct iommu_group {
>>>>>>>>>>      	struct iommu_domain *default_domain;
>>>>>>>>>>      	struct iommu_domain *domain;
>>>>>>>>>>      	struct list_head entry;
>>>>>>>>>> +	unsigned int aux_domain_attached:1;
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>>      struct group_device {
>>>>>>>>>> @@ -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;
>>>>>>>>>> +
>>>>>>>>> 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.
>>>>>>>>      
>>>>>>> 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.
>>>>>>      
>>>>>>> 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
>>>>
>>>> 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.
>>
>> 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.
>>
>>>
>>> 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 looks good to me.
>>
>>>
>>> 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.
>>
>> Another possible implementation, just for discussion purpose:
>>
>> 1. Add a member in group_device to save the iommu_device if it exists:
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index b6858adc4f17..6474e82cf4b4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -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.
> 
> 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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
  2020-07-31 18:14       ` Alex Williamson
@ 2020-08-03  2:15         ` Lu Baolu
  0 siblings, 0 replies; 36+ messages in thread
From: Lu Baolu @ 2020-08-03  2:15 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 8/1/20 2:14 AM, Alex Williamson wrote:
> On Fri, 31 Jul 2020 14:30:03 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> Hi Alex,
>>
>> On 2020/7/30 4:25, Alex Williamson wrote:
>>> On Tue, 14 Jul 2020 13:57:02 +0800
>>> Lu Baolu<baolu.lu@linux.intel.com>  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<alex.williamson@redhat.com>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 18 ++++++++++++++++++
>>>>    include/linux/iommu.h |  7 +++++++
>>>>    2 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index cad5a19ebf22..434bf42b6b9b 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -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;
>>> 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.
>>
>>>
>>> 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/20200707150408.474d81f1@x1.home/
> 
> Yep, I thought that was a bit of a leap in logic.
> 
>> 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.

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2020-08-03  2:20 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
2020-07-29 20:03   ` Alex Williamson
2020-07-30  1:46     ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group() Lu Baolu
2020-07-14 16:39   ` Jacob Pan
2020-07-15  0:47     ` Lu Baolu
2020-07-15 16:01       ` Jacob Pan
2020-07-16  1:07         ` Lu Baolu
2020-07-29 20:03           ` Alex Williamson
2020-07-29 23:34             ` Tian, Kevin
2020-07-30 19:46               ` Alex Williamson
2020-07-31  5:47                 ` Lu Baolu
2020-07-31 18:05                   ` Alex Williamson
2020-08-03  1:57                     ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
2020-07-29 20:25   ` Alex Williamson
2020-07-29 23:49     ` Tian, Kevin
2020-07-30 20:17       ` Alex Williamson
2020-07-31  0:26         ` Tian, Kevin
2020-07-31  2:17         ` Tian, Kevin
2020-07-31  6:30     ` Lu Baolu
2020-07-31 18:14       ` Alex Williamson
2020-08-03  2:15         ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
2020-07-14  8:25   ` Christoph Hellwig
2020-07-14 16:29     ` Jacob Pan
2020-07-15  1:00       ` Lu Baolu
2020-07-15  1:23         ` Tian, Kevin
2020-07-29 20:32   ` Alex Williamson
2020-07-30  2:41     ` Lu Baolu
2020-07-30 21:17       ` Alex Williamson
2020-07-31  1:37         ` Lu Baolu
2020-07-30  9:36   ` Liu, Yi L
2020-07-31  1:39     ` Lu Baolu
2020-07-23 13:55 ` [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).