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

[Marek Szyprowski]

Hi Robin,

On 06.06.2022 14:38, Robin Murphy wrote:
> On 2022-06-02 16:58, Marek Szyprowski wrote:
>> 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.


Okay, I see your point. I'm fine with adding more of the IOVA allocation 
logic to the respective driver (s5p-mfc), however I would still like to 
use the dma-mapping framework and helpers, which depend on it (like 
v4l2-videobuf2, dma-buf, and so on). Would it be fine for you to let 
device drivers to access the IOVA domains hidden by the DMA-IOMMU glue 
code to mark certain parts of IOVA space as reserved or to manually 
remap the buffer (like firmware) with specific address requirements?


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland