Received-SPF: pass (google.com: domain of linux-kernel+bounces-106530-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 18 Mar 2024 19:50:44 +0100
From: Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To: Michael Kelley <mhklinux@outlook.com>
Cc: Christoph Hellwig <hch@lst.de>, Marek Szyprowski
 <m.szyprowski@samsung.com>, Robin Murphy <robin.murphy@arm.com>, Petr
 Tesarik <petr.tesarik1@huawei-partners.com>, Will Deacon <will@kernel.org>,
 open list <linux-kernel@vger.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@lists.linux.dev>, Roberto Sassu <roberto.sassu@huaweicloud.com>,
 Petr Tesarik <petrtesarik@huaweicloud.com>
Subject: Re: [PATCH v2 1/2] swiotlb: extend buffer pre-padding to
 alloc_align_mask if necessary
Message-ID: <20240318195007.2a91dc87@meshulam.tesarici.cz>
In-Reply-To: <SN6PR02MB41571EBB261FA3B5FFCD9BAFD42D2@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240318130447.594-1-petrtesarik@huaweicloud.com>
	<20240318130447.594-2-petrtesarik@huaweicloud.com>
	<SN6PR02MB41571EBB261FA3B5FFCD9BAFD42D2@SN6PR02MB4157.namprd02.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 18 Mar 2024 15:37:16 +0000
Michael Kelley <mhklinux@outlook.com> wrote:

> From: Petr Tesarik <petrtesarik@huaweicloud.com> Sent: Monday, March 18, 2024 6:05 AM
> > 
> > Allow a buffer pre-padding of up to alloc_align_mask. If the allocation
> > alignment is bigger than IO_TLB_SIZE and min_align_mask covers any non-zero
> > bits in the original address between IO_TLB_SIZE and alloc_align_mask,
> > these bits are not preserved in the swiotlb buffer address.
> > 
> > To fix this case, increase the allocation size and use a larger offset
> > within the allocated buffer. As a result, extra padding slots may be
> > allocated before the mapping start address.
> > 
> > Set the orig_addr in these padding slots to INVALID_PHYS_ADDR, because they
> > do not correspond to any CPU buffer and the data must never be synced.  
> 
> This paragraph is now obsolete.

Right. It should now say that orig_addr already _is_ set to
INVALID_PHYS_ADDR, so attempts to sync data will be ignored.

> > 
> > The padding slots should be automatically released when the buffer is
> > unmapped. However, swiotlb_tbl_unmap_single() takes only the address of the
> > DMA buffer slot, not the first padding slot. Save the number of padding
> > slots in struct io_tlb_slot and use it to adjust the slot index in
> > swiotlb_release_slots(), so all allocated slots are properly freed.
> > 
> > Fixes: 2fd4fa5d3fb5 ("swiotlb: Fix alignment checks when both allocation and DMA masks are present")
> > Link: https://lore.kernel.org/linux-iommu/20240311210507.217daf8b@meshulam.tesarici.cz/
> > Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > ---
> >  kernel/dma/swiotlb.c | 35 +++++++++++++++++++++++++++++------
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index 86fe172b5958..aefb05ff55e7 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -69,11 +69,14 @@
> >   * @alloc_size:	Size of the allocated buffer.
> >   * @list:	The free list describing the number of free entries available
> >   *		from each index.
> > + * @pad_slots:	Number of preceding padding slots. Valid only in the first
> > + *		allocated non-padding slot.
> >   */
> >  struct io_tlb_slot {
> >  	phys_addr_t orig_addr;
> >  	size_t alloc_size;
> > -	unsigned int list;
> > +	unsigned short list;
> > +	unsigned short pad_slots;
> >  };
> > 
> >  static bool swiotlb_force_bounce;
> > @@ -287,6 +290,7 @@ static void swiotlb_init_io_tlb_pool(struct io_tlb_pool *mem, phys_addr_t start,
> >  					 mem->nslabs - i);
> >  		mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> >  		mem->slots[i].alloc_size = 0;
> > +		mem->slots[i].pad_slots = 0;
> >  	}
> > 
> >  	memset(vaddr, 0, bytes);
> > @@ -1328,11 +1332,12 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> >  		unsigned long attrs)
> >  {
> >  	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > -	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > +	unsigned int offset;
> >  	struct io_tlb_pool *pool;
> >  	unsigned int i;
> >  	int index;
> >  	phys_addr_t tlb_addr;
> > +	unsigned short pad_slots;
> > 
> >  	if (!mem || !mem->nslabs) {
> >  		dev_warn_ratelimited(dev,
> > @@ -1349,6 +1354,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> >  		return (phys_addr_t)DMA_MAPPING_ERROR;
> >  	}
> > 
> > +	/*
> > +	 * Calculate buffer pre-padding within the allocated space. Use it to
> > +	 * preserve the low bits of the original address according to device's
> > +	 * min_align_mask. Limit the padding to alloc_align_mask or slot size
> > +	 * (whichever is bigger); higher bits of the original address are
> > +	 * preserved by selecting a suitable IO TLB slot.
> > +	 */
> > +	offset = orig_addr & dma_get_min_align_mask(dev) &
> > +		(alloc_align_mask | (IO_TLB_SIZE - 1));
> >  	index = swiotlb_find_slots(dev, orig_addr,
> >  				   alloc_size + offset, alloc_align_mask, &pool);
> >  	if (index == -1) {
> > @@ -1364,6 +1378,10 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> >  	 * This is needed when we sync the memory.  Then we sync the buffer if
> >  	 * needed.
> >  	 */
> > +	pad_slots = offset / IO_TLB_SIZE;
> > +	offset %= IO_TLB_SIZE;
> > +	index += pad_slots;
> > +	pool->slots[index].pad_slots = i;  
> 
> The above line should be:
> 	pool->slots[index].pad_slots = pad_slots;

Doh. Yes, I rewrote it a few times and then forgot to change this.

How did it even pass the test suite?

Thank you for the review.

Petr T

> >  	for (i = 0; i < nr_slots(alloc_size + offset); i++)
> >  		pool->slots[index + i].orig_addr = slot_addr(orig_addr, i);
> >  	tlb_addr = slot_addr(pool->start, index) + offset;
> > @@ -1385,12 +1403,16 @@ static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr)
> >  	struct io_tlb_pool *mem = swiotlb_find_pool(dev, tlb_addr);
> >  	unsigned long flags;
> >  	unsigned int offset = swiotlb_align_offset(dev, tlb_addr);
> > -	int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
> > -	int nslots = nr_slots(mem->slots[index].alloc_size + offset);
> > -	int aindex = index / mem->area_nslabs;
> > -	struct io_tlb_area *area = &mem->areas[aindex];
> > +	int index, nslots, aindex;
> > +	struct io_tlb_area *area;
> >  	int count, i;
> > 
> > +	index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
> > +	index -= mem->slots[index].pad_slots;
> > +	nslots = nr_slots(mem->slots[index].alloc_size + offset);
> > +	aindex = index / mem->area_nslabs;
> > +	area = &mem->areas[aindex];
> > +
> >  	/*
> >  	 * Return the buffer to the free list by setting the corresponding
> >  	 * entries to indicate the number of contiguous entries available.
> > @@ -1413,6 +1435,7 @@ static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr)
> >  		mem->slots[i].list = ++count;
> >  		mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> >  		mem->slots[i].alloc_size = 0;
> > +		mem->slots[i].pad_slots = 0;
> >  	}
> > 
> >  	/*
> > --
> > 2.34.1  
>