Received-SPF: pass (google.com: domain of linux-kernel+bounces-83516-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 27 Feb 2024 07:38:40 -0800
From: Christoph Hellwig <hch@infradead.org>
To: Will Deacon <will@kernel.org>
Cc: Michael Kelley <mhklinux@outlook.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>,
	Robin Murphy <robin.murphy@arm.com>,
	Petr Tesarik <petr.tesarik1@huawei-partners.com>,
	Dexuan Cui <decui@microsoft.com>,
	Nicolin Chen <nicolinc@nvidia.com>
Subject: Re: [PATCH v4 1/5] swiotlb: Fix double-allocation of slots due to
 broken alignment handling
Message-ID: <Zd4CANmJdW_t69S2@infradead.org>
References: <20240221113504.7161-1-will@kernel.org>
 <20240221113504.7161-2-will@kernel.org>
 <SN6PR02MB4157089980E6FC58D5557BCED4572@SN6PR02MB4157.namprd02.prod.outlook.com>
 <20240223124742.GB10641@willie-the-truck>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240223124742.GB10641@willie-the-truck>

On Fri, Feb 23, 2024 at 12:47:43PM +0000, Will Deacon wrote:
> > >  	/*
> > >  	 * For allocations of PAGE_SIZE or larger only look for page aligned
> > >  	 * allocations.
> > >  	 */
> > >  	if (alloc_size >= PAGE_SIZE)
> > > -		iotlb_align_mask |= ~PAGE_MASK;
> > > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > -
> > > -	/*
> > > -	 * For mappings with an alignment requirement don't bother looping to
> > > -	 * unaligned slots once we found an aligned one.
> > > -	 */
> > > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > > +		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> > 
> > Is this special handling of alloc_size >= PAGE_SIZE really needed?
> 
> I've been wondering that as well, but please note that this code (and the
> comment) are in the upstream code, so I was erring in favour of keeping
> that while fixing the bugs. We could have an extra patch dropping it if
> we can convince ourselves that it's not adding anything, though.

This special casing goes back to before git history.  It obviously is not
needed, but it might have made sense back then.  If people come up with
a good argument I'm totally fine with removing it, but I also think we
need to get the fixes here in ASAP, so things that are just cleanups
probably aren't priority right now.

> > While the iommu_dma_map_page() case can already fail due to
> > "too large" requests because of not setting a max mapping size,
> > this patch can cause smaller requests to fail as well until Patch 4
> > gets applied.  That might be problem to avoid, perhaps by
> > merging the Patch 4 changes into this patch.
> 
> I'll leave this up to Christoph. Personally, I'm keen to avoid having
> a giant patch trying to fix all the SWIOTLB allocation issues in one go,
> as it will inevitably get reverted due to a corner case that we weren't
> able to test properly, breaking the common cases at the same time.

Let's keep it split.