Shift overflow in qi_flush_dev_iotlb
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
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
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
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
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
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
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
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
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
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
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
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
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
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