| Message ID | 1551819273-640-1-git-send-email-john.stultz@linaro.org |
|---|---|
| Headers | show |
| Series | DMA-BUF Heaps (destaging ION) | expand |
On Tue, 5 Mar 2019, John Stultz wrote: > Here is a initial RFC of the dma-buf heaps patchset Andrew and I > have been working on which tries to destage a fair chunk of ION > functionality. > > The patchset implements per-heap devices which can be opened > directly and then an ioctl is used to allocate a dmabuf from the > heap. > > The interface is similar, but much simpler then IONs, only > providing an ALLOC ioctl. > > Also, I've provided simple system and cma heaps. The system > heap in particular is missing the page-pool optimizations ION > had, but works well enough to validate the interface. > > I've booted and tested these patches with AOSP on the HiKey960 > using the kernel tree here: > https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap > > And the userspace changes here: > https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 > > > Compared to ION, this patchset is missing the system-contig, > carveout and chunk heaps, as I don't have a device that uses > those, so I'm unable to do much useful validation there. > Additionally we have no upstream users of chunk or carveout, > and the system-contig has been deprecated in the common/andoid-* > kernels, so this should be ok. > > I've also removed the stats accounting for now, since it should > be implemented by the heaps themselves. > > 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. 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. 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. > > That said, the main user-interface is shaping up and I wanted > to get some input on the device model (particularly from GreKH) > and any other API/ABI specific input. > > thanks > -john Thanks John and Andrew, it's great to see this effort to get this functionality out of staging. 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. 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. 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. 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. Liam Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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? > 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
On Wed, 13 Mar 2019, John Stultz wrote: > On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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. > We are actively working to drop our secure careveout heap in order to improve memory utilization so I don't think there would be a good case for upstreaming it. Our other secure heaps are CMA based and system heap based. Because people have had difficulty designing a generic secure heap which would satisfy enough of everybody's use cases to be upstreamable we have been looking into moving away from having local secure heap changes and instead have been looking at how to instead have a separate driver be responsible for making an ION buffer memory secure. The idea was to do this in order to remove a lot of our local ION changes, but if a secure heap was upstreamed that supported our secure use cases I am sure we would be interested in using that. The local change the ION API to support these heaps is the addition of all the VMID flags so that the client can specify where the client wants the memory assigned. > > 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? > I don't have any areas of concern, my thought was just that flushing out a potential design for an upstreamable secure heap would allow us catch early on if there is anything fundamental that would need to change dma-buf heaps framework (such as the allocation API). I don't think this is a necessary task at this point. Liam Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 3/5/19 12:54 PM, John Stultz wrote: > Here is a initial RFC of the dma-buf heaps patchset Andrew and I > have been working on which tries to destage a fair chunk of ION > functionality. > > The patchset implements per-heap devices which can be opened > directly and then an ioctl is used to allocate a dmabuf from the > heap. > > The interface is similar, but much simpler then IONs, only > providing an ALLOC ioctl. > > Also, I've provided simple system and cma heaps. The system > heap in particular is missing the page-pool optimizations ION > had, but works well enough to validate the interface. > > I've booted and tested these patches with AOSP on the HiKey960 > using the kernel tree here: > https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap > > And the userspace changes here: > https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 > > > Compared to ION, this patchset is missing the system-contig, > carveout and chunk heaps, as I don't have a device that uses > those, so I'm unable to do much useful validation there. > Additionally we have no upstream users of chunk or carveout, > and the system-contig has been deprecated in the common/andoid-* > kernels, so this should be ok. > > I've also removed the stats accounting for now, since it should > be implemented by the heaps themselves. > > 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)! > > That said, the main user-interface is shaping up and I wanted > to get some input on the device model (particularly from GreKH) > and any other API/ABI specific input. > > thanks > -john > > Cc: Laura Abbott <labbott@redhat.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Greg KH <gregkh@linuxfoundation.org> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Andrew F. Davis <afd@ti.com> > Cc: Chenbo Feng <fengc@google.com> > Cc: Alistair Strachan <astrachan@google.com> > Cc: dri-devel@lists.freedesktop.org > > Andrew F. Davis (1): > dma-buf: Add dma-buf heaps framework > > John Stultz (4): > dma-buf: heaps: Add heap helpers > dma-buf: heaps: Add system heap to dmabuf heaps > dma-buf: heaps: Add CMA heap to dmabuf heapss > kselftests: Add dma-heap test > > MAINTAINERS | 16 + > drivers/dma-buf/Kconfig | 10 + > drivers/dma-buf/Makefile | 2 + > drivers/dma-buf/dma-heap.c | 191 ++++++++++++ > drivers/dma-buf/heaps/Kconfig | 14 + > drivers/dma-buf/heaps/Makefile | 4 + > drivers/dma-buf/heaps/cma_heap.c | 164 ++++++++++ > drivers/dma-buf/heaps/heap-helpers.c | 335 +++++++++++++++++++++ > drivers/dma-buf/heaps/heap-helpers.h | 48 +++ > drivers/dma-buf/heaps/system_heap.c | 132 ++++++++ > include/linux/dma-heap.h | 65 ++++ > include/uapi/linux/dma-heap.h | 52 ++++ > tools/testing/selftests/dmabuf-heaps/Makefile | 11 + > tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 96 ++++++ > 14 files changed, 1140 insertions(+) > create mode 100644 drivers/dma-buf/dma-heap.c > create mode 100644 drivers/dma-buf/heaps/Kconfig > create mode 100644 drivers/dma-buf/heaps/Makefile > create mode 100644 drivers/dma-buf/heaps/cma_heap.c > create mode 100644 drivers/dma-buf/heaps/heap-helpers.c > create mode 100644 drivers/dma-buf/heaps/heap-helpers.h > create mode 100644 drivers/dma-buf/heaps/system_heap.c > create mode 100644 include/linux/dma-heap.h > create mode 100644 include/uapi/linux/dma-heap.h > create mode 100644 tools/testing/selftests/dmabuf-heaps/Makefile > create mode 100644 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c > This is looking really great. Thanks for doing the work to push this forward. It seems like we're in general agreement about much of this. Which of the issues that have come up do you think are a "hard no" to keeping this from going in? Thanks, Laura
On Tue, Mar 05, 2019 at 12:54:28PM -0800, John Stultz wrote: > Here is a initial RFC of the dma-buf heaps patchset Andrew and I > have been working on which tries to destage a fair chunk of ION > functionality. > > The patchset implements per-heap devices which can be opened > directly and then an ioctl is used to allocate a dmabuf from the > heap. > > The interface is similar, but much simpler then IONs, only > providing an ALLOC ioctl. > > Also, I've provided simple system and cma heaps. The system > heap in particular is missing the page-pool optimizations ION > had, but works well enough to validate the interface. > > I've booted and tested these patches with AOSP on the HiKey960 > using the kernel tree here: > https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap > > And the userspace changes here: > https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 What upstream driver will use this eventualy ? And why is it needed ? Cheers, Jérôme
Le mer. 13 mars 2019 à 23:31, John Stultz <john.stultz@linaro.org> a écrit : > > On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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 :-) 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
On Tue, Mar 19, 2019 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: > > On Tue, Mar 19, 2019 at 1:00 PM Andrew F. Davis <afd@ti.com> wrote: > > > > On 3/19/19 11:54 AM, Benjamin Gaignard wrote: > > > Le mer. 13 mars 2019 à 23:31, John Stultz <john.stultz@linaro.org> a écrit : > > >> > > >> On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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. > > For at least some hw the importing driver needs to configure things > differently for secure buffers :-/ Does the import ioctl need/use a flag for that then? Userland already has to keep meta-data about dmabufs around. thanks -john
Le mar. 19 mars 2019 à 23:36, John Stultz <john.stultz@linaro.org> a écrit : > > On Tue, Mar 19, 2019 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: > > > > On Tue, Mar 19, 2019 at 1:00 PM Andrew F. Davis <afd@ti.com> wrote: > > > > > > On 3/19/19 11:54 AM, Benjamin Gaignard wrote: > > > > Le mer. 13 mars 2019 à 23:31, John Stultz <john.stultz@linaro.org> a écrit : > > > >> > > > >> On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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. > > > > For at least some hw the importing driver needs to configure things > > differently for secure buffers :-/ > > Does the import ioctl need/use a flag for that then? Userland already > has to keep meta-data about dmabufs around. To secure a buffer you need to know who is allowed to write/read it and hardware block involved in the dataflow may need to know that the buffer is secure to configure themself. As example for a video decoding you allow hw video decoder to read in a buffer and display to read it. You can also allow cpu to write on the buffer to add subtitles. For that we need to be able to mmap/kmap the buffer. Using a carveout heap for secure buffer mean that you reserved a large memory region only for this purpose, that isn't possible on embedded device where we are always limited in memory so we use CMA. In the past I have used dmabuf's attach function to know who write into the buffer and then configure who will be able to read it. It was working well but the issue was how to in generic way this behavior. > > thanks > -john
On 3/20/19 4:16 AM, Benjamin Gaignard wrote: > Le mar. 19 mars 2019 à 23:36, John Stultz <john.stultz@linaro.org> a écrit : >> >> On Tue, Mar 19, 2019 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: >>> >>> On Tue, Mar 19, 2019 at 1:00 PM Andrew F. Davis <afd@ti.com> wrote: >>>> >>>> On 3/19/19 11:54 AM, Benjamin Gaignard wrote: >>>>> Le mer. 13 mars 2019 à 23:31, John Stultz <john.stultz@linaro.org> a écrit : >>>>>> >>>>>> On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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. >>> >>> For at least some hw the importing driver needs to configure things >>> differently for secure buffers :-/ >> >> Does the import ioctl need/use a flag for that then? Userland already >> has to keep meta-data about dmabufs around. > > To secure a buffer you need to know who is allowed to write/read it and > hardware block involved in the dataflow may need to know that the buffer > is secure to configure themself. > As example for a video decoding you allow hw video decoder to read in > a buffer and display to read it. You can also allow cpu to write on the buffer > to add subtitles. For that we need to be able to mmap/kmap the buffer. > Using a carveout heap for secure buffer mean that you reserved a large > memory region only for this purpose, that isn't possible on embedded device > where we are always limited in memory so we use CMA. > In the past I have used dmabuf's attach function to know who write into > the buffer and then configure who will be able to read it. It was working well > but the issue was how to in generic way this behavior. > Okay, I think I see what you are saying now. The way we handle secure playback is to firewall everything upfront and it is up to the application to inform the hardware about what it can and cannot do to the buffer, or simply not ask anything not allowed (E.g. writeback the decrypted stream) else it will get a firewall exception. The buffer itself doesn't have to carry any information. It sounds like you want the hardware driver to be able to detect the use-case based on the buffer itself and configure itself accordingly? Or the exporter at attach time to check access permissions? The first would need a change to DMA-BUF framework, maybe an added flag. The second would just need a heap exporter with the system wide smarts, but as you say that is not very generic.. Andrew >> >> thanks >> -john
Le mer. 20 mars 2019 à 15:54, Andrew F. Davis <afd@ti.com> a écrit : > > On 3/20/19 4:16 AM, Benjamin Gaignard wrote: > > Le mar. 19 mars 2019 à 23:36, John Stultz <john.stultz@linaro.org> a écrit : > >> > >> On Tue, Mar 19, 2019 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: > >>> > >>> On Tue, Mar 19, 2019 at 1:00 PM Andrew F. Davis <afd@ti.com> wrote: > >>>> > >>>> On 3/19/19 11:54 AM, Benjamin Gaignard wrote: > >>>>> Le mer. 13 mars 2019 à 23:31, John Stultz <john.stultz@linaro.org> a écrit : > >>>>>> > >>>>>> On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@codeaurora.org> 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. > >>> > >>> For at least some hw the importing driver needs to configure things > >>> differently for secure buffers :-/ > >> > >> Does the import ioctl need/use a flag for that then? Userland already > >> has to keep meta-data about dmabufs around. > > > > To secure a buffer you need to know who is allowed to write/read it and > > hardware block involved in the dataflow may need to know that the buffer > > is secure to configure themself. > > As example for a video decoding you allow hw video decoder to read in > > a buffer and display to read it. You can also allow cpu to write on the buffer > > to add subtitles. For that we need to be able to mmap/kmap the buffer. > > Using a carveout heap for secure buffer mean that you reserved a large > > memory region only for this purpose, that isn't possible on embedded device > > where we are always limited in memory so we use CMA. > > In the past I have used dmabuf's attach function to know who write into > > the buffer and then configure who will be able to read it. It was working well > > but the issue was how to in generic way this behavior. > > > > Okay, I think I see what you are saying now. > > The way we handle secure playback is to firewall everything upfront and > it is up to the application to inform the hardware about what it can and > cannot do to the buffer, or simply not ask anything not allowed (E.g. > writeback the decrypted stream) else it will get a firewall exception. > The buffer itself doesn't have to carry any information. > > It sounds like you want the hardware driver to be able to detect the > use-case based on the buffer itself and configure itself accordingly? Or > the exporter at attach time to check access permissions? Both are needed, the buffer client must know that it is a secure buffer and heap will have to configure the permissions. > > The first would need a change to DMA-BUF framework, maybe an added flag. Sumit will NACK that because dmabuf have to remain neutral and not embedded flags for every possible usage. > The second would just need a heap exporter with the system wide smarts, > but as you say that is not very generic.. yes it is difficult to find a good solution for that. > > Andrew > > >> > >> thanks > >> -john
On Wed, Mar 20, 2019 at 2:16 AM Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote: > Le mar. 19 mars 2019 à 23:36, John Stultz <john.stultz@linaro.org> a écrit : > > On Tue, Mar 19, 2019 at 2:58 PM Rob Clark <robdclark@gmail.com> wrote: > > > For at least some hw the importing driver needs to configure things > > > differently for secure buffers :-/ > > > > Does the import ioctl need/use a flag for that then? Userland already > > has to keep meta-data about dmabufs around. > > To secure a buffer you need to know who is allowed to write/read it and > hardware block involved in the dataflow may need to know that the buffer > is secure to configure themself. > As example for a video decoding you allow hw video decoder to read in > a buffer and display to read it. You can also allow cpu to write on the buffer > to add subtitles. For that we need to be able to mmap/kmap the buffer. > Using a carveout heap for secure buffer mean that you reserved a large > memory region only for this purpose, that isn't possible on embedded device > where we are always limited in memory so we use CMA. > In the past I have used dmabuf's attach function to know who write into > the buffer and then configure who will be able to read it. It was working well > but the issue was how to in generic way this behavior. Given the complexity of the configuration needed when allocating the buffer, instead of trying to make a generic secure buffer allocator, would having per-usege heaps make sense? It just feels there are so many specifics to the secure buffer setup and configuration that maybe there can't be a generic configuration interface. So instead maybe we let the heap implementations provide set usage configs? This doesn't necessarily require that you have separate pools of memory (they can share the same backing store), but by having multiple per-config heap devices, maybe this could avoid trying to fit all the options into one interface? On the import side, I'm not sure how much the importing device needs to know about the specific rules here (out side of "secure buffer" or not), so maybe that's another catch. thanks -john