diff mbox series

[v2,6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

Message ID 20231111111559.8218-7-yong.wu@mediatek.com
State New
Headers
Series dma-buf: heaps: Add secure heap |

Commit Message

Yong Wu Nov. 11, 2023, 11:15 a.m. UTC 
  Add a binding for describing the secure CMA reserved memory range. The
memory range also will be defined in the TEE firmware. It means the TEE
will be configured with the same address/size that is being set in this
DT node.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml

Comments

Krzysztof Kozlowski Nov. 11, 2023, 12:48 p.m. UTC | #1 
On 11/11/2023 12:15, Yong Wu wrote:
> Add a binding for describing the secure CMA reserved memory range. The
> memory range also will be defined in the TEE firmware. It means the TEE
> will be configured with the same address/size that is being set in this
> DT node.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---

What was the outcome of previous discussion? I don't see any references
to the conclusion and your changelog "Reword the dt-binding description"
is way too generic.

You must explain what happened here.

>  .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> new file mode 100644
> index 000000000000..8ab559595fbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Secure Reserved CMA Region
> +
> +description:
> +  This binding describes a CMA region that can dynamically transition

Describe the hardware or firmware, not the binding. Drop first four
words and rephrase it.

> +between secure and non-secure states that a TEE can allocate memory
> +from.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Do not send untested code.

> +
> +maintainers:
> +  - Yong Wu <yong.wu@mediatek.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: secure_cma_region

Still wrong compatible. Look at other bindings - there is nowhere
underscore. Look at other reserved memory bindings especially.

Also, CMA is a Linux thingy, so either not suitable for bindings at all,
or you need Linux specific compatible. I don't quite get why do you even
put CMA there - adding Linux specific stuff will get obvious pushback...


> +
> +required:
> +  - compatible
> +  - reg
> +  - reusable
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +

Stray blank line.

> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        reserved-memory@80000000 {
> +            compatible = "secure_cma_region";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;

reg is second property. Open DTS and check how it is there.

> +        };
> +    };

Best regards,
Krzysztof
Yong Wu Nov. 13, 2023, 6:37 a.m. UTC | #2 
On Sat, 2023-11-11 at 13:48 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 11/11/2023 12:15, Yong Wu wrote:
> > Add a binding for describing the secure CMA reserved memory range.
> The
> > memory range also will be defined in the TEE firmware. It means the
> TEE
> > will be configured with the same address/size that is being set in
> this
> > DT node.
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> 
> What was the outcome of previous discussion? I don't see any
> references
> to the conclusion and your changelog "Reword the dt-binding
> description"
> is way too generic.
> 
> You must explain what happened here.

I don't think there is a final conclusion yet in v1. Jeff helped
explain that this region also is defined in TEE firmware. I put this a
bit in the commit message above.

Sorry for confusing.

> 
> >  .../reserved-memory/secure_cma_region.yaml    | 44
> +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > new file mode 100644
> > index 000000000000..8ab559595fbe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Secure Reserved CMA Region

Will change to: Secure Region. Is it ok?

> > +
> > +description:
> > +  This binding describes a CMA region that can dynamically
> transition
> 
> Describe the hardware or firmware, not the binding. Drop first four
> words and rephrase it.

Memory region for TEE usage, which is also defined in the TEE firmware.
When an activity (e.g. secure video playback) requiring usage of this
starts, this region will be protected by MPU (Memory Protect Unit) in
the TEE firmware. After the activity is completed, the region will be
unprotected by the TEE and usable by the non-secure side (i.e. kernel
and userspace).

Does this description make sense for you?

> 
> > +between secure and non-secure states that a TEE can allocate
> memory
> > +from.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for
> instructions).
> Maybe you need to update your dtschema and yamllint.
> 
> Do not send untested code.

Sorry. I will update them and test this before sending.

> 
> > +
> > +maintainers:
> > +  - Yong Wu <yong.wu@mediatek.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: secure_cma_region
> 
> Still wrong compatible. Look at other bindings - there is nowhere
> underscore. Look at other reserved memory bindings especially.
> 
> Also, CMA is a Linux thingy, so either not suitable for bindings at
> all,
> or you need Linux specific compatible. I don't quite get why do you
> evennot
> put CMA there - adding Linux specific stuff will get obvious
> pushback...

Thanks. I will change to: secure-region. Is this ok?

> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Stray blank line.

Thanks for reviewing so careful. Will fix this and below.

> 
> > +    reserved-memory {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        reserved-memory@80000000 {
> > +            compatible = "secure_cma_region";
> > +            reusable;
> > +            reg = <0x80000000 0x18000000>;
> 
> reg is second property. Open DTS and check how it is there.
> 
> > +        };
> > +    };
> 
> Best regards,
> Krzysztof
>
Rob Herring Nov. 13, 2023, 8:40 p.m. UTC | #3 
On Sat, 11 Nov 2023 19:15:57 +0800, Yong Wu wrote:
> Add a binding for describing the secure CMA reserved memory range. The
> memory range also will be defined in the TEE firmware. It means the TEE
> will be configured with the same address/size that is being set in this
> DT node.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/reserved-memory/secure_cma_region.example.dts'
Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: could not find expected ':'
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/reserved-memory/secure_cma_region.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: could not find expected ':'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231111111559.8218-7-yong.wu@mediatek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Robin Murphy Nov. 14, 2023, 1:18 p.m. UTC | #4 
On 13/11/2023 6:37 am, Yong Wu (吴勇) wrote:
[...]
>>> +properties:
>>> +  compatible:
>>> +    const: secure_cma_region
>>
>> Still wrong compatible. Look at other bindings - there is nowhere
>> underscore. Look at other reserved memory bindings especially.
>>
>> Also, CMA is a Linux thingy, so either not suitable for bindings at
>> all,
>> or you need Linux specific compatible. I don't quite get why do you
>> evennot
>> put CMA there - adding Linux specific stuff will get obvious
>> pushback...
> 
> Thanks. I will change to: secure-region. Is this ok?

No, the previous discussion went off in entirely the wrong direction. To 
reiterate, the point of the binding is not to describe the expected 
usage of the thing nor the general concept of the thing, but to describe 
the actual thing itself. There are any number of different ways software 
may interact with a "secure region", so that is meaningless as a 
compatible. It needs to describe *this* secure memory interface offered 
by *this* TEE, so that software knows that to use it requires making 
those particular SiP calls with that particular UUID etc.

Thanks,
Robin.
Jeffrey Kardatzke Nov. 15, 2023, 11:35 p.m. UTC | #5 
May I suggest the following for the device tree binding? (I'm not very
familiar w/ device trees, so apologies for any oversights, but trying
to process the feedback here and help move Mediatek along). This
should align with my other suggestions for having an MTK specific
portion to their secure heap implementation; which also means there
should be an MTK specific device tree binding.

# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/reserved-memory/mediatek,dynamic-secure-region.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: Mediatek Dynamic Reserved Region

description:
  A memory region that can dynamically transition as a whole between
secure and non-secure states. This memory will be protected by OP-TEE
when allocations are active and unprotected otherwise.

maintainers:
  - Yong Wu <yong.wu@mediatek.com>

allOf:
  - $ref: reserved-memory.yaml

properties:
  compatible:
    const: mediatek,dynamic-secure-region

required:
  - compatible
  - reg
  - reusable

unevaluatedProperties: false

examples:
  - |

    reserved-memory {
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;

        reserved-memory@80000000 {
            compatible = "mediatek,dynamic-secure-region";
            reusable;
            reg = <0x80000000 0x18000000>;
        };
    };

On Tue, Nov 14, 2023 at 5:18 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 13/11/2023 6:37 am, Yong Wu (吴勇) wrote:
> [...]
> >>> +properties:
> >>> +  compatible:
> >>> +    const: secure_cma_region
> >>
> >> Still wrong compatible. Look at other bindings - there is nowhere
> >> underscore. Look at other reserved memory bindings especially.
> >>
> >> Also, CMA is a Linux thingy, so either not suitable for bindings at
> >> all,
> >> or you need Linux specific compatible. I don't quite get why do you
> >> evennot
> >> put CMA there - adding Linux specific stuff will get obvious
> >> pushback...
> >
> > Thanks. I will change to: secure-region. Is this ok?
>
> No, the previous discussion went off in entirely the wrong direction. To
> reiterate, the point of the binding is not to describe the expected
> usage of the thing nor the general concept of the thing, but to describe
> the actual thing itself. There are any number of different ways software
> may interact with a "secure region", so that is meaningless as a
> compatible. It needs to describe *this* secure memory interface offered
> by *this* TEE, so that software knows that to use it requires making
> those particular SiP calls with that particular UUID etc.
>
> Thanks,
> Robin.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
new file mode 100644
index 000000000000..8ab559595fbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
@@ -0,0 +1,44 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+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>;
+        };
+    };