| Message ID | 20240112092014.23999-4-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: heaps: Add restricted heap | expand |
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. 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
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
On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > 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: > > > 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. thanks -john
On Fri, Jan 12, 2024 at 3:51 PM John Stultz <jstultz@google.com> wrote: > > On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > > 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: > > > > 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
On Fri, Jan 12, 2024 at 4:13 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > On Fri, Jan 12, 2024 at 3:51 PM John Stultz <jstultz@google.com> wrote: > > > > On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > > > On Fri, Jan 12, 2024 at 2:52 PM John Stultz <jstultz@google.com> wrote: > > > > 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. Yeah. I apologize, as I know how frustrating the contradictory feedback can be. I don't mean to demotivate. :( I think folks would very much like to see consolidation around the various implementations, and I agree! I just worry that creating the common framework for this concept in a dmabuf heaps driver is maybe too peripheral/close to userland. > 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. Yeah. If there is a clear meaning to "restricted" here, I think having a query method on the dmabuf is reasonable. My only fret is if the meaning is too vague and userland starts depending on it meaning what it meant for vendor1, but doesn't mean for vendor2. So we need some clarity in what "restricted" really means. For instance, it being not cpu mappable vs other potential variations like being cpu mappable, but not cpu accessible. Or not cpu mappable, but only mappable between a set of 3 devices (Which 3 devices?! How can we tell?). And if there is variation, maybe we need to enumerate the types of "restricted" buffers so we can be specific when it's queried. That's where maybe having the framework for this be more central or closer to the kernel mm code and not just a sub-type of a dmabuf heap driver might be better? > 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). If there's a need and the best place to put something is in the dma_buf or dma_buf_ops, that's where it should go. Folks may reasonably disagree if it's the best place (there may be yet better spots for the state to sit in the abstractions), but for stuff going upstream, there's no reason to try to hack around things or smuggle state just to avoid changing core structures. Especially if core structures are internal only and have no ABI implications. Sima's suggestion that attachments should fail if the device cannot properly map the restricted buffer makes sense to me. Though I don't quite see why all attachments should fail, and I don't really like the idea of a private api, but I need to look more at the suggested virtio example (but even they said that wasn't their preferred route). My sense of attach was only that it was supposed to connect a device's interest in the buffer, allowing lazy allocation to satisfy various device constraints before first mapping - a design feature that I don't think anyone ever implemented. So my sense was it didn't have much meaning otherwise (but was a requirement to call before map). But that may have evolved since the early days. And I'm sure the method to figure out if the attachment can work with the device may be complicated/difficult, so it sounding reasonable can be far from it being reasonable to implement. And again, I don't mean to frustrate or demotivate here. I'm really excited to see this effort being pushed upstream! thanks -john
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/ > 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(-) > > 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? > + 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? > + 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? 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. > }; > > int restricted_heap_add(struct restricted_heap *rstrd_heap); > -- > 2.25.1 >
On Fri, Jan 12, 2024 at 05:23:07PM -0800, John Stultz wrote: > On Fri, Jan 12, 2024 at 4:13 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > > On Fri, Jan 12, 2024 at 3:51 PM John Stultz <jstultz@google.com> wrote: > > > > > > On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@google.com> wrote: > > > > On Fri, Jan 12, 2024 at 2:52 PM John Stultz <jstultz@google.com> wrote: > > > > > 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. > > Yeah. I apologize, as I know how frustrating the contradictory > feedback can be. I don't mean to demotivate. :( > > I think folks would very much like to see consolidation around the > various implementations, and I agree! > I just worry that creating the common framework for this concept in a > dmabuf heaps driver is maybe too peripheral/close to userland. > > > 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. > > Yeah. If there is a clear meaning to "restricted" here, I think having > a query method on the dmabuf is reasonable. > My only fret is if the meaning is too vague and userland starts > depending on it meaning what it meant for vendor1, but doesn't mean > for vendor2. > > So we need some clarity in what "restricted" really means. For > instance, it being not cpu mappable vs other potential variations like > being cpu mappable, but not cpu accessible. Or not cpu mappable, but > only mappable between a set of 3 devices (Which 3 devices?! How can we > tell?). > Can we flip things around? I.e., instead of saying which devices are allowed to use the restricted buffer, can we instead say where it's not allowed to be used? My view has been that by default the contents of the types of buffers where talking about here is only accessible to things running on the secure side, i.e, typically S-EL3, S-EL1 and a specific Trusted Application running in S-EL0. I guess that serves as some kind of baseline. From there, things turns to a more dynamic nature, where firewalls etc, can be configured to give access to various IPs, blocks and runtimes. I understand that it's nice to be able to know all this from the Linux kernel point of view, but does it have to be aware of this? What's the major drawback if Linux doesn't know about it? > And if there is variation, maybe we need to enumerate the types of > "restricted" buffers so we can be specific when it's queried. > > That's where maybe having the framework for this be more central or > closer to the kernel mm code and not just a sub-type of a dmabuf heap > driver might be better? > > > 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). > > If there's a need and the best place to put something is in the > dma_buf or dma_buf_ops, that's where it should go. Folks may > reasonably disagree if it's the best place (there may be yet better > spots for the state to sit in the abstractions), but for stuff going > upstream, there's no reason to try to hack around things or smuggle > state just to avoid changing core structures. Especially if core > structures are internal only and have no ABI implications. > > Sima's suggestion that attachments should fail if the device cannot > properly map the restricted buffer makes sense to me. Though I don't > quite see why all attachments should fail, and I don't really like the > idea of a private api, but I need to look more at the suggested virtio > example (but even they said that wasn't their preferred route). > > My sense of attach was only that it was supposed to connect a device's > interest in the buffer, allowing lazy allocation to satisfy various > device constraints before first mapping - a design feature that I > don't think anyone ever implemented. So my sense was it didn't have > much meaning otherwise (but was a requirement to call before map). But > that may have evolved since the early days. > > And I'm sure the method to figure out if the attachment can work with > the device may be complicated/difficult, so it sounding reasonable can > be far from it being reasonable to implement. > > And again, I don't mean to frustrate or demotivate here. I'm really > excited to see this effort being pushed upstream! > > thanks > -john
On Wed, Jan 31, 2024 at 6:15 AM Joakim Bech <joakim.bech@linaro.org> wrote: > On Fri, Jan 12, 2024 at 05:23:07PM -0800, John Stultz wrote: > > So we need some clarity in what "restricted" really means. For > > instance, it being not cpu mappable vs other potential variations like > > being cpu mappable, but not cpu accessible. Or not cpu mappable, but > > only mappable between a set of 3 devices (Which 3 devices?! How can we > > tell?). > > > Can we flip things around? I.e., instead of saying which devices are > allowed to use the restricted buffer, can we instead say where it's not > allowed to be used? My view has been that by default the contents of the > types of buffers where talking about here is only accessible to things > running on the secure side, i.e, typically S-EL3, S-EL1 and a specific > Trusted Application running in S-EL0. I guess that serves as some kind > of baseline. ? This seems like you're suggesting enumerating badness? I'm not sure I understand the benefit of that. > From there, things turns to a more dynamic nature, where firewalls etc, > can be configured to give access to various IPs, blocks and runtimes. > > I understand that it's nice to be able to know all this from the Linux > kernel point of view, but does it have to be aware of this? What's the > major drawback if Linux doesn't know about it? Indeed, it doesn't necessarily. The idea with DMABUF heaps is it provides a name to abstract/wrap a type of constraint. So you can then allocate buffers that satisfy that constraint. Admittedly the downside with DMABUF heaps is that it has a bit of a gap in the abstraction in that we don't have a mapping of device constraints, so in Android gralloc provides a device specific usage/pipeline -> heap mapping. (Note: This I don't think is very problematic - I often use the example of fstab as device-specific config everyone is comfortable with - but I know folks would like to have something more generic) I believe Christian has previously proposed to have the devices provide something like symlinks from their sysfs nodes to the heaps the device supports, which is an interesting idea to mostly close that issue. Applications could then scan the devices in a pipeline and find the type they all support, and the specific names wouldn't matter. However, I'd expect the same hardware pipeline might support both restricted and unrestricted playback, so there would need to be some way to differentiate for the use case, so I'm not sure you can get away from some heap name to functionality mapping. My main concern with this patch series is that it seems to want to bundle all the different types of "restricted" buffers that might be possible under a single "restricted" heap name. Since we likely have devices with different security domains, thus different types of restrictions. So we may need to be able to differentiate between "secure video playback" uses and "protected device firmware" uses on the same machine. Thus, I'm not sure it's a good idea to bundle all of these under the same heap name. thanks -john
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 <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(-) > > > > 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
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); + 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 { + 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);
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(-)