Re: [PATCH v1 0/2] nvme-pci: Fix dma-iommu mapping failures when PAGE_SIZE=64KB

[Will Deacon <will@kernel.org>]

Hi Nicolin,

Thanks for sharing all the logs, .config etc.

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
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.

Cheers,

Will

--->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))