Received-SPF: pass (google.com: domain of linux-kernel+bounces-100075-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 12 Mar 2024 10:51:03 +0100
From: Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Will Deacon <will@kernel.org>, Michael Kelley <mhklinux@outlook.com>,
 Nicolin Chen <nicolinc@nvidia.com>, "linux-kernel@vger.kernel.org"
 <linux-kernel@vger.kernel.org>, "kernel-team@android.com"
 <kernel-team@android.com>, "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
 Christoph Hellwig <hch@lst.de>, Marek Szyprowski
 <m.szyprowski@samsung.com>, Petr Tesarik
 <petr.tesarik1@huawei-partners.com>, Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH v6 4/6] swiotlb: Fix alignment checks when both
 allocation and DMA masks are present
Message-ID: <20240312105103.0f3b6a80@meshulam.tesarici.cz>
In-Reply-To: <2612acb1-48f9-4117-ae06-9b8f430034ca@arm.com>
References: <20240308152829.25754-1-will@kernel.org>
	<20240308152829.25754-5-will@kernel.org>
	<20240311210507.217daf8b@meshulam.tesarici.cz>
	<SN6PR02MB41576E58DDF5A56FC6FC1EB6D4242@SN6PR02MB4157.namprd02.prod.outlook.com>
	<20240311224910.GA28426@willie-the-truck>
	<20240312095223.7a05d5b8@meshulam.tesarici.cz>
	<2612acb1-48f9-4117-ae06-9b8f430034ca@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, 12 Mar 2024 09:38:36 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2024-03-12 8:52 am, Petr Tesa=C5=99=C3=ADk wrote:
> > On Mon, 11 Mar 2024 22:49:11 +0000
> > Will Deacon <will@kernel.org> wrote:
> >  =20
> >> On Mon, Mar 11, 2024 at 09:36:10PM +0000, Michael Kelley wrote: =20
> >>> From: Petr Tesa=C5=99=C3=ADk <petr@tesarici.cz> =20
> >>>> On Fri,  8 Mar 2024 15:28:27 +0000
> >>>> Will Deacon <will@kernel.org> wrote: =20
> >>>>> 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 devi=
ce *dev, struct io_tlb_pool *pool
> >>>>>   	dma_addr_t tbl_dma_addr =3D
> >>>>>   		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> >>>>>   	unsigned long max_slots =3D get_max_slots(boundary_mask);
> >>>>> -	unsigned int iotlb_align_mask =3D
> >>>>> -		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> >>>>> +	unsigned int iotlb_align_mask =3D dma_get_min_align_mask(dev);
> >>>>>   	unsigned int nslots =3D nr_slots(alloc_size), stride;
> >>>>>   	unsigned int offset =3D swiotlb_align_offset(dev, orig_addr);
> >>>>>   	unsigned int index, slots_checked, count =3D 0, i;
> >>>>> @@ -993,6 +992,14 @@ static int swiotlb_search_pool_area(struct dev=
ice *dev, struct io_tlb_pool *pool
> >>>>>   	BUG_ON(!nslots);
> >>>>>   	BUG_ON(area_index >=3D 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 |=3D (IO_TLB_SIZE - 1);
> >>>>> +	iotlb_align_mask &=3D ~alloc_align_mask;
> >>>>> + =20
> >>>>
> >>>> 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 =3D 0xfff
> >>>> * a device with min_align_mask =3D 0xfff
> >>>> * the 12 lowest bits of orig_addr are 0xfa0
> >>>>
> >>>> The min_align_mask becomes zero after the masking added by this patc=
h,
> >>>> and the 12 lowest bits of the returned address are 0x7a0, i.e. not
> >>>> equal to 0xfa0. =20
> >>>
> >>> 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* t=
he
> >>> offset is added. Can you debug which of these isn't true for the case
> >>> you are seeing? =20
> >>
> >> I was just looking into this, and I think the problem starts because
> >> swiotlb_align_offset() doesn't return the offset modulo the min_align_=
mask,
> >> but instead returns the offset *into the slot*:
> >>
> >> 	return addr & dma_get_min_align_mask(dev) & (IO_TLB_SIZE - 1);
> >>
> >> so this presumably lops off bit 11 without adjusting the slot number. =
=20
> >=20
> > Yes. You will never see an offset bigger than IO_TLB_SIZE.
> >  =20
> >> I don't think swiotlb_find_slots() should be handling this though; it's
> >> more about how swiotlb_tbl_map_single() puts the address back together
> >> again. =20
> >>>> In other words, the min_align_mask constraint is not honored. Of cou=
rse,
> >>>> given the above values, it is not possible to honor both min_align_m=
ask
> >>>> and alloc_align_mask. =20
> >>>
> >>> When orig_addr is specified and min_align_mask is set, alloc_align_ma=
sk
> >>> governs the address of the _allocated_ space, which is not necessaril=
y the
> >>> returned physical address.  The min_align_mask may dictate some
> >>> pre-padding of size "offset" within the allocated space, and the retu=
rned
> >>> address is *after* that pre-padding.  In this way, both can be honore=
d. =20
> >>
> >> I agree, modulo the issue with the offset calculation. =20
> >=20
> > *sigh*
> >=20
> > This is exactly what I tried to suggest here:
> >=20
> >    https://lore.kernel.org/linux-iommu/20240301180853.5ac20b27@meshulam=
tesarici.cz/
> >=20
> > To which Robin Murphy replied:
> >  =20
> >> That doesn't make sense - a caller asks to map some range of kernel
> >> addresses and they get back a corresponding range of DMA addresses; th=
ey
> >> cannot make any reasonable assumptions about DMA addresses *outside*
> >> that range. =20
> >=20
> > It sounded like a misunderstanding back then already, but in light of
> > the present findings, should I send the corresponding patch after all? =
=20
>=20
> No, that comment was in reference to the idea of effectively forcing=20
> alloc_align_mask in order to honour min_align_mask - specifically that=20
> the reasoning given for it was spurious, but it's clear now it would=20
> also simply exacerbate this problem.
>=20
> Simply put, if min_align_mask is specified alone, SWIOTLB can allocate a=
=20
> roughly-aligned range of slots such that the bounce offset is always=20
> less than IO_TLB_SIZE from the start of the allocation; if both=20
> min_align_mask and alloc_align_mask are specified, then the bounce=20
> offset may be larger than IO_TLB_SIZE, and SWIOTLB needs to be able to=20
> handle that correctly. There is still no benefit in forcing the latter=20
> case to happen more often than it needs to.

So yes, it was a misunderstanding. Here's what I wrote:

I thought about it some more, and I believe I know what should happen
if the first two constraints appear to be mutually exclusive.

I thought it was clear that the two constraints "appear mutually
exclusive" only if both are specified. Admittedly, I also tried to
combine the historic page alignment with the explicit alloc_align_mask
somehow, so that could have caused confusion.

Anyway, let me send the patch and discuss it in a new thread.

Petr T