On Fri, Jan 12, 2024 at 2:52 PM John Stultz jstultz@google.com wrote:
On Fri, Jan 12, 2024 at 1:21 AM Yong Wu yong.wu@mediatek.com 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@mediatek.com
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