[PATCH 0/3] Misc bug fixes for VT-d SVM

[PATCH 0/3] Misc bug fixes for VT-d SVM

[Jacob Pan]

Just a few bug fixes while testing Intel SVM code.

Jacob Pan (3):
  iommu/vt-d: Remove redundant IOTLB flush
  iommu/vt-d: Fix mm reference leak
  iommu/vt-d: Add build dependency on IOASID

 drivers/iommu/Kconfig     |  1 +
 drivers/iommu/intel-svm.c | 13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 0/3] Misc bug fixes for VT-d SVM

[Jacob Pan]

Just a few bug fixes while testing Intel SVM code.

Jacob Pan (3):
  iommu/vt-d: Remove redundant IOTLB flush
  iommu/vt-d: Fix mm reference leak
  iommu/vt-d: Add build dependency on IOASID

 drivers/iommu/Kconfig     |  1 +
 drivers/iommu/intel-svm.c | 13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

IOTLB flush already included in the PASID tear down process. There
is no need to flush again.

Cc: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 8f42d717d8d7..1483f1845762 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -268,10 +268,9 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * *has* to handle gracefully without affecting other processes.
 	 */
 	rcu_read_lock();
-	list_for_each_entry_rcu(sdev, &svm->devs, list) {
+	list_for_each_entry_rcu(sdev, &svm->devs, list)
 		intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm->pasid);
-		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
-	}
+
 	rcu_read_unlock();
 
 }
@@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 			 * large and has to be physically contiguous. So it's
 			 * hard to be as defensive as we might like. */
 			intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
-			intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
 			kfree_rcu(sdev, rcu);
 
 			if (list_empty(&svm->devs)) {
-- 
2.7.4

[PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

IOTLB flush already included in the PASID tear down process. There
is no need to flush again.

Cc: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 8f42d717d8d7..1483f1845762 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -268,10 +268,9 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * *has* to handle gracefully without affecting other processes.
 	 */
 	rcu_read_lock();
-	list_for_each_entry_rcu(sdev, &svm->devs, list) {
+	list_for_each_entry_rcu(sdev, &svm->devs, list)
 		intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm->pasid);
-		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
-	}
+
 	rcu_read_unlock();
 
 }
@@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 			 * large and has to be physically contiguous. So it's
 			 * hard to be as defensive as we might like. */
 			intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
-			intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
 			kfree_rcu(sdev, rcu);
 
 			if (list_empty(&svm->devs)) {
-- 
2.7.4

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

[PATCH 2/3] iommu/vt-d: Fix mm reference leak

[Jacob Pan]

Move canonical address check before mmget_not_zero() to avoid mm
reference leak.

Fixes: 9d8c3af31607 ("iommu/vt-d: IOMMU Page Request needs to check if
address is canonical.")

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 1483f1845762..56253c59ca10 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -861,14 +861,15 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		 * any faults on kernel addresses. */
 		if (!svm->mm)
 			goto bad_req;
-		/* If the mm is already defunct, don't handle faults. */
-		if (!mmget_not_zero(svm->mm))
-			goto bad_req;
 
 		/* If address is not canonical, return invalid response */
 		if (!is_canonical_address(address))
 			goto bad_req;
 
+		/* If the mm is already defunct, don't handle faults. */
+		if (!mmget_not_zero(svm->mm))
+			goto bad_req;
+
 		down_read(&svm->mm->mmap_sem);
 		vma = find_extend_vma(svm->mm, address);
 		if (!vma || address < vma->vm_start)
-- 
2.7.4

[PATCH 2/3] iommu/vt-d: Fix mm reference leak

[Jacob Pan]

Move canonical address check before mmget_not_zero() to avoid mm
reference leak.

Fixes: 9d8c3af31607 ("iommu/vt-d: IOMMU Page Request needs to check if
address is canonical.")

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 1483f1845762..56253c59ca10 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -861,14 +861,15 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		 * any faults on kernel addresses. */
 		if (!svm->mm)
 			goto bad_req;
-		/* If the mm is already defunct, don't handle faults. */
-		if (!mmget_not_zero(svm->mm))
-			goto bad_req;
 
 		/* If address is not canonical, return invalid response */
 		if (!is_canonical_address(address))
 			goto bad_req;
 
+		/* If the mm is already defunct, don't handle faults. */
+		if (!mmget_not_zero(svm->mm))
+			goto bad_req;
+
 		down_read(&svm->mm->mmap_sem);
 		vma = find_extend_vma(svm->mm, address);
 		if (!vma || address < vma->vm_start)
-- 
2.7.4

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

[PATCH 3/3] iommu/vt-d: Add build dependency on IOASID

[Jacob Pan]

IOASID code is needed by VT-d scalable mode for PASID allocation.
Add explicit dependency such that IOASID is built-in whenever Intel
IOMMU is enabled.
Otherwise, aux domain code will fail when IOMMU is built-in and IOASID
is compiled as a module.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index d2fade984999..25149544d57c 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -188,6 +188,7 @@ config INTEL_IOMMU
 	select NEED_DMA_MAP_STATE
 	select DMAR_TABLE
 	select SWIOTLB
+	select IOASID
 	help
 	  DMA remapping (DMAR) devices support enables independent address
 	  translations for Direct Memory Access (DMA) from devices.
-- 
2.7.4

[PATCH 3/3] iommu/vt-d: Add build dependency on IOASID

[Jacob Pan]

IOASID code is needed by VT-d scalable mode for PASID allocation.
Add explicit dependency such that IOASID is built-in whenever Intel
IOMMU is enabled.
Otherwise, aux domain code will fail when IOMMU is built-in and IOASID
is compiled as a module.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index d2fade984999..25149544d57c 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -188,6 +188,7 @@ config INTEL_IOMMU
 	select NEED_DMA_MAP_STATE
 	select DMAR_TABLE
 	select SWIOTLB
+	select IOASID
 	help
 	  DMA remapping (DMAR) devices support enables independent address
 	  translations for Direct Memory Access (DMA) from devices.
-- 
2.7.4

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

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> IOTLB flush already included in the PASID tear down process. There
> is no need to flush again.

It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
based device TLB?

Best regards,
baolu

> 
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/intel-svm.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 8f42d717d8d7..1483f1845762 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -268,10 +268,9 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>   	 * *has* to handle gracefully without affecting other processes.
>   	 */
>   	rcu_read_lock();
> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
>   		intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm->pasid);
> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> -	}
> +
>   	rcu_read_unlock();
>   
>   }
> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
>   			 * large and has to be physically contiguous. So it's
>   			 * hard to be as defensive as we might like. */
>   			intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> -			intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
>   			kfree_rcu(sdev, rcu);
>   
>   			if (list_empty(&svm->devs)) {
> 

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> IOTLB flush already included in the PASID tear down process. There
> is no need to flush again.

It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
based device TLB?

Best regards,
baolu

> 
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/intel-svm.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 8f42d717d8d7..1483f1845762 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -268,10 +268,9 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>   	 * *has* to handle gracefully without affecting other processes.
>   	 */
>   	rcu_read_lock();
> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
>   		intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm->pasid);
> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> -	}
> +
>   	rcu_read_unlock();
>   
>   }
> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
>   			 * large and has to be physically contiguous. So it's
>   			 * hard to be as defensive as we might like. */
>   			intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> -			intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
>   			kfree_rcu(sdev, rcu);
>   
>   			if (list_empty(&svm->devs)) {
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/3] iommu/vt-d: Fix mm reference leak

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> Move canonical address check before mmget_not_zero() to avoid mm
> reference leak.
> 
> Fixes: 9d8c3af31607 ("iommu/vt-d: IOMMU Page Request needs to check if
> address is canonical.")
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/intel-svm.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 1483f1845762..56253c59ca10 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -861,14 +861,15 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   		 * any faults on kernel addresses. */
>   		if (!svm->mm)
>   			goto bad_req;
> -		/* If the mm is already defunct, don't handle faults. */
> -		if (!mmget_not_zero(svm->mm))
> -			goto bad_req;
>   
>   		/* If address is not canonical, return invalid response */
>   		if (!is_canonical_address(address))
>   			goto bad_req;
>   
> +		/* If the mm is already defunct, don't handle faults. */
> +		if (!mmget_not_zero(svm->mm))
> +			goto bad_req;
> +
>   		down_read(&svm->mm->mmap_sem);
>   		vma = find_extend_vma(svm->mm, address);
>   		if (!vma || address < vma->vm_start)
> 

Re: [PATCH 2/3] iommu/vt-d: Fix mm reference leak

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> Move canonical address check before mmget_not_zero() to avoid mm
> reference leak.
> 
> Fixes: 9d8c3af31607 ("iommu/vt-d: IOMMU Page Request needs to check if
> address is canonical.")
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/intel-svm.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 1483f1845762..56253c59ca10 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -861,14 +861,15 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   		 * any faults on kernel addresses. */
>   		if (!svm->mm)
>   			goto bad_req;
> -		/* If the mm is already defunct, don't handle faults. */
> -		if (!mmget_not_zero(svm->mm))
> -			goto bad_req;
>   
>   		/* If address is not canonical, return invalid response */
>   		if (!is_canonical_address(address))
>   			goto bad_req;
>   
> +		/* If the mm is already defunct, don't handle faults. */
> +		if (!mmget_not_zero(svm->mm))
> +			goto bad_req;
> +
>   		down_read(&svm->mm->mmap_sem);
>   		vma = find_extend_vma(svm->mm, address);
>   		if (!vma || address < vma->vm_start)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] iommu/vt-d: Add build dependency on IOASID

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> IOASID code is needed by VT-d scalable mode for PASID allocation.
> Add explicit dependency such that IOASID is built-in whenever Intel
> IOMMU is enabled.
> Otherwise, aux domain code will fail when IOMMU is built-in and IOASID
> is compiled as a module.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Fixes: 59a623374dc38 ("iommu/vt-d: Replace Intel specific PASID 
allocator with IOASID")
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d2fade984999..25149544d57c 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -188,6 +188,7 @@ config INTEL_IOMMU
>   	select NEED_DMA_MAP_STATE
>   	select DMAR_TABLE
>   	select SWIOTLB
> +	select IOASID
>   	help
>   	  DMA remapping (DMAR) devices support enables independent address
>   	  translations for Direct Memory Access (DMA) from devices.
> 

Re: [PATCH 3/3] iommu/vt-d: Add build dependency on IOASID

[Lu Baolu]

On 2020/3/20 12:32, Jacob Pan wrote:
> IOASID code is needed by VT-d scalable mode for PASID allocation.
> Add explicit dependency such that IOASID is built-in whenever Intel
> IOMMU is enabled.
> Otherwise, aux domain code will fail when IOMMU is built-in and IOASID
> is compiled as a module.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Fixes: 59a623374dc38 ("iommu/vt-d: Replace Intel specific PASID 
allocator with IOASID")
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d2fade984999..25149544d57c 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -188,6 +188,7 @@ config INTEL_IOMMU
>   	select NEED_DMA_MAP_STATE
>   	select DMAR_TABLE
>   	select SWIOTLB
> +	select IOASID
>   	help
>   	  DMA remapping (DMAR) devices support enables independent address
>   	  translations for Direct Memory Access (DMA) from devices.
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

On Fri, 20 Mar 2020 21:45:26 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> On 2020/3/20 12:32, Jacob Pan wrote:
> > IOTLB flush already included in the PASID tear down process. There
> > is no need to flush again.  
> 
> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
> based device TLB?
> 
I saw this code in intel_pasid_tear_down_entry(). Isn't the last line
flush the devtlb? Not in guest of course since the passdown tlb flush
is inclusive.

	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
	iotlb_invalidation_with_pasid(iommu, did, pasid);

	/* Device IOTLB doesn't need to be flushed in caching mode. */
	if (!cap_caching_mode(iommu->cap))
		devtlb_invalidation_with_pasid(iommu, dev, pasid);

> Best regards,
> baolu
> 
> > 
> > Cc: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >   drivers/iommu/intel-svm.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 8f42d717d8d7..1483f1845762 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -268,10 +268,9 @@ static void intel_mm_release(struct
> > mmu_notifier *mn, struct mm_struct *mm)
> >   	 * *has* to handle gracefully without affecting other
> > processes. */
> >   	rcu_read_lock();
> > -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> > +	list_for_each_entry_rcu(sdev, &svm->devs, list)
> >   		intel_pasid_tear_down_entry(svm->iommu,
> > sdev->dev, svm->pasid);
> > -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> > -	}
> > +
> >   	rcu_read_unlock();
> >   
> >   }
> > @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int
> > pasid)
> >   			 * large and has to be physically
> > contiguous. So it's
> >   			 * hard to be as defensive as we might
> > like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> > -			intel_flush_svm_range_dev(svm, sdev, 0,
> > -1, 0); kfree_rcu(sdev, rcu);
> >   
> >   			if (list_empty(&svm->devs)) {
> >   

[Jacob Pan]

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

On Fri, 20 Mar 2020 21:45:26 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> On 2020/3/20 12:32, Jacob Pan wrote:
> > IOTLB flush already included in the PASID tear down process. There
> > is no need to flush again.  
> 
> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
> based device TLB?
> 
I saw this code in intel_pasid_tear_down_entry(). Isn't the last line
flush the devtlb? Not in guest of course since the passdown tlb flush
is inclusive.

	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
	iotlb_invalidation_with_pasid(iommu, did, pasid);

	/* Device IOTLB doesn't need to be flushed in caching mode. */
	if (!cap_caching_mode(iommu->cap))
		devtlb_invalidation_with_pasid(iommu, dev, pasid);

> Best regards,
> baolu
> 
> > 
> > Cc: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >   drivers/iommu/intel-svm.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 8f42d717d8d7..1483f1845762 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -268,10 +268,9 @@ static void intel_mm_release(struct
> > mmu_notifier *mn, struct mm_struct *mm)
> >   	 * *has* to handle gracefully without affecting other
> > processes. */
> >   	rcu_read_lock();
> > -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> > +	list_for_each_entry_rcu(sdev, &svm->devs, list)
> >   		intel_pasid_tear_down_entry(svm->iommu,
> > sdev->dev, svm->pasid);
> > -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> > -	}
> > +
> >   	rcu_read_unlock();
> >   
> >   }
> > @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int
> > pasid)
> >   			 * large and has to be physically
> > contiguous. So it's
> >   			 * hard to be as defensive as we might
> > like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> > -			intel_flush_svm_range_dev(svm, sdev, 0,
> > -1, 0); kfree_rcu(sdev, rcu);
> >   
> >   			if (list_empty(&svm->devs)) {
> >   

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

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/21 0:20, Jacob Pan wrote:
> On Fri, 20 Mar 2020 21:45:26 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> On 2020/3/20 12:32, Jacob Pan wrote:
>>> IOTLB flush already included in the PASID tear down process. There
>>> is no need to flush again.
>>
>> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
>> based device TLB?
>>
> I saw this code in intel_pasid_tear_down_entry(). Isn't the last line
> flush the devtlb? Not in guest of course since the passdown tlb flush
> is inclusive.
> 
> 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> 	iotlb_invalidation_with_pasid(iommu, did, pasid);
> 
> 	/* Device IOTLB doesn't need to be flushed in caching mode. */
> 	if (!cap_caching_mode(iommu->cap))
> 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> 

But devtlb_invalidation_with_pasid() doesn't do the right thing, it
flushes the device tlb, instead of pasid-based device tlb.

static void
devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
                                struct device *dev, int pasid)
{
         struct device_domain_info *info;
         u16 sid, qdep, pfsid;

         info = dev->archdata.iommu;
         if (!info || !info->ats_enabled)
                 return;

         sid = info->bus << 8 | info->devfn;
         qdep = info->ats_qdep;
         pfsid = info->pfsid;

         qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
VTD_PAGE_SHIFT);
}

Best regards,
baolu

>> Best regards,
>> baolu
>>
>>>
>>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
>>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>> ---
>>>    drivers/iommu/intel-svm.c | 6 ++----
>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>>> index 8f42d717d8d7..1483f1845762 100644
>>> --- a/drivers/iommu/intel-svm.c
>>> +++ b/drivers/iommu/intel-svm.c
>>> @@ -268,10 +268,9 @@ static void intel_mm_release(struct
>>> mmu_notifier *mn, struct mm_struct *mm)
>>>    	 * *has* to handle gracefully without affecting other
>>> processes. */
>>>    	rcu_read_lock();
>>> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
>>> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
>>>    		intel_pasid_tear_down_entry(svm->iommu,
>>> sdev->dev, svm->pasid);
>>> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
>>> -	}
>>> +
>>>    	rcu_read_unlock();
>>>    
>>>    }
>>> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int
>>> pasid)
>>>    			 * large and has to be physically
>>> contiguous. So it's
>>>    			 * hard to be as defensive as we might
>>> like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
>>> -			intel_flush_svm_range_dev(svm, sdev, 0,
>>> -1, 0); kfree_rcu(sdev, rcu);
>>>    
>>>    			if (list_empty(&svm->devs)) {
>>>    
> 
> [Jacob Pan]
> 

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/21 0:20, Jacob Pan wrote:
> On Fri, 20 Mar 2020 21:45:26 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> On 2020/3/20 12:32, Jacob Pan wrote:
>>> IOTLB flush already included in the PASID tear down process. There
>>> is no need to flush again.
>>
>> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
>> based device TLB?
>>
> I saw this code in intel_pasid_tear_down_entry(). Isn't the last line
> flush the devtlb? Not in guest of course since the passdown tlb flush
> is inclusive.
> 
> 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> 	iotlb_invalidation_with_pasid(iommu, did, pasid);
> 
> 	/* Device IOTLB doesn't need to be flushed in caching mode. */
> 	if (!cap_caching_mode(iommu->cap))
> 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> 

But devtlb_invalidation_with_pasid() doesn't do the right thing, it
flushes the device tlb, instead of pasid-based device tlb.

static void
devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
                                struct device *dev, int pasid)
{
         struct device_domain_info *info;
         u16 sid, qdep, pfsid;

         info = dev->archdata.iommu;
         if (!info || !info->ats_enabled)
                 return;

         sid = info->bus << 8 | info->devfn;
         qdep = info->ats_qdep;
         pfsid = info->pfsid;

         qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
VTD_PAGE_SHIFT);
}

Best regards,
baolu

>> Best regards,
>> baolu
>>
>>>
>>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
>>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>> ---
>>>    drivers/iommu/intel-svm.c | 6 ++----
>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>>> index 8f42d717d8d7..1483f1845762 100644
>>> --- a/drivers/iommu/intel-svm.c
>>> +++ b/drivers/iommu/intel-svm.c
>>> @@ -268,10 +268,9 @@ static void intel_mm_release(struct
>>> mmu_notifier *mn, struct mm_struct *mm)
>>>    	 * *has* to handle gracefully without affecting other
>>> processes. */
>>>    	rcu_read_lock();
>>> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
>>> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
>>>    		intel_pasid_tear_down_entry(svm->iommu,
>>> sdev->dev, svm->pasid);
>>> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
>>> -	}
>>> +
>>>    	rcu_read_unlock();
>>>    
>>>    }
>>> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev, int
>>> pasid)
>>>    			 * large and has to be physically
>>> contiguous. So it's
>>>    			 * hard to be as defensive as we might
>>> like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
>>> -			intel_flush_svm_range_dev(svm, sdev, 0,
>>> -1, 0); kfree_rcu(sdev, rcu);
>>>    
>>>    			if (list_empty(&svm->devs)) {
>>>    
> 
> [Jacob Pan]
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

On Sat, 21 Mar 2020 09:32:45 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> On 2020/3/21 0:20, Jacob Pan wrote:
> > On Fri, 20 Mar 2020 21:45:26 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >   
> >> On 2020/3/20 12:32, Jacob Pan wrote:  
> >>> IOTLB flush already included in the PASID tear down process. There
> >>> is no need to flush again.  
> >>
> >> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
> >> based device TLB?
> >>  
> > I saw this code in intel_pasid_tear_down_entry(). Isn't the last
> > line flush the devtlb? Not in guest of course since the passdown
> > tlb flush is inclusive.
> > 
> > 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> > 	iotlb_invalidation_with_pasid(iommu, did, pasid);
> > 
> > 	/* Device IOTLB doesn't need to be flushed in caching mode.
> > */ if (!cap_caching_mode(iommu->cap))
> > 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> >   
> 
> But devtlb_invalidation_with_pasid() doesn't do the right thing, it
> flushes the device tlb, instead of pasid-based device tlb.
> 
Hmm, you are right. But the function name is misleading, pasid argument
is not used, is there a reason why?
This is used for PASID based device IOTLB flush, right?

> static void
> devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
>                                 struct device *dev, int pasid)
> {
>          struct device_domain_info *info;
>          u16 sid, qdep, pfsid;
> 
>          info = dev->archdata.iommu;
>          if (!info || !info->ats_enabled)
>                  return;
> 
>          sid = info->bus << 8 | info->devfn;
>          qdep = info->ats_qdep;
>          pfsid = info->pfsid;
> 
>          qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
> VTD_PAGE_SHIFT);
> }
> 
> Best regards,
> baolu
> 
> >> Best regards,
> >> baolu
> >>  
> >>>
> >>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> ---
> >>>    drivers/iommu/intel-svm.c | 6 ++----
> >>>    1 file changed, 2 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> >>> index 8f42d717d8d7..1483f1845762 100644
> >>> --- a/drivers/iommu/intel-svm.c
> >>> +++ b/drivers/iommu/intel-svm.c
> >>> @@ -268,10 +268,9 @@ static void intel_mm_release(struct
> >>> mmu_notifier *mn, struct mm_struct *mm)
> >>>    	 * *has* to handle gracefully without affecting other
> >>> processes. */
> >>>    	rcu_read_lock();
> >>> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> >>> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
> >>>    		intel_pasid_tear_down_entry(svm->iommu,
> >>> sdev->dev, svm->pasid);
> >>> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> >>> -	}
> >>> +
> >>>    	rcu_read_unlock();
> >>>    
> >>>    }
> >>> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev,
> >>> int pasid)
> >>>    			 * large and has to be physically
> >>> contiguous. So it's
> >>>    			 * hard to be as defensive as we might
> >>> like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> >>> -			intel_flush_svm_range_dev(svm, sdev, 0,
> >>> -1, 0); kfree_rcu(sdev, rcu);
> >>>    
> >>>    			if (list_empty(&svm->devs)) {
> >>>      
> > 
> > [Jacob Pan]
> >   

[Jacob Pan]

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Jacob Pan]

On Sat, 21 Mar 2020 09:32:45 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> On 2020/3/21 0:20, Jacob Pan wrote:
> > On Fri, 20 Mar 2020 21:45:26 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >   
> >> On 2020/3/20 12:32, Jacob Pan wrote:  
> >>> IOTLB flush already included in the PASID tear down process. There
> >>> is no need to flush again.  
> >>
> >> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
> >> based device TLB?
> >>  
> > I saw this code in intel_pasid_tear_down_entry(). Isn't the last
> > line flush the devtlb? Not in guest of course since the passdown
> > tlb flush is inclusive.
> > 
> > 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> > 	iotlb_invalidation_with_pasid(iommu, did, pasid);
> > 
> > 	/* Device IOTLB doesn't need to be flushed in caching mode.
> > */ if (!cap_caching_mode(iommu->cap))
> > 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> >   
> 
> But devtlb_invalidation_with_pasid() doesn't do the right thing, it
> flushes the device tlb, instead of pasid-based device tlb.
> 
Hmm, you are right. But the function name is misleading, pasid argument
is not used, is there a reason why?
This is used for PASID based device IOTLB flush, right?

> static void
> devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
>                                 struct device *dev, int pasid)
> {
>          struct device_domain_info *info;
>          u16 sid, qdep, pfsid;
> 
>          info = dev->archdata.iommu;
>          if (!info || !info->ats_enabled)
>                  return;
> 
>          sid = info->bus << 8 | info->devfn;
>          qdep = info->ats_qdep;
>          pfsid = info->pfsid;
> 
>          qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
> VTD_PAGE_SHIFT);
> }
> 
> Best regards,
> baolu
> 
> >> Best regards,
> >> baolu
> >>  
> >>>
> >>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> ---
> >>>    drivers/iommu/intel-svm.c | 6 ++----
> >>>    1 file changed, 2 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> >>> index 8f42d717d8d7..1483f1845762 100644
> >>> --- a/drivers/iommu/intel-svm.c
> >>> +++ b/drivers/iommu/intel-svm.c
> >>> @@ -268,10 +268,9 @@ static void intel_mm_release(struct
> >>> mmu_notifier *mn, struct mm_struct *mm)
> >>>    	 * *has* to handle gracefully without affecting other
> >>> processes. */
> >>>    	rcu_read_lock();
> >>> -	list_for_each_entry_rcu(sdev, &svm->devs, list) {
> >>> +	list_for_each_entry_rcu(sdev, &svm->devs, list)
> >>>    		intel_pasid_tear_down_entry(svm->iommu,
> >>> sdev->dev, svm->pasid);
> >>> -		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> >>> -	}
> >>> +
> >>>    	rcu_read_unlock();
> >>>    
> >>>    }
> >>> @@ -731,7 +730,6 @@ int intel_svm_unbind_mm(struct device *dev,
> >>> int pasid)
> >>>    			 * large and has to be physically
> >>> contiguous. So it's
> >>>    			 * hard to be as defensive as we might
> >>> like. */ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> >>> -			intel_flush_svm_range_dev(svm, sdev, 0,
> >>> -1, 0); kfree_rcu(sdev, rcu);
> >>>    
> >>>    			if (list_empty(&svm->devs)) {
> >>>      
> > 
> > [Jacob Pan]
> >   

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

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/24 23:31, Jacob Pan wrote:
> On Sat, 21 Mar 2020 09:32:45 +0800
> Lu Baolu<baolu.lu@linux.intel.com>  wrote:
> 
>> On 2020/3/21 0:20, Jacob Pan wrote:
>>> On Fri, 20 Mar 2020 21:45:26 +0800
>>> Lu Baolu<baolu.lu@linux.intel.com>  wrote:
>>>    
>>>> On 2020/3/20 12:32, Jacob Pan wrote:
>>>>> IOTLB flush already included in the PASID tear down process. There
>>>>> is no need to flush again.
>>>> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
>>>> based device TLB?
>>>>   
>>> I saw this code in intel_pasid_tear_down_entry(). Isn't the last
>>> line flush the devtlb? Not in guest of course since the passdown
>>> tlb flush is inclusive.
>>>
>>> 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
>>> 	iotlb_invalidation_with_pasid(iommu, did, pasid);
>>>
>>> 	/* Device IOTLB doesn't need to be flushed in caching mode.
>>> */ if (!cap_caching_mode(iommu->cap))
>>> 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
>>>    
>> But devtlb_invalidation_with_pasid() doesn't do the right thing, it
>> flushes the device tlb, instead of pasid-based device tlb.
>>
> Hmm, you are right. But the function name is misleading, pasid argument
> is not used, is there a reason why?
> This is used for PASID based device IOTLB flush, right?
> 

Yes. I will fix and put your patch after it.

Best regards,
baolu

Re: [PATCH 1/3] iommu/vt-d: Remove redundant IOTLB flush

[Lu Baolu]

On 2020/3/24 23:31, Jacob Pan wrote:
> On Sat, 21 Mar 2020 09:32:45 +0800
> Lu Baolu<baolu.lu@linux.intel.com>  wrote:
> 
>> On 2020/3/21 0:20, Jacob Pan wrote:
>>> On Fri, 20 Mar 2020 21:45:26 +0800
>>> Lu Baolu<baolu.lu@linux.intel.com>  wrote:
>>>    
>>>> On 2020/3/20 12:32, Jacob Pan wrote:
>>>>> IOTLB flush already included in the PASID tear down process. There
>>>>> is no need to flush again.
>>>> It seems that intel_pasid_tear_down_entry() doesn't flush the pasid
>>>> based device TLB?
>>>>   
>>> I saw this code in intel_pasid_tear_down_entry(). Isn't the last
>>> line flush the devtlb? Not in guest of course since the passdown
>>> tlb flush is inclusive.
>>>
>>> 	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
>>> 	iotlb_invalidation_with_pasid(iommu, did, pasid);
>>>
>>> 	/* Device IOTLB doesn't need to be flushed in caching mode.
>>> */ if (!cap_caching_mode(iommu->cap))
>>> 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
>>>    
>> But devtlb_invalidation_with_pasid() doesn't do the right thing, it
>> flushes the device tlb, instead of pasid-based device tlb.
>>
> Hmm, you are right. But the function name is misleading, pasid argument
> is not used, is there a reason why?
> This is used for PASID based device IOTLB flush, right?
> 

Yes. I will fix and put your patch after it.

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

Re: [PATCH 0/3] Misc bug fixes for VT-d SVM

[Joerg Roedel]

On Thu, Mar 19, 2020 at 09:32:28PM -0700, Jacob Pan wrote:
>   iommu/vt-d: Fix mm reference leak
>   iommu/vt-d: Add build dependency on IOASID

Applied these two, thanks.

Re: [PATCH 0/3] Misc bug fixes for VT-d SVM

[Joerg Roedel]

On Thu, Mar 19, 2020 at 09:32:28PM -0700, Jacob Pan wrote:
>   iommu/vt-d: Fix mm reference leak
>   iommu/vt-d: Add build dependency on IOASID

Applied these two, thanks.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu