On 2022-06-02 16:58, Marek Szyprowski wrote:
Hi Robin,
On 23.05.2022 19:30, Robin Murphy wrote:
On 2022-05-11 13:15, Ajay Kumar wrote:
From: Marek Szyprowski <m.szyprow...@samsung.com>
Zero is a valid DMA and IOVA address on many architectures, so adjust
the
IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
(~0UL) is introduced as a generic value for the error case. Adjust all
callers of the alloc_iova_fast() function for the new return value.
And when does anything actually need this? In fact if you were to stop
iommu-dma from reserving IOVA 0 - which you don't - it would only show
how patch #3 is broken.
I don't get why you says that patch #3 is broken.
My mistake, in fact it's already just as broken either way - I forgot
that that's done implicitly via iovad->start_pfn rather than an actual
reserve_iova() entry. I see there is an initial calculation attempting
to honour start_pfn in patch #3, but it's always immediately
overwritten. More critically, the rb_first()/rb_next walk means the
starting point can only creep inevitably upwards as older allocations
are freed, so will end up pathologically stuck at or above limit_pfn and
unable to recover. At best it's more "next-fit" than "first-fit", and
TBH the fact that it could ever even allocate anything at all in an
empty domain makes me want to change the definition of IOVA_ANCHOR ;)
Them main issue with
address 0 being reserved appears when one uses first fit allocation
algorithm. In such case the first allocation will be at the 0 address,
what causes some issues. This patch simply makes the whole iova related
code capable of such case. This mimics the similar code already used on
ARM 32bit. There are drivers that rely on the way the IOVAs are
allocated. This is especially important for the drivers for devices with
limited addressing capabilities. And there exists such and they can be
even behind the IOMMU.
Lets focus on the s5p-mfc driver and the way one of the supported
hardware does the DMA. The hardware has very limited DMA window (256M)
and addresses all memory buffers as an offset from the firmware base.
Currently it has been assumed that the first allocated buffer will be on
the beginning of the IOVA space. This worked fine on ARM 32bit and
supporting such case is a target of this patchset.
I still understand the use-case; I object to a solution where PFN 0 is
always reserved yet sometimes allocated. Not to mention the slightly
more fundamental thing of the whole lot not actually working the way
it's supposed to.
I also remain opposed to baking in secret ABIs where allocators are
expected to honour a particular undocumented behaviour. FWIW I've had
plans for a while now to make the IOVA walk bidirectional to make the
existing retry case more efficient, at which point it's then almost
trivial to implement "bottom-up" allocation, which I'm thinking I might
then force on by default for CONFIG_ARM to minimise any further
surprises for v2 of the default domain conversion. However I'm
increasingly less convinced that it's really sufficient for your case,
and am leaning back towards the opinion that any driver with really
specific constraints on how its DMA addresses are laid out should not be
passively relying on a generic allocator to do exactly what it wants. A
simple flag saying "try to allocate DMA addresses bottom-up" doesn't
actually mean "allocate this thing on a 256MB-aligned address and
everything subsequent within a 256MB window above it", so let's not
build a fragile house of cards on top of pretending that it does.
Also note that it's really nothing to do with architecture either way;
iommu-dma simply chooses to reserve IOVA 0 for its own convenience,
mostly because it can. Much the same way that 0 is typically a valid
CPU VA, but mapping something meaningful there is just asking for a
world of pain debugging NULL-dereference bugs.
Right that it is not directly related to the architecture, but it is
much more common case for some architectures where DMA (IOVA) address =
physical address + some offset. The commit message can be rephrased,
though.
I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also
uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation
algorithm is used no one has ever need to consider such case so far.
Because it's the most convenient and efficient thing to do. Remember we
tried to use 0 for DMA_MAPPING_ERROR too, but that turned out to be a
usable physical RAM address on some systems. With a virtual address
space, on the other hand, we're free to do whatever we want; that's kind
of the point.
Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
