[PATCH 1/5] iommu/vt-d: Fix misuse of ALIGN in qi_flush_piotlb()

[PATCH 1/5] iommu/vt-d: Fix misuse of ALIGN in qi_flush_piotlb()

[Lu Baolu]

Use IS_ALIGNED() instead. Otherwise, an unaligned address will be ignored.

Fixes: 33cd6e642d6a7 ("iommu/vt-d: Flush PASID-based iotlb for iova over first level")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/dmar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index b46dbfa6d0ed..004feaed3c72 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1461,8 +1461,8 @@ void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, u64 addr,
 		int mask = ilog2(__roundup_pow_of_two(npages));
 		unsigned long align = (1ULL << (VTD_PAGE_SHIFT + mask));
 
-		if (WARN_ON_ONCE(!ALIGN(addr, align)))
-			addr &= ~(align - 1);
+		if (WARN_ON_ONCE(!IS_ALIGNED(addr, align)))
+			addr = ALIGN_DOWN(addr, align);
 
 		desc.qw0 = QI_EIOTLB_PASID(pasid) |
 				QI_EIOTLB_DID(did) |
-- 
2.25.1

[PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Lu Baolu]

The VT-d hardware will ignore those Addr bits which have been masked by
the AM field in the PASID-based-IOTLB invalidation descriptor. As the
result, if the starting address in the descriptor is not aligned with
the address mask, some IOTLB caches might not invalidate. Hence people
will see below errors.

[ 1093.704661] dmar_fault: 29 callbacks suppressed
[ 1093.704664] DMAR: DRHD: handling fault status reg 3
[ 1093.712738] DMAR: [DMA Read] Request device [7a:02.0] PASID 2
               fault addr 7f81c968d000 [fault reason 113]
               SM: Present bit in first-level paging entry is clear

Fix this by using aligned address for PASID-based-IOTLB invalidation.

Fixes: 1c4f88b7f1f92 ("iommu/vt-d: Shared virtual address in scalable mode")
Reported-and-tested-by: Guo Kaijie <Kaijie.Guo@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 69566695d032..b16a4791acfb 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -118,8 +118,10 @@ void intel_svm_check(struct intel_iommu *iommu)
 	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
 }
 
-static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_dev *sdev,
-				unsigned long address, unsigned long pages, int ih)
+static void __flush_svm_range_dev(struct intel_svm *svm,
+				  struct intel_svm_dev *sdev,
+				  unsigned long address,
+				  unsigned long pages, int ih)
 {
 	struct qi_desc desc;
 
@@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_d
 	}
 }
 
+static void intel_flush_svm_range_dev(struct intel_svm *svm,
+				      struct intel_svm_dev *sdev,
+				      unsigned long address,
+				      unsigned long pages, int ih)
+{
+	unsigned long shift = ilog2(__roundup_pow_of_two(pages));
+	unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
+	unsigned long start = ALIGN_DOWN(address, align);
+	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
+
+	while (start < end) {
+		__flush_svm_range_dev(svm, sdev, start, align >> VTD_PAGE_SHIFT, ih);
+		start += align;
+	}
+}
+
 static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,
 				unsigned long pages, int ih)
 {
-- 
2.25.1

[PATCH 3/5] iommu/vt-d: Remove unused dma map/unmap trace events

[Lu Baolu]

With commit c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to
the iommu ops"), the trace events for dma map/unmap have no users any
more. Remove them so that they don't show up under
/sys/kernel/debug/tracing/events/intel_iommu. The users should use the
map/unmap traces defined in the iommu core from now on.

Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu ops")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/trace/events/intel_iommu.h | 119 -----------------------------
 1 file changed, 119 deletions(-)

diff --git a/include/trace/events/intel_iommu.h b/include/trace/events/intel_iommu.h
index 112bd06487bf..31c74202d8c3 100644
--- a/include/trace/events/intel_iommu.h
+++ b/include/trace/events/intel_iommu.h
@@ -16,125 +16,6 @@
 #include <linux/tracepoint.h>
 #include <linux/intel-iommu.h>
 
-DECLARE_EVENT_CLASS(dma_map,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, phys_addr_t phys_addr,
-		 size_t size),
-
-	TP_ARGS(dev, dev_addr, phys_addr, size),
-
-	TP_STRUCT__entry(
-		__string(dev_name, dev_name(dev))
-		__field(dma_addr_t, dev_addr)
-		__field(phys_addr_t, phys_addr)
-		__field(size_t,	size)
-	),
-
-	TP_fast_assign(
-		__assign_str(dev_name, dev_name(dev));
-		__entry->dev_addr = dev_addr;
-		__entry->phys_addr = phys_addr;
-		__entry->size = size;
-	),
-
-	TP_printk("dev=%s dev_addr=0x%llx phys_addr=0x%llx size=%zu",
-		  __get_str(dev_name),
-		  (unsigned long long)__entry->dev_addr,
-		  (unsigned long long)__entry->phys_addr,
-		  __entry->size)
-);
-
-DEFINE_EVENT(dma_map, map_single,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, phys_addr_t phys_addr,
-		 size_t size),
-	TP_ARGS(dev, dev_addr, phys_addr, size)
-);
-
-DEFINE_EVENT(dma_map, bounce_map_single,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, phys_addr_t phys_addr,
-		 size_t size),
-	TP_ARGS(dev, dev_addr, phys_addr, size)
-);
-
-DECLARE_EVENT_CLASS(dma_unmap,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, size_t size),
-
-	TP_ARGS(dev, dev_addr, size),
-
-	TP_STRUCT__entry(
-		__string(dev_name, dev_name(dev))
-		__field(dma_addr_t, dev_addr)
-		__field(size_t,	size)
-	),
-
-	TP_fast_assign(
-		__assign_str(dev_name, dev_name(dev));
-		__entry->dev_addr = dev_addr;
-		__entry->size = size;
-	),
-
-	TP_printk("dev=%s dev_addr=0x%llx size=%zu",
-		  __get_str(dev_name),
-		  (unsigned long long)__entry->dev_addr,
-		  __entry->size)
-);
-
-DEFINE_EVENT(dma_unmap, unmap_single,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, size_t size),
-	TP_ARGS(dev, dev_addr, size)
-);
-
-DEFINE_EVENT(dma_unmap, unmap_sg,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, size_t size),
-	TP_ARGS(dev, dev_addr, size)
-);
-
-DEFINE_EVENT(dma_unmap, bounce_unmap_single,
-	TP_PROTO(struct device *dev, dma_addr_t dev_addr, size_t size),
-	TP_ARGS(dev, dev_addr, size)
-);
-
-DECLARE_EVENT_CLASS(dma_map_sg,
-	TP_PROTO(struct device *dev, int index, int total,
-		 struct scatterlist *sg),
-
-	TP_ARGS(dev, index, total, sg),
-
-	TP_STRUCT__entry(
-		__string(dev_name, dev_name(dev))
-		__field(dma_addr_t, dev_addr)
-		__field(phys_addr_t, phys_addr)
-		__field(size_t,	size)
-		__field(int, index)
-		__field(int, total)
-	),
-
-	TP_fast_assign(
-		__assign_str(dev_name, dev_name(dev));
-		__entry->dev_addr = sg->dma_address;
-		__entry->phys_addr = sg_phys(sg);
-		__entry->size = sg->dma_length;
-		__entry->index = index;
-		__entry->total = total;
-	),
-
-	TP_printk("dev=%s [%d/%d] dev_addr=0x%llx phys_addr=0x%llx size=%zu",
-		  __get_str(dev_name), __entry->index, __entry->total,
-		  (unsigned long long)__entry->dev_addr,
-		  (unsigned long long)__entry->phys_addr,
-		  __entry->size)
-);
-
-DEFINE_EVENT(dma_map_sg, map_sg,
-	TP_PROTO(struct device *dev, int index, int total,
-		 struct scatterlist *sg),
-	TP_ARGS(dev, index, total, sg)
-);
-
-DEFINE_EVENT(dma_map_sg, bounce_map_sg,
-	TP_PROTO(struct device *dev, int index, int total,
-		 struct scatterlist *sg),
-	TP_ARGS(dev, index, total, sg)
-);
 #endif /* _TRACE_INTEL_IOMMU_H */
 
 /* This part must be outside protection */
-- 
2.25.1

[PATCH 4/5] Revert "iommu: Add quirk for Intel graphic devices in map_sg"

[Lu Baolu]

This reverts commit 65f746e8285f0a67d43517d86fedb9e29ead49f2.

As commit 8a473dbadccfc ("drm/i915: Fix DMA mapped scatterlist walks") and
commit 934941ed5a307 ("drm/i915: Fix DMA mapped scatterlist lookup") fixed
the DMA scatterlist limitations in the i915 driver, remove this temporary
workaround.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tom Murphy <murphyt7@tcd.ie>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/dma-iommu.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index f0305e6aac1b..4078358ed66e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -863,33 +863,6 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
 	unsigned int cur_len = 0, max_len = dma_get_max_seg_size(dev);
 	int i, count = 0;
 
-	/*
-	 * The Intel graphic driver is used to assume that the returned
-	 * sg list is not combound. This blocks the efforts of converting
-	 * Intel IOMMU driver to dma-iommu api's. Add this quirk to make the
-	 * device driver work and should be removed once it's fixed in i915
-	 * driver.
-	 */
-	if (IS_ENABLED(CONFIG_DRM_I915) && dev_is_pci(dev) &&
-	    to_pci_dev(dev)->vendor == PCI_VENDOR_ID_INTEL &&
-	    (to_pci_dev(dev)->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
-		for_each_sg(sg, s, nents, i) {
-			unsigned int s_iova_off = sg_dma_address(s);
-			unsigned int s_length = sg_dma_len(s);
-			unsigned int s_iova_len = s->length;
-
-			s->offset += s_iova_off;
-			s->length = s_length;
-			sg_dma_address(s) = dma_addr + s_iova_off;
-			sg_dma_len(s) = s_length;
-			dma_addr += s_iova_len;
-
-			pr_info_once("sg combining disabled due to i915 driver\n");
-		}
-
-		return nents;
-	}
-
 	for_each_sg(sg, s, nents, i) {
 		/* Restore this segment's original unaligned fields first */
 		unsigned int s_iova_off = sg_dma_address(s);
-- 
2.25.1

[PATCH 5/5] iommu/vt-d: Fix lockdep splat in sva bind()/unbind()

[Lu Baolu]

Lock(&iommu->lock) without disabling irq causes lockdep warnings.

========================================================
WARNING: possible irq lock inversion dependency detected
5.11.0-rc1+ #828 Not tainted
--------------------------------------------------------
kworker/0:1H/120 just changed the state of lock:
ffffffffad9ea1b8 (device_domain_lock){..-.}-{2:2}, at:
iommu_flush_dev_iotlb.part.0+0x32/0x120
but this lock took another, SOFTIRQ-unsafe lock in the past:
 (&iommu->lock){+.+.}-{2:2}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&iommu->lock);
                               local_irq_disable();
                               lock(device_domain_lock);
                               lock(&iommu->lock);
  <Interrupt>
    lock(device_domain_lock);

 *** DEADLOCK ***

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

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index b16a4791acfb..18a9f05df407 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -299,6 +299,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
 	struct dmar_domain *dmar_domain;
 	struct device_domain_info *info;
 	struct intel_svm *svm = NULL;
+	unsigned long iflags;
 	int ret = 0;
 
 	if (WARN_ON(!iommu) || !data)
@@ -400,12 +401,12 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
 	 * each bind of a new device even with an existing PASID, we need to
 	 * call the nested mode setup function here.
 	 */
-	spin_lock(&iommu->lock);
+	spin_lock_irqsave(&iommu->lock, iflags);
 	ret = intel_pasid_setup_nested(iommu, dev,
 				       (pgd_t *)(uintptr_t)data->gpgd,
 				       data->hpasid, &data->vendor.vtd, dmar_domain,
 				       data->addr_width);
-	spin_unlock(&iommu->lock);
+	spin_unlock_irqrestore(&iommu->lock, iflags);
 	if (ret) {
 		dev_err_ratelimited(dev, "Failed to set up PASID %llu in nested mode, Err %d\n",
 				    data->hpasid, ret);
@@ -505,6 +506,7 @@ intel_svm_bind_mm(struct device *dev, unsigned int flags,
 	struct device_domain_info *info;
 	struct intel_svm_dev *sdev;
 	struct intel_svm *svm = NULL;
+	unsigned long iflags;
 	int pasid_max;
 	int ret;
 
@@ -624,14 +626,14 @@ intel_svm_bind_mm(struct device *dev, unsigned int flags,
 			}
 		}
 
-		spin_lock(&iommu->lock);
+		spin_lock_irqsave(&iommu->lock, iflags);
 		ret = intel_pasid_setup_first_level(iommu, dev,
 				mm ? mm->pgd : init_mm.pgd,
 				svm->pasid, FLPT_DEFAULT_DID,
 				(mm ? 0 : PASID_FLAG_SUPERVISOR_MODE) |
 				(cpu_feature_enabled(X86_FEATURE_LA57) ?
 				 PASID_FLAG_FL5LP : 0));
-		spin_unlock(&iommu->lock);
+		spin_unlock_irqrestore(&iommu->lock, iflags);
 		if (ret) {
 			if (mm)
 				mmu_notifier_unregister(&svm->notifier, mm);
@@ -651,14 +653,14 @@ intel_svm_bind_mm(struct device *dev, unsigned int flags,
 		 * Binding a new device with existing PASID, need to setup
 		 * the PASID entry.
 		 */
-		spin_lock(&iommu->lock);
+		spin_lock_irqsave(&iommu->lock, iflags);
 		ret = intel_pasid_setup_first_level(iommu, dev,
 						mm ? mm->pgd : init_mm.pgd,
 						svm->pasid, FLPT_DEFAULT_DID,
 						(mm ? 0 : PASID_FLAG_SUPERVISOR_MODE) |
 						(cpu_feature_enabled(X86_FEATURE_LA57) ?
 						PASID_FLAG_FL5LP : 0));
-		spin_unlock(&iommu->lock);
+		spin_unlock_irqrestore(&iommu->lock, iflags);
 		if (ret) {
 			kfree(sdev);
 			goto out;
-- 
2.25.1

Re: [PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Will Deacon]

On Thu, Dec 31, 2020 at 08:53:20AM +0800, Lu Baolu wrote:
> The VT-d hardware will ignore those Addr bits which have been masked by
> the AM field in the PASID-based-IOTLB invalidation descriptor. As the
> result, if the starting address in the descriptor is not aligned with
> the address mask, some IOTLB caches might not invalidate. Hence people
> will see below errors.
> 
> [ 1093.704661] dmar_fault: 29 callbacks suppressed
> [ 1093.704664] DMAR: DRHD: handling fault status reg 3
> [ 1093.712738] DMAR: [DMA Read] Request device [7a:02.0] PASID 2
>                fault addr 7f81c968d000 [fault reason 113]
>                SM: Present bit in first-level paging entry is clear
> 
> Fix this by using aligned address for PASID-based-IOTLB invalidation.
> 
> Fixes: 1c4f88b7f1f92 ("iommu/vt-d: Shared virtual address in scalable mode")
> Reported-and-tested-by: Guo Kaijie <Kaijie.Guo@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/svm.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 69566695d032..b16a4791acfb 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -118,8 +118,10 @@ void intel_svm_check(struct intel_iommu *iommu)
>  	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>  }
>  
> -static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_dev *sdev,
> -				unsigned long address, unsigned long pages, int ih)
> +static void __flush_svm_range_dev(struct intel_svm *svm,
> +				  struct intel_svm_dev *sdev,
> +				  unsigned long address,
> +				  unsigned long pages, int ih)
>  {
>  	struct qi_desc desc;
>  
> @@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_d
>  	}
>  }
>  
> +static void intel_flush_svm_range_dev(struct intel_svm *svm,
> +				      struct intel_svm_dev *sdev,
> +				      unsigned long address,
> +				      unsigned long pages, int ih)
> +{
> +	unsigned long shift = ilog2(__roundup_pow_of_two(pages));
> +	unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
> +	unsigned long start = ALIGN_DOWN(address, align);
> +	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
> +
> +	while (start < end) {
> +		__flush_svm_range_dev(svm, sdev, start, align >> VTD_PAGE_SHIFT, ih);
> +		start += align;
> +	}
> +}

Given that this only seems to be called from intel_invalidate_range(), which
has to compute 'pages' only to have it pulled apart again here, perhaps it
would be cleaner for intel_flush_svm_range() to take something like an
'order' argument instead?

What do you think?

Will

Re: [PATCH 3/5] iommu/vt-d: Remove unused dma map/unmap trace events

[Will Deacon]

On Thu, Dec 31, 2020 at 08:53:21AM +0800, Lu Baolu wrote:
> With commit c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to
> the iommu ops"), the trace events for dma map/unmap have no users any
> more. Remove them so that they don't show up under
> /sys/kernel/debug/tracing/events/intel_iommu. The users should use the
> map/unmap traces defined in the iommu core from now on.
> 
> Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu ops")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/trace/events/intel_iommu.h | 119 -----------------------------
>  1 file changed, 119 deletions(-)

Is this needed in 5.11, or can it wait until 5.12?

Will

Re: [PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Lu Baolu]

Hi Will,

Happy New Year!

On 2021/1/6 3:03, Will Deacon wrote:
> On Thu, Dec 31, 2020 at 08:53:20AM +0800, Lu Baolu wrote:
>> The VT-d hardware will ignore those Addr bits which have been masked by
>> the AM field in the PASID-based-IOTLB invalidation descriptor. As the
>> result, if the starting address in the descriptor is not aligned with
>> the address mask, some IOTLB caches might not invalidate. Hence people
>> will see below errors.
>>
>> [ 1093.704661] dmar_fault: 29 callbacks suppressed
>> [ 1093.704664] DMAR: DRHD: handling fault status reg 3
>> [ 1093.712738] DMAR: [DMA Read] Request device [7a:02.0] PASID 2
>>                 fault addr 7f81c968d000 [fault reason 113]
>>                 SM: Present bit in first-level paging entry is clear
>>
>> Fix this by using aligned address for PASID-based-IOTLB invalidation.
>>
>> Fixes: 1c4f88b7f1f92 ("iommu/vt-d: Shared virtual address in scalable mode")
>> Reported-and-tested-by: Guo Kaijie <Kaijie.Guo@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/svm.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 69566695d032..b16a4791acfb 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -118,8 +118,10 @@ void intel_svm_check(struct intel_iommu *iommu)
>>   	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>>   }
>>   
>> -static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_dev *sdev,
>> -				unsigned long address, unsigned long pages, int ih)
>> +static void __flush_svm_range_dev(struct intel_svm *svm,
>> +				  struct intel_svm_dev *sdev,
>> +				  unsigned long address,
>> +				  unsigned long pages, int ih)
>>   {
>>   	struct qi_desc desc;
>>   
>> @@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_d
>>   	}
>>   }
>>   
>> +static void intel_flush_svm_range_dev(struct intel_svm *svm,
>> +				      struct intel_svm_dev *sdev,
>> +				      unsigned long address,
>> +				      unsigned long pages, int ih)
>> +{
>> +	unsigned long shift = ilog2(__roundup_pow_of_two(pages));
>> +	unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
>> +	unsigned long start = ALIGN_DOWN(address, align);
>> +	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
>> +
>> +	while (start < end) {
>> +		__flush_svm_range_dev(svm, sdev, start, align >> VTD_PAGE_SHIFT, ih);
>> +		start += align;
>> +	}
>> +}
> 
> Given that this only seems to be called from intel_invalidate_range(), which
> has to compute 'pages' only to have it pulled apart again here, perhaps it
> would be cleaner for intel_flush_svm_range() to take something like an
> 'order' argument instead?
> 
> What do you think?

We need to clean up here. It's duplicate with the qi_flush_piotlb()
helper. I have a patch under testing for this. I will post it for review
later.

> 
> Will
> 

Best regards,
baolu

Re: [PATCH 3/5] iommu/vt-d: Remove unused dma map/unmap trace events

[Lu Baolu]

Hi Will,

On 2021/1/6 3:04, Will Deacon wrote:
> On Thu, Dec 31, 2020 at 08:53:21AM +0800, Lu Baolu wrote:
>> With commit c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to
>> the iommu ops"), the trace events for dma map/unmap have no users any
>> more. Remove them so that they don't show up under
>> /sys/kernel/debug/tracing/events/intel_iommu. The users should use the
>> map/unmap traces defined in the iommu core from now on.
>>
>> Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu ops")
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   include/trace/events/intel_iommu.h | 119 -----------------------------
>>   1 file changed, 119 deletions(-)
> 
> Is this needed in 5.11, or can it wait until 5.12?

It's necessary for 5.11 as far as I can see. Without this, users still
see the events under /sys/kernel/debug/tracing/events/intel_iommu, but
they will get nothing traced even they enable the events.

> 
> Will
> 

Best regards,
baolu

Re: [PATCH 1/5] iommu/vt-d: Fix misuse of ALIGN in qi_flush_piotlb()

[Will Deacon]

On Thu, 31 Dec 2020 08:53:19 +0800, Lu Baolu wrote:
> Use IS_ALIGNED() instead. Otherwise, an unaligned address will be ignored.

Applied patches 1, 4 and 5 to arm64 (for-next/iommu/fixes), thanks!

[1/5] iommu/vt-d: Fix misuse of ALIGN in qi_flush_piotlb()
      https://git.kernel.org/arm64/c/1efd17e7acb6
[4/5] Revert "iommu: Add quirk for Intel graphic devices in map_sg"
      https://git.kernel.org/arm64/c/4df7b2268ad8
[5/5] iommu/vt-d: Fix lockdep splat in sva bind()/unbind()
      https://git.kernel.org/arm64/c/420d42f6f9db

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH 3/5] iommu/vt-d: Remove unused dma map/unmap trace events

[Will Deacon]

On Wed, Jan 06, 2021 at 09:14:22AM +0800, Lu Baolu wrote:
> On 2021/1/6 3:04, Will Deacon wrote:
> > On Thu, Dec 31, 2020 at 08:53:21AM +0800, Lu Baolu wrote:
> > > With commit c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to
> > > the iommu ops"), the trace events for dma map/unmap have no users any
> > > more. Remove them so that they don't show up under
> > > /sys/kernel/debug/tracing/events/intel_iommu. The users should use the
> > > map/unmap traces defined in the iommu core from now on.
> > > 
> > > Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu ops")
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > ---
> > >   include/trace/events/intel_iommu.h | 119 -----------------------------
> > >   1 file changed, 119 deletions(-)
> > 
> > Is this needed in 5.11, or can it wait until 5.12?
> 
> It's necessary for 5.11 as far as I can see. Without this, users still
> see the events under /sys/kernel/debug/tracing/events/intel_iommu, but
> they will get nothing traced even they enable the events.

I'm just a bit wary about breaking userspace by removing them altogether,
although I see that there's plenty of precedent for removing events from
the include/trace/events directory, so it's probably fine.

However, the patch as-is results in this warning for me:

 | In file included from include/trace/define_trace.h:102,
 |                  from include/trace/events/intel_iommu.h:22,
 |                  from drivers/iommu/intel/trace.c:14:
 | include/trace/trace_events.h:27:23: warning: ‘str__intel_iommu__trace_system_name’ defined but not used [-Wunused-const-variable=]
 |    27 | #define __app__(x, y) str__##x##y
 |       |                       ^~~~~
 | include/trace/trace_events.h:28:21: note: in expansion of macro ‘__app__’
 |    28 | #define __app(x, y) __app__(x, y)
 |       |                     ^~~~~~~
 | include/trace/trace_events.h:30:29: note: in expansion of macro ‘__app’
 |    30 | #define TRACE_SYSTEM_STRING __app(TRACE_SYSTEM_VAR,__trace_system_name)
 |       |                             ^~~~~
 | include/trace/trace_events.h:33:20: note: in expansion of macro ‘TRACE_SYSTEM_STRING’
 |    33 |  static const char TRACE_SYSTEM_STRING[] = \
 |       |                    ^~~~~~~~~~~~~~~~~~~
 | include/trace/trace_events.h:36:1: note: in expansion of macro ‘TRACE_MAKE_SYSTEM_STR’
 |    36 | TRACE_MAKE_SYSTEM_STR();
 |       | ^~~~~~~~~~~~~~~~~~~~~

so I'll drop this for now.

Will

Re: [PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Lu Baolu]

Hi Will,

On 2021/1/6 9:09, Lu Baolu wrote:
> Hi Will,
> 
> Happy New Year!
> 
> On 2021/1/6 3:03, Will Deacon wrote:
>> On Thu, Dec 31, 2020 at 08:53:20AM +0800, Lu Baolu wrote:
>>> The VT-d hardware will ignore those Addr bits which have been masked by
>>> the AM field in the PASID-based-IOTLB invalidation descriptor. As the
>>> result, if the starting address in the descriptor is not aligned with
>>> the address mask, some IOTLB caches might not invalidate. Hence people
>>> will see below errors.
>>>
>>> [ 1093.704661] dmar_fault: 29 callbacks suppressed
>>> [ 1093.704664] DMAR: DRHD: handling fault status reg 3
>>> [ 1093.712738] DMAR: [DMA Read] Request device [7a:02.0] PASID 2
>>>                 fault addr 7f81c968d000 [fault reason 113]
>>>                 SM: Present bit in first-level paging entry is clear
>>>
>>> Fix this by using aligned address for PASID-based-IOTLB invalidation.
>>>
>>> Fixes: 1c4f88b7f1f92 ("iommu/vt-d: Shared virtual address in scalable 
>>> mode")
>>> Reported-and-tested-by: Guo Kaijie <Kaijie.Guo@intel.com>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   drivers/iommu/intel/svm.c | 22 ++++++++++++++++++++--
>>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>>> index 69566695d032..b16a4791acfb 100644
>>> --- a/drivers/iommu/intel/svm.c
>>> +++ b/drivers/iommu/intel/svm.c
>>> @@ -118,8 +118,10 @@ void intel_svm_check(struct intel_iommu *iommu)
>>>       iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>>>   }
>>> -static void intel_flush_svm_range_dev (struct intel_svm *svm, struct 
>>> intel_svm_dev *sdev,
>>> -                unsigned long address, unsigned long pages, int ih)
>>> +static void __flush_svm_range_dev(struct intel_svm *svm,
>>> +                  struct intel_svm_dev *sdev,
>>> +                  unsigned long address,
>>> +                  unsigned long pages, int ih)
>>>   {
>>>       struct qi_desc desc;
>>> @@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev (struct 
>>> intel_svm *svm, struct intel_svm_d
>>>       }
>>>   }
>>> +static void intel_flush_svm_range_dev(struct intel_svm *svm,
>>> +                      struct intel_svm_dev *sdev,
>>> +                      unsigned long address,
>>> +                      unsigned long pages, int ih)
>>> +{
>>> +    unsigned long shift = ilog2(__roundup_pow_of_two(pages));
>>> +    unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
>>> +    unsigned long start = ALIGN_DOWN(address, align);
>>> +    unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), 
>>> align);
>>> +
>>> +    while (start < end) {
>>> +        __flush_svm_range_dev(svm, sdev, start, align >> 
>>> VTD_PAGE_SHIFT, ih);
>>> +        start += align;
>>> +    }
>>> +}
>>
>> Given that this only seems to be called from intel_invalidate_range(), 
>> which
>> has to compute 'pages' only to have it pulled apart again here, 
>> perhaps it
>> would be cleaner for intel_flush_svm_range() to take something like an
>> 'order' argument instead?
>>
>> What do you think?
> 
> We need to clean up here. It's duplicate with the qi_flush_piotlb()
> helper. I have a patch under testing for this. I will post it for review
> later.

I'm sorry, above reply is a little vague.

I meant to say, let's take 'pages' as the argument. We are going to use
qi_flush_piotlb() here to avoid duplicate QI interactions. The
qi_flush_piotlb() helper also takes 'pages', so keep 'pages' here will
make things easier.

My cleanup patch is for v5.12. Can you please take this for v5.11?

Best regards,
baolu

Re: [PATCH 3/5] iommu/vt-d: Remove unused dma map/unmap trace events

[Lu Baolu]

Hi Will,

On 2021/1/7 22:40, Will Deacon wrote:
> On Wed, Jan 06, 2021 at 09:14:22AM +0800, Lu Baolu wrote:
>> On 2021/1/6 3:04, Will Deacon wrote:
>>> On Thu, Dec 31, 2020 at 08:53:21AM +0800, Lu Baolu wrote:
>>>> With commit c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to
>>>> the iommu ops"), the trace events for dma map/unmap have no users any
>>>> more. Remove them so that they don't show up under
>>>> /sys/kernel/debug/tracing/events/intel_iommu. The users should use the
>>>> map/unmap traces defined in the iommu core from now on.
>>>>
>>>> Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu ops")
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>    include/trace/events/intel_iommu.h | 119 -----------------------------
>>>>    1 file changed, 119 deletions(-)
>>>
>>> Is this needed in 5.11, or can it wait until 5.12?
>>
>> It's necessary for 5.11 as far as I can see. Without this, users still
>> see the events under /sys/kernel/debug/tracing/events/intel_iommu, but
>> they will get nothing traced even they enable the events.
> 
> I'm just a bit wary about breaking userspace by removing them altogether,
> although I see that there's plenty of precedent for removing events from
> the include/trace/events directory, so it's probably fine.
> 
> However, the patch as-is results in this warning for me:
> 
>   | In file included from include/trace/define_trace.h:102,
>   |                  from include/trace/events/intel_iommu.h:22,
>   |                  from drivers/iommu/intel/trace.c:14:
>   | include/trace/trace_events.h:27:23: warning: ‘str__intel_iommu__trace_system_name’ defined but not used [-Wunused-const-variable=]
>   |    27 | #define __app__(x, y) str__##x##y
>   |       |                       ^~~~~
>   | include/trace/trace_events.h:28:21: note: in expansion of macro ‘__app__’
>   |    28 | #define __app(x, y) __app__(x, y)
>   |       |                     ^~~~~~~
>   | include/trace/trace_events.h:30:29: note: in expansion of macro ‘__app’
>   |    30 | #define TRACE_SYSTEM_STRING __app(TRACE_SYSTEM_VAR,__trace_system_name)
>   |       |                             ^~~~~
>   | include/trace/trace_events.h:33:20: note: in expansion of macro ‘TRACE_SYSTEM_STRING’
>   |    33 |  static const char TRACE_SYSTEM_STRING[] = \
>   |       |                    ^~~~~~~~~~~~~~~~~~~
>   | include/trace/trace_events.h:36:1: note: in expansion of macro ‘TRACE_MAKE_SYSTEM_STR’
>   |    36 | TRACE_MAKE_SYSTEM_STR();
>   |       | ^~~~~~~~~~~~~~~~~~~~~
> 
> so I'll drop this for now.

Okay, I will rework this. Thanks!

> 
> Will
> 

Best regards,
baolu

Re: [PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Will Deacon]

Hi Lu,

On Fri, Jan 08, 2021 at 07:52:47AM +0800, Lu Baolu wrote:
> On 2021/1/6 9:09, Lu Baolu wrote:
> > On 2021/1/6 3:03, Will Deacon wrote:
> > > On Thu, Dec 31, 2020 at 08:53:20AM +0800, Lu Baolu wrote:
> > > > @@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev
> > > > (struct intel_svm *svm, struct intel_svm_d
> > > >       }
> > > >   }
> > > > +static void intel_flush_svm_range_dev(struct intel_svm *svm,
> > > > +                      struct intel_svm_dev *sdev,
> > > > +                      unsigned long address,
> > > > +                      unsigned long pages, int ih)
> > > > +{
> > > > +    unsigned long shift = ilog2(__roundup_pow_of_two(pages));
> > > > +    unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
> > > > +    unsigned long start = ALIGN_DOWN(address, align);
> > > > +    unsigned long end = ALIGN(address + (pages <<
> > > > VTD_PAGE_SHIFT), align);
> > > > +
> > > > +    while (start < end) {
> > > > +        __flush_svm_range_dev(svm, sdev, start, align >>
> > > > VTD_PAGE_SHIFT, ih);
> > > > +        start += align;
> > > > +    }
> > > > +}
> > > 
> > > Given that this only seems to be called from
> > > intel_invalidate_range(), which
> > > has to compute 'pages' only to have it pulled apart again here,
> > > perhaps it
> > > would be cleaner for intel_flush_svm_range() to take something like an
> > > 'order' argument instead?
> > > 
> > > What do you think?
> > 
> > We need to clean up here. It's duplicate with the qi_flush_piotlb()
> > helper. I have a patch under testing for this. I will post it for review
> > later.
> 
> I'm sorry, above reply is a little vague.
> 
> I meant to say, let's take 'pages' as the argument. We are going to use
> qi_flush_piotlb() here to avoid duplicate QI interactions. The
> qi_flush_piotlb() helper also takes 'pages', so keep 'pages' here will
> make things easier.
> 
> My cleanup patch is for v5.12. Can you please take this for v5.11?

Ah sorry, I didn't realise that was your plan. Please just include this
patch in a series of 2 when you post a fixed version of the trace event
removal and then I'll queue them up next week, as I've already prepared
the pull for today.

Apologies,

Will

Re: [PATCH 2/5] iommu/vt-d: Fix unaligned addresses for intel_flush_svm_range_dev()

[Lu Baolu]

Hi Will,

On 2021/1/8 22:09, Will Deacon wrote:
> Hi Lu,
> 
> On Fri, Jan 08, 2021 at 07:52:47AM +0800, Lu Baolu wrote:
>> On 2021/1/6 9:09, Lu Baolu wrote:
>>> On 2021/1/6 3:03, Will Deacon wrote:
>>>> On Thu, Dec 31, 2020 at 08:53:20AM +0800, Lu Baolu wrote:
>>>>> @@ -170,6 +172,22 @@ static void intel_flush_svm_range_dev
>>>>> (struct intel_svm *svm, struct intel_svm_d
>>>>>        }
>>>>>    }
>>>>> +static void intel_flush_svm_range_dev(struct intel_svm *svm,
>>>>> +                      struct intel_svm_dev *sdev,
>>>>> +                      unsigned long address,
>>>>> +                      unsigned long pages, int ih)
>>>>> +{
>>>>> +    unsigned long shift = ilog2(__roundup_pow_of_two(pages));
>>>>> +    unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
>>>>> +    unsigned long start = ALIGN_DOWN(address, align);
>>>>> +    unsigned long end = ALIGN(address + (pages <<
>>>>> VTD_PAGE_SHIFT), align);
>>>>> +
>>>>> +    while (start < end) {
>>>>> +        __flush_svm_range_dev(svm, sdev, start, align >>
>>>>> VTD_PAGE_SHIFT, ih);
>>>>> +        start += align;
>>>>> +    }
>>>>> +}
>>>>
>>>> Given that this only seems to be called from
>>>> intel_invalidate_range(), which
>>>> has to compute 'pages' only to have it pulled apart again here,
>>>> perhaps it
>>>> would be cleaner for intel_flush_svm_range() to take something like an
>>>> 'order' argument instead?
>>>>
>>>> What do you think?
>>>
>>> We need to clean up here. It's duplicate with the qi_flush_piotlb()
>>> helper. I have a patch under testing for this. I will post it for review
>>> later.
>>
>> I'm sorry, above reply is a little vague.
>>
>> I meant to say, let's take 'pages' as the argument. We are going to use
>> qi_flush_piotlb() here to avoid duplicate QI interactions. The
>> qi_flush_piotlb() helper also takes 'pages', so keep 'pages' here will
>> make things easier.
>>
>> My cleanup patch is for v5.12. Can you please take this for v5.11?
> 
> Ah sorry, I didn't realise that was your plan. Please just include this
> patch in a series of 2 when you post a fixed version of the trace event
> removal and then I'll queue them up next week, as I've already prepared
> the pull for today.

Sure and sorry for my vague reply.

> 
> Apologies,

It's okay. :-)

> 
> Will
> 

Best regards,
baolu