Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops

[Jeffrey Kardatzke]

On Fri, Jan 12, 2024 at 2:52 PM John Stultz <jstultz@xxxxxxxxxx> wrote:
>
> On Fri, Jan 12, 2024 at 1:21 AM Yong Wu <yong.wu@xxxxxxxxxxxx> 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
> > be part of memory_alloc.
> >
> > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> > ---
> >  drivers/dma-buf/heaps/restricted_heap.c | 41 ++++++++++++++++++++++++-
> >  drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++
> >  2 files changed, 52 insertions(+), 1 deletion(-)
> >
>
> Thanks for sending this out! A thought below.
>
> > 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 {
> > +       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);
> >  };
> >
> >  int restricted_heap_add(struct restricted_heap *rstrd_heap);
>
> So, I'm a little worried here, because you're basically turning the
> restricted_heap dma-buf heap driver into a framework itself.
> Where this patch is creating a subdriver framework.
>
> Part of my hesitancy, is you're introducing this under the dma-buf
> heaps. For things like CMA, that's more of a core subsystem that has
> multiple users, and exporting cma buffers via dmabuf heaps is just an
> additional interface.  What I like about that is the core kernel has
> to define the semantics for the memory type and then the dmabuf heap
> is just exporting that well understood type of buffer.
>
> But with these restricted buffers, I'm not sure there's yet a well
> understood set of semantics nor a central abstraction for that which
> other drivers use directly.
>
> I know part of this effort here is to start to centralize all these
> vendor specific restricted buffer implementations, which I think is
> great, but I just worry that in doing it in the dmabuf heap interface,
> it is a bit too user-facing. The likelihood of encoding a particular
> semantic to the singular "restricted_heap" name seems high.

In patch #5 it has the actual driver implementation for the MTK heap
that includes the heap name of "restricted_mtk_cm", so there shouldn't
be a heap named "restricted_heap" that's actually getting exposed. The
restricted_heap code is a framework, and not a driver itself.  Unless
I'm missing something in this patchset...but that's the way it's
*supposed* to be.

>
> Similarly we might run into systems with multiple types of restricted
> buffers (imagine a discrete gpu having one type along with TEE
> protected buffers also being used on the same system).
>
> So the one question I have: Why not just have a mediatek specific
> restricted_heap dmabuf heap driver?  Since there's already been some
> talk of slight semantic differences in various restricted buffer
> implementations, should we just start with separately named dmabuf
> heaps for each? Maybe consolidating to a common name as more drivers
> arrive and we gain a better understanding of the variations of
> semantics folks are using?
>
> thanks
> -john