diff mbox series

[8/9] dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP

Message ID 20230911023038.30649-9-yong.wu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series dma-buf: heaps: Add MediaTek secure heap | expand

Commit Message

Yong Wu (吴勇) Sept. 11, 2023, 2:30 a.m. UTC
This adds the binding for describing a CMA memory for MediaTek SVP(Secure
Video Path).

Signed-off-by: Yong Wu <yong.wu@mediatek.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

Comments

Rob Herring (Arm) Sept. 11, 2023, 3:44 p.m. UTC | #1
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@mediatek.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?

> +
> +description:
> +  This binding describes the reserved memory for secure video path.
> +
> +maintainers:
> +  - Yong Wu <yong.wu@mediatek.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: mediatek,secure_cma_chunkmem
> +
> +required:
> +  - compatible
> +  - reg
> +  - reusable
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +
> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        reserved-memory@80000000 {
> +            compatible = "mediatek,secure_cma_chunkmem";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;
> +        };
> +    };
> -- 
> 2.25.1
>
Yong Wu (吴勇) Sept. 12, 2023, 6:16 a.m. UTC | #2
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@mediatek.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
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.

Then in our usage case, is there some similar method to do this? or
any other suggestion?
 
Appreciate in advance.

> 
> > +
> > +description:
> > +  This binding describes the reserved memory for secure video
> path.
> > +
> > +maintainers:
> > +  - Yong Wu <yong.wu@mediatek.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,secure_cma_chunkmem
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    reserved-memory {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        reserved-memory@80000000 {
> > +            compatible = "mediatek,secure_cma_chunkmem";
> > +            reusable;
> > +            reg = <0x80000000 0x18000000>;
> > +        };
> > +    };
> > -- 
> > 2.25.1
> >
Krzysztof Kozlowski Sept. 12, 2023, 8:28 a.m. UTC | #3
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@mediatek.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.

Best regards,
Krzysztof
Robin Murphy Sept. 12, 2023, 10:13 a.m. UTC | #4
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@mediatek.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.

However, the fact that Linux's implementation of how to reuse reserved 
memory areas is called CMA is indeed still irrelevant and has no place 
in the binding itself.

Thanks,
Robin.
Robin Murphy Sept. 12, 2023, 4:05 p.m. UTC | #5
On 12/09/2023 4:53 pm, Rob Herring wrote:
> 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@mediatek.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.)
> 
> My issue here is more that 'secure video memory' is not any way Mediatek
> specific. AIUI, it's a requirement from certain content providers for
> video playback to work. So why the Mediatek specific binding?

Based on the implementation, I'd ask the question the other way round - 
the way it works looks to be at least somewhat dependent on Mediatek's 
TEE, in ways where other vendors' equivalent implementations may be 
functionally incompatible, however nothing suggests it's actually 
specific to video (beyond that presumably being the primary use-case 
they had in mind).

Thanks,
Robin.
Yong Wu (吴勇) Sept. 18, 2023, 10:47 a.m. UTC | #6
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@mediatek.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.)

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.

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?

Regarding our special, the new heap driver may be different, and other
HWs share this reserve buffer and manage it through this pure SW heap.

>  AIUI, it's a requirement from certain content providers for 
> video playback to work. So why the Mediatek specific binding?
> 
> Rob
Jeffrey Kardatzke Sept. 19, 2023, 10:15 p.m. UTC | #7
On Mon, Sep 18, 2023 at 3:47 AM Yong Wu (吴勇) <Yong.Wu@mediatek.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@mediatek.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@mediatek.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@80000000 {
+            compatible = "secure_cma_region";
+            reusable;
+            reg = <0x80000000 0x18000000>;
+        };
+    };
Yong Wu (吴勇) Oct. 12, 2023, 6:54 a.m. UTC | #8
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@mediatek.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@mediatek.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@mediatek.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@80000000 {
> +            compatible = "secure_cma_region";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;
> +        };
> +    };
Krzysztof Kozlowski Oct. 12, 2023, 7:07 a.m. UTC | #9
On 12/10/2023 08:54, Yong Wu (吴勇) wrote:
> 
> Thanks Jeffrey for the addition.
> 
> Hi Rob, krzysztof,
> 
> Just a ping. Is Jeffrey's reply ok for you?

I did not see any patch posted and I am way behind reviewing patches to
review also non-patches-patches...

Best regards,
Krzysztof
Yong Wu (吴勇) Oct. 12, 2023, 11:15 a.m. UTC | #10
On Thu, 2023-10-12 at 09:07 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 12/10/2023 08:54, Yong Wu (吴勇) wrote:
> > 
> > Thanks Jeffrey for the addition.
> > 
> > Hi Rob, krzysztof,
> > 
> > Just a ping. Is Jeffrey's reply ok for you?
> 
> I did not see any patch posted and I am way behind reviewing patches
> to
> review also non-patches-patches...

Sorry, I haven't sent a new version yet. I plan to prepare the new
version after having a conclusion here.

In Jeffrey's help reply, this memory range is defined in TEE firmware,
thus this looks could be ok for a binding, right?

Thanks.

> 
> Best regards,
> Krzysztof
>
Vijayanand Jitta Oct. 19, 2023, 4:46 a.m. UTC | #11
On 9/11/2023 8:00 AM, Yong Wu wrote:
> This adds the binding for describing a CMA memory for MediaTek SVP(Secure
> Video Path).
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.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
> +
> +description:
> +  This binding describes the reserved memory for secure video path.
> +
> +maintainers:
> +  - Yong Wu <yong.wu@mediatek.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: mediatek,secure_cma_chunkmem
> +
> +required:
> +  - compatible
> +  - reg
> +  - reusable
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +
> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        reserved-memory@80000000 {
> +            compatible = "mediatek,secure_cma_chunkmem";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;
> +        };
> +    };

Instead of having a vendor specific binding for cma area, How about retrieving
https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/ ?
dma_heap_add_cma can just associate cma region and create a heap. So, we can reuse cma heap
code for allocation instead of replicating that code here.

Thanks,
Vijay
Yong Wu (吴勇) Oct. 20, 2023, 9:50 a.m. UTC | #12
On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> 
> On 9/11/2023 8:00 AM, Yong Wu wrote:
> > This adds the binding for describing a CMA memory for MediaTek
> SVP(Secure
> > Video Path).
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.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
> > +
> > +description:
> > +  This binding describes the reserved memory for secure video
> path.
> > +
> > +maintainers:
> > +  - Yong Wu <yong.wu@mediatek.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,secure_cma_chunkmem
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    reserved-memory {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        reserved-memory@80000000 {
> > +            compatible = "mediatek,secure_cma_chunkmem";
> > +            reusable;
> > +            reg = <0x80000000 0x18000000>;
> > +        };
> > +    };
> 
> Instead of having a vendor specific binding for cma area, How about
> retrieving
> 
https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
>  ?
> dma_heap_add_cma can just associate cma region and create a heap. So,
> we can reuse cma heap
> code for allocation instead of replicating that code here.
> 

Thanks for the reference. I guess we can't use it. There are two
reasons:
  
a) The secure heap driver is a pure software driver and we have no
device for it, therefore we cannot call dma_heap_add_cma.
  
b) The CMA area here is dynamic for SVP. Normally this CMA can be used
in the kernel. In the SVP case we use cma_alloc to get it and pass the
entire CMA physical start address and size into TEE to protect the CMA
region. The original CMA heap cannot help with the TEE part.

Thanks.

> Thanks,
> Vijay
> 
> 
>
Jaskaran Singh Nov. 1, 2023, 5:50 a.m. UTC | #13
On 10/20/2023 3:20 PM, Yong Wu (吴勇) wrote:
> On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
>>  	 
>> Instead of having a vendor specific binding for cma area, How about
>> retrieving
>>
> https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
>>  ?
>> dma_heap_add_cma can just associate cma region and create a heap. So,
>> we can reuse cma heap
>> code for allocation instead of replicating that code here.
>>
> 
> Thanks for the reference. I guess we can't use it. There are two
> reasons:
>   
> a) The secure heap driver is a pure software driver and we have no
> device for it, therefore we cannot call dma_heap_add_cma.
>   

Hi Yong,

We're considering using struct cma as the function argument to
dma_heap_add_cma() rather than struct device. Would this help
resolve the problem of usage with dma_heap_add_cma()?

> b) The CMA area here is dynamic for SVP. Normally this CMA can be used
> in the kernel. In the SVP case we use cma_alloc to get it and pass the
> entire CMA physical start address and size into TEE to protect the CMA
> region. The original CMA heap cannot help with the TEE part.
>

Referring the conversation at
https://lore.kernel.org/lkml/7a2995de23c24ef22c071c6976c02b97e9b50126.camel@mediatek.com/;

since we're considering abstracting secure mem ops, would it make sense
to use the default CMA heap ops (cma_heap_ops), allocate buffers from it
and secure each allocated buffer?

Thanks,
Jaskaran.

> Thanks.
> 
>> Thanks,
>> Vijay
>>
>>
>>
Yong Wu (吴勇) Nov. 6, 2023, 5:56 a.m. UTC | #14
On Wed, 2023-11-01 at 11:20 +0530, Jaskaran Singh wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 10/20/2023 3:20 PM, Yong Wu (吴勇) wrote:
> > On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
> >>   
> >> Instead of having a vendor specific binding for cma area, How
> about
> >> retrieving
> >>
> > 
> https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
> >>  ?
> >> dma_heap_add_cma can just associate cma region and create a heap.
> So,
> >> we can reuse cma heap
> >> code for allocation instead of replicating that code here.
> >>
> > 
> > Thanks for the reference. I guess we can't use it. There are two
> > reasons:
> >   
> > a) The secure heap driver is a pure software driver and we have no
> > device for it, therefore we cannot call dma_heap_add_cma.
> >   
> 
> Hi Yong,
> 
> We're considering using struct cma as the function argument to
> dma_heap_add_cma() rather than struct device. Would this help
> resolve the problem of usage with dma_heap_add_cma()?

Yes. If we use "struct cma", I guess it works.

> 
> > b) The CMA area here is dynamic for SVP. Normally this CMA can be
> used
> > in the kernel. In the SVP case we use cma_alloc to get it and pass
> the
> > entire CMA physical start address and size into TEE to protect the
> CMA
> > region. The original CMA heap cannot help with the TEE part.
> >
> 
> Referring the conversation at
> 
https://lore.kernel.org/lkml/7a2995de23c24ef22c071c6976c02b97e9b50126.camel@mediatek.com/
> ;
> 
> since we're considering abstracting secure mem ops, would it make
> sense
> to use the default CMA heap ops (cma_heap_ops), allocate buffers from
> it
> and secure each allocated buffer?

Then it looks you also need tee operation like tee_client_open_session
and tee_client_invoke_func, right?

It seems we also need change a bit for "cma_heap_allocate" to allow cma
support operations from secure heap.

I will send a v2 to move the discussion forward. The v2 is based on our
case, It won't include the cma part.

> 
> Thanks,
> Jaskaran.
> 
> > Thanks.
> > 
> >> Thanks,
> >> Vijay
> >>
> >>
> >>
>
Jaskaran Singh Nov. 20, 2023, 8:20 a.m. UTC | #15
On 11/6/2023 11:26 AM, Yong Wu (吴勇) wrote:
> On Wed, 2023-11-01 at 11:20 +0530, Jaskaran Singh wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 10/20/2023 3:20 PM, Yong Wu (吴勇) wrote:
>>> On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
>>>>   
>>>> Instead of having a vendor specific binding for cma area, How
>> about
>>>> retrieving
>>>>
>>>
>> https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
>>>>  ?
>>>> dma_heap_add_cma can just associate cma region and create a heap.
>> So,
>>>> we can reuse cma heap
>>>> code for allocation instead of replicating that code here.
>>>>
>>>
>>> Thanks for the reference. I guess we can't use it. There are two
>>> reasons:
>>>   
>>> a) The secure heap driver is a pure software driver and we have no
>>> device for it, therefore we cannot call dma_heap_add_cma.
>>>   
>>
>> Hi Yong,
>>
>> We're considering using struct cma as the function argument to
>> dma_heap_add_cma() rather than struct device. Would this help
>> resolve the problem of usage with dma_heap_add_cma()?
> 
> Yes. If we use "struct cma", I guess it works.
>

Great; I've posted a v2[1] for the API incorporating this change.

Thanks,
Jaskaran.

[1]
https://lore.kernel.org/lkml/20231117100337.5215-1-quic_jasksing@quicinc.com/
diff mbox series

Patch

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
+
+description:
+  This binding describes the reserved memory for secure video path.
+
+maintainers:
+  - Yong Wu <yong.wu@mediatek.com>
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: mediatek,secure_cma_chunkmem
+
+required:
+  - compatible
+  - reg
+  - reusable
+
+unevaluatedProperties: false
+
+examples:
+  - |
+
+    reserved-memory {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+
+        reserved-memory@80000000 {
+            compatible = "mediatek,secure_cma_chunkmem";
+            reusable;
+            reg = <0x80000000 0x18000000>;
+        };
+    };