Message-ID: <8dbdb1e3e18cc290c8949947245b3c1eda83b6a3.camel@mediatek.com>
Date:   Thu, 12 Oct 2023 06:54:48 +0000
From:   Yong Wu (吴勇) <Yong.Wu@...iatek.com>
To:     "robh@...nel.org" <robh@...nel.org>,
        "jkardatzke@...gle.com" <jkardatzke@...gle.com>,
        "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>
CC:     "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "jstultz@...gle.com" <jstultz@...gle.com>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Jianjiao Zeng (曾健姣) 
        <Jianjiao.Zeng@...iatek.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        Kuohong Wang (王國鴻) 
        <kuohong.wang@...iatek.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "krzysztof.kozlow.ski+dt@...aro.org" 
        <krzysztof.kozlow.ski+dt@...aro.org>,
        "Brian.Starkey@....com" <Brian.Starkey@....com>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>,
        "tjmercier@...gle.com" <tjmercier@...gle.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH 8/9] dt-bindings: reserved-memory: MediaTek: Add reserved
 memory for SVP


Thanks Jeffrey for the addition.

Hi Rob, krzysztof,

Just a ping. Is Jeffrey's reply ok for you?

Thanks.


On Tue, 2023-09-19 at 15:15 -0700, Jeffrey Kardatzke wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Mon, Sep 18, 2023 at 3:47 AM Yong Wu (吴勇) <Yong.Wu@...iatek.com>
> wrote:
> >
> > On Tue, 2023-09-12 at 10:53 -0500, Rob Herring wrote:
> > >
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > >  On Tue, Sep 12, 2023 at 11:13:50AM +0100, Robin Murphy wrote:
> > > > On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote:
> > > > > On 12/09/2023 08:16, Yong Wu (吴勇) wrote:
> > > > > > Hi Rob,
> > > > > >
> > > > > > Thanks for your review.
> > > > > >
> > > > > > On Mon, 2023-09-11 at 10:44 -0500, Rob Herring wrote:
> > > > > > >
> > > > > > > External email : Please do not click links or open
> > > attachments until
> > > > > > > you have verified the sender or the content.
> > > > > > >   On Mon, Sep 11, 2023 at 10:30:37AM +0800, Yong Wu
> wrote:
> > > > > > > > This adds the binding for describing a CMA memory for
> > > MediaTek
> > > > > > > SVP(Secure
> > > > > > > > Video Path).
> > > > > > >
> > > > > > > CMA is a Linux thing. How is this related to CMA?
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> > > > > > > > ---
> > > > > > > >   .../mediatek,secure_cma_chunkmem.yaml         | 42
> > > > > > > +++++++++++++++++++
> > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > >   create mode 100644
> > > Documentation/devicetree/bindings/reserved-
> > > > > > > memory/mediatek,secure_cma_chunkmem.yaml
> > > > > > > >
> > > > > > > > diff --git
> a/Documentation/devicetree/bindings/reserved-
> > > > > > > memory/mediatek,secure_cma_chunkmem.yaml
> > > > > > > b/Documentation/devicetree/bindings/reserved-
> > > > > > > memory/mediatek,secure_cma_chunkmem.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..cc10e00d35c4
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/reserved-
> > > > > > > memory/mediatek,secure_cma_chunkmem.yaml
> > > > > > > > @@ -0,0 +1,42 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-
> Clause)
> > > > > > > > +%YAML 1.2
> > > > > > > > +---
> > > > > > > > +$id:
> > > > > > >
> > > 
> http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: MediaTek Secure Video Path Reserved Memory
> > > > > > >
> > > > > > > What makes this specific to Mediatek? Secure video path
> is
> > > fairly
> > > > > > > common, right?
> > > > > >
> > > > > > Here we just reserve a buffer and would like to create a
> dma-
> > > buf secure
> > > > > > heap for SVP, then the secure engines(Vcodec and DRM) could
> > > prepare
> > > > > > secure buffer through it.
> > > > > > But the heap driver is pure SW driver, it is not platform
> > > device and
> > > > >
> > > > > All drivers are pure SW.
> > > > >
> > > > > > we don't have a corresponding HW unit for it. Thus I don't
> > > think I
> > > > > > could create a platform dtsi node and use "memory-region"
> > > pointer to
> > > > > > the region. I used RESERVEDMEM_OF_DECLARE currently(The
> code is
> > > in
> > > > > > [9/9]). Sorry if this is not right.
> > > > >
> > > > > If this is not for any hardware and you already understand
> this
> > > (since
> > > > > you cannot use other bindings) then you cannot have custom
> > > bindings for
> > > > > it either.
> > > > >
> > > > > >
> > > > > > Then in our usage case, is there some similar method to do
> > > this? or
> > > > > > any other suggestion?
> > > > >
> > > > > Don't stuff software into DTS.
> > > >
> > > > Aren't most reserved-memory bindings just software policy if
> you
> > > look at it
> > > > that way, though? IIUC this is a pool of memory that is visible
> and
> > > > available to the Non-Secure OS, but is fundamentally owned by
> the
> > > Secure
> > > > TEE, and pages that the TEE allocates from it will become
> > > physically
> > > > inaccessible to the OS. Thus the platform does impose
> constraints
> > > on how the
> > > > Non-Secure OS may use it, and per the rest of the reserved-
> memory
> > > bindings,
> > > > describing it as a "reusable" reservation seems entirely
> > > appropriate. If
> > > > anything that's *more* platform-related and so DT-relevant than
> > > typical
> > > > arbitrary reservations which just represent "save some memory
> to
> > > dedicate to
> > > > a particular driver" and don't actually bear any relationship
> to
> > > firmware or
> > > > hardware at all.
> > >
> > > Yes, a memory range defined by hardware or firmware is within
> scope
> > > of
> > > DT. (CMA at aribitrary address was questionable.)
> >
> 
> Before I reply, my context is that I'm using these patches from
> Mediatek on ChromeOS to implement secure video playback.
> 
> > I guess the memory range is not "defined" by HW in our case, but
> this
> > reserve buffer is indeed prepared for and used by HW.
> >
> The memory range is defined in the firmware. The TEE is configured
> with the same address/size that is being set in this DT node. (so
> based on comments already, this is appropriate to put in the DT).
> 
> > If this is a normal reserved buffer for some device, we could
> define a
> > reserved-memory with "shared-dma-pool", then the device use it via
> > "memory-region" property, is this right?
> >
> > Here it is a secure buffer case and this usage relationship is
> > indirect. We create a new heap for this new secure type memory,
> other
> > users such as VCODEC and DRM allocate secure memory through the new
> > heap.
> >
> > About the aribitrary address is because we have HW register for it.
> As
> > long as this is a legal dram address, it is fine. When this address
> is
> > passed into TEE, it will be protected by HW.
> >
> > >
> > > My issue here is more that 'secure video memory' is not any way
> > > Mediatek
> > > specific.
> >
> > Sorry, I don't know if there already is an SVP case in the current
> > kernel. If so, could you help share it?
> 
> I don't think there is any SVP (Secure Video Path) case in the
> current
> kernel. I agree this shouldn't be a Mediatek specific setting, as
> this
> could be usable to other SVP implementations.
> 
> I do think this should have 'cma' in the DT description, as it does
> relate to what the driver is going to do with this memory region. It
> will establish it as a CMA region in Linux. The reason it needs to be
> a CMA region is that the entire memory region will need to transition
> between secure (i.e. TEE owned) and non-secure (i.e. kernel owned).
> Some chipsets have the ability to change memory states between secure
> & non-secure at page level granularity, and in these cases you don't
> need to worry about having a CMA region like this. That is not the
> case on the Mediatek chips where there is a limit to how many regions
> you can mark as secure. In order to deal with that limitation, once a
> secure allocation needs to be performed, the kernel driver allocates
> the entire CMA region so nothing else will use it. Then it marks that
> whole region secure and the TEE can do its own allocations from that
> region. When all those allocations are freed, it can mark that region
> as non-secure again, drop the whole CMA allocation and the kernel can
> make use of that CMA region again.  (there is the other dma-heap in
> their patches, which is for a smaller region that can always be
> secure...but that one is a permanent carveout, the CMA region is
> available to the kernel while not in use for secure memory)
> 
> So maybe something like:
> 
> +title:Secure Reserved CMA Region
> +
> +description:
> +  This binding describes a CMA region that can dynamically
> transition
> between secure and non-secure states that a TEE can allocate memory
> from.
> +
> +maintainers:
> +  - Yong Wu <yong.wu@...iatek.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: secure_cma_region
> +
> +required:
> +  - compatible
> +  - reg
> +  - reusable
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +
> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        reserved-memory@...00000 {
> +            compatible = "secure_cma_region";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;
> +        };
> +    };

Powered by blists - more mailing lists