Received-SPF: pass (google.com: domain of linux-kernel+bounces-43414-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Jan 2024 19:33:36 +0000
From: Will Deacon <will@kernel.org>
To: Petr =?utf-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com,
	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 0/2] Fix double allocation in swiotlb_alloc()
Message-ID: <20240129193335.GC12631@willie-the-truck>
References: <20240126151956.10014-1-will@kernel.org>
 <20240126172059.48ad7b9f@meshulam.tesarici.cz>
 <20240129184255.GA12631@willie-the-truck>
 <20240129202619.69178dea@meshulam.tesarici.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240129202619.69178dea@meshulam.tesarici.cz>
User-Agent: Mutt/1.10.1 (2018-07-13)

On Mon, Jan 29, 2024 at 08:26:19PM +0100, Petr Tesařík wrote:
> On Mon, 29 Jan 2024 18:42:55 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > On Fri, Jan 26, 2024 at 05:20:59PM +0100, Petr Tesařík wrote:
> > > On Fri, 26 Jan 2024 15:19:54 +0000
> > > Will Deacon <will@kernel.org> wrote:
> > >   
> > > > Hi folks,
> > > > 
> > > > These two patches fix a nasty double allocation problem in swiotlb_alloc()
> > > > and add a diagnostic to help catch any similar issues in future. This was
> > > > a royal pain to track down and I've had to make a bit of a leap at the
> > > > correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).  
> > > 
> > > Welcome to the club. I believe you had to re-discover what I described here:
> > > 
> > >   https://lore.kernel.org/linux-iommu/20231108101347.77cab795@meshulam.tesarici.cz/  
> > 
> > Lucky me...
> > 
> > > The relevant part would be this:
> > > 
> > >   To sum it up, there are two types of alignment:
> > > 
> > >   1. specified by a device's min_align_mask; this says how many low
> > >      bits of a buffer's physical address must be preserved,
> > > 
> > >   2. specified by allocation size and/or the alignment parameter;
> > >      this says how many low bits in the first IO TLB slot's physical
> > >      address must be zero.  
> > > 
> > > Fix for that has been sitting on my TODO list for too long. :-(  
> > 
> > FWIW, it did _used_ to work (or appear to work), so it would be good to
> > at least get it back to the old behaviour if nothing else.
> 
> Yes, now that I look at the code, it was probably misunderstanding on
> my side as to how the three different alignment requirements are
> supposed to work together.
> 
> AFAICT your patch addresses everything that has ever worked. The rest
> needs some more thought, and before I touch this loop again, I'll write
> a proper test case.

Thanks, that would be much appreciated!

Will