[PATCH 0/5] iommu/vt-d: Force snooping improvement

[PATCH 0/5] iommu/vt-d: Force snooping improvement

[Lu Baolu]

Hi folks,

Previously, the IOMMU capability of enforcing cache coherency is queried
through iommu_capable(IOMMU_CAP_CACHE_COHERENCY). This is a global
capability, hence the IOMMU driver reports support for this capability
only when all IOMMUs in the system has this support.

Commit 6043257b1de06 ("iommu: Introduce the domain op
enforce_cache_coherency()") converts this into a per-domain test-and-set
option, and the previous iommu_capable(IOMMU_CAP_CACHE_COHERENCY) is
deprecated.

This is a follow-up series which improves the Intel IOMMU driver to
support the per-domain scheme better.

Best regards,
baolu 

Lu Baolu (5):
  iommu/vt-d: Block force-snoop domain attaching if no SC support
  iommu/vt-d: Set SNP bit only in second-level page table entries
  iommu/vt-d: Check domain force_snooping against attached devices
  iommu/vt-d: Remove domain_update_iommu_snooping()
  iommu/vt-d: Remove hard coding PGSNP bit in PASID entries

 drivers/iommu/intel/pasid.h |  3 +-
 drivers/iommu/intel/iommu.c | 92 ++++++++++++++++++++++---------------
 drivers/iommu/intel/pasid.c | 24 +++++++---
 3 files changed, 74 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH 1/5] iommu/vt-d: Block force-snoop domain attaching if no SC support

[Lu Baolu]

In the attach_dev callback of the default domain ops, if the domain has
been set force_snooping, but the iommu hardware of the device does not
support SC(Snoop Control) capability, the callback should block it and
return a corresponding error code.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cf43e8f9091b..d68f5bbf3e93 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4354,6 +4354,9 @@ static int prepare_domain_attach_device(struct iommu_domain *domain,
 	if (!iommu)
 		return -ENODEV;
 
+	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
+		return -EOPNOTSUPP;
+
 	/* check if this iommu agaw is sufficient for max mapped address */
 	addr_width = agaw_to_width(iommu->agaw);
 	if (addr_width > cap_mgaw(iommu->cap))
-- 
2.25.1

Re: [PATCH 1/5] iommu/vt-d: Block force-snoop domain attaching if no SC support

[Jason Gunthorpe]

On Sun, May 01, 2022 at 07:24:30PM +0800, Lu Baolu wrote:
> In the attach_dev callback of the default domain ops, if the domain has
> been set force_snooping, but the iommu hardware of the device does not
> support SC(Snoop Control) capability, the callback should block it and
> return a corresponding error code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 3 +++
>  1 file changed, 3 insertions(+)

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

Jason

[PATCH 2/5] iommu/vt-d: Set SNP bit only in second-level page table entries

[Lu Baolu]

The SNP bit is only valid for second-level PTEs. Setting this bit in the
first-level PTEs has no functional impact because the Intel IOMMU always
ignores the same bit in first-level PTEs. Anyway, let's check the page
table type before setting SNP bit in PTEs to make the code more readable.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d68f5bbf3e93..98050943d863 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4431,7 +4431,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		prot |= DMA_PTE_READ;
 	if (iommu_prot & IOMMU_WRITE)
 		prot |= DMA_PTE_WRITE;
-	if (dmar_domain->force_snooping)
+	if (dmar_domain->force_snooping && !domain_use_first_level(dmar_domain))
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
-- 
2.25.1

Re: [PATCH 2/5] iommu/vt-d: Set SNP bit only in second-level page table entries

[Jason Gunthorpe]

On Sun, May 01, 2022 at 07:24:31PM +0800, Lu Baolu wrote:
> The SNP bit is only valid for second-level PTEs. Setting this bit in the
> first-level PTEs has no functional impact because the Intel IOMMU always
> ignores the same bit in first-level PTEs. Anyway, let's check the page
> table type before setting SNP bit in PTEs to make the code more readable.

Shouldn't this be tested before setting force_snooping and not during
every map?

Jason

Re: [PATCH 2/5] iommu/vt-d: Set SNP bit only in second-level page table entries

[Baolu Lu]

Hi Jason,

On 2022/5/2 21:05, Jason Gunthorpe wrote:
> On Sun, May 01, 2022 at 07:24:31PM +0800, Lu Baolu wrote:
>> The SNP bit is only valid for second-level PTEs. Setting this bit in the
>> first-level PTEs has no functional impact because the Intel IOMMU always
>> ignores the same bit in first-level PTEs. Anyway, let's check the page
>> table type before setting SNP bit in PTEs to make the code more readable.
> Shouldn't this be tested before setting force_snooping and not during
> every map?

The check is in the following patch. This just makes sure that SNP is
only set in second-level page table entries.

Best regards,
baolu

Re: [PATCH 2/5] iommu/vt-d: Set SNP bit only in second-level page table entries

[Jason Gunthorpe]

On Wed, May 04, 2022 at 03:25:50PM +0800, Baolu Lu wrote:
> Hi Jason,
> 
> On 2022/5/2 21:05, Jason Gunthorpe wrote:
> > On Sun, May 01, 2022 at 07:24:31PM +0800, Lu Baolu wrote:
> > > The SNP bit is only valid for second-level PTEs. Setting this bit in the
> > > first-level PTEs has no functional impact because the Intel IOMMU always
> > > ignores the same bit in first-level PTEs. Anyway, let's check the page
> > > table type before setting SNP bit in PTEs to make the code more readable.
> > Shouldn't this be tested before setting force_snooping and not during
> > every map?
> 
> The check is in the following patch. This just makes sure that SNP is
> only set in second-level page table entries.

I think you should add a 2nd flag to indicate 'set SNP bit in PTEs'
and take care of computing that flag in the enforce_no_snoop function

Jason

Re: [PATCH 2/5] iommu/vt-d: Set SNP bit only in second-level page table entries

[Baolu Lu]

On 2022/5/4 21:31, Jason Gunthorpe wrote:
> On Wed, May 04, 2022 at 03:25:50PM +0800, Baolu Lu wrote:
>> Hi Jason,
>>
>> On 2022/5/2 21:05, Jason Gunthorpe wrote:
>>> On Sun, May 01, 2022 at 07:24:31PM +0800, Lu Baolu wrote:
>>>> The SNP bit is only valid for second-level PTEs. Setting this bit in the
>>>> first-level PTEs has no functional impact because the Intel IOMMU always
>>>> ignores the same bit in first-level PTEs. Anyway, let's check the page
>>>> table type before setting SNP bit in PTEs to make the code more readable.
>>> Shouldn't this be tested before setting force_snooping and not during
>>> every map?
>>
>> The check is in the following patch. This just makes sure that SNP is
>> only set in second-level page table entries.
> 
> I think you should add a 2nd flag to indicate 'set SNP bit in PTEs'
> and take care of computing that flag in the enforce_no_snoop function

Yours looks better. Will add it in the next version.

> Jason

Best regards,
baolu

[PATCH 3/5] iommu/vt-d: Check domain force_snooping against attached devices

[Lu Baolu]

As domain->force_snooping only impacts the devices attached with the
domain, there's no need to check against all IOMMU units. At the same
time, for a brand new domain (hasn't been attached to any device), the
force_snooping field could be set, but the attach_dev callback will
return failure if it wants to attach to a device which IOMMU has no
snoop control capability.

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

diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index ab4408c824a5..583ea67fc783 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -123,4 +123,6 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 				 bool fault_ignore);
 int vcmd_alloc_pasid(struct intel_iommu *iommu, u32 *pasid);
 void vcmd_free_pasid(struct intel_iommu *iommu, u32 pasid);
+void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
+					  struct device *dev, u32 pasid);
 #endif /* __INTEL_PASID_H */
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 98050943d863..3c1c228f9031 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4554,13 +4554,61 @@ static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
 	return phys;
 }
 
+static bool domain_support_force_snooping(struct dmar_domain *domain)
+{
+	struct device_domain_info *info;
+	unsigned long flags;
+	bool support = true;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	if (list_empty(&domain->devices))
+		goto out;
+
+	list_for_each_entry(info, &domain->devices, link) {
+		if (!ecap_sc_support(info->iommu->ecap)) {
+			support = false;
+			break;
+		}
+	}
+out:
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+	return support;
+}
+
+static void domain_set_force_snooping(struct dmar_domain *domain)
+{
+	struct device_domain_info *info;
+	unsigned long flags;
+
+	/*
+	 * Second level page table supports per-PTE snoop control. The
+	 * iommu_map() interface will handle this by setting SNP bit.
+	 */
+	if (!domain_use_first_level(domain))
+		return;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	if (list_empty(&domain->devices))
+		goto out_unlock;
+
+	list_for_each_entry(info, &domain->devices, link)
+		intel_pasid_setup_page_snoop_control(info->iommu, info->dev,
+						     PASID_RID2PASID);
+
+out_unlock:
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+}
+
 static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 
-	if (!domain_update_iommu_snooping(NULL))
+	if (!domain_support_force_snooping(dmar_domain))
 		return false;
+
+	domain_set_force_snooping(dmar_domain);
 	dmar_domain->force_snooping = true;
+
 	return true;
 }
 
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index f8d215d85695..815c744e6a34 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -762,3 +762,21 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 
 	return 0;
 }
+
+/*
+ * Set the page snoop control for a pasid entry which has been set up.
+ */
+void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
+					  struct device *dev, u32 pasid)
+{
+	struct pasid_entry *pte;
+	u16 did;
+
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (WARN_ON(!pte || !pasid_pte_is_present(pte)))
+		return;
+
+	pasid_set_pgsnp(pte);
+	did = pasid_get_domain_id(pte);
+	pasid_flush_caches(iommu, pte, pasid, did);
+}
-- 
2.25.1

Re: [PATCH 3/5] iommu/vt-d: Check domain force_snooping against attached devices

[Jason Gunthorpe]

On Sun, May 01, 2022 at 07:24:32PM +0800, Lu Baolu wrote:
> +static bool domain_support_force_snooping(struct dmar_domain *domain)
> +{
> +	struct device_domain_info *info;
> +	unsigned long flags;
> +	bool support = true;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	if (list_empty(&domain->devices))
> +		goto out;

Why? list_for_each_entry will just do nothing..

> +	list_for_each_entry(info, &domain->devices, link) {
> +		if (!ecap_sc_support(info->iommu->ecap)) {
> +			support = false;
> +			break;
> +		}
> +	}
> +out:
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +	return support;
> +}
> +
> +static void domain_set_force_snooping(struct dmar_domain *domain)
> +{
> +	struct device_domain_info *info;
> +	unsigned long flags;
> +
> +	/*
> +	 * Second level page table supports per-PTE snoop control. The
> +	 * iommu_map() interface will handle this by setting SNP bit.
> +	 */
> +	if (!domain_use_first_level(domain))
> +		return;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	if (list_empty(&domain->devices))
> +		goto out_unlock;
> +
> +	list_for_each_entry(info, &domain->devices, link)
> +		intel_pasid_setup_page_snoop_control(info->iommu, info->dev,
> +						     PASID_RID2PASID);
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +}
> +
>  static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>  {
>  	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>  
> -	if (!domain_update_iommu_snooping(NULL))
> +	if (!domain_support_force_snooping(dmar_domain))
>  		return false;

Maybe exit early if force_snooping = true?

> +	domain_set_force_snooping(dmar_domain);
>  	dmar_domain->force_snooping = true;
> +
>  	return true;
>  }
>  
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index f8d215d85695..815c744e6a34 100644
> +++ b/drivers/iommu/intel/pasid.c
> @@ -762,3 +762,21 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
>  
>  	return 0;
>  }
> +
> +/*
> + * Set the page snoop control for a pasid entry which has been set up.
> + */

So the 'first level' is only used with pasid?

> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
> +					  struct device *dev, u32 pasid)
> +{
> +	struct pasid_entry *pte;
> +	u16 did;
> +
> +	pte = intel_pasid_get_entry(dev, pasid);
> +	if (WARN_ON(!pte || !pasid_pte_is_present(pte)))
> +		return;
> +
> +	pasid_set_pgsnp(pte);

Doesn't this need to be done in other places too, like when a new attach
is made? Patch 5 removed it, but should that be made if
domain->force_snooping?

Jason

Re: [PATCH 3/5] iommu/vt-d: Check domain force_snooping against attached devices

[Baolu Lu]

On 2022/5/2 21:17, Jason Gunthorpe wrote:
> On Sun, May 01, 2022 at 07:24:32PM +0800, Lu Baolu wrote:
>> +static bool domain_support_force_snooping(struct dmar_domain *domain)
>> +{
>> +	struct device_domain_info *info;
>> +	unsigned long flags;
>> +	bool support = true;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	if (list_empty(&domain->devices))
>> +		goto out;
> 
> Why? list_for_each_entry will just do nothing..

Yes. I will remove above two lines.

> 
>> +	list_for_each_entry(info, &domain->devices, link) {
>> +		if (!ecap_sc_support(info->iommu->ecap)) {
>> +			support = false;
>> +			break;
>> +		}
>> +	}
>> +out:
>> +	spin_unlock_irqrestore(&device_domain_lock, flags);
>> +	return support;
>> +}
>> +
>> +static void domain_set_force_snooping(struct dmar_domain *domain)
>> +{
>> +	struct device_domain_info *info;
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * Second level page table supports per-PTE snoop control. The
>> +	 * iommu_map() interface will handle this by setting SNP bit.
>> +	 */
>> +	if (!domain_use_first_level(domain))
>> +		return;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	if (list_empty(&domain->devices))
>> +		goto out_unlock;
>> +
>> +	list_for_each_entry(info, &domain->devices, link)
>> +		intel_pasid_setup_page_snoop_control(info->iommu, info->dev,
>> +						     PASID_RID2PASID);
>> +
>> +out_unlock:
>> +	spin_unlock_irqrestore(&device_domain_lock, flags);
>> +}
>> +
>>   static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>>   {
>>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>>   
>> -	if (!domain_update_iommu_snooping(NULL))
>> +	if (!domain_support_force_snooping(dmar_domain))
>>   		return false;
> 
> Maybe exit early if force_snooping = true?

Yes, should check "force_snooping = true" and return directly if
force_snooping has already been set. As you pointed below, the new
domain_attach should take care of this flag as well. Thanks!

> 
>> +	domain_set_force_snooping(dmar_domain);
>>   	dmar_domain->force_snooping = true;
>> +
>>   	return true;
>>   }
>>   
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index f8d215d85695..815c744e6a34 100644
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -762,3 +762,21 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
>>   
>>   	return 0;
>>   }
>> +
>> +/*
>> + * Set the page snoop control for a pasid entry which has been set up.
>> + */
> 
> So the 'first level' is only used with pasid?

Yes. A fake pasid (RID2PASID in spec) is used for legacy transactions
(those w/o pasid).

> 
>> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
>> +					  struct device *dev, u32 pasid)
>> +{
>> +	struct pasid_entry *pte;
>> +	u16 did;
>> +
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	if (WARN_ON(!pte || !pasid_pte_is_present(pte)))
>> +		return;
>> +
>> +	pasid_set_pgsnp(pte);
> 
> Doesn't this need to be done in other places too, like when a new attach
> is made? Patch 5 removed it, but should that be made if
> domain->force_snooping?

Yes. I missed this. Will take care of this in the next version.

> 
> Jason

Best regards,
baolu

Re: [PATCH 3/5] iommu/vt-d: Check domain force_snooping against attached devices

[Jacob Pan]

Hi BaoLu,

On Sun, 1 May 2022 19:24:32 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> As domain->force_snooping only impacts the devices attached with the
> domain, there's no need to check against all IOMMU units. At the same
> time, for a brand new domain (hasn't been attached to any device), the
> force_snooping field could be set, but the attach_dev callback will
> return failure if it wants to attach to a device which IOMMU has no
> snoop control capability.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/pasid.h |  2 ++
>  drivers/iommu/intel/iommu.c | 50 ++++++++++++++++++++++++++++++++++++-
>  drivers/iommu/intel/pasid.c | 18 +++++++++++++
>  3 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
> index ab4408c824a5..583ea67fc783 100644
> --- a/drivers/iommu/intel/pasid.h
> +++ b/drivers/iommu/intel/pasid.h
> @@ -123,4 +123,6 @@ void intel_pasid_tear_down_entry(struct intel_iommu
> *iommu, bool fault_ignore);
>  int vcmd_alloc_pasid(struct intel_iommu *iommu, u32 *pasid);
>  void vcmd_free_pasid(struct intel_iommu *iommu, u32 pasid);
> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
> +					  struct device *dev, u32 pasid);
>  #endif /* __INTEL_PASID_H */
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 98050943d863..3c1c228f9031 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4554,13 +4554,61 @@ static phys_addr_t
> intel_iommu_iova_to_phys(struct iommu_domain *domain, return phys;
>  }
>  
> +static bool domain_support_force_snooping(struct dmar_domain *domain)
> +{
> +	struct device_domain_info *info;
> +	unsigned long flags;
> +	bool support = true;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	if (list_empty(&domain->devices))
> +		goto out;
> +
> +	list_for_each_entry(info, &domain->devices, link) {
> +		if (!ecap_sc_support(info->iommu->ecap)) {
> +			support = false;
> +			break;
> +		}
> +	}
why not just check the flag dmar_domain->force_snooping? devices wouldn't
be able to attach if !ecap_sc, right?

> +out:
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +	return support;
> +}
> +
> +static void domain_set_force_snooping(struct dmar_domain *domain)
> +{
> +	struct device_domain_info *info;
> +	unsigned long flags;
> +
> +	/*
> +	 * Second level page table supports per-PTE snoop control. The
> +	 * iommu_map() interface will handle this by setting SNP bit.
> +	 */
> +	if (!domain_use_first_level(domain))
> +		return;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	if (list_empty(&domain->devices))
> +		goto out_unlock;
> +
> +	list_for_each_entry(info, &domain->devices, link)
> +		intel_pasid_setup_page_snoop_control(info->iommu,
> info->dev,
> +						     PASID_RID2PASID);
> +
I guess other DMA API PASIDs need to have sc bit set as well. I will keep
this in mind for my DMA API PASID patch.

> +out_unlock:
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +}
> +
>  static bool intel_iommu_enforce_cache_coherency(struct iommu_domain
> *domain) {
>  	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>  
> -	if (!domain_update_iommu_snooping(NULL))
> +	if (!domain_support_force_snooping(dmar_domain))
>  		return false;
> +
> +	domain_set_force_snooping(dmar_domain);
>  	dmar_domain->force_snooping = true;
> +
nit: spurious change
>  	return true;
>  }
>  
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index f8d215d85695..815c744e6a34 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -762,3 +762,21 @@ int intel_pasid_setup_pass_through(struct
> intel_iommu *iommu, 
>  	return 0;
>  }
> +
> +/*
> + * Set the page snoop control for a pasid entry which has been set up.
> + */
> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
> +					  struct device *dev, u32 pasid)
> +{
> +	struct pasid_entry *pte;
> +	u16 did;
> +
> +	pte = intel_pasid_get_entry(dev, pasid);
> +	if (WARN_ON(!pte || !pasid_pte_is_present(pte)))
> +		return;
> +
> +	pasid_set_pgsnp(pte);
> +	did = pasid_get_domain_id(pte);
> +	pasid_flush_caches(iommu, pte, pasid, did);
> +}


Thanks,

Jacob

Re: [PATCH 3/5] iommu/vt-d: Check domain force_snooping against attached devices

[Baolu Lu]

On 2022/5/3 05:31, Jacob Pan wrote:
> Hi BaoLu,

Hi Jacob,

> 
> On Sun, 1 May 2022 19:24:32 +0800, Lu Baolu <baolu.lu@linux.intel.com>
> wrote:
> 
>> As domain->force_snooping only impacts the devices attached with the
>> domain, there's no need to check against all IOMMU units. At the same
>> time, for a brand new domain (hasn't been attached to any device), the
>> force_snooping field could be set, but the attach_dev callback will
>> return failure if it wants to attach to a device which IOMMU has no
>> snoop control capability.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/pasid.h |  2 ++
>>   drivers/iommu/intel/iommu.c | 50 ++++++++++++++++++++++++++++++++++++-
>>   drivers/iommu/intel/pasid.c | 18 +++++++++++++
>>   3 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
>> index ab4408c824a5..583ea67fc783 100644
>> --- a/drivers/iommu/intel/pasid.h
>> +++ b/drivers/iommu/intel/pasid.h
>> @@ -123,4 +123,6 @@ void intel_pasid_tear_down_entry(struct intel_iommu
>> *iommu, bool fault_ignore);
>>   int vcmd_alloc_pasid(struct intel_iommu *iommu, u32 *pasid);
>>   void vcmd_free_pasid(struct intel_iommu *iommu, u32 pasid);
>> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
>> +					  struct device *dev, u32 pasid);
>>   #endif /* __INTEL_PASID_H */
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 98050943d863..3c1c228f9031 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4554,13 +4554,61 @@ static phys_addr_t
>> intel_iommu_iova_to_phys(struct iommu_domain *domain, return phys;
>>   }
>>   
>> +static bool domain_support_force_snooping(struct dmar_domain *domain)
>> +{
>> +	struct device_domain_info *info;
>> +	unsigned long flags;
>> +	bool support = true;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	if (list_empty(&domain->devices))
>> +		goto out;
>> +
>> +	list_for_each_entry(info, &domain->devices, link) {
>> +		if (!ecap_sc_support(info->iommu->ecap)) {
>> +			support = false;
>> +			break;
>> +		}
>> +	}
> why not just check the flag dmar_domain->force_snooping? devices wouldn't
> be able to attach if !ecap_sc, right?

I should check "dmar_domain->force_snooping" first. If this is the first
time that this flag is about to set, then check the capabilities.

> 
>> +out:
>> +	spin_unlock_irqrestore(&device_domain_lock, flags);
>> +	return support;
>> +}
>> +
>> +static void domain_set_force_snooping(struct dmar_domain *domain)
>> +{
>> +	struct device_domain_info *info;
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * Second level page table supports per-PTE snoop control. The
>> +	 * iommu_map() interface will handle this by setting SNP bit.
>> +	 */
>> +	if (!domain_use_first_level(domain))
>> +		return;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	if (list_empty(&domain->devices))
>> +		goto out_unlock;
>> +
>> +	list_for_each_entry(info, &domain->devices, link)
>> +		intel_pasid_setup_page_snoop_control(info->iommu,
>> info->dev,
>> +						     PASID_RID2PASID);
>> +
> I guess other DMA API PASIDs need to have sc bit set as well. I will keep
> this in mind for my DMA API PASID patch.

Kernel DMA don't need to set the PGSNP bit. The x86 arch is always DMA
coherent. The force snooping is only needed when the device is
controlled by user space, but the VMM is optimized not to support the
virtualization of the wbinv instruction.

> 
>> +out_unlock:
>> +	spin_unlock_irqrestore(&device_domain_lock, flags);
>> +}
>> +
>>   static bool intel_iommu_enforce_cache_coherency(struct iommu_domain
>> *domain) {
>>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>>   
>> -	if (!domain_update_iommu_snooping(NULL))
>> +	if (!domain_support_force_snooping(dmar_domain))
>>   		return false;
>> +
>> +	domain_set_force_snooping(dmar_domain);
>>   	dmar_domain->force_snooping = true;
>> +
> nit: spurious change

I expect a blank line before return in the end.

>>   	return true;
>>   }
>>   
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index f8d215d85695..815c744e6a34 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -762,3 +762,21 @@ int intel_pasid_setup_pass_through(struct
>> intel_iommu *iommu,
>>   	return 0;
>>   }
>> +
>> +/*
>> + * Set the page snoop control for a pasid entry which has been set up.
>> + */
>> +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
>> +					  struct device *dev, u32 pasid)
>> +{
>> +	struct pasid_entry *pte;
>> +	u16 did;
>> +
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	if (WARN_ON(!pte || !pasid_pte_is_present(pte)))
>> +		return;
>> +
>> +	pasid_set_pgsnp(pte);
>> +	did = pasid_get_domain_id(pte);
>> +	pasid_flush_caches(iommu, pte, pasid, did);
>> +}
> 
> 
> Thanks,
> 
> Jacob

Best regards,
baolu

[PATCH 4/5] iommu/vt-d: Remove domain_update_iommu_snooping()

[Lu Baolu]

The IOMMU force snooping capability is not required to be consistent
among all the IOMMUs anymore. Remove force snooping capability check
in the IOMMU hot-add path and domain_update_iommu_snooping() becomes
a dead code now.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3c1c228f9031..d5808495eb64 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -533,33 +533,6 @@ static void domain_update_iommu_coherency(struct dmar_domain *domain)
 	rcu_read_unlock();
 }
 
-static bool domain_update_iommu_snooping(struct intel_iommu *skip)
-{
-	struct dmar_drhd_unit *drhd;
-	struct intel_iommu *iommu;
-	bool ret = true;
-
-	rcu_read_lock();
-	for_each_active_iommu(iommu, drhd) {
-		if (iommu != skip) {
-			/*
-			 * If the hardware is operating in the scalable mode,
-			 * the snooping control is always supported since we
-			 * always set PASID-table-entry.PGSNP bit if the domain
-			 * is managed outside (UNMANAGED).
-			 */
-			if (!sm_supported(iommu) &&
-			    !ecap_sc_support(iommu->ecap)) {
-				ret = false;
-				break;
-			}
-		}
-	}
-	rcu_read_unlock();
-
-	return ret;
-}
-
 static int domain_update_iommu_superpage(struct dmar_domain *domain,
 					 struct intel_iommu *skip)
 {
@@ -3593,12 +3566,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 			iommu->name);
 		return -ENXIO;
 	}
-	if (!ecap_sc_support(iommu->ecap) &&
-	    domain_update_iommu_snooping(iommu)) {
-		pr_warn("%s: Doesn't support snooping.\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",
-- 
2.25.1

Re: [PATCH 4/5] iommu/vt-d: Remove domain_update_iommu_snooping()

[Jason Gunthorpe]

On Sun, May 01, 2022 at 07:24:33PM +0800, Lu Baolu wrote:
> The IOMMU force snooping capability is not required to be consistent
> among all the IOMMUs anymore. Remove force snooping capability check
> in the IOMMU hot-add path and domain_update_iommu_snooping() becomes
> a dead code now.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 34 +---------------------------------
>  1 file changed, 1 insertion(+), 33 deletions(-)

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

Jason

Re: [PATCH 4/5] iommu/vt-d: Remove domain_update_iommu_snooping()

[Jacob Pan]

Hi BaoLu,

On Sun, 1 May 2022 19:24:33 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> The IOMMU force snooping capability is not required to be consistent
> among all the IOMMUs anymore. Remove force snooping capability check
> in the IOMMU hot-add path and domain_update_iommu_snooping() becomes
> a dead code now.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 34 +---------------------------------
>  1 file changed, 1 insertion(+), 33 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 3c1c228f9031..d5808495eb64 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -533,33 +533,6 @@ static void domain_update_iommu_coherency(struct
> dmar_domain *domain) rcu_read_unlock();
>  }
>  
> -static bool domain_update_iommu_snooping(struct intel_iommu *skip)
> -{
> -	struct dmar_drhd_unit *drhd;
> -	struct intel_iommu *iommu;
> -	bool ret = true;
> -
> -	rcu_read_lock();
> -	for_each_active_iommu(iommu, drhd) {
> -		if (iommu != skip) {
> -			/*
> -			 * If the hardware is operating in the scalable
> mode,
> -			 * the snooping control is always supported
> since we
> -			 * always set PASID-table-entry.PGSNP bit if the
> domain
> -			 * is managed outside (UNMANAGED).
> -			 */
> -			if (!sm_supported(iommu) &&
> -			    !ecap_sc_support(iommu->ecap)) {
> -				ret = false;
> -				break;
> -			}
> -		}
> -	}
> -	rcu_read_unlock();
> -
> -	return ret;
> -}
> -
>  static int domain_update_iommu_superpage(struct dmar_domain *domain,
>  					 struct intel_iommu *skip)
>  {
> @@ -3593,12 +3566,7 @@ static int intel_iommu_add(struct dmar_drhd_unit
> *dmaru) iommu->name);
>  		return -ENXIO;
>  	}
> -	if (!ecap_sc_support(iommu->ecap) &&
> -	    domain_update_iommu_snooping(iommu)) {
> -		pr_warn("%s: Doesn't support snooping.\n",
> -			iommu->name);
> -		return -ENXIO;
> -	}
> +
Maybe I missed earlier patches, so this bit can also be deleted?

struct dmar_domain {
	u8 iommu_snooping: 1;		/* indicate snooping control
feature */

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


Thanks,

Jacob

Re: [PATCH 4/5] iommu/vt-d: Remove domain_update_iommu_snooping()

[Baolu Lu]

On 2022/5/3 05:36, Jacob Pan wrote:
> Hi BaoLu,
> 
> On Sun, 1 May 2022 19:24:33 +0800, Lu Baolu <baolu.lu@linux.intel.com>
> wrote:
> 
>> The IOMMU force snooping capability is not required to be consistent
>> among all the IOMMUs anymore. Remove force snooping capability check
>> in the IOMMU hot-add path and domain_update_iommu_snooping() becomes
>> a dead code now.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 34 +---------------------------------
>>   1 file changed, 1 insertion(+), 33 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 3c1c228f9031..d5808495eb64 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -533,33 +533,6 @@ static void domain_update_iommu_coherency(struct
>> dmar_domain *domain) rcu_read_unlock();
>>   }
>>   
>> -static bool domain_update_iommu_snooping(struct intel_iommu *skip)
>> -{
>> -	struct dmar_drhd_unit *drhd;
>> -	struct intel_iommu *iommu;
>> -	bool ret = true;
>> -
>> -	rcu_read_lock();
>> -	for_each_active_iommu(iommu, drhd) {
>> -		if (iommu != skip) {
>> -			/*
>> -			 * If the hardware is operating in the scalable
>> mode,
>> -			 * the snooping control is always supported
>> since we
>> -			 * always set PASID-table-entry.PGSNP bit if the
>> domain
>> -			 * is managed outside (UNMANAGED).
>> -			 */
>> -			if (!sm_supported(iommu) &&
>> -			    !ecap_sc_support(iommu->ecap)) {
>> -				ret = false;
>> -				break;
>> -			}
>> -		}
>> -	}
>> -	rcu_read_unlock();
>> -
>> -	return ret;
>> -}
>> -
>>   static int domain_update_iommu_superpage(struct dmar_domain *domain,
>>   					 struct intel_iommu *skip)
>>   {
>> @@ -3593,12 +3566,7 @@ static int intel_iommu_add(struct dmar_drhd_unit
>> *dmaru) iommu->name);
>>   		return -ENXIO;
>>   	}
>> -	if (!ecap_sc_support(iommu->ecap) &&
>> -	    domain_update_iommu_snooping(iommu)) {
>> -		pr_warn("%s: Doesn't support snooping.\n",
>> -			iommu->name);
>> -		return -ENXIO;
>> -	}
>> +
> Maybe I missed earlier patches, so this bit can also be deleted?
> 
> struct dmar_domain {
> 	u8 iommu_snooping: 1;		/* indicate snooping control
> feature */

It has been cleaned up by below commit:

71cfafda9c9b vfio: Move the Intel no-snoop control off of IOMMU_CACHE

> 
>>   	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",
> 
> 
> Thanks,
> 
> Jacob

Best regards,
baolu

[PATCH 5/5] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries

[Lu Baolu]

As enforce_cache_coherency has been introduced into the iommu_domain_ops,
the kernel component which owns the iommu domain is able to opt-in its
requirement for force snooping support. The iommu driver has no need to
hard code the page snoop control bit in the PASID table entries anymore.

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

diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 583ea67fc783..394e6284ce1f 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -48,7 +48,6 @@
  */
 #define PASID_FLAG_SUPERVISOR_MODE	BIT(0)
 #define PASID_FLAG_NESTED		BIT(1)
-#define PASID_FLAG_PAGE_SNOOP		BIT(2)
 
 /*
  * The PASID_FLAG_FL5LP flag Indicates using 5-level paging for first-
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d5808495eb64..edd3d940eb25 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2432,9 +2432,6 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
 	if (level == 5)
 		flags |= PASID_FLAG_FL5LP;
 
-	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
-		flags |= PASID_FLAG_PAGE_SNOOP;
-
 	return intel_pasid_setup_first_level(iommu, dev, (pgd_t *)pgd, pasid,
 					     domain->iommu_did[iommu->seq_id],
 					     flags);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 815c744e6a34..dc5020320323 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -625,9 +625,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		}
 	}
 
-	if (flags & PASID_FLAG_PAGE_SNOOP)
-		pasid_set_pgsnp(pte);
-
 	pasid_set_domain_id(pte, did);
 	pasid_set_address_width(pte, iommu->agaw);
 	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
@@ -710,9 +707,6 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 	pasid_set_fault_enable(pte);
 	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
 
-	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
-		pasid_set_pgsnp(pte);
-
 	/*
 	 * Since it is a second level only translation setup, we should
 	 * set SRE bit as well (addresses are expected to be GPAs).
-- 
2.25.1

Re: [PATCH 5/5] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries

[Jason Gunthorpe]

On Sun, May 01, 2022 at 07:24:34PM +0800, Lu Baolu wrote:
> As enforce_cache_coherency has been introduced into the iommu_domain_ops,
> the kernel component which owns the iommu domain is able to opt-in its
> requirement for force snooping support. The iommu driver has no need to
> hard code the page snoop control bit in the PASID table entries anymore.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/pasid.h | 1 -
>  drivers/iommu/intel/iommu.c | 3 ---
>  drivers/iommu/intel/pasid.c | 6 ------
>  3 files changed, 10 deletions(-)

It seems fine, but as in the other email where do we do
pasid_set_pgsnp() for a new device attach on an already no-snopp domain?

Jason

Re: [PATCH 5/5] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries

[Baolu Lu]

On 2022/5/2 21:19, Jason Gunthorpe wrote:
> On Sun, May 01, 2022 at 07:24:34PM +0800, Lu Baolu wrote:
>> As enforce_cache_coherency has been introduced into the iommu_domain_ops,
>> the kernel component which owns the iommu domain is able to opt-in its
>> requirement for force snooping support. The iommu driver has no need to
>> hard code the page snoop control bit in the PASID table entries anymore.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/pasid.h | 1 -
>>   drivers/iommu/intel/iommu.c | 3 ---
>>   drivers/iommu/intel/pasid.c | 6 ------
>>   3 files changed, 10 deletions(-)
> 
> It seems fine, but as in the other email where do we do
> pasid_set_pgsnp() for a new device attach on an already no-snopp domain?

Yes. I will take care of this in the next version.

> Jason

Best regards,
baolu