| Message ID | 20210624155526.2775863-7-tientzu@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Restricted DMA | expand |
On Wed, Jun 30, 2021 at 9:43 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Thu, Jun 24, 2021 at 11:55:20PM +0800, 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> > > This patch as commit af452ec1b1a3 ("swiotlb: Use is_swiotlb_force_bounce > for swiotlb data bouncing") causes my Ryzen 3 4300G system to fail to > get to an X session consistently (although not every single time), > presumably due to a crash in the AMDGPU driver that I see in dmesg. > > I have attached logs at af452ec1b1a3 and f127c9556a8e and I am happy > to provide any further information, debug, or test patches as necessary. Are you using swiotlb=force? or the swiotlb_map is called because of !dma_capable? (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/direct.h#n93) `BUG: unable to handle page fault for address: 00000000003a8290` and the fact it crashed at `_raw_spin_lock_irqsave` look like the memory (maybe dev->dma_io_tlb_mem) was corrupted? The dev->dma_io_tlb_mem should be set here (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) through device_initialize. I can't tell what happened from the logs, but maybe we could try KASAN to see if it provides more clue. Thanks, Claire > > Cheers, > Nathan
On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > On Wed, Jun 30, 2021 at 9:43 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Thu, Jun 24, 2021 at 11:55:20PM +0800, 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> > > > > This patch as commit af452ec1b1a3 ("swiotlb: Use is_swiotlb_force_bounce > > for swiotlb data bouncing") causes my Ryzen 3 4300G system to fail to > > get to an X session consistently (although not every single time), > > presumably due to a crash in the AMDGPU driver that I see in dmesg. > > > > I have attached logs at af452ec1b1a3 and f127c9556a8e and I am happy > > to provide any further information, debug, or test patches as necessary. > > Are you using swiotlb=force? or the swiotlb_map is called because of > !dma_capable? (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/direct.h#n93) The command line is in the dmesg: | Kernel command line: initrd=\amd-ucode.img initrd=\initramfs-linux-next-llvm.img root=PARTUUID=8680aa0c-cf09-4a69-8cf3-970478040ee7 rw intel_pstate=no_hwp irqpoll but I worry that this looks _very_ similar to the issue reported by Qian Cai which we thought we had fixed. Nathan -- is the failure deterministic? > `BUG: unable to handle page fault for address: 00000000003a8290` and > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > (maybe dev->dma_io_tlb_mem) was corrupted? > The dev->dma_io_tlb_mem should be set here > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > through device_initialize. I'm less sure about this. 'dma_io_tlb_mem' should be pointing at 'io_tlb_default_mem', which is a page-aligned allocation from memblock. The spinlock is at offset 0x24 in that structure, and looking at the register dump from the crash: Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 Jun 29 18:28:42 hp-4300G kernel: Call Trace: Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and RDX pointing at the spinlock. Yet RAX is holding junk :/ I agree that enabling KASAN would be a good idea, but I also think we probably need to get some more information out of swiotlb_tbl_map_single() to see see what exactly is going wrong in there. Will
Hi Will and Claire, On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > On Wed, Jun 30, 2021 at 9:43 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > On Thu, Jun 24, 2021 at 11:55:20PM +0800, 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> > > > > > > This patch as commit af452ec1b1a3 ("swiotlb: Use is_swiotlb_force_bounce > > > for swiotlb data bouncing") causes my Ryzen 3 4300G system to fail to > > > get to an X session consistently (although not every single time), > > > presumably due to a crash in the AMDGPU driver that I see in dmesg. > > > > > > I have attached logs at af452ec1b1a3 and f127c9556a8e and I am happy > > > to provide any further information, debug, or test patches as necessary. > > > > Are you using swiotlb=force? or the swiotlb_map is called because of > > !dma_capable? (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/direct.h#n93) > > The command line is in the dmesg: > > | Kernel command line: initrd=\amd-ucode.img initrd=\initramfs-linux-next-llvm.img root=PARTUUID=8680aa0c-cf09-4a69-8cf3-970478040ee7 rw intel_pstate=no_hwp irqpoll > > but I worry that this looks _very_ similar to the issue reported by Qian > Cai which we thought we had fixed. Nathan -- is the failure deterministic? Yes, for the most part. It does not happen every single boot so when I was bisecting, I did a series of seven boots and only considered the revision good when all seven of them made it to LightDM's greeter. My results that I notated show most bad revisions failed anywhere from four to six times. > > `BUG: unable to handle page fault for address: 00000000003a8290` and > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > (maybe dev->dma_io_tlb_mem) was corrupted? > > The dev->dma_io_tlb_mem should be set here > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > through device_initialize. > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > The spinlock is at offset 0x24 in that structure, and looking at the > register dump from the crash: > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 > Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 > Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 > Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 > Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 > Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 > Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > I agree that enabling KASAN would be a good idea, but I also think we > probably need to get some more information out of swiotlb_tbl_map_single() > to see see what exactly is going wrong in there. I can certainly enable KASAN and if there is any debug print I can add or dump anything, let me know! Cheers, Nathan
On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > `BUG: unable to handle page fault for address: 00000000003a8290` and > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > The dev->dma_io_tlb_mem should be set here > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > through device_initialize. > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > > The spinlock is at offset 0x24 in that structure, and looking at the > > register dump from the crash: > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 > > Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 > > Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 > > Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 > > Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 > > Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 > > Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > I agree that enabling KASAN would be a good idea, but I also think we > > probably need to get some more information out of swiotlb_tbl_map_single() > > to see see what exactly is going wrong in there. > > I can certainly enable KASAN and if there is any debug print I can add > or dump anything, let me know! I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built x86 defconfig and ran it on my laptop. However, it seems to work fine! Please can you share your .config? Will
Hi Nathan, On Thu, Jul 01, 2021 at 12:52:20AM -0700, Nathan Chancellor wrote: > On 7/1/2021 12:40 AM, Will Deacon wrote: > > On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > > > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > > > `BUG: unable to handle page fault for address: 00000000003a8290` and > > > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > > > The dev->dma_io_tlb_mem should be set here > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > > > through device_initialize. > > > > > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > > > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > > > > The spinlock is at offset 0x24 in that structure, and looking at the > > > > register dump from the crash: > > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 > > > > Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 > > > > Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 > > > > Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 > > > > Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 > > > > Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 > > > > Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 > > > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > > Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 > > > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > > > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > > > > > I agree that enabling KASAN would be a good idea, but I also think we > > > > probably need to get some more information out of swiotlb_tbl_map_single() > > > > to see see what exactly is going wrong in there. > > > > > > I can certainly enable KASAN and if there is any debug print I can add > > > or dump anything, let me know! > > > > I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built > > x86 defconfig and ran it on my laptop. However, it seems to work fine! > > > > Please can you share your .config? > > Sure thing, it is attached. It is just Arch Linux's config run through > olddefconfig. The original is below in case you need to diff it. > > https://raw.githubusercontent.com/archlinux/svntogit-packages/9045405dc835527164f3034b3ceb9a67c7a53cd4/trunk/config > > If there is anything more that I can provide, please let me know. I eventually got this booting (for some reason it was causing LD to SEGV trying to link it for a while...) and sadly it works fine on my laptop. Hmm. Did you manage to try again with KASAN? It might also be worth taking the IOMMU out of the equation, since that interfaces differently with SWIOTLB and I couldn't figure out the code path from the log you provided. What happens if you boot with "amd_iommu=off swiotlb=force"? (although word of warning here: i915 dies horribly on my laptop if I pass swiotlb=force, even with the distro 5.10 kernel) Will
On 2021-07-02 14:58, Will Deacon wrote: > Hi Nathan, > > On Thu, Jul 01, 2021 at 12:52:20AM -0700, Nathan Chancellor wrote: >> On 7/1/2021 12:40 AM, Will Deacon wrote: >>> On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: >>>> On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: >>>>> On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: >>>>>> `BUG: unable to handle page fault for address: 00000000003a8290` and >>>>>> the fact it crashed at `_raw_spin_lock_irqsave` look like the memory >>>>>> (maybe dev->dma_io_tlb_mem) was corrupted? >>>>>> The dev->dma_io_tlb_mem should be set here >>>>>> (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) >>>>>> through device_initialize. >>>>> >>>>> I'm less sure about this. 'dma_io_tlb_mem' should be pointing at >>>>> 'io_tlb_default_mem', which is a page-aligned allocation from memblock. >>>>> The spinlock is at offset 0x24 in that structure, and looking at the >>>>> register dump from the crash: >>>>> >>>>> Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 >>>>> Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 >>>>> Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 >>>>> Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 >>>>> Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 >>>>> Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 >>>>> Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 >>>>> Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>>>> Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 >>>>> Jun 29 18:28:42 hp-4300G kernel: Call Trace: >>>>> Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 >>>>> Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 >>>>> >>>>> Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and >>>>> RDX pointing at the spinlock. Yet RAX is holding junk :/ >>>>> >>>>> I agree that enabling KASAN would be a good idea, but I also think we >>>>> probably need to get some more information out of swiotlb_tbl_map_single() >>>>> to see see what exactly is going wrong in there. >>>> >>>> I can certainly enable KASAN and if there is any debug print I can add >>>> or dump anything, let me know! >>> >>> I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built >>> x86 defconfig and ran it on my laptop. However, it seems to work fine! >>> >>> Please can you share your .config? >> >> Sure thing, it is attached. It is just Arch Linux's config run through >> olddefconfig. The original is below in case you need to diff it. >> >> https://raw.githubusercontent.com/archlinux/svntogit-packages/9045405dc835527164f3034b3ceb9a67c7a53cd4/trunk/config >> >> If there is anything more that I can provide, please let me know. > > I eventually got this booting (for some reason it was causing LD to SEGV > trying to link it for a while...) and sadly it works fine on my laptop. Hmm. > > Did you manage to try again with KASAN? > > It might also be worth taking the IOMMU out of the equation, since that > interfaces differently with SWIOTLB and I couldn't figure out the code path > from the log you provided. What happens if you boot with "amd_iommu=off > swiotlb=force"? Oh, now there's a thing... the chat from the IOMMU API in the boot log implies that the IOMMU *should* be in the picture - we see that default domains are IOMMU_DOMAIN_DMA default and the GPU 0000:0c:00.0 was added to a group. That means dev->dma_ops should be set and DMA API calls should be going through iommu-dma, yet the callstack in the crash says we've gone straight from dma_map_page_attrs() to swiotlb_map(), implying the inline dma_direct_map_page() path. If dev->dma_ops didn't look right in the first place, it's perhaps less surprising that dev->dma_io_tlb_mem might be wild as well. It doesn't seem plausible that we should have a race between initialising the device and probing its driver, so maybe the whole dev pointer is getting trampled earlier in the callchain (or is fundamentally wrong to begin with, but from a quick skim of the amdgpu code it did look like adev->dev and adev->pdev are appropriately set early on by amdgpu_pci_probe()). > (although word of warning here: i915 dies horribly on my laptop if I pass > swiotlb=force, even with the distro 5.10 kernel) FWIW I'd imagine you probably need to massively increase the SWIOTLB buffer size to have hope of that working. Robin.
On Sat, Jul 3, 2021 at 1:55 PM Nathan Chancellor <nathan@kernel.org> wrote: > > Hi Will and Robin, > > On Fri, Jul 02, 2021 at 04:13:50PM +0100, Robin Murphy wrote: > > On 2021-07-02 14:58, Will Deacon wrote: > > > Hi Nathan, > > > > > > On Thu, Jul 01, 2021 at 12:52:20AM -0700, Nathan Chancellor wrote: > > > > On 7/1/2021 12:40 AM, Will Deacon wrote: > > > > > On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > > > > > > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > > > > > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > > > > > > `BUG: unable to handle page fault for address: 00000000003a8290` and > > > > > > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > > > > > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > > > > > > The dev->dma_io_tlb_mem should be set here > > > > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > > > > > > through device_initialize. > > > > > > > > > > > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > > > > > > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > > > > > > > The spinlock is at offset 0x24 in that structure, and looking at the > > > > > > > register dump from the crash: > > > > > > > > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:ffffadb4013db9e8 EFLAGS: 00010006 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RAX: 00000000003a8290 RBX: 0000000000000000 RCX: ffff8900572ad580 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RDX: ffff89005653f024 RSI: 00000000000c0000 RDI: 0000000000001d17 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RBP: 000000000a20d000 R08: 00000000000c0000 R09: 0000000000000000 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: R10: 000000000a20d000 R11: ffff89005653f000 R12: 0000000000000212 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000200000 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: FS: 00007f1f8898ea40(0000) GS:ffff890057280000(0000) knlGS:0000000000000000 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: CR2: 00000000003a8290 CR3: 00000001020d0000 CR4: 0000000000350ee0 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > > > > > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > > > > > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > > > > > > > > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > > > > > > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > > > > > > > > > > > I agree that enabling KASAN would be a good idea, but I also think we > > > > > > > probably need to get some more information out of swiotlb_tbl_map_single() > > > > > > > to see see what exactly is going wrong in there. > > > > > > > > > > > > I can certainly enable KASAN and if there is any debug print I can add > > > > > > or dump anything, let me know! > > > > > > > > > > I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built > > > > > x86 defconfig and ran it on my laptop. However, it seems to work fine! > > > > > > > > > > Please can you share your .config? > > > > > > > > Sure thing, it is attached. It is just Arch Linux's config run through > > > > olddefconfig. The original is below in case you need to diff it. > > > > > > > > https://raw.githubusercontent.com/archlinux/svntogit-packages/9045405dc835527164f3034b3ceb9a67c7a53cd4/trunk/config > > > > > > > > If there is anything more that I can provide, please let me know. > > > > > > I eventually got this booting (for some reason it was causing LD to SEGV > > > trying to link it for a while...) and sadly it works fine on my laptop. Hmm. > > Seems like it might be something specific to the amdgpu module? > > > > Did you manage to try again with KASAN? > > Yes, it took a few times to reproduce the issue but I did manage to get > a dmesg, please find it attached. I build from commit 7d31f1c65cc9 ("swiotlb: > fix implicit debugfs declarations") in Konrad's tree. Looking at the logs, the use-after-free bug looked somehow relevant (and it's nvme again. Qian's crash is about nvme too): [ 2.468288] BUG: KASAN: use-after-free in __iommu_dma_unmap_swiotlb+0x64/0xb0 [ 2.468288] Read of size 8 at addr ffff8881d7830000 by task swapper/0/0 [ 2.468288] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.12.0-rc3-debug #1 [ 2.468288] Hardware name: HP HP Desktop M01-F1xxx/87D6, BIOS F.12 12/17/2020 [ 2.468288] Call Trace: [ 2.468288] <IRQ> [ 2.479433] dump_stack+0x9c/0xcf [ 2.479433] print_address_description.constprop.0+0x18/0x130 [ 2.479433] ? __iommu_dma_unmap_swiotlb+0x64/0xb0 [ 2.479433] kasan_report.cold+0x7f/0x111 [ 2.479433] ? __iommu_dma_unmap_swiotlb+0x64/0xb0 [ 2.479433] __iommu_dma_unmap_swiotlb+0x64/0xb0 [ 2.479433] nvme_pci_complete_rq+0x73/0x130 [ 2.479433] blk_complete_reqs+0x6f/0x80 [ 2.479433] __do_softirq+0xfc/0x3be [ 2.479433] irq_exit_rcu+0xce/0x120 [ 2.479433] common_interrupt+0x80/0xa0 [ 2.479433] </IRQ> [ 2.479433] asm_common_interrupt+0x1e/0x40 [ 2.479433] RIP: 0010:cpuidle_enter_state+0xf9/0x590 I wonder if this ended up unmapping something wrong and messing up the dev->dma_io_tlb_mem (i.e. io_tlb_default_mem)? Could you try this patch on top of 7d31f1c65cc9? This patch helps check if we try to unmap the wrong address. ``` diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index b7f76bca89bf..5ac08d50a394 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -613,6 +613,21 @@ void swiotlb_tbl_unmap_single(struct device *dev, phys_addr_t tlb_addr, size_t mapping_size, enum dma_data_direction dir, unsigned long attrs) { + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + unsigned int offset = swiotlb_align_offset(dev, tlb_addr); + int index; + + if (!is_swiotlb_buffer(dev, tlb_addr - offset)) { + dev_err(dev, "%s: attempt to unmap invalid address (0x%llx, offset=%u)\n", __func__, tlb_addr, offset); + return; + } + + index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT; + if (mem->slots[index].orig_addr == INVALID_PHYS_ADDR) { + dev_err(dev, "%s: memory is not mapped before (0x%llx, offset=%u)\n", __func__, tlb_addr, offset); + return; + } + /* * First, sync the memory before unmapping the entry */ ``` It might be useful to have CONFIG_SLUB_DEBUG=y, CONFIG_SLUB_DEBUG_ON=y and line numbers (scripts/decode_stacktrace.sh) too. Thank you so much for helping! > > > > It might also be worth taking the IOMMU out of the equation, since that > > > interfaces differently with SWIOTLB and I couldn't figure out the code path > > > from the log you provided. What happens if you boot with "amd_iommu=off > > > swiotlb=force"? > > > > Oh, now there's a thing... the chat from the IOMMU API in the boot log > > implies that the IOMMU *should* be in the picture - we see that default > > domains are IOMMU_DOMAIN_DMA default and the GPU 0000:0c:00.0 was added to a > > group. That means dev->dma_ops should be set and DMA API calls should be > > going through iommu-dma, yet the callstack in the crash says we've gone > > straight from dma_map_page_attrs() to swiotlb_map(), implying the inline > > dma_direct_map_page() path. > > > > If dev->dma_ops didn't look right in the first place, it's perhaps less > > surprising that dev->dma_io_tlb_mem might be wild as well. It doesn't seem > > plausible that we should have a race between initialising the device and > > probing its driver, so maybe the whole dev pointer is getting trampled > > earlier in the callchain (or is fundamentally wrong to begin with, but from > > a quick skim of the amdgpu code it did look like adev->dev and adev->pdev > > are appropriately set early on by amdgpu_pci_probe()). > > > > > (although word of warning here: i915 dies horribly on my laptop if I pass > > > swiotlb=force, even with the distro 5.10 kernel) > > > > FWIW I'd imagine you probably need to massively increase the SWIOTLB buffer > > size to have hope of that working. > > Is it worth trying this still then? > > Cheers, > Nathan
Hi Nathan, I may have just spotted something in these logs... On Fri, Jul 02, 2021 at 10:55:17PM -0700, Nathan Chancellor wrote: > [ 2.340956] pci 0000:0c:00.1: Adding to iommu group 4 > [ 2.340996] pci 0000:0c:00.2: Adding to iommu group 4 > [ 2.341038] pci 0000:0c:00.3: Adding to iommu group 4 > [ 2.341078] pci 0000:0c:00.4: Adding to iommu group 4 > [ 2.341122] pci 0000:0c:00.6: Adding to iommu group 4 > [ 2.341163] pci 0000:0d:00.0: Adding to iommu group 4 > [ 2.341203] pci 0000:0d:00.1: Adding to iommu group 4 > [ 2.361821] pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40 > [ 2.361839] pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade): > [ 2.361846] PPR X2APIC NX GT IA GA PC GA_vAPIC > [ 2.361861] AMD-Vi: Interrupt remapping enabled > [ 2.361865] AMD-Vi: Virtual APIC enabled > [ 2.361870] AMD-Vi: X2APIC enabled > [ 2.362272] AMD-Vi: Lazy IO/TLB flushing enabled So at this point, the AMD IOMMU driver does: swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; where 'swiotlb' is a global variable indicating whether or not swiotlb is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which will call swiotlb_exit() if 'swiotlb' is false. Now, that used to work fine, because swiotlb_exit() clears 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I think that all the devices which have successfully probed beforehand will have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' field. Will
On Mon, Jul 05, 2021 at 08:03:52PM +0100, Will Deacon wrote: > So at this point, the AMD IOMMU driver does: > > swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; > > where 'swiotlb' is a global variable indicating whether or not swiotlb > is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which > will call swiotlb_exit() if 'swiotlb' is false. > > Now, that used to work fine, because swiotlb_exit() clears > 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I > think that all the devices which have successfully probed beforehand will > have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' > field. Yeah. I don't think we can do that anymore, and I also think it is a bad idea to start with.
On Tue, Jul 06, 2021 at 06:48:48AM +0200, Christoph Hellwig wrote: > On Mon, Jul 05, 2021 at 08:03:52PM +0100, Will Deacon wrote: > > So at this point, the AMD IOMMU driver does: > > > > swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; > > > > where 'swiotlb' is a global variable indicating whether or not swiotlb > > is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which > > will call swiotlb_exit() if 'swiotlb' is false. > > > > Now, that used to work fine, because swiotlb_exit() clears > > 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I > > think that all the devices which have successfully probed beforehand will > > have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' > > field. > > Yeah. I don't think we can do that anymore, and I also think it is > a bad idea to start with. I've had a crack at reworking things along the following lines: - io_tlb_default_mem now lives in the BSS, the flexible array member is now a pointer and that part is allocated dynamically (downside of this is an extra indirection to get at the slots). - io_tlb_default_mem.nslabs tells you whether the thing is valid - swiotlb_exit() frees the slots array and clears the rest of the structure to 0. I also extended it to free the actual slabs, but I'm not sure why it wasn't doing that before. So a non-NULL dev->dma_io_tlb_mem should always be valid to follow. Untested diff below... Nathan, it would be ace if you're brave enough to give this a shot. Will --->8 diff --git a/drivers/base/core.c b/drivers/base/core.c index bbad7c559901..9e1218f89e4b 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2820,7 +2820,7 @@ void device_initialize(struct device *dev) dev->dma_coherent = dma_default_coherent; #endif #ifdef CONFIG_SWIOTLB - dev->dma_io_tlb_mem = io_tlb_default_mem; + dev->dma_io_tlb_mem = &io_tlb_default_mem; #endif } EXPORT_SYMBOL_GPL(device_initialize); diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 785ec7e8be01..f06d9b4f1e0f 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -164,7 +164,7 @@ int __ref xen_swiotlb_init(void) int rc = -ENOMEM; char *start; - if (io_tlb_default_mem != NULL) { + if (io_tlb_default_mem.nslabs) { pr_warn("swiotlb buffer already initialized\n"); return -EEXIST; } @@ -547,7 +547,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct scatterlist *sgl, static int xen_swiotlb_dma_supported(struct device *hwdev, u64 mask) { - return xen_phys_to_dma(hwdev, io_tlb_default_mem->end - 1) <= mask; + return xen_phys_to_dma(hwdev, io_tlb_default_mem.end - 1) <= mask; } const struct dma_map_ops xen_swiotlb_dma_ops = { diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 39284ff2a6cd..b0cb2a9973f4 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -103,9 +103,9 @@ struct io_tlb_mem { phys_addr_t orig_addr; size_t alloc_size; unsigned int list; - } slots[]; + } *slots; }; -extern struct io_tlb_mem *io_tlb_default_mem; +extern struct io_tlb_mem io_tlb_default_mem; static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 0ffbaae9fba2..91cd1d413027 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -70,7 +70,7 @@ enum swiotlb_force swiotlb_force; -struct io_tlb_mem *io_tlb_default_mem; +struct io_tlb_mem io_tlb_default_mem; /* * Max segment that we can provide which (if pages are contingous) will @@ -101,7 +101,7 @@ early_param("swiotlb", setup_io_tlb_npages); unsigned int swiotlb_max_segment(void) { - return io_tlb_default_mem ? max_segment : 0; + return io_tlb_default_mem.nslabs ? max_segment : 0; } EXPORT_SYMBOL_GPL(swiotlb_max_segment); @@ -134,9 +134,9 @@ void __init swiotlb_adjust_size(unsigned long size) void swiotlb_print_info(void) { - struct io_tlb_mem *mem = io_tlb_default_mem; + struct io_tlb_mem *mem = &io_tlb_default_mem; - if (!mem) { + if (!mem->nslabs) { pr_warn("No low mem\n"); return; } @@ -163,11 +163,11 @@ static inline unsigned long nr_slots(u64 val) */ void __init swiotlb_update_mem_attributes(void) { - struct io_tlb_mem *mem = io_tlb_default_mem; + struct io_tlb_mem *mem = &io_tlb_default_mem; void *vaddr; unsigned long bytes; - if (!mem || mem->late_alloc) + if (!mem->nslabs || mem->late_alloc) return; vaddr = phys_to_virt(mem->start); bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT); @@ -201,25 +201,24 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start, int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose) { - struct io_tlb_mem *mem; + struct io_tlb_mem *mem = &io_tlb_default_mem; size_t alloc_size; if (swiotlb_force == SWIOTLB_NO_FORCE) return 0; /* protect against double initialization */ - if (WARN_ON_ONCE(io_tlb_default_mem)) + if (WARN_ON_ONCE(mem->nslabs)) return -ENOMEM; - alloc_size = PAGE_ALIGN(struct_size(mem, slots, nslabs)); - mem = memblock_alloc(alloc_size, PAGE_SIZE); - if (!mem) + alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs)); + mem->slots = memblock_alloc(alloc_size, PAGE_SIZE); + if (!mem->slots) panic("%s: Failed to allocate %zu bytes align=0x%lx\n", __func__, alloc_size, PAGE_SIZE); swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false); - io_tlb_default_mem = mem; if (verbose) swiotlb_print_info(); swiotlb_set_max_segment(mem->nslabs << IO_TLB_SHIFT); @@ -304,26 +303,24 @@ swiotlb_late_init_with_default_size(size_t default_size) int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs) { - struct io_tlb_mem *mem; + struct io_tlb_mem *mem = &io_tlb_default_mem; unsigned long bytes = nslabs << IO_TLB_SHIFT; if (swiotlb_force == SWIOTLB_NO_FORCE) return 0; /* protect against double initialization */ - if (WARN_ON_ONCE(io_tlb_default_mem)) + if (WARN_ON_ONCE(mem->nslabs)) return -ENOMEM; - mem = (void *)__get_free_pages(GFP_KERNEL, - get_order(struct_size(mem, slots, nslabs))); - if (!mem) + mem->slots = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, + get_order(array_size(sizeof(*mem->slots), nslabs))); + if (!mem->slots) return -ENOMEM; - memset(mem, 0, sizeof(*mem)); set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT); swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true); - io_tlb_default_mem = mem; swiotlb_print_info(); swiotlb_set_max_segment(mem->nslabs << IO_TLB_SHIFT); return 0; @@ -331,18 +328,23 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs) void __init swiotlb_exit(void) { - struct io_tlb_mem *mem = io_tlb_default_mem; - size_t size; + struct io_tlb_mem *mem = &io_tlb_default_mem; + size_t tbl_size, slots_size; - if (!mem) + if (!mem->nslabs) return; - size = struct_size(mem, slots, mem->nslabs); - if (mem->late_alloc) - free_pages((unsigned long)mem, get_order(size)); - else - memblock_free_late(__pa(mem), PAGE_ALIGN(size)); - io_tlb_default_mem = NULL; + tbl_size = mem->end - mem->start; + slots_size = array_size(sizeof(*mem->slots), mem->nslabs); + if (mem->late_alloc) { + free_pages((unsigned long)mem->start, get_order(tbl_size)); + free_pages((unsigned long)mem->slots, get_order(slots_size)); + } else { + memblock_free_late(__pa(mem->start), PAGE_ALIGN(tbl_size)); + memblock_free_late(__pa(mem->slots), PAGE_ALIGN(slots_size)); + } + + memset(mem, 0, sizeof(*mem)); } /* @@ -682,7 +684,9 @@ size_t swiotlb_max_mapping_size(struct device *dev) bool is_swiotlb_active(struct device *dev) { - return dev->dma_io_tlb_mem != NULL; + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + + return mem && mem->nslabs; } EXPORT_SYMBOL_GPL(is_swiotlb_active); @@ -697,10 +701,10 @@ static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem) static int __init swiotlb_create_default_debugfs(void) { - struct io_tlb_mem *mem = io_tlb_default_mem; + struct io_tlb_mem *mem = &io_tlb_default_mem; debugfs_dir = debugfs_create_dir("swiotlb", NULL); - if (mem) { + if (mem->nslabs) { mem->debugfs = debugfs_dir; swiotlb_create_debugfs_files(mem); } @@ -754,10 +758,17 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem, * to it. */ if (!mem) { - mem = kzalloc(struct_size(mem, slots, nslabs), GFP_KERNEL); + mem = kzalloc(sizeof(*mem), GFP_KERNEL); if (!mem) return -ENOMEM; + mem->slots = kzalloc(array_size(sizeof(*mem->slots), nslabs), + GFP_KERNEL); + if (!mem->slots) { + kfree(mem); + return -ENOMEM; + } + set_memory_decrypted((unsigned long)phys_to_virt(rmem->base), rmem->size >> PAGE_SHIFT); swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false); @@ -781,7 +792,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem, static void rmem_swiotlb_device_release(struct reserved_mem *rmem, struct device *dev) { - dev->dma_io_tlb_mem = io_tlb_default_mem; + dev->dma_io_tlb_mem = &io_tlb_default_mem; } static const struct reserved_mem_ops rmem_swiotlb_ops = {
On 2021-07-06 14:24, Will Deacon wrote: > On Tue, Jul 06, 2021 at 06:48:48AM +0200, Christoph Hellwig wrote: >> On Mon, Jul 05, 2021 at 08:03:52PM +0100, Will Deacon wrote: >>> So at this point, the AMD IOMMU driver does: >>> >>> swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; >>> >>> where 'swiotlb' is a global variable indicating whether or not swiotlb >>> is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which >>> will call swiotlb_exit() if 'swiotlb' is false. >>> >>> Now, that used to work fine, because swiotlb_exit() clears >>> 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I >>> think that all the devices which have successfully probed beforehand will >>> have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' >>> field. >> >> Yeah. I don't think we can do that anymore, and I also think it is >> a bad idea to start with. > > I've had a crack at reworking things along the following lines: > > - io_tlb_default_mem now lives in the BSS, the flexible array member > is now a pointer and that part is allocated dynamically (downside of > this is an extra indirection to get at the slots). > > - io_tlb_default_mem.nslabs tells you whether the thing is valid > > - swiotlb_exit() frees the slots array and clears the rest of the > structure to 0. I also extended it to free the actual slabs, but I'm > not sure why it wasn't doing that before. > > So a non-NULL dev->dma_io_tlb_mem should always be valid to follow. FWIW I was pondering the question of whether to do something along those lines or just scrap the default assignment entirely, so since I hadn't got round to saying that I've gone ahead and hacked up the alternative (similarly untested) for comparison :) TBH I'm still not sure which one I prefer... Robin. ----->8----- diff --git a/drivers/base/core.c b/drivers/base/core.c index ea5b85354526..394abf184c1a 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2847,9 +2847,6 @@ void device_initialize(struct device *dev) defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL) dev->dma_coherent = dma_default_coherent; #endif -#ifdef CONFIG_SWIOTLB - dev->dma_io_tlb_mem = io_tlb_default_mem; -#endif } EXPORT_SYMBOL_GPL(device_initialize); diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 39284ff2a6cd..620f16d89a98 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -107,16 +107,21 @@ struct io_tlb_mem { }; extern struct io_tlb_mem *io_tlb_default_mem; +static inline struct io_tlb_mem *dev_iotlb_mem(struct device *dev) +{ + return dev->dma_io_tlb_mem ?: io_tlb_default_mem; +} + static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); return mem && paddr >= mem->start && paddr < mem->end; } static inline bool is_swiotlb_force_bounce(struct device *dev) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); return mem && mem->force_bounce; } @@ -167,7 +172,7 @@ bool swiotlb_free(struct device *dev, struct page *page, size_t size); static inline bool is_swiotlb_for_alloc(struct device *dev) { - return dev->dma_io_tlb_mem->for_alloc; + return dev_iotlb_mem(dev)->for_alloc; } #else static inline struct page *swiotlb_alloc(struct device *dev, size_t size) diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index b7f76bca89bf..f4942149f87d 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -359,7 +359,7 @@ static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size, enum dma_data_direction dir) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT; phys_addr_t orig_addr = mem->slots[index].orig_addr; size_t alloc_size = mem->slots[index].alloc_size; @@ -440,7 +440,7 @@ static unsigned int wrap_index(struct io_tlb_mem *mem, unsigned int index) static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr, size_t alloc_size) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned long boundary_mask = dma_get_seg_boundary(dev); dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, mem->start) & boundary_mask; @@ -522,7 +522,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr, size_t mapping_size, size_t alloc_size, enum dma_data_direction dir, unsigned long attrs) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned int offset = swiotlb_align_offset(dev, orig_addr); unsigned int i; int index; @@ -565,7 +565,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr, static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); unsigned long flags; unsigned int offset = swiotlb_align_offset(dev, tlb_addr); int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT; @@ -682,7 +682,7 @@ size_t swiotlb_max_mapping_size(struct device *dev) bool is_swiotlb_active(struct device *dev) { - return dev->dma_io_tlb_mem != NULL; + return dev_iotlb_mem(dev) != NULL; } EXPORT_SYMBOL_GPL(is_swiotlb_active); @@ -729,7 +729,7 @@ static void rmem_swiotlb_debugfs_init(struct reserved_mem *rmem) struct page *swiotlb_alloc(struct device *dev, size_t size) { - struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + struct io_tlb_mem *mem = dev_iotlb_mem(dev); phys_addr_t tlb_addr; int index; @@ -792,7 +792,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem, static void rmem_swiotlb_device_release(struct reserved_mem *rmem, struct device *dev) { - dev->dma_io_tlb_mem = io_tlb_default_mem; + dev->dma_io_tlb_mem = NULL; } static const struct reserved_mem_ops rmem_swiotlb_ops = {
On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > FWIW I was pondering the question of whether to do something along those > lines or just scrap the default assignment entirely, so since I hadn't got > round to saying that I've gone ahead and hacked up the alternative > (similarly untested) for comparison :) > > TBH I'm still not sure which one I prefer... Claire did implement something like your suggestion originally, but I don't really like it as it doesn't scale for adding multiple global pools, e.g. for the 64-bit addressable one for the various encrypted secure guest schemes.
On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > FWIW I was pondering the question of whether to do something along those > > lines or just scrap the default assignment entirely, so since I hadn't got > > round to saying that I've gone ahead and hacked up the alternative > > (similarly untested) for comparison :) > > > > TBH I'm still not sure which one I prefer... > > Claire did implement something like your suggestion originally, but > I don't really like it as it doesn't scale for adding multiple global > pools, e.g. for the 64-bit addressable one for the various encrypted > secure guest schemes. Couple of things: - I am not pushing to Linus the Claire's patchset until we have a resolution on this. I hope you all agree that is a sensible way forward as much as I hate doing that. - I like Robin's fix as it is simplest looking. Would love to see if it does fix the problem. - Christopher - we can always add multiple pools as the next milestone and just focus on this feature getting tested extensively during this release. - Would it be worth (for future or maybe in another tiny fix) to also add a printk in swiotlb when we de-allocate the buffer so when someone looks through the `dmesg` it becomes much easier to diagnose issues?
On 2021-07-06 15:05, Christoph Hellwig wrote: > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: >> FWIW I was pondering the question of whether to do something along those >> lines or just scrap the default assignment entirely, so since I hadn't got >> round to saying that I've gone ahead and hacked up the alternative >> (similarly untested) for comparison :) >> >> TBH I'm still not sure which one I prefer... > > Claire did implement something like your suggestion originally, but > I don't really like it as it doesn't scale for adding multiple global > pools, e.g. for the 64-bit addressable one for the various encrypted > secure guest schemes. Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're not concerned with a minimal fix for backports anyway I'm more than happy to focus on Will's approach. Another thing is that that looks to take us a quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, which is something that some of the hypervisor protection schemes looking to build on top of this series may want to explore at some point. Robin.
On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > FWIW I was pondering the question of whether to do something along those > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > round to saying that I've gone ahead and hacked up the alternative > > > (similarly untested) for comparison :) > > > > > > TBH I'm still not sure which one I prefer... > > > > Claire did implement something like your suggestion originally, but > > I don't really like it as it doesn't scale for adding multiple global > > pools, e.g. for the 64-bit addressable one for the various encrypted > > secure guest schemes. > > Couple of things: > - I am not pushing to Linus the Claire's patchset until we have a > resolution on this. I hope you all agree that is a sensible way > forward as much as I hate doing that. Sure, it's a pity but we could clearly use a bit more time to get these just right and we've run out of time for 5.14. I think the main question I have is how would you like to see patches for 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? Cheers, Will
On Tue, Jul 06, 2021 at 05:57:21PM +0100, Will Deacon wrote: > On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > FWIW I was pondering the question of whether to do something along those > > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > > round to saying that I've gone ahead and hacked up the alternative > > > > (similarly untested) for comparison :) > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > Claire did implement something like your suggestion originally, but > > > I don't really like it as it doesn't scale for adding multiple global > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > secure guest schemes. > > > > Couple of things: > > - I am not pushing to Linus the Claire's patchset until we have a > > resolution on this. I hope you all agree that is a sensible way > > forward as much as I hate doing that. > > Sure, it's a pity but we could clearly use a bit more time to get these > just right and we've run out of time for 5.14. > > I think the main question I have is how would you like to see patches for > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? Yes that would be perfect. If there are any dependencies on the rc1, I can rebase it on top of that. > > Cheers, > > Will
On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: > On 2021-07-06 15:05, Christoph Hellwig wrote: > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > FWIW I was pondering the question of whether to do something along those > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > round to saying that I've gone ahead and hacked up the alternative > > > (similarly untested) for comparison :) > > > > > > TBH I'm still not sure which one I prefer... > > > > Claire did implement something like your suggestion originally, but > > I don't really like it as it doesn't scale for adding multiple global > > pools, e.g. for the 64-bit addressable one for the various encrypted > > secure guest schemes. > > Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're > not concerned with a minimal fix for backports anyway I'm more than happy to > focus on Will's approach. Another thing is that that looks to take us a > quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, > which is something that some of the hypervisor protection schemes looking to > build on top of this series may want to explore at some point. Ok, I'll split that nasty diff I posted up into a reviewable series and we can take it from there. Will
Hi Will and Robin, On 7/6/2021 10:06 AM, Will Deacon wrote: > On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: >> On 2021-07-06 15:05, Christoph Hellwig wrote: >>> On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: >>>> FWIW I was pondering the question of whether to do something along those >>>> lines or just scrap the default assignment entirely, so since I hadn't got >>>> round to saying that I've gone ahead and hacked up the alternative >>>> (similarly untested) for comparison :) >>>> >>>> TBH I'm still not sure which one I prefer... >>> >>> Claire did implement something like your suggestion originally, but >>> I don't really like it as it doesn't scale for adding multiple global >>> pools, e.g. for the 64-bit addressable one for the various encrypted >>> secure guest schemes. >> >> Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're >> not concerned with a minimal fix for backports anyway I'm more than happy to >> focus on Will's approach. Another thing is that that looks to take us a >> quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, >> which is something that some of the hypervisor protection schemes looking to >> build on top of this series may want to explore at some point. > > Ok, I'll split that nasty diff I posted up into a reviewable series and we > can take it from there. For what it's worth, I attempted to boot Will's diff on top of Konrad's devel/for-linus-5.14 and it did not work; in fact, I got no output on my monitor period, even with earlyprintk=, and I do not think this machine has a serial console. Robin's fix does work, it survived ten reboots with no issues getting to X and I do not see the KASAN and slub debug messages anymore but I understand that this is not the preferred solution it seems (although Konrad did want to know if it works). I am happy to test any further patches or follow ups as needed, just keep me on CC. Cheers, Nathan
On Tue, Jul 06, 2021 at 12:14:16PM -0700, Nathan Chancellor wrote: > On 7/6/2021 10:06 AM, Will Deacon wrote: > > On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: > > > On 2021-07-06 15:05, Christoph Hellwig wrote: > > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > > FWIW I was pondering the question of whether to do something along those > > > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > > > round to saying that I've gone ahead and hacked up the alternative > > > > > (similarly untested) for comparison :) > > > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > > > Claire did implement something like your suggestion originally, but > > > > I don't really like it as it doesn't scale for adding multiple global > > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > > secure guest schemes. > > > > > > Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're > > > not concerned with a minimal fix for backports anyway I'm more than happy to > > > focus on Will's approach. Another thing is that that looks to take us a > > > quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, > > > which is something that some of the hypervisor protection schemes looking to > > > build on top of this series may want to explore at some point. > > > > Ok, I'll split that nasty diff I posted up into a reviewable series and we > > can take it from there. > > For what it's worth, I attempted to boot Will's diff on top of Konrad's > devel/for-linus-5.14 and it did not work; in fact, I got no output on my > monitor period, even with earlyprintk=, and I do not think this machine has > a serial console. Looking back at the diff, I completely messed up swiotlb_exit() by mixing up physical and virtual addresses. > Robin's fix does work, it survived ten reboots with no issues getting to X > and I do not see the KASAN and slub debug messages anymore but I understand > that this is not the preferred solution it seems (although Konrad did want > to know if it works). > > I am happy to test any further patches or follow ups as needed, just keep me > on CC. Cheers. Since this isn't 5.14 material any more, I'll CC you on a series next week. Will
On Tue, Jul 06, 2021 at 12:59:57PM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Jul 06, 2021 at 05:57:21PM +0100, Will Deacon wrote: > > On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > > > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > > FWIW I was pondering the question of whether to do something along those > > > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > > > round to saying that I've gone ahead and hacked up the alternative > > > > > (similarly untested) for comparison :) > > > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > > > Claire did implement something like your suggestion originally, but > > > > I don't really like it as it doesn't scale for adding multiple global > > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > > secure guest schemes. > > > > > > Couple of things: > > > - I am not pushing to Linus the Claire's patchset until we have a > > > resolution on this. I hope you all agree that is a sensible way > > > forward as much as I hate doing that. > > > > Sure, it's a pity but we could clearly use a bit more time to get these > > just right and we've run out of time for 5.14. > > > > I think the main question I have is how would you like to see patches for > > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? > > Yes that would be perfect. If there are any dependencies on the rc1, I > can rebase it on top of that. Yes, please, rebasing would be very helpful. The broader rework of 'io_tlb_default_mem' is going to conflict quite badly otherwise. Cheers, Will
..snip.. > > > I think the main question I have is how would you like to see patches for > > > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? > > > > Yes that would be perfect. If there are any dependencies on the rc1, I > > can rebase it on top of that. > > Yes, please, rebasing would be very helpful. The broader rework of > 'io_tlb_default_mem' is going to conflict quite badly otherwise. There is a devel/for-linus-5.15 (based on v5.14-rc1) now. Thank you! > > Cheers, > > Will
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..da348671b0d5 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,13 @@ 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) +{ + struct io_tlb_mem *mem = dev->dma_io_tlb_mem; + + return mem && 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 +129,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);