Re: [PATCH v6,12/24] media: mediatek: vcodec: add interface to allocate/free secure memory
On Wed, 2024-06-12 at 14:22 +0900, Tomasz Figa wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Thu, May 16, 2024 at 08:20:50PM +0800, Yunfei Dong wrote:
> > Need to call dma heap interface to allocate/free secure memory when
> playing
> > secure video.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > .../media/platform/mediatek/vcodec/Kconfig| 1 +
> > .../mediatek/vcodec/common/mtk_vcodec_util.c | 122
> +-
> > .../mediatek/vcodec/common/mtk_vcodec_util.h | 3 +
> > 3 files changed, 123 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/mediatek/vcodec/Kconfig
> b/drivers/media/platform/mediatek/vcodec/Kconfig
> > index bc8292232530..707865703e61 100644
> > --- a/drivers/media/platform/mediatek/vcodec/Kconfig
> > +++ b/drivers/media/platform/mediatek/vcodec/Kconfig
> > @@ -17,6 +17,7 @@ config VIDEO_MEDIATEK_VCODEC
[snip]
> > -void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
> > +static int mtk_vcodec_mem_alloc_sec(struct mtk_vcodec_dec_ctx
> *ctx, struct mtk_vcodec_mem *mem)
> > +{
> > +struct device *dev = &ctx->dev->plat_dev->dev;
> > +struct dma_buf *dma_buffer;
> > +struct dma_heap *vdec_heap;
> > +struct dma_buf_attachment *attach;
> > +struct sg_table *sgt;
> > +unsigned long size = mem->size;
> > +int ret = 0;
> > +
> > +if (!size)
> > +return -EINVAL;
> > +
> > +vdec_heap = dma_heap_find("restricted_mtk_cma");
> > +if (!vdec_heap) {
> > +mtk_v4l2_vdec_err(ctx, "dma heap find failed!");
> > +return -EPERM;
> > +}
>
> How is the heap name determined here? My recollection is that the
> heap
> name comes from the heap node in the DT, so it may vary depending on
> the
> board.
>
> Is the heap name documented anywhere in the DT bindings?
>
> Shouldn't we rather query DT for a phandle to the right heap?
>
Hi Tomasz,
This heap name does not come from dt-binding. It is hard-coded in the
driver[1]. Because the heap driver is a pure SW driver, there is no
corresponding HW unit, and there is no way to add a dtsi node.
[1]
https://lore.kernel.org/linux-mediatek/20240515112308.10171-10-yong...@mediatek.com/
Thanks.
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Thu, 2024-05-16 at 11:59 +0200, AngeloGioacchino Del Regno wrote: > Il 15/05/24 13:23, Yong Wu ha scritto: > > Introduce a FLAG for the restricted memory which means the memory > > is > > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > interface > > has not been added. > > > > Signed-off-by: Yong Wu > > --- > > include/linux/scatterlist.h | 34 > > ++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/include/linux/scatterlist.h > > b/include/linux/scatterlist.h > > index 77df3d7b18a6..a6ad9018eca0 100644 > > --- a/include/linux/scatterlist.h > > +++ b/include/linux/scatterlist.h > > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct > > scatterlist *sg) > > > > #define SG_DMA_BUS_ADDRESS(1 << 0) > > #define SG_DMA_SWIOTLB(1 << 1) > > +#define SG_DMA_RESTRICTED (2 << 1) > > I think you wanted to write (1 << 2) here :-) Apparently, you are right:) Thanks. > > Cheers, > Angelo
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Am 15.05.24 um 13:23 schrieb Yong Wu:
> > Introduce a FLAG for the restricted memory which means the memory
> is
> > protected by TEE or hypervisor, then it's inaccessiable for kernel.
> >
> > Currently we don't use sg_dma_unmark_restricted, thus this
> interface
> > has not been added.
>
> Why should that be part of the scatterlist? It doesn't seem to
> affect
> any of it's functionality.
>
> As far as I can see the scatterlist shouldn't be the transport of
> this
> kind of information.
Thanks for the review. I will remove this.
In our user scenario, DRM will import these buffers and check if this
is a restricted buffer. If yes, it will use secure GCE takes over.
If this judgment is not suitable to be placed in scatterlist. I don't
know if it is ok to limit this inside dma-buf. Adding such an
interface:
static bool dma_buf_is_restricted(struct dma_buf *dmabuf)
{
return !strncmp(dmabuf->exp_name, "restricted", 10);
}
Thanks.
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Yong Wu
> > ---
> > include/linux/scatterlist.h | 34
> ++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/include/linux/scatterlist.h
> b/include/linux/scatterlist.h
> > index 77df3d7b18a6..a6ad9018eca0 100644
> > --- a/include/linux/scatterlist.h
> > +++ b/include/linux/scatterlist.h
> > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct
> scatterlist *sg)
> >
> > #define SG_DMA_BUS_ADDRESS(1 << 0)
> > #define SG_DMA_SWIOTLB(1 << 1)
> > +#define SG_DMA_RESTRICTED(2 << 1)
> >
> > /**
> >* sg_dma_is_bus_address - Return whether a given segment was
> marked
> > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct
> scatterlist *sg)
> > sg->dma_flags |= SG_DMA_SWIOTLB;
> > }
> >
> > +/**
> > + * sg_dma_mark_restricted - Mark the scatterlist for restricted
> buffer.
> > + * @sg:SG entry
> > + *
> > + * Description:
> > + * Marks a a scatterlist for the restricted buffer that may be
> inaccessiable
> > + * in kernel if it is protected.
> > + */
> > +static inline void sg_dma_mark_restricted(struct scatterlist *sg)
> > +{
> > +sg->dma_flags |= SG_DMA_RESTRICTED;
> > +}
> > +
> > +/**
> > + * sg_dma_is_restricted - Return whether the scatterlist was
> marked as restricted
> > + *buffer.
> > + * @sg:SG entry
> > + *
> > + * Description:
> > + * Returns true if the scatterlist was marked as restricted
> buffer.
> > + */
> > +static inline bool sg_dma_is_restricted(struct scatterlist *sg)
> > +{
> > +return sg->dma_flags & SG_DMA_RESTRICTED;
> > +}
> > #else
> >
> > static inline bool sg_dma_is_bus_address(struct scatterlist *sg)
> > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct
> scatterlist *sg)
> > {
> > }
> >
> > +static inline bool sg_dma_is_restricted(struct scatterlist *sg)
> > +{
> > +return false;
> > +}
> > +
> > +static inline void sg_dma_mark_restrited(struct scatterlist *sg)
> > +{
> > +}
> > #endif/* CONFIG_NEED_SG_DMA_FLAGS */
> >
> > /**
>
>
Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops
Hi Joakim,
Sorry for reply so late.
On Wed, 2024-01-31 at 14:53 +0100, Joakim Bech wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Fri, Jan 12, 2024 at 05:20:10PM +0800, Yong Wu wrote:
> > Add "struct restricted_heap_ops". For the restricted memory,
> totally there
> > are two steps:
> > a) memory_alloc: Allocate the buffer in kernel;
> > b) memory_restrict: Restrict/Protect/Secure that buffer.
> > The memory_alloc is mandatory while memory_restrict is optinal
> since it may
> >
> s/optinal/optional/
Will Fix.
>
> > be part of memory_alloc.
> >
> > Signed-off-by: Yong Wu
> > ---
> > drivers/dma-buf/heaps/restricted_heap.c | 41
> -
> > drivers/dma-buf/heaps/restricted_heap.h | 12
> > 2 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-
> buf/heaps/restricted_heap.c
> > index fd7c82abd42e..8c266a0f6192 100644
> > --- a/drivers/dma-buf/heaps/restricted_heap.c
> > +++ b/drivers/dma-buf/heaps/restricted_heap.c
> > @@ -12,10 +12,44 @@
> >
> > #include "restricted_heap.h"
> >
> > +static int
> > +restricted_heap_memory_allocate(struct restricted_heap *heap,
> struct restricted_buffer *buf)
> > +{
> > +const struct restricted_heap_ops *ops = heap->ops;
> > +int ret;
> > +
> > +ret = ops->memory_alloc(heap, buf);
> > +if (ret)
> > +return ret;
> > +
> > +if (ops->memory_restrict) {
> > +ret = ops->memory_restrict(heap, buf);
> > +if (ret)
> > +goto memory_free;
> > +}
> > +return 0;
> > +
> > +memory_free:
> > +ops->memory_free(heap, buf);
> > +return ret;
> > +}
> > +
> > +static void
> > +restricted_heap_memory_free(struct restricted_heap *heap, struct
> restricted_buffer *buf)
> > +{
> > +const struct restricted_heap_ops *ops = heap->ops;
> > +
> > +if (ops->memory_unrestrict)
> > +ops->memory_unrestrict(heap, buf);
> > +
> > +ops->memory_free(heap, buf);
> > +}
> > +
> > static struct dma_buf *
> > restricted_heap_allocate(struct dma_heap *heap, unsigned long
> size,
> > unsigned long fd_flags, unsigned long heap_flags)
> > {
> > +struct restricted_heap *restricted_heap =
> dma_heap_get_drvdata(heap);
> > struct restricted_buffer *restricted_buf;
> > DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > struct dma_buf *dmabuf;
> > @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap,
> unsigned long size,
> > restricted_buf->size = ALIGN(size, PAGE_SIZE);
> > restricted_buf->heap = heap;
> >
> > +ret = restricted_heap_memory_allocate(restricted_heap,
> restricted_buf);
> >
> Can we guarantee that "restricted_heap" here isn't NULL (i.e., heap-
> >priv). If
> not perhaps we should consider adding a check for NULL in the
> restricted_heap_memory_allocate() function?
heap->priv always is set when dma_heap_add is called. Suppose heap-
>priv is NULL, the KE would have already been occurred in
restricted_heap_add. I don't think it can be NULL. is it right?
>
> > +if (ret)
> > +goto err_free_buf;
> > exp_info.exp_name = dma_heap_get_name(heap);
> > exp_info.size = restricted_buf->size;
> > exp_info.flags = fd_flags;
> > @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap,
> unsigned long size,
> > dmabuf = dma_buf_export(&exp_info);
> > if (IS_ERR(dmabuf)) {
> > ret = PTR_ERR(dmabuf);
> > -goto err_free_buf;
> > +goto err_free_restricted_mem;
> > }
> >
> > return dmabuf;
> >
> > +err_free_restricted_mem:
> > +restricted_heap_memory_free(restricted_heap, restricted_buf);
> > err_free_buf:
> > kfree(restricted_buf);
> > return ERR_PTR(ret);
> > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-
> buf/heaps/restricted_heap.h
> > index 443028f6ba3b..ddeaf9805708 100644
> > --- a/drivers/dma-buf/heaps/restricted_heap.h
> > +++ b/drivers/dma-buf/heaps/restricted_heap.h
> > @@ -15,6 +15,18 @@ struct restricted_buffer {
> >
> > struct restricted_heap {
> > const char*name;
> > +
> > +const struct restricted_heap_ops *ops;
> > +};
> > +
> > +struct restricted_heap_ops {
> >
> This have the same name as used for the dma_heap_ops in the file
> restricted_heap.c, this might be a little bit confusing, or?
Thanks very much. I really didn't notice this. I will rename it.
>
> > +int(*heap_init)(struct restricted_heap *heap);
> > +
> > +int(*memory_alloc)(struct restricted_heap *heap, struct
> restricted_buffer *buf);
> > +void(*memory_free)(struct restricted_heap *heap, struct
> restricted_buffer *buf);
> > +
> > +int(*memory_restrict)(struct restricted_heap *heap, struct
> restricted_buffer *buf);
> > +void(*memory_unrestrict)(struct restricted_heap *heap, struct
> restricted_buffer *buf);
> >
> Is the prefix "memory_" superfluous here in these ops?
I will remove "memory_". But it looks like the "restrict" is a
keyword, I can't use it or it will build fail. Therefore I plan to use
alloc/free/restrict_buf/unrestrict_buf in next version.
>
> Also related
Re: [PATCH v4 4/7] dma-buf: heaps: restricted_heap: Add dma_ops
On Fri, 2024-01-12 at 10:49 +0100, Daniel Vetter wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Fri, Jan 12, 2024 at 10:41:14AM +0100, Daniel Vetter wrote:
> > On Fri, Jan 12, 2024 at 05:20:11PM +0800, Yong Wu wrote:
> > > Add the dma_ops for this restricted heap. For restricted buffer,
> > > cache_ops/mmap are not allowed, thus return EPERM for them.
> > >
> > > Signed-off-by: Yong Wu
> > > ---
> > > drivers/dma-buf/heaps/restricted_heap.c | 103
>
> > > 1 file changed, 103 insertions(+)
> > >
> > > diff --git a/drivers/dma-buf/heaps/restricted_heap.c
> b/drivers/dma-buf/heaps/restricted_heap.c
> > > index 8c266a0f6192..ec4c63d2112d 100644
> > > --- a/drivers/dma-buf/heaps/restricted_heap.c
> > > +++ b/drivers/dma-buf/heaps/restricted_heap.c
> > > @@ -12,6 +12,10 @@
> > >
> > > #include "restricted_heap.h"
> > >
> > > +struct restricted_heap_attachment {
> > > +struct sg_table*table;
> > > +};
> > > +
> > > static int
> > > restricted_heap_memory_allocate(struct restricted_heap *heap,
> struct restricted_buffer *buf)
> > > {
> > > @@ -45,6 +49,104 @@ restricted_heap_memory_free(struct
> restricted_heap *heap, struct restricted_buff
> > > ops->memory_free(heap, buf);
> > > }
> > >
> > > +static int restricted_heap_attach(struct dma_buf *dmabuf, struct
> dma_buf_attachment *attachment)
> > > +{
> > > +struct restricted_buffer *restricted_buf = dmabuf->priv;
> > > +struct restricted_heap_attachment *a;
> > > +struct sg_table *table;
> > > +int ret;
> > > +
> > > +a = kzalloc(sizeof(*a), GFP_KERNEL);
> > > +if (!a)
> > > +return -ENOMEM;
> > > +
> > > +table = kzalloc(sizeof(*table), GFP_KERNEL);
> > > +if (!table) {
> > > +ret = -ENOMEM;
> > > +goto err_free_attach;
> > > +}
> > > +
> > > +ret = sg_alloc_table(table, 1, GFP_KERNEL);
> > > +if (ret)
> > > +goto err_free_sgt;
> > > +sg_set_page(table->sgl, NULL, restricted_buf->size, 0);
> >
> > So this is definitely broken and violating the dma-buf api rules.
> You
> > cannot let attach succed and supply a dummy/invalid sg table.
> >
> > Two options:
> >
> > - Reject ->attach for all this buffers with -EBUSY and provide
> instead a
> > private api for these secure buffers, similar to how
> virtio_dma_buf has
> > private virto-specific apis. This interface would need to be
> > standardized across all arm TEE users, so that we don't have a
> > disastrous proliferation of apis.
> >
> > - Allow ->attach, but _only_ for drivers/devices which can access
> the
> > secure buffer correctly, and only if you can put the right secure
> buffer
> > address into the sg table directly. If dma to a secure buffer for
> a
> > given struct device * will not work correctly (i.e. without data
> > corruption), you _must_ reject the attach attempt with -EBUSY.
> >
> > The 2nd approach would be my preferred one, if it's technically
> possible.
> >
> > Also my understanding is that arm TEE is standardized, so I think
> we'll at
> > least want some acks from other soc people whether this will work
> for them
> > too.
> >
> > Finally the usual drill:
> > - this also needs the driver side support, if there's any changes
> needed.
> > Just the new heap isn't enough.
>
> Ok I quickly scrolled through your drm patches and that confirms that
> the
> current dma-buf interface you're implementing is just completely
> breaking
> the api. And you need to paper over that will all kinds of very icky
> special-casing.
>
> So definitely need to rethink the overall design between dma-buf
> heaps and
> drivers here.
Hi,
Thanks very much for the review, and sorry for reply so late. We
reconstructed our TEE commands so that the kernel can obtain the valid
PA/pages, then the sg operations can run normally.
I will send the next version.
Thanks.
> -Sima
>
> > - and for drm you need open userspace for this. Doesn't have to be
> the
> > full content protection decode pipeline, the drivers in drm that
> landed
> > secure buffer support thus far enabled it using the
> > EGL_EXT_protected_content extension using gl, which side steps
> all the
> > complications around content decryption keys and support
> >
> > Cheers, Sima
> >
> > > +
> > > +a->table = table;
> > > +attachment->priv = a;
> > > +
> > > +return 0;
> > > +
> > > +err_free_sgt:
> > > +kfree(table);
> > > +err_free_attach:
> > > +kfree(a);
> > > +return ret;
> > > +}
> > > +
> > > +static void restricted_heap_detach(struct dma_buf *dmabuf,
> struct dma_buf_attachment *attachment)
> > > +{
> > > +struct restricted_heap_attachment *a = attachment->priv;
> > > +
> > > +sg_free_table(a->table);
> > > +kfree(a->table);
> > > +kfree(a);
> > > +}
> > > +
> > > +static struct sg_table *
> > > +restricted_heap_map_dma_buf(struct dma_buf_attachment
> *attachment, enum dma_data_direction direct)
> > > +{
> > > +struct restricted_heap_attachment *a = attachment->priv;
> > > +struct sg_
Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap
On Fri, 2024-01-05 at 10:35 +0100, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 04.01.24 um 20:50 schrieb Jeffrey Kardatzke: > > Any feedback from maintainers on what their preference is? I'm > fine > > with 'restricted' as well, but the main reason we chose secure was > > because of its use in ARM nomenclature and this is more for ARM > usage > > than x86. > > Well AMD calls this "trusted", but I think that's just slightly > better > than "secure". > > +1 for using "restricted" cause that seems to match the technical > consequences. Thanks you all for the discussion and the conclusion. I will send v4 in this week with "restricted". > > Regards, > Christian. > > > > > The main difference with similar buffers on AMD/Intel is that with > > AMD/Intel the buffers are mappable and readable by the CPU in the > > kernel. The problem is their contents are encrypted so you get junk > > back if you do that. On ARM, the buffers are completely > inaccessible > > by the kernel and the memory controller prevents access to them > > completely from the kernel. > > > > There are also other use cases for this where the hypervisor is > what > > is controlling access (second stage in the MMU is providing > > isolation)and in that case I do agree that 'secure' would not > be > > the right terminology for those types of buffers. So I do agree > > something other than 'secure' is probably a better option overall. > > > > > > On Fri, Dec 22, 2023 at 1:40 AM Simon Ser > wrote: > >> On Wednesday, December 13th, 2023 at 15:16, Pekka Paalanen < > ppaala...@gmail.com> wrote: > >> > > It is protected/shielded/fortified from all the kernel and > userspace, > > but a more familiar word to describe that is inaccessible. > > "Inaccessible buffer" per se OTOH sounds like a useless > concept. > > > > It is not secure, because it does not involve security in any > way. In > > fact, given it's so fragile, I'd classify it as mildly opposite > of > > secure, as e.g. clients of a Wayland compositor can potentially > DoS the > > compositor with it by simply sending such a dmabuf. Or DoS the > whole > > system. > I hear what you are saying and DoS is a known problem and attack > vector, > but regardless, we have use cases where we don't want to expose > information in the clear and where we also would like to have > some > guarantees about correctness. That is where various secure > elements and > more generally security is needed. > > So, it sounds like we have two things here, the first is the > naming and > the meaning behind it. I'm pretty sure the people following and > contributing to this thread can agree on a name that makes > sense. Would > you personally be OK with "restricted" as the name? It sounds > like that. > >>> I would. I'm also just a by-stander, not a maintainer of kernel > >>> anything. I have no power to accept nor reject anything here. > >> I'd also personally be OK with "restricted", I think it's a lot > better > >> than "secure". > >> > >> In general I agree with everything Pekka said. >
Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range
On Sat, 2023-11-11 at 13:48 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 11/11/2023 12:15, Yong Wu wrote:
> > Add a binding for describing the secure CMA reserved memory range.
> The
> > memory range also will be defined in the TEE firmware. It means the
> TEE
> > will be configured with the same address/size that is being set in
> this
> > DT node.
> >
> > Signed-off-by: Yong Wu
> > ---
>
> What was the outcome of previous discussion? I don't see any
> references
> to the conclusion and your changelog "Reword the dt-binding
> description"
> is way too generic.
>
> You must explain what happened here.
I don't think there is a final conclusion yet in v1. Jeff helped
explain that this region also is defined in TEE firmware. I put this a
bit in the commit message above.
Sorry for confusing.
>
> > .../reserved-memory/secure_cma_region.yaml| 44
> +++
> > 1 file changed, 44 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > new file mode 100644
> > index ..8ab559595fbe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Secure Reserved CMA Region
Will change to: Secure Region. Is it ok?
> > +
> > +description:
> > + This binding describes a CMA region that can dynamically
> transition
>
> Describe the hardware or firmware, not the binding. Drop first four
> words and rephrase it.
Memory region for TEE usage, which is also defined in the TEE firmware.
When an activity (e.g. secure video playback) requiring usage of this
starts, this region will be protected by MPU (Memory Protect Unit) in
the TEE firmware. After the activity is completed, the region will be
unprotected by the TEE and usable by the non-secure side (i.e. kernel
and userspace).
Does this description make sense for you?
>
> > +between secure and non-secure states that a TEE can allocate
> memory
> > +from.
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for
> instructions).
> Maybe you need to update your dtschema and yamllint.
>
> Do not send untested code.
Sorry. I will update them and test this before sending.
>
> > +
> > +maintainers:
> > + - Yong Wu
> > +
> > +allOf:
> > + - $ref: reserved-memory.yaml
> > +
> > +properties:
> > + compatible:
> > +const: secure_cma_region
>
> Still wrong compatible. Look at other bindings - there is nowhere
> underscore. Look at other reserved memory bindings especially.
>
> Also, CMA is a Linux thingy, so either not suitable for bindings at
> all,
> or you need Linux specific compatible. I don't quite get why do you
> evennot
> put CMA there - adding Linux specific stuff will get obvious
> pushback...
Thanks. I will change to: secure-region. Is this ok?
>
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > +
>
> Stray blank line.
Thanks for reviewing so careful. Will fix this and below.
>
> > +reserved-memory {
> > +#address-cells = <1>;
> > +#size-cells = <1>;
> > +ranges;
> > +
> > +reserved-memory@8000 {
> > +compatible = "secure_cma_region";
> > +reusable;
> > +reg = <0x8000 0x1800>;
>
> reg is second property. Open DTS and check how it is there.
>
> > +};
> > +};
>
> Best regards,
> Krzysztof
>
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
On Wed, 2023-11-01 at 11:20 +0530, Jaskaran Singh wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 10/20/2023 3:20 PM, Yong Wu (吴勇) wrote: > > On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote: > >> > >> Instead of having a vendor specific binding for cma area, How > about > >> retrieving > >> > > > https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunih...@socionext.com/ > >> ? > >> dma_heap_add_cma can just associate cma region and create a heap. > So, > >> we can reuse cma heap > >> code for allocation instead of replicating that code here. > >> > > > > Thanks for the reference. I guess we can't use it. There are two > > reasons: > > > > a) The secure heap driver is a pure software driver and we have no > > device for it, therefore we cannot call dma_heap_add_cma. > > > > Hi Yong, > > We're considering using struct cma as the function argument to > dma_heap_add_cma() rather than struct device. Would this help > resolve the problem of usage with dma_heap_add_cma()? Yes. If we use "struct cma", I guess it works. > > > b) The CMA area here is dynamic for SVP. Normally this CMA can be > used > > in the kernel. In the SVP case we use cma_alloc to get it and pass > the > > entire CMA physical start address and size into TEE to protect the > CMA > > region. The original CMA heap cannot help with the TEE part. > > > > Referring the conversation at > https://lore.kernel.org/lkml/7a2995de23c24ef22c071c6976c02b97e9b50126.ca...@mediatek.com/ > ; > > since we're considering abstracting secure mem ops, would it make > sense > to use the default CMA heap ops (cma_heap_ops), allocate buffers from > it > and secure each allocated buffer? Then it looks you also need tee operation like tee_client_open_session and tee_client_invoke_func, right? It seems we also need change a bit for "cma_heap_allocate" to allow cma support operations from secure heap. I will send a v2 to move the discussion forward. The v2 is based on our case, It won't include the cma part. > > Thanks, > Jaskaran. > > > Thanks. > > > >> Thanks, > >> Vijay > >> > >> > >> >
Re: [PATCH 4/9] dma-buf: heaps: Initialise MediaTek secure heap
On Thu, 2023-10-26 at 10:18 +0530, Vijayanand Jitta wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 10/20/2023 3:29 PM, Yong Wu (吴勇) wrote:
> > On Thu, 2023-10-19 at 10:15 +0530, Vijayanand Jitta wrote:
> >>
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>
> >>
> >> On 9/11/2023 8:00 AM, Yong Wu wrote:
> >>> Initialise a mtk_svp heap. Currently just add a null heap,
> Prepare
> >> for
> >>> the later patches.
> >>>
> >>> Signed-off-by: Yong Wu
> >>> ---
> >>> drivers/dma-buf/heaps/Kconfig | 8 ++
> >>> drivers/dma-buf/heaps/Makefile | 1 +
> >>> drivers/dma-buf/heaps/mtk_secure_heap.c | 99
> >> +
[...]
> >>> +
> >>> +static struct dma_buf *
> >>> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> >>> + unsigned long fd_flags, unsigned long heap_flags)
> >>> +{
> >>> +struct mtk_secure_heap_buffer *sec_buf;
> >>> +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> >>> +struct dma_buf *dmabuf;
> >>> +int ret;
> >>> +
> >>> +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> >>
> >> As we know, kzalloc can only allocate 4MB at max. So, secure heap
> has
> >> this limitation.
> >> can we have a way to allocate more memory in secure heap ? maybe
> >> similar to how system heap does?
> >
> > This is just the size of a internal structure. I guess you mean the
> > secure memory size here. Regarding secure memory allocating flow,
> our
> > flow may be different with yours.
> >
> > Let me explain our flow, we have two secure buffer types(heaps).
> > a) mtk_svp
> > b) mtk_svp_cma which requires the cma binding.
> >
> > The memory management of both is inside the TEE. We only need to
> tell
> > the TEE which type and size of buffer we want, and then the TEE
> will
> > perform and return the memory handle to the kernel. The
> > kzalloc/alloc_pages is for the normal buffers.
> >
> > Regarding the CMA buffer, we only call cma_alloc once, and its
> > management is also within the TEE.
> >
>
> Thanks for the details.
>
> I see for mvp_svp, allocation is also specific to TEE, as TEE takes
> care of allocation as well.
Yes. The allocation management of these two heaps is in the TEE.
>
> I was thinking if allocation path can also be made generic ? without
> having
> dependency on TEE.
> For eg : A case where we want to allocate from kernel and secure that
> memory,
> the current secure heap design can't be used.
Sorry, This may be because our HW is special. The HW could protect a
certain region, but it can only protect 32 regions. So we cannot
allocate them in the kernel arbitrarily and then enter TEE to protect
them.
>
> Also i suppose TEE allocates contiguous memory for mtk_svp ? or does
> it support
> scattered memory ?
Yes. After the TEE runs for a period of time, the TEE memory will
become discontinuous, and a secure IOMMU exists in the TEE.
> >>
> >> Thanks,
> >> Vijay
> >>
Re: [PATCH 6/9] dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer allocating/freeing
Hi Vijayanand,
Thanks very much for your review.
On Thu, 2023-10-19 at 10:15 +0530, Vijayanand Jitta wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 9/11/2023 8:00 AM, Yong Wu wrote:
> > Add TEE service call for secure memory allocating/freeing.
> >
> > Signed-off-by: Anan Sun
> > Signed-off-by: Yong Wu
> > ---
> > drivers/dma-buf/heaps/mtk_secure_heap.c | 69
> -
> > 1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> buf/heaps/mtk_secure_heap.c
> > index e3da33a3d083..14c2a16a7164 100644
> > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > @@ -17,6 +17,9 @@
> >
> > #define MTK_TEE_PARAM_NUM4
> >
> > +#define TZCMD_MEM_SECURECM_UNREF7
> > +#define TZCMD_MEM_SECURECM_ZALLOC15
> > +
> > /*
> > * MediaTek secure (chunk) memory type
> > *
> > @@ -29,6 +32,8 @@ enum kree_mem_type {
> > struct mtk_secure_heap_buffer {
> > struct dma_heap*heap;
> > size_tsize;
> > +
> > +u32sec_handle;
> > };
> >
> > struct mtk_secure_heap {
> > @@ -80,6 +85,63 @@ static int mtk_kree_secure_session_init(struct
> mtk_secure_heap *sec_heap)
> > return ret;
> > }
> >
> > +static int
> > +mtk_sec_mem_tee_service_call(struct tee_context *tee_ctx, u32
> session,
> > + unsigned int command, struct tee_param *params)
> > +{
> > +struct tee_ioctl_invoke_arg arg = {0};
> > +int ret;
> > +
> > +arg.num_params = MTK_TEE_PARAM_NUM;
> > +arg.session = session;
> > +arg.func = command;
> > +
> > +ret = tee_client_invoke_func(tee_ctx, &arg, params);
> > +if (ret < 0 || arg.ret) {
> > +pr_err("%s: cmd %d ret %d:%x.\n", __func__, command, ret,
> arg.ret);
> > +ret = -EOPNOTSUPP;
> > +}
> > +return ret;
> > +}
> > +
> > +static int mtk_sec_mem_allocate(struct mtk_secure_heap *sec_heap,
> > +struct mtk_secure_heap_buffer *sec_buf)
> > +{
> > +struct tee_param params[MTK_TEE_PARAM_NUM] = {0};
> > +u32 mem_session = sec_heap->mem_session;
> > +int ret;
> > +
> > +params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +params[0].u.value.a = SZ_4K;/* alignment */
> > +params[0].u.value.b = sec_heap->mem_type;/* memory type */
> > +params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +params[1].u.value.a = sec_buf->size;
> > +params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
> > +
> > +/* Always request zeroed buffer */
> > +ret = mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,
> > + TZCMD_MEM_SECURECM_ZALLOC, params);
>
> I see here optee calls are being used to secure memory.
>
> For a secure heap, there can be multiple ways on how we want to
> secure memory,
> for eg : by using qcom_scm_assign_mem.
>
> This interface restricts securing memory to only optee calls.
> can we have a way to choose ops that we want to secure memory ?
Thanks for this suggestion. So it looks like there are four operations
in the abstract ops. Something like this?
struct sec_memory_ops {
int (*sec_memory_init)() //we need initialise tee session here.
int (*sec_memory_alloc)()
int (*sec_memory_free)()
void (*sec_memory_uninit)()
}
Do you also need tee operation like tee_client_open_session and
tee_client_invoke_func?
if so, your UUID and TEE command ID value are also different, right?
We may also need new macros on how to choose different sec_memory_ops
since we don't have different bindings.
>
> Thanks,
> Vijay
Re: [PATCH 4/9] dma-buf: heaps: Initialise MediaTek secure heap
On Thu, 2023-10-19 at 10:15 +0530, Vijayanand Jitta wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 9/11/2023 8:00 AM, Yong Wu wrote:
> > Initialise a mtk_svp heap. Currently just add a null heap, Prepare
> for
> > the later patches.
> >
> > Signed-off-by: Yong Wu
> > ---
> > drivers/dma-buf/heaps/Kconfig | 8 ++
> > drivers/dma-buf/heaps/Makefile | 1 +
> > drivers/dma-buf/heaps/mtk_secure_heap.c | 99
> +
> > 3 files changed, 108 insertions(+)
> > create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-
> buf/heaps/Kconfig
> > index a5eef06c4226..729c0cf3eb7c 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -12,3 +12,11 @@ config DMABUF_HEAPS_CMA
> >Choose this option to enable dma-buf CMA heap. This heap is
> backed
> >by the Contiguous Memory Allocator (CMA). If your system has
> these
> >regions, you should say Y here.
> > +
> > +config DMABUF_HEAPS_MTK_SECURE
> > +bool "DMA-BUF MediaTek Secure Heap"
> > +depends on DMABUF_HEAPS && TEE
> > +help
> > + Choose this option to enable dma-buf MediaTek secure heap for
> Secure
> > + Video Path. This heap is backed by TEE client interfaces. If in
> > + doubt, say N.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-
> buf/heaps/Makefile
> > index 974467791032..df559dbe33fe 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -1,3 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> > obj-$(CONFIG_DMABUF_HEAPS_CMA)+= cma_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)+= mtk_secure_heap.o
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> buf/heaps/mtk_secure_heap.c
> > new file mode 100644
> > index ..bbf1c8dce23e
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF mtk_secure_heap exporter
> > + *
> > + * Copyright (C) 2023 MediaTek Inc.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/*
> > + * MediaTek secure (chunk) memory type
> > + *
> > + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for
> trustzone.
> > + */
> > +enum kree_mem_type {
> > +KREE_MEM_SEC_CM_TZ = 1,
> > +};
> > +
> > +struct mtk_secure_heap_buffer {
> > +struct dma_heap*heap;
> > +size_tsize;
> > +};
> > +
> > +struct mtk_secure_heap {
> > +const char*name;
> > +const enum kree_mem_type mem_type;
> > +};
> > +
> > +static struct dma_buf *
> > +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> > + unsigned long fd_flags, unsigned long heap_flags)
> > +{
> > +struct mtk_secure_heap_buffer *sec_buf;
> > +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > +struct dma_buf *dmabuf;
> > +int ret;
> > +
> > +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
>
> As we know, kzalloc can only allocate 4MB at max. So, secure heap has
> this limitation.
> can we have a way to allocate more memory in secure heap ? maybe
> similar to how system heap does?
This is just the size of a internal structure. I guess you mean the
secure memory size here. Regarding secure memory allocating flow, our
flow may be different with yours.
Let me explain our flow, we have two secure buffer types(heaps).
a) mtk_svp
b) mtk_svp_cma which requires the cma binding.
The memory management of both is inside the TEE. We only need to tell
the TEE which type and size of buffer we want, and then the TEE will
perform and return the memory handle to the kernel. The
kzalloc/alloc_pages is for the normal buffers.
Regarding the CMA buffer, we only call cma_alloc once, and its
management is also within the TEE.
>
> Thanks,
> Vijay
>
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 9/11/2023 8:00 AM, Yong Wu wrote:
> > This adds the binding for describing a CMA memory for MediaTek
> SVP(Secure
> > Video Path).
> >
> > Signed-off-by: Yong Wu
> > ---
> > .../mediatek,secure_cma_chunkmem.yaml | 42
> +++
> > 1 file changed, 42 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> b/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> > new file mode 100644
> > index ..cc10e00d35c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Secure Video Path Reserved Memory
> > +
> > +description:
> > + This binding describes the reserved memory for secure video
> path.
> > +
> > +maintainers:
> > + - Yong Wu
> > +
> > +allOf:
> > + - $ref: reserved-memory.yaml
> > +
> > +properties:
> > + compatible:
> > +const: mediatek,secure_cma_chunkmem
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > +
> > +reserved-memory {
> > +#address-cells = <1>;
> > +#size-cells = <1>;
> > +ranges;
> > +
> > +reserved-memory@8000 {
> > +compatible = "mediatek,secure_cma_chunkmem";
> > +reusable;
> > +reg = <0x8000 0x1800>;
> > +};
> > +};
>
> Instead of having a vendor specific binding for cma area, How about
> retrieving
>
https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunih...@socionext.com/
> ?
> dma_heap_add_cma can just associate cma region and create a heap. So,
> we can reuse cma heap
> code for allocation instead of replicating that code here.
>
Thanks for the reference. I guess we can't use it. There are two
reasons:
a) The secure heap driver is a pure software driver and we have no
device for it, therefore we cannot call dma_heap_add_cma.
b) The CMA area here is dynamic for SVP. Normally this CMA can be used
in the kernel. In the SVP case we use cma_alloc to get it and pass the
entire CMA physical start address and size into TEE to protect the CMA
region. The original CMA heap cannot help with the TEE part.
Thanks.
> Thanks,
> Vijay
>
>
>
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
On Thu, 2023-10-12 at 09:07 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 12/10/2023 08:54, Yong Wu (吴勇) wrote: > > > > Thanks Jeffrey for the addition. > > > > Hi Rob, krzysztof, > > > > Just a ping. Is Jeffrey's reply ok for you? > > I did not see any patch posted and I am way behind reviewing patches > to > review also non-patches-patches... Sorry, I haven't sent a new version yet. I plan to prepare the new version after having a conclusion here. In Jeffrey's help reply, this memory range is defined in TEE firmware, thus this looks could be ok for a binding, right? Thanks. > > Best regards, > Krzysztof >
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
Thanks Jeffrey for the addition. Hi Rob, krzysztof, Just a ping. Is Jeffrey's reply ok for you? Thanks. On Tue, 2023-09-19 at 15:15 -0700, Jeffrey Kardatzke wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Mon, Sep 18, 2023 at 3:47 AM Yong Wu (吴勇) > wrote: > > > > On Tue, 2023-09-12 at 10:53 -0500, Rob Herring wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > On Tue, Sep 12, 2023 at 11:13:50AM +0100, Robin Murphy wrote: > > > > On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote: > > > > > On 12/09/2023 08:16, Yong Wu (吴勇) wrote: > > > > > > Hi Rob, > > > > > > > > > > > > Thanks for your review. > > > > > > > > > > > > On Mon, 2023-09-11 at 10:44 -0500, Rob Herring wrote: > > > > > > > > > > > > > > External email : Please do not click links or open > > > attachments until > > > > > > > you have verified the sender or the content. > > > > > > > On Mon, Sep 11, 2023 at 10:30:37AM +0800, Yong Wu > wrote: > > > > > > > > This adds the binding for describing a CMA memory for > > > MediaTek > > > > > > > SVP(Secure > > > > > > > > Video Path). > > > > > > > > > > > > > > CMA is a Linux thing. How is this related to CMA? > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Yong Wu > > > > > > > > --- > > > > > > > > .../mediatek,secure_cma_chunkmem.yaml | 42 > > > > > > > +++ > > > > > > > > 1 file changed, 42 insertions(+) > > > > > > > > create mode 100644 > > > Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > > > > > > > > > diff --git > a/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > b/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > new file mode 100644 > > > > > > > > index ..cc10e00d35c4 > > > > > > > > --- /dev/null > > > > > > > > +++ b/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > @@ -0,0 +1,42 @@ > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2- > Clause) > > > > > > > > +%YAML 1.2 > > > > > > > > +--- > > > > > > > > +$id: > > > > > > > > > > > http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml# > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > > + > > > > > > > > +title: MediaTek Secure Video Path Reserved Memory > > > > > > > > > > > > > > What makes this specific to Mediatek? Secure video path > is > > > fairly > > > > > > > common, right? > > > > > > > > > > > > Here we just reserve a buffer and would like to create a > dma- > > > buf secure > > > > > > heap for SVP, then the secure engines(Vcodec and DRM) could > > > prepare > > > > > > secure buffer through it. > > > > > > But the heap driver is pure SW driver, it is not platform > > > device and > > > > > > > > > > All drivers are pure SW. > > > > > > > > > > > we don't have a corresponding HW unit for it. Thus I don't > > > think I > > > > > > could create a platform dtsi node and use "memory-region" > > > pointer to > > > > > > the region. I used RESERVEDMEM_OF_DECLARE currently(The > code is > > > in > > > > > > [9/9]). Sorry if this is not right. > > > > > > > > > > If this is not for any hardware and you already
Re: [PATCH 4/9] dma-buf: heaps: Initialise MediaTek secure heap
On Wed, 2023-09-27 at 16:42 +0200, Joakim Bech wrote:
External email : Please do not click links or open attachments until you have
verified the sender or the content.
On Mon, Sep 11, 2023 at 10:30:33AM +0800, Yong Wu wrote:
> Initialise a mtk_svp heap. Currently just add a null heap, Prepare for
> the later patches.
>
> Signed-off-by: Yong Wu
> ---
> drivers/dma-buf/heaps/Kconfig | 8 ++
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/mtk_secure_heap.c | 99 +
> 3 files changed, 108 insertions(+)
> create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..729c0cf3eb7c 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,11 @@ config DMABUF_HEAPS_CMA
>Choose this option to enable dma-buf CMA heap. This heap is backed
>by the Contiguous Memory Allocator (CMA). If your system has these
>regions, you should say Y here.
> +
> +config DMABUF_HEAPS_MTK_SECURE
> +bool "DMA-BUF MediaTek Secure Heap"
> +depends on DMABUF_HEAPS && TEE
> +help
> + Choose this option to enable dma-buf MediaTek secure heap for Secure
> + Video Path. This heap is backed by TEE client interfaces. If in
Although this is intended for SVP right now, this is something that very
well could work for other use cases. So, I think I'd not mention "Secure
Video Path" and just mention "secure heap".
Thanks. I will remove the "SVP".
> + doubt, say N.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 974467791032..df559dbe33fe 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,3 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> obj-$(CONFIG_DMABUF_HEAPS_CMA)+= cma_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)+= mtk_secure_heap.o
> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> b/drivers/dma-buf/heaps/mtk_secure_heap.c
> new file mode 100644
> index ..bbf1c8dce23e
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF mtk_secure_heap exporter
> + *
> + * Copyright (C) 2023 MediaTek Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * MediaTek secure (chunk) memory type
> + *
> + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for trustzone.
nit: s/trustzone/TrustZone/
Will Fix.
Thanks.
--
// Regards
Joakim
> + */
> +enum kree_mem_type {
> +KREE_MEM_SEC_CM_TZ = 1,
> +};
> +
> +struct mtk_secure_heap_buffer {
> +struct dma_heap*heap;
> +size_tsize;
> +};
> +
> +struct mtk_secure_heap {
> +const char*name;
> +const enum kree_mem_type mem_type;
> +};
> +
> +static struct dma_buf *
> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> + unsigned long fd_flags, unsigned long heap_flags)
> +{
> +struct mtk_secure_heap_buffer *sec_buf;
> +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +struct dma_buf *dmabuf;
> +int ret;
> +
> +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> +if (!sec_buf)
> +return ERR_PTR(-ENOMEM);
> +
> +sec_buf->size = size;
> +sec_buf->heap = heap;
> +
> +exp_info.exp_name = dma_heap_get_name(heap);
> +exp_info.size = sec_buf->size;
> +exp_info.flags = fd_flags;
> +exp_info.priv = sec_buf;
> +
> +dmabuf = dma_buf_export(&exp_info);
> +if (IS_ERR(dmabuf)) {
> +ret = PTR_ERR(dmabuf);
> +goto err_free_buf;
> +}
> +
> +return dmabuf;
> +
> +err_free_buf:
> +kfree(sec_buf);
> +return ERR_PTR(ret);
> +}
> +
> +static const struct dma_heap_ops mtk_sec_heap_ops = {
> +.allocate= mtk_sec_heap_allocate,
> +};
> +
> +static struct mtk_secure_heap mtk_sec_heap[] = {
> +{
> +.name= "mtk_svp",
> +.mem_type= KREE_MEM_SEC_CM_TZ,
> +},
> +};
> +
> +static int mtk_sec_heap_init(void)
> +{
> +struct mtk_secure_heap *sec_heap = mtk_sec_heap;
> +struct dma_heap_export_info exp_info;
> +struct dma_heap *heap;
> +unsigned int i;
> +
> +for (i = 0; i < ARRAY_SIZE(mtk_sec_heap); i++, sec_heap++) {
> +exp_info.name = sec_heap->name;
> +exp_info.ops = &mtk_sec_heap_ops;
> +exp_info.priv = (void *)sec_heap;
> +
> +heap = dma_heap_add(&exp_info);
> +if (IS_ERR(heap))
> +return PTR_ERR(heap);
> +}
> +return 0;
> +}
> +
> +module_init(mtk_sec_heap_init);
> +MODULE_DESCRIPTION("MediaTek Secure Heap Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Re: [PATCH 6/9] dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer allocating/freeing
Hi Joakim,
Thanks very much for the reviewing.
On Wed, 2023-09-27 at 16:37 +0200, Joakim Bech wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Mon, Sep 11, 2023 at 10:30:35AM +0800, Yong Wu wrote:
> > Add TEE service call for secure memory allocating/freeing.
> >
> > Signed-off-by: Anan Sun
> > Signed-off-by: Yong Wu
> > ---
> > drivers/dma-buf/heaps/mtk_secure_heap.c | 69
> -
> > 1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> buf/heaps/mtk_secure_heap.c
> > index e3da33a3d083..14c2a16a7164 100644
> > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > @@ -17,6 +17,9 @@
> >
> > #define MTK_TEE_PARAM_NUM4
> >
> > +#define TZCMD_MEM_SECURECM_UNREF7
> > +#define TZCMD_MEM_SECURECM_ZALLOC15
> This is related to the discussion around UUID as well. These numbers
> here are specific to the MediaTek TA. If we could make things more
> generic, then these should probably be 0 and 1.
>
> I also find the naming a bit heavy, I think I'd suggest something
> like:
> # define TEE_CMD_SECURE_HEAP_ZALLOC ...
> and so on.
I will check internally and try to follow this. If we can not follow,
I'll give feedback here.
>
> > +
> > /*
> > * MediaTek secure (chunk) memory type
> > *
> > @@ -29,6 +32,8 @@ enum kree_mem_type {
> The "kree" here, is that meant to indicate kernel REE? If yes, then I
> guess that could be dropped since we know we're already in the kernel
> context, perhaps instead name it something with "secure_heap_type"?
>
> > struct mtk_secure_heap_buffer {
> > struct dma_heap*heap;
> > size_tsize;
> > +
> > +u32sec_handle;
> > };
> >
> > struct mtk_secure_heap {
> > @@ -80,6 +85,63 @@ static int mtk_kree_secure_session_init(struct
> mtk_secure_heap *sec_heap)
> > return ret;
> > }
> >
> > +static int
> > +mtk_sec_mem_tee_service_call(struct tee_context *tee_ctx, u32
> session,
> > + unsigned int command, struct tee_param *params)
> > +{
> > +struct tee_ioctl_invoke_arg arg = {0};
> > +int ret;
> > +
> > +arg.num_params = MTK_TEE_PARAM_NUM;
> > +arg.session = session;
> > +arg.func = command;
> > +
> > +ret = tee_client_invoke_func(tee_ctx, &arg, params);
> > +if (ret < 0 || arg.ret) {
> > +pr_err("%s: cmd %d ret %d:%x.\n", __func__, command, ret,
> arg.ret);
> > +ret = -EOPNOTSUPP;
> > +}
> > +return ret;
> > +}
> Perhaps not relevant for this patch set, but since this function is
> just
> a pure TEE call, I'm inclined to suggest that this could even be
> moved
> out as a generic TEE function. I.e., something that could be used
> elsewhere in the Linux kernel.
Good Suggestion. I've seen many places call this, and they are
basically similar. Do you mean we create a simple wrap for this?
something like this:
int tee_client_invoke_func_wrap(struct tee_context *ctx,
u32 session,
u32 func_id,
unsigned int num_params,
struct tee_param *param,
int *invoke_arg_ret/* OUT */)
If this makes sense, it should be done in a separate patchset.
>
> > +
> > +static int mtk_sec_mem_allocate(struct mtk_secure_heap *sec_heap,
> > +struct mtk_secure_heap_buffer *sec_buf)
> > +{
> > +struct tee_param params[MTK_TEE_PARAM_NUM] = {0};
> > +u32 mem_session = sec_heap->mem_session;
> How about name it tee_session? Alternative ta_session? I think that
> would better explain what it actually is.
Thanks for the renaming. Will change.
>
> > +int ret;
> > +
> > +params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +params[0].u.value.a = SZ_4K;/* alignment */
> > +params[0].u.value.b = sec_heap->mem_type;/* memory type */
> > +params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +params[1].u.value.a = sec_buf->size;
> > +params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
> > +
> > +/* Always request zeroed buffer */
> > +ret = mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,
> > + TZCMD_MEM_SECURECM_ZALLOC, params);
> > +if (ret)
> > +return -ENOMEM;
> > +
> > +sec_buf->sec_handle = params[2].u.value.a;
> > +return 0;
> > +}
> > +
> > +static void mtk_sec_mem_release(struct mtk_secure_heap *sec_heap,
> > +struct mtk_secure_heap_buffer *sec_buf)
> > +{
> > +struct tee_param params[MTK_TEE_PARAM_NUM] = {0};
> > +u32 mem_session = sec_heap->mem_session;
> > +
> > +params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +params[0].u.value.a = sec_buf->sec_handle;
> > +params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> Perhaps worth having a comment for params[1] explain why we need the
> VALUE_OUTPUT here?
Will do.
>
> --
> // Regards
> Joakim
>
> > +
> > +mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,
> > + TZCMD_MEM_SECURECM_UNREF, params);
>
Re: [PATCH 5/9] dma-buf: heaps: mtk_sec_heap: Initialise tee session
On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 11/09/23 04:30, Yong Wu ha scritto:
> > > > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> > > > work
> > > > here since this is not a platform driver, therefore initialise
> > > > the
> > > > TEE
> > > > context/session while we allocate the first secure buffer.
> > > >
> > > > Signed-off-by: Yong Wu
> > > > ---
> > > >drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > > > +
> > > >1 file changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > b/drivers/dma-
> > > > buf/heaps/mtk_secure_heap.c
> > > > index bbf1c8dce23e..e3da33a3d083 100644
> > > > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > @@ -10,6 +10,12 @@
> > > >#include
> > > >#include
> > > >#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#define TZ_TA_MEM_UUID "4477588a-8476-11e2-ad15-
> > > > e41f1390d676"
> > > > +
> > >
> > > Is this UUID the same for all SoCs and all TZ versions?
> >
> > Yes. It is the same for all SoCs and all TZ versions currently.
> >
>
> That's good news!
>
> Is this UUID used in any userspace component? (example: Android
> HALs?)
No. Userspace never use it. If userspace would like to allocate this
secure buffer, it can achieve through the existing dmabuf IOCTL via
/dev/dma_heap/mtk_svp node.
> If it is (and I somehow expect that it is), then this definition
> should go
> to a UAPI header, as suggested by Christian.
>
> Cheers!
>
> > >
> > > Thanks,
> > > Angelo
> > >
> > >
> > > > +#define MTK_TEE_PARAM_NUM 4
> > > >
> > > >/*
> > > > * MediaTek secure (chunk) memory type
> > > > @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
> > > >struct mtk_secure_heap {
> > > > const char *name;
> > > > const enum kree_mem_type mem_type;
> > > > + u32 mem_session;
> > > > + struct tee_context *tee_ctx;
> > > >};
> > > >
> > > > +static int mtk_optee_ctx_match(struct tee_ioctl_version_data
> > > > *ver,
> > > > const void *data)
> > > > +{
> > > > + return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > > > +}
> > > > +
> > > > +static int mtk_kree_secure_session_init(struct mtk_secure_heap
> > > > *sec_heap)
> > > > +{
> > > > + struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> > > > + struct tee_ioctl_open_session_arg arg = {0};
> > > > + uuid_t ta_mem_uuid;
> > > > + int ret;
> > > > +
> > > > + sec_heap->tee_ctx = tee_client_open_context(NULL,
> > > > mtk_optee_ctx_match,
> > > > + NULL,
> > > > NULL);
> > > > + if (IS_ERR(sec_heap->tee_ctx)) {
> > > > + pr_err("%s: open context failed, ret=%ld\n",
> > > > sec_heap-
> > > > > name,
> > > >
> > > > + PTR_ERR(sec_heap->tee_ctx));
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + arg.num_params = MTK_TEE_PARAM_NUM;
> > > > + arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > > + ret = uuid_parse(TZ_TA_MEM_UUID, &ta_mem_uuid);
> > > > + if (ret)
> > > > + goto close_context;
> > > > + memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid));
> > > > +
> > > > + ret = tee_client_open_session(sec_heap->tee_ctx, &arg,
> > > > t_param);
> > > > + if (ret < 0 || arg.ret) {
> > > > + pr_err("%s: open session failed, ret=%d:%d\n",
> > > > + sec_heap->name, ret, arg.ret
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
On Tue, 2023-09-12 at 10:53 -0500, Rob Herring wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Tue, Sep 12, 2023 at 11:13:50AM +0100, Robin Murphy wrote: > > On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote: > > > On 12/09/2023 08:16, Yong Wu (吴勇) wrote: > > > > Hi Rob, > > > > > > > > Thanks for your review. > > > > > > > > On Mon, 2023-09-11 at 10:44 -0500, Rob Herring wrote: > > > > > > > > > > External email : Please do not click links or open > attachments until > > > > > you have verified the sender or the content. > > > > > On Mon, Sep 11, 2023 at 10:30:37AM +0800, Yong Wu wrote: > > > > > > This adds the binding for describing a CMA memory for > MediaTek > > > > > SVP(Secure > > > > > > Video Path). > > > > > > > > > > CMA is a Linux thing. How is this related to CMA? > > > > > > > > > > > > > > > > Signed-off-by: Yong Wu > > > > > > --- > > > > > > .../mediatek,secure_cma_chunkmem.yaml | 42 > > > > > +++ > > > > > > 1 file changed, 42 insertions(+) > > > > > > create mode 100644 > Documentation/devicetree/bindings/reserved- > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/reserved- > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > b/Documentation/devicetree/bindings/reserved- > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > new file mode 100644 > > > > > > index ..cc10e00d35c4 > > > > > > --- /dev/null > > > > > > +++ b/Documentation/devicetree/bindings/reserved- > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > @@ -0,0 +1,42 @@ > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > +%YAML 1.2 > > > > > > +--- > > > > > > +$id: > > > > > > http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml# > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > + > > > > > > +title: MediaTek Secure Video Path Reserved Memory > > > > > > > > > > What makes this specific to Mediatek? Secure video path is > fairly > > > > > common, right? > > > > > > > > Here we just reserve a buffer and would like to create a dma- > buf secure > > > > heap for SVP, then the secure engines(Vcodec and DRM) could > prepare > > > > secure buffer through it. > > > > But the heap driver is pure SW driver, it is not platform > device and > > > > > > All drivers are pure SW. > > > > > > > we don't have a corresponding HW unit for it. Thus I don't > think I > > > > could create a platform dtsi node and use "memory-region" > pointer to > > > > the region. I used RESERVEDMEM_OF_DECLARE currently(The code is > in > > > > [9/9]). Sorry if this is not right. > > > > > > If this is not for any hardware and you already understand this > (since > > > you cannot use other bindings) then you cannot have custom > bindings for > > > it either. > > > > > > > > > > > Then in our usage case, is there some similar method to do > this? or > > > > any other suggestion? > > > > > > Don't stuff software into DTS. > > > > Aren't most reserved-memory bindings just software policy if you > look at it > > that way, though? IIUC this is a pool of memory that is visible and > > available to the Non-Secure OS, but is fundamentally owned by the > Secure > > TEE, and pages that the TEE allocates from it will become > physically > > inaccessible to the OS. Thus the platform does impose constraints > on how the > > Non-Secure OS may use it, and per the rest of the reserved-memory > bindings, > > describing it as a "reusable" reservation seems entirely > appropriate. If > > anything that's *more* platform-related and so DT-relevant than > typical > > arbitrary reservations which just represent "
Re: [PATCH 3/9] dma-heap: Provide accessors so that in-kernel drivers can allocate dmabufs from specific heaps
On Tue, 2023-09-12 at 11:05 -0400, Nicolas Dufresne wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Le mardi 12 septembre 2023 à 08:47 +0000, Yong Wu (吴勇) a écrit : > > On Mon, 2023-09-11 at 12:12 -0400, Nicolas Dufresne wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > Hi, > > > > > > Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit : > > > > From: John Stultz > > > > > > > > This allows drivers who don't want to create their own > > > > DMA-BUF exporter to be able to allocate DMA-BUFs directly > > > > from existing DMA-BUF Heaps. > > > > > > > > There is some concern that the premise of DMA-BUF heaps is > > > > that userland knows better about what type of heap memory > > > > is needed for a pipeline, so it would likely be best for > > > > drivers to import and fill DMA-BUFs allocated by userland > > > > instead of allocating one themselves, but this is still > > > > up for debate. > > > > > > > > > Would be nice for the reviewers to provide the information about > the > > > user of > > > this new in-kernel API. I noticed it because I was CCed, but > > > strangely it didn't > > > make it to the mailing list yet and its not clear in the cover > what > > > this is used > > > with. > > > > > > I can explain in my words though, my read is that this is used to > > > allocate both > > > user visible and driver internal memory segments in MTK VCODEC > > > driver. > > > > > > I'm somewhat concerned that DMABuf objects are used to abstract > > > secure memory > > > allocation from tee. For framebuffers that are going to be > exported > > > and shared > > > its probably fair use, but it seems that internal shared memory > and > > > codec > > > specific reference buffers also endup with a dmabuf fd (often > called > > > a secure fd > > > in the v4l2 patchset) for data that is not being shared, and > requires > > > a 1:1 > > > mapping to a tee handle anyway. Is that the design we'd like to > > > follow ? > > > > Yes. basically this is right. > > > > > Can't > > > we directly allocate from the tee, adding needed helper to make > this > > > as simple > > > as allocating from a HEAP ? > > > > If this happens, the memory will always be inside TEE. Here we > create a > > new _CMA heap, it will cma_alloc/free dynamically. Reserve it > before > > SVP start, and release to kernel after SVP done. > > Ok, I see the benefit of having a common driver then. It would add to > the > complexity, but having a driver for the tee allocator and v4l2/heaps > would be > another option? It's ok for v4l2. But our DRM also use this new heap and it will be sent upstream in the next few days. > > > > > Secondly. the v4l2/drm has the mature driver control flow, like > > drm_gem_prime_import_dev that always use dma_buf ops. So we can use > the > > current flow as much as possible without having to re-plan a flow > in > > the TEE. > > From what I've read from Yunfei series, this is only partially true > for V4L2. > The vb2 queue MMAP feature have dmabuf exportation as optional, but > its not a > problem to always back it up with a dmabuf object. But for internal > SHM buffers > used for firmware communication, I've never seen any driver use a > DMABuf. > > Same applies for primary decode buffers when frame buffer compression > or post- > processing it used (or reconstruction buffer in encoders), these are > not user > visible and are usually not DMABuf. If they aren't dmabuf, of course it is ok. I guess we haven't used these. The SHM buffer is got by tee_shm_register_kernel_buf in this case and we just use the existed dmabuf ops to complete SVP. In our case, the vcodec input/output/working buffers and DRM input buffer all use this new secure heap during secure video play. > > > > > > > > > Nicolas > > > > > > > > > > > Signed-off-by: John Stultz > > > > Signed-off-by: T.J. Mercier > > > > Signed-off-by: Yong Wu > > > > [Yong: Fix the checkpatch alignment warning] > > > > --- > > > > drivers/dma-buf/dma-heap.c | 60 > > > > -- > > > > include/linux/dma-heap.h | 25 > > > > 2 files changed, 69 insertions(+), 16 deletions(-) > > > > > > [snip] >
Re: [PATCH 3/9] dma-heap: Provide accessors so that in-kernel drivers can allocate dmabufs from specific heaps
On Tue, 2023-09-12 at 09:06 +0200, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 11.09.23 um 20:29 schrieb John Stultz: > > On Mon, Sep 11, 2023 at 3:14 AM Christian König > > wrote: > >> Am 11.09.23 um 04:30 schrieb Yong Wu: > >>> From: John Stultz > >>> > >>> This allows drivers who don't want to create their own > >>> DMA-BUF exporter to be able to allocate DMA-BUFs directly > >>> from existing DMA-BUF Heaps. > >>> > >>> There is some concern that the premise of DMA-BUF heaps is > >>> that userland knows better about what type of heap memory > >>> is needed for a pipeline, so it would likely be best for > >>> drivers to import and fill DMA-BUFs allocated by userland > >>> instead of allocating one themselves, but this is still > >>> up for debate. > >> The main design goal of having DMA-heaps in the first place is to > avoid > >> per driver allocation and this is not necessary because userland > know > >> better what type of memory it wants. > >> > >> The background is rather that we generally want to decouple > allocation > >> from having a device driver connection so that we have better > chance > >> that multiple devices can work with the same memory. > > Yep, very much agreed, and this is what the comment above is trying > to describe. > > > > Ideally user-allocated buffers would be used to ensure driver's > don't > > create buffers with constraints that limit which devices the > buffers > > might later be shared with. > > > > However, this patch was created as a hold-over from the old ION > logic > > to help vendors transition to dmabuf heaps, as vendors had > situations > > where they still wanted to export dmabufs that were not to be > > generally shared and folks wanted to avoid duplication of logic > > already in existing heaps. At the time, I never pushed it upstream > as > > there were no upstream users. But I think if there is now a > potential > > upstream user, it's worth having the discussion to better > understand > > the need. > > Yeah, that indeed makes much more sense. > > When existing drivers want to avoid their own handling and move > their > memory management over to using DMA-heaps even for internal > allocations > then no objections from my side. That is certainly something we > should > aim for if possible. Thanks. > > But what we should try to avoid is that newly merged drivers provide > both a driver specific UAPI and DMA-heaps. The justification that > this > makes it easier to transit userspace to the new UAPI doesn't really > count. > > That would be adding UAPI already with a plan to deprecate it and > that > is most likely not helpful considering that UAPI must be supported > forever as soon as it is upstream. Sorry, I didn't understand this. I think we have not change the UAPI. Which code are you referring to? > > > So I think this patch is a little confusing in this series, as I > don't > > see much of it actually being used here (though forgive me if I'm > > missing it). > > > > Instead, It seems it get used in a separate patch series here: > > > https://lore.kernel.org/all/20230911125936.10648-1-yunfei.d...@mediatek.com/ > > Please try to avoid stuff like that it is really confusing and eats > reviewers time. My fault, I thought dma-buf and media belonged to the different tree, so I send them separately. The cover letter just said "The consumers of the new heap and new interface are our codecs and DRM, which will be sent upstream soon", and there was no vcodec link at that time. In the next version, we will put the first three patches into the vcodec patchset. Thanks. > > Regards, > Christian. > > > > > Yong, I appreciate you sending this out! But maybe if the secure > heap > > submission doesn't depend on this functionality, I might suggest > > moving this patch (or at least the majority of it) to be part of > the > > vcodec series instead? That way reviewers will have more context > for > > how the code being added is used? Will do. Thanks. > > > > thanks > > -john >
Re: [PATCH 3/9] dma-heap: Provide accessors so that in-kernel drivers can allocate dmabufs from specific heaps
On Mon, 2023-09-11 at 12:12 -0400, Nicolas Dufresne wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi, > > Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit : > > From: John Stultz > > > > This allows drivers who don't want to create their own > > DMA-BUF exporter to be able to allocate DMA-BUFs directly > > from existing DMA-BUF Heaps. > > > > There is some concern that the premise of DMA-BUF heaps is > > that userland knows better about what type of heap memory > > is needed for a pipeline, so it would likely be best for > > drivers to import and fill DMA-BUFs allocated by userland > > instead of allocating one themselves, but this is still > > up for debate. > > > Would be nice for the reviewers to provide the information about the > user of > this new in-kernel API. I noticed it because I was CCed, but > strangely it didn't > make it to the mailing list yet and its not clear in the cover what > this is used > with. > > I can explain in my words though, my read is that this is used to > allocate both > user visible and driver internal memory segments in MTK VCODEC > driver. > > I'm somewhat concerned that DMABuf objects are used to abstract > secure memory > allocation from tee. For framebuffers that are going to be exported > and shared > its probably fair use, but it seems that internal shared memory and > codec > specific reference buffers also endup with a dmabuf fd (often called > a secure fd > in the v4l2 patchset) for data that is not being shared, and requires > a 1:1 > mapping to a tee handle anyway. Is that the design we'd like to > follow ? Yes. basically this is right. > Can't > we directly allocate from the tee, adding needed helper to make this > as simple > as allocating from a HEAP ? If this happens, the memory will always be inside TEE. Here we create a new _CMA heap, it will cma_alloc/free dynamically. Reserve it before SVP start, and release to kernel after SVP done. Secondly. the v4l2/drm has the mature driver control flow, like drm_gem_prime_import_dev that always use dma_buf ops. So we can use the current flow as much as possible without having to re-plan a flow in the TEE. > > Nicolas > > > > > Signed-off-by: John Stultz > > Signed-off-by: T.J. Mercier > > Signed-off-by: Yong Wu > > [Yong: Fix the checkpatch alignment warning] > > --- > > drivers/dma-buf/dma-heap.c | 60 > -- > > include/linux/dma-heap.h | 25 > > 2 files changed, 69 insertions(+), 16 deletions(-) > > [snip]
Re: [PATCH 5/9] dma-buf: heaps: mtk_sec_heap: Initialise tee session
On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/23 04:30, Yong Wu ha scritto:
> > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
> > here since this is not a platform driver, therefore initialise the
> > TEE
> > context/session while we allocate the first secure buffer.
> >
> > Signed-off-by: Yong Wu
> > ---
> > drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > +
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> > buf/heaps/mtk_secure_heap.c
> > index bbf1c8dce23e..e3da33a3d083 100644
> > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > @@ -10,6 +10,12 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > +
> > +#define TZ_TA_MEM_UUID "4477588a-8476-11e2-ad15-
> > e41f1390d676"
> > +
>
> Is this UUID the same for all SoCs and all TZ versions?
Yes. It is the same for all SoCs and all TZ versions currently.
>
> Thanks,
> Angelo
>
>
> > +#define MTK_TEE_PARAM_NUM 4
> >
> > /*
> >* MediaTek secure (chunk) memory type
> > @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
> > struct mtk_secure_heap {
> > const char *name;
> > const enum kree_mem_type mem_type;
> > + u32 mem_session;
> > + struct tee_context *tee_ctx;
> > };
> >
> > +static int mtk_optee_ctx_match(struct tee_ioctl_version_data *ver,
> > const void *data)
> > +{
> > + return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static int mtk_kree_secure_session_init(struct mtk_secure_heap
> > *sec_heap)
> > +{
> > + struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> > + struct tee_ioctl_open_session_arg arg = {0};
> > + uuid_t ta_mem_uuid;
> > + int ret;
> > +
> > + sec_heap->tee_ctx = tee_client_open_context(NULL,
> > mtk_optee_ctx_match,
> > + NULL, NULL);
> > + if (IS_ERR(sec_heap->tee_ctx)) {
> > + pr_err("%s: open context failed, ret=%ld\n", sec_heap-
> > >name,
> > + PTR_ERR(sec_heap->tee_ctx));
> > + return -ENODEV;
> > + }
> > +
> > + arg.num_params = MTK_TEE_PARAM_NUM;
> > + arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > + ret = uuid_parse(TZ_TA_MEM_UUID, &ta_mem_uuid);
> > + if (ret)
> > + goto close_context;
> > + memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid));
> > +
> > + ret = tee_client_open_session(sec_heap->tee_ctx, &arg,
> > t_param);
> > + if (ret < 0 || arg.ret) {
> > + pr_err("%s: open session failed, ret=%d:%d\n",
> > + sec_heap->name, ret, arg.ret);
> > + ret = -EINVAL;
> > + goto close_context;
> > + }
> > + sec_heap->mem_session = arg.session;
> > + return 0;
> > +
> > +close_context:
> > + tee_client_close_context(sec_heap->tee_ctx);
> > + return ret;
> > +}
> > +
> > static struct dma_buf *
> > mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> > unsigned long fd_flags, unsigned long heap_flags)
> > {
> > + struct mtk_secure_heap *sec_heap = dma_heap_get_drvdata(heap);
> > struct mtk_secure_heap_buffer *sec_buf;
> > DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > struct dma_buf *dmabuf;
> > int ret;
> >
> > + /*
> > +* TEE probe may be late. Initialise the secure session in the
> > first
> > +* allocating secure buffer.
> > +*/
> > + if (!sec_heap->mem_session) {
> > + ret = mtk_kree_secure_session_init(sec_heap);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + }
> > +
> > sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> > if (!sec_buf)
> > return ERR_PTR(-ENOMEM);
>
>
Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
Hi Rob,
Thanks for your review.
On Mon, 2023-09-11 at 10:44 -0500, Rob Herring wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Mon, Sep 11, 2023 at 10:30:37AM +0800, Yong Wu wrote:
> > This adds the binding for describing a CMA memory for MediaTek
> SVP(Secure
> > Video Path).
>
> CMA is a Linux thing. How is this related to CMA?
> >
> > Signed-off-by: Yong Wu
> > ---
> > .../mediatek,secure_cma_chunkmem.yaml | 42
> +++
> > 1 file changed, 42 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> b/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> > new file mode 100644
> > index ..cc10e00d35c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-
> memory/mediatek,secure_cma_chunkmem.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Secure Video Path Reserved Memory
>
> What makes this specific to Mediatek? Secure video path is fairly
> common, right?
Here we just reserve a buffer and would like to create a dma-buf secure
heap for SVP, then the secure engines(Vcodec and DRM) could prepare
secure buffer through it.
But the heap driver is pure SW driver, it is not platform device and
we don't have a corresponding HW unit for it. Thus I don't think I
could create a platform dtsi node and use "memory-region" pointer to
the region. I used RESERVEDMEM_OF_DECLARE currently(The code is in
[9/9]). Sorry if this is not right.
Then in our usage case, is there some similar method to do this? or
any other suggestion?
Appreciate in advance.
>
> > +
> > +description:
> > + This binding describes the reserved memory for secure video
> path.
> > +
> > +maintainers:
> > + - Yong Wu
> > +
> > +allOf:
> > + - $ref: reserved-memory.yaml
> > +
> > +properties:
> > + compatible:
> > +const: mediatek,secure_cma_chunkmem
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > +
> > +reserved-memory {
> > +#address-cells = <1>;
> > +#size-cells = <1>;
> > +ranges;
> > +
> > +reserved-memory@8000 {
> > +compatible = "mediatek,secure_cma_chunkmem";
> > +reusable;
> > +reg = <0x8000 0x1800>;
> > +};
> > +};
> > --
> > 2.25.1
> >
Re: [PATCH v7 00/12] Clean up "mediatek,larb"
Hi Matthias, Just ping for this patchset. Thanks. On Fri, 2021-07-30 at 10:52 +0800, Yong Wu wrote: > MediaTek IOMMU block diagram always like below: > > M4U > | > smi-common > | > - > | | ... > | | > larb1 larb2 > | | > vdec venc > > All the consumer connect with smi-larb, then connect with smi-common. > > When the consumer works, it should enable the smi-larb's power which > also > need enable the smi-common's power firstly. > > Thus, Firstly, use the device link connect the consumer and the > smi-larbs. then add device link between the smi-larb and smi-common. > > After adding the device_link, then "mediatek,larb" property can be > removed. > the iommu consumer don't need call the mtk_smi_larb_get/put to enable > the power and clock of smi-larb and smi-common. > > Base on v5.14-rc1, and a jpeg[1] and mdp[2] patchset. > > [1] > https://lore.kernel.org/linux-mediatek/20210702102304.3346429-1-hsi...@chromium.org/ > [2] > https://lore.kernel.org/linux-mediatek/20210709022324.1607884-1-ei...@chromium.org/ > > Change notes: > v7: 1) Fix a arm32 boot fail issue. reported from Frank. > 2) Add a return fail in the mtk drm. suggested by Dafna.
Re: [PATCH v7 00/12] Clean up "mediatek,larb"
On Mon, 2021-08-02 at 11:51 +0200, Joerg Roedel wrote: > On Fri, Jul 30, 2021 at 10:52:26AM +0800, Yong Wu wrote: > > .../display/mediatek/mediatek,disp.txt| 9 > > .../bindings/media/mediatek-jpeg-decoder.yaml | 9 > > .../bindings/media/mediatek-jpeg-encoder.yaml | 9 > > .../bindings/media/mediatek-mdp.txt | 8 > > .../bindings/media/mediatek-vcodec.txt| 4 -- > > arch/arm/boot/dts/mt2701.dtsi | 2 - > > arch/arm/boot/dts/mt7623n.dtsi| 5 -- > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 16 --- > > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 6 --- > > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 9 +++- > > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 9 +++- > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 15 +++--- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 36 +-- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 - > > drivers/gpu/drm/mediatek/mtk_drm_drv.c| 5 +- > > drivers/iommu/mtk_iommu.c | 24 +- > > drivers/iommu/mtk_iommu_v1.c | 31 - > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 45 + > > - > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 2 - > > drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 46 + > > -- > > drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 - > > drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 1 - > > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 37 ++- > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 -- > > .../platform/mtk-vcodec/mtk_vcodec_enc.c | 1 - > > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 44 ++- > > --- > > drivers/memory/mtk-smi.c | 14 -- > > include/soc/mediatek/smi.h| 20 > > 28 files changed, 92 insertions(+), 321 deletions(-) > > So this is likely not going through the IOMMU tree, given Matthias > has > reviewed the IOMMU changes you can add my Acked-by: Joerg Roedel < > jroe...@suse.de> Hi Joerg, Thanks very much for your confirm. I will your Ack for iommu part in the next version.
Re: [PATCH v7 04/12] iommu/mediatek: Add device_link between the consumer and the larb devices
On Thu, 2021-08-05 at 15:22 +0200, Dafna Hirschfeld wrote: > > On 30.07.21 04:52, Yong Wu wrote: > > MediaTek IOMMU-SMI diagram is like below. all the consumer connect > > with > > smi-larb, then connect with smi-common. > > > > M4U > > | > > smi-common > > | > >- > >| |... > >| | > > larb1 larb2 > >| | > > vdec venc > > > > When the consumer works, it should enable the smi-larb's power > > which > > also need enable the smi-common's power firstly. > > > > Thus, First of all, use the device link connect the consumer and > > the > > smi-larbs. then add device link between the smi-larb and smi- > > common. > > > > This patch adds device_link between the consumer and the larbs. > > > > When device_link_add, I add the flag DL_FLAG_STATELESS to avoid > > calling > > pm_runtime_xx to keep the original status of clocks. It can avoid > > two > > issues: > > 1) Display HW show fastlogo abnormally reported in [1]. At the > > beggining, > > all the clocks are enabled before entering kernel, but the clocks > > for > > display HW(always in larb0) will be gated after clk_enable and > > clk_disable > > called from device_link_add(->pm_runtime_resume) and rpm_idle. The > > clock > > operation happened before display driver probe. At that time, the > > display > > HW will be abnormal. > > > > 2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip > > pm_runtime_xx to avoid the deadlock. > > > > Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then > > device_link_removed should be added explicitly. > > > > [1] > > https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/ > > [2] https://lore.kernel.org/patchwork/patch/1086569/ > > > > Suggested-by: Tomasz Figa > > Signed-off-by: Yong Wu > > Tested-by: Dafna Hirschfeld # on > > mt8173 > > Hi, unfortunately, I have to take back the Tested-by tag. sorry for inconvenience you. (and sorry for reply late, there is something wrong about my local mail server.) > I am now testing the mtk-vcodec with latest kernel + patches sent > from the mailing list: > https://gitlab.collabora.com/eballetbo/linux/-/commits/topic/chromeos/chromeos-5.14 > which includes this patchset. > > On chromeos I open a video conference with googl-meet which cause the > mtk-vcodec vp8 encoder to run. > If I kill it with `killall -9 chrome` I get some page fault messages > from the iommu: Does the "git bisect" point to this patch? If you don't kill it, Does it also have these error below? I don't know what happen about "killall -9 chrome', Does it cause freeing some buffer? > > [ 837.255952] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read This means "larb0 port0" translation fault. If I am not wrong, you work at mt8173, from [0], this is DISP_OVL0. May be "killall -9 chrome" free the buffer(iova:0xfcff) that DISP_OVL is accessing, then iommu complain it is not a valid iova. [0] https://elixir.bootlin.com/linux/v5.14-rc1/source/include/dt-bindings/memory/mt8173-larb-port.h#L19 > [ 837.265696] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.282367] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.299028] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.315683] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.332345] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.349004] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.365665] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.382329] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > [ 837.42] mtk-iommu 10205000.iommu: fault type=0x5 > iova=0xfcff0001 pa=0x0 larb=0 port=0 layer=1 read > > In addition, running the encoder tests from the shell: > > sudo --user=#1000 /usr/local/libexec/chrome-binary- > tests/video_encode_accelerator_tests -- > gtest_filter=VideoEncoderTest.FlushAtEndOfStream_Multiple* > --codec=vp8 > /usr/local/share/tast/data/chromiumos/tast/local/bundles/cros/video/d > ata/tulip2-320x180.yuv --disable_validator > > At some point it fails with the error > > [ 5472.161821] [MTK_V4L2][ERROR] mtk_vcodec_wait_for_done_ctx:32: > [290] ctx->type=1, cmd=1, wait_event_interruptible_timeout > time=1000ms out 0 0! > [ 5472.174678] [MTK_VCODEC][ERROR][290]: vp8_enc_encode_frame() > irq_status=0 failed > [ 5472.182687] [MTK_V4L2][ERROR] mtk_venc_worker:1239: venc_if_encode > failed=-5 +our venc guy Irui. This looks VENC HW don't start to work. Does this caused by this patchset? this patchset only change the flow of pow
