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

[Yong Wu (吴勇)]

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@xxxxxxxxxxxx>
> > ---
> 
> 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@xxxxxxxxxxxx>
> > +
> > +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
>