Received-SPF: pass (google.com: domain of linux-kernel+bounces-104558-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 15 Mar 2024 16:09:31 +0100
From: Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To: Michael Kelley <mhklinux@outlook.com>
Cc: Petr Tesarik <petrtesarik@huaweicloud.com>, Christoph Hellwig
 <hch@lst.de>, Marek Szyprowski <m.szyprowski@samsung.com>, Robin Murphy
 <robin.murphy@arm.com>, Will Deacon <will@kernel.org>, Nicolin Chen
 <nicolinc@nvidia.com>, "open list:DMA MAPPING HELPERS"
 <iommu@lists.linux.dev>, open list <linux-kernel@vger.kernel.org>, Roberto
 Sassu <roberto.sassu@huaweicloud.com>, Petr Tesarik
 <petr.tesarik1@huawei-partners.com>
Subject: Re: [PATCH 1/1] swiotlb: extend buffer pre-padding to
 alloc_align_mask if necessary
Message-ID: <20240315160931.48879f5c@meshulam.tesarici.cz>
In-Reply-To: <SN6PR02MB41574FB8FF2FA08C26682EC1D4282@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240312134149.497-1-petrtesarik@huaweicloud.com>
	<SN6PR02MB4157FB27CD890E9F29408926D4282@SN6PR02MB4157.namprd02.prod.outlook.com>
	<20240315132613.5a340a69@meshulam.tesarici.cz>
	<SN6PR02MB41574FB8FF2FA08C26682EC1D4282@SN6PR02MB4157.namprd02.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, 15 Mar 2024 14:59:08 +0000
Michael Kelley <mhklinux@outlook.com> wrote:

> From: Petr Tesa=C5=99=C3=ADk <petr@tesarici.cz> Sent: Friday, March 15, 2=
024 5:26 AM
> >=20
> > On Fri, 15 Mar 2024 02:53:10 +0000
> > Michael Kelley <mhklinux@outlook.com> wrote:
> >  =20
> > > From: Petr Tesarik <petrtesarik@huaweicloud.com> Sent: Tuesday, March=
 12, 2024 6:42 AM =20
> > > > =20
>=20
> [snip]
>=20
> > > > @@ -1349,6 +1353,15 @@ phys_addr_t swiotlb_tbl_map_single(struct de=
vice *dev, phys_addr_t orig_addr,
> > > >  		return (phys_addr_t)DMA_MAPPING_ERROR;
> > > >  	}
> > > >
> > > > +	/*
> > > > +	 * Calculate buffer pre-padding within the allocated space. Use i=
t to
> > > > +	 * preserve the low bits of the original address according to dev=
ice'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 =3D orig_addr & dma_get_min_align_mask(dev) &
> > > > +		(alloc_align_mask | (IO_TLB_SIZE - 1));
> > > >  	index =3D swiotlb_find_slots(dev, orig_addr,
> > > >  				   alloc_size + offset, alloc_align_mask, &pool);
> > > >  	if (index =3D=3D -1) {
> > > > @@ -1364,6 +1377,12 @@ phys_addr_t swiotlb_tbl_map_single(struct de=
vice *dev, phys_addr_t orig_addr,
> > > >  	 * This is needed when we sync the memory.  Then we sync the buff=
er if
> > > >  	 * needed.
> > > >  	 */
> > > > +	padding =3D 0;
> > > > +	while (offset >=3D IO_TLB_SIZE) {
> > > > +		pool->slots[index++].orig_addr =3D INVALID_PHYS_ADDR;
> > > > +		pool->slots[index].padding =3D ++padding;
> > > > +		offset -=3D IO_TLB_SIZE;
> > > > +	} =20
> > >
> > > Looping to fill in the padding slots seems unnecessary.
> > > The orig_addr field should already be initialized to
> > > INVALID_PHYS_ADDR, and the padding field should already
> > > be initialized to zero. =20
> >=20
> > Ack.
> >  =20
> > > The value of "padding" should be just
> > > (offset / IO_TLB_SIZE), and it only needs to be stored in the
> > > first non-padding slot where swiotlb_release_slots() will
> > > find it. =20
> >=20
> > This is also right. I asked myself the question what should happen if
> > somebody passes a padding slot's address to swiotlb_tbl_unmap_single().
> > Of course, it's an invalid address, but as a proponent of defensive
> > programming, I still asked what would be the best response? If I store
> > padding in each slot, the whole block is released. If I store it only
> > in the first non-padding slot, some slots may leak.
> >=20
> > On a second thought, the resulting SWIOTLB state is consistent either
> > way, and we don't to care about leaking some slots if a driver is
> > buggy. Maybe it's even better, because the leak will be noticed.
> >=20
> > In short, I agree, let's keep the code simple.
> >  =20
>=20
> One other thought:  Fundamentally, fixing the core problem
> requires swiotlb_tbl_unmap_single() to have some information
> it doesn't have in the current code.  It needs to know the
> number of padding slots, so that it can free them correctly.
> Your solution is to store the # of padding slots in the
> io_tlb_slot array.
>=20
> Another approach would be to have callers pass the
> alloc_align_mask as an argument to swiotlb_tbl_unmap_single().
> It can then calculate the offset and the number of padding
> slots just like swiotlb_tbl_map_single() does.  Nothing
> additional would need to be stored in the io_tlb_slot array.
> The IOMMU code is the only caller than uses a non-zero
> alloc_align_mask.  From a quick look at that code, the
> unmap path has the iova_mask() available, so that would
> work.  Other callers would pass zero, just like they do for
> swiotlb_tbl_map_single().
>=20
> I don't immediately have a strong opinion either way, but
> it's something to think about a bit more.

I believe it's slightly more robust to store how the buffer was
actually allocated than to rely on the caller. It seems to me that this
was also a design goal of the original author. For example, note that
swiotlb_tbl_unmap_single() uses mapping_size only to do the final
buffer sync, but not to determine how many slots should be released.
This information is taken from struct io_tlb_slot.alloc_size.

Petr T