[PATCH v3 0/7] iommu/vt-d: Convert to use static identity domain

[PATCH v3 0/7] iommu/vt-d: Convert to use static identity domain

[Lu Baolu]

Intel's IOMMU driver used a special domain called 1:1 mapping domain to
support the domain of type IOMMU_DOMAIN_IDENTITY, which enables device
drivers to directly utilize physical addresses for DMA access despite
the presence of IOMMU units.

The implementation of the 1:1 mapping domain is influenced by hardware
differences. While modern Intel VT-d implementations support hardware
passthrough translation mode, earlier versions lacked this feature,
which requires a more complex implementation approach.

The 1:1 mapping domain for earlier hardware was implemented by associating
a DMA domain with an IOVA (IO Virtual Address) equivalent to the
physical address. While, for most hardware supporting passthrough mode,
simply setting the hardware's passthrough mode is sufficient. These two
modes were merged together in si_domain, which is a special DMA domain
sharing the domain ops of an ordinary DMA domain.

As the iommu core has evolved, it has introduced global static identity
domain with "never fail" attach semantics. This means that the domain is
always available and cannot fail to attach. The iommu driver now assigns
this domain directly at iommu_ops->identity_domain instead of allocating
it through the domain allocation interface.

This converts the Intel IOMMU driver to embrace the global static
identity domain. For early legacy hardwares that don't support
passthrough translation mode, ask the iommu core to use a DMA type of
default domain. For modern hardwares that support passthrough
translation mode, implement a static global identity domain.

The whole series is also available at

https://github.com/LuBaolu/intel-iommu/commits/vtd-static-identity-domain-v3

Change log:
v3:
 - Kevin worried that some graphic devices might still require identity
   domain. Forcing DMA domain for those drivers might break the existing
   functionality.
   https://lore.kernel.org/linux-iommu/BN9PR11MB52761FF9AB496B422596DDDF8C8AA@BN9PR11MB5276.namprd11.prod.outlook.com/

   After confirmed with the graphic community, we decouple "igfx_off"
   kernel command from graphic identity mapping with the following commits:
   ba00196ca41c ("iommu/vt-d: Decouple igfx_off from graphic identity mapping")
   4b8d18c0c986 ("iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA").

v2: https://lore.kernel.org/linux-iommu/20231205012203.244584-1-baolu.lu@linux.intel.com/
 - Re-orgnize the patches by removing 1:1 mappings before implementing
   global static domain.

v1: https://lore.kernel.org/linux-iommu/20231120112944.142741-1-baolu.lu@linux.intel.com/ 

Lu Baolu (7):
  iommu/vt-d: Require DMA domain if hardware not support passthrough
  iommu/vt-d: Remove identity mappings from si_domain
  iommu/vt-d: Always reserve a domain ID for identity setup
  iommu/vt-d: Prepare for global static identity domain
  iommu/vt-d: Factor out helpers from domain_context_mapping_one()
  iommu/vt-d: Add support for static identity domain
  iommu/vt-d: Cleanup si_domain

 drivers/iommu/intel/iommu.c | 434 +++++++++++++++++-------------------
 1 file changed, 201 insertions(+), 233 deletions(-)

-- 
2.34.1

[PATCH v3 1/7] iommu/vt-d: Require DMA domain if hardware not support passthrough

[Lu Baolu]

The iommu core defines the def_domain_type callback to query the iommu
driver about hardware capability and quirks. The iommu driver should
declare IOMMU_DOMAIN_DMA requirement for hardware lacking pass-through
capability.

Earlier VT-d hardware implementations did not support pass-through
translation mode. The iommu driver relied on a paging domain with all
physical system memory addresses identically mapped to the same IOVA
to simulate pass-through translation before the def_domain_type was
introduced and it has been kept until now. It's time to adjust it now
to make the Intel iommu driver follow the def_domain_type semantics.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9ff8b83c19a3..90ad794a1be7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2149,6 +2149,16 @@ static bool device_rmrr_is_relaxable(struct device *dev)
 
 static int device_def_domain_type(struct device *dev)
 {
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
+	/*
+	 * Hardware does not support the passthrough translation mode.
+	 * Always use a dynamaic mapping domain.
+	 */
+	if (!ecap_pass_through(iommu->ecap))
+		return IOMMU_DOMAIN_DMA;
+
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
 
-- 
2.34.1

[PATCH v3 2/7] iommu/vt-d: Remove identity mappings from si_domain

[Lu Baolu]

As the driver has enforced DMA domains for devices managed by an IOMMU
hardware that doesn't support passthrough translation mode, there is no
need for static identity mappings in the si_domain. Remove the identity
mapping code to avoid dead code.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 90ad794a1be7..723ea9f3f501 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -167,14 +167,7 @@ static void device_rbtree_remove(struct device_domain_info *info)
 	spin_unlock_irqrestore(&iommu->device_rbtree_lock, flags);
 }
 
-/*
- * This domain is a statically identity mapping domain.
- *	1. This domain creats a static 1:1 mapping to all usable memory.
- * 	2. It maps to each iommu if successful.
- *	3. Each iommu mapps to this domain if successful.
- */
 static struct dmar_domain *si_domain;
-static int hw_pass_through = 1;
 
 struct dmar_rmrr_unit {
 	struct list_head list;		/* list of rmrr units	*/
@@ -1647,7 +1640,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	struct context_entry *context;
 	int agaw, ret;
 
-	if (hw_pass_through && domain_type_is_si(domain))
+	if (domain_type_is_si(domain))
 		translation = CONTEXT_TT_PASS_THROUGH;
 
 	pr_debug("Set context mapping for %02x:%02x.%d\n",
@@ -1998,29 +1991,10 @@ static bool dev_is_real_dma_subdevice(struct device *dev)
 	       pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev);
 }
 
-static int iommu_domain_identity_map(struct dmar_domain *domain,
-				     unsigned long first_vpfn,
-				     unsigned long last_vpfn)
-{
-	/*
-	 * RMRR range might have overlap with physical memory range,
-	 * clear it first
-	 */
-	dma_pte_clear_range(domain, first_vpfn, last_vpfn);
-
-	return __domain_mapping(domain, first_vpfn,
-				first_vpfn, last_vpfn - first_vpfn + 1,
-				DMA_PTE_READ|DMA_PTE_WRITE, GFP_KERNEL);
-}
-
 static int md_domain_init(struct dmar_domain *domain, int guest_width);
 
-static int __init si_domain_init(int hw)
+static int __init si_domain_init(void)
 {
-	struct dmar_rmrr_unit *rmrr;
-	struct device *dev;
-	int i, nid, ret;
-
 	si_domain = alloc_domain(IOMMU_DOMAIN_IDENTITY);
 	if (!si_domain)
 		return -EFAULT;
@@ -2031,44 +2005,6 @@ static int __init si_domain_init(int hw)
 		return -EFAULT;
 	}
 
-	if (hw)
-		return 0;
-
-	for_each_online_node(nid) {
-		unsigned long start_pfn, end_pfn;
-		int i;
-
-		for_each_mem_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
-			ret = iommu_domain_identity_map(si_domain,
-					mm_to_dma_pfn_start(start_pfn),
-					mm_to_dma_pfn_end(end_pfn-1));
-			if (ret)
-				return ret;
-		}
-	}
-
-	/*
-	 * Identity map the RMRRs so that devices with RMRRs could also use
-	 * the si_domain.
-	 */
-	for_each_rmrr_units(rmrr) {
-		for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
-					  i, dev) {
-			unsigned long long start = rmrr->base_address;
-			unsigned long long end = rmrr->end_address;
-
-			if (WARN_ON(end < start ||
-				    end >> agaw_to_width(si_domain->agaw)))
-				continue;
-
-			ret = iommu_domain_identity_map(si_domain,
-					mm_to_dma_pfn_start(start >> PAGE_SHIFT),
-					mm_to_dma_pfn_end(end >> PAGE_SHIFT));
-			if (ret)
-				return ret;
-		}
-	}
-
 	return 0;
 }
 
@@ -2094,7 +2030,7 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 
 	if (!sm_supported(iommu))
 		ret = domain_context_mapping(domain, dev);
-	else if (hw_pass_through && domain_type_is_si(domain))
+	else if (domain_type_is_si(domain))
 		ret = intel_pasid_setup_pass_through(iommu, dev, IOMMU_NO_PASID);
 	else if (domain->use_first_level)
 		ret = domain_setup_first_level(iommu, domain, dev, IOMMU_NO_PASID);
@@ -2449,8 +2385,6 @@ static int __init init_dmars(void)
 			}
 		}
 
-		if (!ecap_pass_through(iommu->ecap))
-			hw_pass_through = 0;
 		intel_svm_check(iommu);
 	}
 
@@ -2466,7 +2400,7 @@ static int __init init_dmars(void)
 
 	check_tylersburg_isoch();
 
-	ret = si_domain_init(hw_pass_through);
+	ret = si_domain_init();
 	if (ret)
 		goto free_iommu;
 
@@ -2893,12 +2827,6 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 	if (ret)
 		goto out;
 
-	if (hw_pass_through && !ecap_pass_through(iommu->ecap)) {
-		pr_warn("%s: Doesn't support hardware pass through.\n",
-			iommu->name);
-		return -ENXIO;
-	}
-
 	sp = domain_update_iommu_superpage(NULL, iommu) - 1;
 	if (sp >= 0 && !(cap_super_page_val(iommu->cap) & (1 << sp))) {
 		pr_warn("%s: Doesn't support large page.\n",
@@ -3149,43 +3077,6 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
 	return 0;
 }
 
-static int intel_iommu_memory_notifier(struct notifier_block *nb,
-				       unsigned long val, void *v)
-{
-	struct memory_notify *mhp = v;
-	unsigned long start_vpfn = mm_to_dma_pfn_start(mhp->start_pfn);
-	unsigned long last_vpfn = mm_to_dma_pfn_end(mhp->start_pfn +
-			mhp->nr_pages - 1);
-
-	switch (val) {
-	case MEM_GOING_ONLINE:
-		if (iommu_domain_identity_map(si_domain,
-					      start_vpfn, last_vpfn)) {
-			pr_warn("Failed to build identity map for [%lx-%lx]\n",
-				start_vpfn, last_vpfn);
-			return NOTIFY_BAD;
-		}
-		break;
-
-	case MEM_OFFLINE:
-	case MEM_CANCEL_ONLINE:
-		{
-			LIST_HEAD(freelist);
-
-			domain_unmap(si_domain, start_vpfn, last_vpfn, &freelist);
-			iommu_put_pages_list(&freelist);
-		}
-		break;
-	}
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block intel_iommu_memory_nb = {
-	.notifier_call = intel_iommu_memory_notifier,
-	.priority = 0
-};
-
 static void intel_disable_iommus(void)
 {
 	struct intel_iommu *iommu = NULL;
@@ -3482,12 +3373,7 @@ int __init intel_iommu_init(void)
 
 		iommu_pmu_register(iommu);
 	}
-	up_read(&dmar_global_lock);
 
-	if (si_domain && !hw_pass_through)
-		register_memory_notifier(&intel_iommu_memory_nb);
-
-	down_read(&dmar_global_lock);
 	if (probe_acpi_namespace_devices())
 		pr_warn("ACPI name space devices didn't probe correctly\n");
 
-- 
2.34.1

[PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Lu Baolu]

We will use a global static identity domain. Reserve a static domain ID
for it.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 723ea9f3f501..c019fb3b3e78 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
 	 * entry for first-level or pass-through translation modes should
 	 * be programmed with a domain id different from those used for
 	 * second-level or nested translation. We reserve a domain id for
-	 * this purpose.
+	 * this purpose. This domain id is also used for identity domain
+	 * in legacy mode.
 	 */
-	if (sm_supported(iommu))
-		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
+	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

[Lu Baolu]

With the static identity domain in place, the domain field of per-device
iommu driver data can be either a pointer to a DMA translation domain, or
NULL, indicating that the static identity domain is attached. Refactor
the code to prepare for this change.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c019fb3b3e78..f37c8c3cba3c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
 	bool has_iotlb_device = false;
 	unsigned long flags;
 
+	if (!domain)
+		return;
+
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(info, &domain->devices, link) {
 		if (info->ats_enabled) {
@@ -3706,11 +3709,9 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
 static int intel_iommu_attach_device(struct iommu_domain *domain,
 				     struct device *dev)
 {
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	int ret;
 
-	if (info->domain)
-		device_block_translation(dev);
+	device_block_translation(dev);
 
 	ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
@@ -4321,6 +4322,11 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	struct intel_iommu *iommu = info->iommu;
 	unsigned long flags;
 
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+		return;
+	}
+
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
 		if (curr->dev == dev && curr->pasid == pasid) {
-- 
2.34.1

[PATCH v3 5/7] iommu/vt-d: Factor out helpers from domain_context_mapping_one()

[Lu Baolu]

Extract common code from domain_context_mapping_one() into new helpers,
making it reusable by other functions such as the upcoming identity domain
implementation. No intentional functional changes.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f37c8c3cba3c..2ac56e2355e1 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1631,6 +1631,61 @@ static void domain_exit(struct dmar_domain *domain)
 	kfree(domain);
 }
 
+/*
+ * For kdump cases, old valid entries may be cached due to the
+ * in-flight DMA and copied pgtable, but there is no unmapping
+ * behaviour for them, thus we need an explicit cache flush for
+ * the newly-mapped device. For kdump, at this point, the device
+ * is supposed to finish reset at its driver probe stage, so no
+ * in-flight DMA will exist, and we don't need to worry anymore
+ * hereafter.
+ */
+static void copied_context_tear_down(struct intel_iommu *iommu,
+				     struct context_entry *context,
+				     u8 bus, u8 devfn)
+{
+	u16 did_old;
+
+	if (!context_copied(iommu, bus, devfn))
+		return;
+
+	assert_spin_locked(&iommu->lock);
+
+	did_old = context_domain_id(context);
+	context_clear_entry(context);
+
+	if (did_old < cap_ndoms(iommu->cap)) {
+		iommu->flush.flush_context(iommu, did_old,
+					   (((u16)bus) << 8) | devfn,
+					   DMA_CCMD_MASK_NOBIT,
+					   DMA_CCMD_DEVICE_INVL);
+		iommu->flush.flush_iotlb(iommu, did_old, 0, 0,
+					 DMA_TLB_DSI_FLUSH);
+	}
+
+	clear_context_copied(iommu, bus, devfn);
+}
+
+/*
+ * It's a non-present to present mapping. If hardware doesn't cache
+ * non-present entry we only need to flush the write-buffer. If the
+ * _does_ cache non-present entries, then it does so in the special
+ * domain #0, which we have to flush:
+ */
+static void context_present_cache_flush(struct intel_iommu *iommu, u16 did,
+					u8 bus, u8 devfn)
+{
+	if (cap_caching_mode(iommu->cap)) {
+		iommu->flush.flush_context(iommu, 0,
+					   (((u16)bus) << 8) | devfn,
+					   DMA_CCMD_MASK_NOBIT,
+					   DMA_CCMD_DEVICE_INVL);
+		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
+	} else {
+		iommu_flush_write_buffer(iommu);
+	}
+}
+
 static int domain_context_mapping_one(struct dmar_domain *domain,
 				      struct intel_iommu *iommu,
 				      u8 bus, u8 devfn)
@@ -1659,31 +1714,9 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	if (context_present(context) && !context_copied(iommu, bus, devfn))
 		goto out_unlock;
 
-	/*
-	 * For kdump cases, old valid entries may be cached due to the
-	 * in-flight DMA and copied pgtable, but there is no unmapping
-	 * behaviour for them, thus we need an explicit cache flush for
-	 * the newly-mapped device. For kdump, at this point, the device
-	 * is supposed to finish reset at its driver probe stage, so no
-	 * in-flight DMA will exist, and we don't need to worry anymore
-	 * hereafter.
-	 */
-	if (context_copied(iommu, bus, devfn)) {
-		u16 did_old = context_domain_id(context);
-
-		if (did_old < cap_ndoms(iommu->cap)) {
-			iommu->flush.flush_context(iommu, did_old,
-						   (((u16)bus) << 8) | devfn,
-						   DMA_CCMD_MASK_NOBIT,
-						   DMA_CCMD_DEVICE_INVL);
-			iommu->flush.flush_iotlb(iommu, did_old, 0, 0,
-						 DMA_TLB_DSI_FLUSH);
-		}
-
-		clear_context_copied(iommu, bus, devfn);
-	}
-
+	copied_context_tear_down(iommu, context, bus, devfn);
 	context_clear_entry(context);
+
 	context_set_domain_id(context, did);
 
 	if (translation != CONTEXT_TT_PASS_THROUGH) {
@@ -1719,23 +1752,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	context_set_present(context);
 	if (!ecap_coherent(iommu->ecap))
 		clflush_cache_range(context, sizeof(*context));
-
-	/*
-	 * It's a non-present to present mapping. If hardware doesn't cache
-	 * non-present entry we only need to flush the write-buffer. If the
-	 * _does_ cache non-present entries, then it does so in the special
-	 * domain #0, which we have to flush:
-	 */
-	if (cap_caching_mode(iommu->cap)) {
-		iommu->flush.flush_context(iommu, 0,
-					   (((u16)bus) << 8) | devfn,
-					   DMA_CCMD_MASK_NOBIT,
-					   DMA_CCMD_DEVICE_INVL);
-		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
-	} else {
-		iommu_flush_write_buffer(iommu);
-	}
-
+	context_present_cache_flush(iommu, did, bus, devfn);
 	ret = 0;
 
 out_unlock:
-- 
2.34.1

[PATCH v3 6/7] iommu/vt-d: Add support for static identity domain

[Lu Baolu]

Software determines VT-d hardware support for passthrough translation by
inspecting the capability register. If passthrough translation is not
supported, the device is instructed to use DMA domain for its default
domain.

Add a global static identity domain with guaranteed attach semantics for
IOMMUs that support passthrough translation mode.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2ac56e2355e1..9e7b4159e53f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4570,9 +4570,111 @@ static const struct iommu_dirty_ops intel_dirty_ops = {
 	.read_and_clear_dirty = intel_iommu_read_and_clear_dirty,
 };
 
+static int context_setup_pass_through(struct device *dev, u8 bus, u8 devfn)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	struct context_entry *context;
+
+	spin_lock(&iommu->lock);
+	context = iommu_context_addr(iommu, bus, devfn, 1);
+	if (!context) {
+		spin_unlock(&iommu->lock);
+		return -ENOMEM;
+	}
+
+	if (context_present(context) && !context_copied(iommu, bus, devfn)) {
+		spin_unlock(&iommu->lock);
+		return 0;
+	}
+
+	copied_context_tear_down(iommu, context, bus, devfn);
+	context_clear_entry(context);
+	context_set_domain_id(context, FLPT_DEFAULT_DID);
+
+	/*
+	 * In pass through mode, AW must be programmed to indicate the largest
+	 * AGAW value supported by hardware. And ASR is ignored by hardware.
+	 */
+	context_set_address_width(context, iommu->msagaw);
+	context_set_translation_type(context, CONTEXT_TT_PASS_THROUGH);
+	context_set_fault_enable(context);
+	context_set_present(context);
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(context, sizeof(*context));
+	context_present_cache_flush(iommu, FLPT_DEFAULT_DID, bus, devfn);
+	spin_unlock(&iommu->lock);
+
+	return 0;
+}
+
+static int context_setup_pass_through_cb(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct device *dev = data;
+
+	if (dev != &pdev->dev)
+		return 0;
+
+	return context_setup_pass_through(dev, PCI_BUS_NUM(alias), alias & 0xff);
+}
+
+static int device_setup_pass_through(struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+
+	if (!dev_is_pci(dev))
+		return context_setup_pass_through(dev, info->bus, info->devfn);
+
+	return pci_for_each_dma_alias(to_pci_dev(dev),
+				      context_setup_pass_through_cb, dev);
+}
+
+static int identity_domain_attach_dev(struct iommu_domain *domain, struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	int ret;
+
+	device_block_translation(dev);
+
+	if (dev_is_real_dma_subdevice(dev))
+		return 0;
+
+	if (sm_supported(iommu)) {
+		ret = intel_pasid_setup_pass_through(iommu, dev, IOMMU_NO_PASID);
+		if (!ret)
+			iommu_enable_pci_caps(info);
+	} else {
+		ret = device_setup_pass_through(dev);
+	}
+
+	return ret;
+}
+
+static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
+	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;
+
+	return intel_pasid_setup_pass_through(iommu, dev, pasid);
+}
+
+static struct iommu_domain identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &(const struct iommu_domain_ops) {
+		.attach_dev	= identity_domain_attach_dev,
+		.set_dev_pasid	= identity_domain_set_dev_pasid,
+	},
+};
+
 const struct iommu_ops intel_iommu_ops = {
 	.blocked_domain		= &blocking_domain,
 	.release_domain		= &blocking_domain,
+	.identity_domain	= &identity_domain,
 	.capable		= intel_iommu_capable,
 	.hw_info		= intel_iommu_hw_info,
 	.domain_alloc		= intel_iommu_domain_alloc,
-- 
2.34.1

[PATCH v3 7/7] iommu/vt-d: Cleanup si_domain

[Lu Baolu]

The static identity domain has been introduced, rendering the si_domain
obsolete. Remove si_domain and cleanup the code accordingly.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9e7b4159e53f..8f8dce602c86 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -167,8 +167,6 @@ static void device_rbtree_remove(struct device_domain_info *info)
 	spin_unlock_irqrestore(&iommu->device_rbtree_lock, flags);
 }
 
-static struct dmar_domain *si_domain;
-
 struct dmar_rmrr_unit {
 	struct list_head list;		/* list of rmrr units	*/
 	struct acpi_dmar_header *hdr;	/* ACPI header		*/
@@ -286,11 +284,6 @@ static int __init intel_iommu_setup(char *str)
 }
 __setup("intel_iommu=", intel_iommu_setup);
 
-static int domain_type_is_si(struct dmar_domain *domain)
-{
-	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
-}
-
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -1698,9 +1691,6 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	struct context_entry *context;
 	int agaw, ret;
 
-	if (domain_type_is_si(domain))
-		translation = CONTEXT_TT_PASS_THROUGH;
-
 	pr_debug("Set context mapping for %02x:%02x.%d\n",
 		bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 
@@ -1719,34 +1709,24 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 
 	context_set_domain_id(context, did);
 
-	if (translation != CONTEXT_TT_PASS_THROUGH) {
-		/*
-		 * Skip top levels of page tables for iommu which has
-		 * less agaw than default. Unnecessary for PT mode.
-		 */
-		for (agaw = domain->agaw; agaw > iommu->agaw; agaw--) {
-			ret = -ENOMEM;
-			pgd = phys_to_virt(dma_pte_addr(pgd));
-			if (!dma_pte_present(pgd))
-				goto out_unlock;
-		}
-
-		if (info && info->ats_supported)
-			translation = CONTEXT_TT_DEV_IOTLB;
-		else
-			translation = CONTEXT_TT_MULTI_LEVEL;
-
-		context_set_address_root(context, virt_to_phys(pgd));
-		context_set_address_width(context, agaw);
-	} else {
-		/*
-		 * In pass through mode, AW must be programmed to
-		 * indicate the largest AGAW value supported by
-		 * hardware. And ASR is ignored by hardware.
-		 */
-		context_set_address_width(context, iommu->msagaw);
+	/*
+	 * Skip top levels of page tables for iommu which has
+	 * less agaw than default. Unnecessary for PT mode.
+	 */
+	for (agaw = domain->agaw; agaw > iommu->agaw; agaw--) {
+		ret = -ENOMEM;
+		pgd = phys_to_virt(dma_pte_addr(pgd));
+		if (!dma_pte_present(pgd))
+			goto out_unlock;
 	}
 
+	if (info && info->ats_supported)
+		translation = CONTEXT_TT_DEV_IOTLB;
+	else
+		translation = CONTEXT_TT_MULTI_LEVEL;
+
+	context_set_address_root(context, virt_to_phys(pgd));
+	context_set_address_width(context, agaw);
 	context_set_translation_type(context, translation);
 	context_set_fault_enable(context);
 	context_set_present(context);
@@ -2011,23 +1991,6 @@ static bool dev_is_real_dma_subdevice(struct device *dev)
 	       pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev);
 }
 
-static int md_domain_init(struct dmar_domain *domain, int guest_width);
-
-static int __init si_domain_init(void)
-{
-	si_domain = alloc_domain(IOMMU_DOMAIN_IDENTITY);
-	if (!si_domain)
-		return -EFAULT;
-
-	if (md_domain_init(si_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
-		domain_exit(si_domain);
-		si_domain = NULL;
-		return -EFAULT;
-	}
-
-	return 0;
-}
-
 static int dmar_domain_attach_device(struct dmar_domain *domain,
 				     struct device *dev)
 {
@@ -2050,8 +2013,6 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 
 	if (!sm_supported(iommu))
 		ret = domain_context_mapping(domain, dev);
-	else if (domain_type_is_si(domain))
-		ret = intel_pasid_setup_pass_through(iommu, dev, IOMMU_NO_PASID);
 	else if (domain->use_first_level)
 		ret = domain_setup_first_level(iommu, domain, dev, IOMMU_NO_PASID);
 	else
@@ -2060,8 +2021,7 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 	if (ret)
 		goto out_block_translation;
 
-	if (sm_supported(info->iommu) || !domain_type_is_si(info->domain))
-		iommu_enable_pci_caps(info);
+	iommu_enable_pci_caps(info);
 
 	ret = cache_tag_assign_domain(domain, dev, IOMMU_NO_PASID);
 	if (ret)
@@ -2420,10 +2380,6 @@ static int __init init_dmars(void)
 
 	check_tylersburg_isoch();
 
-	ret = si_domain_init();
-	if (ret)
-		goto free_iommu;
-
 	/*
 	 * for each drhd
 	 *   enable fault log
@@ -2469,10 +2425,6 @@ static int __init init_dmars(void)
 		disable_dmar_iommu(iommu);
 		free_dmar_iommu(iommu);
 	}
-	if (si_domain) {
-		domain_exit(si_domain);
-		si_domain = NULL;
-	}
 
 	return ret;
 }
@@ -3607,8 +3559,6 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
 		domain->geometry.force_aperture = true;
 
 		return domain;
-	case IOMMU_DOMAIN_IDENTITY:
-		return &si_domain->domain;
 	default:
 		return NULL;
 	}
@@ -3675,8 +3625,7 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)
 
 	WARN_ON(dmar_domain->nested_parent &&
 		!list_empty(&dmar_domain->s1_domains));
-	if (domain != &si_domain->domain)
-		domain_exit(dmar_domain);
+	domain_exit(dmar_domain);
 }
 
 int prepare_domain_attach_device(struct iommu_domain *domain,
@@ -4398,9 +4347,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (ret)
 		goto out_detach_iommu;
 
-	if (domain_type_is_si(dmar_domain))
-		ret = intel_pasid_setup_pass_through(iommu, dev, pasid);
-	else if (dmar_domain->use_first_level)
+	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
 	else
-- 
2.34.1

Re: [PATCH v3 1/7] iommu/vt-d: Require DMA domain if hardware not support passthrough

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:35AM +0800, Lu Baolu wrote:
> The iommu core defines the def_domain_type callback to query the iommu
> driver about hardware capability and quirks. The iommu driver should
> declare IOMMU_DOMAIN_DMA requirement for hardware lacking pass-through
> capability.
> 
> Earlier VT-d hardware implementations did not support pass-through
> translation mode. The iommu driver relied on a paging domain with all
> physical system memory addresses identically mapped to the same IOVA
> to simulate pass-through translation before the def_domain_type was
> introduced and it has been kept until now. It's time to adjust it now
> to make the Intel iommu driver follow the def_domain_type semantics.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 2/7] iommu/vt-d: Remove identity mappings from si_domain

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:36AM +0800, Lu Baolu wrote:
> As the driver has enforced DMA domains for devices managed by an IOMMU
> hardware that doesn't support passthrough translation mode, there is no
> need for static identity mappings in the si_domain. Remove the identity
> mapping code to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 122 ++----------------------------------
>  1 file changed, 4 insertions(+), 118 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

I always wondered why this was so fine grained, I would have expected
it to work on blocks of 512 x 1G, once you allocate a memory for the
1G table level you may as well fill it with identity IOPTEs. The only
optimization point is how many blocks of 512x1G are needed..

Jason

Re: [PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:37AM +0800, Lu Baolu wrote:
> We will use a global static identity domain. Reserve a static domain ID
> for it.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 723ea9f3f501..c019fb3b3e78 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
>  	 * entry for first-level or pass-through translation modes should
>  	 * be programmed with a domain id different from those used for
>  	 * second-level or nested translation. We reserve a domain id for
> -	 * this purpose.
> +	 * this purpose. This domain id is also used for identity domain
> +	 * in legacy mode.
>  	 */
> -	if (sm_supported(iommu))
> -		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
> +	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);

That should probablyturn into an IDA someday, it would likely be more
memory efficient than bitmap_zalloc()

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c019fb3b3e78..f37c8c3cba3c 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
>  	bool has_iotlb_device = false;
>  	unsigned long flags;
>  
> +	if (!domain)
> +		return;
> +

This seems really strange, maybe wrong..

The only callers that could take advantage are
iommu_enable_pci_caps()/iommu_disable_pci_caps()

But if they are mucking with ATS then the ATC flushes should not be
done wrong!

So I looked at this and, uh, who even reads domain->has_iotlb_device ?

So I'd just delete  domain->has_iotlb_device and domain_update_iotlb()
as well.

Jason

Re: [PATCH v3 5/7] iommu/vt-d: Factor out helpers from domain_context_mapping_one()

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:39AM +0800, Lu Baolu wrote:
> Extract common code from domain_context_mapping_one() into new helpers,
> making it reusable by other functions such as the upcoming identity domain
> implementation. No intentional functional changes.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 99 ++++++++++++++++++++++---------------
>  1 file changed, 58 insertions(+), 41 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 6/7] iommu/vt-d: Add support for static identity domain

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:40AM +0800, Lu Baolu wrote:
> Software determines VT-d hardware support for passthrough translation by
> inspecting the capability register. If passthrough translation is not
> supported, the device is instructed to use DMA domain for its default
> domain.
> 
> Add a global static identity domain with guaranteed attach semantics for
> IOMMUs that support passthrough translation mode.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 102 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

> +static int identity_domain_attach_dev(struct iommu_domain *domain, struct device *dev)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +	int ret;
> +
> +	device_block_translation(dev);

Though it would be nice if this wasn't hitfull like this, someday..

Jason

Re: [PATCH v3 7/7] iommu/vt-d: Cleanup si_domain

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 10:39:41AM +0800, Lu Baolu wrote:
> The static identity domain has been introduced, rendering the si_domain
> obsolete. Remove si_domain and cleanup the code accordingly.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 91 ++++++++-----------------------------
>  1 file changed, 19 insertions(+), 72 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 2/7] iommu/vt-d: Remove identity mappings from si_domain

[Baolu Lu]

On 2024/8/7 1:05, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 10:39:36AM +0800, Lu Baolu wrote:
>> As the driver has enforced DMA domains for devices managed by an IOMMU
>> hardware that doesn't support passthrough translation mode, there is no
>> need for static identity mappings in the si_domain. Remove the identity
>> mapping code to avoid dead code.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 122 ++----------------------------------
>>   1 file changed, 4 insertions(+), 118 deletions(-)
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> 
> I always wondered why this was so fine grained, I would have expected
> it to work on blocks of 512 x 1G, once you allocate a memory for the
> 1G table level you may as well fill it with identity IOPTEs. The only
> optimization point is how many blocks of 512x1G are needed..

I am not sure whether or not those early hardware support super pages.
  :-).

Thanks,
baolu

Re: [PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Baolu Lu]

On 2024/8/7 1:06, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 10:39:37AM +0800, Lu Baolu wrote:
>> We will use a global static identity domain. Reserve a static domain ID
>> for it.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 723ea9f3f501..c019fb3b3e78 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
>>   	 * entry for first-level or pass-through translation modes should
>>   	 * be programmed with a domain id different from those used for
>>   	 * second-level or nested translation. We reserve a domain id for
>> -	 * this purpose.
>> +	 * this purpose. This domain id is also used for identity domain
>> +	 * in legacy mode.
>>   	 */
>> -	if (sm_supported(iommu))
>> -		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
>> +	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
> That should probablyturn into an IDA someday, it would likely be more
> memory efficient than bitmap_zalloc()

I have tried to. But I failed to find a suitable ida interface to
calculate the count of allocated domain IDs to replace bitmap_weight()
in below code.

static ssize_t domains_used_show(struct device *dev,
                                  struct device_attribute *attr, char *buf)
{
         struct intel_iommu *iommu = dev_to_intel_iommu(dev);
         return sysfs_emit(buf, "%d\n",
                           bitmap_weight(iommu->domain_ids,
                                         cap_ndoms(iommu->cap)));
}
static DEVICE_ATTR_RO(domains_used);

Thanks,
baolu

Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

[Baolu Lu]

On 2024/8/7 1:12, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index c019fb3b3e78..f37c8c3cba3c 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
>>   	bool has_iotlb_device = false;
>>   	unsigned long flags;
>>   
>> +	if (!domain)
>> +		return;
>> +
> This seems really strange, maybe wrong..
> 
> The only callers that could take advantage are
> iommu_enable_pci_caps()/iommu_disable_pci_caps()

Yes.

When the PCI ATS status changes, the domain attached to the device
should have its domain->has_iotlb_device flag updated.

The global static identity domain is a dummy domain without a
corresponding dmar_domain structure. Consequently, the device's
info->domain will be NULL. This is why a check is necessary.

I might need to make this check explicit with an additional change.

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f37c8c3cba3c..d59e9ac223ba 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1270,9 +1270,6 @@ void domain_update_iotlb(struct dmar_domain *domain)
         bool has_iotlb_device = false;
         unsigned long flags;

-       if (!domain)
-               return;
-
         spin_lock_irqsave(&domain->lock, flags);
         list_for_each_entry(info, &domain->devices, link) {
                 if (info->ats_enabled) {
@@ -1330,7 +1327,8 @@ static void iommu_enable_pci_caps(struct 
device_domain_info *info)
         if (info->ats_supported && pci_ats_page_aligned(pdev) &&
             !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
                 info->ats_enabled = 1;
-               domain_update_iotlb(info->domain);
+               if (info->domain)
+                       domain_update_iotlb(info->domain);
         }
  }

@@ -1346,7 +1344,8 @@ static void iommu_disable_pci_caps(struct 
device_domain_info *info)
         if (info->ats_enabled) {
                 pci_disable_ats(pdev);
                 info->ats_enabled = 0;
-               domain_update_iotlb(info->domain);
+               if (info->domain)
+                       domain_update_iotlb(info->domain);
         }

         if (info->pasid_enabled) {

> 
> But if they are mucking with ATS then the ATC flushes should not be
> done wrong!
> 
> So I looked at this and, uh, who even reads domain->has_iotlb_device ?

The has_iotlb_device flag indicates whether a domain is attached to
devices with ATS enabled. If a domain lacks this flag, no device TBLs
need to be invalidated during unmap operations. This optimization avoids
unnecessary looping through all attached devices.

> So I'd just delete  domain->has_iotlb_device and domain_update_iotlb()
> as well.

Thanks,
baolu

Re: [PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 02:19:57PM +0800, Baolu Lu wrote:
> On 2024/8/7 1:06, Jason Gunthorpe wrote:
> > On Tue, Aug 06, 2024 at 10:39:37AM +0800, Lu Baolu wrote:
> > > We will use a global static identity domain. Reserve a static domain ID
> > > for it.
> > > 
> > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > ---
> > >   drivers/iommu/intel/iommu.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index 723ea9f3f501..c019fb3b3e78 100644
> > > --- a/drivers/iommu/intel/iommu.c
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
> > >   	 * entry for first-level or pass-through translation modes should
> > >   	 * be programmed with a domain id different from those used for
> > >   	 * second-level or nested translation. We reserve a domain id for
> > > -	 * this purpose.
> > > +	 * this purpose. This domain id is also used for identity domain
> > > +	 * in legacy mode.
> > >   	 */
> > > -	if (sm_supported(iommu))
> > > -		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
> > > +	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
> > That should probablyturn into an IDA someday, it would likely be more
> > memory efficient than bitmap_zalloc()
> 
> I have tried to. But I failed to find a suitable ida interface to
> calculate the count of allocated domain IDs to replace bitmap_weight()
> in below code.

For something debugging like that just use 
  idr_for_each_entry()
     count++;

It is a weird thing to put in sysfs in the first place...

Jason

Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 02:41:39PM +0800, Baolu Lu wrote:
> On 2024/8/7 1:12, Jason Gunthorpe wrote:
> > On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index c019fb3b3e78..f37c8c3cba3c 100644
> > > --- a/drivers/iommu/intel/iommu.c
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
> > >   	bool has_iotlb_device = false;
> > >   	unsigned long flags;
> > > +	if (!domain)
> > > +		return;
> > > +
> > This seems really strange, maybe wrong..
> > 
> > The only callers that could take advantage are
> > iommu_enable_pci_caps()/iommu_disable_pci_caps()
> 
> Yes.
> 
> When the PCI ATS status changes, the domain attached to the device
> should have its domain->has_iotlb_device flag updated.
> 
> The global static identity domain is a dummy domain without a
> corresponding dmar_domain structure. Consequently, the device's
> info->domain will be NULL. This is why a check is necessary.

I get it, but you can't have ATS turned on at all if you can push the
invalidations. So it seems like something is missing to enforce that
with the identity domains.

> > So I looked at this and, uh, who even reads domain->has_iotlb_device ?
> 
> The has_iotlb_device flag indicates whether a domain is attached to
> devices with ATS enabled. If a domain lacks this flag, no device TBLs
> need to be invalidated during unmap operations. This optimization avoids
> unnecessary looping through all attached devices.

Not any more, that was removed in commit 06792d067989 ("iommu/vt-d:
Cleanup use of iommu_flush_iotlb_psi()")

This compiles, so you should do this instead:

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ad81db026ab236..6549488d1f6bb1 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -492,7 +492,6 @@ void domain_update_iommu_cap(struct dmar_domain *domain)
 		domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw);
 
 	domain->domain.pgsize_bitmap |= domain_super_pgsize_bitmap(domain);
-	domain_update_iotlb(domain);
 }
 
 struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
@@ -1270,32 +1269,6 @@ domain_lookup_dev_info(struct dmar_domain *domain,
 	return NULL;
 }
 
-void domain_update_iotlb(struct dmar_domain *domain)
-{
-	struct dev_pasid_info *dev_pasid;
-	struct device_domain_info *info;
-	bool has_iotlb_device = false;
-	unsigned long flags;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	list_for_each_entry(info, &domain->devices, link) {
-		if (info->ats_enabled) {
-			has_iotlb_device = true;
-			break;
-		}
-	}
-
-	list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
-		info = dev_iommu_priv_get(dev_pasid->dev);
-		if (info->ats_enabled) {
-			has_iotlb_device = true;
-			break;
-		}
-	}
-	domain->has_iotlb_device = has_iotlb_device;
-	spin_unlock_irqrestore(&domain->lock, flags);
-}
-
 /*
  * The extra devTLB flush quirk impacts those QAT devices with PCI device
  * IDs ranging from 0x4940 to 0x4943. It is exempted from risky_device()
@@ -1332,10 +1305,8 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)
 		info->pasid_enabled = 1;
 
 	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
-	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
+	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT))
 		info->ats_enabled = 1;
-		domain_update_iotlb(info->domain);
-	}
 }
 
 static void iommu_disable_pci_caps(struct device_domain_info *info)
@@ -1350,7 +1321,6 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
 	if (info->ats_enabled) {
 		pci_disable_ats(pdev);
 		info->ats_enabled = 0;
-		domain_update_iotlb(info->domain);
 	}
 
 	if (info->pasid_enabled) {
@@ -1524,7 +1494,6 @@ static struct dmar_domain *alloc_domain(unsigned int type)
 	domain->nid = NUMA_NO_NODE;
 	if (first_level_by_default(type))
 		domain->use_first_level = true;
-	domain->has_iotlb_device = false;
 	INIT_LIST_HEAD(&domain->devices);
 	INIT_LIST_HEAD(&domain->dev_pasids);
 	INIT_LIST_HEAD(&domain->cache_tags);
@@ -3622,7 +3591,6 @@ static struct dmar_domain *paging_domain_alloc(struct device *dev, bool first_st
 	xa_init(&domain->iommu_array);
 
 	domain->nid = dev_to_node(dev);
-	domain->has_iotlb_device = info->ats_enabled;
 	domain->use_first_level = first_stage;
 
 	/* calculate the address width */
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index b67c14da12408b..e80aed3bc06e61 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -588,7 +588,6 @@ struct dmar_domain {
 	int	nid;			/* node id */
 	struct xarray iommu_array;	/* Attached IOMMU array */
 
-	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
 	u8 force_snooping : 1;		/* Create IOPTEs with snoop control */
 	u8 set_pte_snp:1;

Re: [PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Baolu Lu]

On 2024/8/7 20:09, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 02:19:57PM +0800, Baolu Lu wrote:
>> On 2024/8/7 1:06, Jason Gunthorpe wrote:
>>> On Tue, Aug 06, 2024 at 10:39:37AM +0800, Lu Baolu wrote:
>>>> We will use a global static identity domain. Reserve a static domain ID
>>>> for it.
>>>>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/intel/iommu.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index 723ea9f3f501..c019fb3b3e78 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
>>>>    	 * entry for first-level or pass-through translation modes should
>>>>    	 * be programmed with a domain id different from those used for
>>>>    	 * second-level or nested translation. We reserve a domain id for
>>>> -	 * this purpose.
>>>> +	 * this purpose. This domain id is also used for identity domain
>>>> +	 * in legacy mode.
>>>>    	 */
>>>> -	if (sm_supported(iommu))
>>>> -		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
>>>> +	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
>>> That should probablyturn into an IDA someday, it would likely be more
>>> memory efficient than bitmap_zalloc()
>> I have tried to. But I failed to find a suitable ida interface to
>> calculate the count of allocated domain IDs to replace bitmap_weight()
>> in below code.
> For something debugging like that just use
>    idr_for_each_entry()
>       count++;

Yeah! It works.

> It is a weird thing to put in sysfs in the first place...

Agreed. But it has been there for years. :-)

Thanks,
baolu

Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

[Baolu Lu]

On 2024/8/7 20:17, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 02:41:39PM +0800, Baolu Lu wrote:
>> On 2024/8/7 1:12, Jason Gunthorpe wrote:
>>> On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index c019fb3b3e78..f37c8c3cba3c 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
>>>>    	bool has_iotlb_device = false;
>>>>    	unsigned long flags;
>>>> +	if (!domain)
>>>> +		return;
>>>> +
>>> This seems really strange, maybe wrong..
>>>
>>> The only callers that could take advantage are
>>> iommu_enable_pci_caps()/iommu_disable_pci_caps()
>> Yes.
>>
>> When the PCI ATS status changes, the domain attached to the device
>> should have its domain->has_iotlb_device flag updated.
>>
>> The global static identity domain is a dummy domain without a
>> corresponding dmar_domain structure. Consequently, the device's
>> info->domain will be NULL. This is why a check is necessary.
> I get it, but you can't have ATS turned on at all if you can push the
> invalidations. So it seems like something is missing to enforce that
> with the identity domains.
> 
>>> So I looked at this and, uh, who even reads domain->has_iotlb_device ?
>> The has_iotlb_device flag indicates whether a domain is attached to
>> devices with ATS enabled. If a domain lacks this flag, no device TBLs
>> need to be invalidated during unmap operations. This optimization avoids
>> unnecessary looping through all attached devices.
> Not any more, that was removed in commit 06792d067989 ("iommu/vt-d:
> Cleanup use of iommu_flush_iotlb_psi()")

Yeah! How stupid I was! It's actually a dead code after the
implementation of cache tags.

> 
> This compiles, so you should do this instead:

Yes. I will cleanup this in a separated patch.

Thanks,
baolu

Re: [PATCH v3 1/7] iommu/vt-d: Require DMA domain if hardware not support passthrough

[Jerry Snitselaar]

On Tue, Aug 06, 2024 at 10:39:35AM GMT, Lu Baolu wrote:
> The iommu core defines the def_domain_type callback to query the iommu
> driver about hardware capability and quirks. The iommu driver should
> declare IOMMU_DOMAIN_DMA requirement for hardware lacking pass-through
> capability.
> 
> Earlier VT-d hardware implementations did not support pass-through
> translation mode. The iommu driver relied on a paging domain with all
> physical system memory addresses identically mapped to the same IOVA
> to simulate pass-through translation before the def_domain_type was
> introduced and it has been kept until now. It's time to adjust it now
> to make the Intel iommu driver follow the def_domain_type semantics.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/iommu/intel/iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9ff8b83c19a3..90ad794a1be7 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2149,6 +2149,16 @@ static bool device_rmrr_is_relaxable(struct device *dev)
>  
>  static int device_def_domain_type(struct device *dev)
>  {
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +
> +	/*
> +	 * Hardware does not support the passthrough translation mode.
> +	 * Always use a dynamaic mapping domain.
> +	 */
> +	if (!ecap_pass_through(iommu->ecap))
> +		return IOMMU_DOMAIN_DMA;
> +
>  	if (dev_is_pci(dev)) {
>  		struct pci_dev *pdev = to_pci_dev(dev);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v3 5/7] iommu/vt-d: Factor out helpers from domain_context_mapping_one()

[Jerry Snitselaar]

On Tue, Aug 06, 2024 at 10:39:39AM GMT, Lu Baolu wrote:
> Extract common code from domain_context_mapping_one() into new helpers,
> making it reusable by other functions such as the upcoming identity domain
> implementation. No intentional functional changes.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH v3 3/7] iommu/vt-d: Always reserve a domain ID for identity setup

[Jerry Snitselaar]

On Tue, Aug 06, 2024 at 10:39:37AM GMT, Lu Baolu wrote:
> We will use a global static identity domain. Reserve a static domain ID
> for it.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/iommu/intel/iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 723ea9f3f501..c019fb3b3e78 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1440,10 +1440,10 @@ static int iommu_init_domains(struct intel_iommu *iommu)
>  	 * entry for first-level or pass-through translation modes should
>  	 * be programmed with a domain id different from those used for
>  	 * second-level or nested translation. We reserve a domain id for
> -	 * this purpose.
> +	 * this purpose. This domain id is also used for identity domain
> +	 * in legacy mode.
>  	 */
> -	if (sm_supported(iommu))
> -		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
> +	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
>