Message-ID: <ZdBB8BYsK6WvwTYC@Asurada-Nvidia>
Date: Fri, 16 Feb 2024 21:19:44 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: Will Deacon <will@...nel.org>
CC: <sagi@...mberg.me>, <hch@....de>, <axboe@...nel.dk>, <kbusch@...nel.org>,
	<joro@...tes.org>, <robin.murphy@....com>, <jgg@...dia.com>,
	<linux-nvme@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<iommu@...ts.linux.dev>, <murphyt7@....ie>, <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH v1 0/2] nvme-pci: Fix dma-iommu mapping failures when
 PAGE_SIZE=64KB

Hi Will,

On Fri, Feb 16, 2024 at 04:13:12PM +0000, Will Deacon wrote:
> On Thu, Feb 15, 2024 at 04:26:23PM -0800, Nicolin Chen wrote:
> > On Thu, Feb 15, 2024 at 04:35:45PM +0000, Will Deacon wrote:
> > > On Thu, Feb 15, 2024 at 02:22:09PM +0000, Will Deacon wrote:
> > > > On Wed, Feb 14, 2024 at 11:57:32AM -0800, Nicolin Chen wrote:
> > > > > On Wed, Feb 14, 2024 at 04:41:38PM +0000, Will Deacon wrote:
> > > > > > On Tue, Feb 13, 2024 at 01:53:55PM -0800, Nicolin Chen wrote:
> > > > > And it seems to get worse, as even a 64KB mapping is failing:
> > > > > [    0.239821] nvme 0000:00:01.0: swiotlb buffer is full (sz: 65536 bytes), total 32768 (slots), used 0 (slots)
> > > > >
> > > > > With a printk, I found the iotlb_align_mask isn't correct:
> > > > >    swiotlb_area_find_slots:alloc_align_mask 0xffff, iotlb_align_mask 0x800
> > > > >
> > > > > But fixing the iotlb_align_mask to 0x7ff still fails the 64KB
> > > > > mapping..
> > > >
> > > > Hmm. A mask of 0x7ff doesn't make a lot of sense given that the slabs
> > > > are 2KiB aligned. I'll try plugging in some of the constants you have
> > > > here, as something definitely isn't right...
> > >
> > > Sorry, another ask: please can you print 'orig_addr' in the case of the
> > > failing allocation?
> >
> > I added nvme_print_sgl() in the nvme-pci driver before its
> > dma_map_sgtable() call, so the orig_addr isn't aligned with
> > PAGE_SIZE=64K or NVME_CTRL_PAGE_SIZE=4K:
> >  sg[0] phys_addr:0x0000000105774600 offset:17920 length:512 dma_address:0x0000000000000000 dma_length:0
> >
> > Also attaching some verbose logs, in case you'd like to check:
> >    nvme 0000:00:01.0: swiotlb_area_find_slots: dma_get_min_align_mask 0xfff, IO_TLB_SIZE 0xfffff7ff
> >    nvme 0000:00:01.0: swiotlb_area_find_slots: alloc_align_mask 0xffff, iotlb_align_mask 0x7ff
> >    nvme 0000:00:01.0: swiotlb_area_find_slots: stride 0x20, max 0xffff
> >    nvme 0000:00:01.0: swiotlb_area_find_slots: tlb_addr=0xbd830000, iotlb_align_mask=0x7ff, alloc_align_mask=0xffff
> > => nvme 0000:00:01.0: swiotlb_area_find_slots: orig_addr=0x105774600, iotlb_align_mask=0x7ff
> 
> With my patches, I think 'iotlb_align_mask' will be 0x800 here, so this

Oops, my bad. I forgot to revert the part that I mentioned in
my previous reply.

> particular allocation might be alright, however I think I'm starting to
> see the wider problem. The IOMMU code is asking for a 64k-aligned
> allocation so that it can map it safely, but at the same time
> dma_get_min_align_mask() is asking for congruence in the 4k NVME page
> offset. Now, because we're going to allocate a 64k-aligned mapping and
> offset it, I think the NVME alignment will just fall out in the wash and
> checking the 'orig_addr' (which includes the offset) is wrong.
> 
> So perhaps this diff (which I'm sadly not able to test) will help? You'll
> want to apply it on top of my other patches. The idea is to ignore the
> bits of 'orig_addr' which will be aligned automatically by offseting from
> the aligned allocation. I fixed the max() thing too, although that's only
> an issue for older kernels.
 
Yea, I tested all 4 patches. They still failed at some large
mapping, until I added on top of them my PATCH-1 implementing
the max_mapping_size op. IOW, with your patches it looks like
252KB max_mapping_size is working :)

Though we seem to have a solution now, I hope we can make it
applicable to older kernels too. The mapping failure on arm64
with PAGE_SIZE=64KB looks like a regression to me, since dma-
iommu started to use swiotlb bounce buffer.

Thanks
Nicolin

> --->8
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 283eea33dd22..4a000d97f568 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -981,8 +981,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>         dma_addr_t tbl_dma_addr =
>                 phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
>         unsigned long max_slots = get_max_slots(boundary_mask);
> -       unsigned int iotlb_align_mask =
> -               dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> +       unsigned int iotlb_align_mask = dma_get_min_align_mask(dev);
>         unsigned int nslots = nr_slots(alloc_size), stride;
>         unsigned int offset = swiotlb_align_offset(dev, orig_addr);
>         unsigned int index, slots_checked, count = 0, i;
> @@ -993,6 +992,9 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>         BUG_ON(!nslots);
>         BUG_ON(area_index >= pool->nareas);
> 
> +       alloc_align_mask |= (IO_TLB_SIZE - 1);
> +       iotlb_align_mask &= ~alloc_align_mask;
> +
>         /*
>          * For mappings with an alignment requirement don't bother looping to
>          * unaligned slots once we found an aligned one.
> @@ -1004,7 +1006,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>          * allocations.
>          */
>         if (alloc_size >= PAGE_SIZE)
> -               stride = max(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> +               stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> 
>         spin_lock_irqsave(&area->lock, flags);
>         if (unlikely(nslots > pool->area_nslabs - area->used))
> 

Powered by blists - more mailing lists