[PATCH 0/3] iommu: Fix domain check on release (part 2/2)

[PATCH 0/3] iommu: Fix domain check on release (part 2/2)

[Lu Baolu]

This is a follow-up to the discussion thread here:

https://lore.kernel.org/linux-iommu/20240221154012.GC13491@ziepe.ca/

This change includes cleanups and refactoring in the Intel iommu driver
related to the use of the iommu_ops->release_domain.

There are two parts of this topic:
[ ] Introduce release_domain and fix a kernel NULL pointer dereference
    issue in the intel iommu driver.
[x] A follow-up series to cleanup intel iommu driver.

Best regards,
baolu

Lu Baolu (3):
  iommu/vt-d: Setup scalable mode context entry in probe path
  iommu/vt-d: Remove scalable mode context entry setup from attach_dev
  iommu/vt-d: Remove scalabe mode in domain_context_clear_one()

 drivers/iommu/intel/pasid.h |   1 +
 drivers/iommu/intel/iommu.c | 183 +++++++++++-------------------------
 drivers/iommu/intel/pasid.c | 116 +++++++++++++++++++++++
 3 files changed, 174 insertions(+), 126 deletions(-)

-- 
2.34.1

[PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

[Lu Baolu]

In contrast to legacy mode, the DMA translation table is configured in
the PASID table entry instead of the context entry for scalable mode.
For this reason, it is more appropriate to set up the scalable mode
context entry in the device_probe callback and direct it to the
appropriate PASID table.

The iommu domain attach/detach operations only affect the PASID table
entry. Therefore, there is no need to modify the context entry when
configuring the translation type and page table.

The only exception is the kdump case, where context entry setup is
postponed until the device driver invokes the first DMA interface.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/pasid.h |   1 +
 drivers/iommu/intel/iommu.c |  12 ++++
 drivers/iommu/intel/pasid.c | 116 ++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 42fda97fd851..da9978fef7ac 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -318,5 +318,6 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 				 bool fault_ignore);
 void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 					  struct device *dev, u32 pasid);
+int intel_pasid_setup_sm_context(struct device *dev);
 void intel_pasid_teardown_sm_context(struct device *dev);
 #endif /* __INTEL_PASID_H */
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f74d42d3258f..9b96d36b9d2a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4073,6 +4073,10 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
 		dmar_domain->agaw--;
 	}
 
+	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev) &&
+	    context_copied(iommu, info->bus, info->devfn))
+		return intel_pasid_setup_sm_context(dev);
+
 	return 0;
 }
 
@@ -4386,11 +4390,19 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 			dev_err(dev, "PASID table allocation failed\n");
 			goto clear_rbtree;
 		}
+
+		if (!context_copied(iommu, info->bus, info->devfn)) {
+			ret = intel_pasid_setup_sm_context(dev);
+			if (ret)
+				goto free_table;
+		}
 	}
 
 	intel_iommu_debugfs_create_dev(info);
 
 	return &iommu->iommu;
+free_table:
+	intel_pasid_free_table(dev);
 clear_rbtree:
 	device_rbtree_remove(info);
 free:
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 52068cf52fe2..4ea8f35bd460 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -750,3 +750,119 @@ void intel_pasid_teardown_sm_context(struct device *dev)
 
 	pci_for_each_dma_alias(to_pci_dev(dev), pci_pasid_table_teardown, dev);
 }
+
+/*
+ * Get the PASID directory size for scalable mode context entry.
+ * Value of X in the PDTS field of a scalable mode context entry
+ * indicates PASID directory with 2^(X + 7) entries.
+ */
+static unsigned long context_get_sm_pds(struct pasid_table *table)
+{
+	unsigned long pds, max_pde;
+
+	max_pde = table->max_pasid >> PASID_PDE_SHIFT;
+	pds = find_first_bit(&max_pde, MAX_NR_PASID_BITS);
+	if (pds < 7)
+		return 0;
+
+	return pds - 7;
+}
+
+static int context_entry_set_pasid_table(struct context_entry *context,
+					 struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct pasid_table *table = info->pasid_table;
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long pds;
+
+	context_clear_entry(context);
+
+	pds = context_get_sm_pds(table);
+	context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);
+	context_set_sm_rid2pasid(context, IOMMU_NO_PASID);
+
+	if (info->ats_supported)
+		context_set_sm_dte(context);
+	if (info->pri_supported)
+		context_set_sm_pre(context);
+	if (info->pasid_supported)
+		context_set_pasid(context);
+
+	context_set_fault_enable(context);
+	context_set_present(context);
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(context, sizeof(*context));
+
+	return 0;
+}
+
+static int device_pasid_table_setup(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, true);
+	if (!context) {
+		spin_unlock(&iommu->lock);
+		return -ENOMEM;
+	}
+
+	if (context_present(context) && !context_copied(iommu, bus, devfn)) {
+		spin_unlock(&iommu->lock);
+		return 0;
+	}
+
+	/*
+	 * For kdump case, 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)) {
+		context_clear_entry(context);
+		if (!ecap_coherent(iommu->ecap))
+			clflush_cache_range(context, sizeof(*context));
+		sm_context_flush_caches(dev);
+		clear_context_copied(iommu, bus, devfn);
+	}
+
+	context_entry_set_pasid_table(context, dev);
+	spin_unlock(&iommu->lock);
+
+	/*
+	 * It's a non-present to present mapping. If hardware doesn't cache
+	 * non-present entry we don't need to flush the caches.
+	 */
+	if (cap_caching_mode(iommu->cap))
+		sm_context_flush_caches(dev);
+
+	return 0;
+}
+
+static int pci_pasid_table_setup(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct device *dev = data;
+
+	if (dev != &pdev->dev)
+		return 0;
+
+	return device_pasid_table_setup(dev, PCI_BUS_NUM(alias), alias & 0xff);
+}
+
+/*
+ * Set the device's PASID table to its context table entry.
+ *
+ * The PASID table is set to the context entries of both device itself
+ * and its alias requester ID for DMA.
+ */
+int intel_pasid_setup_sm_context(struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+
+	if (!dev_is_pci(dev))
+		return device_pasid_table_setup(dev, info->bus, info->devfn);
+
+	return pci_for_each_dma_alias(to_pci_dev(dev), pci_pasid_table_setup, dev);
+}
-- 
2.34.1

[PATCH 2/3] iommu/vt-d: Remove scalable mode context entry setup from attach_dev

[Lu Baolu]

The scalable mode context entry is now setup in the probe_device path,
eliminating the need to configure it in the attach_dev path. Removes the
redundant code from the attach_dev path to avoid dead code.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9b96d36b9d2a..d682eb6ad4d2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1850,34 +1850,17 @@ static void domain_exit(struct dmar_domain *domain)
 	kfree(domain);
 }
 
-/*
- * Get the PASID directory size for scalable mode context entry.
- * Value of X in the PDTS field of a scalable mode context entry
- * indicates PASID directory with 2^(X + 7) entries.
- */
-static unsigned long context_get_sm_pds(struct pasid_table *table)
-{
-	unsigned long pds, max_pde;
-
-	max_pde = table->max_pasid >> PASID_PDE_SHIFT;
-	pds = find_first_bit(&max_pde, MAX_NR_PASID_BITS);
-	if (pds < 7)
-		return 0;
-
-	return pds - 7;
-}
-
 static int domain_context_mapping_one(struct dmar_domain *domain,
 				      struct intel_iommu *iommu,
-				      struct pasid_table *table,
 				      u8 bus, u8 devfn)
 {
 	struct device_domain_info *info =
 			domain_lookup_dev_info(domain, iommu, bus, devfn);
 	u16 did = domain_id_iommu(domain, iommu);
 	int translation = CONTEXT_TT_MULTI_LEVEL;
+	struct dma_pte *pgd = domain->pgd;
 	struct context_entry *context;
-	int ret;
+	int agaw, ret;
 
 	if (hw_pass_through && domain_type_is_si(domain))
 		translation = CONTEXT_TT_PASS_THROUGH;
@@ -1920,65 +1903,37 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	}
 
 	context_clear_entry(context);
+	context_set_domain_id(context, did);
 
-	if (sm_supported(iommu)) {
-		unsigned long pds;
-
-		/* Setup the PASID DIR pointer: */
-		pds = context_get_sm_pds(table);
-		context->lo = (u64)virt_to_phys(table->table) |
-				context_pdts(pds);
-
-		/* Setup the RID_PASID field: */
-		context_set_sm_rid2pasid(context, IOMMU_NO_PASID);
-
+	if (translation != CONTEXT_TT_PASS_THROUGH) {
 		/*
-		 * Setup the Device-TLB enable bit and Page request
-		 * Enable bit:
+		 * 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)
-			context_set_sm_dte(context);
-		if (info && info->pri_supported)
-			context_set_sm_pre(context);
-		if (info && info->pasid_supported)
-			context_set_pasid(context);
+			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 {
-		struct dma_pte *pgd = domain->pgd;
-		int agaw;
-
-		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);
-		}
-
-		context_set_translation_type(context, translation);
+		/*
+		 * 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, translation);
 	context_set_fault_enable(context);
 	context_set_present(context);
 	if (!ecap_coherent(iommu->ecap))
@@ -2008,43 +1963,29 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	return ret;
 }
 
-struct domain_context_mapping_data {
-	struct dmar_domain *domain;
-	struct intel_iommu *iommu;
-	struct pasid_table *table;
-};
-
 static int domain_context_mapping_cb(struct pci_dev *pdev,
 				     u16 alias, void *opaque)
 {
-	struct domain_context_mapping_data *data = opaque;
+	struct device_domain_info *info = dev_iommu_priv_get(&pdev->dev);
+	struct intel_iommu *iommu = info->iommu;
+	struct dmar_domain *domain = opaque;
 
-	return domain_context_mapping_one(data->domain, data->iommu,
-					  data->table, PCI_BUS_NUM(alias),
-					  alias & 0xff);
+	return domain_context_mapping_one(domain, iommu,
+					  PCI_BUS_NUM(alias), alias & 0xff);
 }
 
 static int
 domain_context_mapping(struct dmar_domain *domain, struct device *dev)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct domain_context_mapping_data data;
 	struct intel_iommu *iommu = info->iommu;
 	u8 bus = info->bus, devfn = info->devfn;
-	struct pasid_table *table;
-
-	table = intel_pasid_get_table(dev);
 
 	if (!dev_is_pci(dev))
-		return domain_context_mapping_one(domain, iommu, table,
-						  bus, devfn);
-
-	data.domain = domain;
-	data.iommu = iommu;
-	data.table = table;
+		return domain_context_mapping_one(domain, iommu, bus, devfn);
 
 	return pci_for_each_dma_alias(to_pci_dev(dev),
-				      &domain_context_mapping_cb, &data);
+				      domain_context_mapping_cb, domain);
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */
@@ -2404,28 +2345,19 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 	list_add(&info->link, &domain->devices);
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-	/* PASID table is mandatory for a PCI device in scalable mode. */
-	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
-		/* Setup the PASID entry for requests without PASID: */
-		if (hw_pass_through && 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
-			ret = intel_pasid_setup_second_level(iommu, domain,
-					dev, IOMMU_NO_PASID);
-		if (ret) {
-			dev_err(dev, "Setup RID2PASID failed\n");
-			device_block_translation(dev);
-			return ret;
-		}
-	}
+	if (dev_is_real_dma_subdevice(dev))
+		return 0;
+
+	if (!sm_supported(iommu))
+		ret = domain_context_mapping(domain, dev);
+	else if (hw_pass_through && 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
+		ret = intel_pasid_setup_second_level(iommu, domain, dev, IOMMU_NO_PASID);
 
-	ret = domain_context_mapping(domain, dev);
 	if (ret) {
-		dev_err(dev, "Domain context map failed\n");
 		device_block_translation(dev);
 		return ret;
 	}
-- 
2.34.1

[PATCH 3/3] iommu/vt-d: Remove scalabe mode in domain_context_clear_one()

[Lu Baolu]

domain_context_clear_one() only handles the context entry teardown in
legacy mode. Remove the scalable mode check in it to avoid dead code.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d682eb6ad4d2..50eb9aed47cc 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2175,9 +2175,6 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 	struct context_entry *context;
 	u16 did_old;
 
-	if (!iommu)
-		return;
-
 	spin_lock(&iommu->lock);
 	context = iommu_context_addr(iommu, bus, devfn, 0);
 	if (!context) {
@@ -2185,14 +2182,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 		return;
 	}
 
-	if (sm_supported(iommu)) {
-		if (hw_pass_through && domain_type_is_si(info->domain))
-			did_old = FLPT_DEFAULT_DID;
-		else
-			did_old = domain_id_iommu(info->domain, iommu);
-	} else {
-		did_old = context_domain_id(context);
-	}
+	did_old = context_domain_id(context);
 
 	context_clear_entry(context);
 	__iommu_flush_cache(iommu, context, sizeof(*context));
@@ -2203,9 +2193,6 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 				   DMA_CCMD_MASK_NOBIT,
 				   DMA_CCMD_DEVICE_INVL);
 
-	if (sm_supported(iommu))
-		qi_flush_pasid_cache(iommu, did_old, QI_PC_ALL_PASIDS, 0);
-
 	iommu->flush.flush_iotlb(iommu,
 				 did_old,
 				 0,
-- 
2.34.1

RE: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, February 29, 2024 5:48 PM
> 
> +static int device_pasid_table_setup(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, true);
> +	if (!context) {
> +		spin_unlock(&iommu->lock);
> +		return -ENOMEM;
> +	}
> +
> +	if (context_present(context) && !context_copied(iommu, bus, devfn))
> {
> +		spin_unlock(&iommu->lock);
> +		return 0;
> +	}
> +
> +	/*
> +	 * For kdump case, 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)) {
> +		context_clear_entry(context);
> +		if (!ecap_coherent(iommu->ecap))
> +			clflush_cache_range(context, sizeof(*context));
> +		sm_context_flush_caches(dev);
> +		clear_context_copied(iommu, bus, devfn);
> +	}

it's unclear to me why this doesn't need refer to old did as done in the
existing code. If scalable mode makes any difference could you extend
the comment to explain so people can avoid similar confusion when
comparing the different paths between legacy and sm?

anyway it's kind of a semantics change probably worth a separate patch
to special case sm for bisect and then doing cleanup...

RE: [PATCH 2/3] iommu/vt-d: Remove scalable mode context entry setup from attach_dev

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, February 29, 2024 5:48 PM
> 
> The scalable mode context entry is now setup in the probe_device path,
> eliminating the need to configure it in the attach_dev path. Removes the
> redundant code from the attach_dev path to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

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

RE: [PATCH 3/3] iommu/vt-d: Remove scalabe mode in domain_context_clear_one()

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, February 29, 2024 5:48 PM
> 
> @@ -2175,9 +2175,6 @@ static void domain_context_clear_one(struct
> device_domain_info *info, u8 bus, u8
>  	struct context_entry *context;
>  	u16 did_old;
> 
> -	if (!iommu)
> -		return;
> -

is this check only relevant to sm mode or should it be removed for
both legacy/sm? If the latter please add a note in the commit msg.

otherwise,

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

Re: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

[Baolu Lu]

On 2024/3/4 15:48, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, February 29, 2024 5:48 PM
>>
>> +static int device_pasid_table_setup(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, true);
>> +	if (!context) {
>> +		spin_unlock(&iommu->lock);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (context_present(context) && !context_copied(iommu, bus, devfn))
>> {
>> +		spin_unlock(&iommu->lock);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * For kdump case, 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)) {
>> +		context_clear_entry(context);
>> +		if (!ecap_coherent(iommu->ecap))
>> +			clflush_cache_range(context, sizeof(*context));
>> +		sm_context_flush_caches(dev);
>> +		clear_context_copied(iommu, bus, devfn);
>> +	}
> 
> it's unclear to me why this doesn't need refer to old did as done in the
> existing code. If scalable mode makes any difference could you extend
> the comment to explain so people can avoid similar confusion when
> comparing the different paths between legacy and sm?

The previous code gets the domain ID from the copied context entry:

u16 did_old = context_domain_id(context);

This makes no sense for scalable mode, as the domain ID has been moved
to the PASID entry in scalable mode. As the result, did_old always gets
0.

> anyway it's kind of a semantics change probably worth a separate patch
> to special case sm for bisect and then doing cleanup...

This change doesn't impact anything as the hardware will skip domain id
field in the Context-cache Invalidate Descriptor in scalable mode.

Spec section 6.5.2.1 Context-cache Invalidate Descriptor:

Domain-ID (DID): ... This field is ignored by hardware when operating in
scalable mode (RTADDR_REG.TTM=01b).

Best regards,
baolu

Re: [PATCH 3/3] iommu/vt-d: Remove scalabe mode in domain_context_clear_one()

[Baolu Lu]

On 2024/3/4 15:53, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Thursday, February 29, 2024 5:48 PM
>>
>> @@ -2175,9 +2175,6 @@ static void domain_context_clear_one(struct
>> device_domain_info *info, u8 bus, u8
>>   	struct context_entry *context;
>>   	u16 did_old;
>>
>> -	if (!iommu)
>> -		return;
>> -
> is this check only relevant to sm mode or should it be removed for
> both legacy/sm? If the latter please add a note in the commit msg.

This kind of check makes no sense. I didn't take it to sm mode. So only
need to remove it in the legacy path.

Anyway, let me update the commit message to reflect this change.

Best regards,
baolu

RE: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, March 4, 2024 4:24 PM
> 
> On 2024/3/4 15:48, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, February 29, 2024 5:48 PM
> >>
> >> +	/*
> >> +	 * For kdump case, 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)) {
> >> +		context_clear_entry(context);
> >> +		if (!ecap_coherent(iommu->ecap))
> >> +			clflush_cache_range(context, sizeof(*context));
> >> +		sm_context_flush_caches(dev);
> >> +		clear_context_copied(iommu, bus, devfn);
> >> +	}
> >
> > it's unclear to me why this doesn't need refer to old did as done in the
> > existing code. If scalable mode makes any difference could you extend
> > the comment to explain so people can avoid similar confusion when
> > comparing the different paths between legacy and sm?
> 
> The previous code gets the domain ID from the copied context entry:
> 
> u16 did_old = context_domain_id(context);
> 
> This makes no sense for scalable mode, as the domain ID has been moved
> to the PASID entry in scalable mode. As the result, did_old always gets
> 0.

The point is whether the driver requires to invalidate cache for old did
which is orthogonal to using legacy or sm. If yes, then we should fix the
code to find the right did instead of ignoring it. If no then the legacy path
should be cleared too to avoid unnecessary burden.

> 
> > anyway it's kind of a semantics change probably worth a separate patch
> > to special case sm for bisect and then doing cleanup...
> 
> This change doesn't impact anything as the hardware will skip domain id
> field in the Context-cache Invalidate Descriptor in scalable mode.
> 

no semantics change but if old code has bug we should fix it instead
of carrying the behavior. 😊

Re: [PATCH 1/3] iommu/vt-d: Setup scalable mode context entry in probe path

[Baolu Lu]

On 2024/3/4 17:05, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, March 4, 2024 4:24 PM
>>
>> On 2024/3/4 15:48, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Thursday, February 29, 2024 5:48 PM
>>>>
>>>> +	/*
>>>> +	 * For kdump case, 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)) {
>>>> +		context_clear_entry(context);
>>>> +		if (!ecap_coherent(iommu->ecap))
>>>> +			clflush_cache_range(context, sizeof(*context));
>>>> +		sm_context_flush_caches(dev);
>>>> +		clear_context_copied(iommu, bus, devfn);
>>>> +	}
>>>
>>> it's unclear to me why this doesn't need refer to old did as done in the
>>> existing code. If scalable mode makes any difference could you extend
>>> the comment to explain so people can avoid similar confusion when
>>> comparing the different paths between legacy and sm?
>>
>> The previous code gets the domain ID from the copied context entry:
>>
>> u16 did_old = context_domain_id(context);
>>
>> This makes no sense for scalable mode, as the domain ID has been moved
>> to the PASID entry in scalable mode. As the result, did_old always gets
>> 0.
> 
> The point is whether the driver requires to invalidate cache for old did
> which is orthogonal to using legacy or sm. If yes, then we should fix the
> code to find the right did instead of ignoring it. If no then the legacy path
> should be cleared too to avoid unnecessary burden.
> 
>>
>>> anyway it's kind of a semantics change probably worth a separate patch
>>> to special case sm for bisect and then doing cleanup...
>>
>> This change doesn't impact anything as the hardware will skip domain id
>> field in the Context-cache Invalidate Descriptor in scalable mode.
>>
> 
> no semantics change but if old code has bug we should fix it instead
> of carrying the behavior. 😊

The driver is required to invalidate the cache for the old did.

The previous code invalidated the cache twice, once in
intel_pasid_tear_down_entry() and another time in
domain_context_clear().

The new code attempts to eliminate this duplication by invalidating the
cache for the did during blocking domain attachment and skipping it in
sm-context-entry teardown.

Best regards,
baolu