Shift overflow in qi_flush_dev_iotlb

2017-10-20 Thread Tvrtko Ursulin 


Hi all,

Detected by ubsan:

 UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1345:3
 shift exponent 64 is too large for 32-bit type 'int'
 CPU: 2 PID: 1167 Comm: perf_pmu Not tainted 4.14.0-rc5+ #532
 Hardware name: LENOVO 80MX/Lenovo E31-80, BIOS DCCN34WW(V2.03) 12/01/2015
 Call Trace:
  
  dump_stack+0xab/0xfe
  ? _atomic_dec_and_lock+0x112/0x112
  ? get_unsigned_val+0x48/0x91
  ubsan_epilogue+0x9/0x49
  __ubsan_handle_shift_out_of_bounds+0x1ea/0x241
  ? __ubsan_handle_load_invalid_value+0x136/0x136
  ? _raw_spin_unlock_irqrestore+0x32/0x50
  ? qi_submit_sync+0x642/0x820
  ? qi_flush_dev_iotlb+0x158/0x1b0
  qi_flush_dev_iotlb+0x158/0x1b0
  ? qi_flush_iotlb+0x110/0x110
  ? do_raw_spin_lock+0x93/0x130
  iommu_flush_dev_iotlb+0xff/0x170
  iommu_flush_iova+0x168/0x220
  iova_domain_flush+0x2b/0x50
  fq_flush_timeout+0xa6/0x1e0
  ? fq_ring_free+0x260/0x260
  ? call_timer_fn+0xfd/0x600
  call_timer_fn+0x160/0x600
  ? fq_ring_free+0x260/0x260
  ? trace_timer_cancel+0x1d0/0x1d0
  ? _raw_spin_unlock_irq+0x29/0x40
  ? fq_ring_free+0x260/0x260
  ? fq_ring_free+0x260/0x260
  run_timer_softirq+0x3bb/0x9a0
  ? timer_fixup_init+0x30/0x30
  ? __lock_is_held+0x35/0x150
  ? sched_clock_cpu+0x14/0x180
  __do_softirq+0x159/0x9c7
  irq_exit+0x118/0x170
  smp_apic_timer_interrupt+0xda/0x530
  apic_timer_interrupt+0x9d/0xb0
  
 RIP: 0010:__asan_load4+0xa/0x80
 RSP: 0018:88012f647380 EFLAGS: 0246 ORIG_RAX: ff10
 RAX: 7fff RBX: 88012f647548 RCX: 9a07ea01
 RDX: dc00 RSI: 88012f647808 RDI: 88012f647548
 RBP: 88012f64 R08: fbfff3b27e96 R09: fbfff3b27e95
 R10: 9d93f4ad R11: fbfff3b27e96 R12: 88012f648000
 R13: 88012f647558 R14: 88012f647550 R15: 88012f647808
  ? stack_access_ok+0x61/0x110
  stack_access_ok+0x6d/0x110
  deref_stack_reg+0x64/0x120
  ? __read_once_size_nocheck.constprop.3+0x50/0x50
  ? deref_stack_reg+0x120/0x120
  ? __kmalloc+0x13a/0x550
  unwind_next_frame+0xc04/0x14e0
  ? kasan_kmalloc+0xa0/0xd0
  ? __kmalloc+0x13a/0x550
  ? __kmalloc+0x13a/0x550
  ? deref_stack_reg+0x120/0x120
  ? unwind_next_frame+0xf3e/0x14e0
  ? i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
  __save_stack_trace+0x7a/0x120
  ? __kmalloc+0x13a/0x550
  save_stack+0x33/0xa0
  ? save_stack+0x33/0xa0
  ? kasan_kmalloc+0xa0/0xd0
  ? _raw_spin_unlock+0x24/0x30
  ? deactivate_slab+0x650/0xb90
  ? ___slab_alloc+0x3e0/0x940
  ? ___slab_alloc+0x3e0/0x940
  ? i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
  ? __lock_is_held+0x35/0x150
  ? mark_held_locks+0x33/0x130
  ? kasan_unpoison_shadow+0x30/0x40
  kasan_kmalloc+0xa0/0xd0
  __kmalloc+0x13a/0x550
  ? depot_save_stack+0x16a/0x7f0
  i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
  ? save_stack+0x92/0xa0
  ? eb_relocate_slow+0x890/0x890 [i915]
  ? debug_check_no_locks_freed+0x200/0x200
  ? ___slab_alloc+0x3e0/0x940
  ? ___slab_alloc+0x3e0/0x940
  ? i915_gem_execbuffer2+0xdb/0x5f0 [i915]
  ? __lock_is_held+0x35/0x150
  ? i915_gem_execbuffer+0x580/0x580 [i915]
  i915_gem_execbuffer2+0x2c1/0x5f0 [i915]
  ? i915_gem_execbuffer+0x580/0x580 [i915]
  ? lock_downgrade+0x310/0x310
  ? i915_gem_execbuffer+0x580/0x580 [i915]
  drm_ioctl_kernel+0xdc/0x190
  drm_ioctl+0x46a/0x6e0
  ? i915_gem_execbuffer+0x580/0x580 [i915]
  ? drm_setversion+0x430/0x430
  ? lock_downgrade+0x310/0x310
  do_vfs_ioctl+0x138/0xbf0
  ? ioctl_preallocate+0x180/0x180
  ? do_raw_spin_unlock+0xf5/0x1d0
  ? do_raw_spin_trylock+0x90/0x90
  ? task_work_run+0x35/0x120
  ? mark_held_locks+0x33/0x130
  ? _raw_spin_unlock_irq+0x29/0x40
  ? mark_held_locks+0x33/0x130
  ? entry_SYSCALL_64_fastpath+0x5/0xad
  ? trace_hardirqs_on_caller+0x184/0x360
  SyS_ioctl+0x3b/0x70
  entry_SYSCALL_64_fastpath+0x18/0xad
 RIP: 0033:0x7fc0e1f0d587
 RSP: 002b:7ffcfd9929f8 EFLAGS: 0246 ORIG_RAX: 0010
 RAX: ffda RBX: 0006 RCX: 7fc0e1f0d587
 RDX: 7ffcfd992a90 RSI: 40406469 RDI: 0003
 RBP: 0003 R08: 7ffcfd992a50 R09: 
 R10: 0073 R11: 0246 R12: 0001
 R13:  R14:  R15: 

Code is:

void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
u64 addr, unsigned mask)
{
struct qi_desc desc;

if (mask) {
BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));

^^^ This last line is where the warning comes. Digging deeper 
VTD_PAGE_SHIFT is 12 and the passed in mask comes from iommu_flush_iova 
(via iommu_flush_dev_iotlb) as MAX_AGAW_PFN_WIDTH which is:


#define MAX_AGAW_WIDTH 64
#define MAX_AGAW_PFN_WIDTH  (MAX_AGAW_WIDTH - VTD_PAGE_SHIFT)

So mask is 64, which overflows the left shift in the BUG_ON.

Regards,

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

Re: [Intel-gfx] [RFC 06/17] drm: i915: fix sg_table nents vs. orig_nents misuse

2020-04-28 Thread Tvrtko Ursulin 



On 28/04/2020 14:19, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
numer of the created entries in the DMA address space. However the
subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
called with the original number of entries passed to dma_map_sg. The
sg_table->nents in turn holds the result of the dma_map_sg call as stated
in include/linux/scatterlist.h. Adapt the code to obey those rules.

Signed-off-by: Marek Szyprowski 
---
  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c   | 13 +++--
  drivers/gpu/drm/i915/gem/i915_gem_internal.c |  4 ++--
  drivers/gpu/drm/i915/gem/i915_gem_region.c   |  4 ++--
  drivers/gpu/drm/i915/gem/i915_gem_shmem.c|  5 +++--
  drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 10 +-
  drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c |  5 +++--
  drivers/gpu/drm/i915/gt/intel_ggtt.c | 12 ++--
  drivers/gpu/drm/i915/i915_gem_gtt.c  | 12 +++-
  drivers/gpu/drm/i915/i915_scatterlist.c  |  4 ++--
  drivers/gpu/drm/i915/selftests/scatterlist.c |  8 
  10 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c 
b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 7db5a79..d829852 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -36,21 +36,22 @@ static struct sg_table *i915_gem_map_dma_buf(struct 
dma_buf_attachment *attachme
goto err_unpin_pages;
}
  
-	ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);

+   ret = sg_alloc_table(st, obj->mm.pages->orig_nents, GFP_KERNEL);
if (ret)
goto err_free;
  
  	src = obj->mm.pages->sgl;

dst = st->sgl;
-   for (i = 0; i < obj->mm.pages->nents; i++) {
+   for (i = 0; i < obj->mm.pages->orig_nents; i++) {
sg_set_page(dst, sg_page(src), src->length, 0);
dst = sg_next(dst);
src = sg_next(src);
}
  
-	if (!dma_map_sg_attrs(attachment->dev,

- st->sgl, st->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC)) {
+   st->nents = dma_map_sg_attrs(attachment->dev,
+st->sgl, st->orig_nents, dir,
+DMA_ATTR_SKIP_CPU_SYNC);
+   if (!st->nents) {
ret = -ENOMEM;
goto err_free_sg;
}
@@ -74,7 +75,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment 
*attachment,
struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
  
  	dma_unmap_sg_attrs(attachment->dev,

-  sg->sgl, sg->nents, dir,
+  sg->sgl, sg->orig_nents, dir,
   DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sg);
kfree(sg);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c 
b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index cbbff81..a8ebfdd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -73,7 +73,7 @@ static int i915_gem_object_get_pages_internal(struct 
drm_i915_gem_object *obj)
}
  
  	sg = st->sgl;

-   st->nents = 0;
+   st->nents = st->orig_nents = 0;
sg_page_sizes = 0;
  
  	do {

@@ -94,7 +94,7 @@ static int i915_gem_object_get_pages_internal(struct 
drm_i915_gem_object *obj)
  
  		sg_set_page(sg, page, PAGE_SIZE << order, 0);

sg_page_sizes |= PAGE_SIZE << order;
-   st->nents++;
+   st->nents = st->orig_nents = st->nents + 1;
  
  		npages -= 1 << order;

if (!npages) {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index 1515384..58ca560 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -53,7 +53,7 @@
GEM_BUG_ON(list_empty(blocks));
  
  	sg = st->sgl;

-   st->nents = 0;
+   st->nents = st->orig_nents = 0;
sg_page_sizes = 0;
prev_end = (resource_size_t)-1;
  
@@ -78,7 +78,7 @@
  
  			sg->length = block_size;
  
-			st->nents++;

+   st->nents = st->orig_nents = st->nents + 1;
} else {
sg->length += block_size;
sg_dma_len(sg) += block_size;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 5d5d7ee..851a732 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -80,7 +80,7 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
noreclaim |= __GFP_NORETRY | __GFP_NOWARN;
  
  	sg = st->sgl;

-   st->nents = 0;
+   st->nents = st->orig_nents = 0;
sg_page_sizes = 0;
for (i = 0; i < page_count; i++) {

Re: [Intel-gfx] i915 and swiotlb_max_segment

2021-06-03 Thread Tvrtko Ursulin 



Hi,

On 03/06/2021 09:40, Joonas Lahtinen wrote:

+ Tvrtko to take a look

Quoting Konrad Rzeszutek Wilk (2021-05-20 18:12:58)

On Mon, May 10, 2021 at 05:25:25PM +0200, Christoph Hellwig wrote:

Hi all,

swiotlb_max_segment is a rather strange "API" export by swiotlb.c,
and i915 is the only (remaining) user.

swiotlb_max_segment returns 0 if swiotlb is not in use, 1 if
SWIOTLB_FORCE is set or swiotlb-zen is set, and the swiotlb segment
size when swiotlb is otherwise enabled.

i915 then uses it to:

  a) decided on the max order in i915_gem_object_get_pages_internal
  b) decide on a max segment size in i915_sg_segment_size

for a) it really seems i915 should switch to dma_alloc_noncoherent
or dma_alloc_noncontigous ASAP instead of using alloc_page and
streaming DMA mappings.  Any chance I could trick one of the i915
maintaines into doing just that given that the callchain is not
exactly trivial?

For b) I'm not sure swiotlb and i915 really agree on the meaning
of the value.  swiotlb_set_max_segment basically returns the entire
size of the swiotlb buffer, while i915 seems to use it to limit
the size each scatterlist entry.  It seems like dma_max_mapping_size
might be the best value to use here.


Yes. The background behind that was SWIOTLB would fail because well, the
size of the sg was too large. And some way to limit it to max size
was needed - the dma_max_mapping_size "should" be just fine.


Can't say I am 100% at home here but what I remember is that the limiting 
factor was maximum size of a sg segment and not total size of the mapping.

Looking at the code today, if we would replace usage swiotlb_max_segment() with 
dma_max_mapping_size(), I don't see that would work when we call 
dma_map_sg_attrs().

Because AFAICT code can end up in dma_direct_max_mapping_size() (not sure when the 
ops->map_sg path is active and where to trace that) where we have:

size_t dma_direct_max_mapping_size(struct device *dev)
{
/* If SWIOTLB is active, use its maximum mapping size */
if (is_swiotlb_active() &&
(dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
return swiotlb_max_mapping_size(dev);
return SIZE_MAX;
}

So for all swiotlb cases, including force, we get:

size_t swiotlb_max_mapping_size(struct device *dev)
{
return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE;
}

Which is fixed and doesn't align with swiotlb_max_segment(). But you guys are 
the experts here so please feel to correct me.

Regards,

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

[PATCH] iommu/vt-d: Do not falsely log intel_iommu is unsupported kernel option

2021-08-31 Thread Tvrtko Ursulin 
From: Tvrtko Ursulin 

Handling of intel_iommu kernel command line option should return "true" to
indicate option is valid and so avoid logging it as unknown by the core
parsing code.

Also log unknown sub-options at the notice level to let user know of
potential typos or similar.

Signed-off-by: Tvrtko Ursulin 
Reported-by: Eero Tamminen 
Cc: Lu Baolu 
---
 drivers/iommu/intel/iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 327f477a8553..41d163e275b2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -463,13 +463,15 @@ static int __init intel_iommu_setup(char *str)
} else if (!strncmp(str, "tboot_noforce", 13)) {
pr_info("Intel-IOMMU: not forcing on after tboot. This 
could expose security risk for tboot\n");
intel_iommu_tboot_noforce = 1;
+   } else {
+   pr_notice("Unknown option - '%s'\n", str);
}
 
str += strcspn(str, ",");
while (*str == ',')
str++;
}
-   return 0;
+   return 1;
 }
 __setup("intel_iommu=", intel_iommu_setup);
 
-- 
2.30.2

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

Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api

2020-09-08 Thread Tvrtko Ursulin 



Hi,

On 27/08/2020 22:36, Logan Gunthorpe wrote:

On 2020-08-23 6:04 p.m., Tom Murphy wrote:

I have added a check for the sg_dma_len == 0 :
"""
  } __sgt_iter(struct scatterlist *sgl, bool dma) {
 struct sgt_iter s = { .sgp = sgl };

+   if (sgl && sg_dma_len(sgl) == 0)
+   s.sgp = NULL;

 if (s.sgp) {
 .
"""
at location [1].
but it doens't fix the problem.


Based on my read of the code, it looks like we also need to change usage
of sgl->length... Something like the rough patch below, maybe?


This thread was brought to my attention and I initially missed this 
reply. Essentially I came to the same conclusion about the need to use 
sg_dma_len. One small correction below:



Also, Tom, do you have an updated version of the patchset to convert the
Intel IOMMU to dma-iommu available? The last one I've found doesn't
apply cleanly (I'm assuming parts of it have been merged in slightly
modified forms).

Thanks,

Logan

--

diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
  } __sgt_iter(struct scatterlist *sgl, bool dma) {
 struct sgt_iter s = { .sgp = sgl };

+   if (sgl && !sg_dma_len(s.sgp))


I'd extend the condition to be, just to be safe:

if (dma && sgl && !sg_dma_len(s.sgp))


+   s.sgp = NULL;
+
 if (s.sgp) {
 s.max = s.curr = s.sgp->offset;
-   s.max += s.sgp->length;
-   if (dma)
+
+   if (dma) {
+   s.max += sg_dma_len(s.sgp);
 s.dma = sg_dma_address(s.sgp);
-   else
+   } else {
+   s.max += s.sgp->length;
 s.pfn = page_to_pfn(sg_page(s.sgp));
+   }


Otherwise has this been tested or alternatively how to test it? (How to 
repro the issue.)


Regards,

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

Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api

2020-09-08 Thread Tvrtko Ursulin 


On 08/09/2020 16:44, Logan Gunthorpe wrote:

On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:


diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
   } __sgt_iter(struct scatterlist *sgl, bool dma) {
  struct sgt_iter s = { .sgp = sgl };

+   if (sgl && !sg_dma_len(s.sgp))


I'd extend the condition to be, just to be safe:
 if (dma && sgl && !sg_dma_len(s.sgp))



Right, good catch, that's definitely necessary.


+   s.sgp = NULL;
+
  if (s.sgp) {
  s.max = s.curr = s.sgp->offset;
-   s.max += s.sgp->length;
-   if (dma)
+
+   if (dma) {
+   s.max += sg_dma_len(s.sgp);
  s.dma = sg_dma_address(s.sgp);
-   else
+   } else {
+   s.max += s.sgp->length;
  s.pfn = page_to_pfn(sg_page(s.sgp));
+   }


Otherwise has this been tested or alternatively how to test it? (How to
repro the issue.)


It has not been tested. To test it, you need Tom's patch set without the
last "DO NOT MERGE" patch:

https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/


Tom, do you have a branch somewhere I could pull from? (Just being lazy 
about downloading a bunch of messages from the archives.)


What GPU is in your Lenovo x1 carbon 5th generation and what 
graphical/desktop setup I need to repro?


Regards,

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

Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api

2020-09-09 Thread Tvrtko Ursulin 



On 08/09/2020 23:43, Tom Murphy wrote:

On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
 wrote:



On 08/09/2020 16:44, Logan Gunthorpe wrote:

On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:


diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
} __sgt_iter(struct scatterlist *sgl, bool dma) {
   struct sgt_iter s = { .sgp = sgl };

+   if (sgl && !sg_dma_len(s.sgp))


I'd extend the condition to be, just to be safe:
  if (dma && sgl && !sg_dma_len(s.sgp))



Right, good catch, that's definitely necessary.


+   s.sgp = NULL;
+
   if (s.sgp) {
   s.max = s.curr = s.sgp->offset;
-   s.max += s.sgp->length;
-   if (dma)
+
+   if (dma) {
+   s.max += sg_dma_len(s.sgp);
   s.dma = sg_dma_address(s.sgp);
-   else
+   } else {
+   s.max += s.sgp->length;
   s.pfn = page_to_pfn(sg_page(s.sgp));
+   }


Otherwise has this been tested or alternatively how to test it? (How to
repro the issue.)


It has not been tested. To test it, you need Tom's patch set without the
last "DO NOT MERGE" patch:

https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/


Tom, do you have a branch somewhere I could pull from? (Just being lazy
about downloading a bunch of messages from the archives.)


I don't unfortunately. I'm working locally with poor internet.



What GPU is in your Lenovo x1 carbon 5th generation and what
graphical/desktop setup I need to repro?



Is this enough info?:

$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
 Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
 Flags: bus master, fast devsel, latency 0, IRQ 148
 Memory at eb00 (64-bit, non-prefetchable) [size=16M]
 Memory at 6000 (64-bit, prefetchable) [size=256M]
 I/O ports at e000 [size=64]
 [virtual] Expansion ROM at 000c [disabled] [size=128K]
 Capabilities: [40] Vendor Specific Information: Len=0c 
 Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
 Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
 Capabilities: [d0] Power Management version 2
 Capabilities: [100] Process Address Space ID (PASID)
 Capabilities: [200] Address Translation Service (ATS)


Works for a start. What about the steps to repro? Any desktop 
environment and it is just visual corruption, no hangs/stalls or such?


I've submitted a series consisting of what I understood are the patches 
needed to repro the issue to our automated CI here:


https://patchwork.freedesktop.org/series/81489/

So will see if it will catch something, or more targeted testing will be 
required. Hopefully it does trip over in which case I can add the patch 
suggested by Logan on top and see if that fixes it. Or I'll need to 
write a new test case.


If you could glance over my series to check I identified the patches 
correctly it would be appreciated.


Regards,

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

Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api

2020-09-09 Thread Tvrtko Ursulin 


On 09/09/2020 10:16, Tvrtko Ursulin wrote:

On 08/09/2020 23:43, Tom Murphy wrote:

On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
 wrote:

On 08/09/2020 16:44, Logan Gunthorpe wrote:

On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:


diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
    } __sgt_iter(struct scatterlist *sgl, bool dma) {
   struct sgt_iter s = { .sgp = sgl };

+   if (sgl && !sg_dma_len(s.sgp))


I'd extend the condition to be, just to be safe:
  if (dma && sgl && !sg_dma_len(s.sgp))



Right, good catch, that's definitely necessary.


+   s.sgp = NULL;
+
   if (s.sgp) {
   s.max = s.curr = s.sgp->offset;
-   s.max += s.sgp->length;
-   if (dma)
+
+   if (dma) {
+   s.max += sg_dma_len(s.sgp);
   s.dma = sg_dma_address(s.sgp);
-   else
+   } else {
+   s.max += s.sgp->length;
   s.pfn = page_to_pfn(sg_page(s.sgp));
+   }


Otherwise has this been tested or alternatively how to test it? 
(How to

repro the issue.)


It has not been tested. To test it, you need Tom's patch set without 
the

last "DO NOT MERGE" patch:

https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/


Tom, do you have a branch somewhere I could pull from? (Just being lazy
about downloading a bunch of messages from the archives.)


I don't unfortunately. I'm working locally with poor internet.



What GPU is in your Lenovo x1 carbon 5th generation and what
graphical/desktop setup I need to repro?



Is this enough info?:

$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
 Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
 Flags: bus master, fast devsel, latency 0, IRQ 148
 Memory at eb00 (64-bit, non-prefetchable) [size=16M]
 Memory at 6000 (64-bit, prefetchable) [size=256M]
 I/O ports at e000 [size=64]
 [virtual] Expansion ROM at 000c [disabled] [size=128K]
 Capabilities: [40] Vendor Specific Information: Len=0c 
 Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
 Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
 Capabilities: [d0] Power Management version 2
 Capabilities: [100] Process Address Space ID (PASID)
 Capabilities: [200] Address Translation Service (ATS)


Works for a start. What about the steps to repro? Any desktop 
environment and it is just visual corruption, no hangs/stalls or such?


I've submitted a series consisting of what I understood are the patches 
needed to repro the issue to our automated CI here:


https://patchwork.freedesktop.org/series/81489/

So will see if it will catch something, or more targeted testing will be 
required. Hopefully it does trip over in which case I can add the patch 
suggested by Logan on top and see if that fixes it. Or I'll need to 
write a new test case.


If you could glance over my series to check I identified the patches 
correctly it would be appreciated.


Our CI was more than capable at catching the breakage so I've copied you 
on a patch (https://patchwork.freedesktop.org/series/81497/) which has a 
good potential to fix this. (Or improve the robustness of our sg walks, 
depends how you look at it.)


Would you be able to test it in your environment by any chance? If it 
works I understand it unblocks your IOMMU work, right?


Regards,

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

Re: [PATCH v3 0/6] Convert the intel iommu driver to the dma-iommu api

2020-09-14 Thread Tvrtko Ursulin 


Hi,

On 12/09/2020 04:21, Lu Baolu wrote:
> Tom Murphy has almost done all the work. His latest patch series was
> posted here.
> 
> https://lore.kernel.org/linux-iommu/20200903201839.7327-1-murph...@tcd.ie/
> 
> Thanks a lot!
> 
> This series is a follow-up with below changes:
> 
> 1. Add a quirk for the i915 driver issue described in Tom's cover
> letter.

Last week I have copied you on an i915 series which appears to remove the need 
for this quirk. so if we get those i915 patches reviewed and merged, do you 
still want to pursue this quirk?

> 2. Fix several bugs in patch "iommu: Allow the dma-iommu api to use
> bounce buffers" to make the bounce buffer work for untrusted devices.
> 3. Several cleanups in iommu/vt-d driver after the conversion.

With the previous version of the series I hit a problem on Ivybridge where 
apparently the dma engine width is not respected. At least that is my layman 
interpretation of the errors. From the older thread:

<3> [209.526605] DMAR: intel_iommu_map: iommu width (39) is not sufficient for 
the mapped address (008000)

Relevant iommu boot related messages are:

<6>[0.184234] DMAR: Host address width 36
<6>[0.184245] DMAR: DRHD base: 0x00fed9 flags: 0x0
<6>[0.184288] DMAR: dmar0: reg_base_addr fed9 ver 1:0 cap 
c020e60262 ecap f0101a
<6>[0.184308] DMAR: DRHD base: 0x00fed91000 flags: 0x1
<6>[0.184337] DMAR: dmar1: reg_base_addr fed91000 ver 1:0 cap 
c9008020660262 ecap f0105a
<6>[0.184357] DMAR: RMRR base: 0x00d8d28000 end: 0x00d8d46fff
<6>[0.184377] DMAR: RMRR base: 0x00db00 end: 0x00df1f
<6>[0.184398] DMAR-IR: IOAPIC id 2 under DRHD base  0xfed91000 IOMMU 1
<6>[0.184414] DMAR-IR: HPET id 0 under DRHD base 0xfed91000
<6>[0.184428] DMAR-IR: Queued invalidation will be enabled to support 
x2apic and Intr-remapping.
<6>[0.185173] DMAR-IR: Enabled IRQ remapping in x2apic mode

<6>[0.878934] DMAR: No ATSR found
<6>[0.878966] DMAR: dmar0: Using Queued invalidation
<6>[0.879007] DMAR: dmar1: Using Queued invalidation

<6>[0.915032] DMAR: Intel(R) Virtualization Technology for Directed I/O
<6>[0.915060] PCI-DMA: Using software bounce buffering for IO (SWIOTLB)
<6>[0.915084] software IO TLB: mapped [mem 0xc80d4000-0xcc0d4000] (64MB)

(Full boot log at 
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_7054/fi-ivb-3770/boot0.txt, 
failures at 
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_7054/fi-ivb-3770/igt@i915_selftest@l...@blt.html.)

Does this look familiar or at least plausible to you? Is this something your 
new series has fixed?

Regards,

Tvrtko

> 
> Please review and test.
> 
> Best regards,
> baolu
> 
> Lu Baolu (2):
>iommu: Add quirk for Intel graphic devices in map_sg
>iommu/vt-d: Cleanup after converting to dma-iommu ops
> 
> Tom Murphy (4):
>iommu: Handle freelists when using deferred flushing in iommu drivers
>iommu: Add iommu_dma_free_cpu_cached_iovas()
>iommu: Allow the dma-iommu api to use bounce buffers
>iommu/vt-d: Convert intel iommu driver to the iommu ops
> 
>   .../admin-guide/kernel-parameters.txt |   5 -
>   drivers/iommu/dma-iommu.c | 229 -
>   drivers/iommu/intel/Kconfig   |   1 +
>   drivers/iommu/intel/iommu.c   | 885 +++---
>   include/linux/dma-iommu.h |   8 +
>   include/linux/iommu.h |   1 +
>   6 files changed, 323 insertions(+), 806 deletions(-)
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 0/6] Convert the intel iommu driver to the dma-iommu api

2020-09-15 Thread Tvrtko Ursulin 


On 15/09/2020 02:47, Lu Baolu wrote:

Hi Tvrtko,

On 9/14/20 4:04 PM, Tvrtko Ursulin wrote:


Hi,

On 12/09/2020 04:21, Lu Baolu wrote:

Tom Murphy has almost done all the work. His latest patch series was
posted here.

https://lore.kernel.org/linux-iommu/20200903201839.7327-1-murph...@tcd.ie/ 



Thanks a lot!

This series is a follow-up with below changes:

1. Add a quirk for the i915 driver issue described in Tom's cover
letter.


Last week I have copied you on an i915 series which appears to remove 
the need for this quirk. so if we get those i915 patches reviewed and 
merged, do you still want to pursue this quirk?


It's up to the graphic guys. I don't know the details in i915 driver.
I don't think my tests could cover all cases.


I am the graphic guy. :) I just need some reviews (internally) for my 
series and then we can merge it, at which point you don't need the quirk 
patch any more. I'll try to accelerate this.


With regards to testing, you could send your series with my patches on 
top to our trybot mailing list (intel-gfx-try...@lists.freedesktop.org / 
https://patchwork.freedesktop.org/project/intel-gfx-trybot/series/?ordering=-last_updated) 
which would show you if it is still hitting the DMAR issues in i915.





2. Fix several bugs in patch "iommu: Allow the dma-iommu api to use
bounce buffers" to make the bounce buffer work for untrusted devices.
3. Several cleanups in iommu/vt-d driver after the conversion.


With the previous version of the series I hit a problem on Ivybridge 
where apparently the dma engine width is not respected. At least that 
is my layman interpretation of the errors. From the older thread:


<3> [209.526605] DMAR: intel_iommu_map: iommu width (39) is not 
sufficient for the mapped address (008000)


Relevant iommu boot related messages are:

<6>[    0.184234] DMAR: Host address width 36
<6>[    0.184245] DMAR: DRHD base: 0x00fed9 flags: 0x0
<6>[    0.184288] DMAR: dmar0: reg_base_addr fed9 ver 1:0 cap 
c020e60262 ecap f0101a

<6>[    0.184308] DMAR: DRHD base: 0x00fed91000 flags: 0x1
<6>[    0.184337] DMAR: dmar1: reg_base_addr fed91000 ver 1:0 cap 
c9008020660262 ecap f0105a

<6>[    0.184357] DMAR: RMRR base: 0x00d8d28000 end: 0x00d8d46fff
<6>[    0.184377] DMAR: RMRR base: 0x00db00 end: 0x00df1f
<6>[    0.184398] DMAR-IR: IOAPIC id 2 under DRHD base  0xfed91000 
IOMMU 1

<6>[    0.184414] DMAR-IR: HPET id 0 under DRHD base 0xfed91000
<6>[    0.184428] DMAR-IR: Queued invalidation will be enabled to 
support x2apic and Intr-remapping.

<6>[    0.185173] DMAR-IR: Enabled IRQ remapping in x2apic mode

<6>[    0.878934] DMAR: No ATSR found
<6>[    0.878966] DMAR: dmar0: Using Queued invalidation
<6>[    0.879007] DMAR: dmar1: Using Queued invalidation

<6>[    0.915032] DMAR: Intel(R) Virtualization Technology for 
Directed I/O
<6>[    0.915060] PCI-DMA: Using software bounce buffering for IO 
(SWIOTLB)
<6>[    0.915084] software IO TLB: mapped [mem 0xc80d4000-0xcc0d4000] 
(64MB)


(Full boot log at 
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_7054/fi-ivb-3770/boot0.txt, 
failures at 
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_7054/fi-ivb-3770/igt@i915_selftest@l...@blt.html.) 



Does this look familiar or at least plausible to you? Is this 
something your new series has fixed?


This happens during attaching a domain to device. It has nothing to do
with this patch series. I will look into this issue, but not in this
email thread context.


I am not sure what step is attaching domain to device, but these type 
messages:


<3> [209.526605] DMAR: intel_iommu_map: iommu width (39) is not
>> sufficient for the mapped address (008000)

They definitely appear to happen at runtime, as i915 is getting 
exercised by userspace.


Regards,

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

Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api

2020-09-28 Thread Tvrtko Ursulin 



On 27/09/2020 07:34, Lu Baolu wrote:

Hi,

The previous post of this series could be found here.

https://lore.kernel.org/linux-iommu/20200912032200.11489-1-baolu...@linux.intel.com/

This version introduce a new patch [4/7] to fix an issue reported here.

https://lore.kernel.org/linux-iommu/51a1baec-48d1-c0ac-181b-1fba92aa4...@linux.intel.com/

There aren't any other changes.

Please help to test and review.

Best regards,
baolu

Lu Baolu (3):
   iommu: Add quirk for Intel graphic devices in map_sg


Since I do have patches to fix i915 to handle this, do we want to 
co-ordinate the two and avoid having to add this quirk and then later 
remove it? Or you want to go the staged approach?


Regards,

Tvrtko


   iommu/vt-d: Update domain geometry in iommu_ops.at(de)tach_dev
   iommu/vt-d: Cleanup after converting to dma-iommu ops

Tom Murphy (4):
   iommu: Handle freelists when using deferred flushing in iommu drivers
   iommu: Add iommu_dma_free_cpu_cached_iovas()
   iommu: Allow the dma-iommu api to use bounce buffers
   iommu/vt-d: Convert intel iommu driver to the iommu ops

  .../admin-guide/kernel-parameters.txt |   5 -
  drivers/iommu/dma-iommu.c | 228 -
  drivers/iommu/intel/Kconfig   |   1 +
  drivers/iommu/intel/iommu.c   | 901 +++---
  include/linux/dma-iommu.h |   8 +
  include/linux/iommu.h |   1 +
  6 files changed, 336 insertions(+), 808 deletions(-)


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

Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api

2020-10-12 Thread Tvrtko Ursulin 


On 29/09/2020 01:11, Lu Baolu wrote:

Hi Tvrtko,

On 9/28/20 5:44 PM, Tvrtko Ursulin wrote:


On 27/09/2020 07:34, Lu Baolu wrote:

Hi,

The previous post of this series could be found here.

https://lore.kernel.org/linux-iommu/20200912032200.11489-1-baolu...@linux.intel.com/ 



This version introduce a new patch [4/7] to fix an issue reported here.

https://lore.kernel.org/linux-iommu/51a1baec-48d1-c0ac-181b-1fba92aa4...@linux.intel.com/ 



There aren't any other changes.

Please help to test and review.

Best regards,
baolu

Lu Baolu (3):
   iommu: Add quirk for Intel graphic devices in map_sg


Since I do have patches to fix i915 to handle this, do we want to 
co-ordinate the two and avoid having to add this quirk and then later 
remove it? Or you want to go the staged approach?


I have no preference. It depends on which patch goes first. Let the
maintainers help here.


FYI we have merged the required i915 patches to out tree last week or 
so. I *think* this means they will go into 5.11. So the i915 specific 
workaround patch will not be needed in Intel IOMMU.


Regards,

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

Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api

2020-11-02 Thread Tvrtko Ursulin 


On 02/11/2020 02:00, Lu Baolu wrote:

Hi Tvrtko,
On 10/12/20 4:44 PM, Tvrtko Ursulin wrote:


On 29/09/2020 01:11, Lu Baolu wrote:

Hi Tvrtko,

On 9/28/20 5:44 PM, Tvrtko Ursulin wrote:


On 27/09/2020 07:34, Lu Baolu wrote:

Hi,

The previous post of this series could be found here.

https://lore.kernel.org/linux-iommu/20200912032200.11489-1-baolu...@linux.intel.com/ 



This version introduce a new patch [4/7] to fix an issue reported 
here.


https://lore.kernel.org/linux-iommu/51a1baec-48d1-c0ac-181b-1fba92aa4...@linux.intel.com/ 



There aren't any other changes.

Please help to test and review.

Best regards,
baolu

Lu Baolu (3):
   iommu: Add quirk for Intel graphic devices in map_sg


Since I do have patches to fix i915 to handle this, do we want to 
co-ordinate the two and avoid having to add this quirk and then 
later remove it? Or you want to go the staged approach?


I have no preference. It depends on which patch goes first. Let the
maintainers help here.


FYI we have merged the required i915 patches to out tree last week or 
so. I *think* this means they will go into 5.11. So the i915 specific 
workaround patch will not be needed in Intel IOMMU.


Do you mind telling me what's the status of this fix patch? I tried this
series on v5.10-rc1 with the graphic quirk patch dropped. I am still
seeing dma faults from graphic device.


Hmm back then I thought i915 fixes for this would land in 5.11 so I will 
stick with that. :) (See my quoted text a paragraph above yours.)


Regards,

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

Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api

2020-11-03 Thread Tvrtko Ursulin 



On 03/11/2020 02:53, Lu Baolu wrote:

On 11/2/20 7:52 PM, Tvrtko Ursulin wrote:


On 02/11/2020 02:00, Lu Baolu wrote:

Hi Tvrtko,
On 10/12/20 4:44 PM, Tvrtko Ursulin wrote:


On 29/09/2020 01:11, Lu Baolu wrote:

Hi Tvrtko,

On 9/28/20 5:44 PM, Tvrtko Ursulin wrote:


On 27/09/2020 07:34, Lu Baolu wrote:

Hi,

The previous post of this series could be found here.

https://lore.kernel.org/linux-iommu/20200912032200.11489-1-baolu...@linux.intel.com/ 



This version introduce a new patch [4/7] to fix an issue reported 
here.


https://lore.kernel.org/linux-iommu/51a1baec-48d1-c0ac-181b-1fba92aa4...@linux.intel.com/ 



There aren't any other changes.

Please help to test and review.

Best regards,
baolu

Lu Baolu (3):
   iommu: Add quirk for Intel graphic devices in map_sg


Since I do have patches to fix i915 to handle this, do we want to 
co-ordinate the two and avoid having to add this quirk and then 
later remove it? Or you want to go the staged approach?


I have no preference. It depends on which patch goes first. Let the
maintainers help here.


FYI we have merged the required i915 patches to out tree last week 
or so. I *think* this means they will go into 5.11. So the i915 
specific workaround patch will not be needed in Intel IOMMU.


Do you mind telling me what's the status of this fix patch? I tried this
series on v5.10-rc1 with the graphic quirk patch dropped. I am still
seeing dma faults from graphic device.


Hmm back then I thought i915 fixes for this would land in 5.11 so I 
will stick with that. :) (See my quoted text a paragraph above yours.)


What size are those fixes? I am considering pushing this series for
v5.11. Is it possible to get some acks for those patches and let them
go to Linus through iommu tree?


For 5.10 you mean? They feel a bit too large for comfort to go via a 
non-i915/drm tree. These are the two patches required:


https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-gt-next&id=8a473dbadccfc6206150de3db3223c40785da348
https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-gt-next&id=934941ed5a3070a7833c688c9b1d71484fc01a68

I'll copy Joonas as our maintainer - how does the idea of taking the 
above two patches through the iommu tree sound to you?


Regards,

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