| Message ID | 20210619034043.199220-7-tientzu@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restricted DMA | expand |
On 6/18/2021 11:40 PM, Claire Chang wrote: > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and > use it to determine whether to bounce the data or not. This will be > useful later to allow for different pools. > > Signed-off-by: Claire Chang <tientzu@chromium.org> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Tested-by: Stefano Stabellini <sstabellini@kernel.org> > Tested-by: Will Deacon <will@kernel.org> > Acked-by: Stefano Stabellini <sstabellini@kernel.org> Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next. [ 22.286574][ T7] Unable to handle kernel paging request at virtual address dfff80000000000e [ 22.295225][ T7] Mem abort info: [ 22.298743][ T7] ESR = 0x96000004 [ 22.302496][ T7] EC = 0x25: DABT (current EL), IL = 32 bits [ 22.308525][ T7] SET = 0, FnV = 0 [ 22.312274][ T7] EA = 0, S1PTW = 0 [ 22.316131][ T7] FSC = 0x04: level 0 translation fault [ 22.321704][ T7] Data abort info: [ 22.325278][ T7] ISV = 0, ISS = 0x00000004 [ 22.329840][ T7] CM = 0, WnR = 0 [ 22.333503][ T7] [dfff80000000000e] address between user and kernel address ranges [ 22.338543][ T256] igb 0006:01:00.0: Intel(R) Gigabit Ethernet Network Connection [ 22.341400][ T7] Internal error: Oops: 96000004 [#1] SMP [ 22.348915][ T256] igb 0006:01:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 4c:38:d5:09:c8:83 [ 22.354458][ T7] Modules linked in: igb(+) i2c_algo_bit nvme mlx5_core(+) i2c_core nvme_core firmware_class [ 22.362512][ T256] igb 0006:01:00.0: eth0: PBA No: G69016-004 [ 22.372287][ T7] CPU: 13 PID: 7 Comm: kworker/u64:0 Not tainted 5.13.0-rc7-next-20210623+ #47 [ 22.372293][ T7] Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 1.6 06/28/2020 [ 22.372298][ T7] Workqueue: nvme-reset-wq nvme_reset_work [nvme] [ 22.378145][ T256] igb 0006:01:00.0: Using MSI-X interrupts. 4 rx queue(s), 4 tx queue(s) [ 22.386901][ T7] [ 22.386905][ T7] pstate: 10000005 (nzcV daif -PAN -UAO -TCO BTYPE=--) [ 22.386910][ T7] pc : dma_direct_map_sg+0x304/0x8f0 is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 (inlined by) dma_direct_map_page at /usr/src/linux-next/kernel/dma/direct.h:90 (inlined by) dma_direct_map_sg at /usr/src/linux-next/kernel/dma/direct.c:428 [ 22.386919][ T7] lr : dma_map_sg_attrs+0x6c/0x118 [ 22.386924][ T7] sp : ffff80001dc8eac0 [ 22.386926][ T7] x29: ffff80001dc8eac0 x28: ffff0000199e70b0 x27: 0000000000000000 [ 22.386935][ T7] x26: ffff000847ee7000 x25: ffff80001158e570 x24: 0000000000000002 [ 22.386943][ T7] x23: dfff800000000000 x22: 0000000000000100 x21: ffff0000199e7460 [ 22.386951][ T7] x20: ffff0000199e7488 x19: 0000000000000001 x18: ffff000010062670 [ 22.386955][ T253] Unable to handle kernel paging request at virtual address dfff80000000000e [ 22.386958][ T7] x17: ffff8000109f6a90 x16: ffff8000109e1b4c x15: ffff800009303420 [ 22.386965][ T253] Mem abort info: [ 22.386967][ T7] x14: 0000000000000001 x13: ffff80001158e000 [ 22.386970][ T253] ESR = 0x96000004 [ 22.386972][ T7] x12: 1fffe00108fdce01 [ 22.386975][ T253] EC = 0x25: DABT (current EL), IL = 32 bits [ 22.386976][ T7] x11: 1fffe00108fdce03 x10: ffff000847ee700c x9 : 0000000000000004 [ 22.386981][ T253] SET = 0, FnV = 0 [ 22.386983][ T7] [ 22.386985][ T7] x8 : ffff700003b91d72 [ 22.386986][ T253] EA = 0, S1PTW = 0 [ 22.386987][ T7] x7 : 0000000000000000 x6 : 000000000000000e [ 22.386990][ T253] FSC = 0x04: level 0 translation fault [ 22.386992][ T7] [ 22.386994][ T7] x5 : dfff800000000000 [ 22.386995][ T253] Data abort info: [ 22.386997][ T7] x4 : 00000008c7ede000 [ 22.386999][ T253] ISV = 0, ISS = 0x00000004 [ 22.386999][ T7] x3 : 00000008c7ede000 [ 22.387003][ T7] x2 : 0000000000001000 [ 22.387003][ T253] CM = 0, WnR = 0 [ 22.387006][ T7] x1 : 0000000000000000 x0 : 0000000000000071 [ 22.387008][ T253] [dfff80000000000e] address between user and kernel address ranges [ 22.387011][ T7] [ 22.387013][ T7] Call trace: [ 22.387016][ T7] dma_direct_map_sg+0x304/0x8f0 [ 22.387022][ T7] dma_map_sg_attrs+0x6c/0x118 [ 22.387026][ T7] nvme_map_data+0x2ec/0x21d8 [nvme] [ 22.387040][ T7] nvme_queue_rq+0x274/0x3f0 [nvme] [ 22.387052][ T7] blk_mq_dispatch_rq_list+0x2ec/0x18a0 [ 22.387060][ T7] __blk_mq_sched_dispatch_requests+0x2a0/0x3e8 [ 22.387065][ T7] blk_mq_sched_dispatch_requests+0xa4/0x100 [ 22.387070][ T7] __blk_mq_run_hw_queue+0x148/0x1d8 [ 22.387075][ T7] __blk_mq_delay_run_hw_queue+0x3f8/0x730 [ 22.414539][ T269] igb 0006:01:00.0 enP6p1s0: renamed from eth0 [ 22.418957][ T7] blk_mq_run_hw_queue+0x148/0x248 [ 22.418969][ T7] blk_mq_sched_insert_request+0x2a4/0x330 [ 22.418975][ T7] blk_execute_rq_nowait+0xc8/0x118 [ 22.418981][ T7] blk_execute_rq+0xd4/0x188 [ 22.453203][ T255] udevadm (255) used greatest stack depth: 57408 bytes left [ 22.456504][ T7] __nvme_submit_sync_cmd+0x4e0/0x730 [nvme_core] [ 22.673245][ T7] nvme_identify_ctrl.isra.0+0x124/0x1e0 [nvme_core] [ 22.679784][ T7] nvme_init_identify+0x90/0x1868 [nvme_core] [ 22.685713][ T7] nvme_init_ctrl_finish+0x1a8/0xb88 [nvme_core] [ 22.691903][ T7] nvme_reset_work+0xe5c/0x2aa4 [nvme] [ 22.697219][ T7] process_one_work+0x7e4/0x19a0 [ 22.702005][ T7] worker_thread+0x334/0xae0 [ 22.706442][ T7] kthread+0x3bc/0x470 [ 22.710359][ T7] ret_from_fork+0x10/0x18 [ 22.714627][ T7] Code: f941ef81 9101c420 1200080e d343fc06 (38f768c6) [ 22.721407][ T7] ---[ end trace 1f3c4181ae408676 ]--- [ 22.726712][ T7] Kernel panic - not syncing: Oops: Fatal exception [ 22.733169][ T7] SMP: stopping secondary CPUs [ 23.765164][ T7] SMP: failed to stop secondary CPUs 13,15 [ 23.770818][ T7] Kernel Offset: disabled [ 23.774991][ T7] CPU features: 0x00000251,20000846 [ 23.780034][ T7] Memory Limit: none [ 23.783794][ T7] ---[ end Kernel panic - not syncing: Oops: Fatal exception ]--- > --- > drivers/xen/swiotlb-xen.c | 2 +- > include/linux/swiotlb.h | 11 +++++++++++ > kernel/dma/direct.c | 2 +- > kernel/dma/direct.h | 2 +- > kernel/dma/swiotlb.c | 4 ++++ > 5 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 0c6ed09f8513..4730a146fa35 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > if (dma_capable(dev, dev_addr, size, true) && > !range_straddles_page_boundary(phys, size) && > !xen_arch_need_swiotlb(dev, phys, dev_addr) && > - swiotlb_force != SWIOTLB_FORCE) > + !is_swiotlb_force_bounce(dev)) > goto done; > > /* > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > index dd1c30a83058..8d8855c77d9a 100644 > --- a/include/linux/swiotlb.h > +++ b/include/linux/swiotlb.h > @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force; > * unmap calls. > * @debugfs: The dentry to debugfs. > * @late_alloc: %true if allocated using the page allocator > + * @force_bounce: %true if swiotlb bouncing is forced > */ > struct io_tlb_mem { > phys_addr_t start; > @@ -94,6 +95,7 @@ struct io_tlb_mem { > spinlock_t lock; > struct dentry *debugfs; > bool late_alloc; > + bool force_bounce; > struct io_tlb_slot { > phys_addr_t orig_addr; > size_t alloc_size; > @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) > return mem && paddr >= mem->start && paddr < mem->end; > } > > +static inline bool is_swiotlb_force_bounce(struct device *dev) > +{ > + return dev->dma_io_tlb_mem->force_bounce; > +} > + > void __init swiotlb_exit(void); > unsigned int swiotlb_max_segment(void); > size_t swiotlb_max_mapping_size(struct device *dev); > @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) > { > return false; > } > +static inline bool is_swiotlb_force_bounce(struct device *dev) > +{ > + return false; > +} > static inline void swiotlb_exit(void) > { > } > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 7a88c34d0867..a92465b4eb12 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev) > { > /* If SWIOTLB is active, use its maximum mapping size */ > if (is_swiotlb_active(dev) && > - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE)) > + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev))) > return swiotlb_max_mapping_size(dev); > return SIZE_MAX; > } > diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h > index 13e9e7158d94..4632b0f4f72e 100644 > --- a/kernel/dma/direct.h > +++ b/kernel/dma/direct.h > @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device *dev, > phys_addr_t phys = page_to_phys(page) + offset; > dma_addr_t dma_addr = phys_to_dma(dev, phys); > > - if (unlikely(swiotlb_force == SWIOTLB_FORCE)) > + if (is_swiotlb_force_bounce(dev)) > return swiotlb_map(dev, phys, size, dir, attrs); > > if (unlikely(!dma_capable(dev, dma_addr, size, true))) { > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 8a120f42340b..0d294bbf274c 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -179,6 +179,10 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start, > mem->end = mem->start + bytes; > mem->index = 0; > mem->late_alloc = late_alloc; > + > + if (swiotlb_force == SWIOTLB_FORCE) > + mem->force_bounce = true; > + > spin_lock_init(&mem->lock); > for (i = 0; i < mem->nslabs; i++) { > mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i); >
On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote: > > > On 6/18/2021 11:40 PM, Claire Chang wrote: > > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and > > use it to determine whether to bounce the data or not. This will be > > useful later to allow for different pools. > > > > Signed-off-by: Claire Chang <tientzu@chromium.org> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Tested-by: Stefano Stabellini <sstabellini@kernel.org> > > Tested-by: Will Deacon <will@kernel.org> > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > > Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next. Hmm, so that makes patch 7 the suspicious one, right? Looking at that one more closely, it looks like swiotlb_find_slots() takes 'alloc_size + offset' as its 'alloc_size' parameter from swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based on 'alloc_size + offset', which looks like a change in behaviour from the old code, which didn't include the offset there. swiotlb_release_slots() then adds the offset back on afaict, so we end up accounting for it twice and possibly unmap more than we're supposed to? Will
On 6/23/2021 2:37 PM, Will Deacon wrote: > On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote: >> >> >> On 6/18/2021 11:40 PM, Claire Chang wrote: >>> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and >>> use it to determine whether to bounce the data or not. This will be >>> useful later to allow for different pools. >>> >>> Signed-off-by: Claire Chang <tientzu@chromium.org> >>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>> Tested-by: Stefano Stabellini <sstabellini@kernel.org> >>> Tested-by: Will Deacon <will@kernel.org> >>> Acked-by: Stefano Stabellini <sstabellini@kernel.org> >> >> Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next. > > Hmm, so that makes patch 7 the suspicious one, right? Will, no. It is rather patch #6 (this patch). Only the patch from #6 to #12 were reverted to fix the issue. Also, looking at this offset of the crash, pc : dma_direct_map_sg+0x304/0x8f0 is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 is_swiotlb_force_bounce() was the new function introduced in this patch here. +static inline bool is_swiotlb_force_bounce(struct device *dev) +{ + return dev->dma_io_tlb_mem->force_bounce; +} > > Looking at that one more closely, it looks like swiotlb_find_slots() takes > 'alloc_size + offset' as its 'alloc_size' parameter from > swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based > on 'alloc_size + offset', which looks like a change in behaviour from the > old code, which didn't include the offset there. > > swiotlb_release_slots() then adds the offset back on afaict, so we end up > accounting for it twice and possibly unmap more than we're supposed to? > > Will >
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: > is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 > > is_swiotlb_force_bounce() was the new function introduced in this patch here. > > +static inline bool is_swiotlb_force_bounce(struct device *dev) > +{ > + return dev->dma_io_tlb_mem->force_bounce; > +} To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you turn this into : return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; for a quick debug check?
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: > > is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 > > > > is_swiotlb_force_bounce() was the new function introduced in this patch here. > > > > +static inline bool is_swiotlb_force_bounce(struct device *dev) > > +{ > > + return dev->dma_io_tlb_mem->force_bounce; > > +} > > To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you > turn this into : > > return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; > > for a quick debug check? I just realized that dma_io_tlb_mem might be NULL like Christoph pointed out since swiotlb might not get initialized. However, `Unable to handle kernel paging request at virtual address dfff80000000000e` looks more like the address is garbage rather than NULL? I wonder if that's because dev->dma_io_tlb_mem is not assigned properly (which means device_initialize is not called?).
On 2021-06-24 07:05, Claire Chang wrote: > On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote: >> >> On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: >>> is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 >>> >>> is_swiotlb_force_bounce() was the new function introduced in this patch here. >>> >>> +static inline bool is_swiotlb_force_bounce(struct device *dev) >>> +{ >>> + return dev->dma_io_tlb_mem->force_bounce; >>> +} >> >> To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you >> turn this into : >> >> return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; >> >> for a quick debug check? > > I just realized that dma_io_tlb_mem might be NULL like Christoph > pointed out since swiotlb might not get initialized. > However, `Unable to handle kernel paging request at virtual address > dfff80000000000e` looks more like the address is garbage rather than > NULL? > I wonder if that's because dev->dma_io_tlb_mem is not assigned > properly (which means device_initialize is not called?). What also looks odd is that the base "address" 0xdfff800000000000 is held in a couple of registers, but the offset 0xe looks too small to match up to any relevant structure member in that dereference chain :/ Robin.
On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote: > On 2021-06-24 07:05, Claire Chang wrote: > > On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote: > > > > > > On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: > > > > is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 > > > > > > > > is_swiotlb_force_bounce() was the new function introduced in this patch here. > > > > > > > > +static inline bool is_swiotlb_force_bounce(struct device *dev) > > > > +{ > > > > + return dev->dma_io_tlb_mem->force_bounce; > > > > +} > > > > > > To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you > > > turn this into : > > > > > > return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; > > > > > > for a quick debug check? > > > > I just realized that dma_io_tlb_mem might be NULL like Christoph > > pointed out since swiotlb might not get initialized. > > However, `Unable to handle kernel paging request at virtual address > > dfff80000000000e` looks more like the address is garbage rather than > > NULL? > > I wonder if that's because dev->dma_io_tlb_mem is not assigned > > properly (which means device_initialize is not called?). > > What also looks odd is that the base "address" 0xdfff800000000000 is held in > a couple of registers, but the offset 0xe looks too small to match up to any > relevant structure member in that dereference chain :/ FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't been initialised but we dereference 'dev->dma_io_tlb_mem', so I think Christoph's suggestion is needed regardless. But I agree that it won't help with the issue reported by Qian Cai. Qian Cai: please can you share your .config and your command line? Thanks, Will
On 2021-06-24 12:18, Will Deacon wrote: > On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote: >> On 2021-06-24 07:05, Claire Chang wrote: >>> On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote: >>>> >>>> On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: >>>>> is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 >>>>> >>>>> is_swiotlb_force_bounce() was the new function introduced in this patch here. >>>>> >>>>> +static inline bool is_swiotlb_force_bounce(struct device *dev) >>>>> +{ >>>>> + return dev->dma_io_tlb_mem->force_bounce; >>>>> +} >>>> >>>> To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you >>>> turn this into : >>>> >>>> return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; >>>> >>>> for a quick debug check? >>> >>> I just realized that dma_io_tlb_mem might be NULL like Christoph >>> pointed out since swiotlb might not get initialized. >>> However, `Unable to handle kernel paging request at virtual address >>> dfff80000000000e` looks more like the address is garbage rather than >>> NULL? >>> I wonder if that's because dev->dma_io_tlb_mem is not assigned >>> properly (which means device_initialize is not called?). >> >> What also looks odd is that the base "address" 0xdfff800000000000 is held in >> a couple of registers, but the offset 0xe looks too small to match up to any >> relevant structure member in that dereference chain :/ > > FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't > been initialised but we dereference 'dev->dma_io_tlb_mem', so I think > Christoph's suggestion is needed regardless. Ack to that - for SWIOTLB_NO_FORCE, io_tlb_default_mem will remain NULL. The massive jump in KernelCI baseline failures as of yesterday looks like every arm64 machine with less than 4GB of RAM blowing up... Robin. > But I agree that it won't help > with the issue reported by Qian Cai. > > Qian Cai: please can you share your .config and your command line? > > Thanks, > > Will >
On Thu, Jun 24, 2021 at 12:34:09PM +0100, Robin Murphy wrote: > On 2021-06-24 12:18, Will Deacon wrote: > > On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote: > > > On 2021-06-24 07:05, Claire Chang wrote: > > > > On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote: > > > > > > > > > > On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote: > > > > > > is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119 > > > > > > > > > > > > is_swiotlb_force_bounce() was the new function introduced in this patch here. > > > > > > > > > > > > +static inline bool is_swiotlb_force_bounce(struct device *dev) > > > > > > +{ > > > > > > + return dev->dma_io_tlb_mem->force_bounce; > > > > > > +} > > > > > > > > > > To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you > > > > > turn this into : > > > > > > > > > > return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce; > > > > > > > > > > for a quick debug check? > > > > > > > > I just realized that dma_io_tlb_mem might be NULL like Christoph > > > > pointed out since swiotlb might not get initialized. > > > > However, `Unable to handle kernel paging request at virtual address > > > > dfff80000000000e` looks more like the address is garbage rather than > > > > NULL? > > > > I wonder if that's because dev->dma_io_tlb_mem is not assigned > > > > properly (which means device_initialize is not called?). > > > > > > What also looks odd is that the base "address" 0xdfff800000000000 is held in > > > a couple of registers, but the offset 0xe looks too small to match up to any > > > relevant structure member in that dereference chain :/ > > > > FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't > > been initialised but we dereference 'dev->dma_io_tlb_mem', so I think > > Christoph's suggestion is needed regardless. > > Ack to that - for SWIOTLB_NO_FORCE, io_tlb_default_mem will remain NULL. The > massive jump in KernelCI baseline failures as of yesterday looks like every > arm64 machine with less than 4GB of RAM blowing up... Ok, diff below which attempts to tackle the offset issue I mentioned as well. Qian Cai -- please can you try with these changes? Will --->8 diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 175b6c113ed8..39284ff2a6cd 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) static inline bool is_swiotlb_force_bounce(struct device *dev) { - return dev->dma_io_tlb_mem->force_bounce; + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + + return mem && mem->force_bounce; } void __init swiotlb_exit(void); diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 44be8258e27b..0ffbaae9fba2 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1); unsigned int nslots = nr_slots(alloc_size), stride; unsigned int index, wrap, count = 0, i; + unsigned int offset = swiotlb_align_offset(dev, orig_addr); unsigned long flags; BUG_ON(!nslots); @@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, for (i = index; i < index + nslots; i++) { mem->slots[i].list = 0; mem->slots[i].alloc_size = - alloc_size - ((i - index) << IO_TLB_SHIFT); + alloc_size - (offset + ((i - index) << IO_TLB_SHIFT)); } for (i = index - 1; io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 &&
On 6/24/2021 7:48 AM, Will Deacon wrote: > Ok, diff below which attempts to tackle the offset issue I mentioned as > well. Qian Cai -- please can you try with these changes? This works fine. > > Will > > --->8 > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > index 175b6c113ed8..39284ff2a6cd 100644 > --- a/include/linux/swiotlb.h > +++ b/include/linux/swiotlb.h > @@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) > > static inline bool is_swiotlb_force_bounce(struct device *dev) > { > - return dev->dma_io_tlb_mem->force_bounce; > + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > + > + return mem && mem->force_bounce; > } > > void __init swiotlb_exit(void); > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 44be8258e27b..0ffbaae9fba2 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1); > unsigned int nslots = nr_slots(alloc_size), stride; > unsigned int index, wrap, count = 0, i; > + unsigned int offset = swiotlb_align_offset(dev, orig_addr); > unsigned long flags; > > BUG_ON(!nslots); > @@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > for (i = index; i < index + nslots; i++) { > mem->slots[i].list = 0; > mem->slots[i].alloc_size = > - alloc_size - ((i - index) << IO_TLB_SHIFT); > + alloc_size - (offset + ((i - index) << IO_TLB_SHIFT)); > } > for (i = index - 1; > io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 && >
On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote: > > > On 6/24/2021 7:48 AM, Will Deacon wrote: > > Ok, diff below which attempts to tackle the offset issue I mentioned as > > well. Qian Cai -- please can you try with these changes? > > This works fine. Cool. Let me squash this patch in #6 and rebase the rest of them. Claire, could you check the devel/for-linus-5.14 say by end of today to double check that I didn't mess anything up please? Will, Thank you for generating the fix! I am going to run it on x86 and Xen to make sure all is good (granted last time I ran devel/for-linus-5.14 on that setup I didn't see any errors so I need to double check I didn't do something silly like run a wrong kernel). > > > > > Will > > > > --->8 > > > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > > index 175b6c113ed8..39284ff2a6cd 100644 > > --- a/include/linux/swiotlb.h > > +++ b/include/linux/swiotlb.h > > @@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) > > > > static inline bool is_swiotlb_force_bounce(struct device *dev) > > { > > - return dev->dma_io_tlb_mem->force_bounce; > > + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > > + > > + return mem && mem->force_bounce; > > } > > > > void __init swiotlb_exit(void); > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > > index 44be8258e27b..0ffbaae9fba2 100644 > > --- a/kernel/dma/swiotlb.c > > +++ b/kernel/dma/swiotlb.c > > @@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > > dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1); > > unsigned int nslots = nr_slots(alloc_size), stride; > > unsigned int index, wrap, count = 0, i; > > + unsigned int offset = swiotlb_align_offset(dev, orig_addr); > > unsigned long flags; > > > > BUG_ON(!nslots); > > @@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > > for (i = index; i < index + nslots; i++) { > > mem->slots[i].list = 0; > > mem->slots[i].alloc_size = > > - alloc_size - ((i - index) << IO_TLB_SHIFT); > > + alloc_size - (offset + ((i - index) << IO_TLB_SHIFT)); > > } > > for (i = index - 1; > > io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 && > >
On Thu, Jun 24, 2021 at 11:56 PM Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote: > > > > > > On 6/24/2021 7:48 AM, Will Deacon wrote: > > > Ok, diff below which attempts to tackle the offset issue I mentioned as > > > well. Qian Cai -- please can you try with these changes? > > > > This works fine. > > Cool. Let me squash this patch in #6 and rebase the rest of them. > > Claire, could you check the devel/for-linus-5.14 say by end of today to > double check that I didn't mess anything up please? I just submitted v15 here (https://lore.kernel.org/patchwork/cover/1451322/) in case it's helpful. I'll double check of course. Thanks for the efforts! > > Will, > > Thank you for generating the fix! I am going to run it on x86 and Xen > to make sure all is good (granted last time I ran devel/for-linus-5.14 > on that setup I didn't see any errors so I need to double check > I didn't do something silly like run a wrong kernel). > > > > > > > > > > Will > > > > > > --->8 > > > > > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > > > index 175b6c113ed8..39284ff2a6cd 100644 > > > --- a/include/linux/swiotlb.h > > > +++ b/include/linux/swiotlb.h > > > @@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) > > > > > > static inline bool is_swiotlb_force_bounce(struct device *dev) > > > { > > > - return dev->dma_io_tlb_mem->force_bounce; > > > + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; > > > + > > > + return mem && mem->force_bounce; > > > } > > > > > > void __init swiotlb_exit(void); > > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > > > index 44be8258e27b..0ffbaae9fba2 100644 > > > --- a/kernel/dma/swiotlb.c > > > +++ b/kernel/dma/swiotlb.c > > > @@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > > > dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1); > > > unsigned int nslots = nr_slots(alloc_size), stride; > > > unsigned int index, wrap, count = 0, i; > > > + unsigned int offset = swiotlb_align_offset(dev, orig_addr); > > > unsigned long flags; > > > > > > BUG_ON(!nslots); > > > @@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, > > > for (i = index; i < index + nslots; i++) { > > > mem->slots[i].list = 0; > > > mem->slots[i].alloc_size = > > > - alloc_size - ((i - index) << IO_TLB_SHIFT); > > > + alloc_size - (offset + ((i - index) << IO_TLB_SHIFT)); > > > } > > > for (i = index - 1; > > > io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 && > > >
On Thu, Jun 24, 2021 at 11:58:57PM +0800, Claire Chang wrote: > On Thu, Jun 24, 2021 at 11:56 PM Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: > > > > On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote: > > > > > > > > > On 6/24/2021 7:48 AM, Will Deacon wrote: > > > > Ok, diff below which attempts to tackle the offset issue I mentioned as > > > > well. Qian Cai -- please can you try with these changes? > > > > > > This works fine. > > > > Cool. Let me squash this patch in #6 and rebase the rest of them. > > > > Claire, could you check the devel/for-linus-5.14 say by end of today to > > double check that I didn't mess anything up please? > > I just submitted v15 here > (https://lore.kernel.org/patchwork/cover/1451322/) in case it's > helpful. Oh! Nice! > I'll double check of course. Thanks for the efforts! I ended up using your patch #6 and #7. Please double-check.
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 0c6ed09f8513..4730a146fa35 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, if (dma_capable(dev, dev_addr, size, true) && !range_straddles_page_boundary(phys, size) && !xen_arch_need_swiotlb(dev, phys, dev_addr) && - swiotlb_force != SWIOTLB_FORCE) + !is_swiotlb_force_bounce(dev)) goto done; /* diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index dd1c30a83058..8d8855c77d9a 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force; * unmap calls. * @debugfs: The dentry to debugfs. * @late_alloc: %true if allocated using the page allocator + * @force_bounce: %true if swiotlb bouncing is forced */ struct io_tlb_mem { phys_addr_t start; @@ -94,6 +95,7 @@ struct io_tlb_mem { spinlock_t lock; struct dentry *debugfs; bool late_alloc; + bool force_bounce; struct io_tlb_slot { phys_addr_t orig_addr; size_t alloc_size; @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) return mem && paddr >= mem->start && paddr < mem->end; } +static inline bool is_swiotlb_force_bounce(struct device *dev) +{ + return dev->dma_io_tlb_mem->force_bounce; +} + void __init swiotlb_exit(void); unsigned int swiotlb_max_segment(void); size_t swiotlb_max_mapping_size(struct device *dev); @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { return false; } +static inline bool is_swiotlb_force_bounce(struct device *dev) +{ + return false; +} static inline void swiotlb_exit(void) { } diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 7a88c34d0867..a92465b4eb12 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev) { /* If SWIOTLB is active, use its maximum mapping size */ if (is_swiotlb_active(dev) && - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE)) + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev))) return swiotlb_max_mapping_size(dev); return SIZE_MAX; } diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h index 13e9e7158d94..4632b0f4f72e 100644 --- a/kernel/dma/direct.h +++ b/kernel/dma/direct.h @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device *dev, phys_addr_t phys = page_to_phys(page) + offset; dma_addr_t dma_addr = phys_to_dma(dev, phys); - if (unlikely(swiotlb_force == SWIOTLB_FORCE)) + if (is_swiotlb_force_bounce(dev)) return swiotlb_map(dev, phys, size, dir, attrs); if (unlikely(!dma_capable(dev, dma_addr, size, true))) { diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8a120f42340b..0d294bbf274c 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -179,6 +179,10 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start, mem->end = mem->start + bytes; mem->index = 0; mem->late_alloc = late_alloc; + + if (swiotlb_force == SWIOTLB_FORCE) + mem->force_bounce = true; + spin_lock_init(&mem->lock); for (i = 0; i < mem->nslabs; i++) { mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);