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 <[email protected]>
> > ---
> > 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