Messages in this thread | ![/](/images/icornerl.gif) | | From | Michael Kelley <> | Subject | RE: [PATCH v6 4/6] swiotlb: Fix alignment checks when both allocation and DMA masks are present | Date | Mon, 11 Mar 2024 21:36:10 +0000 |
| |
From: Petr Tesařík <petr@tesarici.cz> > On Fri, 8 Mar 2024 15:28:27 +0000 > Will Deacon <will@kernel.org> wrote:
[snip]
> > > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > > index c20324fba814..c381a7ed718f 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,14 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool > > BUG_ON(!nslots); > > BUG_ON(area_index >= pool->nareas); > > > > + /* > > + * Ensure that the allocation is at least slot-aligned and update > > + * 'iotlb_align_mask' to ignore bits that will be preserved when > > + * offsetting into the allocation. > > + */ > > + alloc_align_mask |= (IO_TLB_SIZE - 1); > > + iotlb_align_mask &= ~alloc_align_mask; > > + > > I have started writing the KUnit test suite, and the results look > incorrect to me for this case. > > I'm calling swiotlb_tbl_map_single() with: > > * alloc_align_mask = 0xfff > * a device with min_align_mask = 0xfff > * the 12 lowest bits of orig_addr are 0xfa0 > > The min_align_mask becomes zero after the masking added by this patch, > and the 12 lowest bits of the returned address are 0x7a0, i.e. not > equal to 0xfa0.
The address returned by swiotlb_tbl_map_single() is the slot index converted to an address, plus the offset modulo the min_align_mask for the device. The local variable "offset" in swiotlb_tbl_map_single() should be 0xfa0. The slot index should be an even number to meet the alloc_align_mask requirement. And the pool->start address should be at least page aligned, producing a page-aligned address *before* the offset is added. Can you debug which of these isn't true for the case you are seeing?
> > In other words, the min_align_mask constraint is not honored. Of course, > given the above values, it is not possible to honor both min_align_mask > and alloc_align_mask.
When orig_addr is specified and min_align_mask is set, alloc_align_mask governs the address of the _allocated_ space, which is not necessarily the returned physical address. The min_align_mask may dictate some pre-padding of size "offset" within the allocated space, and the returned address is *after* that pre-padding. In this way, both can be honored.
> I find it somewhat surprising that NVMe does not > in fact require that the NVME_CTRL_PAGE_SHIFT low bits are preserved, > as suggested by Nicolin's successful testing. > > Why is that?
I saw only one stack trace from Nicolin, and it was file system buffer flushing code that initiated the I/O. In such cases, it's very likely that the original address is at least 4K aligned. Hence the offset is zero and the low bits will typically be correct.
> > Does IOMMU do some additional post-processing of the bounce buffer > address to restore the value of bit 11?
Not that I can see.
> > Or is this bit always zero in all real-world scenarios?
For file system initiated I/Os, probably yes. But for raw disk I/Os initiated from user space, not necessarily.
Michael
| ![\](/images/icornerr.gif) |