linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] iommu/vt-d: Force snooping improvement
@ 2022-05-05  1:07 Lu Baolu
  2022-05-05  1:07 ` [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support Lu Baolu
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Lu Baolu @ 2022-05-05  1:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe, Alex Williamson, Kevin Tian
  Cc: Jacob jun Pan, Liu Yi L, iommu, linux-kernel, Lu Baolu

Hi folks,

Previously, the IOMMU capability of enforcing cache coherency was 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

Change log:
v2:
 - Check whether force_snooping has already been set in
   intel_iommu_enforce_cache_coherency().
 - Set PGSNP pasid bit field during domain attaching if forcing_snooping
   is set.
 - Remove redundant list_empty() checks.
 - Add dmar_domain->set_pte_snp and set it if force snooping is enforced
   on a domain with 2nd-level translation.

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

Lu Baolu (4):
  iommu/vt-d: Block force-snoop domain attaching if no SC support
  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

 include/linux/intel-iommu.h |  1 +
 drivers/iommu/intel/pasid.h |  2 +
 drivers/iommu/intel/iommu.c | 88 ++++++++++++++++++++++---------------
 drivers/iommu/intel/pasid.c | 26 +++++++++--
 4 files changed, 78 insertions(+), 39 deletions(-)

-- 
2.25.1


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

* [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support
  2022-05-05  1:07 [PATCH v2 0/4] iommu/vt-d: Force snooping improvement Lu Baolu
@ 2022-05-05  1:07 ` Lu Baolu
  2022-05-05  8:34   ` Tian, Kevin
  2022-05-05  1:07 ` [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices Lu Baolu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Lu Baolu @ 2022-05-05  1:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe, Alex Williamson, Kevin Tian
  Cc: Jacob jun Pan, Liu Yi L, iommu, linux-kernel, 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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.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


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

* [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices
  2022-05-05  1:07 [PATCH v2 0/4] iommu/vt-d: Force snooping improvement Lu Baolu
  2022-05-05  1:07 ` [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support Lu Baolu
@ 2022-05-05  1:07 ` Lu Baolu
  2022-05-05  8:43   ` Tian, Kevin
  2022-05-05  1:07 ` [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping() Lu Baolu
  2022-05-05  1:07 ` [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries Lu Baolu
  3 siblings, 1 reply; 11+ messages in thread
From: Lu Baolu @ 2022-05-05  1:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe, Alex Williamson, Kevin Tian
  Cc: Jacob jun Pan, Liu Yi L, iommu, linux-kernel, 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>
---
 include/linux/intel-iommu.h |  1 +
 drivers/iommu/intel/pasid.h |  2 ++
 drivers/iommu/intel/iommu.c | 51 ++++++++++++++++++++++++++++++++++---
 drivers/iommu/intel/pasid.c | 23 +++++++++++++++++
 4 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 72e5d7900e71..4f29139bbfc3 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -540,6 +540,7 @@ struct dmar_domain {
 	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;
 
 	struct list_head devices;	/* all devices' list */
 	struct iova_domain iovad;	/* iova's that belong to this domain */
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 d68f5bbf3e93..98112228ae93 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2459,7 +2459,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
 	if (level == 5)
 		flags |= PASID_FLAG_FL5LP;
 
-	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
+	if (domain->force_snooping)
 		flags |= PASID_FLAG_PAGE_SNOOP;
 
 	return intel_pasid_setup_first_level(iommu, dev, (pgd_t *)pgd, pasid,
@@ -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->set_pte_snp)
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
@@ -4554,13 +4554,58 @@ 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);
+	list_for_each_entry(info, &domain->devices, link) {
+		if (!ecap_sc_support(info->iommu->ecap)) {
+			support = false;
+			break;
+		}
+	}
+	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)) {
+		domain->set_pte_snp = true;
+		return;
+	}
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_for_each_entry(info, &domain->devices, link)
+		intel_pasid_setup_page_snoop_control(info->iommu, info->dev,
+						     PASID_RID2PASID);
+	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 (dmar_domain->force_snooping)
+		return true;
+
+	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..41a0e3b02c79 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -762,3 +762,26 @@ 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;
+
+	spin_lock(&iommu->lock);
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (WARN_ON(!pte || !pasid_pte_is_present(pte))) {
+		spin_unlock(&iommu->lock);
+		return;
+	}
+
+	pasid_set_pgsnp(pte);
+	did = pasid_get_domain_id(pte);
+	spin_unlock(&iommu->lock);
+
+	pasid_flush_caches(iommu, pte, pasid, did);
+}
-- 
2.25.1


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

* [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping()
  2022-05-05  1:07 [PATCH v2 0/4] iommu/vt-d: Force snooping improvement Lu Baolu
  2022-05-05  1:07 ` [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support Lu Baolu
  2022-05-05  1:07 ` [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices Lu Baolu
@ 2022-05-05  1:07 ` Lu Baolu
  2022-05-05  8:44   ` Tian, Kevin
  2022-05-05  1:07 ` [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries Lu Baolu
  3 siblings, 1 reply; 11+ messages in thread
From: Lu Baolu @ 2022-05-05  1:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe, Alex Williamson, Kevin Tian
  Cc: Jacob jun Pan, Liu Yi L, iommu, linux-kernel, 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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.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 98112228ae93..da4bfb34ae4b 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


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

* [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries
  2022-05-05  1:07 [PATCH v2 0/4] iommu/vt-d: Force snooping improvement Lu Baolu
                   ` (2 preceding siblings ...)
  2022-05-05  1:07 ` [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping() Lu Baolu
@ 2022-05-05  1:07 ` Lu Baolu
  2022-05-05  8:46   ` Tian, Kevin
  3 siblings, 1 reply; 11+ messages in thread
From: Lu Baolu @ 2022-05-05  1:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe, Alex Williamson, Kevin Tian
  Cc: Jacob jun Pan, Liu Yi L, iommu, linux-kernel, 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.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 41a0e3b02c79..0abfa7fc7fb0 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -710,9 +710,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


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

* RE: [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support
  2022-05-05  1:07 ` [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support Lu Baolu
@ 2022-05-05  8:34   ` Tian, Kevin
  0 siblings, 0 replies; 11+ messages in thread
From: Tian, Kevin @ 2022-05-05  8:34 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, May 5, 2022 9:07 AM
> 
> 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>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@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


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

* RE: [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices
  2022-05-05  1:07 ` [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices Lu Baolu
@ 2022-05-05  8:43   ` Tian, Kevin
  2022-05-05 12:12     ` Baolu Lu
  0 siblings, 1 reply; 11+ messages in thread
From: Tian, Kevin @ 2022-05-05  8:43 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, May 5, 2022 9:07 AM
> 
> 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.

The description about brand new domain is not very clear. I think the
point here is that force_snooping could be set on a domain no matter
whether it has been attached or not and once set it is an immutable
flag. If no device attached the operation always succeeds then this
empty domain can be only attached to a device of which the IOMMU
supports snoop control.

>  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 (dmar_domain->force_snooping)
> +		return true;
> +
> +	if (!domain_support_force_snooping(dmar_domain))
>  		return false;
> +

Who guarantees that domain->devices won't change between
above condition check and following set operation?

> +	domain_set_force_snooping(dmar_domain);
>  	dmar_domain->force_snooping = true;
> +
>  	return true;
>  }
> 

Thanks
Kevin

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

* RE: [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping()
  2022-05-05  1:07 ` [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping() Lu Baolu
@ 2022-05-05  8:44   ` Tian, Kevin
  0 siblings, 0 replies; 11+ messages in thread
From: Tian, Kevin @ 2022-05-05  8:44 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, May 5, 2022 9:07 AM
> 
> 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>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@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 98112228ae93..da4bfb34ae4b 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


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

* RE: [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries
  2022-05-05  1:07 ` [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries Lu Baolu
@ 2022-05-05  8:46   ` Tian, Kevin
  2022-05-05 12:14     ` Baolu Lu
  0 siblings, 1 reply; 11+ messages in thread
From: Tian, Kevin @ 2022-05-05  8:46 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, May 5, 2022 9:07 AM
> 
> 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>

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

> ---
>  drivers/iommu/intel/pasid.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 41a0e3b02c79..0abfa7fc7fb0 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -710,9 +710,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));

Probably in a separate patch but above should really be renamed
to pasid_set_page_walk_snoop().

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


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

* Re: [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices
  2022-05-05  8:43   ` Tian, Kevin
@ 2022-05-05 12:12     ` Baolu Lu
  0 siblings, 0 replies; 11+ messages in thread
From: Baolu Lu @ 2022-05-05 12:12 UTC (permalink / raw)
  To: Tian, Kevin, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

On 2022/5/5 16:43, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, May 5, 2022 9:07 AM
>>
>> 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.
> 
> The description about brand new domain is not very clear. I think the
> point here is that force_snooping could be set on a domain no matter
> whether it has been attached or not and once set it is an immutable
> flag. If no device attached the operation always succeeds then this
> empty domain can be only attached to a device of which the IOMMU
> supports snoop control.

Exactly. Will update this description.

> 
>>   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 (dmar_domain->force_snooping)
>> +		return true;
>> +
>> +	if (!domain_support_force_snooping(dmar_domain))
>>   		return false;
>> +
> 
> Who guarantees that domain->devices won't change between
> above condition check and following set operation?

Good catch. Should lift the lock up here.

> 
>> +	domain_set_force_snooping(dmar_domain);
>>   	dmar_domain->force_snooping = true;
>> +
>>   	return true;
>>   }
>>
> 
> Thanks
> Kevin

Best regards,
baolu

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

* Re: [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries
  2022-05-05  8:46   ` Tian, Kevin
@ 2022-05-05 12:14     ` Baolu Lu
  0 siblings, 0 replies; 11+ messages in thread
From: Baolu Lu @ 2022-05-05 12:14 UTC (permalink / raw)
  To: Tian, Kevin, Joerg Roedel, Jason Gunthorpe, Alex Williamson
  Cc: Pan, Jacob jun, Liu, Yi L, iommu, linux-kernel

On 2022/5/5 16:46, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, May 5, 2022 9:07 AM
>>
>> 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>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:
> 
>> ---
>>   drivers/iommu/intel/pasid.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 41a0e3b02c79..0abfa7fc7fb0 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -710,9 +710,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));
> 
> Probably in a separate patch but above should really be renamed
> to pasid_set_page_walk_snoop().

Yeah! Need a cleanup here. Above name is confusing.

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

Best regards,
baolu

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

end of thread, other threads:[~2022-05-05 12:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-05  1:07 [PATCH v2 0/4] iommu/vt-d: Force snooping improvement Lu Baolu
2022-05-05  1:07 ` [PATCH v2 1/4] iommu/vt-d: Block force-snoop domain attaching if no SC support Lu Baolu
2022-05-05  8:34   ` Tian, Kevin
2022-05-05  1:07 ` [PATCH v2 2/4] iommu/vt-d: Check domain force_snooping against attached devices Lu Baolu
2022-05-05  8:43   ` Tian, Kevin
2022-05-05 12:12     ` Baolu Lu
2022-05-05  1:07 ` [PATCH v2 3/4] iommu/vt-d: Remove domain_update_iommu_snooping() Lu Baolu
2022-05-05  8:44   ` Tian, Kevin
2022-05-05  1:07 ` [PATCH v2 4/4] iommu/vt-d: Remove hard coding PGSNP bit in PASID entries Lu Baolu
2022-05-05  8:46   ` Tian, Kevin
2022-05-05 12:14     ` Baolu Lu

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