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

[Jeffrey Kardatzke]

On Fri, Jan 12, 2024 at 3:51 PM John Stultz <jstultz@xxxxxxxxxx> wrote:
>
> On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@xxxxxxxxxx> wrote:
> > 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:
> > > > 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
>
> Ah, I appreciate that clarification! Indeed, you're right the name is
> passed through. Apologies for missing that detail.
>
> > 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.
>
> So I guess I'm not sure I understand the benefit of the extra
> indirection. What then does the restricted_heap.c logic itself
> provide?
> The dmabuf heaps framework already provides a way to add heap implementations.

So in the v1 patchset, it was done with just a Mediatek specific heap
with no framework or abstractions for another vendor to build on top
of. The feedback was to make this more generic since Mediatek won't be
the only vendor who wants a restricted heap..which is how it ended up
here. There was more code in the framework before relating to TEE
calls, but then that was moved to the vendor specific code since not
all restricted heaps are allocated through a TEE.

This was also desirable for the V4L2 pieces since there's going to be
a V4L2 flag set when using restricted dma_bufs (and it wants to
validate that)....so in order to keep that more generic, there should
be a higher level concept of restricted dma_bufs that isn't specific
to a single vendor.  One other thing that would ideally come out of
this is a cleaner way to check that a dma_buf is restricted or not.
The current V4L2 patchset just attaches the dma_buf and then checks if
the page table is empty....and if so, it's restricted. But now I see
there's other feedback indicating attaching a restricted dma_buf
shouldn't even be allowed, so we'll need another strategy for
detecting them. Ideally there is some function/macro like
is_dma_buf_restricted(struct dma_buf*) that can indicate that...but we
haven't come up with a good way to do that yet which doesn't involve
adding another field to dma_buf or to dma_buf_ops (and if such a thing
would be fine, then OK...but I had assumed we would get pushback on
modifying either of those structs).

>
> thanks
> -john