[PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Robin Murphy]

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.szyprowski at 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.