RE: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Nicolas, all Nicolas, please see below in the thread for your concerns regarding video decoding and hdmi receivers with secure memory. But i can also propose to setup calls to discuss about that. We are almost done with a proposal to add support of Linaro secure dmabuff heaps in V4L2. Today It is functional with Linux 5.15 + i.MX8MQ evk board, I mean we can playback video using a secure dmabuf heap memory Linux can't read/write, but we still need some time to clean the code and have something ready for a code review. So maybe 2 different calls could happen: 1) A first one to define a generic mechanism for Linux to manage (allocate/free) secure memory. By secure memory i mean a memory Linux can't read and write with the cpu running in non-secure mode. - Linaro secure dmabuf heaps seems to be a reasonable approach and is available - Secure OS in charge of the hardware management to protect the secure memory, without any action to be done from Linux side, I mean when Linux kernel starts, a secure dmabuf heap is already protected ,seems a reasonable approach - then Linux Kernel needs to read device tree to know such secure heaps exist and will expose them to user space world. - For memory isolation/sandboxing use cases, we may need different secure heaps. For example, one secure heap, the video decoder is allowed to access (Secure Video Path like applications),and second secure heap Video heap decoder is not allowed to access (secure payment like applications), but this is under responsibility of secure OS to configure such memory security rules before Linux kernel starts. - Using current Linux CMA, and ask dynamically the secure OS to allocate and secure memory from existing CMA heap doesn't seem to be a right approach to me. I think we should be more or less all aligned with that. Why is it not the right approach: - when you release secure memory, you need to "memset" it to 0, because this memory can potentially be reuse by non-secure world. It can take a long time with 4K video buffers and during that time, the Linux process calling the secure OS is stuck. Not good for real time or smooth video playback. - we need direct interaction between Linux and Secure OS for each allocate/free of secure memory: - different secure OS means different API to be called from Linux. - it takes time for the CPU to switch from non-secure Linux-> secure OS -> non-secure Linux. From what I have in mind, something like 1ms with a arm a53 cpu running at 1.5 Ghz, so max 1000 alloc/free per seconds, and again during that time, the Linux process calling the secure OS is stuck, waiting secure OS. - Linux and secure OS shall be in sync regarding memory allocation in CMA. Seems a very complex mechanism to maintain. -> It is why we need dedicated dma buf heaps for secure memory, and why Linaro secure dmabuf heap support is needed in Linux Kernel. 2) A second call to discuss V4L2 using Linaro secure dmabuff heap, when we will be ready for the code review. Please let me know if you agree with this proposal to setup 2 different calls, as they are 2 different topics to be addressed. Regards. -Original Message- From: Nicolas Dufresne Sent: Friday, August 19, 2022 5:14 PM To: Cyrille Fleury ; Olivier Masse ; brian.star...@arm.com Cc: sumit.sem...@linaro.org; linux-ker...@vger.kernel.org; linaro-mm-...@lists.linaro.org; christian.koe...@amd.com; linux-me...@vger.kernel.org; n...@arm.com; Clément Faure ; dri-devel@lists.freedesktop.org; benjamin.gaign...@collabora.com Subject: Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support Caution: EXT Email Hi, thanks for the additional information, we are starting to have a (still partial) overview of your team goals. Le jeudi 18 août 2022 à 05:25 +, Cyrille Fleury a écrit : > Hi Nicolas, all, > > The short reply: > - For DRM, gstreamer, ffmeg, ... we don't use anymore NXP VPU > proprietary API > - We need secure dma-buf heaps to replace secure ion heaps > > The more detailed reply to address concerns below in the thread: > - NXP doesn't design VPU, but rely on third party VPU hardware > IP we integrate in our soc. NXP proprietary API are for legacy > applications our customers did without using gstreamer or ffmpeg, but > we are now relying on > V4L2 API for WPE/gstreamer, chromium/ffmpeg ... > - Even with NXP legacy BSP, there was no API impact for WPE (or > chromium) due to NXP VPU API. We use WPE/gstreamer, then a gstreamer > pluging relying on NXP VPU proprietary API. But now we use V4L2. So we > can forget NXP VPU proprietary API, and I'm very happy with that
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
o one register bank, to setup > dedicated security rules for DRM. What you wrote here is about as much as I heard about the new security model coming in newer chips (this is not NXP specific). I think in order to push forward designs and APIs, it would be logical to first present about these mechanism, now they work and how they affect drivers and user space. Its not clear how this mechanism inforces usage of non-mappable to kernel mmu memory. Providing Open Source kernel and userland to demonstrate and use this feature is also very helpful for reviewers and adopters, but also a requirement in the drm tree. regards, Nicolas > > I'm on vacation until end of this week. I can setup a call next week to > discuss this topic if more clarifications are needed. > > Regards. > > -Original Message- > From: Olivier Masse > Sent: Wednesday, August 17, 2022 4:52 PM > To: nico...@ndufresne.ca; Cyrille Fleury ; > brian.star...@arm.com > Cc: sumit.sem...@linaro.org; linux-ker...@vger.kernel.org; > linaro-mm-...@lists.linaro.org; christian.koe...@amd.com; > linux-me...@vger.kernel.org; n...@arm.com; Clément Faure > ; dri-devel@lists.freedesktop.org; > benjamin.gaign...@collabora.com > Subject: Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf > heap support > > +Cyrille > > Hi Nicolas, > > On mer., 2022-08-17 at 10:29 -0400, Nicolas Dufresne wrote: > > Caution: EXT Email > > > > Hi Folks, > > > > Le mardi 16 août 2022 à 11:20 +, Olivier Masse a écrit : > > > Hi Brian, > > > > > > > > > On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote: > > > > Caution: EXT Ema > > > > > > > > [...] > > > > > > > > > > Interesting, that's not how the devices I've worked on operated. > > > > > > > > Are you saying that you have to have a display controller driver > > > > running in the TEE to display one of these buffers? > > > > > > In fact the display controller is managing 3 plans : UI, PiP and > > > video. The video plan is protected in secure as you can see on slide > > > 11: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fsan19%2Fpresentations%2Fsan19-107.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7Ce0e00be789a54dff8e5208da805ce2f6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637963433695707516%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GHjEfbgqRkfHK16oyNaYJob4LRVqvoffRElKR%2F7Rtes%3D&reserved=0 > > > > > > > > just wanted to highlight that all the WPE/GStreamer bit in this > > presentation is based on NXP Vendor Media CODEC design, which rely on > > their own i.MX VPU API. I don't see any effort to extend this to a > > wider audience. It is not explaining how this can work with a mainline > > kernel with v4l2 stateful or stateless drivers and generic > > GStreamer/FFMPEG/Chromium support. > > Maybe Cyrille can explain what it is currently done at NXP level regarding > the integration of v4l2 with NXP VPU. > > > > > I'm raising this, since I'm worried that no one cares of solving that > > high level problem from a generic point of view. In that context, any > > additions to the mainline Linux kernel can only be flawed and will > > only serves specific vendors and not the larger audience. > > > > Another aspect, is that this design might be bound to a specific (NXP > > ?) > > security design. I've learn recently that newer HW is going to use > > multiple level of MMU (like virtual machines do) to protect the memory > > rather then marking pages. Will all this work for that too ? > > our fire-walling hardware is protecting memory behind the MMU and so rely on > physical memory layout. > this work is only relying on a reserved physical memory. > > Regards, > Olivier > > > > > regards, > > Nicolas
RE: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Nicolas, all, The short reply: - For DRM, gstreamer, ffmeg, ... we don't use anymore NXP VPU proprietary API - We need secure dma-buf heaps to replace secure ion heaps The more detailed reply to address concerns below in the thread: - NXP doesn't design VPU, but rely on third party VPU hardware IP we integrate in our soc. NXP proprietary API are for legacy applications our customers did without using gstreamer or ffmpeg, but we are now relying on V4L2 API for WPE/gstreamer, chromium/ffmpeg ... - Even with NXP legacy BSP, there was no API impact for WPE (or chromium) due to NXP VPU API. We use WPE/gstreamer, then a gstreamer pluging relying on NXP VPU proprietary API. But now we use V4L2. So we can forget NXP VPU proprietary API, and I'm very happy with that. - We have moved from ion buffer to dma buff to manage secure memory management. This is why we need secure dma-buf heaps, we protect with NXP hardware as we did with ion heaps in the presentation Olivier shared. - For secure video playback, the changes we need to do are in user space world (gstreamer, WPE, ...), to update our patches managing secure ion heaps by secure dma-buf heaps. But dma-buf is file descriptor based as ion heap are. - What will change between platforms, is how memory is protected. This is why we requested to have dtb in OPTEE for secure memory, to be able to provide a common API to secure DDR memory, and then to rely on proprietary code in OPTEE to secure it. - We don't have a display controller or VPU decoder running in TEE. They remain under the full control of Linux/REE Word. If Linux/REE ask something breaking Widevine/PlayReady security rules, for example decode secure memory to non-secure memory, read from secure memory will return 0, write to secure memory will be ignored. Same with keys, certificates ... - i.MX8 socs have a stateless VPU and there is no VPU firmware. i.MX9 socs have a stateful VPU with firmware. In secure memory context, with secure memory, at software level, stateful VPU are even more simple to manage -> less read/write operations performed by Linux world to parse the stream, so less patch to be done in the video framework. But for memory management, stateful/stateless, same concern: we need to provide support of secure dma heaps to Linux, to allocate secure memory for the VPU and the display controller, so it is just a different dma-buf heaps, so a different file descriptor. - i.MX9 VPU will support "Virtual Machine VPU". Till now I don't see why it will not work. I'm not an expert in VM, but from what I understood from my discussions with NXP VPU team integrating the new VPU hardware IP, from outside world, VPU is seen as multiple VPUs, with multiple register banks. So virtualized OS will continue to read/write registers as today, and at software level, secure memory is as non-secure memory, I mean at this end, it is physical DDR memory. Of course hardware shall be able to read/write it, but this is not software related, this is hardware concern. And even without VM, we target to dedicate one virtual VPU to DRM, so one register bank, to setup dedicated security rules for DRM. I'm on vacation until end of this week. I can setup a call next week to discuss this topic if more clarifications are needed. Regards. -Original Message- From: Olivier Masse Sent: Wednesday, August 17, 2022 4:52 PM To: nico...@ndufresne.ca; Cyrille Fleury ; brian.star...@arm.com Cc: sumit.sem...@linaro.org; linux-ker...@vger.kernel.org; linaro-mm-...@lists.linaro.org; christian.koe...@amd.com; linux-me...@vger.kernel.org; n...@arm.com; Clément Faure ; dri-devel@lists.freedesktop.org; benjamin.gaign...@collabora.com Subject: Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support +Cyrille Hi Nicolas, On mer., 2022-08-17 at 10:29 -0400, Nicolas Dufresne wrote: > Caution: EXT Email > > Hi Folks, > > Le mardi 16 août 2022 à 11:20 +, Olivier Masse a écrit : > > Hi Brian, > > > > > > On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote: > > > Caution: EXT Ema > > > > > [...] > > > > > > > Interesting, that's not how the devices I've worked on operated. > > > > > > Are you saying that you have to have a display controller driver > > > running in the TEE to display one of these buffers? > > > > In fact the display controller is managing 3 plans : UI, PiP and > > video. The video plan is protected in secure as you can see on slide > > 11: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fsan19%2Fpresentations%2Fsan19-107.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7Ce0e00b
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Am Mittwoch, dem 17.08.2022 um 10:29 -0400 schrieb Nicolas Dufresne: > Hi Folks, > > Le mardi 16 août 2022 à 11:20 +, Olivier Masse a écrit : > > Hi Brian, > > > > > > On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote: > > > Caution: EXT Ema > > > > > [...] > > > > > > > Interesting, that's not how the devices I've worked on operated. > > > > > > Are you saying that you have to have a display controller driver > > > running in the TEE to display one of these buffers? > > > > In fact the display controller is managing 3 plans : UI, PiP and > > video. The video plan is protected in secure as you can see on slide > > 11: > > https://static.linaro.org/connect/san19/presentations/san19-107.pdf > > > > just wanted to highlight that all the WPE/GStreamer bit in this presentation > is > based on NXP Vendor Media CODEC design, which rely on their own i.MX VPU API. > I > don't see any effort to extend this to a wider audience. It is not explaining > how this can work with a mainline kernel with v4l2 stateful or stateless > drivers > and generic GStreamer/FFMPEG/Chromium support. > > I'm raising this, since I'm worried that no one cares of solving that high > level > problem from a generic point of view. In that context, any additions to the > mainline Linux kernel can only be flawed and will only serves specific vendors > and not the larger audience. > > Another aspect, is that this design might be bound to a specific (NXP ?) > security design. I've learn recently that newer HW is going to use multiple > level of MMU (like virtual machines do) to protect the memory rather then > marking pages. Will all this work for that too ? > I have not looked in any of this for quite a while, but IIRC the plan was something like that: The NXP RDC hardware is able to segment the DDR memory into sections and define access policies for all masters in the system. So for example for the secure VPU to display controller path you would define such a section, where only the VPU is able to write and DCSS is able to read from. CPU or other masters are not allowed to use this section. This then gets exposed to Linux as a DMA heap. The VPU driver could then allocate capture buffers from this heap and share them via dma-buf to the DCSS driver. Both drivers can live in non-trusted userspace and even the address allocation for the DMA heap can be done from Linux. Non-trusted Linux kernel/userspace just has no way to access the buffers directly. The more interesting question is on the VPU side: how do you make sure that the capture buffer is located in secure memory when the output buffer containing the secret bitstream is also in a secure heap? I guess you need some kind of TEE application to validate those settings, which means you can't give the non-trusted driver direct MMIO access to the VPU. Regards, Lucas
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
+Cyrille Hi Nicolas, On mer., 2022-08-17 at 10:29 -0400, Nicolas Dufresne wrote: > Caution: EXT Email > > Hi Folks, > > Le mardi 16 août 2022 à 11:20 +, Olivier Masse a écrit : > > Hi Brian, > > > > > > On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote: > > > Caution: EXT Ema > > > > > [...] > > > > > > > Interesting, that's not how the devices I've worked on operated. > > > > > > Are you saying that you have to have a display controller driver > > > running in the TEE to display one of these buffers? > > > > In fact the display controller is managing 3 plans : UI, PiP and > > video. The video plan is protected in secure as you can see on > > slide > > 11: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fsan19%2Fpresentations%2Fsan19-107.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7Ce0e00be789a54dff8e5208da805ce2f6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637963433695707516%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GHjEfbgqRkfHK16oyNaYJob4LRVqvoffRElKR%2F7Rtes%3D&reserved=0 > > > > just wanted to highlight that all the WPE/GStreamer bit in this > presentation is > based on NXP Vendor Media CODEC design, which rely on their own i.MX > VPU API. I > don't see any effort to extend this to a wider audience. It is not > explaining > how this can work with a mainline kernel with v4l2 stateful or > stateless drivers > and generic GStreamer/FFMPEG/Chromium support. Maybe Cyrille can explain what it is currently done at NXP level regarding the integration of v4l2 with NXP VPU. > > I'm raising this, since I'm worried that no one cares of solving that > high level > problem from a generic point of view. In that context, any additions > to the > mainline Linux kernel can only be flawed and will only serves > specific vendors > and not the larger audience. > > Another aspect, is that this design might be bound to a specific (NXP > ?) > security design. I've learn recently that newer HW is going to use > multiple > level of MMU (like virtual machines do) to protect the memory rather > then > marking pages. Will all this work for that too ? our fire-walling hardware is protecting memory behind the MMU and so rely on physical memory layout. this work is only relying on a reserved physical memory. Regards, Olivier > > regards, > Nicolas
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Folks, Le mardi 16 août 2022 à 11:20 +, Olivier Masse a écrit : > Hi Brian, > > > On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote: > > Caution: EXT Ema > > [...] > > > > Interesting, that's not how the devices I've worked on operated. > > > > Are you saying that you have to have a display controller driver > > running in the TEE to display one of these buffers? > > In fact the display controller is managing 3 plans : UI, PiP and > video. The video plan is protected in secure as you can see on slide > 11: > https://static.linaro.org/connect/san19/presentations/san19-107.pdf just wanted to highlight that all the WPE/GStreamer bit in this presentation is based on NXP Vendor Media CODEC design, which rely on their own i.MX VPU API. I don't see any effort to extend this to a wider audience. It is not explaining how this can work with a mainline kernel with v4l2 stateful or stateless drivers and generic GStreamer/FFMPEG/Chromium support. I'm raising this, since I'm worried that no one cares of solving that high level problem from a generic point of view. In that context, any additions to the mainline Linux kernel can only be flawed and will only serves specific vendors and not the larger audience. Another aspect, is that this design might be bound to a specific (NXP ?) security design. I've learn recently that newer HW is going to use multiple level of MMU (like virtual machines do) to protect the memory rather then marking pages. Will all this work for that too ? regards, Nicolas
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
On Tue, Aug 16, 2022 at 11:20:50AM +, Olivier Masse wrote:
> On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote:
> > On Mon, Aug 08, 2022 at 02:39:53PM +, Olivier Masse wrote:
> > > On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> > > > On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
.. snip
> > > > > +
> > > > > +static struct sg_table *secure_heap_map_dma_buf(struct
> > > > > dma_buf_attachment *attachment,
> > > > > + enum
> > > > > dma_data_direction direction)
> > > > > +{
> > > > > + struct secure_heap_attachment *a = attachment->priv;
> > > > > +
> > > > > + return a->table;
> > > >
> > > > I think you still need to implement mapping and unmapping using
> > > > the
> > > > DMA APIs. For example devices might be behind IOMMUs and the
> > > > buffer
> > > > will need mapping into the IOMMU.
> > >
> > > Devices that will need access to the buffer must be in secure.
> > > The tee driver will only need the scatter-list table to get dma
> > > address
> > > and len. Mapping will be done in the TEE.
> > > Please find tee_shm_register_fd in the following commit
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux%2Fcommit%2F41e21e5c405530590dc2dd10b2a8dbe64589840f&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OKZhaNevD5dj7Wjm6zbZlij0mPA9XYyio1NAN3VjTVM%3D&reserved=0
> > >
> > > This patch need to be up-streamed as well.
> > >
> >
> > Interesting, that's not how the devices I've worked on operated.
> >
> > Are you saying that you have to have a display controller driver
> > running in the TEE to display one of these buffers?
>
> In fact the display controller is managing 3 plans : UI, PiP and
> video. The video plan is protected in secure as you can see on slide
> 11:
> https://static.linaro.org/connect/san19/presentations/san19-107.pdf
>
> The DCSS (display controller) is able to read from the protected secure
> heap and composition result is send directly to the HDMI/HDCP port.
But it sounds like the DCSS driver is running in non-trusted Linux?
>
>
> > If everything
> > needs to be in the TEE, then why even have these buffers allocated
> > by non-secure Linux at all?
>
> The TEE is only doing decryption using the HW Crypto Accelerator
> (CAAM).
> The CAAM will read from a non protected encrypted buffer to write clear
> content to a secure buffer allocated with DMABUF and mapped in secure
> by OPTEE OS.
>
> >
> > I would have expected there to be HW enforcement of buffer access,
> > but for the display driver to be in non-secure Linux. That's how
> > TZMP1 works:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fhkg18%2Fpresentations%2Fhkg18-408.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XVpI93dXYu%2BGswLE8dcYboq%2FAWzSJn9j9LMlngpr238%3D&reserved=0
> >
> > Looking at this SDP presentation, that also seems to be the case
> > there:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fsan19%2Fpresentations%2Fsan19-107.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5Ec61NC1f0UQU%2F3BEURZQhEBrZ%2FuvJ1vaoSN4ChMn%2Bw%3D&reserved=0
> >
>
> Indeed, TZMP1 is similar to our implementation.
>
> > Based on those presentations, I think this heap should be
> > implementing
> > map_dma_buf in the "normal" way, using the DMA API to map the buffer
> > to the device. It's up to the TEE and HW firewall to prevent access
> > to those mappings from non-secure devices.
>
> In fact, our devices (VPU and DCSS) do not need any mapping, but only
> the physical address of buffers which need to be contiguous.
That's not how dma-buf or the DMA APIs work though - you should use
dma_map_sgtable and let the DMA API take care of whether a mapping
is needed or not.
> The VPU decoder, run by the CPU, read video meta data from a non
> protected buffer and send physical memory address of encoded buffer to
> the VPU HW.
> As well, the DCSS get physical address of contiguous decoded video
> buffer to do the composition.
>
Can you share the DCSS side of this? Maybe that will help to clear it
up.
Thanks,
-Brian
> >
> > My understanding is:
> >
> > * The memory region should never be mapped or accessed from the host
> > CPU. This is not a security requirement - the
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Brian,
On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote:
> Caution: EXT Email
>
> Hi,
>
> On Mon, Aug 08, 2022 at 02:39:53PM +, Olivier Masse wrote:
> > Hi Brian,
> >
> > On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> > > Caution: EXT Email
> > >
> > > Hi Olivier,
> > >
> > > Thanks, I think this is looking much better.
> > >
> > > I'd like to know how others feel about landing this heap; there's
> > > been
> > > push-back in the past about heaps in device-tree and discussions
> > > around how "custom" heaps should be treated (though IMO this is
> > > quite
> > > a generic one).
> > >
> > > On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
> > > > add Linaro secure heap bindings: linaro,secure-heap
> > > > use genalloc to allocate/free buffer from buffer pool.
> > > > buffer pool info is from dts.
> > > > use sg_table instore the allocated memory info, the length of
> > > > sg_table is 1.
> > > > implement secure_heap_buf_ops to implement buffer share in
> > > > difference device:
> > > > 1. Userspace passes this fd to all drivers it wants this buffer
> > > > to share with: First the filedescriptor is converted to a
> > > > &dma_buf
> > > > using
> > > > dma_buf_get(). Then the buffer is attached to the device using
> > > > dma_buf_attach().
> > > > 2. Once the buffer is attached to all devices userspace can
> > > > initiate DMA
> > > > access to the shared buffer. In the kernel this is done by
> > > > calling
> > > > dma_buf_map_attachment()
> > > > 3. get sg_table with dma_buf_map_attachment in difference
> > > > device.
> > > >
> > >
> > > I think this commit message could use a little rework. A few
> > > thoughts:
> > >
> > > * The bindings are in a separate commit, so seems strange to
> > > mention
> > > here.
> >
> > what about:
> > "add Linaro secure heap compatible reserved memory: linaro,secure-
> > heap"
> >
>
> I'd say something like:
>
> Add a dma-buf heap to allocate secure buffers from a reserved-memory
> region.
>
> ..snip
ok right.
>
> > > > +
> > > > +static struct sg_table *secure_heap_map_dma_buf(struct
> > > > dma_buf_attachment *attachment,
> > > > + enum
> > > > dma_data_direction direction)
> > > > +{
> > > > + struct secure_heap_attachment *a = attachment->priv;
> > > > +
> > > > + return a->table;
> > >
> > > I think you still need to implement mapping and unmapping using
> > > the
> > > DMA APIs. For example devices might be behind IOMMUs and the
> > > buffer
> > > will need mapping into the IOMMU.
> >
> > Devices that will need access to the buffer must be in secure.
> > The tee driver will only need the scatter-list table to get dma
> > address
> > and len. Mapping will be done in the TEE.
> > Please find tee_shm_register_fd in the following commit
> >
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux%2Fcommit%2F41e21e5c405530590dc2dd10b2a8dbe64589840f&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OKZhaNevD5dj7Wjm6zbZlij0mPA9XYyio1NAN3VjTVM%3D&reserved=0
> >
> > This patch need to be up-streamed as well.
> >
>
> Interesting, that's not how the devices I've worked on operated.
>
> Are you saying that you have to have a display controller driver
> running in the TEE to display one of these buffers?
In fact the display controller is managing 3 plans : UI, PiP and
video. The video plan is protected in secure as you can see on slide
11:
https://static.linaro.org/connect/san19/presentations/san19-107.pdf
The DCSS (display controller) is able to read from the protected secure
heap and composition result is send directly to the HDMI/HDCP port.
> If everything
> needs to be in the TEE, then why even have these buffers allocated
> by non-secure Linux at all?
The TEE is only doing decryption using the HW Crypto Accelerator
(CAAM).
The CAAM will read from a non protected encrypted buffer to write clear
content to a secure buffer allocated with DMABUF and mapped in secure
by OPTEE OS.
>
> I would have expected there to be HW enforcement of buffer access,
> but for the display driver to be in non-secure Linux. That's how
> TZMP1 works:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fhkg18%2Fpresentations%2Fhkg18-408.pdf&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XVpI93dXYu%2BGswLE8dcYboq%2FAWzSJn9j9LMlngpr238%3D&reserved=0
>
> Looking at this SDP presentation, that also seems to be the case
> there:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi,
On Mon, Aug 08, 2022 at 02:39:53PM +, Olivier Masse wrote:
> Hi Brian,
>
> On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> > Caution: EXT Email
> >
> > Hi Olivier,
> >
> > Thanks, I think this is looking much better.
> >
> > I'd like to know how others feel about landing this heap; there's
> > been
> > push-back in the past about heaps in device-tree and discussions
> > around how "custom" heaps should be treated (though IMO this is quite
> > a generic one).
> >
> > On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
> > > add Linaro secure heap bindings: linaro,secure-heap
> > > use genalloc to allocate/free buffer from buffer pool.
> > > buffer pool info is from dts.
> > > use sg_table instore the allocated memory info, the length of
> > > sg_table is 1.
> > > implement secure_heap_buf_ops to implement buffer share in
> > > difference device:
> > > 1. Userspace passes this fd to all drivers it wants this buffer
> > > to share with: First the filedescriptor is converted to a &dma_buf
> > > using
> > > dma_buf_get(). Then the buffer is attached to the device using
> > > dma_buf_attach().
> > > 2. Once the buffer is attached to all devices userspace can
> > > initiate DMA
> > > access to the shared buffer. In the kernel this is done by calling
> > > dma_buf_map_attachment()
> > > 3. get sg_table with dma_buf_map_attachment in difference device.
> > >
> >
> > I think this commit message could use a little rework. A few
> > thoughts:
> >
> > * The bindings are in a separate commit, so seems strange to mention
> > here.
>
> what about:
> "add Linaro secure heap compatible reserved memory: linaro,secure-heap"
>
I'd say something like:
Add a dma-buf heap to allocate secure buffers from a reserved-memory
region.
..snip
> > > +
> > > +static struct sg_table *secure_heap_map_dma_buf(struct
> > > dma_buf_attachment *attachment,
> > > + enum
> > > dma_data_direction direction)
> > > +{
> > > + struct secure_heap_attachment *a = attachment->priv;
> > > +
> > > + return a->table;
> >
> > I think you still need to implement mapping and unmapping using the
> > DMA APIs. For example devices might be behind IOMMUs and the buffer
> > will need mapping into the IOMMU.
>
> Devices that will need access to the buffer must be in secure.
> The tee driver will only need the scatter-list table to get dma address
> and len. Mapping will be done in the TEE.
> Please find tee_shm_register_fd in the following commit
> https://github.com/linaro-swg/linux/commit/41e21e5c405530590dc2dd10b2a8dbe64589840f
>
> This patch need to be up-streamed as well.
>
Interesting, that's not how the devices I've worked on operated.
Are you saying that you have to have a display controller driver
running in the TEE to display one of these buffers? If everything
needs to be in the TEE, then why even have these buffers allocated
by non-secure Linux at all?
I would have expected there to be HW enforcement of buffer access,
but for the display driver to be in non-secure Linux. That's how
TZMP1 works: https://static.linaro.org/connect/hkg18/presentations/hkg18-408.pdf
Looking at this SDP presentation, that also seems to be the case
there: https://static.linaro.org/connect/san19/presentations/san19-107.pdf
Based on those presentations, I think this heap should be implementing
map_dma_buf in the "normal" way, using the DMA API to map the buffer
to the device. It's up to the TEE and HW firewall to prevent access
to those mappings from non-secure devices.
My understanding is:
* The memory region should never be mapped or accessed from the host
CPU. This is not a security requirement - the CPU will be denied
access by whatever hardware is enforcing security - but any CPU
accesses will fail, so there is no point in ever having a CPU
mapping.
* The allocated buffers _should_ be mapped to devices via map_dma_buf.
Again the HW enforcement will prevent access from devices which
aren't permitted access, but for example a display controller
may be allowed to read the secure buffer, composite it with other
buffers, and display it on the screen.
Am I wrong? Even if SDP doesn't work this way, I think we should make
the heap as generic as possible so that it can work with different
secure video implementations.
>
> >
.. snip
> > > +
> > > +RESERVEDMEM_OF_DECLARE(secure_heap, "linaro,secure-heap",
> > > rmem_secure_heap_setup);
> >
> > Is there anything linaro-specific about this? Could it be
> > linux,secure-heap?
>
> for now, it's specific to Linaro OPTEE OS.
> but in a more generic way, it could be
> linux,unmapped-heap ?
If these buffers can never be mapped, not to the CPU nor to devices,
then actually I don't see why it should be a dma-buf heap at all.
If this is just an interface to associate some identifier (in this
case an fd) with a region of physical address space, then why is it
useful to pretend that it's a dma-buf, if none of the
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Christian,
Thanks for your comments, please find my answer below.
On mer., 2022-08-10 at 11:43 +0200, Christian König wrote:
> Caution: EXT Email
>
> Hi guys,
>
> Am 05.08.22 um 15:53 schrieb Olivier Masse:
> > add Linaro secure heap bindings: linaro,secure-heap
> > use genalloc to allocate/free buffer from buffer pool.
> > buffer pool info is from dts.
> > use sg_table instore the allocated memory info, the length of
> > sg_table is 1.
> > implement secure_heap_buf_ops to implement buffer share in
> > difference device:
> > 1. Userspace passes this fd to all drivers it wants this buffer
> > to share with: First the filedescriptor is converted to a &dma_buf
> > using
> > dma_buf_get(). Then the buffer is attached to the device using
> > dma_buf_attach().
> > 2. Once the buffer is attached to all devices userspace can
> > initiate DMA
> > access to the shared buffer. In the kernel this is done by calling
> > dma_buf_map_attachment()
> > 3. get sg_table with dma_buf_map_attachment in difference device.
>
> I'm not sure Christoph will approve that you are messing with the sg
> table internals so much here.
>
> Why are you not using the DMA API directly for this?
Do you mean for secure_heap_map_dma_buf and secure_heap_unmap_dma_buf ?
If yes, maybe something like the following could be more appropriate:
static struct sg_table *secure_heap_map_dma_buf(struct dma_buf_attachment
*attachment,
enum dma_data_direction
direction)
{
struct secure_heap_attachment *a = attachment->priv;
struct sg_table *sgt;
sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
if (!sgt) {
dev_err(a->dev, "failed to alloc sg table\n");
return NULL;
}
ret = dma_get_sgtable(a->dev, sgt, NULL, sg_dma_address(a->table->sgl),
sg_dma_len(a->table->sgl));
if (ret < 0) {
dev_err(a->dev, "failed to get scatterlist from DMA API\n");
kfree(sgt);
return NULL;
}
return sgt;
}
Regards,
Olivier
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Olivier Masse
> > ---
> > drivers/dma-buf/heaps/Kconfig | 9 +
> > drivers/dma-buf/heaps/Makefile | 1 +
> > drivers/dma-buf/heaps/secure_heap.c | 357
> >
> > 3 files changed, 367 insertions(+)
> > create mode 100644 drivers/dma-buf/heaps/secure_heap.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-
> > buf/heaps/Kconfig
> > index 3782eeeb91c0..c9070c728b9a 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
> > Choose this option to enable the dsp dmabuf heap. The
> > dsp heap
> > is allocated by gen allocater. it's allocated according
> > the dts.
> > If in doubt, say Y.
> > +
> > +config DMABUF_HEAPS_SECURE
> > + tristate "DMA-BUF Secure Heap"
> > + depends on DMABUF_HEAPS
> > + help
> > + Choose this option to enable the secure dmabuf heap. The
> > secure heap
> > + pools are defined according to the DT. Heaps are allocated
> > + in the pools using gen allocater.
> > + If in doubt, say Y.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-
> > buf/heaps/Makefile
> > index 29733f84c354..863ef10056a3 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -2,3 +2,4 @@
> > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
> > obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> > obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_SECURE)+= secure_heap.o
> > diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-
> > buf/heaps/secure_heap.c
> > new file mode 100644
> > index ..25b3629613f3
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/secure_heap.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF secure heap exporter
> > + *
> > + * Copyright 2021 NXP.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define MAX_SECURE_HEAP 2
> > +#define MAX_HEAP_NAME_LEN 32
> > +
> > +struct secure_heap_buffer {
> > + struct dma_heap *heap;
> > + struct list_head attachments;
> > + struct mutex lock;
> > + unsigned long len;
> > + struct sg_table sg_table;
> > + int vmap_cnt;
> > + void *vaddr;
> > +};
> > +
> > +struct secure_heap_attachment {
> > + struct device *dev;
> > + struct sg_table *table;
> > + struct list_head list;
> > +};
> > +
> > +struct secure_heap_info {
> > + struct gen_pool *pool;
> > +};
> > +
> > +struct rmem_secure {
> > + phys_addr_t base;
> > + phys_addr_t size;
>
Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi guys,
Am 05.08.22 um 15:53 schrieb Olivier Masse:
add Linaro secure heap bindings: linaro,secure-heap
use genalloc to allocate/free buffer from buffer pool.
buffer pool info is from dts.
use sg_table instore the allocated memory info, the length of sg_table is 1.
implement secure_heap_buf_ops to implement buffer share in difference device:
1. Userspace passes this fd to all drivers it wants this buffer
to share with: First the filedescriptor is converted to a &dma_buf using
dma_buf_get(). Then the buffer is attached to the device using dma_buf_attach().
2. Once the buffer is attached to all devices userspace can initiate DMA
access to the shared buffer. In the kernel this is done by calling
dma_buf_map_attachment()
3. get sg_table with dma_buf_map_attachment in difference device.
I'm not sure Christoph will approve that you are messing with the sg
table internals so much here.
Why are you not using the DMA API directly for this?
Regards,
Christian.
Signed-off-by: Olivier Masse
---
drivers/dma-buf/heaps/Kconfig | 9 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/secure_heap.c | 357
3 files changed, 367 insertions(+)
create mode 100644 drivers/dma-buf/heaps/secure_heap.c
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 3782eeeb91c0..c9070c728b9a 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
Choose this option to enable the dsp dmabuf heap. The dsp heap
is allocated by gen allocater. it's allocated according the dts.
If in doubt, say Y.
+
+config DMABUF_HEAPS_SECURE
+ tristate "DMA-BUF Secure Heap"
+ depends on DMABUF_HEAPS
+ help
+ Choose this option to enable the secure dmabuf heap. The secure heap
+ pools are defined according to the DT. Heaps are allocated
+ in the pools using gen allocater.
+ If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 29733f84c354..863ef10056a3 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA)+= cma_heap.o
obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_SECURE) += secure_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c
b/drivers/dma-buf/heaps/secure_heap.c
new file mode 100644
index ..25b3629613f3
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -0,0 +1,357 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright 2021 NXP.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_SECURE_HEAP 2
+#define MAX_HEAP_NAME_LEN 32
+
+struct secure_heap_buffer {
+ struct dma_heap *heap;
+ struct list_head attachments;
+ struct mutex lock;
+ unsigned long len;
+ struct sg_table sg_table;
+ int vmap_cnt;
+ void *vaddr;
+};
+
+struct secure_heap_attachment {
+ struct device *dev;
+ struct sg_table *table;
+ struct list_head list;
+};
+
+struct secure_heap_info {
+ struct gen_pool *pool;
+};
+
+struct rmem_secure {
+ phys_addr_t base;
+ phys_addr_t size;
+
+ char name[MAX_HEAP_NAME_LEN];
+};
+
+static struct rmem_secure secure_data[MAX_SECURE_HEAP] = {0};
+static unsigned int secure_data_count;
+
+static struct sg_table *dup_sg_table(struct sg_table *table)
+{
+ struct sg_table *new_table;
+ int ret, i;
+ struct scatterlist *sg, *new_sg;
+
+ new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
+ if (!new_table)
+ return ERR_PTR(-ENOMEM);
+
+ ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
+ if (ret) {
+ kfree(new_table);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ new_sg = new_table->sgl;
+ for_each_sgtable_sg(table, sg, i) {
+ sg_set_page(new_sg, sg_page(sg), sg->length, sg->offset);
+ new_sg->dma_address = sg->dma_address;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+ new_sg->dma_length = sg->dma_length;
+#endif
+ new_sg = sg_next(new_sg);
+ }
+
+ return new_table;
+}
+
+static int secure_heap_attach(struct dma_buf *dmabuf,
+ struct dma_buf_attachment *attachment)
+{
+ struct secure_heap_buffer *buffer = dmabuf->priv;
+ struct secure_heap_attachment *a;
+ struct sg_table *table;
+
+ a = kzalloc(sizeof(*a), GFP_KERNEL);
+ if (!a)
+ return -ENOMEM;
+
+ table = dup_sg_table(&buffer->sg_table);
+ if (IS_ERR(table)) {
+ kfree(a);
+ return -ENOM
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Brian,
> > +
> > + rmem->ops = &rmem_dma_ops;
> > + pr_info("Reserved memory: DMA buf secure pool %s at
> > %pa, size %ld MiB\n",
> > + secure_data[secure_data_count].name,
> > + &rmem->base, (unsigned long)rmem->size /
> > SZ_1M);
>
> nit: What if rmem->size < SZ_1M, or not 1M-aligned
>
Let's assume that size is 1K-aligned, maybe something like that could
be better ?
unsigned long mb = rmem->size >> 20;
unsigned long kb = (rmem->size & (SZ_1M - 1)) >> 10;
pr_info("Reserved memory: DMA buf secure pool %s at %pa, size
%ld MiB and %ld KiB",
secure_data[secure_data_count].name,
&rmem->base, mb, kb);
Cheers,
Olivier
[PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
add Linaro secure heap compatible reserved mem: linaro,secure-heap
use genalloc to allocate/free buffer from buffer pool.
buffer pool info is defined from a dts reserved memory.
Signed-off-by: Olivier Masse
---
drivers/dma-buf/heaps/Kconfig | 9 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/secure_heap.c | 339
3 files changed, 349 insertions(+)
create mode 100644 drivers/dma-buf/heaps/secure_heap.c
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 3782eeeb91c0..c9070c728b9a 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
Choose this option to enable the dsp dmabuf heap. The dsp heap
is allocated by gen allocater. it's allocated according the dts.
If in doubt, say Y.
+
+config DMABUF_HEAPS_SECURE
+ tristate "DMA-BUF Secure Heap"
+ depends on DMABUF_HEAPS
+ help
+ Choose this option to enable the secure dmabuf heap. The secure heap
+ pools are defined according to the DT. Heaps are allocated
+ in the pools using gen allocater.
+ If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 29733f84c354..863ef10056a3 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_SECURE) += secure_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c
b/drivers/dma-buf/heaps/secure_heap.c
new file mode 100644
index ..a3023bf8d457
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright 2022 NXP.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_SECURE_HEAP 2
+#define MAX_HEAP_NAME_LEN 32
+
+struct secure_heap_buffer {
+ struct dma_heap *heap;
+ struct list_head attachments;
+ struct mutex lock;
+ unsigned long len;
+ struct sg_table sg_table;
+ int vmap_cnt;
+ void *vaddr;
+};
+
+struct secure_heap_attachment {
+ struct device *dev;
+ struct sg_table *table;
+ struct list_head list;
+};
+
+struct secure_heap_info {
+ struct gen_pool *pool;
+};
+
+struct rmem_secure {
+ phys_addr_t base;
+ phys_addr_t size;
+
+ char name[MAX_HEAP_NAME_LEN];
+};
+
+static struct rmem_secure secure_data[MAX_SECURE_HEAP] = {0};
+static unsigned int secure_data_count;
+
+static struct sg_table *dup_sg_table(struct sg_table *table)
+{
+ struct sg_table *new_table;
+ int ret, i;
+ struct scatterlist *sg, *new_sg;
+
+ new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
+ if (!new_table)
+ return ERR_PTR(-ENOMEM);
+
+ ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
+ if (ret) {
+ kfree(new_table);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ new_sg = new_table->sgl;
+ for_each_sgtable_sg(table, sg, i) {
+ sg_set_page(new_sg, sg_page(sg), sg->length, sg->offset);
+ new_sg->dma_address = sg->dma_address;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+ new_sg->dma_length = sg->dma_length;
+#endif
+ new_sg = sg_next(new_sg);
+ }
+
+ return new_table;
+}
+
+static int secure_heap_attach(struct dma_buf *dmabuf,
+ struct dma_buf_attachment *attachment)
+{
+ struct secure_heap_buffer *buffer = dmabuf->priv;
+ struct secure_heap_attachment *a;
+ struct sg_table *table;
+
+ a = kzalloc(sizeof(*a), GFP_KERNEL);
+ if (!a)
+ return -ENOMEM;
+
+ table = dup_sg_table(&buffer->sg_table);
+ if (IS_ERR(table)) {
+ kfree(a);
+ return PTR_ERR(table);
+ }
+
+ a->table = table;
+ a->dev = attachment->dev;
+ INIT_LIST_HEAD(&a->list);
+ attachment->priv = a;
+
+ mutex_lock(&buffer->lock);
+ list_add(&a->list, &buffer->attachments);
+ mutex_unlock(&buffer->lock);
+
+ return 0;
+}
+
+static void secure_heap_detach(struct dma_buf *dmabuf,
+ struct dma_buf_attachment *attachment)
+{
+ struct secure_heap_buffer *buffer = dmabuf->priv;
+ struct secure_heap_attachment *a = attachment->priv;
+
+ mutex_lock(&buffer->lock);
+ list_del(&a->list);
+ mutex_unlock(&buffer->lock);
+
+ sg_free_table(a->table);
+ kfree(a->table);
+ kfree(a);
+}
+
+static struct sg_table *secure_heap_map_dma_buf(struct dma_buf_attachment
*attachment,
Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Brian,
On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> Caution: EXT Email
>
> Hi Olivier,
>
> Thanks, I think this is looking much better.
>
> I'd like to know how others feel about landing this heap; there's
> been
> push-back in the past about heaps in device-tree and discussions
> around how "custom" heaps should be treated (though IMO this is quite
> a generic one).
>
> On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
> > add Linaro secure heap bindings: linaro,secure-heap
> > use genalloc to allocate/free buffer from buffer pool.
> > buffer pool info is from dts.
> > use sg_table instore the allocated memory info, the length of
> > sg_table is 1.
> > implement secure_heap_buf_ops to implement buffer share in
> > difference device:
> > 1. Userspace passes this fd to all drivers it wants this buffer
> > to share with: First the filedescriptor is converted to a &dma_buf
> > using
> > dma_buf_get(). Then the buffer is attached to the device using
> > dma_buf_attach().
> > 2. Once the buffer is attached to all devices userspace can
> > initiate DMA
> > access to the shared buffer. In the kernel this is done by calling
> > dma_buf_map_attachment()
> > 3. get sg_table with dma_buf_map_attachment in difference device.
> >
>
> I think this commit message could use a little rework. A few
> thoughts:
>
> * The bindings are in a separate commit, so seems strange to mention
> here.
what about:
"add Linaro secure heap compatible reserved memory: linaro,secure-heap"
> * "buffer pool info is from dts" --> I think you should mention that
> this uses a reserved-memory region.
ok
> * sg_table nents and genalloc seem like low-level implementation
> details, so probably not needed in the commit message
> * The usage steps 1, 2, 3 aren't specific to this heap - that's how
> all dma-buf sharing works.
ok, let's cleanup and removed this.
>
> > Signed-off-by: Olivier Masse
> > ---
> > drivers/dma-buf/heaps/Kconfig | 9 +
> > drivers/dma-buf/heaps/Makefile | 1 +
> > drivers/dma-buf/heaps/secure_heap.c | 357
> >
> > 3 files changed, 367 insertions(+)
> > create mode 100644 drivers/dma-buf/heaps/secure_heap.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-
> > buf/heaps/Kconfig
> > index 3782eeeb91c0..c9070c728b9a 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
> >Choose this option to enable the dsp dmabuf heap. The
> > dsp heap
> >is allocated by gen allocater. it's allocated according
> > the dts.
> >If in doubt, say Y.
> > +
> > +config DMABUF_HEAPS_SECURE
> > + tristate "DMA-BUF Secure Heap"
> > + depends on DMABUF_HEAPS
> > + help
> > + Choose this option to enable the secure dmabuf heap. The
> > secure heap
> > + pools are defined according to the DT. Heaps are allocated
> > + in the pools using gen allocater.
> > + If in doubt, say Y.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-
> > buf/heaps/Makefile
> > index 29733f84c354..863ef10056a3 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -2,3 +2,4 @@
> > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> > obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> > obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_SECURE)+= secure_heap.o
> > diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-
> > buf/heaps/secure_heap.c
> > new file mode 100644
> > index ..25b3629613f3
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/secure_heap.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF secure heap exporter
> > + *
> > + * Copyright 2021 NXP.
>
> It's 2022 :-)
>
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define MAX_SECURE_HEAP 2
> > +#define MAX_HEAP_NAME_LEN 32
> > +
> > +struct secure_heap_buffer {
> > + struct dma_heap *heap;
> > + struct list_head attachments;
> > + struct mutex lock;
> > + unsigned long len;
> > + struct sg_table sg_table;
> > + int vmap_cnt;
> > + void *vaddr;
> > +};
> > +
> > +struct secure_heap_attachment {
> > + struct device *dev;
> > + struct sg_table *table;
> > + struct list_head list;
> > +};
> > +
> > +struct secure_heap_info {
> > + struct gen_pool *pool;
> > +};
> > +
> > +struct rmem_secure {
> > + phys_addr_t base;
> > + phys_addr_t size;
> > +
> > + char name[MAX_HEAP_NAME_LEN];
> > +};
> > +
> > +static struct rmem_secure secure_data[MAX_SECURE_HEAP] = {0};
> > +static unsigned int secure_data_count;
> > +
> > +static struct sg_table *dup_sg_table(stru
Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
Hi Olivier,
Thanks, I think this is looking much better.
I'd like to know how others feel about landing this heap; there's been
push-back in the past about heaps in device-tree and discussions
around how "custom" heaps should be treated (though IMO this is quite
a generic one).
On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
> add Linaro secure heap bindings: linaro,secure-heap
> use genalloc to allocate/free buffer from buffer pool.
> buffer pool info is from dts.
> use sg_table instore the allocated memory info, the length of sg_table is 1.
> implement secure_heap_buf_ops to implement buffer share in difference device:
> 1. Userspace passes this fd to all drivers it wants this buffer
> to share with: First the filedescriptor is converted to a &dma_buf using
> dma_buf_get(). Then the buffer is attached to the device using
> dma_buf_attach().
> 2. Once the buffer is attached to all devices userspace can initiate DMA
> access to the shared buffer. In the kernel this is done by calling
> dma_buf_map_attachment()
> 3. get sg_table with dma_buf_map_attachment in difference device.
>
I think this commit message could use a little rework. A few thoughts:
* The bindings are in a separate commit, so seems strange to mention
here.
* "buffer pool info is from dts" --> I think you should mention that
this uses a reserved-memory region.
* sg_table nents and genalloc seem like low-level implementation
details, so probably not needed in the commit message
* The usage steps 1, 2, 3 aren't specific to this heap - that's how
all dma-buf sharing works.
> Signed-off-by: Olivier Masse
> ---
> drivers/dma-buf/heaps/Kconfig | 9 +
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/secure_heap.c | 357
> 3 files changed, 367 insertions(+)
> create mode 100644 drivers/dma-buf/heaps/secure_heap.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index 3782eeeb91c0..c9070c728b9a 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
>Choose this option to enable the dsp dmabuf heap. The dsp heap
>is allocated by gen allocater. it's allocated according the dts.
>If in doubt, say Y.
> +
> +config DMABUF_HEAPS_SECURE
> + tristate "DMA-BUF Secure Heap"
> + depends on DMABUF_HEAPS
> + help
> + Choose this option to enable the secure dmabuf heap. The secure heap
> + pools are defined according to the DT. Heaps are allocated
> + in the pools using gen allocater.
> + If in doubt, say Y.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 29733f84c354..863ef10056a3 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -2,3 +2,4 @@
> obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_SECURE)+= secure_heap.o
> diff --git a/drivers/dma-buf/heaps/secure_heap.c
> b/drivers/dma-buf/heaps/secure_heap.c
> new file mode 100644
> index ..25b3629613f3
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF secure heap exporter
> + *
> + * Copyright 2021 NXP.
It's 2022 :-)
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_SECURE_HEAP 2
> +#define MAX_HEAP_NAME_LEN 32
> +
> +struct secure_heap_buffer {
> + struct dma_heap *heap;
> + struct list_head attachments;
> + struct mutex lock;
> + unsigned long len;
> + struct sg_table sg_table;
> + int vmap_cnt;
> + void *vaddr;
> +};
> +
> +struct secure_heap_attachment {
> + struct device *dev;
> + struct sg_table *table;
> + struct list_head list;
> +};
> +
> +struct secure_heap_info {
> + struct gen_pool *pool;
> +};
> +
> +struct rmem_secure {
> + phys_addr_t base;
> + phys_addr_t size;
> +
> + char name[MAX_HEAP_NAME_LEN];
> +};
> +
> +static struct rmem_secure secure_data[MAX_SECURE_HEAP] = {0};
> +static unsigned int secure_data_count;
> +
> +static struct sg_table *dup_sg_table(struct sg_table *table)
> +{
> + struct sg_table *new_table;
> + int ret, i;
> + struct scatterlist *sg, *new_sg;
> +
> + new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
> + if (!new_table)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
> + if (ret) {
> + kfree(new_table);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + new_sg = new_table->sgl;
> + for_each_sgtable_sg(table, sg, i) {
> + sg_set
[PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support
add Linaro secure heap bindings: linaro,secure-heap
use genalloc to allocate/free buffer from buffer pool.
buffer pool info is from dts.
use sg_table instore the allocated memory info, the length of sg_table is 1.
implement secure_heap_buf_ops to implement buffer share in difference device:
1. Userspace passes this fd to all drivers it wants this buffer
to share with: First the filedescriptor is converted to a &dma_buf using
dma_buf_get(). Then the buffer is attached to the device using dma_buf_attach().
2. Once the buffer is attached to all devices userspace can initiate DMA
access to the shared buffer. In the kernel this is done by calling
dma_buf_map_attachment()
3. get sg_table with dma_buf_map_attachment in difference device.
Signed-off-by: Olivier Masse
---
drivers/dma-buf/heaps/Kconfig | 9 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/secure_heap.c | 357
3 files changed, 367 insertions(+)
create mode 100644 drivers/dma-buf/heaps/secure_heap.c
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 3782eeeb91c0..c9070c728b9a 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -20,3 +20,12 @@ config DMABUF_HEAPS_DSP
Choose this option to enable the dsp dmabuf heap. The dsp heap
is allocated by gen allocater. it's allocated according the dts.
If in doubt, say Y.
+
+config DMABUF_HEAPS_SECURE
+ tristate "DMA-BUF Secure Heap"
+ depends on DMABUF_HEAPS
+ help
+ Choose this option to enable the secure dmabuf heap. The secure heap
+ pools are defined according to the DT. Heaps are allocated
+ in the pools using gen allocater.
+ If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 29733f84c354..863ef10056a3 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
obj-$(CONFIG_DMABUF_HEAPS_DSP) += dsp_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_SECURE) += secure_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c
b/drivers/dma-buf/heaps/secure_heap.c
new file mode 100644
index ..25b3629613f3
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -0,0 +1,357 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright 2021 NXP.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_SECURE_HEAP 2
+#define MAX_HEAP_NAME_LEN 32
+
+struct secure_heap_buffer {
+ struct dma_heap *heap;
+ struct list_head attachments;
+ struct mutex lock;
+ unsigned long len;
+ struct sg_table sg_table;
+ int vmap_cnt;
+ void *vaddr;
+};
+
+struct secure_heap_attachment {
+ struct device *dev;
+ struct sg_table *table;
+ struct list_head list;
+};
+
+struct secure_heap_info {
+ struct gen_pool *pool;
+};
+
+struct rmem_secure {
+ phys_addr_t base;
+ phys_addr_t size;
+
+ char name[MAX_HEAP_NAME_LEN];
+};
+
+static struct rmem_secure secure_data[MAX_SECURE_HEAP] = {0};
+static unsigned int secure_data_count;
+
+static struct sg_table *dup_sg_table(struct sg_table *table)
+{
+ struct sg_table *new_table;
+ int ret, i;
+ struct scatterlist *sg, *new_sg;
+
+ new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
+ if (!new_table)
+ return ERR_PTR(-ENOMEM);
+
+ ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
+ if (ret) {
+ kfree(new_table);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ new_sg = new_table->sgl;
+ for_each_sgtable_sg(table, sg, i) {
+ sg_set_page(new_sg, sg_page(sg), sg->length, sg->offset);
+ new_sg->dma_address = sg->dma_address;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+ new_sg->dma_length = sg->dma_length;
+#endif
+ new_sg = sg_next(new_sg);
+ }
+
+ return new_table;
+}
+
+static int secure_heap_attach(struct dma_buf *dmabuf,
+ struct dma_buf_attachment *attachment)
+{
+ struct secure_heap_buffer *buffer = dmabuf->priv;
+ struct secure_heap_attachment *a;
+ struct sg_table *table;
+
+ a = kzalloc(sizeof(*a), GFP_KERNEL);
+ if (!a)
+ return -ENOMEM;
+
+ table = dup_sg_table(&buffer->sg_table);
+ if (IS_ERR(table)) {
+ kfree(a);
+ return -ENOMEM;
+ }
+
+ a->table = table;
+ a->dev = attachment->dev;
+ INIT_LIST_HEAD(&a->list);
+ attachment->priv = a;
+
+ mutex_lock(&buffer->lock);
+ list_add(&a->list, &buffer->attachments);
+ mutex_unlock(&buff
