Received-SPF: pass (google.com: domain of linux-kernel+bounces-25102-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240112092014.23999-1-yong.wu@mediatek.com> <20240112092014.23999-4-yong.wu@mediatek.com>
 <CANDhNCrxpeqEhJD0xJzu3vm8a4jMXD2v+_dbDNvaKhLsLB5-4g@mail.gmail.com>
In-Reply-To: <CANDhNCrxpeqEhJD0xJzu3vm8a4jMXD2v+_dbDNvaKhLsLB5-4g@mail.gmail.com>
From: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Fri, 12 Jan 2024 15:27:27 -0800
Message-ID: <CA+ddPcNdniUTpE_pJb-fL7+MHNSUZTkQojL48iqvW9JPr-Tc-g@mail.gmail.com>
Subject: Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops
To: John Stultz <jstultz@google.com>
Cc: Yong Wu <yong.wu@mediatek.com>, Rob Herring <robh+dt@kernel.org>, 
	Matthias Brugger <matthias.bgg@gmail.com>, christian.koenig@amd.com, 
	Sumit Semwal <sumit.semwal@linaro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Benjamin Gaignard <benjamin.gaignard@collabora.com>, Brian Starkey <Brian.Starkey@arm.com>, 
	tjmercier@google.com, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, 
	dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, 
	linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, 
	Robin Murphy <robin.murphy@arm.com>, Vijayanand Jitta <quic_vjitta@quicinc.com>, 
	Joakim Bech <joakim.bech@linaro.org>, Pavel Machek <pavel@ucw.cz>, Simon Ser <contact@emersion.fr>, 
	Pekka Paalanen <ppaalanen@gmail.com>, jianjiao.zeng@mediatek.com, kuohong.wang@mediatek.com, 
	youlin.pei@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 12, 2024 at 2:52=E2=80=AFPM John Stultz <jstultz@google.com> wr=
ote:
>
> On Fri, Jan 12, 2024 at 1:21=E2=80=AFAM Yong Wu <yong.wu@mediatek.com> wr=
ote:
> >
> > Add "struct restricted_heap_ops". For the restricted memory, totally th=
ere
> > 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 re=
stricted_buffer *buf);
> > +       void    (*memory_free)(struct restricted_heap *heap, struct res=
tricted_buffer *buf);
> > +
> > +       int     (*memory_restrict)(struct restricted_heap *heap, struct=
 restricted_buffer *buf);
> > +       void    (*memory_unrestrict)(struct restricted_heap *heap, stru=
ct 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