linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] Replace private domain with per-group default
@ 2020-03-07  6:20 Lu Baolu
  2020-03-07  6:20 ` [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops Lu Baolu
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu

Some devices are reqired to use a specific type (identity or dma)
of default domain when they are used with a vendor iommu. When the
system level default domain type is different from it, the vendor
iommu driver has to request a new default domain with either
iommu_request_dma_domain_for_dev() or iommu_request_dm_for_dev()
in the add_dev() callback. Unfortunately, these two helpers only
work when the group hasn't been assigned to any other devices,
hence, some vendor iommu driver has to use a private domain if
it fails to request a new default one.

This patch series aims to remove the private domain requirement
in vendor iommu driver with enabling the iommu generic code to
support configuring per-group default domain. It introduces a
new callback in iommu_ops, named dev_def_domain_type(), so that
the iommu generic code could check whether a device is required
to use any specific type of default domain during the process of
device probing.

If unlikely a device requires a special default domain type other
than that in use, iommu probe procedure will either allocate a new
domain according to the specified domain type, or (if the group has
other devices sitting in it) change the default domain. The vendor
iommu driver which exposes the dev_def_domain_type callback should
guarantee that there're no multiple devices belonging to a same
group require differnt types of default domain.

Please help to review.

Best regards,
baolu

Lu Baolu (5):
  iommu: Configure default domain with dev_def_domain_type
  iommu/vt-d: Don't force 32bit devices to uses DMA domain
  iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
  iommu/vt-d: Add dev_def_domain_type callback
  iommu/vt-d: Apply per-device dma_ops

Sai Praneeth Prakhya (1):
  iommu: Add dev_def_domain_type() callback in iommu_ops

 drivers/iommu/intel-iommu.c | 453 +++---------------------------------
 drivers/iommu/iommu.c       |  93 +++++++-
 include/linux/iommu.h       |   6 +
 3 files changed, 126 insertions(+), 426 deletions(-)

-- 
2.17.1


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

* [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-07 14:18   ` Christoph Hellwig
  2020-03-07  6:20 ` [PATCH 2/6] iommu: Configure default domain with dev_def_domain_type Lu Baolu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Sai Praneeth Prakhya, Lu Baolu

From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

Some devices are reqired to use a specific type (identity or dma)
of default domain when they are used with a vendor iommu. When the
system level default domain type is different from it, the vendor
iommu driver has to request a new default domain with
iommu_request_dma_domain_for_dev() and iommu_request_dm_for_dev()
in the add_dev() callback. Unfortunately, these two helpers only
work when the group hasn't been assigned to any other devices,
hence, some vendor iommu driver has to use a private domain if
it fails to request a new default one.

This adds dev_def_domain_type() callback in the iommu_ops, so that
the special requirement of default domain for a device could be
aware by the iommu generic layer.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d1b5f4d98569..0c1e95e6b263 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -248,6 +248,10 @@ struct iommu_iotlb_gather {
  * @cache_invalidate: invalidate translation caches
  * @sva_bind_gpasid: bind guest pasid and mm
  * @sva_unbind_gpasid: unbind guest pasid and mm
+ * @dev_def_domain_type: device default domain type, return value:
+ *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
+ *		- IOMMU_DOMAIN_DMA: must use a dma domain
+ *		- 0: use the default setting
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  */
@@ -318,6 +322,8 @@ struct iommu_ops {
 
 	int (*sva_unbind_gpasid)(struct device *dev, int pasid);
 
+	int (*dev_def_domain_type)(struct device *dev);
+
 	unsigned long pgsize_bitmap;
 	struct module *owner;
 };
-- 
2.17.1


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

* [PATCH 2/6] iommu: Configure default domain with dev_def_domain_type
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
  2020-03-07  6:20 ` [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-07  6:20 ` [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain Lu Baolu
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu, Sai Praneeth Prakhya

With dev_def_domain_type introduced, iommu default domain framework
is now able to determine a proper default domain for a group. Let's
use this to configure a group's default domain.

If unlikely a device requires a special default domain type other
than that in use, iommu probe procedure will either allocate a new
domain according to the specified domain type, or (if the group has
other devices sitting in it) change the default domain. The vendor
iommu driver which exposes the dev_def_domain_type callback should
guarantee that there're no multiple devices belonging to a same
group requires differnt types of default domain.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 93 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3e3528436e0b..095e0bc563c4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1361,6 +1361,79 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
+/**
+ * Changes the default domain of a group
+ *
+ * @group: The group for which the default domain should be changed
+ * @type: The new default domain type
+ *
+ * The caller should hold the group->mutex before call into this function.
+ * Returns 0 on success and error code on failure.
+ */
+static int iommu_group_change_def_domain(struct iommu_group *group, int type)
+{
+	struct group_device *grp_dev, *temp;
+	struct iommu_domain *new, *old;
+	const struct iommu_ops *ops;
+	int ret = 0;
+
+	if ((type != IOMMU_DOMAIN_IDENTITY && type != IOMMU_DOMAIN_DMA) ||
+	    !group->default_domain || type == group->default_domain->type ||
+	    !group->default_domain->ops)
+		return -EINVAL;
+
+	if (group->domain != group->default_domain)
+		return -EBUSY;
+
+	iommu_group_ref_get(group);
+	old = group->default_domain;
+	ops = group->default_domain->ops;
+
+	/* Allocate a new domain of requested type. */
+	new = ops->domain_alloc(type);
+	if (!new) {
+		ret = -ENOMEM;
+		goto domain_out;
+	}
+	new->type = type;
+	new->ops = ops;
+	new->pgsize_bitmap = group->default_domain->pgsize_bitmap;
+
+	group->default_domain = new;
+	group->domain = new;
+	list_for_each_entry_safe(grp_dev, temp, &group->devices, list) {
+		struct device *dev;
+
+		dev = grp_dev->dev;
+		if (device_is_bound(dev)) {
+			ret = -EINVAL;
+			goto device_out;
+		}
+
+		iommu_group_create_direct_mappings(group, dev);
+		ret = __iommu_attach_device(group->domain, dev);
+		if (ret)
+			goto device_out;
+
+		ret = ops->add_device(dev);
+		if (ret)
+			goto device_out;
+	}
+
+	iommu_group_put(group);
+	iommu_domain_free(old);
+
+	return 0;
+
+device_out:
+	group->default_domain = old;
+	__iommu_attach_group(old, group);
+	iommu_domain_free(new);
+domain_out:
+	iommu_group_put(group);
+	return ret;
+}
+
 /**
  * iommu_group_get_for_dev - Find or create the IOMMU group for a device
  * @dev: target device
@@ -1375,6 +1448,7 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 {
 	const struct iommu_ops *ops = dev->bus->iommu_ops;
 	struct iommu_group *group;
+	int dev_def_type = 0;
 	int ret;
 
 	group = iommu_group_get(dev);
@@ -1391,20 +1465,24 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	if (IS_ERR(group))
 		return group;
 
+	if (ops->dev_def_domain_type)
+		dev_def_type = ops->dev_def_domain_type(dev);
+
 	/*
 	 * Try to allocate a default domain - needs support from the
 	 * IOMMU driver.
 	 */
 	if (!group->default_domain) {
 		struct iommu_domain *dom;
+		int type = dev_def_type ? : iommu_def_domain_type;
 
-		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
-		if (!dom && iommu_def_domain_type != IOMMU_DOMAIN_DMA) {
+		dom = __iommu_domain_alloc(dev->bus, type);
+		if (!dom && type != IOMMU_DOMAIN_DMA) {
 			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
 			if (dom) {
 				dev_warn(dev,
 					 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
-					 iommu_def_domain_type);
+					 type);
 			}
 		}
 
@@ -1418,6 +1496,15 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
 					      &attr);
 		}
+	} else if (dev_def_type &&
+		   dev_def_type != group->default_domain->type) {
+		mutex_lock(&group->mutex);
+		ret = iommu_group_change_def_domain(group, dev_def_type);
+		mutex_unlock(&group->mutex);
+		if (ret) {
+			iommu_group_put(group);
+			return ERR_PTR(ret);
+		}
 	}
 
 	ret = iommu_group_add_device(group, dev);
-- 
2.17.1


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

* [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
  2020-03-07  6:20 ` [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops Lu Baolu
  2020-03-07  6:20 ` [PATCH 2/6] iommu: Configure default domain with dev_def_domain_type Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-07 14:21   ` Christoph Hellwig
  2020-03-07  6:20 ` [PATCH 4/6] iommu/vt-d: Don't force PCI sub-hierarchy to use " Lu Baolu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu

Currently, if a 32bit device initially uses an identity domain,
Intel IOMMU driver will convert it forcibly to a DMA one if its
address capability is not enough for the whole system memory.
The motivation was to overcome the overhead caused by possible
bounced buffer.

Unfortunately, this improvement has led to many problems. For
example, some 32bit devices are required to use an identity
domain, forcing them to use DMA domain will cause the device
not to work anymore. On the other hand, the VMD sub-devices
share a domain but each sub-device might have different address
capability. Forcing a VMD sub-device to use DMA domain blindly
will impact the operation of other sub-devices without any
notification. Further more, PCI aliased devices (PCI bridge
and all devices beneath it, VMD devices and various devices
quirked with pci_add_dma_alias()) must use the same domain.
Forcing one device to switch to DMA domain during runtime
will cause in-fligh DMAs for other devices to abort or target
to other memory which might cause undefind system behavior.

Cc: Daniel Drake <drake@endlessm.com>
Cc: Derrick Jonathan <jonathan.derrick@intel.com>
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 46 +------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 33593fea0250..c58ced848206 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3582,47 +3582,13 @@ static struct dmar_domain *get_private_domain_for_dev(struct device *dev)
 /* Check if the dev needs to go through non-identity map and unmap process.*/
 static bool iommu_need_mapping(struct device *dev)
 {
-	int ret;
-
 	if (iommu_dummy(dev))
 		return false;
 
 	if (unlikely(attach_deferred(dev)))
 		do_deferred_attach(dev);
 
-	ret = identity_mapping(dev);
-	if (ret) {
-		u64 dma_mask = *dev->dma_mask;
-
-		if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
-			dma_mask = dev->coherent_dma_mask;
-
-		if (dma_mask >= dma_direct_get_required_mask(dev))
-			return false;
-
-		/*
-		 * 32 bit DMA is removed from si_domain and fall back to
-		 * non-identity mapping.
-		 */
-		dmar_remove_one_dev_info(dev);
-		ret = iommu_request_dma_domain_for_dev(dev);
-		if (ret) {
-			struct iommu_domain *domain;
-			struct dmar_domain *dmar_domain;
-
-			domain = iommu_get_domain_for_dev(dev);
-			if (domain) {
-				dmar_domain = to_dmar_domain(domain);
-				dmar_domain->flags |= DOMAIN_FLAG_LOSE_CHILDREN;
-			}
-			dmar_remove_one_dev_info(dev);
-			get_private_domain_for_dev(dev);
-		}
-
-		dev_info(dev, "32bit DMA uses non-identity mapping\n");
-	}
-
-	return true;
+	return !identity_mapping(dev);
 }
 
 static dma_addr_t __intel_map_single(struct device *dev, phys_addr_t paddr,
@@ -5179,16 +5145,6 @@ int __init intel_iommu_init(void)
 	}
 	up_write(&dmar_global_lock);
 
-#if defined(CONFIG_X86) && defined(CONFIG_SWIOTLB)
-	/*
-	 * If the system has no untrusted device or the user has decided
-	 * to disable the bounce page mechanisms, we don't need swiotlb.
-	 * Mark this and the pre-allocated bounce pages will be released
-	 * later.
-	 */
-	if (!has_untrusted_dev() || intel_no_bounce)
-		swiotlb = 0;
-#endif
 	dma_ops = &intel_dma_ops;
 
 	init_iommu_pm_ops();
-- 
2.17.1


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

* [PATCH 4/6] iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
                   ` (2 preceding siblings ...)
  2020-03-07  6:20 ` [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-07  6:20 ` [PATCH 5/6] iommu/vt-d: Add dev_def_domain_type callback Lu Baolu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu

Before commit fa954e6831789 ("iommu/vt-d: Delegate the dma domain
to upper layer"), Intel IOMMU started off with all devices in the
identity domain, and took them out later if it found they couldn't
access all of memory. This required devices behind a PCI bridge to
use a DMA domain at the beginning because all PCI devices behind
the bridge use the same source-id in their transactions and the
domain couldn't be changed at run-time.

Intel IOMMU driver is now aligned with the default domain framework,
there's no need to keep this requirement anymore.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c58ced848206..299ce175c096 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3048,31 +3048,6 @@ static int device_def_domain_type(struct device *dev)
 
 		if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
 			return IOMMU_DOMAIN_IDENTITY;
-
-		/*
-		 * We want to start off with all devices in the 1:1 domain, and
-		 * take them out later if we find they can't access all of memory.
-		 *
-		 * However, we can't do this for PCI devices behind bridges,
-		 * because all PCI devices behind the same bridge will end up
-		 * with the same source-id on their transactions.
-		 *
-		 * Practically speaking, we can't change things around for these
-		 * devices at run-time, because we can't be sure there'll be no
-		 * DMA transactions in flight for any of their siblings.
-		 *
-		 * So PCI devices (unless they're on the root bus) as well as
-		 * their parent PCI-PCI or PCIe-PCI bridges must be left _out_ of
-		 * the 1:1 domain, just in _case_ one of their siblings turns out
-		 * not to be able to map all of memory.
-		 */
-		if (!pci_is_pcie(pdev)) {
-			if (!pci_is_root_bus(pdev->bus))
-				return IOMMU_DOMAIN_DMA;
-			if (pdev->class >> 8 == PCI_CLASS_BRIDGE_PCI)
-				return IOMMU_DOMAIN_DMA;
-		} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_PCI_BRIDGE)
-			return IOMMU_DOMAIN_DMA;
 	}
 
 	return 0;
-- 
2.17.1


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

* [PATCH 5/6] iommu/vt-d: Add dev_def_domain_type callback
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
                   ` (3 preceding siblings ...)
  2020-03-07  6:20 ` [PATCH 4/6] iommu/vt-d: Don't force PCI sub-hierarchy to use " Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-07  6:20 ` [PATCH 6/6] iommu/vt-d: Apply per-device dma_ops Lu Baolu
  2020-03-10 11:15 ` [PATCH 0/6] Replace private domain with per-group default Joerg Roedel
  6 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu

Add vt-d specific dev_def_domain_type callback and remove the
unnecessary homemade private domain implementation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 311 ++----------------------------------
 1 file changed, 11 insertions(+), 300 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 299ce175c096..d86f4626cda5 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -299,27 +299,19 @@ static int hw_pass_through = 1;
 /* si_domain contains mulitple devices */
 #define DOMAIN_FLAG_STATIC_IDENTITY		BIT(0)
 
-/*
- * This is a DMA domain allocated through the iommu domain allocation
- * interface. But one or more devices belonging to this domain have
- * been chosen to use a private domain. We should avoid to use the
- * map/unmap/iova_to_phys APIs on it.
- */
-#define DOMAIN_FLAG_LOSE_CHILDREN		BIT(1)
-
 /*
  * When VT-d works in the scalable mode, it allows DMA translation to
  * happen through either first level or second level page table. This
  * bit marks that the DMA translation for the domain goes through the
  * first level page table, otherwise, it goes through the second level.
  */
-#define DOMAIN_FLAG_USE_FIRST_LEVEL		BIT(2)
+#define DOMAIN_FLAG_USE_FIRST_LEVEL		BIT(1)
 
 /*
  * Domain represents a virtual machine which demands iommu nested
  * translation mode support.
  */
-#define DOMAIN_FLAG_NESTING_MODE		BIT(3)
+#define DOMAIN_FLAG_NESTING_MODE		BIT(2)
 
 #define for_each_domain_iommu(idx, domain)			\
 	for (idx = 0; idx < g_num_of_iommus; idx++)		\
@@ -355,11 +347,6 @@ static void domain_exit(struct dmar_domain *domain);
 static void domain_remove_dev_info(struct dmar_domain *domain);
 static void dmar_remove_one_dev_info(struct device *dev);
 static void __dmar_remove_one_dev_info(struct device_domain_info *info);
-static void domain_context_clear(struct intel_iommu *iommu,
-				 struct device *dev);
-static int domain_detach_iommu(struct dmar_domain *domain,
-			       struct intel_iommu *iommu);
-static bool device_is_rmrr_locked(struct device *dev);
 static int intel_iommu_attach_device(struct iommu_domain *domain,
 				     struct device *dev);
 static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
@@ -1930,65 +1917,6 @@ static inline int guestwidth_to_adjustwidth(int gaw)
 	return agaw;
 }
 
-static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
-		       int guest_width)
-{
-	int adjust_width, agaw;
-	unsigned long sagaw;
-	int ret;
-
-	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN);
-
-	if (!intel_iommu_strict) {
-		ret = init_iova_flush_queue(&domain->iovad,
-					    iommu_flush_iova, iova_entry_free);
-		if (ret)
-			pr_info("iova flush queue initialization failed\n");
-	}
-
-	domain_reserve_special_ranges(domain);
-
-	/* calculate AGAW */
-	if (guest_width > cap_mgaw(iommu->cap))
-		guest_width = cap_mgaw(iommu->cap);
-	domain->gaw = guest_width;
-	adjust_width = guestwidth_to_adjustwidth(guest_width);
-	agaw = width_to_agaw(adjust_width);
-	sagaw = cap_sagaw(iommu->cap);
-	if (!test_bit(agaw, &sagaw)) {
-		/* hardware doesn't support it, choose a bigger one */
-		pr_debug("Hardware doesn't support agaw %d\n", agaw);
-		agaw = find_next_bit(&sagaw, 5, agaw);
-		if (agaw >= 5)
-			return -ENODEV;
-	}
-	domain->agaw = agaw;
-
-	if (ecap_coherent(iommu->ecap))
-		domain->iommu_coherency = 1;
-	else
-		domain->iommu_coherency = 0;
-
-	if (ecap_sc_support(iommu->ecap))
-		domain->iommu_snooping = 1;
-	else
-		domain->iommu_snooping = 0;
-
-	if (intel_iommu_superpage)
-		domain->iommu_superpage = fls(cap_super_page_val(iommu->cap));
-	else
-		domain->iommu_superpage = 0;
-
-	domain->nid = iommu->node;
-
-	/* always allocate the top pgd */
-	domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
-	if (!domain->pgd)
-		return -ENOMEM;
-	__iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
-	return 0;
-}
-
 static void domain_exit(struct dmar_domain *domain)
 {
 
@@ -2704,94 +2632,6 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	return domain;
 }
 
-static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
-{
-	*(u16 *)opaque = alias;
-	return 0;
-}
-
-static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
-{
-	struct device_domain_info *info;
-	struct dmar_domain *domain = NULL;
-	struct intel_iommu *iommu;
-	u16 dma_alias;
-	unsigned long flags;
-	u8 bus, devfn;
-
-	iommu = device_to_iommu(dev, &bus, &devfn);
-	if (!iommu)
-		return NULL;
-
-	if (dev_is_pci(dev)) {
-		struct pci_dev *pdev = to_pci_dev(dev);
-
-		pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
-		spin_lock_irqsave(&device_domain_lock, flags);
-		info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
-						      PCI_BUS_NUM(dma_alias),
-						      dma_alias & 0xff);
-		if (info) {
-			iommu = info->iommu;
-			domain = info->domain;
-		}
-		spin_unlock_irqrestore(&device_domain_lock, flags);
-
-		/* DMA alias already has a domain, use it */
-		if (info)
-			goto out;
-	}
-
-	/* Allocate and initialize new domain for the device */
-	domain = alloc_domain(0);
-	if (!domain)
-		return NULL;
-	if (domain_init(domain, iommu, gaw)) {
-		domain_exit(domain);
-		return NULL;
-	}
-
-out:
-	return domain;
-}
-
-static struct dmar_domain *set_domain_for_dev(struct device *dev,
-					      struct dmar_domain *domain)
-{
-	struct intel_iommu *iommu;
-	struct dmar_domain *tmp;
-	u16 req_id, dma_alias;
-	u8 bus, devfn;
-
-	iommu = device_to_iommu(dev, &bus, &devfn);
-	if (!iommu)
-		return NULL;
-
-	req_id = ((u16)bus << 8) | devfn;
-
-	if (dev_is_pci(dev)) {
-		struct pci_dev *pdev = to_pci_dev(dev);
-
-		pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
-
-		/* register PCI DMA alias device */
-		if (req_id != dma_alias) {
-			tmp = dmar_insert_one_dev_info(iommu, PCI_BUS_NUM(dma_alias),
-					dma_alias & 0xff, NULL, domain);
-
-			if (!tmp || tmp != domain)
-				return tmp;
-		}
-	}
-
-	tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
-	if (!tmp || tmp != domain)
-		return tmp;
-
-	return domain;
-}
-
 static int iommu_domain_identity_map(struct dmar_domain *domain,
 				     unsigned long long start,
 				     unsigned long long end)
@@ -2817,45 +2657,6 @@ static int iommu_domain_identity_map(struct dmar_domain *domain,
 				DMA_PTE_READ|DMA_PTE_WRITE);
 }
 
-static int domain_prepare_identity_map(struct device *dev,
-				       struct dmar_domain *domain,
-				       unsigned long long start,
-				       unsigned long long end)
-{
-	/* For _hardware_ passthrough, don't bother. But for software
-	   passthrough, we do it anyway -- it may indicate a memory
-	   range which is reserved in E820, so which didn't get set
-	   up to start with in si_domain */
-	if (domain == si_domain && hw_pass_through) {
-		dev_warn(dev, "Ignoring identity map for HW passthrough [0x%Lx - 0x%Lx]\n",
-			 start, end);
-		return 0;
-	}
-
-	dev_info(dev, "Setting identity map [0x%Lx - 0x%Lx]\n", start, end);
-
-	if (end < start) {
-		WARN(1, "Your BIOS is broken; RMRR ends before it starts!\n"
-			"BIOS vendor: %s; Ver: %s; Product Version: %s\n",
-			dmi_get_system_info(DMI_BIOS_VENDOR),
-			dmi_get_system_info(DMI_BIOS_VERSION),
-		     dmi_get_system_info(DMI_PRODUCT_VERSION));
-		return -EIO;
-	}
-
-	if (end >> agaw_to_width(domain->agaw)) {
-		WARN(1, "Your BIOS is broken; RMRR exceeds permitted address width (%d bits)\n"
-		     "BIOS vendor: %s; Ver: %s; Product Version: %s\n",
-		     agaw_to_width(domain->agaw),
-		     dmi_get_system_info(DMI_BIOS_VENDOR),
-		     dmi_get_system_info(DMI_BIOS_VERSION),
-		     dmi_get_system_info(DMI_PRODUCT_VERSION));
-		return -EIO;
-	}
-
-	return iommu_domain_identity_map(domain, start, end);
-}
-
 static int md_domain_init(struct dmar_domain *domain, int guest_width);
 
 static int __init si_domain_init(int hw)
@@ -3031,7 +2832,7 @@ static bool device_is_rmrr_locked(struct device *dev)
  *  - IOMMU_DOMAIN_IDENTITY: device requires an identical mapping domain
  *  - 0: both identity and dynamic domains work for this device
  */
-static int device_def_domain_type(struct device *dev)
+static int intel_iommu_dev_def_domain_type(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
@@ -3506,54 +3307,6 @@ static unsigned long intel_alloc_iova(struct device *dev,
 	return iova_pfn;
 }
 
-static struct dmar_domain *get_private_domain_for_dev(struct device *dev)
-{
-	struct dmar_domain *domain, *tmp;
-	struct dmar_rmrr_unit *rmrr;
-	struct device *i_dev;
-	int i, ret;
-
-	/* Device shouldn't be attached by any domains. */
-	domain = find_domain(dev);
-	if (domain)
-		return NULL;
-
-	domain = find_or_alloc_domain(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
-	if (!domain)
-		goto out;
-
-	/* We have a new domain - setup possible RMRRs for the device */
-	rcu_read_lock();
-	for_each_rmrr_units(rmrr) {
-		for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
-					  i, i_dev) {
-			if (i_dev != dev)
-				continue;
-
-			ret = domain_prepare_identity_map(dev, domain,
-							  rmrr->base_address,
-							  rmrr->end_address);
-			if (ret)
-				dev_err(dev, "Mapping reserved region failed\n");
-		}
-	}
-	rcu_read_unlock();
-
-	tmp = set_domain_for_dev(dev, domain);
-	if (!tmp || domain != tmp) {
-		domain_exit(domain);
-		domain = tmp;
-	}
-
-out:
-	if (!domain)
-		dev_err(dev, "Allocating domain failed\n");
-	else
-		domain->domain.type = IOMMU_DOMAIN_DMA;
-
-	return domain;
-}
-
 /* Check if the dev needs to go through non-identity map and unmap process.*/
 static bool iommu_need_mapping(struct device *dev)
 {
@@ -5218,12 +4971,6 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	domain_detach_iommu(domain, iommu);
 	spin_unlock_irqrestore(&iommu->lock, flags);
 
-	/* free the private domain */
-	if (domain->flags & DOMAIN_FLAG_LOSE_CHILDREN &&
-	    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
-	    list_empty(&domain->devices))
-		domain_exit(info->domain);
-
 	free_devinfo_mem(info);
 }
 
@@ -5705,73 +5452,36 @@ static bool intel_iommu_capable(enum iommu_cap cap)
 
 static int intel_iommu_add_device(struct device *dev)
 {
-	struct dmar_domain *dmar_domain;
-	struct iommu_domain *domain;
 	struct intel_iommu *iommu;
 	struct iommu_group *group;
 	u8 bus, devfn;
-	int ret;
 
 	iommu = device_to_iommu(dev, &bus, &devfn);
 	if (!iommu)
 		return -ENODEV;
 
-	iommu_device_link(&iommu->iommu, dev);
-
 	if (translation_pre_enabled(iommu))
 		dev->archdata.iommu = DEFER_DEVICE_DOMAIN_INFO;
 
-	group = iommu_group_get_for_dev(dev);
+	group = iommu_group_get(dev);
+	if (!group) {
+		group = iommu_group_get_for_dev(dev);
+		iommu_device_link(&iommu->iommu, dev);
+	}
 
 	if (IS_ERR(group)) {
-		ret = PTR_ERR(group);
-		goto unlink;
+		iommu_device_unlink(&iommu->iommu, dev);
+		return PTR_ERR(group);
 	}
 
 	iommu_group_put(group);
 
-	domain = iommu_get_domain_for_dev(dev);
-	dmar_domain = to_dmar_domain(domain);
-	if (domain->type == IOMMU_DOMAIN_DMA) {
-		if (device_def_domain_type(dev) == IOMMU_DOMAIN_IDENTITY) {
-			ret = iommu_request_dm_for_dev(dev);
-			if (ret) {
-				dmar_remove_one_dev_info(dev);
-				dmar_domain->flags |= DOMAIN_FLAG_LOSE_CHILDREN;
-				domain_add_dev_info(si_domain, dev);
-				dev_info(dev,
-					 "Device uses a private identity domain.\n");
-			}
-		}
-	} else {
-		if (device_def_domain_type(dev) == IOMMU_DOMAIN_DMA) {
-			ret = iommu_request_dma_domain_for_dev(dev);
-			if (ret) {
-				dmar_remove_one_dev_info(dev);
-				dmar_domain->flags |= DOMAIN_FLAG_LOSE_CHILDREN;
-				if (!get_private_domain_for_dev(dev)) {
-					dev_warn(dev,
-						 "Failed to get a private domain.\n");
-					ret = -ENOMEM;
-					goto unlink;
-				}
-
-				dev_info(dev,
-					 "Device uses a private dma domain.\n");
-			}
-		}
-	}
-
 	if (device_needs_bounce(dev)) {
 		dev_info(dev, "Use Intel IOMMU bounce page dma_ops\n");
 		set_dma_ops(dev, &bounce_dma_ops);
 	}
 
 	return 0;
-
-unlink:
-	iommu_device_unlink(&iommu->iommu, dev);
-	return ret;
 }
 
 static void intel_iommu_remove_device(struct device *dev)
@@ -6131,6 +5841,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.dev_enable_feat	= intel_iommu_dev_enable_feat,
 	.dev_disable_feat	= intel_iommu_dev_disable_feat,
 	.is_attach_deferred	= intel_iommu_is_attach_deferred,
+	.dev_def_domain_type	= intel_iommu_dev_def_domain_type,
 	.pgsize_bitmap		= INTEL_IOMMU_PGSIZES,
 };
 
-- 
2.17.1


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

* [PATCH 6/6] iommu/vt-d: Apply per-device dma_ops
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
                   ` (4 preceding siblings ...)
  2020-03-07  6:20 ` [PATCH 5/6] iommu/vt-d: Add dev_def_domain_type callback Lu Baolu
@ 2020-03-07  6:20 ` Lu Baolu
  2020-03-10 11:15 ` [PATCH 0/6] Replace private domain with per-group default Joerg Roedel
  6 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-07  6:20 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig, Lu Baolu

Current Intel IOMMU driver sets the system level dma_ops. This
causes each dma API to go through the IOMMU driver even the
devices are using identity mapped domains. This sets per-device
dma_ops only if a device is using a DMA domain. Otherwise, use
the default system level dma_ops for direct dma.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 77 ++++++++++---------------------------
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d86f4626cda5..7c7720b5784a 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2712,17 +2712,6 @@ static int __init si_domain_init(int hw)
 	return 0;
 }
 
-static int identity_mapping(struct device *dev)
-{
-	struct device_domain_info *info;
-
-	info = dev->archdata.iommu;
-	if (info)
-		return (info->domain == si_domain);
-
-	return 0;
-}
-
 static int domain_add_dev_info(struct dmar_domain *domain, struct device *dev)
 {
 	struct dmar_domain *ndomain;
@@ -3307,18 +3296,6 @@ static unsigned long intel_alloc_iova(struct device *dev,
 	return iova_pfn;
 }
 
-/* Check if the dev needs to go through non-identity map and unmap process.*/
-static bool iommu_need_mapping(struct device *dev)
-{
-	if (iommu_dummy(dev))
-		return false;
-
-	if (unlikely(attach_deferred(dev)))
-		do_deferred_attach(dev);
-
-	return !identity_mapping(dev);
-}
-
 static dma_addr_t __intel_map_single(struct device *dev, phys_addr_t paddr,
 				     size_t size, int dir, u64 dma_mask)
 {
@@ -3332,6 +3309,9 @@ static dma_addr_t __intel_map_single(struct device *dev, phys_addr_t paddr,
 
 	BUG_ON(dir == DMA_NONE);
 
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
+
 	domain = find_domain(dev);
 	if (!domain)
 		return DMA_MAPPING_ERROR;
@@ -3383,20 +3363,15 @@ static dma_addr_t intel_map_page(struct device *dev, struct page *page,
 				 enum dma_data_direction dir,
 				 unsigned long attrs)
 {
-	if (iommu_need_mapping(dev))
-		return __intel_map_single(dev, page_to_phys(page) + offset,
-				size, dir, *dev->dma_mask);
-	return dma_direct_map_page(dev, page, offset, size, dir, attrs);
+	return __intel_map_single(dev, page_to_phys(page) + offset,
+				  size, dir, *dev->dma_mask);
 }
 
 static dma_addr_t intel_map_resource(struct device *dev, phys_addr_t phys_addr,
 				     size_t size, enum dma_data_direction dir,
 				     unsigned long attrs)
 {
-	if (iommu_need_mapping(dev))
-		return __intel_map_single(dev, phys_addr, size, dir,
-				*dev->dma_mask);
-	return dma_direct_map_resource(dev, phys_addr, size, dir, attrs);
+	return __intel_map_single(dev, phys_addr, size, dir, *dev->dma_mask);
 }
 
 static void intel_unmap(struct device *dev, dma_addr_t dev_addr, size_t size)
@@ -3447,17 +3422,13 @@ static void intel_unmap_page(struct device *dev, dma_addr_t dev_addr,
 			     size_t size, enum dma_data_direction dir,
 			     unsigned long attrs)
 {
-	if (iommu_need_mapping(dev))
-		intel_unmap(dev, dev_addr, size);
-	else
-		dma_direct_unmap_page(dev, dev_addr, size, dir, attrs);
+	intel_unmap(dev, dev_addr, size);
 }
 
 static void intel_unmap_resource(struct device *dev, dma_addr_t dev_addr,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
-	if (iommu_need_mapping(dev))
-		intel_unmap(dev, dev_addr, size);
+	intel_unmap(dev, dev_addr, size);
 }
 
 static void *intel_alloc_coherent(struct device *dev, size_t size,
@@ -3467,8 +3438,8 @@ static void *intel_alloc_coherent(struct device *dev, size_t size,
 	struct page *page = NULL;
 	int order;
 
-	if (!iommu_need_mapping(dev))
-		return dma_direct_alloc(dev, size, dma_handle, flags, attrs);
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
 
 	size = PAGE_ALIGN(size);
 	order = get_order(size);
@@ -3503,9 +3474,6 @@ static void intel_free_coherent(struct device *dev, size_t size, void *vaddr,
 	int order;
 	struct page *page = virt_to_page(vaddr);
 
-	if (!iommu_need_mapping(dev))
-		return dma_direct_free(dev, size, vaddr, dma_handle, attrs);
-
 	size = PAGE_ALIGN(size);
 	order = get_order(size);
 
@@ -3523,9 +3491,6 @@ static void intel_unmap_sg(struct device *dev, struct scatterlist *sglist,
 	struct scatterlist *sg;
 	int i;
 
-	if (!iommu_need_mapping(dev))
-		return dma_direct_unmap_sg(dev, sglist, nelems, dir, attrs);
-
 	for_each_sg(sglist, sg, nelems, i) {
 		nrpages += aligned_nrpages(sg_dma_address(sg), sg_dma_len(sg));
 	}
@@ -3549,8 +3514,9 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
 	struct intel_iommu *iommu;
 
 	BUG_ON(dir == DMA_NONE);
-	if (!iommu_need_mapping(dev))
-		return dma_direct_map_sg(dev, sglist, nelems, dir, attrs);
+
+	if (unlikely(attach_deferred(dev)))
+		do_deferred_attach(dev);
 
 	domain = find_domain(dev);
 	if (!domain)
@@ -3597,8 +3563,6 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
 
 static u64 intel_get_required_mask(struct device *dev)
 {
-	if (!iommu_need_mapping(dev))
-		return dma_direct_get_required_mask(dev);
 	return DMA_BIT_MASK(32);
 }
 
@@ -4873,8 +4837,6 @@ int __init intel_iommu_init(void)
 	}
 	up_write(&dmar_global_lock);
 
-	dma_ops = &intel_dma_ops;
-
 	init_iommu_pm_ops();
 
 	for_each_active_iommu(iommu, drhd) {
@@ -5452,6 +5414,7 @@ static bool intel_iommu_capable(enum iommu_cap cap)
 
 static int intel_iommu_add_device(struct device *dev)
 {
+	struct iommu_domain *domain;
 	struct intel_iommu *iommu;
 	struct iommu_group *group;
 	u8 bus, devfn;
@@ -5476,10 +5439,13 @@ static int intel_iommu_add_device(struct device *dev)
 
 	iommu_group_put(group);
 
-	if (device_needs_bounce(dev)) {
-		dev_info(dev, "Use Intel IOMMU bounce page dma_ops\n");
+	domain = iommu_get_domain_for_dev(dev);
+	if (device_needs_bounce(dev))
 		set_dma_ops(dev, &bounce_dma_ops);
-	}
+	else if (domain && domain->type == IOMMU_DOMAIN_DMA)
+		set_dma_ops(dev, &intel_dma_ops);
+	else
+		set_dma_ops(dev, NULL);
 
 	return 0;
 }
@@ -5499,8 +5465,7 @@ static void intel_iommu_remove_device(struct device *dev)
 
 	iommu_device_unlink(&iommu->iommu, dev);
 
-	if (device_needs_bounce(dev))
-		set_dma_ops(dev, NULL);
+	set_dma_ops(dev, NULL);
 }
 
 static void intel_iommu_get_resv_regions(struct device *device,
-- 
2.17.1


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

* Re: [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops
  2020-03-07  6:20 ` [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops Lu Baolu
@ 2020-03-07 14:18   ` Christoph Hellwig
  2020-03-08  2:08     ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2020-03-07 14:18 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Joerg Roedel, ashok.raj, jacob.jun.pan, kevin.tian, iommu,
	linux-kernel, Daniel Drake, Derrick Jonathan, Jerry Snitselaar,
	Robin Murphy, Christoph Hellwig, Sai Praneeth Prakhya

Do we really need the dev_ prefix in the method name?  Shouldn't the
struct device parameter be hint enough?

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

* Re: [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain
  2020-03-07  6:20 ` [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain Lu Baolu
@ 2020-03-07 14:21   ` Christoph Hellwig
  2020-03-08  2:15     ` Lu Baolu
  2020-03-10 10:58     ` Joerg Roedel
  0 siblings, 2 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-03-07 14:21 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Joerg Roedel, ashok.raj, jacob.jun.pan, kevin.tian, iommu,
	linux-kernel, Daniel Drake, Derrick Jonathan, Jerry Snitselaar,
	Robin Murphy, Christoph Hellwig

On Sat, Mar 07, 2020 at 02:20:11PM +0800, Lu Baolu wrote:
> Currently, if a 32bit device initially uses an identity domain,
> Intel IOMMU driver will convert it forcibly to a DMA one if its
> address capability is not enough for the whole system memory.
> The motivation was to overcome the overhead caused by possible
> bounced buffer.
> 
> Unfortunately, this improvement has led to many problems. For
> example, some 32bit devices are required to use an identity
> domain, forcing them to use DMA domain will cause the device
> not to work anymore. On the other hand, the VMD sub-devices
> share a domain but each sub-device might have different address
> capability. Forcing a VMD sub-device to use DMA domain blindly
> will impact the operation of other sub-devices without any
> notification. Further more, PCI aliased devices (PCI bridge
> and all devices beneath it, VMD devices and various devices
> quirked with pci_add_dma_alias()) must use the same domain.
> Forcing one device to switch to DMA domain during runtime
> will cause in-fligh DMAs for other devices to abort or target
> to other memory which might cause undefind system behavior.

I still don't like the idea to enforce either a strict dynamic
IOMMU mapping or an identify mapping mode.

Can we add a new AUTO domain which will allow using the identity
mapping when available?  That somewhat matches the existing x86
default, and also what powerpc does.  I have a series to lift
that bypass mode into the core dma-mapping code that I need
to repost, which I think would be suitable for intel-iommu as well.

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

* Re: [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops
  2020-03-07 14:18   ` Christoph Hellwig
@ 2020-03-08  2:08     ` Lu Baolu
  0 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-08  2:08 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: baolu.lu, Joerg Roedel, ashok.raj, jacob.jun.pan, kevin.tian,
	iommu, linux-kernel, Daniel Drake, Derrick Jonathan,
	Jerry Snitselaar, Robin Murphy, Sai Praneeth Prakhya

Hi Christoph,

Thanks for your review.

On 2020/3/7 22:18, Christoph Hellwig wrote:
> Do we really need the dev_ prefix in the method name?  Shouldn't the
> struct device parameter be hint enough?

Fair enough. Will use def_domain_type().

Best regards,
baolu


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

* Re: [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain
  2020-03-07 14:21   ` Christoph Hellwig
@ 2020-03-08  2:15     ` Lu Baolu
  2020-03-10 10:58     ` Joerg Roedel
  1 sibling, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-08  2:15 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: baolu.lu, Joerg Roedel, ashok.raj, jacob.jun.pan, kevin.tian,
	iommu, linux-kernel, Daniel Drake, Derrick Jonathan,
	Jerry Snitselaar, Robin Murphy

Hi Christoph,

On 2020/3/7 22:21, Christoph Hellwig wrote:
> On Sat, Mar 07, 2020 at 02:20:11PM +0800, Lu Baolu wrote:
>> Currently, if a 32bit device initially uses an identity domain,
>> Intel IOMMU driver will convert it forcibly to a DMA one if its
>> address capability is not enough for the whole system memory.
>> The motivation was to overcome the overhead caused by possible
>> bounced buffer.
>>
>> Unfortunately, this improvement has led to many problems. For
>> example, some 32bit devices are required to use an identity
>> domain, forcing them to use DMA domain will cause the device
>> not to work anymore. On the other hand, the VMD sub-devices
>> share a domain but each sub-device might have different address
>> capability. Forcing a VMD sub-device to use DMA domain blindly
>> will impact the operation of other sub-devices without any
>> notification. Further more, PCI aliased devices (PCI bridge
>> and all devices beneath it, VMD devices and various devices
>> quirked with pci_add_dma_alias()) must use the same domain.
>> Forcing one device to switch to DMA domain during runtime
>> will cause in-fligh DMAs for other devices to abort or target
>> to other memory which might cause undefind system behavior.
> 
> I still don't like the idea to enforce either a strict dynamic
> IOMMU mapping or an identify mapping mode.
> 
> Can we add a new AUTO domain which will allow using the identity
> mapping when available?  That somewhat matches the existing x86
> default, and also what powerpc does.

Sai is proposing a series to change the default domain through sysfs
during runtime.

https://lore.kernel.org/linux-iommu/FFF73D592F13FD46B8700F0A279B802F4FBF7E4B@ORSMSX114.amr.corp.intel.com/T/#mb919da5567da7692ee7058a00a137145adf950a1

It has evolved into v2. Not sure whether it's what you want.

> I have a series to lift
> that bypass mode into the core dma-mapping code that I need
> to repost, which I think would be suitable for intel-iommu as well.
> 

Looking forward to your repost.

Best regards,
baolu

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

* Re: [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain
  2020-03-07 14:21   ` Christoph Hellwig
  2020-03-08  2:15     ` Lu Baolu
@ 2020-03-10 10:58     ` Joerg Roedel
  1 sibling, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2020-03-10 10:58 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Lu Baolu, ashok.raj, jacob.jun.pan, kevin.tian, iommu,
	linux-kernel, Daniel Drake, Derrick Jonathan, Jerry Snitselaar,
	Robin Murphy

On Sat, Mar 07, 2020 at 03:21:44PM +0100, Christoph Hellwig wrote:
> Can we add a new AUTO domain which will allow using the identity
> mapping when available?  That somewhat matches the existing x86
> default, and also what powerpc does.  I have a series to lift
> that bypass mode into the core dma-mapping code that I need
> to repost, which I think would be suitable for intel-iommu as well.

Please Cc me on that series when you re-post it.


Thanks,

	Joerg

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

* Re: [PATCH 0/6] Replace private domain with per-group default
  2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
                   ` (5 preceding siblings ...)
  2020-03-07  6:20 ` [PATCH 6/6] iommu/vt-d: Apply per-device dma_ops Lu Baolu
@ 2020-03-10 11:15 ` Joerg Roedel
  2020-03-11  6:50   ` Lu Baolu
  6 siblings, 1 reply; 16+ messages in thread
From: Joerg Roedel @ 2020-03-10 11:15 UTC (permalink / raw)
  To: Lu Baolu
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig

Hi Baolu,

On Sat, Mar 07, 2020 at 02:20:08PM +0800, Lu Baolu wrote:
> Lu Baolu (5):
>   iommu: Configure default domain with dev_def_domain_type
>   iommu/vt-d: Don't force 32bit devices to uses DMA domain
>   iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
>   iommu/vt-d: Add dev_def_domain_type callback
>   iommu/vt-d: Apply per-device dma_ops
> 
> Sai Praneeth Prakhya (1):
>   iommu: Add dev_def_domain_type() callback in iommu_ops

I like this patch-set, but I fear some regressions from patch
"iommu/vt-d: Don't force 32bit devices to uses DMA domain". Have you
tested this series on a couple of machines, ideally even older ones from
the first generation of VT-d hardware?

Regards,

	Joerg

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

* Re: [PATCH 0/6] Replace private domain with per-group default
  2020-03-10 11:15 ` [PATCH 0/6] Replace private domain with per-group default Joerg Roedel
@ 2020-03-11  6:50   ` Lu Baolu
  2020-03-13 13:36     ` Joerg Roedel
  0 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2020-03-11  6:50 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: baolu.lu, ashok.raj, jacob.jun.pan, kevin.tian, iommu,
	linux-kernel, Daniel Drake, Derrick Jonathan, Jerry Snitselaar,
	Robin Murphy, Christoph Hellwig

Hi Joerg,

On 2020/3/10 19:15, Joerg Roedel wrote:
> Hi Baolu,
> 
> On Sat, Mar 07, 2020 at 02:20:08PM +0800, Lu Baolu wrote:
>> Lu Baolu (5):
>>    iommu: Configure default domain with dev_def_domain_type
>>    iommu/vt-d: Don't force 32bit devices to uses DMA domain
>>    iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
>>    iommu/vt-d: Add dev_def_domain_type callback
>>    iommu/vt-d: Apply per-device dma_ops
>>
>> Sai Praneeth Prakhya (1):
>>    iommu: Add dev_def_domain_type() callback in iommu_ops
> 
> I like this patch-set, but I fear some regressions from patch
> "iommu/vt-d: Don't force 32bit devices to uses DMA domain". Have you
> tested this series on a couple of machines, ideally even older ones from
> the first generation of VT-d hardware?

The oldest hardware I have is Ivy Bridge. :-) Actually, The effect of
using identity domain for 32-bit devices is the same as that of adding
intel_iommu=off in the kernel parameter. Hence, if there is any
regression, people should also find it with intel_iommu=off.
intel_iommu=off support is added at the very beginning of VT-d driver.

Best regards,
baolu

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

* Re: [PATCH 0/6] Replace private domain with per-group default
  2020-03-11  6:50   ` Lu Baolu
@ 2020-03-13 13:36     ` Joerg Roedel
  2020-03-14  1:13       ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Joerg Roedel @ 2020-03-13 13:36 UTC (permalink / raw)
  To: Lu Baolu
  Cc: ashok.raj, jacob.jun.pan, kevin.tian, iommu, linux-kernel,
	Daniel Drake, Derrick Jonathan, Jerry Snitselaar, Robin Murphy,
	Christoph Hellwig

On Wed, Mar 11, 2020 at 02:50:39PM +0800, Lu Baolu wrote:
> On 2020/3/10 19:15, Joerg Roedel wrote:
> > Hi Baolu,
> > 
> > On Sat, Mar 07, 2020 at 02:20:08PM +0800, Lu Baolu wrote:
> > > Lu Baolu (5):
> > >    iommu: Configure default domain with dev_def_domain_type
> > >    iommu/vt-d: Don't force 32bit devices to uses DMA domain
> > >    iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
> > >    iommu/vt-d: Add dev_def_domain_type callback
> > >    iommu/vt-d: Apply per-device dma_ops
> > > 
> > > Sai Praneeth Prakhya (1):
> > >    iommu: Add dev_def_domain_type() callback in iommu_ops
> > 
> > I like this patch-set, but I fear some regressions from patch
> > "iommu/vt-d: Don't force 32bit devices to uses DMA domain". Have you
> > tested this series on a couple of machines, ideally even older ones from
> > the first generation of VT-d hardware?
> 
> The oldest hardware I have is Ivy Bridge. :-) Actually, The effect of
> using identity domain for 32-bit devices is the same as that of adding
> intel_iommu=off in the kernel parameter. Hence, if there is any
> regression, people should also find it with intel_iommu=off.
> intel_iommu=off support is added at the very beginning of VT-d driver.

Okay, I will also do some testing on it, one of my VT-d machines is a
Haswell. Please send a new version with the recent comments addressed.

Regards,

	Joerg

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

* Re: [PATCH 0/6] Replace private domain with per-group default
  2020-03-13 13:36     ` Joerg Roedel
@ 2020-03-14  1:13       ` Lu Baolu
  0 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2020-03-14  1:13 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: baolu.lu, ashok.raj, jacob.jun.pan, kevin.tian, iommu,
	linux-kernel, Daniel Drake, Derrick Jonathan, Jerry Snitselaar,
	Robin Murphy, Christoph Hellwig

On 2020/3/13 21:36, Joerg Roedel wrote:
> On Wed, Mar 11, 2020 at 02:50:39PM +0800, Lu Baolu wrote:
>> On 2020/3/10 19:15, Joerg Roedel wrote:
>>> Hi Baolu,
>>>
>>> On Sat, Mar 07, 2020 at 02:20:08PM +0800, Lu Baolu wrote:
>>>> Lu Baolu (5):
>>>>     iommu: Configure default domain with dev_def_domain_type
>>>>     iommu/vt-d: Don't force 32bit devices to uses DMA domain
>>>>     iommu/vt-d: Don't force PCI sub-hierarchy to use DMA domain
>>>>     iommu/vt-d: Add dev_def_domain_type callback
>>>>     iommu/vt-d: Apply per-device dma_ops
>>>>
>>>> Sai Praneeth Prakhya (1):
>>>>     iommu: Add dev_def_domain_type() callback in iommu_ops
>>>
>>> I like this patch-set, but I fear some regressions from patch
>>> "iommu/vt-d: Don't force 32bit devices to uses DMA domain". Have you
>>> tested this series on a couple of machines, ideally even older ones from
>>> the first generation of VT-d hardware?
>>
>> The oldest hardware I have is Ivy Bridge. :-) Actually, The effect of
>> using identity domain for 32-bit devices is the same as that of adding
>> intel_iommu=off in the kernel parameter. Hence, if there is any
>> regression, people should also find it with intel_iommu=off.
>> intel_iommu=off support is added at the very beginning of VT-d driver.
> 
> Okay, I will also do some testing on it, one of my VT-d machines is a
> Haswell. Please send a new version with the recent comments addressed.

Sure.

Best regards,
baolu

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

end of thread, other threads:[~2020-03-14  1:13 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-07  6:20 [PATCH 0/6] Replace private domain with per-group default Lu Baolu
2020-03-07  6:20 ` [PATCH 1/6] iommu: Add dev_def_domain_type() callback in iommu_ops Lu Baolu
2020-03-07 14:18   ` Christoph Hellwig
2020-03-08  2:08     ` Lu Baolu
2020-03-07  6:20 ` [PATCH 2/6] iommu: Configure default domain with dev_def_domain_type Lu Baolu
2020-03-07  6:20 ` [PATCH 3/6] iommu/vt-d: Don't force 32bit devices to uses DMA domain Lu Baolu
2020-03-07 14:21   ` Christoph Hellwig
2020-03-08  2:15     ` Lu Baolu
2020-03-10 10:58     ` Joerg Roedel
2020-03-07  6:20 ` [PATCH 4/6] iommu/vt-d: Don't force PCI sub-hierarchy to use " Lu Baolu
2020-03-07  6:20 ` [PATCH 5/6] iommu/vt-d: Add dev_def_domain_type callback Lu Baolu
2020-03-07  6:20 ` [PATCH 6/6] iommu/vt-d: Apply per-device dma_ops Lu Baolu
2020-03-10 11:15 ` [PATCH 0/6] Replace private domain with per-group default Joerg Roedel
2020-03-11  6:50   ` Lu Baolu
2020-03-13 13:36     ` Joerg Roedel
2020-03-14  1:13       ` Lu Baolu

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