[PATCH v2 0/5] Simplify vfio_iommu_type1 attach/detach routine

[PATCH v2 0/5] Simplify vfio_iommu_type1 attach/detach routine

[Nicolin Chen]

This is a preparatory series for IOMMUFD v2 patches. It enforces error
code -EMEDIUMTYPE in iommu_attach_device() and iommu_attach_group() when
an IOMMU domain and a device/group are incompatible. It also drops the
useless domain->ops check since it won't fail in current environment.

These allow VFIO iommu code to simplify its group attachment routine, by
avoiding the extra IOMMU domain allocations and attach/detach sequences
of the old code.

Worths mentioning the exact match for enforce_cache_coherency is removed
with this series, since there's very less value in doing that since KVM
won't be able to take advantage of it -- this just wastes domain memory.
Instead, we rely on Intel IOMMU driver taking care of that internally.

This is on github: https://github.com/nicolinc/iommufd/commits/vfio_iommu_attach

Changelog
v2:
 * Added -EMEDIUMTYPE to more IOMMU drivers that fit the category.
 * Changed dev_err to dev_dbg for -EMEDIUMTYPE to avoid kernel log spam.
 * Dropped iommu_ops patch, and removed domain->ops in VFIO directly,
   since there's no mixed-driver use case that would fail the sanity.
 * Updated commit log of the patch removing enforce_cache_coherency.
 * Fixed a misplace of "num_non_pinned_groups--" in detach_group patch.
 * Moved "num_non_pinned_groups++" in PATCH-5 to the common path between
   domain-reusing and new-domain pathways, like the code previously did.
 * Fixed a typo in EMEDIUMTYPE patch.
v1: https://lore.kernel.org/kvm/20220606061927.26049-1-nicolinc@nvidia.com/

Jason Gunthorpe (1):
  vfio/iommu_type1: Prefer to reuse domains vs match enforced cache
    coherency

Nicolin Chen (4):
  iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
  vfio/iommu_type1: Remove the domain->ops comparison
  vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
  vfio/iommu_type1: Simplify group attachment

 drivers/iommu/amd/iommu.c                   |   2 +-
 drivers/iommu/apple-dart.c                  |   4 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  12 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |   4 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 +-
 drivers/iommu/intel/iommu.c                 |   6 +-
 drivers/iommu/iommu.c                       |  28 ++
 drivers/iommu/ipmmu-vmsa.c                  |   4 +-
 drivers/iommu/mtk_iommu_v1.c                |   2 +-
 drivers/iommu/omap-iommu.c                  |   4 +-
 drivers/iommu/s390-iommu.c                  |   2 +-
 drivers/iommu/sprd-iommu.c                  |   4 +-
 drivers/iommu/tegra-gart.c                  |   2 +-
 drivers/iommu/virtio-iommu.c                |   4 +-
 drivers/vfio/vfio_iommu_type1.c             | 317 ++++++++++----------
 15 files changed, 220 insertions(+), 179 deletions(-)

-- 
2.17.1

[PATCH v2 0/5] Simplify vfio_iommu_type1 attach/detach routine

[Nicolin Chen]

This is a preparatory series for IOMMUFD v2 patches. It enforces error
code -EMEDIUMTYPE in iommu_attach_device() and iommu_attach_group() when
an IOMMU domain and a device/group are incompatible. It also drops the
useless domain->ops check since it won't fail in current environment.

These allow VFIO iommu code to simplify its group attachment routine, by
avoiding the extra IOMMU domain allocations and attach/detach sequences
of the old code.

Worths mentioning the exact match for enforce_cache_coherency is removed
with this series, since there's very less value in doing that since KVM
won't be able to take advantage of it -- this just wastes domain memory.
Instead, we rely on Intel IOMMU driver taking care of that internally.

This is on github: https://github.com/nicolinc/iommufd/commits/vfio_iommu_attach

Changelog
v2:
 * Added -EMEDIUMTYPE to more IOMMU drivers that fit the category.
 * Changed dev_err to dev_dbg for -EMEDIUMTYPE to avoid kernel log spam.
 * Dropped iommu_ops patch, and removed domain->ops in VFIO directly,
   since there's no mixed-driver use case that would fail the sanity.
 * Updated commit log of the patch removing enforce_cache_coherency.
 * Fixed a misplace of "num_non_pinned_groups--" in detach_group patch.
 * Moved "num_non_pinned_groups++" in PATCH-5 to the common path between
   domain-reusing and new-domain pathways, like the code previously did.
 * Fixed a typo in EMEDIUMTYPE patch.
v1: https://lore.kernel.org/kvm/20220606061927.26049-1-nicolinc@nvidia.com/

Jason Gunthorpe (1):
  vfio/iommu_type1: Prefer to reuse domains vs match enforced cache
    coherency

Nicolin Chen (4):
  iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
  vfio/iommu_type1: Remove the domain->ops comparison
  vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
  vfio/iommu_type1: Simplify group attachment

 drivers/iommu/amd/iommu.c                   |   2 +-
 drivers/iommu/apple-dart.c                  |   4 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  12 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |   4 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 +-
 drivers/iommu/intel/iommu.c                 |   6 +-
 drivers/iommu/iommu.c                       |  28 ++
 drivers/iommu/ipmmu-vmsa.c                  |   4 +-
 drivers/iommu/mtk_iommu_v1.c                |   2 +-
 drivers/iommu/omap-iommu.c                  |   4 +-
 drivers/iommu/s390-iommu.c                  |   2 +-
 drivers/iommu/sprd-iommu.c                  |   4 +-
 drivers/iommu/tegra-gart.c                  |   2 +-
 drivers/iommu/virtio-iommu.c                |   4 +-
 drivers/vfio/vfio_iommu_type1.c             | 317 ++++++++++----------
 15 files changed, 220 insertions(+), 179 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 0/5] Simplify vfio_iommu_type1 attach/detach routine

[Nicolin Chen via iommu]

This is a preparatory series for IOMMUFD v2 patches. It enforces error
code -EMEDIUMTYPE in iommu_attach_device() and iommu_attach_group() when
an IOMMU domain and a device/group are incompatible. It also drops the
useless domain->ops check since it won't fail in current environment.

These allow VFIO iommu code to simplify its group attachment routine, by
avoiding the extra IOMMU domain allocations and attach/detach sequences
of the old code.

Worths mentioning the exact match for enforce_cache_coherency is removed
with this series, since there's very less value in doing that since KVM
won't be able to take advantage of it -- this just wastes domain memory.
Instead, we rely on Intel IOMMU driver taking care of that internally.

This is on github: https://github.com/nicolinc/iommufd/commits/vfio_iommu_attach

Changelog
v2:
 * Added -EMEDIUMTYPE to more IOMMU drivers that fit the category.
 * Changed dev_err to dev_dbg for -EMEDIUMTYPE to avoid kernel log spam.
 * Dropped iommu_ops patch, and removed domain->ops in VFIO directly,
   since there's no mixed-driver use case that would fail the sanity.
 * Updated commit log of the patch removing enforce_cache_coherency.
 * Fixed a misplace of "num_non_pinned_groups--" in detach_group patch.
 * Moved "num_non_pinned_groups++" in PATCH-5 to the common path between
   domain-reusing and new-domain pathways, like the code previously did.
 * Fixed a typo in EMEDIUMTYPE patch.
v1: https://lore.kernel.org/kvm/20220606061927.26049-1-nicolinc@nvidia.com/

Jason Gunthorpe (1):
  vfio/iommu_type1: Prefer to reuse domains vs match enforced cache
    coherency

Nicolin Chen (4):
  iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
  vfio/iommu_type1: Remove the domain->ops comparison
  vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
  vfio/iommu_type1: Simplify group attachment

 drivers/iommu/amd/iommu.c                   |   2 +-
 drivers/iommu/apple-dart.c                  |   4 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  12 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |   4 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 +-
 drivers/iommu/intel/iommu.c                 |   6 +-
 drivers/iommu/iommu.c                       |  28 ++
 drivers/iommu/ipmmu-vmsa.c                  |   4 +-
 drivers/iommu/mtk_iommu_v1.c                |   2 +-
 drivers/iommu/omap-iommu.c                  |   4 +-
 drivers/iommu/s390-iommu.c                  |   2 +-
 drivers/iommu/sprd-iommu.c                  |   4 +-
 drivers/iommu/tegra-gart.c                  |   2 +-
 drivers/iommu/virtio-iommu.c                |   4 +-
 drivers/vfio/vfio_iommu_type1.c             | 317 ++++++++++----------
 15 files changed, 220 insertions(+), 179 deletions(-)

-- 
2.17.1

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

[PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen]

Cases like VFIO wish to attach a device to an existing domain that was
not allocated specifically from the device. This raises a condition
where the IOMMU driver can fail the domain attach because the domain and
device are incompatible with each other.

This is a soft failure that can be resolved by using a different domain.

Provide a dedicated errno from the IOMMU driver during attach that the
reason attached failed is because of domain incompatability. EMEDIUMTYPE
is chosen because it is never used within the iommu subsystem today and
evokes a sense that the 'medium' aka the domain is incompatible.

VFIO can use this to know attach is a soft failure and it should continue
searching. Otherwise the attach will be a hard failure and VFIO will
return the code to userspace.

Update all drivers to return EMEDIUMTYPE in their failure paths that are
related to domain incompatability. Also turn adjacent error prints into
debug prints, for these soft failures, to prevent a kernel log spam.

Add kdocs describing this behavior.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/amd/iommu.c                   |  2 +-
 drivers/iommu/apple-dart.c                  |  4 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  4 +--
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |  4 +--
 drivers/iommu/intel/iommu.c                 |  6 ++---
 drivers/iommu/iommu.c                       | 28 +++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c                  |  4 +--
 drivers/iommu/mtk_iommu_v1.c                |  2 +-
 drivers/iommu/omap-iommu.c                  |  4 +--
 drivers/iommu/s390-iommu.c                  |  2 +-
 drivers/iommu/sprd-iommu.c                  |  4 +--
 drivers/iommu/tegra-gart.c                  |  2 +-
 drivers/iommu/virtio-iommu.c                |  4 +--
 14 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 840831d5d2ad..ad499658a6b6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1662,7 +1662,7 @@ static int attach_device(struct device *dev,
 	if (domain->flags & PD_IOMMUV2_MASK) {
 		struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
 
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		if (def_domain->type != IOMMU_DOMAIN_IDENTITY)
 			goto out;
 
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 8af0242a90d9..e58dc310afd7 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -495,10 +495,10 @@ static int apple_dart_attach_dev(struct iommu_domain *domain,
 
 	if (cfg->stream_maps[0].dart->force_bypass &&
 	    domain->type != IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	if (!cfg->stream_maps[0].dart->supports_bypass &&
 	    domain->type == IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 
 	ret = apple_dart_finalize_domain(domain, cfg);
 	if (ret)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 88817a3376ef..1c66e4b6d852 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2420,24 +2420,24 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			goto out_unlock;
 		}
 	} else if (smmu_domain->smmu != smmu) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to SMMU %s (upstream of %s)\n",
 			dev_name(smmu_domain->smmu->dev),
 			dev_name(smmu->dev));
-		ret = -ENXIO;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to incompatible domain (%u SSID bits != %u)\n",
 			smmu_domain->s1_cfg.s1cdmax, master->ssid_bits);
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   smmu_domain->stall_enabled != master->stall_enabled) {
-		dev_err(dev, "cannot attach to stall-%s domain\n",
+		dev_dbg(dev, "cannot attach to stall-%s domain\n",
 			smmu_domain->stall_enabled ? "enabled" : "disabled");
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	}
 
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 2ed3594f384e..1d40023253d8 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1136,8 +1136,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
-		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
-		return -ENXIO;
+		dev_dbg(dev, "cannot attach to SMMU, is it on the same bus?\n");
+		return -EMEDIUMTYPE;
 	}
 
 	/*
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4c077c38fbd6..fd4e3f10d2bf 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -382,11 +382,11 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev
 	 * different IOMMUs.
 	 */
 	if (qcom_domain->iommu != qcom_iommu) {
-		dev_err(dev, "cannot attach to IOMMU %s while already "
+		dev_dbg(dev, "cannot attach to IOMMU %s while already "
 			"attached to domain on IOMMU %s\n",
 			dev_name(qcom_domain->iommu->dev),
 			dev_name(qcom_iommu->dev));
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	}
 
 	return 0;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..0dd13330fe12 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		return -ENODEV;
 
 	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
-		return -EOPNOTSUPP;
+		return -EMEDIUMTYPE;
 
 	/* check if this iommu agaw is sufficient for max mapped address */
 	addr_width = agaw_to_width(iommu->agaw);
@@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		addr_width = cap_mgaw(iommu->cap);
 
 	if (dmar_domain->max_addr > (1LL << addr_width)) {
-		dev_err(dev, "%s: iommu width (%d) is not "
+		dev_dbg(dev, "%s: iommu width (%d) is not "
 		        "sufficient for the mapped address (%llx)\n",
 		        __func__, addr_width, dmar_domain->max_addr);
-		return -EFAULT;
+		return -EMEDIUMTYPE;
 	}
 	dmar_domain->gaw = addr_width;
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..ea118a2acfe5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1972,6 +1972,20 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_device - Attach a device to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @dev: Device that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
@@ -2098,6 +2112,20 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_group - Attach an IOMMU group to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @group: IOMMU group that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	int ret;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..9e6ef5eb7722 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -628,9 +628,9 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 		 * Something is wrong, we can't attach two devices using
 		 * different IOMMUs to the same domain.
 		 */
-		dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
+		dev_dbg(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
 			dev_name(mmu->dev), dev_name(domain->mmu->dev));
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 	} else
 		dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
 
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index e1cb51b9866c..5386d889429d 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
 	/* Only allow the domain created internally. */
 	mtk_mapping = data->mapping;
 	if (mtk_mapping->domain != domain)
-		return 0;
+		return -EMEDIUMTYPE;
 
 	if (!data->m4u_dom) {
 		data->m4u_dom = dom;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..b5429adb6aed 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1471,8 +1471,8 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	/* only a single client device can be attached to a domain */
 	if (omap_domain->dev) {
-		dev_err(dev, "iommu domain is already attached\n");
-		ret = -EBUSY;
+		dev_dbg(dev, "iommu domain is already attached\n");
+		ret = -EMEDIUMTYPE;
 		goto out;
 	}
 
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index c898bcbbce11..ddcb78b284bb 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -127,7 +127,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	/* Allow only devices with identical DMA range limits */
 	} else if (domain->geometry.aperture_start != zdev->start_dma ||
 		   domain->geometry.aperture_end != zdev->end_dma) {
-		rc = -EINVAL;
+		rc = -EMEDIUMTYPE;
 		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
 		goto out_restore;
 	}
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index bd409bab6286..6881021f8c48 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -238,8 +238,8 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain,
 	size_t pgt_size = sprd_iommu_pgt_size(domain);
 
 	if (dom->sdev) {
-		pr_err("There's already a device attached to this domain.\n");
-		return -EINVAL;
+		pr_debug("There's already a device attached to this domain.\n");
+		return -EMEDIUMTYPE;
 	}
 
 	dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a6700a40a6f8..011c33e6ae31 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -112,7 +112,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	spin_lock(&gart->dom_lock);
 
 	if (gart->active_domain && gart->active_domain != domain) {
-		ret = -EBUSY;
+		ret = -EMEDIUMTYPE;
 	} else if (dev_iommu_priv_get(dev) != domain) {
 		dev_iommu_priv_set(dev, domain);
 		gart->active_domain = domain;
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 25be4b822aa0..0a8988d3f7bd 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -733,8 +733,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		 */
 		ret = viommu_domain_finalise(vdev, domain);
 	} else if (vdomain->viommu != vdev->viommu) {
-		dev_err(dev, "cannot attach to foreign vIOMMU\n");
-		ret = -EXDEV;
+		dev_dbg(dev, "cannot attach to foreign vIOMMU\n");
+		ret = -EMEDIUMTYPE;
 	}
 	mutex_unlock(&vdomain->mutex);
 
-- 
2.17.1

[PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen]

Cases like VFIO wish to attach a device to an existing domain that was
not allocated specifically from the device. This raises a condition
where the IOMMU driver can fail the domain attach because the domain and
device are incompatible with each other.

This is a soft failure that can be resolved by using a different domain.

Provide a dedicated errno from the IOMMU driver during attach that the
reason attached failed is because of domain incompatability. EMEDIUMTYPE
is chosen because it is never used within the iommu subsystem today and
evokes a sense that the 'medium' aka the domain is incompatible.

VFIO can use this to know attach is a soft failure and it should continue
searching. Otherwise the attach will be a hard failure and VFIO will
return the code to userspace.

Update all drivers to return EMEDIUMTYPE in their failure paths that are
related to domain incompatability. Also turn adjacent error prints into
debug prints, for these soft failures, to prevent a kernel log spam.

Add kdocs describing this behavior.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/amd/iommu.c                   |  2 +-
 drivers/iommu/apple-dart.c                  |  4 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  4 +--
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |  4 +--
 drivers/iommu/intel/iommu.c                 |  6 ++---
 drivers/iommu/iommu.c                       | 28 +++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c                  |  4 +--
 drivers/iommu/mtk_iommu_v1.c                |  2 +-
 drivers/iommu/omap-iommu.c                  |  4 +--
 drivers/iommu/s390-iommu.c                  |  2 +-
 drivers/iommu/sprd-iommu.c                  |  4 +--
 drivers/iommu/tegra-gart.c                  |  2 +-
 drivers/iommu/virtio-iommu.c                |  4 +--
 14 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 840831d5d2ad..ad499658a6b6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1662,7 +1662,7 @@ static int attach_device(struct device *dev,
 	if (domain->flags & PD_IOMMUV2_MASK) {
 		struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
 
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		if (def_domain->type != IOMMU_DOMAIN_IDENTITY)
 			goto out;
 
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 8af0242a90d9..e58dc310afd7 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -495,10 +495,10 @@ static int apple_dart_attach_dev(struct iommu_domain *domain,
 
 	if (cfg->stream_maps[0].dart->force_bypass &&
 	    domain->type != IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	if (!cfg->stream_maps[0].dart->supports_bypass &&
 	    domain->type == IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 
 	ret = apple_dart_finalize_domain(domain, cfg);
 	if (ret)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 88817a3376ef..1c66e4b6d852 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2420,24 +2420,24 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			goto out_unlock;
 		}
 	} else if (smmu_domain->smmu != smmu) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to SMMU %s (upstream of %s)\n",
 			dev_name(smmu_domain->smmu->dev),
 			dev_name(smmu->dev));
-		ret = -ENXIO;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to incompatible domain (%u SSID bits != %u)\n",
 			smmu_domain->s1_cfg.s1cdmax, master->ssid_bits);
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   smmu_domain->stall_enabled != master->stall_enabled) {
-		dev_err(dev, "cannot attach to stall-%s domain\n",
+		dev_dbg(dev, "cannot attach to stall-%s domain\n",
 			smmu_domain->stall_enabled ? "enabled" : "disabled");
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	}
 
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 2ed3594f384e..1d40023253d8 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1136,8 +1136,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
-		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
-		return -ENXIO;
+		dev_dbg(dev, "cannot attach to SMMU, is it on the same bus?\n");
+		return -EMEDIUMTYPE;
 	}
 
 	/*
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4c077c38fbd6..fd4e3f10d2bf 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -382,11 +382,11 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev
 	 * different IOMMUs.
 	 */
 	if (qcom_domain->iommu != qcom_iommu) {
-		dev_err(dev, "cannot attach to IOMMU %s while already "
+		dev_dbg(dev, "cannot attach to IOMMU %s while already "
 			"attached to domain on IOMMU %s\n",
 			dev_name(qcom_domain->iommu->dev),
 			dev_name(qcom_iommu->dev));
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	}
 
 	return 0;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..0dd13330fe12 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		return -ENODEV;
 
 	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
-		return -EOPNOTSUPP;
+		return -EMEDIUMTYPE;
 
 	/* check if this iommu agaw is sufficient for max mapped address */
 	addr_width = agaw_to_width(iommu->agaw);
@@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		addr_width = cap_mgaw(iommu->cap);
 
 	if (dmar_domain->max_addr > (1LL << addr_width)) {
-		dev_err(dev, "%s: iommu width (%d) is not "
+		dev_dbg(dev, "%s: iommu width (%d) is not "
 		        "sufficient for the mapped address (%llx)\n",
 		        __func__, addr_width, dmar_domain->max_addr);
-		return -EFAULT;
+		return -EMEDIUMTYPE;
 	}
 	dmar_domain->gaw = addr_width;
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..ea118a2acfe5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1972,6 +1972,20 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_device - Attach a device to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @dev: Device that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
@@ -2098,6 +2112,20 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_group - Attach an IOMMU group to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @group: IOMMU group that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	int ret;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..9e6ef5eb7722 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -628,9 +628,9 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 		 * Something is wrong, we can't attach two devices using
 		 * different IOMMUs to the same domain.
 		 */
-		dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
+		dev_dbg(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
 			dev_name(mmu->dev), dev_name(domain->mmu->dev));
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 	} else
 		dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
 
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index e1cb51b9866c..5386d889429d 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
 	/* Only allow the domain created internally. */
 	mtk_mapping = data->mapping;
 	if (mtk_mapping->domain != domain)
-		return 0;
+		return -EMEDIUMTYPE;
 
 	if (!data->m4u_dom) {
 		data->m4u_dom = dom;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..b5429adb6aed 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1471,8 +1471,8 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	/* only a single client device can be attached to a domain */
 	if (omap_domain->dev) {
-		dev_err(dev, "iommu domain is already attached\n");
-		ret = -EBUSY;
+		dev_dbg(dev, "iommu domain is already attached\n");
+		ret = -EMEDIUMTYPE;
 		goto out;
 	}
 
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index c898bcbbce11..ddcb78b284bb 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -127,7 +127,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	/* Allow only devices with identical DMA range limits */
 	} else if (domain->geometry.aperture_start != zdev->start_dma ||
 		   domain->geometry.aperture_end != zdev->end_dma) {
-		rc = -EINVAL;
+		rc = -EMEDIUMTYPE;
 		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
 		goto out_restore;
 	}
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index bd409bab6286..6881021f8c48 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -238,8 +238,8 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain,
 	size_t pgt_size = sprd_iommu_pgt_size(domain);
 
 	if (dom->sdev) {
-		pr_err("There's already a device attached to this domain.\n");
-		return -EINVAL;
+		pr_debug("There's already a device attached to this domain.\n");
+		return -EMEDIUMTYPE;
 	}
 
 	dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a6700a40a6f8..011c33e6ae31 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -112,7 +112,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	spin_lock(&gart->dom_lock);
 
 	if (gart->active_domain && gart->active_domain != domain) {
-		ret = -EBUSY;
+		ret = -EMEDIUMTYPE;
 	} else if (dev_iommu_priv_get(dev) != domain) {
 		dev_iommu_priv_set(dev, domain);
 		gart->active_domain = domain;
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 25be4b822aa0..0a8988d3f7bd 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -733,8 +733,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		 */
 		ret = viommu_domain_finalise(vdev, domain);
 	} else if (vdomain->viommu != vdev->viommu) {
-		dev_err(dev, "cannot attach to foreign vIOMMU\n");
-		ret = -EXDEV;
+		dev_dbg(dev, "cannot attach to foreign vIOMMU\n");
+		ret = -EMEDIUMTYPE;
 	}
 	mutex_unlock(&vdomain->mutex);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen via iommu]

Cases like VFIO wish to attach a device to an existing domain that was
not allocated specifically from the device. This raises a condition
where the IOMMU driver can fail the domain attach because the domain and
device are incompatible with each other.

This is a soft failure that can be resolved by using a different domain.

Provide a dedicated errno from the IOMMU driver during attach that the
reason attached failed is because of domain incompatability. EMEDIUMTYPE
is chosen because it is never used within the iommu subsystem today and
evokes a sense that the 'medium' aka the domain is incompatible.

VFIO can use this to know attach is a soft failure and it should continue
searching. Otherwise the attach will be a hard failure and VFIO will
return the code to userspace.

Update all drivers to return EMEDIUMTYPE in their failure paths that are
related to domain incompatability. Also turn adjacent error prints into
debug prints, for these soft failures, to prevent a kernel log spam.

Add kdocs describing this behavior.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/amd/iommu.c                   |  2 +-
 drivers/iommu/apple-dart.c                  |  4 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  4 +--
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |  4 +--
 drivers/iommu/intel/iommu.c                 |  6 ++---
 drivers/iommu/iommu.c                       | 28 +++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c                  |  4 +--
 drivers/iommu/mtk_iommu_v1.c                |  2 +-
 drivers/iommu/omap-iommu.c                  |  4 +--
 drivers/iommu/s390-iommu.c                  |  2 +-
 drivers/iommu/sprd-iommu.c                  |  4 +--
 drivers/iommu/tegra-gart.c                  |  2 +-
 drivers/iommu/virtio-iommu.c                |  4 +--
 14 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 840831d5d2ad..ad499658a6b6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1662,7 +1662,7 @@ static int attach_device(struct device *dev,
 	if (domain->flags & PD_IOMMUV2_MASK) {
 		struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
 
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		if (def_domain->type != IOMMU_DOMAIN_IDENTITY)
 			goto out;
 
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 8af0242a90d9..e58dc310afd7 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -495,10 +495,10 @@ static int apple_dart_attach_dev(struct iommu_domain *domain,
 
 	if (cfg->stream_maps[0].dart->force_bypass &&
 	    domain->type != IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	if (!cfg->stream_maps[0].dart->supports_bypass &&
 	    domain->type == IOMMU_DOMAIN_IDENTITY)
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 
 	ret = apple_dart_finalize_domain(domain, cfg);
 	if (ret)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 88817a3376ef..1c66e4b6d852 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2420,24 +2420,24 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			goto out_unlock;
 		}
 	} else if (smmu_domain->smmu != smmu) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to SMMU %s (upstream of %s)\n",
 			dev_name(smmu_domain->smmu->dev),
 			dev_name(smmu->dev));
-		ret = -ENXIO;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
-		dev_err(dev,
+		dev_dbg(dev,
 			"cannot attach to incompatible domain (%u SSID bits != %u)\n",
 			smmu_domain->s1_cfg.s1cdmax, master->ssid_bits);
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
 		   smmu_domain->stall_enabled != master->stall_enabled) {
-		dev_err(dev, "cannot attach to stall-%s domain\n",
+		dev_dbg(dev, "cannot attach to stall-%s domain\n",
 			smmu_domain->stall_enabled ? "enabled" : "disabled");
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 		goto out_unlock;
 	}
 
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 2ed3594f384e..1d40023253d8 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1136,8 +1136,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
-		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
-		return -ENXIO;
+		dev_dbg(dev, "cannot attach to SMMU, is it on the same bus?\n");
+		return -EMEDIUMTYPE;
 	}
 
 	/*
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4c077c38fbd6..fd4e3f10d2bf 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -382,11 +382,11 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev
 	 * different IOMMUs.
 	 */
 	if (qcom_domain->iommu != qcom_iommu) {
-		dev_err(dev, "cannot attach to IOMMU %s while already "
+		dev_dbg(dev, "cannot attach to IOMMU %s while already "
 			"attached to domain on IOMMU %s\n",
 			dev_name(qcom_domain->iommu->dev),
 			dev_name(qcom_iommu->dev));
-		return -EINVAL;
+		return -EMEDIUMTYPE;
 	}
 
 	return 0;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..0dd13330fe12 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		return -ENODEV;
 
 	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
-		return -EOPNOTSUPP;
+		return -EMEDIUMTYPE;
 
 	/* check if this iommu agaw is sufficient for max mapped address */
 	addr_width = agaw_to_width(iommu->agaw);
@@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 		addr_width = cap_mgaw(iommu->cap);
 
 	if (dmar_domain->max_addr > (1LL << addr_width)) {
-		dev_err(dev, "%s: iommu width (%d) is not "
+		dev_dbg(dev, "%s: iommu width (%d) is not "
 		        "sufficient for the mapped address (%llx)\n",
 		        __func__, addr_width, dmar_domain->max_addr);
-		return -EFAULT;
+		return -EMEDIUMTYPE;
 	}
 	dmar_domain->gaw = addr_width;
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..ea118a2acfe5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1972,6 +1972,20 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_device - Attach a device to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @dev: Device that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
@@ -2098,6 +2112,20 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	return ret;
 }
 
+/**
+ * iommu_attach_group - Attach an IOMMU group to an IOMMU domain
+ * @domain: IOMMU domain to attach
+ * @group: IOMMU group that will be attached
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and
+ * the device are incompatible in some way. This indicates that a caller should
+ * try another existing IOMMU domain or allocate a new one. And note that it's
+ * recommended to keep kernel print free or simply use dev_dbg() when reporting
+ * -EMEDIUMTYPE error, since this function can be called to test compatibility
+ * with domains that will fail the test, which will result in a kernel log spam.
+ */
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	int ret;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..9e6ef5eb7722 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -628,9 +628,9 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 		 * Something is wrong, we can't attach two devices using
 		 * different IOMMUs to the same domain.
 		 */
-		dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
+		dev_dbg(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
 			dev_name(mmu->dev), dev_name(domain->mmu->dev));
-		ret = -EINVAL;
+		ret = -EMEDIUMTYPE;
 	} else
 		dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
 
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index e1cb51b9866c..5386d889429d 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
 	/* Only allow the domain created internally. */
 	mtk_mapping = data->mapping;
 	if (mtk_mapping->domain != domain)
-		return 0;
+		return -EMEDIUMTYPE;
 
 	if (!data->m4u_dom) {
 		data->m4u_dom = dom;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..b5429adb6aed 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1471,8 +1471,8 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	/* only a single client device can be attached to a domain */
 	if (omap_domain->dev) {
-		dev_err(dev, "iommu domain is already attached\n");
-		ret = -EBUSY;
+		dev_dbg(dev, "iommu domain is already attached\n");
+		ret = -EMEDIUMTYPE;
 		goto out;
 	}
 
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index c898bcbbce11..ddcb78b284bb 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -127,7 +127,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	/* Allow only devices with identical DMA range limits */
 	} else if (domain->geometry.aperture_start != zdev->start_dma ||
 		   domain->geometry.aperture_end != zdev->end_dma) {
-		rc = -EINVAL;
+		rc = -EMEDIUMTYPE;
 		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
 		goto out_restore;
 	}
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index bd409bab6286..6881021f8c48 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -238,8 +238,8 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain,
 	size_t pgt_size = sprd_iommu_pgt_size(domain);
 
 	if (dom->sdev) {
-		pr_err("There's already a device attached to this domain.\n");
-		return -EINVAL;
+		pr_debug("There's already a device attached to this domain.\n");
+		return -EMEDIUMTYPE;
 	}
 
 	dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a6700a40a6f8..011c33e6ae31 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -112,7 +112,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	spin_lock(&gart->dom_lock);
 
 	if (gart->active_domain && gart->active_domain != domain) {
-		ret = -EBUSY;
+		ret = -EMEDIUMTYPE;
 	} else if (dev_iommu_priv_get(dev) != domain) {
 		dev_iommu_priv_set(dev, domain);
 		gart->active_domain = domain;
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 25be4b822aa0..0a8988d3f7bd 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -733,8 +733,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		 */
 		ret = viommu_domain_finalise(vdev, domain);
 	} else if (vdomain->viommu != vdev->viommu) {
-		dev_err(dev, "cannot attach to foreign vIOMMU\n");
-		ret = -EXDEV;
+		dev_dbg(dev, "cannot attach to foreign vIOMMU\n");
+		ret = -EMEDIUMTYPE;
 	}
 	mutex_unlock(&vdomain->mutex);
 
-- 
2.17.1

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

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Baolu Lu]

On 2022/6/16 08:03, Nicolin Chen wrote:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 44016594831d..0dd13330fe12 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		return -ENODEV;
>   
>   	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> -		return -EOPNOTSUPP;
> +		return -EMEDIUMTYPE;
>   
>   	/* check if this iommu agaw is sufficient for max mapped address */
>   	addr_width = agaw_to_width(iommu->agaw);
> @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		addr_width = cap_mgaw(iommu->cap);
>   
>   	if (dmar_domain->max_addr > (1LL << addr_width)) {
> -		dev_err(dev, "%s: iommu width (%d) is not "
> +		dev_dbg(dev, "%s: iommu width (%d) is not "
>   		        "sufficient for the mapped address (%llx)\n",
>   		        __func__, addr_width, dmar_domain->max_addr);
> -		return -EFAULT;
> +		return -EMEDIUMTYPE;
>   	}
>   	dmar_domain->gaw = addr_width;

Can we simply remove the dev_err()? As the return value has explicitly
explained the failure reason, putting a print statement won't help much.

Best regards,
baolu

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Baolu Lu]

On 2022/6/16 08:03, Nicolin Chen wrote:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 44016594831d..0dd13330fe12 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		return -ENODEV;
>   
>   	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> -		return -EOPNOTSUPP;
> +		return -EMEDIUMTYPE;
>   
>   	/* check if this iommu agaw is sufficient for max mapped address */
>   	addr_width = agaw_to_width(iommu->agaw);
> @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		addr_width = cap_mgaw(iommu->cap);
>   
>   	if (dmar_domain->max_addr > (1LL << addr_width)) {
> -		dev_err(dev, "%s: iommu width (%d) is not "
> +		dev_dbg(dev, "%s: iommu width (%d) is not "
>   		        "sufficient for the mapped address (%llx)\n",
>   		        __func__, addr_width, dmar_domain->max_addr);
> -		return -EFAULT;
> +		return -EMEDIUMTYPE;
>   	}
>   	dmar_domain->gaw = addr_width;

Can we simply remove the dev_err()? As the return value has explicitly
explained the failure reason, putting a print statement won't help much.

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Baolu Lu]

On 2022/6/16 08:03, Nicolin Chen wrote:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 44016594831d..0dd13330fe12 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		return -ENODEV;
>   
>   	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> -		return -EOPNOTSUPP;
> +		return -EMEDIUMTYPE;
>   
>   	/* check if this iommu agaw is sufficient for max mapped address */
>   	addr_width = agaw_to_width(iommu->agaw);
> @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
>   		addr_width = cap_mgaw(iommu->cap);
>   
>   	if (dmar_domain->max_addr > (1LL << addr_width)) {
> -		dev_err(dev, "%s: iommu width (%d) is not "
> +		dev_dbg(dev, "%s: iommu width (%d) is not "
>   		        "sufficient for the mapped address (%llx)\n",
>   		        __func__, addr_width, dmar_domain->max_addr);
> -		return -EFAULT;
> +		return -EMEDIUMTYPE;
>   	}
>   	dmar_domain->gaw = addr_width;

Can we simply remove the dev_err()? As the return value has explicitly
explained the failure reason, putting a print statement won't help much.

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

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen]

On Thu, Jun 16, 2022 at 10:09:49AM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022/6/16 08:03, Nicolin Chen wrote:
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 44016594831d..0dd13330fe12 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               return -ENODEV;
> > 
> >       if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> > -             return -EOPNOTSUPP;
> > +             return -EMEDIUMTYPE;
> > 
> >       /* check if this iommu agaw is sufficient for max mapped address */
> >       addr_width = agaw_to_width(iommu->agaw);
> > @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               addr_width = cap_mgaw(iommu->cap);
> > 
> >       if (dmar_domain->max_addr > (1LL << addr_width)) {
> > -             dev_err(dev, "%s: iommu width (%d) is not "
> > +             dev_dbg(dev, "%s: iommu width (%d) is not "
> >                       "sufficient for the mapped address (%llx)\n",
> >                       __func__, addr_width, dmar_domain->max_addr);
> > -             return -EFAULT;
> > +             return -EMEDIUMTYPE;
> >       }
> >       dmar_domain->gaw = addr_width;
> 
> Can we simply remove the dev_err()? As the return value has explicitly
> explained the failure reason, putting a print statement won't help much.

Yes. As long as no one has objection, I can remove that in the next
version.

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen]

On Thu, Jun 16, 2022 at 10:09:49AM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022/6/16 08:03, Nicolin Chen wrote:
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 44016594831d..0dd13330fe12 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               return -ENODEV;
> > 
> >       if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> > -             return -EOPNOTSUPP;
> > +             return -EMEDIUMTYPE;
> > 
> >       /* check if this iommu agaw is sufficient for max mapped address */
> >       addr_width = agaw_to_width(iommu->agaw);
> > @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               addr_width = cap_mgaw(iommu->cap);
> > 
> >       if (dmar_domain->max_addr > (1LL << addr_width)) {
> > -             dev_err(dev, "%s: iommu width (%d) is not "
> > +             dev_dbg(dev, "%s: iommu width (%d) is not "
> >                       "sufficient for the mapped address (%llx)\n",
> >                       __func__, addr_width, dmar_domain->max_addr);
> > -             return -EFAULT;
> > +             return -EMEDIUMTYPE;
> >       }
> >       dmar_domain->gaw = addr_width;
> 
> Can we simply remove the dev_err()? As the return value has explicitly
> explained the failure reason, putting a print statement won't help much.

Yes. As long as no one has objection, I can remove that in the next
version.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Nicolin Chen via iommu]

On Thu, Jun 16, 2022 at 10:09:49AM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022/6/16 08:03, Nicolin Chen wrote:
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 44016594831d..0dd13330fe12 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4323,7 +4323,7 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               return -ENODEV;
> > 
> >       if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
> > -             return -EOPNOTSUPP;
> > +             return -EMEDIUMTYPE;
> > 
> >       /* check if this iommu agaw is sufficient for max mapped address */
> >       addr_width = agaw_to_width(iommu->agaw);
> > @@ -4331,10 +4331,10 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
> >               addr_width = cap_mgaw(iommu->cap);
> > 
> >       if (dmar_domain->max_addr > (1LL << addr_width)) {
> > -             dev_err(dev, "%s: iommu width (%d) is not "
> > +             dev_dbg(dev, "%s: iommu width (%d) is not "
> >                       "sufficient for the mapped address (%llx)\n",
> >                       __func__, addr_width, dmar_domain->max_addr);
> > -             return -EFAULT;
> > +             return -EMEDIUMTYPE;
> >       }
> >       dmar_domain->gaw = addr_width;
> 
> Can we simply remove the dev_err()? As the return value has explicitly
> explained the failure reason, putting a print statement won't help much.

Yes. As long as no one has objection, I can remove that in the next
version.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Cases like VFIO wish to attach a device to an existing domain that was
> not allocated specifically from the device. This raises a condition
> where the IOMMU driver can fail the domain attach because the domain and
> device are incompatible with each other.
> 
> This is a soft failure that can be resolved by using a different domain.
> 
> Provide a dedicated errno from the IOMMU driver during attach that the
> reason attached failed is because of domain incompatability. EMEDIUMTYPE
> is chosen because it is never used within the iommu subsystem today and
> evokes a sense that the 'medium' aka the domain is incompatible.
> 
> VFIO can use this to know attach is a soft failure and it should continue
> searching. Otherwise the attach will be a hard failure and VFIO will
> return the code to userspace.
> 
> Update all drivers to return EMEDIUMTYPE in their failure paths that are
> related to domain incompatability. Also turn adjacent error prints into
> debug prints, for these soft failures, to prevent a kernel log spam.
> 
> Add kdocs describing this behavior.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Cases like VFIO wish to attach a device to an existing domain that was
> not allocated specifically from the device. This raises a condition
> where the IOMMU driver can fail the domain attach because the domain and
> device are incompatible with each other.
> 
> This is a soft failure that can be resolved by using a different domain.
> 
> Provide a dedicated errno from the IOMMU driver during attach that the
> reason attached failed is because of domain incompatability. EMEDIUMTYPE
> is chosen because it is never used within the iommu subsystem today and
> evokes a sense that the 'medium' aka the domain is incompatible.
> 
> VFIO can use this to know attach is a soft failure and it should continue
> searching. Otherwise the attach will be a hard failure and VFIO will
> return the code to userspace.
> 
> Update all drivers to return EMEDIUMTYPE in their failure paths that are
> related to domain incompatability. Also turn adjacent error prints into
> debug prints, for these soft failures, to prevent a kernel log spam.
> 
> Add kdocs describing this behavior.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Cases like VFIO wish to attach a device to an existing domain that was
> not allocated specifically from the device. This raises a condition
> where the IOMMU driver can fail the domain attach because the domain and
> device are incompatible with each other.
> 
> This is a soft failure that can be resolved by using a different domain.
> 
> Provide a dedicated errno from the IOMMU driver during attach that the
> reason attached failed is because of domain incompatability. EMEDIUMTYPE
> is chosen because it is never used within the iommu subsystem today and
> evokes a sense that the 'medium' aka the domain is incompatible.
> 
> VFIO can use this to know attach is a soft failure and it should continue
> searching. Otherwise the attach will be a hard failure and VFIO will
> return the code to userspace.
> 
> Update all drivers to return EMEDIUMTYPE in their failure paths that are
> related to domain incompatability. Also turn adjacent error prints into
> debug prints, for these soft failures, to prevent a kernel log spam.
> 
> Add kdocs describing this behavior.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v2 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Cases like VFIO wish to attach a device to an existing domain that was
> not allocated specifically from the device. This raises a condition
> where the IOMMU driver can fail the domain attach because the domain and
> device are incompatible with each other.
> 
> This is a soft failure that can be resolved by using a different domain.
> 
> Provide a dedicated errno from the IOMMU driver during attach that the
> reason attached failed is because of domain incompatability. EMEDIUMTYPE
> is chosen because it is never used within the iommu subsystem today and
> evokes a sense that the 'medium' aka the domain is incompatible.
> 
> VFIO can use this to know attach is a soft failure and it should continue
> searching. Otherwise the attach will be a hard failure and VFIO will
> return the code to userspace.
> 
> Update all drivers to return EMEDIUMTYPE in their failure paths that are
> related to domain incompatability. Also turn adjacent error prints into
> debug prints, for these soft failures, to prevent a kernel log spam.
> 
> Add kdocs describing this behavior.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

The KVM mechanism for controlling wbinvd is based on OR of the coherency
property of all devices attached to a guest, no matter those devices are
attached to a single domain or multiple domains.

So, there is no value in trying to push a device that could do enforced
cache coherency to a dedicated domain vs re-using an existing domain
which is non-coherent since KVM won't be able to take advantage of it.
This just wastes domain memory.

Simplify this code and eliminate the test. This removes the only logic
that needed to have a dummy domain attached prior to searching for a
matching domain and simplifies the next patches.

It's unclear whether we want to further optimize the Intel driver to
update the domain coherency after a device is detached from it, at
least not before KVM can be verified to handle such dynamics in related
emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
we don't see an usage requiring such optimization as the only device
which imposes such non-coherency is Intel GPU which even doesn't
support hotplug/hot remove.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e357..f4e3b423a453 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 * testing if they're on the same bus_type.
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops &&
-		    d->enforce_cache_coherency ==
-			    domain->enforce_cache_coherency) {
+		if (d->domain->ops == domain->domain->ops) {
 			iommu_detach_group(domain->domain, group->iommu_group);
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
-- 
2.17.1

[PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

The KVM mechanism for controlling wbinvd is based on OR of the coherency
property of all devices attached to a guest, no matter those devices are
attached to a single domain or multiple domains.

So, there is no value in trying to push a device that could do enforced
cache coherency to a dedicated domain vs re-using an existing domain
which is non-coherent since KVM won't be able to take advantage of it.
This just wastes domain memory.

Simplify this code and eliminate the test. This removes the only logic
that needed to have a dummy domain attached prior to searching for a
matching domain and simplifies the next patches.

It's unclear whether we want to further optimize the Intel driver to
update the domain coherency after a device is detached from it, at
least not before KVM can be verified to handle such dynamics in related
emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
we don't see an usage requiring such optimization as the only device
which imposes such non-coherency is Intel GPU which even doesn't
support hotplug/hot remove.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e357..f4e3b423a453 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 * testing if they're on the same bus_type.
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops &&
-		    d->enforce_cache_coherency ==
-			    domain->enforce_cache_coherency) {
+		if (d->domain->ops == domain->domain->ops) {
 			iommu_detach_group(domain->domain, group->iommu_group);
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen via iommu]

From: Jason Gunthorpe <jgg@nvidia.com>

The KVM mechanism for controlling wbinvd is based on OR of the coherency
property of all devices attached to a guest, no matter those devices are
attached to a single domain or multiple domains.

So, there is no value in trying to push a device that could do enforced
cache coherency to a dedicated domain vs re-using an existing domain
which is non-coherent since KVM won't be able to take advantage of it.
This just wastes domain memory.

Simplify this code and eliminate the test. This removes the only logic
that needed to have a dummy domain attached prior to searching for a
matching domain and simplifies the next patches.

It's unclear whether we want to further optimize the Intel driver to
update the domain coherency after a device is detached from it, at
least not before KVM can be verified to handle such dynamics in related
emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
we don't see an usage requiring such optimization as the only device
which imposes such non-coherency is Intel GPU which even doesn't
support hotplug/hot remove.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e357..f4e3b423a453 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 * testing if they're on the same bus_type.
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops &&
-		    d->enforce_cache_coherency ==
-			    domain->enforce_cache_coherency) {
+		if (d->domain->ops == domain->domain->ops) {
 			iommu_detach_group(domain->domain, group->iommu_group);
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
-- 
2.17.1

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

RE: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Alex Williamson]

On Wed, 15 Jun 2022 17:03:01 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.

The 2nd paragraph above is quite misleading in this respect.  I think
it would be more accurate to explain that the benefit to using separate
domains was that devices attached to domains supporting enforced cache
coherency always mapped with the attributes necessary to provide that
feature, therefore if a non-enforced domain was dropped, the associated
group removal would re-trigger an evaluation by KVM.  We can then go on
to discuss that in practice the only known cases of such mixed domains
included an Intel IGD device behind an IOMMU lacking snoop control,
where such devices do not support hotplug, therefore this scenario lacks
testing and is not considered sufficiently relevant to support.  Thanks,

Alex

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e357..f4e3b423a453 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 * testing if they're on the same bus_type.
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
> -		if (d->domain->ops == domain->domain->ops &&
> -		    d->enforce_cache_coherency ==
> -			    domain->enforce_cache_coherency) {
> +		if (d->domain->ops == domain->domain->ops) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Alex Williamson]

On Wed, 15 Jun 2022 17:03:01 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.

The 2nd paragraph above is quite misleading in this respect.  I think
it would be more accurate to explain that the benefit to using separate
domains was that devices attached to domains supporting enforced cache
coherency always mapped with the attributes necessary to provide that
feature, therefore if a non-enforced domain was dropped, the associated
group removal would re-trigger an evaluation by KVM.  We can then go on
to discuss that in practice the only known cases of such mixed domains
included an Intel IGD device behind an IOMMU lacking snoop control,
where such devices do not support hotplug, therefore this scenario lacks
testing and is not considered sufficiently relevant to support.  Thanks,

Alex

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e357..f4e3b423a453 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 * testing if they're on the same bus_type.
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
> -		if (d->domain->ops == domain->domain->ops &&
> -		    d->enforce_cache_coherency ==
> -			    domain->enforce_cache_coherency) {
> +		if (d->domain->ops == domain->domain->ops) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Alex Williamson]

On Wed, 15 Jun 2022 17:03:01 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.

The 2nd paragraph above is quite misleading in this respect.  I think
it would be more accurate to explain that the benefit to using separate
domains was that devices attached to domains supporting enforced cache
coherency always mapped with the attributes necessary to provide that
feature, therefore if a non-enforced domain was dropped, the associated
group removal would re-trigger an evaluation by KVM.  We can then go on
to discuss that in practice the only known cases of such mixed domains
included an Intel IGD device behind an IOMMU lacking snoop control,
where such devices do not support hotplug, therefore this scenario lacks
testing and is not considered sufficiently relevant to support.  Thanks,

Alex

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e357..f4e3b423a453 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 * testing if they're on the same bus_type.
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
> -		if (d->domain->ops == domain->domain->ops &&
> -		    d->enforce_cache_coherency ==
> -			    domain->enforce_cache_coherency) {
> +		if (d->domain->ops == domain->domain->ops) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {

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

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Alex Williamson]

On Wed, 15 Jun 2022 17:03:01 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The KVM mechanism for controlling wbinvd is based on OR of the coherency
> property of all devices attached to a guest, no matter those devices are
> attached to a single domain or multiple domains.
> 
> So, there is no value in trying to push a device that could do enforced
> cache coherency to a dedicated domain vs re-using an existing domain
> which is non-coherent since KVM won't be able to take advantage of it.
> This just wastes domain memory.
> 
> Simplify this code and eliminate the test. This removes the only logic
> that needed to have a dummy domain attached prior to searching for a
> matching domain and simplifies the next patches.
> 
> It's unclear whether we want to further optimize the Intel driver to
> update the domain coherency after a device is detached from it, at
> least not before KVM can be verified to handle such dynamics in related
> emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> we don't see an usage requiring such optimization as the only device
> which imposes such non-coherency is Intel GPU which even doesn't
> support hotplug/hot remove.

The 2nd paragraph above is quite misleading in this respect.  I think
it would be more accurate to explain that the benefit to using separate
domains was that devices attached to domains supporting enforced cache
coherency always mapped with the attributes necessary to provide that
feature, therefore if a non-enforced domain was dropped, the associated
group removal would re-trigger an evaluation by KVM.  We can then go on
to discuss that in practice the only known cases of such mixed domains
included an Intel IGD device behind an IOMMU lacking snoop control,
where such devices do not support hotplug, therefore this scenario lacks
testing and is not considered sufficiently relevant to support.  Thanks,

Alex

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e357..f4e3b423a453 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2285,9 +2285,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 * testing if they're on the same bus_type.
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
> -		if (d->domain->ops == domain->domain->ops &&
> -		    d->enforce_cache_coherency ==
> -			    domain->enforce_cache_coherency) {
> +		if (d->domain->ops == domain->domain->ops) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {

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

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen]

On Tue, Jun 21, 2022 at 04:46:02PM -0600, Alex Williamson wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 15 Jun 2022 17:03:01 -0700
> Nicolin Chen <nicolinc@nvidia.com> wrote:
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> >
> > The KVM mechanism for controlling wbinvd is based on OR of the coherency
> > property of all devices attached to a guest, no matter those devices are
> > attached to a single domain or multiple domains.
> >
> > So, there is no value in trying to push a device that could do enforced
> > cache coherency to a dedicated domain vs re-using an existing domain
> > which is non-coherent since KVM won't be able to take advantage of it.
> > This just wastes domain memory.
> >
> > Simplify this code and eliminate the test. This removes the only logic
> > that needed to have a dummy domain attached prior to searching for a
> > matching domain and simplifies the next patches.
> >
> > It's unclear whether we want to further optimize the Intel driver to
> > update the domain coherency after a device is detached from it, at
> > least not before KVM can be verified to handle such dynamics in related
> > emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> > we don't see an usage requiring such optimization as the only device
> > which imposes such non-coherency is Intel GPU which even doesn't
> > support hotplug/hot remove.
> 
> The 2nd paragraph above is quite misleading in this respect.  I think
> it would be more accurate to explain that the benefit to using separate
> domains was that devices attached to domains supporting enforced cache
> coherency always mapped with the attributes necessary to provide that
> feature, therefore if a non-enforced domain was dropped, the associated
> group removal would re-trigger an evaluation by KVM.  We can then go on
> to discuss that in practice the only known cases of such mixed domains
> included an Intel IGD device behind an IOMMU lacking snoop control,
> where such devices do not support hotplug, therefore this scenario lacks
> testing and is not considered sufficiently relevant to support.  Thanks,

Thanks for the input. I integrated that into the commit log:

    vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

    The KVM mechanism for controlling wbinvd is based on OR of the coherency
    property of all devices attached to a guest, no matter whether those
    devices are attached to a single domain or multiple domains.

    On the other hand, the benefit to using separate domains was that those
    devices attached to domains supporting enforced cache coherency always
    mapped with the attributes necessary to provide that feature, therefore
    if a non-enforced domain was dropped, the associated group removal would
    re-trigger an evaluation by KVM.

    In practice however, the only known cases of such mixed domains included
    an Intel IGD device behind an IOMMU lacking snoop control, where such
    devices do not support hotplug, therefore this scenario lacks testing and
    is not considered sufficiently relevant to support.

    After all, KVM won't take advantage of trying to push a device that could
    do enforced cache coherency to a dedicated domain vs re-using an existing
    domain, which is non-coherent.

    Simplify this code and eliminate the test. This removes the only logic
    that needed to have a dummy domain attached prior to searching for a
    matching domain and simplifies the next patches.

    It's unclear whether we want to further optimize the Intel driver to
    update the domain coherency after a device is detached from it, at
    least not before KVM can be verified to handle such dynamics in related
    emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
    we don't see an usage requiring such optimization as the only device
    which imposes such non-coherency is Intel GPU which even doesn't
    support hotplug/hot remove.

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen]

On Tue, Jun 21, 2022 at 04:46:02PM -0600, Alex Williamson wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 15 Jun 2022 17:03:01 -0700
> Nicolin Chen <nicolinc@nvidia.com> wrote:
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> >
> > The KVM mechanism for controlling wbinvd is based on OR of the coherency
> > property of all devices attached to a guest, no matter those devices are
> > attached to a single domain or multiple domains.
> >
> > So, there is no value in trying to push a device that could do enforced
> > cache coherency to a dedicated domain vs re-using an existing domain
> > which is non-coherent since KVM won't be able to take advantage of it.
> > This just wastes domain memory.
> >
> > Simplify this code and eliminate the test. This removes the only logic
> > that needed to have a dummy domain attached prior to searching for a
> > matching domain and simplifies the next patches.
> >
> > It's unclear whether we want to further optimize the Intel driver to
> > update the domain coherency after a device is detached from it, at
> > least not before KVM can be verified to handle such dynamics in related
> > emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> > we don't see an usage requiring such optimization as the only device
> > which imposes such non-coherency is Intel GPU which even doesn't
> > support hotplug/hot remove.
> 
> The 2nd paragraph above is quite misleading in this respect.  I think
> it would be more accurate to explain that the benefit to using separate
> domains was that devices attached to domains supporting enforced cache
> coherency always mapped with the attributes necessary to provide that
> feature, therefore if a non-enforced domain was dropped, the associated
> group removal would re-trigger an evaluation by KVM.  We can then go on
> to discuss that in practice the only known cases of such mixed domains
> included an Intel IGD device behind an IOMMU lacking snoop control,
> where such devices do not support hotplug, therefore this scenario lacks
> testing and is not considered sufficiently relevant to support.  Thanks,

Thanks for the input. I integrated that into the commit log:

    vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

    The KVM mechanism for controlling wbinvd is based on OR of the coherency
    property of all devices attached to a guest, no matter whether those
    devices are attached to a single domain or multiple domains.

    On the other hand, the benefit to using separate domains was that those
    devices attached to domains supporting enforced cache coherency always
    mapped with the attributes necessary to provide that feature, therefore
    if a non-enforced domain was dropped, the associated group removal would
    re-trigger an evaluation by KVM.

    In practice however, the only known cases of such mixed domains included
    an Intel IGD device behind an IOMMU lacking snoop control, where such
    devices do not support hotplug, therefore this scenario lacks testing and
    is not considered sufficiently relevant to support.

    After all, KVM won't take advantage of trying to push a device that could
    do enforced cache coherency to a dedicated domain vs re-using an existing
    domain, which is non-coherent.

    Simplify this code and eliminate the test. This removes the only logic
    that needed to have a dummy domain attached prior to searching for a
    matching domain and simplifies the next patches.

    It's unclear whether we want to further optimize the Intel driver to
    update the domain coherency after a device is detached from it, at
    least not before KVM can be verified to handle such dynamics in related
    emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
    we don't see an usage requiring such optimization as the only device
    which imposes such non-coherency is Intel GPU which even doesn't
    support hotplug/hot remove.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

[Nicolin Chen via iommu]

On Tue, Jun 21, 2022 at 04:46:02PM -0600, Alex Williamson wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 15 Jun 2022 17:03:01 -0700
> Nicolin Chen <nicolinc@nvidia.com> wrote:
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> >
> > The KVM mechanism for controlling wbinvd is based on OR of the coherency
> > property of all devices attached to a guest, no matter those devices are
> > attached to a single domain or multiple domains.
> >
> > So, there is no value in trying to push a device that could do enforced
> > cache coherency to a dedicated domain vs re-using an existing domain
> > which is non-coherent since KVM won't be able to take advantage of it.
> > This just wastes domain memory.
> >
> > Simplify this code and eliminate the test. This removes the only logic
> > that needed to have a dummy domain attached prior to searching for a
> > matching domain and simplifies the next patches.
> >
> > It's unclear whether we want to further optimize the Intel driver to
> > update the domain coherency after a device is detached from it, at
> > least not before KVM can be verified to handle such dynamics in related
> > emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
> > we don't see an usage requiring such optimization as the only device
> > which imposes such non-coherency is Intel GPU which even doesn't
> > support hotplug/hot remove.
> 
> The 2nd paragraph above is quite misleading in this respect.  I think
> it would be more accurate to explain that the benefit to using separate
> domains was that devices attached to domains supporting enforced cache
> coherency always mapped with the attributes necessary to provide that
> feature, therefore if a non-enforced domain was dropped, the associated
> group removal would re-trigger an evaluation by KVM.  We can then go on
> to discuss that in practice the only known cases of such mixed domains
> included an Intel IGD device behind an IOMMU lacking snoop control,
> where such devices do not support hotplug, therefore this scenario lacks
> testing and is not considered sufficiently relevant to support.  Thanks,

Thanks for the input. I integrated that into the commit log:

    vfio/iommu_type1: Prefer to reuse domains vs match enforced cache coherency

    The KVM mechanism for controlling wbinvd is based on OR of the coherency
    property of all devices attached to a guest, no matter whether those
    devices are attached to a single domain or multiple domains.

    On the other hand, the benefit to using separate domains was that those
    devices attached to domains supporting enforced cache coherency always
    mapped with the attributes necessary to provide that feature, therefore
    if a non-enforced domain was dropped, the associated group removal would
    re-trigger an evaluation by KVM.

    In practice however, the only known cases of such mixed domains included
    an Intel IGD device behind an IOMMU lacking snoop control, where such
    devices do not support hotplug, therefore this scenario lacks testing and
    is not considered sufficiently relevant to support.

    After all, KVM won't take advantage of trying to push a device that could
    do enforced cache coherency to a dedicated domain vs re-using an existing
    domain, which is non-coherent.

    Simplify this code and eliminate the test. This removes the only logic
    that needed to have a dummy domain attached prior to searching for a
    matching domain and simplifies the next patches.

    It's unclear whether we want to further optimize the Intel driver to
    update the domain coherency after a device is detached from it, at
    least not before KVM can be verified to handle such dynamics in related
    emulation paths (wbinvd, vcpu load, write_cr0, ept, etc.). In reality
    we don't see an usage requiring such optimization as the only device
    which imposes such non-coherency is Intel GPU which even doesn't
    support hotplug/hot remove.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.

Per discussion with Robin, in future when many can be permitted we will
rely on the IOMMU core code to check the domain->ops:
https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-967750ddff23@arm.com/

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f4e3b423a453..11be5f95580b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2277,29 +2277,19 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			domain->domain->ops->enforce_cache_coherency(
 				domain->domain);
 
-	/*
-	 * Try to match an existing compatible domain.  We don't want to
-	 * preclude an IOMMU driver supporting multiple bus_types and being
-	 * able to include different bus_types in the same IOMMU domain, so
-	 * we test whether the domains use the same iommu_ops rather than
-	 * testing if they're on the same bus_type.
-	 */
+	/* Try to match an existing compatible domain */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops) {
-			iommu_detach_group(domain->domain, group->iommu_group);
-			if (!iommu_attach_group(d->domain,
-						group->iommu_group)) {
-				list_add(&group->next, &d->group_list);
-				iommu_domain_free(domain->domain);
-				kfree(domain);
-				goto done;
-			}
-
-			ret = iommu_attach_group(domain->domain,
-						 group->iommu_group);
-			if (ret)
-				goto out_domain;
+		iommu_detach_group(domain->domain, group->iommu_group);
+		if (!iommu_attach_group(d->domain, group->iommu_group)) {
+			list_add(&group->next, &d->group_list);
+			iommu_domain_free(domain->domain);
+			kfree(domain);
+			goto done;
 		}
+
+		ret = iommu_attach_group(domain->domain,  group->iommu_group);
+		if (ret)
+			goto out_domain;
 	}
 
 	vfio_test_domain_fgsp(domain);
-- 
2.17.1

[PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.

Per discussion with Robin, in future when many can be permitted we will
rely on the IOMMU core code to check the domain->ops:
https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-967750ddff23@arm.com/

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f4e3b423a453..11be5f95580b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2277,29 +2277,19 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			domain->domain->ops->enforce_cache_coherency(
 				domain->domain);
 
-	/*
-	 * Try to match an existing compatible domain.  We don't want to
-	 * preclude an IOMMU driver supporting multiple bus_types and being
-	 * able to include different bus_types in the same IOMMU domain, so
-	 * we test whether the domains use the same iommu_ops rather than
-	 * testing if they're on the same bus_type.
-	 */
+	/* Try to match an existing compatible domain */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops) {
-			iommu_detach_group(domain->domain, group->iommu_group);
-			if (!iommu_attach_group(d->domain,
-						group->iommu_group)) {
-				list_add(&group->next, &d->group_list);
-				iommu_domain_free(domain->domain);
-				kfree(domain);
-				goto done;
-			}
-
-			ret = iommu_attach_group(domain->domain,
-						 group->iommu_group);
-			if (ret)
-				goto out_domain;
+		iommu_detach_group(domain->domain, group->iommu_group);
+		if (!iommu_attach_group(d->domain, group->iommu_group)) {
+			list_add(&group->next, &d->group_list);
+			iommu_domain_free(domain->domain);
+			kfree(domain);
+			goto done;
 		}
+
+		ret = iommu_attach_group(domain->domain,  group->iommu_group);
+		if (ret)
+			goto out_domain;
 	}
 
 	vfio_test_domain_fgsp(domain);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen via iommu]

The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.

Per discussion with Robin, in future when many can be permitted we will
rely on the IOMMU core code to check the domain->ops:
https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-967750ddff23@arm.com/

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f4e3b423a453..11be5f95580b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2277,29 +2277,19 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			domain->domain->ops->enforce_cache_coherency(
 				domain->domain);
 
-	/*
-	 * Try to match an existing compatible domain.  We don't want to
-	 * preclude an IOMMU driver supporting multiple bus_types and being
-	 * able to include different bus_types in the same IOMMU domain, so
-	 * we test whether the domains use the same iommu_ops rather than
-	 * testing if they're on the same bus_type.
-	 */
+	/* Try to match an existing compatible domain */
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		if (d->domain->ops == domain->domain->ops) {
-			iommu_detach_group(domain->domain, group->iommu_group);
-			if (!iommu_attach_group(d->domain,
-						group->iommu_group)) {
-				list_add(&group->next, &d->group_list);
-				iommu_domain_free(domain->domain);
-				kfree(domain);
-				goto done;
-			}
-
-			ret = iommu_attach_group(domain->domain,
-						 group->iommu_group);
-			if (ret)
-				goto out_domain;
+		iommu_detach_group(domain->domain, group->iommu_group);
+		if (!iommu_attach_group(d->domain, group->iommu_group)) {
+			list_add(&group->next, &d->group_list);
+			iommu_domain_free(domain->domain);
+			kfree(domain);
+			goto done;
 		}
+
+		ret = iommu_attach_group(domain->domain,  group->iommu_group);
+		if (ret)
+			goto out_domain;
 	}
 
 	vfio_test_domain_fgsp(domain);
-- 
2.17.1

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

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> The domain->ops validation was added, as a precaution, for mixed-driver
> systems. However, at this moment only one iommu driver is possible. So
> remove it.

It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊

> 
> Per discussion with Robin, in future when many can be permitted we will
> rely on the IOMMU core code to check the domain->ops:
> https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-
> 967750ddff23@arm.com/
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Apart from that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> The domain->ops validation was added, as a precaution, for mixed-driver
> systems. However, at this moment only one iommu driver is possible. So
> remove it.

It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊

> 
> Per discussion with Robin, in future when many can be permitted we will
> rely on the IOMMU core code to check the domain->ops:
> https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-
> 967750ddff23@arm.com/
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Apart from that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> The domain->ops validation was added, as a precaution, for mixed-driver
> systems. However, at this moment only one iommu driver is possible. So
> remove it.

It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊

> 
> Per discussion with Robin, in future when many can be permitted we will
> rely on the IOMMU core code to check the domain->ops:
> https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-
> 967750ddff23@arm.com/
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Apart from that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> The domain->ops validation was added, as a precaution, for mixed-driver
> systems. However, at this moment only one iommu driver is possible. So
> remove it.

It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊

> 
> Per discussion with Robin, in future when many can be permitted we will
> rely on the IOMMU core code to check the domain->ops:
> https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-
> 967750ddff23@arm.com/
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Apart from that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:

> > The domain->ops validation was added, as a precaution, for mixed-driver
> > systems. However, at this moment only one iommu driver is possible. So
> > remove it.
> 
> It's true on a physical platform. But I'm not sure whether a virtual platform
> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> there is plenty more significant problems than this to solve instead of simply
> saying that only one iommu driver is possible if we don't have explicit code
> to reject such configuration. 😊

Will edit this part. Thanks!

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:

> > The domain->ops validation was added, as a precaution, for mixed-driver
> > systems. However, at this moment only one iommu driver is possible. So
> > remove it.
> 
> It's true on a physical platform. But I'm not sure whether a virtual platform
> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> there is plenty more significant problems than this to solve instead of simply
> saying that only one iommu driver is possible if we don't have explicit code
> to reject such configuration. 😊

Will edit this part. Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen via iommu]

On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:

> > The domain->ops validation was added, as a precaution, for mixed-driver
> > systems. However, at this moment only one iommu driver is possible. So
> > remove it.
> 
> It's true on a physical platform. But I'm not sure whether a virtual platform
> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> there is plenty more significant problems than this to solve instead of simply
> saying that only one iommu driver is possible if we don't have explicit code
> to reject such configuration. 😊

Will edit this part. Thanks!
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-16 23:23, Nicolin Chen wrote:
> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> 
>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>> systems. However, at this moment only one iommu driver is possible. So
>>> remove it.
>>
>> It's true on a physical platform. But I'm not sure whether a virtual platform
>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>> there is plenty more significant problems than this to solve instead of simply
>> saying that only one iommu driver is possible if we don't have explicit code
>> to reject such configuration. 😊
> 
> Will edit this part. Thanks!

Oh, physical platforms with mixed IOMMUs definitely exist already. The 
main point is that while bus_set_iommu still exists, the core code 
effectively *does* prevent multiple drivers from registering - even in 
emulated cases like the example above, virtio-iommu and VT-d would both 
try to bus_set_iommu(&pci_bus_type), and one of them will lose. The 
aspect which might warrant clarification is that there's no combination 
of supported drivers which claim non-overlapping buses *and* could 
appear in the same system - even if you tried to contrive something by 
emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you 
could still only describe one or the other due to ACPI vs. Devicetree.

Thanks,
Robin.

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-16 23:23, Nicolin Chen wrote:
> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> 
>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>> systems. However, at this moment only one iommu driver is possible. So
>>> remove it.
>>
>> It's true on a physical platform. But I'm not sure whether a virtual platform
>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>> there is plenty more significant problems than this to solve instead of simply
>> saying that only one iommu driver is possible if we don't have explicit code
>> to reject such configuration. 😊
> 
> Will edit this part. Thanks!

Oh, physical platforms with mixed IOMMUs definitely exist already. The 
main point is that while bus_set_iommu still exists, the core code 
effectively *does* prevent multiple drivers from registering - even in 
emulated cases like the example above, virtio-iommu and VT-d would both 
try to bus_set_iommu(&pci_bus_type), and one of them will lose. The 
aspect which might warrant clarification is that there's no combination 
of supported drivers which claim non-overlapping buses *and* could 
appear in the same system - even if you tried to contrive something by 
emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you 
could still only describe one or the other due to ACPI vs. Devicetree.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-16 23:23, Nicolin Chen wrote:
> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> 
>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>> systems. However, at this moment only one iommu driver is possible. So
>>> remove it.
>>
>> It's true on a physical platform. But I'm not sure whether a virtual platform
>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>> there is plenty more significant problems than this to solve instead of simply
>> saying that only one iommu driver is possible if we don't have explicit code
>> to reject such configuration. 😊
> 
> Will edit this part. Thanks!

Oh, physical platforms with mixed IOMMUs definitely exist already. The 
main point is that while bus_set_iommu still exists, the core code 
effectively *does* prevent multiple drivers from registering - even in 
emulated cases like the example above, virtio-iommu and VT-d would both 
try to bus_set_iommu(&pci_bus_type), and one of them will lose. The 
aspect which might warrant clarification is that there's no combination 
of supported drivers which claim non-overlapping buses *and* could 
appear in the same system - even if you tried to contrive something by 
emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you 
could still only describe one or the other due to ACPI vs. Devicetree.

Thanks,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-16 23:23, Nicolin Chen wrote:
> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> 
>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>> systems. However, at this moment only one iommu driver is possible. So
>>> remove it.
>>
>> It's true on a physical platform. But I'm not sure whether a virtual platform
>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>> there is plenty more significant problems than this to solve instead of simply
>> saying that only one iommu driver is possible if we don't have explicit code
>> to reject such configuration. 😊
> 
> Will edit this part. Thanks!

Oh, physical platforms with mixed IOMMUs definitely exist already. The 
main point is that while bus_set_iommu still exists, the core code 
effectively *does* prevent multiple drivers from registering - even in 
emulated cases like the example above, virtio-iommu and VT-d would both 
try to bus_set_iommu(&pci_bus_type), and one of them will lose. The 
aspect which might warrant clarification is that there's no combination 
of supported drivers which claim non-overlapping buses *and* could 
appear in the same system - even if you tried to contrive something by 
emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you 
could still only describe one or the other due to ACPI vs. Devicetree.

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

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, June 22, 2022 3:55 PM
> 
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> >
> >>> The domain->ops validation was added, as a precaution, for mixed-
> driver
> >>> systems. However, at this moment only one iommu driver is possible. So
> >>> remove it.
> >>
> >> It's true on a physical platform. But I'm not sure whether a virtual
> platform
> >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> d
> >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> >> there is plenty more significant problems than this to solve instead of
> simply
> >> saying that only one iommu driver is possible if we don't have explicit
> code
> >> to reject such configuration. 😊
> >
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The
> main point is that while bus_set_iommu still exists, the core code
> effectively *does* prevent multiple drivers from registering - even in
> emulated cases like the example above, virtio-iommu and VT-d would both
> try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> aspect which might warrant clarification is that there's no combination
> of supported drivers which claim non-overlapping buses *and* could
> appear in the same system - even if you tried to contrive something by
> emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> could still only describe one or the other due to ACPI vs. Devicetree.
> 

This explanation is much clearer! thanks.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, June 22, 2022 3:55 PM
> 
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> >
> >>> The domain->ops validation was added, as a precaution, for mixed-
> driver
> >>> systems. However, at this moment only one iommu driver is possible. So
> >>> remove it.
> >>
> >> It's true on a physical platform. But I'm not sure whether a virtual
> platform
> >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> d
> >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> >> there is plenty more significant problems than this to solve instead of
> simply
> >> saying that only one iommu driver is possible if we don't have explicit
> code
> >> to reject such configuration. 😊
> >
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The
> main point is that while bus_set_iommu still exists, the core code
> effectively *does* prevent multiple drivers from registering - even in
> emulated cases like the example above, virtio-iommu and VT-d would both
> try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> aspect which might warrant clarification is that there's no combination
> of supported drivers which claim non-overlapping buses *and* could
> appear in the same system - even if you tried to contrive something by
> emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> could still only describe one or the other due to ACPI vs. Devicetree.
> 

This explanation is much clearer! thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, June 22, 2022 3:55 PM
> 
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> >
> >>> The domain->ops validation was added, as a precaution, for mixed-
> driver
> >>> systems. However, at this moment only one iommu driver is possible. So
> >>> remove it.
> >>
> >> It's true on a physical platform. But I'm not sure whether a virtual
> platform
> >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> d
> >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> >> there is plenty more significant problems than this to solve instead of
> simply
> >> saying that only one iommu driver is possible if we don't have explicit
> code
> >> to reject such configuration. 😊
> >
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The
> main point is that while bus_set_iommu still exists, the core code
> effectively *does* prevent multiple drivers from registering - even in
> emulated cases like the example above, virtio-iommu and VT-d would both
> try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> aspect which might warrant clarification is that there's no combination
> of supported drivers which claim non-overlapping buses *and* could
> appear in the same system - even if you tried to contrive something by
> emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> could still only describe one or the other due to ACPI vs. Devicetree.
> 

This explanation is much clearer! thanks.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, June 22, 2022 3:55 PM
> 
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> >
> >>> The domain->ops validation was added, as a precaution, for mixed-
> driver
> >>> systems. However, at this moment only one iommu driver is possible. So
> >>> remove it.
> >>
> >> It's true on a physical platform. But I'm not sure whether a virtual
> platform
> >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> d
> >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> >> there is plenty more significant problems than this to solve instead of
> simply
> >> saying that only one iommu driver is possible if we don't have explicit
> code
> >> to reject such configuration. 😊
> >
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The
> main point is that while bus_set_iommu still exists, the core code
> effectively *does* prevent multiple drivers from registering - even in
> emulated cases like the example above, virtio-iommu and VT-d would both
> try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> aspect which might warrant clarification is that there's no combination
> of supported drivers which claim non-overlapping buses *and* could
> appear in the same system - even if you tried to contrive something by
> emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> could still only describe one or the other due to ACPI vs. Devicetree.
> 

This explanation is much clearer! thanks.

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

On Thu, Jun 23, 2022 at 03:50:22AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Robin Murphy <robin.murphy@arm.com>
> > Sent: Wednesday, June 22, 2022 3:55 PM
> >
> > On 2022-06-16 23:23, Nicolin Chen wrote:
> > > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > >
> > >>> The domain->ops validation was added, as a precaution, for mixed-
> > driver
> > >>> systems. However, at this moment only one iommu driver is possible. So
> > >>> remove it.
> > >>
> > >> It's true on a physical platform. But I'm not sure whether a virtual
> > platform
> > >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> > d
> > >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > >> there is plenty more significant problems than this to solve instead of
> > simply
> > >> saying that only one iommu driver is possible if we don't have explicit
> > code
> > >> to reject such configuration. 😊
> > >
> > > Will edit this part. Thanks!
> >
> > Oh, physical platforms with mixed IOMMUs definitely exist already. The
> > main point is that while bus_set_iommu still exists, the core code
> > effectively *does* prevent multiple drivers from registering - even in
> > emulated cases like the example above, virtio-iommu and VT-d would both
> > try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> > aspect which might warrant clarification is that there's no combination
> > of supported drivers which claim non-overlapping buses *and* could
> > appear in the same system - even if you tried to contrive something by
> > emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> > could still only describe one or the other due to ACPI vs. Devicetree.
> >
> 
> This explanation is much clearer! thanks.

Thanks +1

I've also updated the commit log.

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen]

On Thu, Jun 23, 2022 at 03:50:22AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Robin Murphy <robin.murphy@arm.com>
> > Sent: Wednesday, June 22, 2022 3:55 PM
> >
> > On 2022-06-16 23:23, Nicolin Chen wrote:
> > > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > >
> > >>> The domain->ops validation was added, as a precaution, for mixed-
> > driver
> > >>> systems. However, at this moment only one iommu driver is possible. So
> > >>> remove it.
> > >>
> > >> It's true on a physical platform. But I'm not sure whether a virtual
> > platform
> > >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> > d
> > >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > >> there is plenty more significant problems than this to solve instead of
> > simply
> > >> saying that only one iommu driver is possible if we don't have explicit
> > code
> > >> to reject such configuration. 😊
> > >
> > > Will edit this part. Thanks!
> >
> > Oh, physical platforms with mixed IOMMUs definitely exist already. The
> > main point is that while bus_set_iommu still exists, the core code
> > effectively *does* prevent multiple drivers from registering - even in
> > emulated cases like the example above, virtio-iommu and VT-d would both
> > try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> > aspect which might warrant clarification is that there's no combination
> > of supported drivers which claim non-overlapping buses *and* could
> > appear in the same system - even if you tried to contrive something by
> > emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> > could still only describe one or the other due to ACPI vs. Devicetree.
> >
> 
> This explanation is much clearer! thanks.

Thanks +1

I've also updated the commit log.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Nicolin Chen via iommu]

On Thu, Jun 23, 2022 at 03:50:22AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Robin Murphy <robin.murphy@arm.com>
> > Sent: Wednesday, June 22, 2022 3:55 PM
> >
> > On 2022-06-16 23:23, Nicolin Chen wrote:
> > > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > >
> > >>> The domain->ops validation was added, as a precaution, for mixed-
> > driver
> > >>> systems. However, at this moment only one iommu driver is possible. So
> > >>> remove it.
> > >>
> > >> It's true on a physical platform. But I'm not sure whether a virtual
> > platform
> > >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-
> > d
> > >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > >> there is plenty more significant problems than this to solve instead of
> > simply
> > >> saying that only one iommu driver is possible if we don't have explicit
> > code
> > >> to reject such configuration. 😊
> > >
> > > Will edit this part. Thanks!
> >
> > Oh, physical platforms with mixed IOMMUs definitely exist already. The
> > main point is that while bus_set_iommu still exists, the core code
> > effectively *does* prevent multiple drivers from registering - even in
> > emulated cases like the example above, virtio-iommu and VT-d would both
> > try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> > aspect which might warrant clarification is that there's no combination
> > of supported drivers which claim non-overlapping buses *and* could
> > appear in the same system - even if you tried to contrive something by
> > emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> > could still only describe one or the other due to ACPI vs. Devicetree.
> >
> 
> This explanation is much clearer! thanks.

Thanks +1

I've also updated the commit log.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe]

On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > 
> > > > The domain->ops validation was added, as a precaution, for mixed-driver
> > > > systems. However, at this moment only one iommu driver is possible. So
> > > > remove it.
> > > 
> > > It's true on a physical platform. But I'm not sure whether a virtual platform
> > > is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> > > or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > > there is plenty more significant problems than this to solve instead of simply
> > > saying that only one iommu driver is possible if we don't have explicit code
> > > to reject such configuration. 😊
> > 
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> point is that while bus_set_iommu still exists, the core code effectively
> *does* prevent multiple drivers from registering - even in emulated cases
> like the example above, virtio-iommu and VT-d would both try to
> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> might warrant clarification is that there's no combination of supported
> drivers which claim non-overlapping buses *and* could appear in the same
> system - even if you tried to contrive something by emulating, say, VT-d
> (PCI) alongside rockchip-iommu (platform), you could still only describe one
> or the other due to ACPI vs. Devicetree.

Right, and that is still something we need to protect against with
this ops check. VFIO is not checking that the bus's are the same
before attempting to re-use a domain.

So it is actually functional and does protect against systems with
multiple iommu drivers on different busses.

Jason

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe]

On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > 
> > > > The domain->ops validation was added, as a precaution, for mixed-driver
> > > > systems. However, at this moment only one iommu driver is possible. So
> > > > remove it.
> > > 
> > > It's true on a physical platform. But I'm not sure whether a virtual platform
> > > is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> > > or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > > there is plenty more significant problems than this to solve instead of simply
> > > saying that only one iommu driver is possible if we don't have explicit code
> > > to reject such configuration. 😊
> > 
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> point is that while bus_set_iommu still exists, the core code effectively
> *does* prevent multiple drivers from registering - even in emulated cases
> like the example above, virtio-iommu and VT-d would both try to
> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> might warrant clarification is that there's no combination of supported
> drivers which claim non-overlapping buses *and* could appear in the same
> system - even if you tried to contrive something by emulating, say, VT-d
> (PCI) alongside rockchip-iommu (platform), you could still only describe one
> or the other due to ACPI vs. Devicetree.

Right, and that is still something we need to protect against with
this ops check. VFIO is not checking that the bus's are the same
before attempting to re-use a domain.

So it is actually functional and does protect against systems with
multiple iommu drivers on different busses.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe via iommu]

On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
> > 
> > > > The domain->ops validation was added, as a precaution, for mixed-driver
> > > > systems. However, at this moment only one iommu driver is possible. So
> > > > remove it.
> > > 
> > > It's true on a physical platform. But I'm not sure whether a virtual platform
> > > is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> > > or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> > > there is plenty more significant problems than this to solve instead of simply
> > > saying that only one iommu driver is possible if we don't have explicit code
> > > to reject such configuration. 😊
> > 
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> point is that while bus_set_iommu still exists, the core code effectively
> *does* prevent multiple drivers from registering - even in emulated cases
> like the example above, virtio-iommu and VT-d would both try to
> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> might warrant clarification is that there's no combination of supported
> drivers which claim non-overlapping buses *and* could appear in the same
> system - even if you tried to contrive something by emulating, say, VT-d
> (PCI) alongside rockchip-iommu (platform), you could still only describe one
> or the other due to ACPI vs. Devicetree.

Right, and that is still something we need to protect against with
this ops check. VFIO is not checking that the bus's are the same
before attempting to re-use a domain.

So it is actually functional and does protect against systems with
multiple iommu drivers on different busses.

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

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-24 14:16, Jason Gunthorpe wrote:
> On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
>> On 2022-06-16 23:23, Nicolin Chen wrote:
>>> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
>>>
>>>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>>>> systems. However, at this moment only one iommu driver is possible. So
>>>>> remove it.
>>>>
>>>> It's true on a physical platform. But I'm not sure whether a virtual platform
>>>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>>>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>>>> there is plenty more significant problems than this to solve instead of simply
>>>> saying that only one iommu driver is possible if we don't have explicit code
>>>> to reject such configuration. 😊
>>>
>>> Will edit this part. Thanks!
>>
>> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
>> point is that while bus_set_iommu still exists, the core code effectively
>> *does* prevent multiple drivers from registering - even in emulated cases
>> like the example above, virtio-iommu and VT-d would both try to
>> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
>> might warrant clarification is that there's no combination of supported
>> drivers which claim non-overlapping buses *and* could appear in the same
>> system - even if you tried to contrive something by emulating, say, VT-d
>> (PCI) alongside rockchip-iommu (platform), you could still only describe one
>> or the other due to ACPI vs. Devicetree.
> 
> Right, and that is still something we need to protect against with
> this ops check. VFIO is not checking that the bus's are the same
> before attempting to re-use a domain.
> 
> So it is actually functional and does protect against systems with
> multiple iommu drivers on different busses.

But as above, which systems *are* those? Everything that's on my radar 
would have drivers all competing for the platform bus - Intel and s390 
are somewhat the odd ones out in that respect, but are also non-issues 
as above. FWIW my iommu/bus dev branch has got as far as the final bus 
ops removal and allowing multiple driver registrations, and before it 
allows that, it does now have the common attach check that I sketched 
out in the previous discussion of this.

It's probably also noteworthy that domain->ops is no longer the same 
domain->ops that this code was written to check, and may now be 
different between domains from the same driver.

Thanks,
Robin.

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-24 14:16, Jason Gunthorpe wrote:
> On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
>> On 2022-06-16 23:23, Nicolin Chen wrote:
>>> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
>>>
>>>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>>>> systems. However, at this moment only one iommu driver is possible. So
>>>>> remove it.
>>>>
>>>> It's true on a physical platform. But I'm not sure whether a virtual platform
>>>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>>>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>>>> there is plenty more significant problems than this to solve instead of simply
>>>> saying that only one iommu driver is possible if we don't have explicit code
>>>> to reject such configuration. 😊
>>>
>>> Will edit this part. Thanks!
>>
>> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
>> point is that while bus_set_iommu still exists, the core code effectively
>> *does* prevent multiple drivers from registering - even in emulated cases
>> like the example above, virtio-iommu and VT-d would both try to
>> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
>> might warrant clarification is that there's no combination of supported
>> drivers which claim non-overlapping buses *and* could appear in the same
>> system - even if you tried to contrive something by emulating, say, VT-d
>> (PCI) alongside rockchip-iommu (platform), you could still only describe one
>> or the other due to ACPI vs. Devicetree.
> 
> Right, and that is still something we need to protect against with
> this ops check. VFIO is not checking that the bus's are the same
> before attempting to re-use a domain.
> 
> So it is actually functional and does protect against systems with
> multiple iommu drivers on different busses.

But as above, which systems *are* those? Everything that's on my radar 
would have drivers all competing for the platform bus - Intel and s390 
are somewhat the odd ones out in that respect, but are also non-issues 
as above. FWIW my iommu/bus dev branch has got as far as the final bus 
ops removal and allowing multiple driver registrations, and before it 
allows that, it does now have the common attach check that I sketched 
out in the previous discussion of this.

It's probably also noteworthy that domain->ops is no longer the same 
domain->ops that this code was written to check, and may now be 
different between domains from the same driver.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-24 14:16, Jason Gunthorpe wrote:
> On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
>> On 2022-06-16 23:23, Nicolin Chen wrote:
>>> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
>>>
>>>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>>>> systems. However, at this moment only one iommu driver is possible. So
>>>>> remove it.
>>>>
>>>> It's true on a physical platform. But I'm not sure whether a virtual platform
>>>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>>>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>>>> there is plenty more significant problems than this to solve instead of simply
>>>> saying that only one iommu driver is possible if we don't have explicit code
>>>> to reject such configuration. 😊
>>>
>>> Will edit this part. Thanks!
>>
>> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
>> point is that while bus_set_iommu still exists, the core code effectively
>> *does* prevent multiple drivers from registering - even in emulated cases
>> like the example above, virtio-iommu and VT-d would both try to
>> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
>> might warrant clarification is that there's no combination of supported
>> drivers which claim non-overlapping buses *and* could appear in the same
>> system - even if you tried to contrive something by emulating, say, VT-d
>> (PCI) alongside rockchip-iommu (platform), you could still only describe one
>> or the other due to ACPI vs. Devicetree.
> 
> Right, and that is still something we need to protect against with
> this ops check. VFIO is not checking that the bus's are the same
> before attempting to re-use a domain.
> 
> So it is actually functional and does protect against systems with
> multiple iommu drivers on different busses.

But as above, which systems *are* those? Everything that's on my radar 
would have drivers all competing for the platform bus - Intel and s390 
are somewhat the odd ones out in that respect, but are also non-issues 
as above. FWIW my iommu/bus dev branch has got as far as the final bus 
ops removal and allowing multiple driver registrations, and before it 
allows that, it does now have the common attach check that I sketched 
out in the previous discussion of this.

It's probably also noteworthy that domain->ops is no longer the same 
domain->ops that this code was written to check, and may now be 
different between domains from the same driver.

Thanks,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Robin Murphy]

On 2022-06-24 14:16, Jason Gunthorpe wrote:
> On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
>> On 2022-06-16 23:23, Nicolin Chen wrote:
>>> On Thu, Jun 16, 2022 at 06:40:14AM +0000, Tian, Kevin wrote:
>>>
>>>>> The domain->ops validation was added, as a precaution, for mixed-driver
>>>>> systems. However, at this moment only one iommu driver is possible. So
>>>>> remove it.
>>>>
>>>> It's true on a physical platform. But I'm not sure whether a virtual platform
>>>> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
>>>> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
>>>> there is plenty more significant problems than this to solve instead of simply
>>>> saying that only one iommu driver is possible if we don't have explicit code
>>>> to reject such configuration. 😊
>>>
>>> Will edit this part. Thanks!
>>
>> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
>> point is that while bus_set_iommu still exists, the core code effectively
>> *does* prevent multiple drivers from registering - even in emulated cases
>> like the example above, virtio-iommu and VT-d would both try to
>> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
>> might warrant clarification is that there's no combination of supported
>> drivers which claim non-overlapping buses *and* could appear in the same
>> system - even if you tried to contrive something by emulating, say, VT-d
>> (PCI) alongside rockchip-iommu (platform), you could still only describe one
>> or the other due to ACPI vs. Devicetree.
> 
> Right, and that is still something we need to protect against with
> this ops check. VFIO is not checking that the bus's are the same
> before attempting to re-use a domain.
> 
> So it is actually functional and does protect against systems with
> multiple iommu drivers on different busses.

But as above, which systems *are* those? Everything that's on my radar 
would have drivers all competing for the platform bus - Intel and s390 
are somewhat the odd ones out in that respect, but are also non-issues 
as above. FWIW my iommu/bus dev branch has got as far as the final bus 
ops removal and allowing multiple driver registrations, and before it 
allows that, it does now have the common attach check that I sketched 
out in the previous discussion of this.

It's probably also noteworthy that domain->ops is no longer the same 
domain->ops that this code was written to check, and may now be 
different between domains from the same driver.

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

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe]

On Fri, Jun 24, 2022 at 07:31:47PM +0100, Robin Murphy wrote:

> > > Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> > > point is that while bus_set_iommu still exists, the core code effectively
> > > *does* prevent multiple drivers from registering - even in emulated cases
> > > like the example above, virtio-iommu and VT-d would both try to
> > > bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> > > might warrant clarification is that there's no combination of supported
> > > drivers which claim non-overlapping buses *and* could appear in the same
> > > system - even if you tried to contrive something by emulating, say, VT-d
> > > (PCI) alongside rockchip-iommu (platform), you could still only describe one
> > > or the other due to ACPI vs. Devicetree.
> > 
> > Right, and that is still something we need to protect against with
> > this ops check. VFIO is not checking that the bus's are the same
> > before attempting to re-use a domain.
> > 
> > So it is actually functional and does protect against systems with
> > multiple iommu drivers on different busses.
> 
> But as above, which systems *are* those? 

IDK it seems wrong that the system today will allow different buses to
have different IOMMU drivers and not provide a trivial protection
check.

> FWIW my iommu/bus dev branch has got as far as the final bus ops removal and
> allowing multiple driver registrations, and before it allows that, it does
> now have the common attach check that I sketched out in the previous
> discussion of this.

If you want to put the check in your series that seems fine too, as
long as we get it in the end.

> It's probably also noteworthy that domain->ops is no longer the same
> domain->ops that this code was written to check, and may now be different
> between domains from the same driver.

Yes, the vfio check is not good anymore.

Jason

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe]

On Fri, Jun 24, 2022 at 07:31:47PM +0100, Robin Murphy wrote:

> > > Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> > > point is that while bus_set_iommu still exists, the core code effectively
> > > *does* prevent multiple drivers from registering - even in emulated cases
> > > like the example above, virtio-iommu and VT-d would both try to
> > > bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> > > might warrant clarification is that there's no combination of supported
> > > drivers which claim non-overlapping buses *and* could appear in the same
> > > system - even if you tried to contrive something by emulating, say, VT-d
> > > (PCI) alongside rockchip-iommu (platform), you could still only describe one
> > > or the other due to ACPI vs. Devicetree.
> > 
> > Right, and that is still something we need to protect against with
> > this ops check. VFIO is not checking that the bus's are the same
> > before attempting to re-use a domain.
> > 
> > So it is actually functional and does protect against systems with
> > multiple iommu drivers on different busses.
> 
> But as above, which systems *are* those? 

IDK it seems wrong that the system today will allow different buses to
have different IOMMU drivers and not provide a trivial protection
check.

> FWIW my iommu/bus dev branch has got as far as the final bus ops removal and
> allowing multiple driver registrations, and before it allows that, it does
> now have the common attach check that I sketched out in the previous
> discussion of this.

If you want to put the check in your series that seems fine too, as
long as we get it in the end.

> It's probably also noteworthy that domain->ops is no longer the same
> domain->ops that this code was written to check, and may now be different
> between domains from the same driver.

Yes, the vfio check is not good anymore.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

[Jason Gunthorpe via iommu]

On Fri, Jun 24, 2022 at 07:31:47PM +0100, Robin Murphy wrote:

> > > Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> > > point is that while bus_set_iommu still exists, the core code effectively
> > > *does* prevent multiple drivers from registering - even in emulated cases
> > > like the example above, virtio-iommu and VT-d would both try to
> > > bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> > > might warrant clarification is that there's no combination of supported
> > > drivers which claim non-overlapping buses *and* could appear in the same
> > > system - even if you tried to contrive something by emulating, say, VT-d
> > > (PCI) alongside rockchip-iommu (platform), you could still only describe one
> > > or the other due to ACPI vs. Devicetree.
> > 
> > Right, and that is still something we need to protect against with
> > this ops check. VFIO is not checking that the bus's are the same
> > before attempting to re-use a domain.
> > 
> > So it is actually functional and does protect against systems with
> > multiple iommu drivers on different busses.
> 
> But as above, which systems *are* those? 

IDK it seems wrong that the system today will allow different buses to
have different IOMMU drivers and not provide a trivial protection
check.

> FWIW my iommu/bus dev branch has got as far as the final bus ops removal and
> allowing multiple driver registrations, and before it allows that, it does
> now have the common attach check that I sketched out in the previous
> discussion of this.

If you want to put the check in your series that seems fine too, as
long as we get it in the end.

> It's probably also noteworthy that domain->ops is no longer the same
> domain->ops that this code was written to check, and may now be different
> between domains from the same driver.

Yes, the vfio check is not good anymore.

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

[PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen]

All devices in emulated_iommu_groups have pinned_page_dirty_scope
set, so the update_dirty_scope in the first list_for_each_entry
is always false. Clean it up, and move the "if update_dirty_scope"
part from the detach_group_done routine to the domain_list part.

Rename the "detach_group_done" goto label accordingly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 11be5f95580b..573caf320788 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2453,14 +2453,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain;
 	struct vfio_iommu_group *group;
-	bool update_dirty_scope = false;
 	LIST_HEAD(iova_copy);
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(group, &iommu->emulated_iommu_groups, next) {
 		if (group->iommu_group != iommu_group)
 			continue;
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
 		kfree(group);
 
@@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			WARN_ON(iommu->notifier.head);
 			vfio_iommu_unmap_unpin_all(iommu);
 		}
-		goto detach_group_done;
+		goto out_unlock;
 	}
 
 	/*
@@ -2485,9 +2483,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			continue;
 
 		iommu_detach_group(domain->domain, group->iommu_group);
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
-		kfree(group);
 		/*
 		 * Group ownership provides privilege, if the group list is
 		 * empty, the domain goes away. If it's the last domain with
@@ -2510,6 +2506,16 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			vfio_iommu_aper_expand(iommu, &iova_copy);
 			vfio_update_pgsize_bitmap(iommu);
 		}
+		/*
+		 * Removal of a group without dirty tracking may allow
+		 * the iommu scope to be promoted.
+		 */
+		if (!group->pinned_page_dirty_scope) {
+			iommu->num_non_pinned_groups--;
+			if (iommu->dirty_page_tracking)
+				vfio_iommu_populate_bitmap_full(iommu);
+		}
+		kfree(group);
 		break;
 	}
 
@@ -2518,16 +2524,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	else
 		vfio_iommu_iova_free(&iova_copy);
 
-detach_group_done:
-	/*
-	 * Removal of a group without dirty tracking may allow the iommu scope
-	 * to be promoted.
-	 */
-	if (update_dirty_scope) {
-		iommu->num_non_pinned_groups--;
-		if (iommu->dirty_page_tracking)
-			vfio_iommu_populate_bitmap_full(iommu);
-	}
+out_unlock:
 	mutex_unlock(&iommu->lock);
 }
 
-- 
2.17.1

[PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen]

All devices in emulated_iommu_groups have pinned_page_dirty_scope
set, so the update_dirty_scope in the first list_for_each_entry
is always false. Clean it up, and move the "if update_dirty_scope"
part from the detach_group_done routine to the domain_list part.

Rename the "detach_group_done" goto label accordingly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 11be5f95580b..573caf320788 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2453,14 +2453,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain;
 	struct vfio_iommu_group *group;
-	bool update_dirty_scope = false;
 	LIST_HEAD(iova_copy);
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(group, &iommu->emulated_iommu_groups, next) {
 		if (group->iommu_group != iommu_group)
 			continue;
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
 		kfree(group);
 
@@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			WARN_ON(iommu->notifier.head);
 			vfio_iommu_unmap_unpin_all(iommu);
 		}
-		goto detach_group_done;
+		goto out_unlock;
 	}
 
 	/*
@@ -2485,9 +2483,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			continue;
 
 		iommu_detach_group(domain->domain, group->iommu_group);
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
-		kfree(group);
 		/*
 		 * Group ownership provides privilege, if the group list is
 		 * empty, the domain goes away. If it's the last domain with
@@ -2510,6 +2506,16 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			vfio_iommu_aper_expand(iommu, &iova_copy);
 			vfio_update_pgsize_bitmap(iommu);
 		}
+		/*
+		 * Removal of a group without dirty tracking may allow
+		 * the iommu scope to be promoted.
+		 */
+		if (!group->pinned_page_dirty_scope) {
+			iommu->num_non_pinned_groups--;
+			if (iommu->dirty_page_tracking)
+				vfio_iommu_populate_bitmap_full(iommu);
+		}
+		kfree(group);
 		break;
 	}
 
@@ -2518,16 +2524,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	else
 		vfio_iommu_iova_free(&iova_copy);
 
-detach_group_done:
-	/*
-	 * Removal of a group without dirty tracking may allow the iommu scope
-	 * to be promoted.
-	 */
-	if (update_dirty_scope) {
-		iommu->num_non_pinned_groups--;
-		if (iommu->dirty_page_tracking)
-			vfio_iommu_populate_bitmap_full(iommu);
-	}
+out_unlock:
 	mutex_unlock(&iommu->lock);
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen via iommu]

All devices in emulated_iommu_groups have pinned_page_dirty_scope
set, so the update_dirty_scope in the first list_for_each_entry
is always false. Clean it up, and move the "if update_dirty_scope"
part from the detach_group_done routine to the domain_list part.

Rename the "detach_group_done" goto label accordingly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 11be5f95580b..573caf320788 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2453,14 +2453,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain;
 	struct vfio_iommu_group *group;
-	bool update_dirty_scope = false;
 	LIST_HEAD(iova_copy);
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(group, &iommu->emulated_iommu_groups, next) {
 		if (group->iommu_group != iommu_group)
 			continue;
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
 		kfree(group);
 
@@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			WARN_ON(iommu->notifier.head);
 			vfio_iommu_unmap_unpin_all(iommu);
 		}
-		goto detach_group_done;
+		goto out_unlock;
 	}
 
 	/*
@@ -2485,9 +2483,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			continue;
 
 		iommu_detach_group(domain->domain, group->iommu_group);
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
-		kfree(group);
 		/*
 		 * Group ownership provides privilege, if the group list is
 		 * empty, the domain goes away. If it's the last domain with
@@ -2510,6 +2506,16 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			vfio_iommu_aper_expand(iommu, &iova_copy);
 			vfio_update_pgsize_bitmap(iommu);
 		}
+		/*
+		 * Removal of a group without dirty tracking may allow
+		 * the iommu scope to be promoted.
+		 */
+		if (!group->pinned_page_dirty_scope) {
+			iommu->num_non_pinned_groups--;
+			if (iommu->dirty_page_tracking)
+				vfio_iommu_populate_bitmap_full(iommu);
+		}
+		kfree(group);
 		break;
 	}
 
@@ -2518,16 +2524,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	else
 		vfio_iommu_iova_free(&iova_copy);
 
-detach_group_done:
-	/*
-	 * Removal of a group without dirty tracking may allow the iommu scope
-	 * to be promoted.
-	 */
-	if (update_dirty_scope) {
-		iommu->num_non_pinned_groups--;
-		if (iommu->dirty_page_tracking)
-			vfio_iommu_populate_bitmap_full(iommu);
-	}
+out_unlock:
 	mutex_unlock(&iommu->lock);
 }
 
-- 
2.17.1

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

RE: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> All devices in emulated_iommu_groups have pinned_page_dirty_scope
> set, so the update_dirty_scope in the first list_for_each_entry
> is always false. Clean it up, and move the "if update_dirty_scope"
> part from the detach_group_done routine to the domain_list part.
> 
> Rename the "detach_group_done" goto label accordingly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> @@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			WARN_ON(iommu->notifier.head);
>  			vfio_iommu_unmap_unpin_all(iommu);
>  		}
> -		goto detach_group_done;
> +		goto out_unlock;
...
> +out_unlock:
>  	mutex_unlock(&iommu->lock);
>  }
> 

I'd just replace the goto with a direct unlock and then return there. 
the readability is slightly better.

RE: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> All devices in emulated_iommu_groups have pinned_page_dirty_scope
> set, so the update_dirty_scope in the first list_for_each_entry
> is always false. Clean it up, and move the "if update_dirty_scope"
> part from the detach_group_done routine to the domain_list part.
> 
> Rename the "detach_group_done" goto label accordingly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> @@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			WARN_ON(iommu->notifier.head);
>  			vfio_iommu_unmap_unpin_all(iommu);
>  		}
> -		goto detach_group_done;
> +		goto out_unlock;
...
> +out_unlock:
>  	mutex_unlock(&iommu->lock);
>  }
> 

I'd just replace the goto with a direct unlock and then return there. 
the readability is slightly better.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> All devices in emulated_iommu_groups have pinned_page_dirty_scope
> set, so the update_dirty_scope in the first list_for_each_entry
> is always false. Clean it up, and move the "if update_dirty_scope"
> part from the detach_group_done routine to the domain_list part.
> 
> Rename the "detach_group_done" goto label accordingly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> @@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			WARN_ON(iommu->notifier.head);
>  			vfio_iommu_unmap_unpin_all(iommu);
>  		}
> -		goto detach_group_done;
> +		goto out_unlock;
...
> +out_unlock:
>  	mutex_unlock(&iommu->lock);
>  }
> 

I'd just replace the goto with a direct unlock and then return there. 
the readability is slightly better.

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

RE: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> All devices in emulated_iommu_groups have pinned_page_dirty_scope
> set, so the update_dirty_scope in the first list_for_each_entry
> is always false. Clean it up, and move the "if update_dirty_scope"
> part from the detach_group_done routine to the domain_list part.
> 
> Rename the "detach_group_done" goto label accordingly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> @@ -2469,7 +2467,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			WARN_ON(iommu->notifier.head);
>  			vfio_iommu_unmap_unpin_all(iommu);
>  		}
> -		goto detach_group_done;
> +		goto out_unlock;
...
> +out_unlock:
>  	mutex_unlock(&iommu->lock);
>  }
> 

I'd just replace the goto with a direct unlock and then return there. 
the readability is slightly better.

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

Re: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen]

On Thu, Jun 16, 2022 at 06:45:09AM +0000, Tian, Kevin wrote:

> > +out_unlock:
> >       mutex_unlock(&iommu->lock);
> >  }
> >
> 
> I'd just replace the goto with a direct unlock and then return there.
> the readability is slightly better.

OK. Will do that.

Re: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen]

On Thu, Jun 16, 2022 at 06:45:09AM +0000, Tian, Kevin wrote:

> > +out_unlock:
> >       mutex_unlock(&iommu->lock);
> >  }
> >
> 
> I'd just replace the goto with a direct unlock and then return there.
> the readability is slightly better.

OK. Will do that.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

[Nicolin Chen via iommu]

On Thu, Jun 16, 2022 at 06:45:09AM +0000, Tian, Kevin wrote:

> > +out_unlock:
> >       mutex_unlock(&iommu->lock);
> >  }
> >
> 
> I'd just replace the goto with a direct unlock and then return there.
> the readability is slightly better.

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

[PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

Un-inline the domain specific logic from the attach/detach_group ops into
two paired functions vfio_iommu_alloc_attach_domain() and
vfio_iommu_detach_destroy_domain() that strictly deal with creating and
destroying struct vfio_domains.

Add the logic to check for EMEDIUMTYPE return code of iommu_attach_group()
and avoid the extra domain allocations and attach/detach sequences of the
old code. This allows properly detecting an actual attach error, like
-ENOMEM, vs treating all attach errors as an incompatible domain.

Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
 1 file changed, 163 insertions(+), 135 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 573caf320788..5986c68e59ee 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -86,6 +86,7 @@ struct vfio_domain {
 	struct list_head	group_list;
 	bool			fgsp : 1;	/* Fine-grained super pages */
 	bool			enforce_cache_coherency : 1;
+	bool			msi_cookie : 1;
 };
 
 struct vfio_dma {
@@ -2153,12 +2154,163 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static struct vfio_domain *
+vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu *iommu,
+			       struct vfio_iommu_group *group)
+{
+	struct iommu_domain *new_domain;
+	struct vfio_domain *domain;
+	int ret = 0;
+
+	/* Try to match an existing compatible domain */
+	list_for_each_entry (domain, &iommu->domain_list, next) {
+		ret = iommu_attach_group(domain->domain, group->iommu_group);
+		if (ret == -EMEDIUMTYPE)
+			continue;
+		if (ret)
+			return ERR_PTR(ret);
+		goto done;
+	}
+
+	new_domain = iommu_domain_alloc(bus);
+	if (!new_domain)
+		return ERR_PTR(-EIO);
+
+	if (iommu->nesting) {
+		ret = iommu_enable_nesting(new_domain);
+		if (ret)
+			goto out_free_iommu_domain;
+	}
+
+	ret = iommu_attach_group(new_domain, group->iommu_group);
+	if (ret)
+		goto out_free_iommu_domain;
+
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (!domain) {
+		ret = -ENOMEM;
+		goto out_detach;
+	}
+
+	domain->domain = new_domain;
+	vfio_test_domain_fgsp(domain);
+
+	/*
+	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+	 * no-snoop set) then VFIO always turns this feature on because on Intel
+	 * platforms it optimizes KVM to disable wbinvd emulation.
+	 */
+	if (new_domain->ops->enforce_cache_coherency)
+		domain->enforce_cache_coherency =
+			new_domain->ops->enforce_cache_coherency(new_domain);
+
+	/* replay mappings on new domains */
+	ret = vfio_iommu_replay(iommu, domain);
+	if (ret)
+		goto out_free_domain;
+
+	INIT_LIST_HEAD(&domain->group_list);
+	list_add(&domain->next, &iommu->domain_list);
+	vfio_update_pgsize_bitmap(iommu);
+
+done:
+	list_add(&group->next, &domain->group_list);
+
+	/*
+	 * An iommu backed group can dirty memory directly and therefore
+	 * demotes the iommu scope until it declares itself dirty tracking
+	 * capable via the page pinning interface.
+	 */
+	iommu->num_non_pinned_groups++;
+
+	return domain;
+
+out_free_domain:
+	kfree(domain);
+out_detach:
+	iommu_detach_group(new_domain, group->iommu_group);
+out_free_iommu_domain:
+	iommu_domain_free(new_domain);
+	return ERR_PTR(ret);
+}
+
+static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
+{
+	struct rb_node *node;
+
+	while ((node = rb_first(&iommu->dma_list)))
+		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
+}
+
+static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
+{
+	struct rb_node *n, *p;
+
+	n = rb_first(&iommu->dma_list);
+	for (; n; n = rb_next(n)) {
+		struct vfio_dma *dma;
+		long locked = 0, unlocked = 0;
+
+		dma = rb_entry(n, struct vfio_dma, node);
+		unlocked += vfio_unmap_unpin(iommu, dma, false);
+		p = rb_first(&dma->pfn_list);
+		for (; p; p = rb_next(p)) {
+			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
+							 node);
+
+			if (!is_invalid_reserved_pfn(vpfn->pfn))
+				locked++;
+		}
+		vfio_lock_acct(dma, locked - unlocked, true);
+	}
+}
+
+static void vfio_iommu_detach_destroy_domain(struct vfio_domain *domain,
+					     struct vfio_iommu *iommu,
+					     struct vfio_iommu_group *group)
+{
+	iommu_detach_group(domain->domain, group->iommu_group);
+	list_del(&group->next);
+	if (!list_empty(&domain->group_list))
+		goto out_dirty;
+
+	/*
+	 * Group ownership provides privilege, if the group list is empty, the
+	 * domain goes away. If it's the last domain with iommu and external
+	 * domain doesn't exist, then all the mappings go away too. If it's the
+	 * last domain with iommu and external domain exist, update accounting
+	 */
+	if (list_is_singular(&iommu->domain_list)) {
+		if (list_empty(&iommu->emulated_iommu_groups)) {
+			WARN_ON(iommu->notifier.head);
+			vfio_iommu_unmap_unpin_all(iommu);
+		} else {
+			vfio_iommu_unmap_unpin_reaccount(iommu);
+		}
+	}
+	iommu_domain_free(domain->domain);
+	list_del(&domain->next);
+	kfree(domain);
+	vfio_update_pgsize_bitmap(iommu);
+
+out_dirty:
+	/*
+	 * Removal of a group without dirty tracking may allow the iommu scope
+	 * to be promoted.
+	 */
+	if (!group->pinned_page_dirty_scope) {
+		iommu->num_non_pinned_groups--;
+		if (iommu->dirty_page_tracking)
+			vfio_iommu_populate_bitmap_full(iommu);
+	}
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_iommu_group *group;
-	struct vfio_domain *domain, *d;
+	struct vfio_domain *domain;
 	struct bus_type *bus = NULL;
 	bool resv_msi, msi_remap;
 	phys_addr_t resv_msi_base = 0;
@@ -2197,26 +2349,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_free_group;
 
-	ret = -ENOMEM;
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
+	domain = vfio_iommu_alloc_attach_domain(bus, iommu, group);
+	if (IS_ERR(domain)) {
+		ret = PTR_ERR(domain);
 		goto out_free_group;
-
-	ret = -EIO;
-	domain->domain = iommu_domain_alloc(bus);
-	if (!domain->domain)
-		goto out_free_domain;
-
-	if (iommu->nesting) {
-		ret = iommu_enable_nesting(domain->domain);
-		if (ret)
-			goto out_domain;
 	}
 
-	ret = iommu_attach_group(domain->domain, group->iommu_group);
-	if (ret)
-		goto out_domain;
-
 	/* Get aperture info */
 	geo = &domain->domain->geometry;
 	if (vfio_iommu_aper_conflict(iommu, geo->aperture_start,
@@ -2254,9 +2392,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 
 	resv_msi = vfio_iommu_has_sw_msi(&group_resv_regions, &resv_msi_base);
 
-	INIT_LIST_HEAD(&domain->group_list);
-	list_add(&group->next, &domain->group_list);
-
 	msi_remap = irq_domain_check_msi_remap() ||
 		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
 
@@ -2267,107 +2402,32 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
-	/*
-	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
-	 * no-snoop set) then VFIO always turns this feature on because on Intel
-	 * platforms it optimizes KVM to disable wbinvd emulation.
-	 */
-	if (domain->domain->ops->enforce_cache_coherency)
-		domain->enforce_cache_coherency =
-			domain->domain->ops->enforce_cache_coherency(
-				domain->domain);
-
-	/* Try to match an existing compatible domain */
-	list_for_each_entry(d, &iommu->domain_list, next) {
-		iommu_detach_group(domain->domain, group->iommu_group);
-		if (!iommu_attach_group(d->domain, group->iommu_group)) {
-			list_add(&group->next, &d->group_list);
-			iommu_domain_free(domain->domain);
-			kfree(domain);
-			goto done;
-		}
-
-		ret = iommu_attach_group(domain->domain,  group->iommu_group);
-		if (ret)
-			goto out_domain;
-	}
-
-	vfio_test_domain_fgsp(domain);
-
-	/* replay mappings on new domains */
-	ret = vfio_iommu_replay(iommu, domain);
-	if (ret)
-		goto out_detach;
-
-	if (resv_msi) {
+	if (resv_msi && !domain->msi_cookie) {
 		ret = iommu_get_msi_cookie(domain->domain, resv_msi_base);
 		if (ret && ret != -ENODEV)
 			goto out_detach;
+		domain->msi_cookie = true;
 	}
 
-	list_add(&domain->next, &iommu->domain_list);
-	vfio_update_pgsize_bitmap(iommu);
-done:
 	/* Delete the old one and insert new iova list */
 	vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 
-	/*
-	 * An iommu backed group can dirty memory directly and therefore
-	 * demotes the iommu scope until it declares itself dirty tracking
-	 * capable via the page pinning interface.
-	 */
-	iommu->num_non_pinned_groups++;
 	mutex_unlock(&iommu->lock);
 	vfio_iommu_resv_free(&group_resv_regions);
 
 	return 0;
 
 out_detach:
-	iommu_detach_group(domain->domain, group->iommu_group);
-out_domain:
-	iommu_domain_free(domain->domain);
-	vfio_iommu_iova_free(&iova_copy);
-	vfio_iommu_resv_free(&group_resv_regions);
-out_free_domain:
-	kfree(domain);
+	vfio_iommu_detach_destroy_domain(domain, iommu, group);
 out_free_group:
 	kfree(group);
 out_unlock:
 	mutex_unlock(&iommu->lock);
+	vfio_iommu_iova_free(&iova_copy);
+	vfio_iommu_resv_free(&group_resv_regions);
 	return ret;
 }
 
-static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
-{
-	struct rb_node *node;
-
-	while ((node = rb_first(&iommu->dma_list)))
-		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
-}
-
-static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
-{
-	struct rb_node *n, *p;
-
-	n = rb_first(&iommu->dma_list);
-	for (; n; n = rb_next(n)) {
-		struct vfio_dma *dma;
-		long locked = 0, unlocked = 0;
-
-		dma = rb_entry(n, struct vfio_dma, node);
-		unlocked += vfio_unmap_unpin(iommu, dma, false);
-		p = rb_first(&dma->pfn_list);
-		for (; p; p = rb_next(p)) {
-			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
-							 node);
-
-			if (!is_invalid_reserved_pfn(vpfn->pfn))
-				locked++;
-		}
-		vfio_lock_acct(dma, locked - unlocked, true);
-	}
-}
-
 /*
  * Called when a domain is removed in detach. It is possible that
  * the removed domain decided the iova aperture window. Modify the
@@ -2481,44 +2541,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 		group = find_iommu_group(domain, iommu_group);
 		if (!group)
 			continue;
-
-		iommu_detach_group(domain->domain, group->iommu_group);
-		list_del(&group->next);
-		/*
-		 * Group ownership provides privilege, if the group list is
-		 * empty, the domain goes away. If it's the last domain with
-		 * iommu and external domain doesn't exist, then all the
-		 * mappings go away too. If it's the last domain with iommu and
-		 * external domain exist, update accounting
-		 */
-		if (list_empty(&domain->group_list)) {
-			if (list_is_singular(&iommu->domain_list)) {
-				if (list_empty(&iommu->emulated_iommu_groups)) {
-					WARN_ON(iommu->notifier.head);
-					vfio_iommu_unmap_unpin_all(iommu);
-				} else {
-					vfio_iommu_unmap_unpin_reaccount(iommu);
-				}
-			}
-			iommu_domain_free(domain->domain);
-			list_del(&domain->next);
-			kfree(domain);
-			vfio_iommu_aper_expand(iommu, &iova_copy);
-			vfio_update_pgsize_bitmap(iommu);
-		}
-		/*
-		 * Removal of a group without dirty tracking may allow
-		 * the iommu scope to be promoted.
-		 */
-		if (!group->pinned_page_dirty_scope) {
-			iommu->num_non_pinned_groups--;
-			if (iommu->dirty_page_tracking)
-				vfio_iommu_populate_bitmap_full(iommu);
-		}
+		vfio_iommu_detach_destroy_domain(domain, iommu, group);
 		kfree(group);
 		break;
 	}
 
+	vfio_iommu_aper_expand(iommu, &iova_copy);
 	if (!vfio_iommu_resv_refresh(iommu, &iova_copy))
 		vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 	else
-- 
2.17.1

[PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

Un-inline the domain specific logic from the attach/detach_group ops into
two paired functions vfio_iommu_alloc_attach_domain() and
vfio_iommu_detach_destroy_domain() that strictly deal with creating and
destroying struct vfio_domains.

Add the logic to check for EMEDIUMTYPE return code of iommu_attach_group()
and avoid the extra domain allocations and attach/detach sequences of the
old code. This allows properly detecting an actual attach error, like
-ENOMEM, vs treating all attach errors as an incompatible domain.

Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
 1 file changed, 163 insertions(+), 135 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 573caf320788..5986c68e59ee 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -86,6 +86,7 @@ struct vfio_domain {
 	struct list_head	group_list;
 	bool			fgsp : 1;	/* Fine-grained super pages */
 	bool			enforce_cache_coherency : 1;
+	bool			msi_cookie : 1;
 };
 
 struct vfio_dma {
@@ -2153,12 +2154,163 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static struct vfio_domain *
+vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu *iommu,
+			       struct vfio_iommu_group *group)
+{
+	struct iommu_domain *new_domain;
+	struct vfio_domain *domain;
+	int ret = 0;
+
+	/* Try to match an existing compatible domain */
+	list_for_each_entry (domain, &iommu->domain_list, next) {
+		ret = iommu_attach_group(domain->domain, group->iommu_group);
+		if (ret == -EMEDIUMTYPE)
+			continue;
+		if (ret)
+			return ERR_PTR(ret);
+		goto done;
+	}
+
+	new_domain = iommu_domain_alloc(bus);
+	if (!new_domain)
+		return ERR_PTR(-EIO);
+
+	if (iommu->nesting) {
+		ret = iommu_enable_nesting(new_domain);
+		if (ret)
+			goto out_free_iommu_domain;
+	}
+
+	ret = iommu_attach_group(new_domain, group->iommu_group);
+	if (ret)
+		goto out_free_iommu_domain;
+
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (!domain) {
+		ret = -ENOMEM;
+		goto out_detach;
+	}
+
+	domain->domain = new_domain;
+	vfio_test_domain_fgsp(domain);
+
+	/*
+	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+	 * no-snoop set) then VFIO always turns this feature on because on Intel
+	 * platforms it optimizes KVM to disable wbinvd emulation.
+	 */
+	if (new_domain->ops->enforce_cache_coherency)
+		domain->enforce_cache_coherency =
+			new_domain->ops->enforce_cache_coherency(new_domain);
+
+	/* replay mappings on new domains */
+	ret = vfio_iommu_replay(iommu, domain);
+	if (ret)
+		goto out_free_domain;
+
+	INIT_LIST_HEAD(&domain->group_list);
+	list_add(&domain->next, &iommu->domain_list);
+	vfio_update_pgsize_bitmap(iommu);
+
+done:
+	list_add(&group->next, &domain->group_list);
+
+	/*
+	 * An iommu backed group can dirty memory directly and therefore
+	 * demotes the iommu scope until it declares itself dirty tracking
+	 * capable via the page pinning interface.
+	 */
+	iommu->num_non_pinned_groups++;
+
+	return domain;
+
+out_free_domain:
+	kfree(domain);
+out_detach:
+	iommu_detach_group(new_domain, group->iommu_group);
+out_free_iommu_domain:
+	iommu_domain_free(new_domain);
+	return ERR_PTR(ret);
+}
+
+static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
+{
+	struct rb_node *node;
+
+	while ((node = rb_first(&iommu->dma_list)))
+		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
+}
+
+static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
+{
+	struct rb_node *n, *p;
+
+	n = rb_first(&iommu->dma_list);
+	for (; n; n = rb_next(n)) {
+		struct vfio_dma *dma;
+		long locked = 0, unlocked = 0;
+
+		dma = rb_entry(n, struct vfio_dma, node);
+		unlocked += vfio_unmap_unpin(iommu, dma, false);
+		p = rb_first(&dma->pfn_list);
+		for (; p; p = rb_next(p)) {
+			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
+							 node);
+
+			if (!is_invalid_reserved_pfn(vpfn->pfn))
+				locked++;
+		}
+		vfio_lock_acct(dma, locked - unlocked, true);
+	}
+}
+
+static void vfio_iommu_detach_destroy_domain(struct vfio_domain *domain,
+					     struct vfio_iommu *iommu,
+					     struct vfio_iommu_group *group)
+{
+	iommu_detach_group(domain->domain, group->iommu_group);
+	list_del(&group->next);
+	if (!list_empty(&domain->group_list))
+		goto out_dirty;
+
+	/*
+	 * Group ownership provides privilege, if the group list is empty, the
+	 * domain goes away. If it's the last domain with iommu and external
+	 * domain doesn't exist, then all the mappings go away too. If it's the
+	 * last domain with iommu and external domain exist, update accounting
+	 */
+	if (list_is_singular(&iommu->domain_list)) {
+		if (list_empty(&iommu->emulated_iommu_groups)) {
+			WARN_ON(iommu->notifier.head);
+			vfio_iommu_unmap_unpin_all(iommu);
+		} else {
+			vfio_iommu_unmap_unpin_reaccount(iommu);
+		}
+	}
+	iommu_domain_free(domain->domain);
+	list_del(&domain->next);
+	kfree(domain);
+	vfio_update_pgsize_bitmap(iommu);
+
+out_dirty:
+	/*
+	 * Removal of a group without dirty tracking may allow the iommu scope
+	 * to be promoted.
+	 */
+	if (!group->pinned_page_dirty_scope) {
+		iommu->num_non_pinned_groups--;
+		if (iommu->dirty_page_tracking)
+			vfio_iommu_populate_bitmap_full(iommu);
+	}
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_iommu_group *group;
-	struct vfio_domain *domain, *d;
+	struct vfio_domain *domain;
 	struct bus_type *bus = NULL;
 	bool resv_msi, msi_remap;
 	phys_addr_t resv_msi_base = 0;
@@ -2197,26 +2349,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_free_group;
 
-	ret = -ENOMEM;
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
+	domain = vfio_iommu_alloc_attach_domain(bus, iommu, group);
+	if (IS_ERR(domain)) {
+		ret = PTR_ERR(domain);
 		goto out_free_group;
-
-	ret = -EIO;
-	domain->domain = iommu_domain_alloc(bus);
-	if (!domain->domain)
-		goto out_free_domain;
-
-	if (iommu->nesting) {
-		ret = iommu_enable_nesting(domain->domain);
-		if (ret)
-			goto out_domain;
 	}
 
-	ret = iommu_attach_group(domain->domain, group->iommu_group);
-	if (ret)
-		goto out_domain;
-
 	/* Get aperture info */
 	geo = &domain->domain->geometry;
 	if (vfio_iommu_aper_conflict(iommu, geo->aperture_start,
@@ -2254,9 +2392,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 
 	resv_msi = vfio_iommu_has_sw_msi(&group_resv_regions, &resv_msi_base);
 
-	INIT_LIST_HEAD(&domain->group_list);
-	list_add(&group->next, &domain->group_list);
-
 	msi_remap = irq_domain_check_msi_remap() ||
 		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
 
@@ -2267,107 +2402,32 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
-	/*
-	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
-	 * no-snoop set) then VFIO always turns this feature on because on Intel
-	 * platforms it optimizes KVM to disable wbinvd emulation.
-	 */
-	if (domain->domain->ops->enforce_cache_coherency)
-		domain->enforce_cache_coherency =
-			domain->domain->ops->enforce_cache_coherency(
-				domain->domain);
-
-	/* Try to match an existing compatible domain */
-	list_for_each_entry(d, &iommu->domain_list, next) {
-		iommu_detach_group(domain->domain, group->iommu_group);
-		if (!iommu_attach_group(d->domain, group->iommu_group)) {
-			list_add(&group->next, &d->group_list);
-			iommu_domain_free(domain->domain);
-			kfree(domain);
-			goto done;
-		}
-
-		ret = iommu_attach_group(domain->domain,  group->iommu_group);
-		if (ret)
-			goto out_domain;
-	}
-
-	vfio_test_domain_fgsp(domain);
-
-	/* replay mappings on new domains */
-	ret = vfio_iommu_replay(iommu, domain);
-	if (ret)
-		goto out_detach;
-
-	if (resv_msi) {
+	if (resv_msi && !domain->msi_cookie) {
 		ret = iommu_get_msi_cookie(domain->domain, resv_msi_base);
 		if (ret && ret != -ENODEV)
 			goto out_detach;
+		domain->msi_cookie = true;
 	}
 
-	list_add(&domain->next, &iommu->domain_list);
-	vfio_update_pgsize_bitmap(iommu);
-done:
 	/* Delete the old one and insert new iova list */
 	vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 
-	/*
-	 * An iommu backed group can dirty memory directly and therefore
-	 * demotes the iommu scope until it declares itself dirty tracking
-	 * capable via the page pinning interface.
-	 */
-	iommu->num_non_pinned_groups++;
 	mutex_unlock(&iommu->lock);
 	vfio_iommu_resv_free(&group_resv_regions);
 
 	return 0;
 
 out_detach:
-	iommu_detach_group(domain->domain, group->iommu_group);
-out_domain:
-	iommu_domain_free(domain->domain);
-	vfio_iommu_iova_free(&iova_copy);
-	vfio_iommu_resv_free(&group_resv_regions);
-out_free_domain:
-	kfree(domain);
+	vfio_iommu_detach_destroy_domain(domain, iommu, group);
 out_free_group:
 	kfree(group);
 out_unlock:
 	mutex_unlock(&iommu->lock);
+	vfio_iommu_iova_free(&iova_copy);
+	vfio_iommu_resv_free(&group_resv_regions);
 	return ret;
 }
 
-static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
-{
-	struct rb_node *node;
-
-	while ((node = rb_first(&iommu->dma_list)))
-		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
-}
-
-static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
-{
-	struct rb_node *n, *p;
-
-	n = rb_first(&iommu->dma_list);
-	for (; n; n = rb_next(n)) {
-		struct vfio_dma *dma;
-		long locked = 0, unlocked = 0;
-
-		dma = rb_entry(n, struct vfio_dma, node);
-		unlocked += vfio_unmap_unpin(iommu, dma, false);
-		p = rb_first(&dma->pfn_list);
-		for (; p; p = rb_next(p)) {
-			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
-							 node);
-
-			if (!is_invalid_reserved_pfn(vpfn->pfn))
-				locked++;
-		}
-		vfio_lock_acct(dma, locked - unlocked, true);
-	}
-}
-
 /*
  * Called when a domain is removed in detach. It is possible that
  * the removed domain decided the iova aperture window. Modify the
@@ -2481,44 +2541,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 		group = find_iommu_group(domain, iommu_group);
 		if (!group)
 			continue;
-
-		iommu_detach_group(domain->domain, group->iommu_group);
-		list_del(&group->next);
-		/*
-		 * Group ownership provides privilege, if the group list is
-		 * empty, the domain goes away. If it's the last domain with
-		 * iommu and external domain doesn't exist, then all the
-		 * mappings go away too. If it's the last domain with iommu and
-		 * external domain exist, update accounting
-		 */
-		if (list_empty(&domain->group_list)) {
-			if (list_is_singular(&iommu->domain_list)) {
-				if (list_empty(&iommu->emulated_iommu_groups)) {
-					WARN_ON(iommu->notifier.head);
-					vfio_iommu_unmap_unpin_all(iommu);
-				} else {
-					vfio_iommu_unmap_unpin_reaccount(iommu);
-				}
-			}
-			iommu_domain_free(domain->domain);
-			list_del(&domain->next);
-			kfree(domain);
-			vfio_iommu_aper_expand(iommu, &iova_copy);
-			vfio_update_pgsize_bitmap(iommu);
-		}
-		/*
-		 * Removal of a group without dirty tracking may allow
-		 * the iommu scope to be promoted.
-		 */
-		if (!group->pinned_page_dirty_scope) {
-			iommu->num_non_pinned_groups--;
-			if (iommu->dirty_page_tracking)
-				vfio_iommu_populate_bitmap_full(iommu);
-		}
+		vfio_iommu_detach_destroy_domain(domain, iommu, group);
 		kfree(group);
 		break;
 	}
 
+	vfio_iommu_aper_expand(iommu, &iova_copy);
 	if (!vfio_iommu_resv_refresh(iommu, &iova_copy))
 		vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 	else
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen via iommu]

Un-inline the domain specific logic from the attach/detach_group ops into
two paired functions vfio_iommu_alloc_attach_domain() and
vfio_iommu_detach_destroy_domain() that strictly deal with creating and
destroying struct vfio_domains.

Add the logic to check for EMEDIUMTYPE return code of iommu_attach_group()
and avoid the extra domain allocations and attach/detach sequences of the
old code. This allows properly detecting an actual attach error, like
-ENOMEM, vs treating all attach errors as an incompatible domain.

Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
 1 file changed, 163 insertions(+), 135 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 573caf320788..5986c68e59ee 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -86,6 +86,7 @@ struct vfio_domain {
 	struct list_head	group_list;
 	bool			fgsp : 1;	/* Fine-grained super pages */
 	bool			enforce_cache_coherency : 1;
+	bool			msi_cookie : 1;
 };
 
 struct vfio_dma {
@@ -2153,12 +2154,163 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static struct vfio_domain *
+vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu *iommu,
+			       struct vfio_iommu_group *group)
+{
+	struct iommu_domain *new_domain;
+	struct vfio_domain *domain;
+	int ret = 0;
+
+	/* Try to match an existing compatible domain */
+	list_for_each_entry (domain, &iommu->domain_list, next) {
+		ret = iommu_attach_group(domain->domain, group->iommu_group);
+		if (ret == -EMEDIUMTYPE)
+			continue;
+		if (ret)
+			return ERR_PTR(ret);
+		goto done;
+	}
+
+	new_domain = iommu_domain_alloc(bus);
+	if (!new_domain)
+		return ERR_PTR(-EIO);
+
+	if (iommu->nesting) {
+		ret = iommu_enable_nesting(new_domain);
+		if (ret)
+			goto out_free_iommu_domain;
+	}
+
+	ret = iommu_attach_group(new_domain, group->iommu_group);
+	if (ret)
+		goto out_free_iommu_domain;
+
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (!domain) {
+		ret = -ENOMEM;
+		goto out_detach;
+	}
+
+	domain->domain = new_domain;
+	vfio_test_domain_fgsp(domain);
+
+	/*
+	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+	 * no-snoop set) then VFIO always turns this feature on because on Intel
+	 * platforms it optimizes KVM to disable wbinvd emulation.
+	 */
+	if (new_domain->ops->enforce_cache_coherency)
+		domain->enforce_cache_coherency =
+			new_domain->ops->enforce_cache_coherency(new_domain);
+
+	/* replay mappings on new domains */
+	ret = vfio_iommu_replay(iommu, domain);
+	if (ret)
+		goto out_free_domain;
+
+	INIT_LIST_HEAD(&domain->group_list);
+	list_add(&domain->next, &iommu->domain_list);
+	vfio_update_pgsize_bitmap(iommu);
+
+done:
+	list_add(&group->next, &domain->group_list);
+
+	/*
+	 * An iommu backed group can dirty memory directly and therefore
+	 * demotes the iommu scope until it declares itself dirty tracking
+	 * capable via the page pinning interface.
+	 */
+	iommu->num_non_pinned_groups++;
+
+	return domain;
+
+out_free_domain:
+	kfree(domain);
+out_detach:
+	iommu_detach_group(new_domain, group->iommu_group);
+out_free_iommu_domain:
+	iommu_domain_free(new_domain);
+	return ERR_PTR(ret);
+}
+
+static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
+{
+	struct rb_node *node;
+
+	while ((node = rb_first(&iommu->dma_list)))
+		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
+}
+
+static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
+{
+	struct rb_node *n, *p;
+
+	n = rb_first(&iommu->dma_list);
+	for (; n; n = rb_next(n)) {
+		struct vfio_dma *dma;
+		long locked = 0, unlocked = 0;
+
+		dma = rb_entry(n, struct vfio_dma, node);
+		unlocked += vfio_unmap_unpin(iommu, dma, false);
+		p = rb_first(&dma->pfn_list);
+		for (; p; p = rb_next(p)) {
+			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
+							 node);
+
+			if (!is_invalid_reserved_pfn(vpfn->pfn))
+				locked++;
+		}
+		vfio_lock_acct(dma, locked - unlocked, true);
+	}
+}
+
+static void vfio_iommu_detach_destroy_domain(struct vfio_domain *domain,
+					     struct vfio_iommu *iommu,
+					     struct vfio_iommu_group *group)
+{
+	iommu_detach_group(domain->domain, group->iommu_group);
+	list_del(&group->next);
+	if (!list_empty(&domain->group_list))
+		goto out_dirty;
+
+	/*
+	 * Group ownership provides privilege, if the group list is empty, the
+	 * domain goes away. If it's the last domain with iommu and external
+	 * domain doesn't exist, then all the mappings go away too. If it's the
+	 * last domain with iommu and external domain exist, update accounting
+	 */
+	if (list_is_singular(&iommu->domain_list)) {
+		if (list_empty(&iommu->emulated_iommu_groups)) {
+			WARN_ON(iommu->notifier.head);
+			vfio_iommu_unmap_unpin_all(iommu);
+		} else {
+			vfio_iommu_unmap_unpin_reaccount(iommu);
+		}
+	}
+	iommu_domain_free(domain->domain);
+	list_del(&domain->next);
+	kfree(domain);
+	vfio_update_pgsize_bitmap(iommu);
+
+out_dirty:
+	/*
+	 * Removal of a group without dirty tracking may allow the iommu scope
+	 * to be promoted.
+	 */
+	if (!group->pinned_page_dirty_scope) {
+		iommu->num_non_pinned_groups--;
+		if (iommu->dirty_page_tracking)
+			vfio_iommu_populate_bitmap_full(iommu);
+	}
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_iommu_group *group;
-	struct vfio_domain *domain, *d;
+	struct vfio_domain *domain;
 	struct bus_type *bus = NULL;
 	bool resv_msi, msi_remap;
 	phys_addr_t resv_msi_base = 0;
@@ -2197,26 +2349,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_free_group;
 
-	ret = -ENOMEM;
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
+	domain = vfio_iommu_alloc_attach_domain(bus, iommu, group);
+	if (IS_ERR(domain)) {
+		ret = PTR_ERR(domain);
 		goto out_free_group;
-
-	ret = -EIO;
-	domain->domain = iommu_domain_alloc(bus);
-	if (!domain->domain)
-		goto out_free_domain;
-
-	if (iommu->nesting) {
-		ret = iommu_enable_nesting(domain->domain);
-		if (ret)
-			goto out_domain;
 	}
 
-	ret = iommu_attach_group(domain->domain, group->iommu_group);
-	if (ret)
-		goto out_domain;
-
 	/* Get aperture info */
 	geo = &domain->domain->geometry;
 	if (vfio_iommu_aper_conflict(iommu, geo->aperture_start,
@@ -2254,9 +2392,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 
 	resv_msi = vfio_iommu_has_sw_msi(&group_resv_regions, &resv_msi_base);
 
-	INIT_LIST_HEAD(&domain->group_list);
-	list_add(&group->next, &domain->group_list);
-
 	msi_remap = irq_domain_check_msi_remap() ||
 		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
 
@@ -2267,107 +2402,32 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
-	/*
-	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
-	 * no-snoop set) then VFIO always turns this feature on because on Intel
-	 * platforms it optimizes KVM to disable wbinvd emulation.
-	 */
-	if (domain->domain->ops->enforce_cache_coherency)
-		domain->enforce_cache_coherency =
-			domain->domain->ops->enforce_cache_coherency(
-				domain->domain);
-
-	/* Try to match an existing compatible domain */
-	list_for_each_entry(d, &iommu->domain_list, next) {
-		iommu_detach_group(domain->domain, group->iommu_group);
-		if (!iommu_attach_group(d->domain, group->iommu_group)) {
-			list_add(&group->next, &d->group_list);
-			iommu_domain_free(domain->domain);
-			kfree(domain);
-			goto done;
-		}
-
-		ret = iommu_attach_group(domain->domain,  group->iommu_group);
-		if (ret)
-			goto out_domain;
-	}
-
-	vfio_test_domain_fgsp(domain);
-
-	/* replay mappings on new domains */
-	ret = vfio_iommu_replay(iommu, domain);
-	if (ret)
-		goto out_detach;
-
-	if (resv_msi) {
+	if (resv_msi && !domain->msi_cookie) {
 		ret = iommu_get_msi_cookie(domain->domain, resv_msi_base);
 		if (ret && ret != -ENODEV)
 			goto out_detach;
+		domain->msi_cookie = true;
 	}
 
-	list_add(&domain->next, &iommu->domain_list);
-	vfio_update_pgsize_bitmap(iommu);
-done:
 	/* Delete the old one and insert new iova list */
 	vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 
-	/*
-	 * An iommu backed group can dirty memory directly and therefore
-	 * demotes the iommu scope until it declares itself dirty tracking
-	 * capable via the page pinning interface.
-	 */
-	iommu->num_non_pinned_groups++;
 	mutex_unlock(&iommu->lock);
 	vfio_iommu_resv_free(&group_resv_regions);
 
 	return 0;
 
 out_detach:
-	iommu_detach_group(domain->domain, group->iommu_group);
-out_domain:
-	iommu_domain_free(domain->domain);
-	vfio_iommu_iova_free(&iova_copy);
-	vfio_iommu_resv_free(&group_resv_regions);
-out_free_domain:
-	kfree(domain);
+	vfio_iommu_detach_destroy_domain(domain, iommu, group);
 out_free_group:
 	kfree(group);
 out_unlock:
 	mutex_unlock(&iommu->lock);
+	vfio_iommu_iova_free(&iova_copy);
+	vfio_iommu_resv_free(&group_resv_regions);
 	return ret;
 }
 
-static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
-{
-	struct rb_node *node;
-
-	while ((node = rb_first(&iommu->dma_list)))
-		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
-}
-
-static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
-{
-	struct rb_node *n, *p;
-
-	n = rb_first(&iommu->dma_list);
-	for (; n; n = rb_next(n)) {
-		struct vfio_dma *dma;
-		long locked = 0, unlocked = 0;
-
-		dma = rb_entry(n, struct vfio_dma, node);
-		unlocked += vfio_unmap_unpin(iommu, dma, false);
-		p = rb_first(&dma->pfn_list);
-		for (; p; p = rb_next(p)) {
-			struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
-							 node);
-
-			if (!is_invalid_reserved_pfn(vpfn->pfn))
-				locked++;
-		}
-		vfio_lock_acct(dma, locked - unlocked, true);
-	}
-}
-
 /*
  * Called when a domain is removed in detach. It is possible that
  * the removed domain decided the iova aperture window. Modify the
@@ -2481,44 +2541,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 		group = find_iommu_group(domain, iommu_group);
 		if (!group)
 			continue;
-
-		iommu_detach_group(domain->domain, group->iommu_group);
-		list_del(&group->next);
-		/*
-		 * Group ownership provides privilege, if the group list is
-		 * empty, the domain goes away. If it's the last domain with
-		 * iommu and external domain doesn't exist, then all the
-		 * mappings go away too. If it's the last domain with iommu and
-		 * external domain exist, update accounting
-		 */
-		if (list_empty(&domain->group_list)) {
-			if (list_is_singular(&iommu->domain_list)) {
-				if (list_empty(&iommu->emulated_iommu_groups)) {
-					WARN_ON(iommu->notifier.head);
-					vfio_iommu_unmap_unpin_all(iommu);
-				} else {
-					vfio_iommu_unmap_unpin_reaccount(iommu);
-				}
-			}
-			iommu_domain_free(domain->domain);
-			list_del(&domain->next);
-			kfree(domain);
-			vfio_iommu_aper_expand(iommu, &iova_copy);
-			vfio_update_pgsize_bitmap(iommu);
-		}
-		/*
-		 * Removal of a group without dirty tracking may allow
-		 * the iommu scope to be promoted.
-		 */
-		if (!group->pinned_page_dirty_scope) {
-			iommu->num_non_pinned_groups--;
-			if (iommu->dirty_page_tracking)
-				vfio_iommu_populate_bitmap_full(iommu);
-		}
+		vfio_iommu_detach_destroy_domain(domain, iommu, group);
 		kfree(group);
 		break;
 	}
 
+	vfio_iommu_aper_expand(iommu, &iova_copy);
 	if (!vfio_iommu_resv_refresh(iommu, &iova_copy))
 		vfio_iommu_iova_insert_copy(iommu, &iova_copy);
 	else
-- 
2.17.1

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

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
> 
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
> 
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
>  1 file changed, 163 insertions(+), 135 deletions(-)
> 

...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +			       struct vfio_iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	struct vfio_domain *domain;
> +	int ret = 0;
> +
> +	/* Try to match an existing compatible domain */
> +	list_for_each_entry (domain, &iommu->domain_list, next) {
> +		ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> +		if (ret == -EMEDIUMTYPE)
> +			continue;

Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.

...
> -	if (resv_msi) {
> +	if (resv_msi && !domain->msi_cookie) {
>  		ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
>  		if (ret && ret != -ENODEV)
>  			goto out_detach;
> +		domain->msi_cookie = true;
>  	}

why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.

...
> -		if (list_empty(&domain->group_list)) {
> -			if (list_is_singular(&iommu->domain_list)) {
> -				if (list_empty(&iommu-
> >emulated_iommu_groups)) {
> -					WARN_ON(iommu->notifier.head);
> -
> 	vfio_iommu_unmap_unpin_all(iommu);
> -				} else {
> -
> 	vfio_iommu_unmap_unpin_reaccount(iommu);
> -				}
> -			}
> -			iommu_domain_free(domain->domain);
> -			list_del(&domain->next);
> -			kfree(domain);
> -			vfio_iommu_aper_expand(iommu, &iova_copy);

Previously the aperture is adjusted when a domain is freed...

> -			vfio_update_pgsize_bitmap(iommu);
> -		}
> -		/*
> -		 * Removal of a group without dirty tracking may allow
> -		 * the iommu scope to be promoted.
> -		 */
> -		if (!group->pinned_page_dirty_scope) {
> -			iommu->num_non_pinned_groups--;
> -			if (iommu->dirty_page_tracking)
> -				vfio_iommu_populate_bitmap_full(iommu);
> -		}
> +		vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
>  		kfree(group);
>  		break;
>  	}
> 
> +	vfio_iommu_aper_expand(iommu, &iova_copy);

but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
> 
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
> 
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
>  1 file changed, 163 insertions(+), 135 deletions(-)
> 

...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +			       struct vfio_iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	struct vfio_domain *domain;
> +	int ret = 0;
> +
> +	/* Try to match an existing compatible domain */
> +	list_for_each_entry (domain, &iommu->domain_list, next) {
> +		ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> +		if (ret == -EMEDIUMTYPE)
> +			continue;

Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.

...
> -	if (resv_msi) {
> +	if (resv_msi && !domain->msi_cookie) {
>  		ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
>  		if (ret && ret != -ENODEV)
>  			goto out_detach;
> +		domain->msi_cookie = true;
>  	}

why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.

...
> -		if (list_empty(&domain->group_list)) {
> -			if (list_is_singular(&iommu->domain_list)) {
> -				if (list_empty(&iommu-
> >emulated_iommu_groups)) {
> -					WARN_ON(iommu->notifier.head);
> -
> 	vfio_iommu_unmap_unpin_all(iommu);
> -				} else {
> -
> 	vfio_iommu_unmap_unpin_reaccount(iommu);
> -				}
> -			}
> -			iommu_domain_free(domain->domain);
> -			list_del(&domain->next);
> -			kfree(domain);
> -			vfio_iommu_aper_expand(iommu, &iova_copy);

Previously the aperture is adjusted when a domain is freed...

> -			vfio_update_pgsize_bitmap(iommu);
> -		}
> -		/*
> -		 * Removal of a group without dirty tracking may allow
> -		 * the iommu scope to be promoted.
> -		 */
> -		if (!group->pinned_page_dirty_scope) {
> -			iommu->num_non_pinned_groups--;
> -			if (iommu->dirty_page_tracking)
> -				vfio_iommu_populate_bitmap_full(iommu);
> -		}
> +		vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
>  		kfree(group);
>  		break;
>  	}
> 
> +	vfio_iommu_aper_expand(iommu, &iova_copy);

but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
> 
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
> 
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
>  1 file changed, 163 insertions(+), 135 deletions(-)
> 

...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +			       struct vfio_iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	struct vfio_domain *domain;
> +	int ret = 0;
> +
> +	/* Try to match an existing compatible domain */
> +	list_for_each_entry (domain, &iommu->domain_list, next) {
> +		ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> +		if (ret == -EMEDIUMTYPE)
> +			continue;

Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.

...
> -	if (resv_msi) {
> +	if (resv_msi && !domain->msi_cookie) {
>  		ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
>  		if (ret && ret != -ENODEV)
>  			goto out_detach;
> +		domain->msi_cookie = true;
>  	}

why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.

...
> -		if (list_empty(&domain->group_list)) {
> -			if (list_is_singular(&iommu->domain_list)) {
> -				if (list_empty(&iommu-
> >emulated_iommu_groups)) {
> -					WARN_ON(iommu->notifier.head);
> -
> 	vfio_iommu_unmap_unpin_all(iommu);
> -				} else {
> -
> 	vfio_iommu_unmap_unpin_reaccount(iommu);
> -				}
> -			}
> -			iommu_domain_free(domain->domain);
> -			list_del(&domain->next);
> -			kfree(domain);
> -			vfio_iommu_aper_expand(iommu, &iova_copy);

Previously the aperture is adjusted when a domain is freed...

> -			vfio_update_pgsize_bitmap(iommu);
> -		}
> -		/*
> -		 * Removal of a group without dirty tracking may allow
> -		 * the iommu scope to be promoted.
> -		 */
> -		if (!group->pinned_page_dirty_scope) {
> -			iommu->num_non_pinned_groups--;
> -			if (iommu->dirty_page_tracking)
> -				vfio_iommu_populate_bitmap_full(iommu);
> -		}
> +		vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
>  		kfree(group);
>  		break;
>  	}
> 
> +	vfio_iommu_aper_expand(iommu, &iova_copy);

but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.

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

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
> 
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
> 
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 298 +++++++++++++++++---------------
>  1 file changed, 163 insertions(+), 135 deletions(-)
> 

...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +			       struct vfio_iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	struct vfio_domain *domain;
> +	int ret = 0;
> +
> +	/* Try to match an existing compatible domain */
> +	list_for_each_entry (domain, &iommu->domain_list, next) {
> +		ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> +		if (ret == -EMEDIUMTYPE)
> +			continue;

Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.

...
> -	if (resv_msi) {
> +	if (resv_msi && !domain->msi_cookie) {
>  		ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
>  		if (ret && ret != -ENODEV)
>  			goto out_detach;
> +		domain->msi_cookie = true;
>  	}

why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.

...
> -		if (list_empty(&domain->group_list)) {
> -			if (list_is_singular(&iommu->domain_list)) {
> -				if (list_empty(&iommu-
> >emulated_iommu_groups)) {
> -					WARN_ON(iommu->notifier.head);
> -
> 	vfio_iommu_unmap_unpin_all(iommu);
> -				} else {
> -
> 	vfio_iommu_unmap_unpin_reaccount(iommu);
> -				}
> -			}
> -			iommu_domain_free(domain->domain);
> -			list_del(&domain->next);
> -			kfree(domain);
> -			vfio_iommu_aper_expand(iommu, &iova_copy);

Previously the aperture is adjusted when a domain is freed...

> -			vfio_update_pgsize_bitmap(iommu);
> -		}
> -		/*
> -		 * Removal of a group without dirty tracking may allow
> -		 * the iommu scope to be promoted.
> -		 */
> -		if (!group->pinned_page_dirty_scope) {
> -			iommu->num_non_pinned_groups--;
> -			if (iommu->dirty_page_tracking)
> -				vfio_iommu_populate_bitmap_full(iommu);
> -		}
> +		vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
>  		kfree(group);
>  		break;
>  	}
> 
> +	vfio_iommu_aper_expand(iommu, &iova_copy);

but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.

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

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Thu, Jun 16, 2022 at 07:08:10AM +0000, Tian, Kevin wrote:
> ...
> > +static struct vfio_domain *
> > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> > *iommu,
> > +                            struct vfio_iommu_group *group)
> > +{
> > +     struct iommu_domain *new_domain;
> > +     struct vfio_domain *domain;
> > +     int ret = 0;
> > +
> > +     /* Try to match an existing compatible domain */
> > +     list_for_each_entry (domain, &iommu->domain_list, next) {
> > +             ret = iommu_attach_group(domain->domain, group-
> > >iommu_group);
> > +             if (ret == -EMEDIUMTYPE)
> > +                     continue;
> 
> Probably good to add one line comment here for what EMEDIUMTYPE
> represents. It's not a widely-used retry type like EAGAIN. A comment
> can save the time of digging out the fact by jumping to iommu file.

Sure. I can add that.

> ...
> > -     if (resv_msi) {
> > +     if (resv_msi && !domain->msi_cookie) {
> >               ret = iommu_get_msi_cookie(domain->domain,
> > resv_msi_base);
> >               if (ret && ret != -ENODEV)
> >                       goto out_detach;
> > +             domain->msi_cookie = true;
> >       }
> 
> why not moving to alloc_attach_domain() then no need for the new
> domain field? It's required only when a new domain is allocated.

When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.

> ...
> > -             if (list_empty(&domain->group_list)) {
> > -                     if (list_is_singular(&iommu->domain_list)) {
> > -                             if (list_empty(&iommu-
> > >emulated_iommu_groups)) {
> > -                                     WARN_ON(iommu->notifier.head);
> > -
> >       vfio_iommu_unmap_unpin_all(iommu);
> > -                             } else {
> > -
> >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > -                             }
> > -                     }
> > -                     iommu_domain_free(domain->domain);
> > -                     list_del(&domain->next);
> > -                     kfree(domain);
> > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> Previously the aperture is adjusted when a domain is freed...
> 
> > -                     vfio_update_pgsize_bitmap(iommu);
> > -             }
> > -             /*
> > -              * Removal of a group without dirty tracking may allow
> > -              * the iommu scope to be promoted.
> > -              */
> > -             if (!group->pinned_page_dirty_scope) {
> > -                     iommu->num_non_pinned_groups--;
> > -                     if (iommu->dirty_page_tracking)
> > -                             vfio_iommu_populate_bitmap_full(iommu);
> > -             }
> > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > group);
> >               kfree(group);
> >               break;
> >       }
> >
> > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> but now it's done for every group detach. The aperture is decided
> by domain geometry which is not affected by attached groups.

Yea, I've noticed this part. Actually Jason did this change for
simplicity, and I think it'd be safe to do so?

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Thu, Jun 16, 2022 at 07:08:10AM +0000, Tian, Kevin wrote:
> ...
> > +static struct vfio_domain *
> > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> > *iommu,
> > +                            struct vfio_iommu_group *group)
> > +{
> > +     struct iommu_domain *new_domain;
> > +     struct vfio_domain *domain;
> > +     int ret = 0;
> > +
> > +     /* Try to match an existing compatible domain */
> > +     list_for_each_entry (domain, &iommu->domain_list, next) {
> > +             ret = iommu_attach_group(domain->domain, group-
> > >iommu_group);
> > +             if (ret == -EMEDIUMTYPE)
> > +                     continue;
> 
> Probably good to add one line comment here for what EMEDIUMTYPE
> represents. It's not a widely-used retry type like EAGAIN. A comment
> can save the time of digging out the fact by jumping to iommu file.

Sure. I can add that.

> ...
> > -     if (resv_msi) {
> > +     if (resv_msi && !domain->msi_cookie) {
> >               ret = iommu_get_msi_cookie(domain->domain,
> > resv_msi_base);
> >               if (ret && ret != -ENODEV)
> >                       goto out_detach;
> > +             domain->msi_cookie = true;
> >       }
> 
> why not moving to alloc_attach_domain() then no need for the new
> domain field? It's required only when a new domain is allocated.

When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.

> ...
> > -             if (list_empty(&domain->group_list)) {
> > -                     if (list_is_singular(&iommu->domain_list)) {
> > -                             if (list_empty(&iommu-
> > >emulated_iommu_groups)) {
> > -                                     WARN_ON(iommu->notifier.head);
> > -
> >       vfio_iommu_unmap_unpin_all(iommu);
> > -                             } else {
> > -
> >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > -                             }
> > -                     }
> > -                     iommu_domain_free(domain->domain);
> > -                     list_del(&domain->next);
> > -                     kfree(domain);
> > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> Previously the aperture is adjusted when a domain is freed...
> 
> > -                     vfio_update_pgsize_bitmap(iommu);
> > -             }
> > -             /*
> > -              * Removal of a group without dirty tracking may allow
> > -              * the iommu scope to be promoted.
> > -              */
> > -             if (!group->pinned_page_dirty_scope) {
> > -                     iommu->num_non_pinned_groups--;
> > -                     if (iommu->dirty_page_tracking)
> > -                             vfio_iommu_populate_bitmap_full(iommu);
> > -             }
> > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > group);
> >               kfree(group);
> >               break;
> >       }
> >
> > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> but now it's done for every group detach. The aperture is decided
> by domain geometry which is not affected by attached groups.

Yea, I've noticed this part. Actually Jason did this change for
simplicity, and I think it'd be safe to do so?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen via iommu]

On Thu, Jun 16, 2022 at 07:08:10AM +0000, Tian, Kevin wrote:
> ...
> > +static struct vfio_domain *
> > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> > *iommu,
> > +                            struct vfio_iommu_group *group)
> > +{
> > +     struct iommu_domain *new_domain;
> > +     struct vfio_domain *domain;
> > +     int ret = 0;
> > +
> > +     /* Try to match an existing compatible domain */
> > +     list_for_each_entry (domain, &iommu->domain_list, next) {
> > +             ret = iommu_attach_group(domain->domain, group-
> > >iommu_group);
> > +             if (ret == -EMEDIUMTYPE)
> > +                     continue;
> 
> Probably good to add one line comment here for what EMEDIUMTYPE
> represents. It's not a widely-used retry type like EAGAIN. A comment
> can save the time of digging out the fact by jumping to iommu file.

Sure. I can add that.

> ...
> > -     if (resv_msi) {
> > +     if (resv_msi && !domain->msi_cookie) {
> >               ret = iommu_get_msi_cookie(domain->domain,
> > resv_msi_base);
> >               if (ret && ret != -ENODEV)
> >                       goto out_detach;
> > +             domain->msi_cookie = true;
> >       }
> 
> why not moving to alloc_attach_domain() then no need for the new
> domain field? It's required only when a new domain is allocated.

When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.

> ...
> > -             if (list_empty(&domain->group_list)) {
> > -                     if (list_is_singular(&iommu->domain_list)) {
> > -                             if (list_empty(&iommu-
> > >emulated_iommu_groups)) {
> > -                                     WARN_ON(iommu->notifier.head);
> > -
> >       vfio_iommu_unmap_unpin_all(iommu);
> > -                             } else {
> > -
> >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > -                             }
> > -                     }
> > -                     iommu_domain_free(domain->domain);
> > -                     list_del(&domain->next);
> > -                     kfree(domain);
> > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> Previously the aperture is adjusted when a domain is freed...
> 
> > -                     vfio_update_pgsize_bitmap(iommu);
> > -             }
> > -             /*
> > -              * Removal of a group without dirty tracking may allow
> > -              * the iommu scope to be promoted.
> > -              */
> > -             if (!group->pinned_page_dirty_scope) {
> > -                     iommu->num_non_pinned_groups--;
> > -                     if (iommu->dirty_page_tracking)
> > -                             vfio_iommu_populate_bitmap_full(iommu);
> > -             }
> > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > group);
> >               kfree(group);
> >               break;
> >       }
> >
> > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> 
> but now it's done for every group detach. The aperture is decided
> by domain geometry which is not affected by attached groups.

Yea, I've noticed this part. Actually Jason did this change for
simplicity, and I think it'd be safe to do so?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 17, 2022 6:41 AM
> 
> > ...
> > > -     if (resv_msi) {
> > > +     if (resv_msi && !domain->msi_cookie) {
> > >               ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > >               if (ret && ret != -ENODEV)
> > >                       goto out_detach;
> > > +             domain->msi_cookie = true;
> > >       }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
> 
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.

Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.

But let's hear whether Robin has a different thought here.

> 
> > ...
> > > -             if (list_empty(&domain->group_list)) {
> > > -                     if (list_is_singular(&iommu->domain_list)) {
> > > -                             if (list_empty(&iommu-
> > > >emulated_iommu_groups)) {
> > > -                                     WARN_ON(iommu->notifier.head);
> > > -
> > >       vfio_iommu_unmap_unpin_all(iommu);
> > > -                             } else {
> > > -
> > >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > > -                             }
> > > -                     }
> > > -                     iommu_domain_free(domain->domain);
> > > -                     list_del(&domain->next);
> > > -                     kfree(domain);
> > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > -                     vfio_update_pgsize_bitmap(iommu);
> > > -             }
> > > -             /*
> > > -              * Removal of a group without dirty tracking may allow
> > > -              * the iommu scope to be promoted.
> > > -              */
> > > -             if (!group->pinned_page_dirty_scope) {
> > > -                     iommu->num_non_pinned_groups--;
> > > -                     if (iommu->dirty_page_tracking)
> > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > -             }
> > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > >               kfree(group);
> > >               break;
> > >       }
> > >
> > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
> 
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?

Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 17, 2022 6:41 AM
> 
> > ...
> > > -     if (resv_msi) {
> > > +     if (resv_msi && !domain->msi_cookie) {
> > >               ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > >               if (ret && ret != -ENODEV)
> > >                       goto out_detach;
> > > +             domain->msi_cookie = true;
> > >       }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
> 
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.

Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.

But let's hear whether Robin has a different thought here.

> 
> > ...
> > > -             if (list_empty(&domain->group_list)) {
> > > -                     if (list_is_singular(&iommu->domain_list)) {
> > > -                             if (list_empty(&iommu-
> > > >emulated_iommu_groups)) {
> > > -                                     WARN_ON(iommu->notifier.head);
> > > -
> > >       vfio_iommu_unmap_unpin_all(iommu);
> > > -                             } else {
> > > -
> > >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > > -                             }
> > > -                     }
> > > -                     iommu_domain_free(domain->domain);
> > > -                     list_del(&domain->next);
> > > -                     kfree(domain);
> > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > -                     vfio_update_pgsize_bitmap(iommu);
> > > -             }
> > > -             /*
> > > -              * Removal of a group without dirty tracking may allow
> > > -              * the iommu scope to be promoted.
> > > -              */
> > > -             if (!group->pinned_page_dirty_scope) {
> > > -                     iommu->num_non_pinned_groups--;
> > > -                     if (iommu->dirty_page_tracking)
> > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > -             }
> > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > >               kfree(group);
> > >               break;
> > >       }
> > >
> > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
> 
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?

Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 17, 2022 6:41 AM
> 
> > ...
> > > -     if (resv_msi) {
> > > +     if (resv_msi && !domain->msi_cookie) {
> > >               ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > >               if (ret && ret != -ENODEV)
> > >                       goto out_detach;
> > > +             domain->msi_cookie = true;
> > >       }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
> 
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.

Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.

But let's hear whether Robin has a different thought here.

> 
> > ...
> > > -             if (list_empty(&domain->group_list)) {
> > > -                     if (list_is_singular(&iommu->domain_list)) {
> > > -                             if (list_empty(&iommu-
> > > >emulated_iommu_groups)) {
> > > -                                     WARN_ON(iommu->notifier.head);
> > > -
> > >       vfio_iommu_unmap_unpin_all(iommu);
> > > -                             } else {
> > > -
> > >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > > -                             }
> > > -                     }
> > > -                     iommu_domain_free(domain->domain);
> > > -                     list_del(&domain->next);
> > > -                     kfree(domain);
> > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > -                     vfio_update_pgsize_bitmap(iommu);
> > > -             }
> > > -             /*
> > > -              * Removal of a group without dirty tracking may allow
> > > -              * the iommu scope to be promoted.
> > > -              */
> > > -             if (!group->pinned_page_dirty_scope) {
> > > -                     iommu->num_non_pinned_groups--;
> > > -                     if (iommu->dirty_page_tracking)
> > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > -             }
> > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > >               kfree(group);
> > >               break;
> > >       }
> > >
> > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
> 
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?

Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 17, 2022 6:41 AM
> 
> > ...
> > > -     if (resv_msi) {
> > > +     if (resv_msi && !domain->msi_cookie) {
> > >               ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > >               if (ret && ret != -ENODEV)
> > >                       goto out_detach;
> > > +             domain->msi_cookie = true;
> > >       }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
> 
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.

Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.

But let's hear whether Robin has a different thought here.

> 
> > ...
> > > -             if (list_empty(&domain->group_list)) {
> > > -                     if (list_is_singular(&iommu->domain_list)) {
> > > -                             if (list_empty(&iommu-
> > > >emulated_iommu_groups)) {
> > > -                                     WARN_ON(iommu->notifier.head);
> > > -
> > >       vfio_iommu_unmap_unpin_all(iommu);
> > > -                             } else {
> > > -
> > >       vfio_iommu_unmap_unpin_reaccount(iommu);
> > > -                             }
> > > -                     }
> > > -                     iommu_domain_free(domain->domain);
> > > -                     list_del(&domain->next);
> > > -                     kfree(domain);
> > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > -                     vfio_update_pgsize_bitmap(iommu);
> > > -             }
> > > -             /*
> > > -              * Removal of a group without dirty tracking may allow
> > > -              * the iommu scope to be promoted.
> > > -              */
> > > -             if (!group->pinned_page_dirty_scope) {
> > > -                     iommu->num_non_pinned_groups--;
> > > -                     if (iommu->dirty_page_tracking)
> > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > -             }
> > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > >               kfree(group);
> > >               break;
> > >       }
> > >
> > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
> 
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?

Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Fri, Jun 17, 2022 at 02:53:13AM +0000, Tian, Kevin wrote:
> > > ...
> > > > -     if (resv_msi) {
> > > > +     if (resv_msi && !domain->msi_cookie) {
> > > >               ret = iommu_get_msi_cookie(domain->domain,
> > > > resv_msi_base);
> > > >               if (ret && ret != -ENODEV)
> > > >                       goto out_detach;
> > > > +             domain->msi_cookie = true;
> > > >       }
> > >
> > > why not moving to alloc_attach_domain() then no need for the new
> > > domain field? It's required only when a new domain is allocated.
> >
> > When reusing an existing domain that doesn't have an msi_cookie,
> > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.

Do you mean "reusing existing domain" for the "mixed case"?

> But let's hear whether Robin has a different thought here.

Yea, sure.

> > > > -                     iommu_domain_free(domain->domain);
> > > > -                     list_del(&domain->next);
> > > > -                     kfree(domain);
> > > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > Previously the aperture is adjusted when a domain is freed...
> > >
> > > > -                     vfio_update_pgsize_bitmap(iommu);
> > > > -             }
> > > > -             /*
> > > > -              * Removal of a group without dirty tracking may allow
> > > > -              * the iommu scope to be promoted.
> > > > -              */
> > > > -             if (!group->pinned_page_dirty_scope) {
> > > > -                     iommu->num_non_pinned_groups--;
> > > > -                     if (iommu->dirty_page_tracking)
> > > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > > -             }
> > > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > > group);
> > > >               kfree(group);
> > > >               break;
> > > >       }
> > > >
> > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > but now it's done for every group detach. The aperture is decided
> > > by domain geometry which is not affected by attached groups.
> >
> > Yea, I've noticed this part. Actually Jason did this change for
> > simplicity, and I think it'd be safe to do so?
> 
> Perhaps detach_destroy() can return a Boolean to indicate whether
> a domain is destroyed.

It could be a solution but doesn't feel that common for a clean
function to have a return value indicating a special case. Maybe
passing in "&domain" so that we can check if it's NULL after?

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Fri, Jun 17, 2022 at 02:53:13AM +0000, Tian, Kevin wrote:
> > > ...
> > > > -     if (resv_msi) {
> > > > +     if (resv_msi && !domain->msi_cookie) {
> > > >               ret = iommu_get_msi_cookie(domain->domain,
> > > > resv_msi_base);
> > > >               if (ret && ret != -ENODEV)
> > > >                       goto out_detach;
> > > > +             domain->msi_cookie = true;
> > > >       }
> > >
> > > why not moving to alloc_attach_domain() then no need for the new
> > > domain field? It's required only when a new domain is allocated.
> >
> > When reusing an existing domain that doesn't have an msi_cookie,
> > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.

Do you mean "reusing existing domain" for the "mixed case"?

> But let's hear whether Robin has a different thought here.

Yea, sure.

> > > > -                     iommu_domain_free(domain->domain);
> > > > -                     list_del(&domain->next);
> > > > -                     kfree(domain);
> > > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > Previously the aperture is adjusted when a domain is freed...
> > >
> > > > -                     vfio_update_pgsize_bitmap(iommu);
> > > > -             }
> > > > -             /*
> > > > -              * Removal of a group without dirty tracking may allow
> > > > -              * the iommu scope to be promoted.
> > > > -              */
> > > > -             if (!group->pinned_page_dirty_scope) {
> > > > -                     iommu->num_non_pinned_groups--;
> > > > -                     if (iommu->dirty_page_tracking)
> > > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > > -             }
> > > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > > group);
> > > >               kfree(group);
> > > >               break;
> > > >       }
> > > >
> > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > but now it's done for every group detach. The aperture is decided
> > > by domain geometry which is not affected by attached groups.
> >
> > Yea, I've noticed this part. Actually Jason did this change for
> > simplicity, and I think it'd be safe to do so?
> 
> Perhaps detach_destroy() can return a Boolean to indicate whether
> a domain is destroyed.

It could be a solution but doesn't feel that common for a clean
function to have a return value indicating a special case. Maybe
passing in "&domain" so that we can check if it's NULL after?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen via iommu]

On Fri, Jun 17, 2022 at 02:53:13AM +0000, Tian, Kevin wrote:
> > > ...
> > > > -     if (resv_msi) {
> > > > +     if (resv_msi && !domain->msi_cookie) {
> > > >               ret = iommu_get_msi_cookie(domain->domain,
> > > > resv_msi_base);
> > > >               if (ret && ret != -ENODEV)
> > > >                       goto out_detach;
> > > > +             domain->msi_cookie = true;
> > > >       }
> > >
> > > why not moving to alloc_attach_domain() then no need for the new
> > > domain field? It's required only when a new domain is allocated.
> >
> > When reusing an existing domain that doesn't have an msi_cookie,
> > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.

Do you mean "reusing existing domain" for the "mixed case"?

> But let's hear whether Robin has a different thought here.

Yea, sure.

> > > > -                     iommu_domain_free(domain->domain);
> > > > -                     list_del(&domain->next);
> > > > -                     kfree(domain);
> > > > -                     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > Previously the aperture is adjusted when a domain is freed...
> > >
> > > > -                     vfio_update_pgsize_bitmap(iommu);
> > > > -             }
> > > > -             /*
> > > > -              * Removal of a group without dirty tracking may allow
> > > > -              * the iommu scope to be promoted.
> > > > -              */
> > > > -             if (!group->pinned_page_dirty_scope) {
> > > > -                     iommu->num_non_pinned_groups--;
> > > > -                     if (iommu->dirty_page_tracking)
> > > > -                             vfio_iommu_populate_bitmap_full(iommu);
> > > > -             }
> > > > +             vfio_iommu_detach_destroy_domain(domain, iommu,
> > > > group);
> > > >               kfree(group);
> > > >               break;
> > > >       }
> > > >
> > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > >
> > > but now it's done for every group detach. The aperture is decided
> > > by domain geometry which is not affected by attached groups.
> >
> > Yea, I've noticed this part. Actually Jason did this change for
> > simplicity, and I think it'd be safe to do so?
> 
> Perhaps detach_destroy() can return a Boolean to indicate whether
> a domain is destroyed.

It could be a solution but doesn't feel that common for a clean
function to have a return value indicating a special case. Maybe
passing in "&domain" so that we can check if it's NULL after?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Jason Gunthorpe]

On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:

> > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > >
> > > > but now it's done for every group detach. The aperture is decided
> > > > by domain geometry which is not affected by attached groups.
> > >
> > > Yea, I've noticed this part. Actually Jason did this change for
> > > simplicity, and I think it'd be safe to do so?
> > 
> > Perhaps detach_destroy() can return a Boolean to indicate whether
> > a domain is destroyed.
> 
> It could be a solution but doesn't feel that common for a clean
> function to have a return value indicating a special case. Maybe
> passing in "&domain" so that we can check if it's NULL after?

It is harmless to do every time, it just burns a few CPU cycles on a
slow path. We don't need complexity to optmize it.

Jason

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Jason Gunthorpe]

On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:

> > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > >
> > > > but now it's done for every group detach. The aperture is decided
> > > > by domain geometry which is not affected by attached groups.
> > >
> > > Yea, I've noticed this part. Actually Jason did this change for
> > > simplicity, and I think it'd be safe to do so?
> > 
> > Perhaps detach_destroy() can return a Boolean to indicate whether
> > a domain is destroyed.
> 
> It could be a solution but doesn't feel that common for a clean
> function to have a return value indicating a special case. Maybe
> passing in "&domain" so that we can check if it's NULL after?

It is harmless to do every time, it just burns a few CPU cycles on a
slow path. We don't need complexity to optmize it.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Jason Gunthorpe via iommu]

On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:

> > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > >
> > > > but now it's done for every group detach. The aperture is decided
> > > > by domain geometry which is not affected by attached groups.
> > >
> > > Yea, I've noticed this part. Actually Jason did this change for
> > > simplicity, and I think it'd be safe to do so?
> > 
> > Perhaps detach_destroy() can return a Boolean to indicate whether
> > a domain is destroyed.
> 
> It could be a solution but doesn't feel that common for a clean
> function to have a return value indicating a special case. Maybe
> passing in "&domain" so that we can check if it's NULL after?

It is harmless to do every time, it just burns a few CPU cycles on a
slow path. We don't need complexity to optmize it.

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

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen via iommu]

On Mon, Jun 20, 2022 at 01:03:17AM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:
> 
> > > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > > >
> > > > > but now it's done for every group detach. The aperture is decided
> > > > > by domain geometry which is not affected by attached groups.
> > > >
> > > > Yea, I've noticed this part. Actually Jason did this change for
> > > > simplicity, and I think it'd be safe to do so?
> > > 
> > > Perhaps detach_destroy() can return a Boolean to indicate whether
> > > a domain is destroyed.
> > 
> > It could be a solution but doesn't feel that common for a clean
> > function to have a return value indicating a special case. Maybe
> > passing in "&domain" so that we can check if it's NULL after?
> 
> It is harmless to do every time, it just burns a few CPU cycles on a
> slow path. We don't need complexity to optmize it.

OK. I will keep it simple then. If Kevin or other has a further
objection, please let us know.

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

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Mon, Jun 20, 2022 at 01:03:17AM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:
> 
> > > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > > >
> > > > > but now it's done for every group detach. The aperture is decided
> > > > > by domain geometry which is not affected by attached groups.
> > > >
> > > > Yea, I've noticed this part. Actually Jason did this change for
> > > > simplicity, and I think it'd be safe to do so?
> > > 
> > > Perhaps detach_destroy() can return a Boolean to indicate whether
> > > a domain is destroyed.
> > 
> > It could be a solution but doesn't feel that common for a clean
> > function to have a return value indicating a special case. Maybe
> > passing in "&domain" so that we can check if it's NULL after?
> 
> It is harmless to do every time, it just burns a few CPU cycles on a
> slow path. We don't need complexity to optmize it.

OK. I will keep it simple then. If Kevin or other has a further
objection, please let us know.

Thanks
Nic

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Mon, Jun 20, 2022 at 01:03:17AM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote:
> 
> > > > > > +     vfio_iommu_aper_expand(iommu, &iova_copy);
> > > > >
> > > > > but now it's done for every group detach. The aperture is decided
> > > > > by domain geometry which is not affected by attached groups.
> > > >
> > > > Yea, I've noticed this part. Actually Jason did this change for
> > > > simplicity, and I think it'd be safe to do so?
> > > 
> > > Perhaps detach_destroy() can return a Boolean to indicate whether
> > > a domain is destroyed.
> > 
> > It could be a solution but doesn't feel that common for a clean
> > function to have a return value indicating a special case. Maybe
> > passing in "&domain" so that we can check if it's NULL after?
> 
> It is harmless to do every time, it just burns a few CPU cycles on a
> slow path. We don't need complexity to optmize it.

OK. I will keep it simple then. If Kevin or other has a further
objection, please let us know.

Thanks
Nic

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Robin Murphy]

On 2022-06-17 03:53, Tian, Kevin wrote:
>> From: Nicolin Chen <nicolinc@nvidia.com>
>> Sent: Friday, June 17, 2022 6:41 AM
>>
>>> ...
>>>> -     if (resv_msi) {
>>>> +     if (resv_msi && !domain->msi_cookie) {
>>>>                ret = iommu_get_msi_cookie(domain->domain,
>>>> resv_msi_base);
>>>>                if (ret && ret != -ENODEV)
>>>>                        goto out_detach;
>>>> +             domain->msi_cookie = true;
>>>>        }
>>>
>>> why not moving to alloc_attach_domain() then no need for the new
>>> domain field? It's required only when a new domain is allocated.
>>
>> When reusing an existing domain that doesn't have an msi_cookie,
>> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
>> not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.
> 
> But let's hear whether Robin has a different thought here.

Yes, the cookie should logically be tied to the lifetime of the domain 
itself. In the relevant context, "an existing domain that doesn't have 
an msi_cookie" should never exist.

Thanks,
Robin.

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Robin Murphy]

On 2022-06-17 03:53, Tian, Kevin wrote:
>> From: Nicolin Chen <nicolinc@nvidia.com>
>> Sent: Friday, June 17, 2022 6:41 AM
>>
>>> ...
>>>> -     if (resv_msi) {
>>>> +     if (resv_msi && !domain->msi_cookie) {
>>>>                ret = iommu_get_msi_cookie(domain->domain,
>>>> resv_msi_base);
>>>>                if (ret && ret != -ENODEV)
>>>>                        goto out_detach;
>>>> +             domain->msi_cookie = true;
>>>>        }
>>>
>>> why not moving to alloc_attach_domain() then no need for the new
>>> domain field? It's required only when a new domain is allocated.
>>
>> When reusing an existing domain that doesn't have an msi_cookie,
>> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
>> not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.
> 
> But let's hear whether Robin has a different thought here.

Yes, the cookie should logically be tied to the lifetime of the domain 
itself. In the relevant context, "an existing domain that doesn't have 
an msi_cookie" should never exist.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Robin Murphy]

On 2022-06-17 03:53, Tian, Kevin wrote:
>> From: Nicolin Chen <nicolinc@nvidia.com>
>> Sent: Friday, June 17, 2022 6:41 AM
>>
>>> ...
>>>> -     if (resv_msi) {
>>>> +     if (resv_msi && !domain->msi_cookie) {
>>>>                ret = iommu_get_msi_cookie(domain->domain,
>>>> resv_msi_base);
>>>>                if (ret && ret != -ENODEV)
>>>>                        goto out_detach;
>>>> +             domain->msi_cookie = true;
>>>>        }
>>>
>>> why not moving to alloc_attach_domain() then no need for the new
>>> domain field? It's required only when a new domain is allocated.
>>
>> When reusing an existing domain that doesn't have an msi_cookie,
>> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
>> not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.
> 
> But let's hear whether Robin has a different thought here.

Yes, the cookie should logically be tied to the lifetime of the domain 
itself. In the relevant context, "an existing domain that doesn't have 
an msi_cookie" should never exist.

Thanks,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Robin Murphy]

On 2022-06-17 03:53, Tian, Kevin wrote:
>> From: Nicolin Chen <nicolinc@nvidia.com>
>> Sent: Friday, June 17, 2022 6:41 AM
>>
>>> ...
>>>> -     if (resv_msi) {
>>>> +     if (resv_msi && !domain->msi_cookie) {
>>>>                ret = iommu_get_msi_cookie(domain->domain,
>>>> resv_msi_base);
>>>>                if (ret && ret != -ENODEV)
>>>>                        goto out_detach;
>>>> +             domain->msi_cookie = true;
>>>>        }
>>>
>>> why not moving to alloc_attach_domain() then no need for the new
>>> domain field? It's required only when a new domain is allocated.
>>
>> When reusing an existing domain that doesn't have an msi_cookie,
>> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
>> not limited to a new domain.
> 
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.
> 
> But let's hear whether Robin has a different thought here.

Yes, the cookie should logically be tied to the lifetime of the domain 
itself. In the relevant context, "an existing domain that doesn't have 
an msi_cookie" should never exist.

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

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen via iommu]

On Mon, Jun 20, 2022 at 11:11:01AM +0100, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022-06-17 03:53, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Friday, June 17, 2022 6:41 AM
> > > 
> > > > ...
> > > > > -     if (resv_msi) {
> > > > > +     if (resv_msi && !domain->msi_cookie) {
> > > > >                ret = iommu_get_msi_cookie(domain->domain,
> > > > > resv_msi_base);
> > > > >                if (ret && ret != -ENODEV)
> > > > >                        goto out_detach;
> > > > > +             domain->msi_cookie = true;
> > > > >        }
> > > > 
> > > > why not moving to alloc_attach_domain() then no need for the new
> > > > domain field? It's required only when a new domain is allocated.
> > > 
> > > When reusing an existing domain that doesn't have an msi_cookie,
> > > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > > not limited to a new domain.
> > 
> > Looks msi_cookie requirement is per platform (currently only
> > for smmu. see arm_smmu_get_resv_regions()). If there is
> > no mixed case then above check is not required.
> > 
> > But let's hear whether Robin has a different thought here.
> 
> Yes, the cookie should logically be tied to the lifetime of the domain
> itself. In the relevant context, "an existing domain that doesn't have
> an msi_cookie" should never exist.

Thanks for the explanation. I will move the iommu_get_msi_cookie()
into alloc_attach_domain(), as Kevin suggested.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Mon, Jun 20, 2022 at 11:11:01AM +0100, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022-06-17 03:53, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Friday, June 17, 2022 6:41 AM
> > > 
> > > > ...
> > > > > -     if (resv_msi) {
> > > > > +     if (resv_msi && !domain->msi_cookie) {
> > > > >                ret = iommu_get_msi_cookie(domain->domain,
> > > > > resv_msi_base);
> > > > >                if (ret && ret != -ENODEV)
> > > > >                        goto out_detach;
> > > > > +             domain->msi_cookie = true;
> > > > >        }
> > > > 
> > > > why not moving to alloc_attach_domain() then no need for the new
> > > > domain field? It's required only when a new domain is allocated.
> > > 
> > > When reusing an existing domain that doesn't have an msi_cookie,
> > > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > > not limited to a new domain.
> > 
> > Looks msi_cookie requirement is per platform (currently only
> > for smmu. see arm_smmu_get_resv_regions()). If there is
> > no mixed case then above check is not required.
> > 
> > But let's hear whether Robin has a different thought here.
> 
> Yes, the cookie should logically be tied to the lifetime of the domain
> itself. In the relevant context, "an existing domain that doesn't have
> an msi_cookie" should never exist.

Thanks for the explanation. I will move the iommu_get_msi_cookie()
into alloc_attach_domain(), as Kevin suggested.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment

[Nicolin Chen]

On Mon, Jun 20, 2022 at 11:11:01AM +0100, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022-06-17 03:53, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Friday, June 17, 2022 6:41 AM
> > > 
> > > > ...
> > > > > -     if (resv_msi) {
> > > > > +     if (resv_msi && !domain->msi_cookie) {
> > > > >                ret = iommu_get_msi_cookie(domain->domain,
> > > > > resv_msi_base);
> > > > >                if (ret && ret != -ENODEV)
> > > > >                        goto out_detach;
> > > > > +             domain->msi_cookie = true;
> > > > >        }
> > > > 
> > > > why not moving to alloc_attach_domain() then no need for the new
> > > > domain field? It's required only when a new domain is allocated.
> > > 
> > > When reusing an existing domain that doesn't have an msi_cookie,
> > > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > > not limited to a new domain.
> > 
> > Looks msi_cookie requirement is per platform (currently only
> > for smmu. see arm_smmu_get_resv_regions()). If there is
> > no mixed case then above check is not required.
> > 
> > But let's hear whether Robin has a different thought here.
> 
> Yes, the cookie should logically be tied to the lifetime of the domain
> itself. In the relevant context, "an existing domain that doesn't have
> an msi_cookie" should never exist.

Thanks for the explanation. I will move the iommu_get_msi_cookie()
into alloc_attach_domain(), as Kevin suggested.