Received-SPF: pass (google.com: domain of linux-kernel+bounces-99573-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Michael Kelley <mhklinux@outlook.com>
To: =?iso-8859-2?Q?Petr_Tesa=F8=EDk?= <petr@tesarici.cz>, Will Deacon
	<will@kernel.org>, Nicolin Chen <nicolinc@nvidia.com>
CC: "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>, Robin Murphy <robin.murphy@arm.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
Thread-Topic: [PATCH v6 4/6] swiotlb: Fix alignment checks when both
 allocation and DMA masks are present
Thread-Index: AQHacW1UX2JG3DS7FkeH1aW1bgiyobEy+/iAgAAVCNA=
Date: Mon, 11 Mar 2024 21:36:10 +0000
Message-ID:
 <SN6PR02MB41576E58DDF5A56FC6FC1EB6D4242@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240308152829.25754-1-will@kernel.org>
	<20240308152829.25754-5-will@kernel.org>
 <20240311210507.217daf8b@meshulam.tesarici.cz>
In-Reply-To: <20240311210507.217daf8b@meshulam.tesarici.cz>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?iso-8859-2?Q?wpurfSUiUjfWDkFAMhx0LsxEA6fIxJEcQEk35Vwn1WzPcbXXPVdHYq73M2?=
 =?iso-8859-2?Q?4WYZtR7OjkClgqP1eknQ7ejNWASkLyf+SHHBkWQrmujc3ONcXQ9UajoVlz?=
 =?iso-8859-2?Q?+Wtu3K5w+Wlb+ccbU5GJqaILr4+xEv6lsN0atPMwKWTpC2ZwtieTEO4Qp4?=
 =?iso-8859-2?Q?8P9b3+9Sq40OTKyNzsIu0Ko7ytE94jsypgkKg6yhsJwjPKFWDPqcz41zo0?=
 =?iso-8859-2?Q?CJg7beCGm2GJOh7QNYuTDF4tZLmiUsxS4RI4E0eRZ1VAhbFJEaGd8OZIsD?=
 =?iso-8859-2?Q?+GESGdrzq6dhn0XOJlDVdLM8BKalUrDq0ADIvwtMWfu6j2pXU9x1bjmnlb?=
 =?iso-8859-2?Q?a9m9fTS9ylpmX7UaJcxMI5UHS0J7vio40sW2zYnYjwcOcduXP14rb4h8me?=
 =?iso-8859-2?Q?8UoI68v8OEA1mP/FJlZw5V/FBFsOEc17XebSHTT3hpx53WAO0IJW9tlR4Y?=
 =?iso-8859-2?Q?xvNpO6v8gVM4HRKpANxej9yR57XMBwVvR5TurEtXZL6LIhvuPQDVybtiNb?=
 =?iso-8859-2?Q?J35MysA6r32EMRM/xD6Kzc/o1YUEip0JfUVfBmVEPCEF5qfskuxuh0mdX0?=
 =?iso-8859-2?Q?J1TRaO4EilLyrzbfLNQkYhEZvnX5D+BWuEZYzzZwL2o1iCXZ1yuDJVXjHa?=
 =?iso-8859-2?Q?klsrgq6BcHER25q/ltJ5fPKiELfmw1qGjQDWgOEnFekQIksVobDtReIano?=
 =?iso-8859-2?Q?Mu7unONIJFrqooJr/pmZDDqd25taHV75r7AR5pQAnVl00i60u6XVDAdijS?=
 =?iso-8859-2?Q?78ooNkWeIa8vBjAClaNoYQgTRBeRG1IInbBgxLJIGx8X9J2GukxBK0Ve9Z?=
 =?iso-8859-2?Q?u7CC50pC/eQLgZM7iv8diHkyF+1R+irFvPvtANFbQk/4Fazu26bZRoKceQ?=
 =?iso-8859-2?Q?e58g+S1fWdGRDcLGq3H437uWOOE2bpY/WYGPmXvx4n/uKTMYPoOplAerKx?=
 =?iso-8859-2?Q?hiYLIOAFhCiarge3B0RUnixcTSi+HxIbfcB70ZLCyCgWeG9hi4o+ueqmO6?=
 =?iso-8859-2?Q?QfasbyKn6Nh8y35WgC6GkQwX2Y04YpN57dvU/LpHd2bTuuHQPGQDEr+Yhi?=
 =?iso-8859-2?Q?E78arrMhMmf75ISWo+KDfl17nWGYKHJtvhMCBYDR5pi+7AqsT7cSw2dO1V?=
 =?iso-8859-2?Q?Pz/efyTRROGgU4Z1hr0o/hJaRkiIFU6lTsCMFqO9Kye4c3JdpECcy4kwem?=
 =?iso-8859-2?Q?lMsg8UnwSa/6iGZ7iHieAhPhc7f9ToG5pNFFlNMTaOzT5gVTm0maL3WvMc?=
 =?iso-8859-2?Q?pnQt8RhnM67gJ5iEi5YQ=3D=3D?=
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4157.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: ca5d7429-2a15-4e82-ad2a-08dc421344e6
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Mar 2024 21:36:10.7516
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR02MB9607

From: Petr Tesa=F8=EDk <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 =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 device =
*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.
>=20
> I'm calling swiotlb_tbl_map_single() with:
>=20
> * alloc_align_mask =3D 0xfff
> * a device with min_align_mask =3D 0xfff
> * the 12 lowest bits of orig_addr are 0xfa0
>=20
> 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?

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

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

>=20
> Does IOMMU do some additional post-processing of the bounce buffer
> address to restore the value of bit 11?

Not that I can see.

>=20
> 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