Received-SPF: pass (google.com: domain of linux-kernel+bounces-88725-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <4fbcdd49-cd93-4af8-83e2-f1cef0ea2eeb@arm.com>
Date: Fri, 1 Mar 2024 16:38:33 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 6/6] swiotlb: Remove pointless stride adjustment for
 allocations >= PAGE_SIZE
Content-Language: en-GB
To: =?UTF-8?B?UGV0ciBUZXNhxZnDrWs=?= <petr@tesarici.cz>,
 Christoph Hellwig <hch@lst.de>
Cc: Michael Kelley <mhklinux@outlook.com>, Will Deacon <will@kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 Petr Tesarik <petr.tesarik1@huawei-partners.com>,
 "kernel-team@android.com" <kernel-team@android.com>,
 "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
 Marek Szyprowski <m.szyprowski@samsung.com>, Dexuan Cui
 <decui@microsoft.com>, Nicolin Chen <nicolinc@nvidia.com>
References: <20240228133930.15400-1-will@kernel.org>
 <20240228133930.15400-7-will@kernel.org>
 <SN6PR02MB4157A62353559DA8DB8BC4ADD45F2@SN6PR02MB4157.namprd02.prod.outlook.com>
 <SN6PR02MB41577D09E97B1D9645369D58D45F2@SN6PR02MB4157.namprd02.prod.outlook.com>
 <20240229133346.GA7177@lst.de>
 <SN6PR02MB4157314F142D05E279B7991ED45F2@SN6PR02MB4157.namprd02.prod.outlook.com>
 <20240229154756.GA10137@lst.de>
 <20240301163927.18358ee2@meshulam.tesarici.cz>
From: Robin Murphy <robin.murphy@arm.com>
In-Reply-To: <20240301163927.18358ee2@meshulam.tesarici.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 2024-03-01 3:39 pm, Petr Tesařík wrote:
> On Thu, 29 Feb 2024 16:47:56 +0100
> Christoph Hellwig <hch@lst.de> wrote:
> 
>> On Thu, Feb 29, 2024 at 03:44:11PM +0000, Michael Kelley wrote:
>>> Any thoughts on how that historical behavior should apply if
>>> the DMA min_align_mask is non-zero, or the alloc_align_mask
>>> parameter to swiotbl_tbl_map_single() is non-zero? As currently
>>> used, alloc_align_mask is page aligned if the IOMMU granule is
>>>> = PAGE_SIZE. But a non-zero min_align_mask could mandate
>>> returning a buffer that is not page aligned. Perhaps do the
>>> historical behavior only if alloc_align_mask and min_align_mask
>>> are both zero?
>>
>> I think the driver setting min_align_mask is a clear indicator
>> that the driver requested a specific alignment and the defaults
>> don't apply.  For swiotbl_tbl_map_single as used by dma-iommu
>> I'd have to tak a closer look at how it is used.
> 
> I'm not sure it helps in this discussion, but let me dive into a bit
> of ancient history to understand how we ended up here.
> 
> IIRC this behaviour was originally motivated by limitations of PC AT
> hardware. Intel 8237 is a 16-bit DMA controller. To make it somehow
> usable with addresses up to 16MB (yeah, the infamous DMA zone), IBM
> added a page register, but it was on a separate chip and it did not
> increment when the 8237 address rolled over back to zero. Effectively,
> the page register selected a 64K-aligned window of 64K buffers.
> Consequently, DMA buffers could not cross a 64K physical boundary.
> 
> Thanks to how the buddy allocator works, the 64K-boundary constraint
> was satisfied by allocation size, and drivers took advantage of it when
> allocating device buffers. IMO software bounce buffers simply followed
> the same logic that worked for buffers allocated by the buddy allocator.
> 
> OTOH min_align_mask was motivated by NVME which prescribes the value of
> a certain number of low bits in the DMA address (for simplicity assumed
> to be identical with the same bits in the physical address).
> 
> The only pre-existing user of alloc_align_mask is x86 IOMMU code, and
> IIUC it is used to guarantee that unaligned transactions do not share
> the IOMMU granule with another device. This whole thing is weird,
> because swiotlb_tbl_map_single() is called like this:
> 
>                  aligned_size = iova_align(iovad, size);
>                  phys = swiotlb_tbl_map_single(dev, phys, size, aligned_size,
>                                                iova_mask(iovad), dir, attrs);
> 
> Here:
> 
> * alloc_size = iova_align(iovad, size)
> * alloc_align_mask = iova_mask(iovad)
> 
> Now, iova_align() rounds up its argument to a multiple of iova granule
> and iova_mask() is simply "granule - 1". This works, because granule
> size must be a power of 2, and I assume it must also be >= PAGE_SIZE.

Not quite, the granule must *not* be larger than PAGE_SIZE (but can be 
smaller).
> In that case, the alloc_align_mask argument is not even needed if you
> adjust the code to match documentation---the resulting buffer will be
> aligned to a granule boundary by virtue of having a size that is a
> multiple of the granule size.

I think we've conflated two different notions of "allocation" here. The 
page-alignment which Christoph quoted applies for dma_alloc_coherent(), 
which nowadays *should* only be relevant to SWIOTLB code in the 
swiotlb_alloc() path for stealing coherent pages out of restricted DMA 
pools. Historically there was never any official alignment requirement 
for the DMA addresses returned by dma_map_{page,sg}(), until 
min_align_mask was introduced.

> To sum it up:
> 
> 1. min_align_mask is by far the most important constraint. Devices will
>     simply stop working if it is not met.
> 2. Alignment to the smallest PAGE_SIZE order which is greater than or
>     equal to the requested size has been documented, and some drivers
>     may rely on it.

Strictly it stopped being necessary since fafadcd16595 when the 
historical swiotlb_alloc() was removed, but 602d9858f07c implies that 
indeed the assumption of it for streaming mappings had already set in. 
Of course NVMe is now using min_align_mask, but I'm not sure if it's 
worth taking the risk to find out who else should also be.

> 3. alloc_align_mask is a misguided fix for a bug in the above.

No, alloc_align_mask is about describing the explicit requirement rather 
than relying on any implicit behaviour, and thus being able to do the 
optimal thing for, say, a 9KB mapping given a 4KB IOVA granule and 64KB 
PAGE_SIZE.

Thanks,
Robin.

> 
> Correct me if anything of the above is wrong.
> 
> HTH
> Petr T