From: "Petr Tesařík" <petr@tesarici.cz>
To: Will Deacon <will@kernel.org>
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 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Date: Mon, 29 Jan 2024 21:40:34 +0100 [thread overview]
Message-ID: <20240129214034.4beb4258@meshulam.tesarici.cz> (raw)
In-Reply-To: <20240129193250.GB12631@willie-the-truck>
On Mon, 29 Jan 2024 19:32:50 +0000
Will Deacon <will@kernel.org> wrote:
> On Fri, Jan 26, 2024 at 06:01:27PM +0100, Petr Tesařík wrote:
> > On Fri, 26 Jan 2024 15:19:55 +0000
> > Will Deacon <will@kernel.org> wrote:
> > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > > index b079a9a8e087..25febb9e670c 100644
> > > --- a/kernel/dma/swiotlb.c
> > > +++ b/kernel/dma/swiotlb.c
> > > @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > > phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> > > unsigned long max_slots = get_max_slots(boundary_mask);
> > > unsigned int iotlb_align_mask =
> > > - dma_get_min_align_mask(dev) | alloc_align_mask;
> > > + dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> >
> > Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
> > should be preserved in the bounce buffer address, ignoring the offset
> > within the TLB slot...
>
> Yup, this is basically restoring the old behaviour.
>
> > > unsigned int nslots = nr_slots(alloc_size), stride;
> > > unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > > unsigned int index, slots_checked, count = 0, i;
> > > @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > > * allocations.
> > > */
> > > if (alloc_size >= PAGE_SIZE)
> > > - iotlb_align_mask |= ~PAGE_MASK;
> > > - iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > + alloc_align_mask |= ~PAGE_MASK;
> >
> > ...and alloc_align_mask specifies the desired TLB slot alignment.
>
> Yes, although actually I'm now wondering whether there's another bug here
> in that we don't return naturally aligned buffers for allocations bigger
> than a page. I think that was broken in 0eee5ae10256 ("swiotlb: fix slot
> alignment checks") because that stopped aligning the initial search index
> to the stride (which was in turn previously aligned to the allocation size).
The question is whether there is any NEED that allocations bigger than
a page are naturally aligned. For my part, I don't see why there should
be, but I might be missing something.
> > > /*
> > > * 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 = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;
> >
> > I'm not quite sure about this one.
> >
> > And I'm not even sure all combinations make sense!
> >
> > For example, take these values:
> >
> > * TLB_SIZE == 0x800 (2K)
> > * alloc_align_mask == 0xffffffffffffc000 (16K alignment, could be page size)
> > * iotlb_align_mask == 0xffffffffffff0000 (64K alignment)
> > * orig_addr == 0x0000000000001234
> >
> > Only the lowest 16 bits are relevant for the alignment check.
> > Device alignment requires 0x1000.
> > Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
> > Obviously, such allocation must always fail...
>
> Having an iotlb_align_mask with all those upper bits set looks wrong to me.
> Is that the same "braino" as bbb73a103fbb?
I must always stop and think at least twice before I can be sure
whether a "mask" has the high bits set, or the low bits set...
On an x86, PAGE_SHIFT is 12, PAGE_SIZE is 1UL << PAGE_SHIFT or 0x1000,
PAGE_MASK is ~(PAGE_SIZE-1)) or 0xfffffffffffff000, and there's one
more bitwise negation, so you're right. Both masks above should be
inverted, and using max() to find the stride is correct.
Petr T
next prev parent reply other threads:[~2024-01-29 20:40 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 15:19 [PATCH 0/2] Fix double allocation in swiotlb_alloc() Will Deacon
2024-01-26 15:19 ` [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots Will Deacon
2024-01-26 17:01 ` Petr Tesařík
2024-01-29 19:32 ` Will Deacon
2024-01-29 20:40 ` Petr Tesařík [this message]
2024-01-29 22:05 ` Will Deacon
2024-01-26 15:19 ` [PATCH 2/2] swiotlb: Enforce page alignment in swiotlb_alloc() Will Deacon
2024-01-26 16:23 ` Petr Tesařík
2024-01-29 19:35 ` Will Deacon
2024-01-29 6:08 ` Christoph Hellwig
2024-01-29 7:43 ` Petr Tesařík
2024-01-29 7:50 ` Christoph Hellwig
2024-01-29 19:49 ` Will Deacon
2024-01-30 14:34 ` Christoph Hellwig
2024-01-26 16:20 ` [PATCH 0/2] Fix double allocation " Petr Tesařík
2024-01-29 18:42 ` Will Deacon
2024-01-29 19:26 ` Petr Tesařík
2024-01-29 19:33 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240129214034.4beb4258@meshulam.tesarici.cz \
--to=petr@tesarici.cz \
--cc=decui@microsoft.com \
--cc=hch@lst.de \
--cc=iommu@lists.linux.dev \
--cc=kernel-team@android.com \
--cc=linux-kernel@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=petr.tesarik1@huawei-partners.com \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).