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

  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(-)
  

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