Message-ID: <3e2dc2f248cf159a53c927eeb164967ad3b3c09d.camel@mediatek.com>
Date: Wed, 15 May 2024 05:43:17 +0000
From: Yong Wu (吴勇) <Yong.Wu@...iatek.com>
To: "joakim.bech@...aro.org" <joakim.bech@...aro.org>
CC: "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>, "jstultz@...gle.com"
	<jstultz@...gle.com>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "linaro-mm-sig@...ts.linaro.org"
	<linaro-mm-sig@...ts.linaro.org>, "christian.koenig@....com"
	<christian.koenig@....com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "quic_vjitta@...cinc.com"
	<quic_vjitta@...cinc.com>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, Kuohong Wang (王國鴻)
	<kuohong.wang@...iatek.com>, "pavel@....cz" <pavel@....cz>,
	"robin.murphy@....com" <robin.murphy@....com>, "contact@...rsion.fr"
	<contact@...rsion.fr>, "jkardatzke@...gle.com" <jkardatzke@...gle.com>,
	"Brian.Starkey@....com" <Brian.Starkey@....com>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "benjamin.gaignard@...labora.com"
	<benjamin.gaignard@...labora.com>, "tjmercier@...gle.com"
	<tjmercier@...gle.com>, "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "ppaalanen@...il.com" <ppaalanen@...il.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>,
	Youlin Pei (裴友林) <youlin.pei@...iatek.com>,
	Jianjiao Zeng (曾健姣) <Jianjiao.Zeng@...iatek.com>
Subject: 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 <yong.wu@...iatek.com>
> > ---
> >  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 to a comment on the prior patch. The name here is "heap"
> for
> restricted_heap, but below you use rstrd_heap. It's the same struct,
> so I would
> advise to use the same name to avoid confusion when reading the code.
> As
> mentioned before, I think the name "rheap" would be a good choice.

I will use "rheap" to replace everywhere.

Thanks.

> 
> >  };
> >  
> >  int restricted_heap_add(struct restricted_heap *rstrd_heap);
> > -- 
> > 2.25.1
> > 
> 
> -- 
> // Regards
> Joakim

Powered by blists - more mailing lists