Re: [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION)

[Andrew F. Davis]

On 3/19/19 11:54 AM, Benjamin Gaignard wrote:
> Le mer. 13 mars 2019 Ã 23:31, John Stultz <john.stultz@xxxxxxxxxx> a Ãcrit :
>>
>> On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@xxxxxxxxxxxxxx> wrote:
>>> On Tue, 5 Mar 2019, John Stultz wrote:
>>>>
>>>> Eventual TODOS:
>>>> * Reimplement page-pool for system heap (working on this)
>>>> * Add stats accounting to system/cma heaps
>>>> * Make the kselftest actually useful
>>>> * Add other heaps folks see as useful (would love to get
>>>>   some help from actual carveout/chunk users)!
>>>
>>> We use a modified carveout heap for certain secure use cases.
>>
>> Cool! It would be great to see if you have any concerns about adding
>> such a secure-carveout heap to this framework. I suspect it would be
>> fairly similar to how its integrated into ION, but particularly I'd be
>> interested in issues around the lack of private flags and other
>> allocation arguments like alignment.
>>
>>> Although there would probably be some benefit in discssing how the dma-buf
>>> heap framework may want to support
>>> secure heaps in the future it is a large topic which I assume you don't
>>> want to tackle now.
>>
>> So I suspect others (Benjamin?) would have a more informed opinion on
>> the details, but the intent is to allow secure heap implementations.
>> I'm not sure what areas of concern you have for this allocation
>> framework in particular?
> 
> yes I would be great to understand how you provide the information to
> tell that a dmabuf
> is secure (or not) since we can't add flag in dmabuf structure itself.
> An option is manage
> the access rights when a device attach itself to the dmabuf but in
> this case you need define
> a list of allowed devices per heap...
> If you have a good solution for secure heaps you are welcome :-)
> 

Do we really need any of that? A secure buffer is secured by the
hardware firewalls that keep out certain IP (including often the
processor running Linux). So the only thing we need to track internally
is that we should not allow mmap/kmap on the buffer. That can be done in
the per-heap layer, everything else stays the same as a standard
carveout heap.

Andrew

> Benjamin
>>
>>> We don't have any non-secure carveout heap use cases but the client use
>>> case I have seen usually revolve around
>>> wanting large allocations to succeed very quickly.
>>> For example I have seen camera use cases which do very large allocations
>>> on camera bootup from the carveout heap, these allocations would come from
>>> the carveout heap and fallback to the system heap when the carveout heap
>>> was full.
>>> Actual non-secure carveout heap can perhaps provide more detail.
>>
>> Yea, I'm aware that folks still see carveout as preferable to CMA due
>> to more consistent/predictable allocation latency.  I think we still
>> have the issue that we don't have bindings to establish/configure
>> carveout regions w/ dts, and I'm not really wanting to hold up the
>> allocation API on that issue.
>>
>>
>>> Since we are making some fundamental changes to how ION worked and since
>>> Android is likely also be the largest user of the dma-buf heaps framework
>>> I think it would be good
>>> to have a path to resolve the issues which are currently preventing
>>> commercial Android releases from moving to the upstream version of ION.
>>
>> Yea, I do see solving the cache management efficiency issues as
>> critical for the dmabuf heaps to be actually usable (my previous
>> version of this patchset accidentally had my hacks to improve
>> performance rolled in!).  And there are discussions going on in
>> various channels to try to figure out how to either change Android to
>> use dma-bufs more in line with how upstream expects, or what more
>> generic dma-buf changes we may need to allow Android to use dmabufs
>> with the expected performance they need.
>>
>>> I can understand if you don't necessarily want to put all/any of these
>>> changes into the dma-buf heaps framework as part of this series, but my
>>> hope is we can get
>>> the upstream community and the Android framework team to agree on what
>>> upstreamable changes to dma-buf heaps framework, and/or the Android
>>> framework, would be required in order for Android to move to the upstream
>>> dma-buf heaps framework for commercial devices.
>>
>> Yes. Though I also don't want to get the bigger dma-buf usage
>> discussion (which really affects all dmabuf exporters) too tied up
>> with this patch sets attempt to provide a usable allocation interface.
>> Part of the problem that I think we've seen with ION is that there is
>> a nest of of related issues, and the entire thing is just too big to
>> address at once, which I think is part of why ION has sat in staging
>> for so long. This patchset just tries to provide an dmabuf allocation
>> interface, and a few example exporter heap types.
>>
>>> I don't mean to make this specific to Android, but my assumption is that
>>> many of the ION/dma-buf heaps issues which affect Android would likely
>>> affect other new large users of the dma-buf heaps framework, so if we
>>> resolve it for Android we would be helping these future users as well.
>>> And I do understand that some the issues facing Android may need to be
>>> resolved by making changes to Android framework.
>>
>> While true, I also think some of the assumptions in how the dma-bufs
>> are used (pre-attachment of all devices, etc) are maybe not so
>> realistic given how Android is using them.  I do want to explore if
>> Android can change how they use dma-bufs, but I also worry that we
>> need to think about how we could loosen the expectations for dma-bufs,
>> as well as trying to figure out how to support things folks have
>> brought up like partial cache maintenance.
>>
>>> I think it would be helpful to try and get as much of this agreed upon as
>>> possible before the dma-buf heaps framework moves out of staging.
>>>
>>> As part of my review I will highlight some of the issues which would
>>> affect Android.
>>> In my comments I will apply them to the system heap since that is what
>>> Android currently uses for a lot of its use cases.
>>> I realize that this new framework provides more flexibility to heaps, so
>>> perhaps some of these issues can be solved by creating a new type of
>>> system heap which Android can use, but even if the solution involves
>>> creating a new system heap I would like to make sure that this "new"
>>> system heap is upstreamable.
>>
>> So yea, I do realize I'm dodging the hard problem here, but I think
>> the cache-management/usage issue is far more generic.
>>
>> You're right that this implementation give a lot of flexibility to the
>> exporter heaps in how they implement the dmabuf ops (just like how
>> other device drivers that are dmabuf exporters have the same
>> flexibility), but I very much agree we don't want to add a system and
>> then later a "system-android" heap. So yea, a reasonable amount of
>> caution is warranted here.
>>
>> Thanks so much for the review and feedback! I'll try to address things
>> as I can as I'm traveling this week (so I may be a bit spotty).
>>
>> thanks
>> -john