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