Received-SPF: pass (google.com: domain of linux-kernel+bounces-48153-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 1 Feb 2024 12:46:34 +0000
From: Will Deacon <will@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
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>,
	Petr Tesarik <petr.tesarik1@huawei-partners.com>,
	Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement
 when searching slots
Message-ID: <20240201124634.GA15707@willie-the-truck>
References: <20240131122543.14791-1-will@kernel.org>
 <20240131122543.14791-2-will@kernel.org>
 <4c9f50d2-05f9-4a37-ac50-dcd98e40e87f@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4c9f50d2-05f9-4a37-ac50-dcd98e40e87f@arm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)

Hey Robin,

Cheers for having a look.

On Wed, Jan 31, 2024 at 03:54:03PM +0000, Robin Murphy wrote:
> On 31/01/2024 12:25 pm, Will Deacon wrote:
> > Commit bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix"),
> > which was a fix for commit 0eee5ae10256 ("swiotlb: fix slot alignment
> > checks"), causes a functional regression with vsock in a virtual machine
> > using bouncing via a restricted DMA SWIOTLB pool.
> > 
> > When virtio allocates the virtqueues for the vsock device using
> > dma_alloc_coherent(), the SWIOTLB search fails to take into account the
> > 8KiB buffer size and returns page-unaligned allocations if 'area->index'
> > was left unaligned by a previous allocation from the buffer:
> 
> Hmm, but isn't this fundamentally swiotlb_alloc()'s fault for assuming it's
> going to get a page-aligned address back despite asking for 0 alignment in
> the first place? I'm not sure SWIOTLB has ever promised implicit
> size-alignment, so it feels somewhat misplaced to be messing with the
> algorithm before fixing the obvious issue in the caller :/

It's hard to tell which guarantees are intentional here given that this
interface is all internal to swiotlb.c, but the 'alloc_align_mask'
parameter didn't even exist prior to e81e99bacc9f ("swiotlb: Support
aligned swiotlb buffers") and practically the implementation has ensured
page-aligned allocations for buffers >= PAGE_SIZE prior to 0eee5ae10256
("swiotlb: fix slot alignment checks") by virtue of aligning the search
index to the stride.

In any case, this patch is required because the current state of
swiotlb_search_pool_area() conflates the DMA alignment mask, the
allocation alignment mask and the stride so that even if a non-zero
'alloc_align_mask' is passed in, it won't necessarily be honoured.

For example, I just gave it a spin with only patch #3 and then this log:

> >   # Final address in brackets is the SWIOTLB address returned to the caller
> >   | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1645-1649/7168 (0x98326800)

Becomes:

  | virtio-pci 0000:00:07.0: alloc_size 0x2000, iotlb_align_mask 0x1800 stride 0x4: got slot 1645-1649/7168 (0x98326800)

So even though the stride is correct, we still end up with a 2KiB aligned
allocation.

Cheers,

Will