[PATCH v2 0/4] iommu/vt-d: Some fine tuning of SVA

[PATCH v2 0/4] iommu/vt-d: Some fine tuning of SVA

[Lu Baolu]

Hi folks,

This includes several tunings of Intel SVA implementation. I plan to
target them for v5.19. Please help to review.

Best regards,
baolu

Change log:

v2:
 - Move snoop control capability check into a separated patch.
 - Return false if the caller opt-in setting PGSNP with a flag.
 - Add a Fixes tag for "iommu/vt-d: Drop stop marker messages" as it is
   required by the iopf framework.

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

Lu Baolu (4):
  iommu/vt-d: Check before setting PGSNP bit in pasid table entry
  iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding
  iommu/vt-d: Drop stop marker messages
  iommu/vt-d: Size Page Request Queue to avoid overflow condition

 include/linux/intel-svm.h   |  2 +-
 drivers/iommu/intel/pasid.c | 13 ++++++++++---
 drivers/iommu/intel/svm.c   | 13 ++++++++++---
 3 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 0/4] iommu/vt-d: Some fine tuning of SVA

[Lu Baolu]

Hi folks,

This includes several tunings of Intel SVA implementation. I plan to
target them for v5.19. Please help to review.

Best regards,
baolu

Change log:

v2:
 - Move snoop control capability check into a separated patch.
 - Return false if the caller opt-in setting PGSNP with a flag.
 - Add a Fixes tag for "iommu/vt-d: Drop stop marker messages" as it is
   required by the iopf framework.

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

Lu Baolu (4):
  iommu/vt-d: Check before setting PGSNP bit in pasid table entry
  iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding
  iommu/vt-d: Drop stop marker messages
  iommu/vt-d: Size Page Request Queue to avoid overflow condition

 include/linux/intel-svm.h   |  2 +-
 drivers/iommu/intel/pasid.c | 13 ++++++++++---
 drivers/iommu/intel/svm.c   | 13 ++++++++++---
 3 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

The latest VT-d specification states that the PGSNP field in the pasid
table entry should be treated as Reserved(0) for implementations not
supporting Snoop Control (SC=0 in the Extended Capability Register).
This adds a check before setting the field.

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

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index f8d215d85695..5cb2daa2b8cb 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		}
 	}
 
-	if (flags & PASID_FLAG_PAGE_SNOOP)
-		pasid_set_pgsnp(pte);
+	if (flags & PASID_FLAG_PAGE_SNOOP) {
+		if (ecap_sc_support(iommu->ecap)) {
+			pasid_set_pgsnp(pte);
+		} else {
+			pasid_clear_entry(pte);
+			return -EINVAL;
+		}
+	}
 
 	pasid_set_domain_id(pte, did);
 	pasid_set_address_width(pte, iommu->agaw);
@@ -710,7 +716,8 @@ 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)
+	if (ecap_sc_support(iommu->ecap) &&
+	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
 		pasid_set_pgsnp(pte);
 
 	/*
-- 
2.25.1

[PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

The latest VT-d specification states that the PGSNP field in the pasid
table entry should be treated as Reserved(0) for implementations not
supporting Snoop Control (SC=0 in the Extended Capability Register).
This adds a check before setting the field.

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

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index f8d215d85695..5cb2daa2b8cb 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		}
 	}
 
-	if (flags & PASID_FLAG_PAGE_SNOOP)
-		pasid_set_pgsnp(pte);
+	if (flags & PASID_FLAG_PAGE_SNOOP) {
+		if (ecap_sc_support(iommu->ecap)) {
+			pasid_set_pgsnp(pte);
+		} else {
+			pasid_clear_entry(pte);
+			return -EINVAL;
+		}
+	}
 
 	pasid_set_domain_id(pte, did);
 	pasid_set_address_width(pte, iommu->agaw);
@@ -710,7 +716,8 @@ 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)
+	if (ecap_sc_support(iommu->ecap) &&
+	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
 		pasid_set_pgsnp(pte);
 
 	/*
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Lu Baolu]

This field make the requests snoop processor caches irrespective of
other attributes in the request or other fields in paging structure
entries used to translate the request.

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

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a38763c1d1..c720d1be992d 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -391,9 +391,12 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
 	}
 
 	/* Setup the pasid table: */
-	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
-			PASID_FLAG_SUPERVISOR_MODE : 0;
-	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
+	sflags = PASID_FLAG_PAGE_SNOOP;
+	if (flags & SVM_FLAG_SUPERVISOR_MODE)
+		sflags |= PASID_FLAG_SUPERVISOR_MODE;
+	if (cpu_feature_enabled(X86_FEATURE_LA57))
+		sflags |= PASID_FLAG_FL5LP;
+
 	spin_lock_irqsave(&iommu->lock, iflags);
 	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm->pasid,
 					    FLPT_DEFAULT_DID, sflags);
-- 
2.25.1

[PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Lu Baolu]

This field make the requests snoop processor caches irrespective of
other attributes in the request or other fields in paging structure
entries used to translate the request.

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

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a38763c1d1..c720d1be992d 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -391,9 +391,12 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
 	}
 
 	/* Setup the pasid table: */
-	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
-			PASID_FLAG_SUPERVISOR_MODE : 0;
-	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
+	sflags = PASID_FLAG_PAGE_SNOOP;
+	if (flags & SVM_FLAG_SUPERVISOR_MODE)
+		sflags |= PASID_FLAG_SUPERVISOR_MODE;
+	if (cpu_feature_enabled(X86_FEATURE_LA57))
+		sflags |= PASID_FLAG_FL5LP;
+
 	spin_lock_irqsave(&iommu->lock, iflags);
 	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm->pasid,
 					    FLPT_DEFAULT_DID, sflags);
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Lu Baolu]

The page fault handling framework in the IOMMU core explicitly states
that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
discard them before reporting faults. This handles Stop Marker messages
in prq_event_thread() before reporting events to the core.

The VT-d driver explicitly drains the pending page requests when a CPU
page table (represented by a mm struct) is unbound from a PASID according
to the procedures defined in the VT-d spec. The Stop Marker messages do
not need a response. Hence, it is safe to drop the Stop Marker messages
silently if any of them is found in the page request queue.

Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index c720d1be992d..0741ec165673 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -760,6 +760,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			goto bad_req;
 		}
 
+		/* Drop Stop Marker message. No need for a response. */
+		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
+			goto prq_advance;
+
 		if (!svm || svm->pasid != req->pasid) {
 			/*
 			 * It can't go away, because the driver is not permitted
-- 
2.25.1

[PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Lu Baolu]

The page fault handling framework in the IOMMU core explicitly states
that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
discard them before reporting faults. This handles Stop Marker messages
in prq_event_thread() before reporting events to the core.

The VT-d driver explicitly drains the pending page requests when a CPU
page table (represented by a mm struct) is unbound from a PASID according
to the procedures defined in the VT-d spec. The Stop Marker messages do
not need a response. Hence, it is safe to drop the Stop Marker messages
silently if any of them is found in the page request queue.

Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index c720d1be992d..0741ec165673 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -760,6 +760,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			goto bad_req;
 		}
 
+		/* Drop Stop Marker message. No need for a response. */
+		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
+			goto prq_advance;
+
 		if (!svm || svm->pasid != req->pasid) {
 			/*
 			 * It can't go away, because the driver is not permitted
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 4/4] iommu/vt-d: Size Page Request Queue to avoid overflow condition

[Lu Baolu]

PRQ overflow may cause I/O throughput congestion, resulting in unnecessary
degradation of I/O performance. Appropriately increasing the length of PRQ
can greatly reduce the occurrence of PRQ overflow. The count of maximum
page requests that can be generated in parallel by a PCIe device is
statically defined in the Outstanding Page Request Capacity field of the
PCIe ATS configure space.

The new length of PRQ is calculated by summing up the value of Outstanding
Page Request Capacity register across all devices where Page Requests are
supported on the real PR-capable platform (Intel Sapphire Rapids). The
result is round to the nearest higher power of 2.

The PRQ length is also double sized as the VT-d IOMMU driver only updates
the Page Request Queue Head Register (PQH_REG) after processing the entire
queue.

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

diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
index b3b125b332aa..207ef06ba3e1 100644
--- a/include/linux/intel-svm.h
+++ b/include/linux/intel-svm.h
@@ -9,7 +9,7 @@
 #define __INTEL_SVM_H__
 
 /* Page Request Queue depth */
-#define PRQ_ORDER	2
+#define PRQ_ORDER	4
 #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
 #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
 
-- 
2.25.1

[PATCH v2 4/4] iommu/vt-d: Size Page Request Queue to avoid overflow condition

[Lu Baolu]

PRQ overflow may cause I/O throughput congestion, resulting in unnecessary
degradation of I/O performance. Appropriately increasing the length of PRQ
can greatly reduce the occurrence of PRQ overflow. The count of maximum
page requests that can be generated in parallel by a PCIe device is
statically defined in the Outstanding Page Request Capacity field of the
PCIe ATS configure space.

The new length of PRQ is calculated by summing up the value of Outstanding
Page Request Capacity register across all devices where Page Requests are
supported on the real PR-capable platform (Intel Sapphire Rapids). The
result is round to the nearest higher power of 2.

The PRQ length is also double sized as the VT-d IOMMU driver only updates
the Page Request Queue Head Register (PQH_REG) after processing the entire
queue.

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

diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
index b3b125b332aa..207ef06ba3e1 100644
--- a/include/linux/intel-svm.h
+++ b/include/linux/intel-svm.h
@@ -9,7 +9,7 @@
 #define __INTEL_SVM_H__
 
 /* Page Request Queue depth */
-#define PRQ_ORDER	2
+#define PRQ_ORDER	4
 #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
 #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
 
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> The latest VT-d specification states that the PGSNP field in the pasid
> table entry should be treated as Reserved(0) for implementations not
> supporting Snoop Control (SC=0 in the Extended Capability Register).
> This adds a check before setting the field.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index f8d215d85695..5cb2daa2b8cb 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu
> *iommu,
>  		}
>  	}
> 
> -	if (flags & PASID_FLAG_PAGE_SNOOP)
> -		pasid_set_pgsnp(pte);
> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
> +		if (ecap_sc_support(iommu->ecap)) {
> +			pasid_set_pgsnp(pte);
> +		} else {
> +			pasid_clear_entry(pte);
> +			return -EINVAL;
> +		}
> +	}
> 
>  	pasid_set_domain_id(pte, did);
>  	pasid_set_address_width(pte, iommu->agaw);
> @@ -710,7 +716,8 @@ 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)
> +	if (ecap_sc_support(iommu->ecap) &&
> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>  		pasid_set_pgsnp(pte);
> 

This should be rebased on top of Jason's enforce coherency series
instead of blindly setting it. No matter whether it's legacy mode
where we set SNP in PTE or scalable mode where we set PGSNP
in PASID entry for entire page table, the trigger point should be
same i.e. when someone calls enforce_cache_coherency().

Thanks
Kevin

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> The latest VT-d specification states that the PGSNP field in the pasid
> table entry should be treated as Reserved(0) for implementations not
> supporting Snoop Control (SC=0 in the Extended Capability Register).
> This adds a check before setting the field.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index f8d215d85695..5cb2daa2b8cb 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu
> *iommu,
>  		}
>  	}
> 
> -	if (flags & PASID_FLAG_PAGE_SNOOP)
> -		pasid_set_pgsnp(pte);
> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
> +		if (ecap_sc_support(iommu->ecap)) {
> +			pasid_set_pgsnp(pte);
> +		} else {
> +			pasid_clear_entry(pte);
> +			return -EINVAL;
> +		}
> +	}
> 
>  	pasid_set_domain_id(pte, did);
>  	pasid_set_address_width(pte, iommu->agaw);
> @@ -710,7 +716,8 @@ 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)
> +	if (ecap_sc_support(iommu->ecap) &&
> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>  		pasid_set_pgsnp(pte);
> 

This should be rebased on top of Jason's enforce coherency series
instead of blindly setting it. No matter whether it's legacy mode
where we set SNP in PTE or scalable mode where we set PGSNP
in PASID entry for entire page table, the trigger point should be
same i.e. when someone calls enforce_cache_coherency().

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> This field make the requests snoop processor caches irrespective of
> other attributes in the request or other fields in paging structure
> entries used to translate the request.

I think you want to first point out the fact that SVA wants snoop
cache instead of just talking about the effect of PGSNP.

But thinking more I wonder why PGSNP is ever required. This is
similar to DMA API case. x86 is already cache coherent for normal
DMA (if not setting PCI no-snoop) and if the driver knows no-snoop
is incompatible to SVA API then it should avoid triggering no-snoop
traffic for SVA usage. In this case it is pointless for IOMMU driver
to enable force-snooping. Even in the future certain platform allows
no-snoop usage w/ SVA (I'm not sure how it works) this again should
be reflected by additional SVA APIs for driver to explicitly manage.

force-snoop should be enabled only in device assignment case IMHO,
orthogonal to whether vSVA is actually used.

Did I misunderstand the motivation here?

> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/svm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 23a38763c1d1..c720d1be992d 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -391,9 +391,12 @@ static struct iommu_sva
> *intel_svm_bind_mm(struct intel_iommu *iommu,
>  	}
> 
>  	/* Setup the pasid table: */
> -	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
> -			PASID_FLAG_SUPERVISOR_MODE : 0;
> -	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ?
> PASID_FLAG_FL5LP : 0;
> +	sflags = PASID_FLAG_PAGE_SNOOP;
> +	if (flags & SVM_FLAG_SUPERVISOR_MODE)
> +		sflags |= PASID_FLAG_SUPERVISOR_MODE;
> +	if (cpu_feature_enabled(X86_FEATURE_LA57))
> +		sflags |= PASID_FLAG_FL5LP;
> +
>  	spin_lock_irqsave(&iommu->lock, iflags);
>  	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm-
> >pasid,
>  					    FLPT_DEFAULT_DID, sflags);
> --
> 2.25.1

RE: [PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> This field make the requests snoop processor caches irrespective of
> other attributes in the request or other fields in paging structure
> entries used to translate the request.

I think you want to first point out the fact that SVA wants snoop
cache instead of just talking about the effect of PGSNP.

But thinking more I wonder why PGSNP is ever required. This is
similar to DMA API case. x86 is already cache coherent for normal
DMA (if not setting PCI no-snoop) and if the driver knows no-snoop
is incompatible to SVA API then it should avoid triggering no-snoop
traffic for SVA usage. In this case it is pointless for IOMMU driver
to enable force-snooping. Even in the future certain platform allows
no-snoop usage w/ SVA (I'm not sure how it works) this again should
be reflected by additional SVA APIs for driver to explicitly manage.

force-snoop should be enabled only in device assignment case IMHO,
orthogonal to whether vSVA is actually used.

Did I misunderstand the motivation here?

> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/svm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 23a38763c1d1..c720d1be992d 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -391,9 +391,12 @@ static struct iommu_sva
> *intel_svm_bind_mm(struct intel_iommu *iommu,
>  	}
> 
>  	/* Setup the pasid table: */
> -	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
> -			PASID_FLAG_SUPERVISOR_MODE : 0;
> -	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ?
> PASID_FLAG_FL5LP : 0;
> +	sflags = PASID_FLAG_PAGE_SNOOP;
> +	if (flags & SVM_FLAG_SUPERVISOR_MODE)
> +		sflags |= PASID_FLAG_SUPERVISOR_MODE;
> +	if (cpu_feature_enabled(X86_FEATURE_LA57))
> +		sflags |= PASID_FLAG_FL5LP;
> +
>  	spin_lock_irqsave(&iommu->lock, iflags);
>  	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm-
> >pasid,
>  					    FLPT_DEFAULT_DID, sflags);
> --
> 2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> The page fault handling framework in the IOMMU core explicitly states
> that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
> discard them before reporting faults. This handles Stop Marker messages
> in prq_event_thread() before reporting events to the core.
> 
> The VT-d driver explicitly drains the pending page requests when a CPU
> page table (represented by a mm struct) is unbound from a PASID according
> to the procedures defined in the VT-d spec. The Stop Marker messages do
> not need a response. Hence, it is safe to drop the Stop Marker messages
> silently if any of them is found in the page request queue.
> 
> Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

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

> ---
>  drivers/iommu/intel/svm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index c720d1be992d..0741ec165673 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -760,6 +760,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  			goto bad_req;
>  		}
> 
> +		/* Drop Stop Marker message. No need for a response. */
> +		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
> +			goto prq_advance;
> +
>  		if (!svm || svm->pasid != req->pasid) {
>  			/*
>  			 * It can't go away, because the driver is not
> permitted
> --
> 2.25.1

RE: [PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> The page fault handling framework in the IOMMU core explicitly states
> that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
> discard them before reporting faults. This handles Stop Marker messages
> in prq_event_thread() before reporting events to the core.
> 
> The VT-d driver explicitly drains the pending page requests when a CPU
> page table (represented by a mm struct) is unbound from a PASID according
> to the procedures defined in the VT-d spec. The Stop Marker messages do
> not need a response. Hence, it is safe to drop the Stop Marker messages
> silently if any of them is found in the page request queue.
> 
> Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

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

> ---
>  drivers/iommu/intel/svm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index c720d1be992d..0741ec165673 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -760,6 +760,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  			goto bad_req;
>  		}
> 
> +		/* Drop Stop Marker message. No need for a response. */
> +		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
> +			goto prq_advance;
> +
>  		if (!svm || svm->pasid != req->pasid) {
>  			/*
>  			 * It can't go away, because the driver is not
> permitted
> --
> 2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 4/4] iommu/vt-d: Size Page Request Queue to avoid overflow condition

[Tian, Kevin]

> From: Lu Baolu
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> PRQ overflow may cause I/O throughput congestion, resulting in unnecessary
> degradation of I/O performance. Appropriately increasing the length of PRQ
> can greatly reduce the occurrence of PRQ overflow. The count of maximum
> page requests that can be generated in parallel by a PCIe device is
> statically defined in the Outstanding Page Request Capacity field of the
> PCIe ATS configure space.
> 
> The new length of PRQ is calculated by summing up the value of Outstanding
> Page Request Capacity register across all devices where Page Requests are
> supported on the real PR-capable platform (Intel Sapphire Rapids). The
> result is round to the nearest higher power of 2.
> 
> The PRQ length is also double sized as the VT-d IOMMU driver only updates
> the Page Request Queue Head Register (PQH_REG) after processing the
> entire
> queue.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

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

> ---
>  include/linux/intel-svm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
> index b3b125b332aa..207ef06ba3e1 100644
> --- a/include/linux/intel-svm.h
> +++ b/include/linux/intel-svm.h
> @@ -9,7 +9,7 @@
>  #define __INTEL_SVM_H__
> 
>  /* Page Request Queue depth */
> -#define PRQ_ORDER	2
> +#define PRQ_ORDER	4
>  #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
>  #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
> 
> --
> 2.25.1
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 4/4] iommu/vt-d: Size Page Request Queue to avoid overflow condition

[Tian, Kevin]

> From: Lu Baolu
> Sent: Thursday, April 21, 2022 7:36 PM
> 
> PRQ overflow may cause I/O throughput congestion, resulting in unnecessary
> degradation of I/O performance. Appropriately increasing the length of PRQ
> can greatly reduce the occurrence of PRQ overflow. The count of maximum
> page requests that can be generated in parallel by a PCIe device is
> statically defined in the Outstanding Page Request Capacity field of the
> PCIe ATS configure space.
> 
> The new length of PRQ is calculated by summing up the value of Outstanding
> Page Request Capacity register across all devices where Page Requests are
> supported on the real PR-capable platform (Intel Sapphire Rapids). The
> result is round to the nearest higher power of 2.
> 
> The PRQ length is also double sized as the VT-d IOMMU driver only updates
> the Page Request Queue Head Register (PQH_REG) after processing the
> entire
> queue.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

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

> ---
>  include/linux/intel-svm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
> index b3b125b332aa..207ef06ba3e1 100644
> --- a/include/linux/intel-svm.h
> +++ b/include/linux/intel-svm.h
> @@ -9,7 +9,7 @@
>  #define __INTEL_SVM_H__
> 
>  /* Page Request Queue depth */
> -#define PRQ_ORDER	2
> +#define PRQ_ORDER	4
>  #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
>  #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
> 
> --
> 2.25.1
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/22 10:47, Tian, Kevin wrote:
>> From: Lu Baolu
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> The latest VT-d specification states that the PGSNP field in the pasid
>> table entry should be treated as Reserved(0) for implementations not
>> supporting Snoop Control (SC=0 in the Extended Capability Register).
>> This adds a check before setting the field.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index f8d215d85695..5cb2daa2b8cb 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>   		}
>>   	}
>>
>> -	if (flags & PASID_FLAG_PAGE_SNOOP)
>> -		pasid_set_pgsnp(pte);
>> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
>> +		if (ecap_sc_support(iommu->ecap)) {
>> +			pasid_set_pgsnp(pte);
>> +		} else {
>> +			pasid_clear_entry(pte);
>> +			return -EINVAL;
>> +		}
>> +	}
>>
>>   	pasid_set_domain_id(pte, did);
>>   	pasid_set_address_width(pte, iommu->agaw);
>> @@ -710,7 +716,8 @@ 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)
>> +	if (ecap_sc_support(iommu->ecap) &&
>> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>>   		pasid_set_pgsnp(pte);
>>
> This should be rebased on top of Jason's enforce coherency series
> instead of blindly setting it. No matter whether it's legacy mode
> where we set SNP in PTE or scalable mode where we set PGSNP
> in PASID entry for entire page table, the trigger point should be
> same i.e. when someone calls enforce_cache_coherency().

With Jason's enforce coherency series merged, we even don't need to set
PGSNP bit of a pasid entry for second level translation. 2nd level
always supports SNP in PTEs, so set PGSNP in pasid table entry is
unnecessary.

Any thoughts?

Best regards,
baolu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/22 10:47, Tian, Kevin wrote:
>> From: Lu Baolu
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> The latest VT-d specification states that the PGSNP field in the pasid
>> table entry should be treated as Reserved(0) for implementations not
>> supporting Snoop Control (SC=0 in the Extended Capability Register).
>> This adds a check before setting the field.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index f8d215d85695..5cb2daa2b8cb 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>   		}
>>   	}
>>
>> -	if (flags & PASID_FLAG_PAGE_SNOOP)
>> -		pasid_set_pgsnp(pte);
>> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
>> +		if (ecap_sc_support(iommu->ecap)) {
>> +			pasid_set_pgsnp(pte);
>> +		} else {
>> +			pasid_clear_entry(pte);
>> +			return -EINVAL;
>> +		}
>> +	}
>>
>>   	pasid_set_domain_id(pte, did);
>>   	pasid_set_address_width(pte, iommu->agaw);
>> @@ -710,7 +716,8 @@ 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)
>> +	if (ecap_sc_support(iommu->ecap) &&
>> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>>   		pasid_set_pgsnp(pte);
>>
> This should be rebased on top of Jason's enforce coherency series
> instead of blindly setting it. No matter whether it's legacy mode
> where we set SNP in PTE or scalable mode where we set PGSNP
> in PASID entry for entire page table, the trigger point should be
> same i.e. when someone calls enforce_cache_coherency().

With Jason's enforce coherency series merged, we even don't need to set
PGSNP bit of a pasid entry for second level translation. 2nd level
always supports SNP in PTEs, so set PGSNP in pasid table entry is
unnecessary.

Any thoughts?

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Lu Baolu]

On 2022/4/22 11:05, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> This field make the requests snoop processor caches irrespective of
>> other attributes in the request or other fields in paging structure
>> entries used to translate the request.
> 
> I think you want to first point out the fact that SVA wants snoop
> cache instead of just talking about the effect of PGSNP.
> 
> But thinking more I wonder why PGSNP is ever required. This is
> similar to DMA API case. x86 is already cache coherent for normal
> DMA (if not setting PCI no-snoop) and if the driver knows no-snoop
> is incompatible to SVA API then it should avoid triggering no-snoop
> traffic for SVA usage. In this case it is pointless for IOMMU driver
> to enable force-snooping. Even in the future certain platform allows
> no-snoop usage w/ SVA (I'm not sure how it works) this again should
> be reflected by additional SVA APIs for driver to explicitly manage.
> 
> force-snoop should be enabled only in device assignment case IMHO,
> orthogonal to whether vSVA is actually used.
> 
> Did I misunderstand the motivation here?

No, you didn't.

Let's talk with the arch guys for more details before move this patch
ahead. Thanks for pointing this out.

Best regards,
baolu

> 
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/svm.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 23a38763c1d1..c720d1be992d 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -391,9 +391,12 @@ static struct iommu_sva
>> *intel_svm_bind_mm(struct intel_iommu *iommu,
>>   	}
>>
>>   	/* Setup the pasid table: */
>> -	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
>> -			PASID_FLAG_SUPERVISOR_MODE : 0;
>> -	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ?
>> PASID_FLAG_FL5LP : 0;
>> +	sflags = PASID_FLAG_PAGE_SNOOP;
>> +	if (flags & SVM_FLAG_SUPERVISOR_MODE)
>> +		sflags |= PASID_FLAG_SUPERVISOR_MODE;
>> +	if (cpu_feature_enabled(X86_FEATURE_LA57))
>> +		sflags |= PASID_FLAG_FL5LP;
>> +
>>   	spin_lock_irqsave(&iommu->lock, iflags);
>>   	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm-
>>> pasid,
>>   					    FLPT_DEFAULT_DID, sflags);
>> --
>> 2.25.1
> 

Re: [PATCH v2 2/4] iommu/vt-d: Set PGSNP bit in pasid table entry for SVA binding

[Lu Baolu]

On 2022/4/22 11:05, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> This field make the requests snoop processor caches irrespective of
>> other attributes in the request or other fields in paging structure
>> entries used to translate the request.
> 
> I think you want to first point out the fact that SVA wants snoop
> cache instead of just talking about the effect of PGSNP.
> 
> But thinking more I wonder why PGSNP is ever required. This is
> similar to DMA API case. x86 is already cache coherent for normal
> DMA (if not setting PCI no-snoop) and if the driver knows no-snoop
> is incompatible to SVA API then it should avoid triggering no-snoop
> traffic for SVA usage. In this case it is pointless for IOMMU driver
> to enable force-snooping. Even in the future certain platform allows
> no-snoop usage w/ SVA (I'm not sure how it works) this again should
> be reflected by additional SVA APIs for driver to explicitly manage.
> 
> force-snoop should be enabled only in device assignment case IMHO,
> orthogonal to whether vSVA is actually used.
> 
> Did I misunderstand the motivation here?

No, you didn't.

Let's talk with the arch guys for more details before move this patch
ahead. Thanks for pointing this out.

Best regards,
baolu

> 
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/svm.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 23a38763c1d1..c720d1be992d 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -391,9 +391,12 @@ static struct iommu_sva
>> *intel_svm_bind_mm(struct intel_iommu *iommu,
>>   	}
>>
>>   	/* Setup the pasid table: */
>> -	sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
>> -			PASID_FLAG_SUPERVISOR_MODE : 0;
>> -	sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ?
>> PASID_FLAG_FL5LP : 0;
>> +	sflags = PASID_FLAG_PAGE_SNOOP;
>> +	if (flags & SVM_FLAG_SUPERVISOR_MODE)
>> +		sflags |= PASID_FLAG_SUPERVISOR_MODE;
>> +	if (cpu_feature_enabled(X86_FEATURE_LA57))
>> +		sflags |= PASID_FLAG_FL5LP;
>> +
>>   	spin_lock_irqsave(&iommu->lock, iflags);
>>   	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm-
>>> pasid,
>>   					    FLPT_DEFAULT_DID, sflags);
>> --
>> 2.25.1
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Lu Baolu]

On 2022/4/22 11:05, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> The page fault handling framework in the IOMMU core explicitly states
>> that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
>> discard them before reporting faults. This handles Stop Marker messages
>> in prq_event_thread() before reporting events to the core.
>>
>> The VT-d driver explicitly drains the pending page requests when a CPU
>> page table (represented by a mm struct) is unbound from a PASID according
>> to the procedures defined in the VT-d spec. The Stop Marker messages do
>> not need a response. Hence, it is safe to drop the Stop Marker messages
>> silently if any of them is found in the page request queue.
>>
>> Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> Reviewed-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> 

Thank you, Kevin. I will queue this patch to Joerg as a fix for v5.18.

Best regards,
baolu

Re: [PATCH v2 3/4] iommu/vt-d: Drop stop marker messages

[Lu Baolu]

On 2022/4/22 11:05, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Thursday, April 21, 2022 7:36 PM
>>
>> The page fault handling framework in the IOMMU core explicitly states
>> that it doesn't handle PCI PASID Stop Marker and the IOMMU drivers must
>> discard them before reporting faults. This handles Stop Marker messages
>> in prq_event_thread() before reporting events to the core.
>>
>> The VT-d driver explicitly drains the pending page requests when a CPU
>> page table (represented by a mm struct) is unbound from a PASID according
>> to the procedures defined in the VT-d spec. The Stop Marker messages do
>> not need a response. Hence, it is safe to drop the Stop Marker messages
>> silently if any of them is found in the page request queue.
>>
>> Fixes: d5b9e4bfe0d88 ("iommu/vt-d: Report prq to io-pgfault framework")
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> Reviewed-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> 

Thank you, Kevin. I will queue this patch to Joerg as a fix for v5.18.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Friday, April 22, 2022 9:04 PM
> 
> On 2022/4/22 10:47, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> Sent: Thursday, April 21, 2022 7:36 PM
> >>
> >> The latest VT-d specification states that the PGSNP field in the pasid
> >> table entry should be treated as Reserved(0) for implementations not
> >> supporting Snoop Control (SC=0 in the Extended Capability Register).
> >> This adds a check before setting the field.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/intel/pasid.c | 13 ++++++++++---
> >>   1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> >> index f8d215d85695..5cb2daa2b8cb 100644
> >> --- a/drivers/iommu/intel/pasid.c
> >> +++ b/drivers/iommu/intel/pasid.c
> >> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct
> intel_iommu
> >> *iommu,
> >>   		}
> >>   	}
> >>
> >> -	if (flags & PASID_FLAG_PAGE_SNOOP)
> >> -		pasid_set_pgsnp(pte);
> >> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
> >> +		if (ecap_sc_support(iommu->ecap)) {
> >> +			pasid_set_pgsnp(pte);
> >> +		} else {
> >> +			pasid_clear_entry(pte);
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >>
> >>   	pasid_set_domain_id(pte, did);
> >>   	pasid_set_address_width(pte, iommu->agaw);
> >> @@ -710,7 +716,8 @@ 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)
> >> +	if (ecap_sc_support(iommu->ecap) &&
> >> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> >>   		pasid_set_pgsnp(pte);
> >>
> > This should be rebased on top of Jason's enforce coherency series
> > instead of blindly setting it. No matter whether it's legacy mode
> > where we set SNP in PTE or scalable mode where we set PGSNP
> > in PASID entry for entire page table, the trigger point should be
> > same i.e. when someone calls enforce_cache_coherency().
> 
> With Jason's enforce coherency series merged, we even don't need to set
> PGSNP bit of a pasid entry for second level translation. 2nd level
> always supports SNP in PTEs, so set PGSNP in pasid table entry is
> unnecessary.
> 

Yes, this sounds correct for 2nd-level.

but setting PGSNP of 1st level translation is also relevant to that
change when talking about enforcing coherency in the guest. In
this case PASID_FLAG_PAGE_SNOOP should be set also after
enforce_cache_coherency() is called.

Currently it's always set for unmanaged domain in
domain_setup_first_level():

	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
		flags |= PASID_FLAG_PAGE_SNOOP;

Suppose we need a separate interface to update PGSNP after pasid
entry is set up.

Thanks
Kevin

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Friday, April 22, 2022 9:04 PM
> 
> On 2022/4/22 10:47, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> Sent: Thursday, April 21, 2022 7:36 PM
> >>
> >> The latest VT-d specification states that the PGSNP field in the pasid
> >> table entry should be treated as Reserved(0) for implementations not
> >> supporting Snoop Control (SC=0 in the Extended Capability Register).
> >> This adds a check before setting the field.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/intel/pasid.c | 13 ++++++++++---
> >>   1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> >> index f8d215d85695..5cb2daa2b8cb 100644
> >> --- a/drivers/iommu/intel/pasid.c
> >> +++ b/drivers/iommu/intel/pasid.c
> >> @@ -625,8 +625,14 @@ int intel_pasid_setup_first_level(struct
> intel_iommu
> >> *iommu,
> >>   		}
> >>   	}
> >>
> >> -	if (flags & PASID_FLAG_PAGE_SNOOP)
> >> -		pasid_set_pgsnp(pte);
> >> +	if (flags & PASID_FLAG_PAGE_SNOOP) {
> >> +		if (ecap_sc_support(iommu->ecap)) {
> >> +			pasid_set_pgsnp(pte);
> >> +		} else {
> >> +			pasid_clear_entry(pte);
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >>
> >>   	pasid_set_domain_id(pte, did);
> >>   	pasid_set_address_width(pte, iommu->agaw);
> >> @@ -710,7 +716,8 @@ 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)
> >> +	if (ecap_sc_support(iommu->ecap) &&
> >> +	    domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> >>   		pasid_set_pgsnp(pte);
> >>
> > This should be rebased on top of Jason's enforce coherency series
> > instead of blindly setting it. No matter whether it's legacy mode
> > where we set SNP in PTE or scalable mode where we set PGSNP
> > in PASID entry for entire page table, the trigger point should be
> > same i.e. when someone calls enforce_cache_coherency().
> 
> With Jason's enforce coherency series merged, we even don't need to set
> PGSNP bit of a pasid entry for second level translation. 2nd level
> always supports SNP in PTEs, so set PGSNP in pasid table entry is
> unnecessary.
> 

Yes, this sounds correct for 2nd-level.

but setting PGSNP of 1st level translation is also relevant to that
change when talking about enforcing coherency in the guest. In
this case PASID_FLAG_PAGE_SNOOP should be set also after
enforce_cache_coherency() is called.

Currently it's always set for unmanaged domain in
domain_setup_first_level():

	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
		flags |= PASID_FLAG_PAGE_SNOOP;

Suppose we need a separate interface to update PGSNP after pasid
entry is set up.

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/24 11:37, Tian, Kevin wrote:
>>> This should be rebased on top of Jason's enforce coherency series
>>> instead of blindly setting it. No matter whether it's legacy mode
>>> where we set SNP in PTE or scalable mode where we set PGSNP
>>> in PASID entry for entire page table, the trigger point should be
>>> same i.e. when someone calls enforce_cache_coherency().
>> With Jason's enforce coherency series merged, we even don't need to set
>> PGSNP bit of a pasid entry for second level translation. 2nd level
>> always supports SNP in PTEs, so set PGSNP in pasid table entry is
>> unnecessary.
>>
> Yes, this sounds correct for 2nd-level.
> 
> but setting PGSNP of 1st level translation is also relevant to that
> change when talking about enforcing coherency in the guest. In
> this case PASID_FLAG_PAGE_SNOOP should be set also after
> enforce_cache_coherency() is called.

Yes. Agreed.

> Currently it's always set for unmanaged domain in
> domain_setup_first_level():
> 
> 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> 		flags |= PASID_FLAG_PAGE_SNOOP;
> 
> Suppose we need a separate interface to update PGSNP after pasid
> entry is set up.

Currently enforcing coherency is only used in VFIO. In the VFIO use
case, it's safe to always set PGSNP when an UNMANAGED domain is attached
on the first level pasid translation. We could add support of updating
PGSNP after pasid entry setup once there's a real need.

Best regards,
baolu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/24 11:37, Tian, Kevin wrote:
>>> This should be rebased on top of Jason's enforce coherency series
>>> instead of blindly setting it. No matter whether it's legacy mode
>>> where we set SNP in PTE or scalable mode where we set PGSNP
>>> in PASID entry for entire page table, the trigger point should be
>>> same i.e. when someone calls enforce_cache_coherency().
>> With Jason's enforce coherency series merged, we even don't need to set
>> PGSNP bit of a pasid entry for second level translation. 2nd level
>> always supports SNP in PTEs, so set PGSNP in pasid table entry is
>> unnecessary.
>>
> Yes, this sounds correct for 2nd-level.
> 
> but setting PGSNP of 1st level translation is also relevant to that
> change when talking about enforcing coherency in the guest. In
> this case PASID_FLAG_PAGE_SNOOP should be set also after
> enforce_cache_coherency() is called.

Yes. Agreed.

> Currently it's always set for unmanaged domain in
> domain_setup_first_level():
> 
> 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> 		flags |= PASID_FLAG_PAGE_SNOOP;
> 
> Suppose we need a separate interface to update PGSNP after pasid
> entry is set up.

Currently enforcing coherency is only used in VFIO. In the VFIO use
case, it's safe to always set PGSNP when an UNMANAGED domain is attached
on the first level pasid translation. We could add support of updating
PGSNP after pasid entry setup once there's a real need.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Sunday, April 24, 2022 12:38 PM
> 
> On 2022/4/24 11:37, Tian, Kevin wrote:
> >>> This should be rebased on top of Jason's enforce coherency series
> >>> instead of blindly setting it. No matter whether it's legacy mode
> >>> where we set SNP in PTE or scalable mode where we set PGSNP
> >>> in PASID entry for entire page table, the trigger point should be
> >>> same i.e. when someone calls enforce_cache_coherency().
> >> With Jason's enforce coherency series merged, we even don't need to set
> >> PGSNP bit of a pasid entry for second level translation. 2nd level
> >> always supports SNP in PTEs, so set PGSNP in pasid table entry is
> >> unnecessary.
> >>
> > Yes, this sounds correct for 2nd-level.
> >
> > but setting PGSNP of 1st level translation is also relevant to that
> > change when talking about enforcing coherency in the guest. In
> > this case PASID_FLAG_PAGE_SNOOP should be set also after
> > enforce_cache_coherency() is called.
> 
> Yes. Agreed.
> 
> > Currently it's always set for unmanaged domain in
> > domain_setup_first_level():
> >
> > 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> > 		flags |= PASID_FLAG_PAGE_SNOOP;
> >
> > Suppose we need a separate interface to update PGSNP after pasid
> > entry is set up.
> 
> Currently enforcing coherency is only used in VFIO. In the VFIO use
> case, it's safe to always set PGSNP when an UNMANAGED domain is attached
> on the first level pasid translation. We could add support of updating
> PGSNP after pasid entry setup once there's a real need.
> 

The real need is here. The iommu driver should not assume the
policy of VFIO, which is already communicated via the new
enforce_cache_coherency() interface. The same policy should
apply no matter whether 1st or 2nd level is in-use.

RE: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Sunday, April 24, 2022 12:38 PM
> 
> On 2022/4/24 11:37, Tian, Kevin wrote:
> >>> This should be rebased on top of Jason's enforce coherency series
> >>> instead of blindly setting it. No matter whether it's legacy mode
> >>> where we set SNP in PTE or scalable mode where we set PGSNP
> >>> in PASID entry for entire page table, the trigger point should be
> >>> same i.e. when someone calls enforce_cache_coherency().
> >> With Jason's enforce coherency series merged, we even don't need to set
> >> PGSNP bit of a pasid entry for second level translation. 2nd level
> >> always supports SNP in PTEs, so set PGSNP in pasid table entry is
> >> unnecessary.
> >>
> > Yes, this sounds correct for 2nd-level.
> >
> > but setting PGSNP of 1st level translation is also relevant to that
> > change when talking about enforcing coherency in the guest. In
> > this case PASID_FLAG_PAGE_SNOOP should be set also after
> > enforce_cache_coherency() is called.
> 
> Yes. Agreed.
> 
> > Currently it's always set for unmanaged domain in
> > domain_setup_first_level():
> >
> > 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
> > 		flags |= PASID_FLAG_PAGE_SNOOP;
> >
> > Suppose we need a separate interface to update PGSNP after pasid
> > entry is set up.
> 
> Currently enforcing coherency is only used in VFIO. In the VFIO use
> case, it's safe to always set PGSNP when an UNMANAGED domain is attached
> on the first level pasid translation. We could add support of updating
> PGSNP after pasid entry setup once there's a real need.
> 

The real need is here. The iommu driver should not assume the
policy of VFIO, which is already communicated via the new
enforce_cache_coherency() interface. The same policy should
apply no matter whether 1st or 2nd level is in-use.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/24 13:55, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Sunday, April 24, 2022 12:38 PM
>>
>> On 2022/4/24 11:37, Tian, Kevin wrote:
>>>>> This should be rebased on top of Jason's enforce coherency series
>>>>> instead of blindly setting it. No matter whether it's legacy mode
>>>>> where we set SNP in PTE or scalable mode where we set PGSNP
>>>>> in PASID entry for entire page table, the trigger point should be
>>>>> same i.e. when someone calls enforce_cache_coherency().
>>>> With Jason's enforce coherency series merged, we even don't need to set
>>>> PGSNP bit of a pasid entry for second level translation. 2nd level
>>>> always supports SNP in PTEs, so set PGSNP in pasid table entry is
>>>> unnecessary.
>>>>
>>> Yes, this sounds correct for 2nd-level.
>>>
>>> but setting PGSNP of 1st level translation is also relevant to that
>>> change when talking about enforcing coherency in the guest. In
>>> this case PASID_FLAG_PAGE_SNOOP should be set also after
>>> enforce_cache_coherency() is called.
>>
>> Yes. Agreed.
>>
>>> Currently it's always set for unmanaged domain in
>>> domain_setup_first_level():
>>>
>>> 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>>> 		flags |= PASID_FLAG_PAGE_SNOOP;
>>>
>>> Suppose we need a separate interface to update PGSNP after pasid
>>> entry is set up.
>>
>> Currently enforcing coherency is only used in VFIO. In the VFIO use
>> case, it's safe to always set PGSNP when an UNMANAGED domain is attached
>> on the first level pasid translation. We could add support of updating
>> PGSNP after pasid entry setup once there's a real need.
>>
> 
> The real need is here. The iommu driver should not assume the
> policy of VFIO, which is already communicated via the new
> enforce_cache_coherency() interface. The same policy should
> apply no matter whether 1st or 2nd level is in-use.

Okay, I think I will move this patch out of this series and put it in a
separated one for VT-d improvements after Jason's enforcing snoop series
gets merged. Thanks for your suggestions.

Best regards,
baolu

Re: [PATCH v2 1/4] iommu/vt-d: Check before setting PGSNP bit in pasid table entry

[Lu Baolu]

On 2022/4/24 13:55, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Sunday, April 24, 2022 12:38 PM
>>
>> On 2022/4/24 11:37, Tian, Kevin wrote:
>>>>> This should be rebased on top of Jason's enforce coherency series
>>>>> instead of blindly setting it. No matter whether it's legacy mode
>>>>> where we set SNP in PTE or scalable mode where we set PGSNP
>>>>> in PASID entry for entire page table, the trigger point should be
>>>>> same i.e. when someone calls enforce_cache_coherency().
>>>> With Jason's enforce coherency series merged, we even don't need to set
>>>> PGSNP bit of a pasid entry for second level translation. 2nd level
>>>> always supports SNP in PTEs, so set PGSNP in pasid table entry is
>>>> unnecessary.
>>>>
>>> Yes, this sounds correct for 2nd-level.
>>>
>>> but setting PGSNP of 1st level translation is also relevant to that
>>> change when talking about enforcing coherency in the guest. In
>>> this case PASID_FLAG_PAGE_SNOOP should be set also after
>>> enforce_cache_coherency() is called.
>>
>> Yes. Agreed.
>>
>>> Currently it's always set for unmanaged domain in
>>> domain_setup_first_level():
>>>
>>> 	if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
>>> 		flags |= PASID_FLAG_PAGE_SNOOP;
>>>
>>> Suppose we need a separate interface to update PGSNP after pasid
>>> entry is set up.
>>
>> Currently enforcing coherency is only used in VFIO. In the VFIO use
>> case, it's safe to always set PGSNP when an UNMANAGED domain is attached
>> on the first level pasid translation. We could add support of updating
>> PGSNP after pasid entry setup once there's a real need.
>>
> 
> The real need is here. The iommu driver should not assume the
> policy of VFIO, which is already communicated via the new
> enforce_cache_coherency() interface. The same policy should
> apply no matter whether 1st or 2nd level is in-use.

Okay, I think I will move this patch out of this series and put it in a
separated one for VT-d improvements after Jason's enforcing snoop series
gets merged. Thanks for your suggestions.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu