[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Jerome Forissier]

Some platforms may use a single device tree to describe two address
spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
for Secure-only devices"). For these platforms it makes sense to define
a secure counterpart of /chosen, namely: /secure-chosen. This new node
is meant to be used by the secure firmware to pass data to the secure
OS. Only the stdout-path property is supported for now.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 Documentation/devicetree/bindings/arm/secure.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

v2

- Clarify the expected behavior when /secure-chosen/stdout-path is not
present (either the property or the node itself), as proposed by Peter
and Robin. Note that I have made the fallback to /chosen optional.

diff --git a/Documentation/devicetree/bindings/arm/secure.txt b/Documentation/devicetree/bindings/arm/secure.txt
index e31303f..c4a818e 100644
--- a/Documentation/devicetree/bindings/arm/secure.txt
+++ b/Documentation/devicetree/bindings/arm/secure.txt
@@ -32,7 +32,8 @@ describe the view of Secure world using the standard bindings. These
 secure- bindings only need to be used where both the Secure and Normal
 world views need to be described in a single device tree.
 
-Valid Secure world properties:
+Valid Secure world properties
+-----------------------------
 
 - secure-status : specifies whether the device is present and usable
   in the secure world. The combination of this with "status" allows
@@ -51,3 +52,19 @@ Valid Secure world properties:
    status = "disabled"; secure-status = "okay";     /* S-only */
    status = "disabled";                             /* disabled in both */
    status = "disabled"; secure-status = "disabled"; /* disabled in both */
+
+The secure-chosen node
+----------------------
+
+Similar to the /chosen node which serves as a place for passing data
+between firmware and the operating system, the /secure-chosen node may
+be used to pass data to the Secure OS. Only the properties defined
+below may appear in the /secure-chosen node.
+
+- stdout-path : specifies the device to be used by the Secure OS for
+  its console output. The syntax is the same as for /chosen/stdout-path.
+  If the /secure-chosen node exists but the stdout-path property is not
+  present, the Secure OS should not perform any console output. If
+  /secure-chosen does not exist, the Secure OS may use the value of
+  /chosen/stdout-path instead (that is, use the same device as the
+  Normal world OS).
-- 
2.7.4

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Rob Herring]

On Fri, Mar 03, 2017 at 10:24:33AM +0100, Jerome Forissier wrote:
> Some platforms may use a single device tree to describe two address
> spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
> for Secure-only devices"). For these platforms it makes sense to define
> a secure counterpart of /chosen, namely: /secure-chosen. This new node
> is meant to be used by the secure firmware to pass data to the secure
> OS. Only the stdout-path property is supported for now.
> 
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/secure.txt | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> v2
> 
> - Clarify the expected behavior when /secure-chosen/stdout-path is not
> present (either the property or the node itself), as proposed by Peter
> and Robin. Note that I have made the fallback to /chosen optional.

I'm okay with this, but would like to hold off merging it until your are 
really using it. You did say you were "experimenting" and it doesn't 
sound like the DT usage in secure world is that far along. I just want 
to see if there are other issues that come up.

Rob

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Jerome Forissier]

On 03/31/2017 12:28 AM, Rob Herring wrote:
> On Fri, Mar 03, 2017 at 10:24:33AM +0100, Jerome Forissier wrote:
>> Some platforms may use a single device tree to describe two address
>> spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
>> for Secure-only devices"). For these platforms it makes sense to define
>> a secure counterpart of /chosen, namely: /secure-chosen. This new node
>> is meant to be used by the secure firmware to pass data to the secure
>> OS. Only the stdout-path property is supported for now.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/arm/secure.txt | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> v2
>>
>> - Clarify the expected behavior when /secure-chosen/stdout-path is not
>> present (either the property or the node itself), as proposed by Peter
>> and Robin. Note that I have made the fallback to /chosen optional.
> 
> I'm okay with this, but would like to hold off merging it until your are 
> really using it. You did say you were "experimenting" and it doesn't 
> sound like the DT usage in secure world is that far along. I just want 
> to see if there are other issues that come up.

Fair enough.

Thanks,
-- 
Jerome

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Jerome Forissier]

Some platforms may use a single device tree to describe two address
spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
for Secure-only devices"). For these platforms it makes sense to define
a secure counterpart of /chosen, namely: /secure-chosen. This new node
is meant to be used by the secure firmware to pass data to the secure
OS. Only the stdout-path property is supported for now.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 Documentation/devicetree/bindings/arm/secure.txt | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/secure.txt b/Documentation/devicetree/bindings/arm/secure.txt
index e31303f..e7c596a 100644
--- a/Documentation/devicetree/bindings/arm/secure.txt
+++ b/Documentation/devicetree/bindings/arm/secure.txt
@@ -32,7 +32,8 @@ describe the view of Secure world using the standard bindings. These
 secure- bindings only need to be used where both the Secure and Normal
 world views need to be described in a single device tree.
 
-Valid Secure world properties:
+Valid Secure world properties
+-----------------------------
 
 - secure-status : specifies whether the device is present and usable
   in the secure world. The combination of this with "status" allows
@@ -51,3 +52,15 @@ Valid Secure world properties:
    status = "disabled"; secure-status = "okay";     /* S-only */
    status = "disabled";                             /* disabled in both */
    status = "disabled"; secure-status = "disabled"; /* disabled in both */
+
+The secure-chosen node
+----------------------
+
+Similar to the /chosen node which serves as a place for passing data
+between firmware and the operating system, the /secure-chosen node may
+be used to pass data to the secure OS. Only the properties defined
+below may appear in the /secure-chosen node. They have the same
+definition as when used under /chosen, unless explicitely stated
+otherwise.
+
+- stdout-path
-- 
2.7.4

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Peter Maydell]

On 1 March 2017 at 17:08, Jerome Forissier <jerome.forissier@linaro.org> wrote:
> Some platforms may use a single device tree to describe two address
> spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
> for Secure-only devices"). For these platforms it makes sense to define
> a secure counterpart of /chosen, namely: /secure-chosen. This new node
> is meant to be used by the secure firmware to pass data to the secure
> OS. Only the stdout-path property is supported for now.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/secure.txt | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/secure.txt b/Documentation/devicetree/bindings/arm/secure.txt
> index e31303f..e7c596a 100644
> --- a/Documentation/devicetree/bindings/arm/secure.txt
> +++ b/Documentation/devicetree/bindings/arm/secure.txt
> @@ -32,7 +32,8 @@ describe the view of Secure world using the standard bindings. These
>  secure- bindings only need to be used where both the Secure and Normal
>  world views need to be described in a single device tree.
>
> -Valid Secure world properties:
> +Valid Secure world properties
> +-----------------------------
>
>  - secure-status : specifies whether the device is present and usable
>    in the secure world. The combination of this with "status" allows
> @@ -51,3 +52,15 @@ Valid Secure world properties:
>     status = "disabled"; secure-status = "okay";     /* S-only */
>     status = "disabled";                             /* disabled in both */
>     status = "disabled"; secure-status = "disabled"; /* disabled in both */
> +
> +The secure-chosen node
> +----------------------
> +
> +Similar to the /chosen node which serves as a place for passing data
> +between firmware and the operating system, the /secure-chosen node may
> +be used to pass data to the secure OS. Only the properties defined
> +below may appear in the /secure-chosen node. They have the same
> +definition as when used under /chosen, unless explicitely stated

typo: "explicitly".

> +otherwise.
> +
> +- stdout-path

What's the default for the Secure world if (a) the secure-chosen
node doesn't exist at all or (b) it does exist but doesn't
define stdout-path? Presumably it should be "fall back to
using the chosen node's stdout-path", to match the way we
do fallback for other secure world properties, but it would
be good to say so explicitly I think.

thanks
-- PMM

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Robin Murphy]

On 01/03/17 17:43, Peter Maydell wrote:
> On 1 March 2017 at 17:08, Jerome Forissier <jerome.forissier@linaro.org> wrote:
>> Some platforms may use a single device tree to describe two address
>> spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
>> for Secure-only devices"). For these platforms it makes sense to define
>> a secure counterpart of /chosen, namely: /secure-chosen. This new node
>> is meant to be used by the secure firmware to pass data to the secure
>> OS. Only the stdout-path property is supported for now.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/arm/secure.txt | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/secure.txt b/Documentation/devicetree/bindings/arm/secure.txt
>> index e31303f..e7c596a 100644
>> --- a/Documentation/devicetree/bindings/arm/secure.txt
>> +++ b/Documentation/devicetree/bindings/arm/secure.txt
>> @@ -32,7 +32,8 @@ describe the view of Secure world using the standard bindings. These
>>  secure- bindings only need to be used where both the Secure and Normal
>>  world views need to be described in a single device tree.
>>
>> -Valid Secure world properties:
>> +Valid Secure world properties
>> +-----------------------------
>>
>>  - secure-status : specifies whether the device is present and usable
>>    in the secure world. The combination of this with "status" allows
>> @@ -51,3 +52,15 @@ Valid Secure world properties:
>>     status = "disabled"; secure-status = "okay";     /* S-only */
>>     status = "disabled";                             /* disabled in both */
>>     status = "disabled"; secure-status = "disabled"; /* disabled in both */
>> +
>> +The secure-chosen node
>> +----------------------
>> +
>> +Similar to the /chosen node which serves as a place for passing data
>> +between firmware and the operating system, the /secure-chosen node may
>> +be used to pass data to the secure OS. Only the properties defined
>> +below may appear in the /secure-chosen node. They have the same
>> +definition as when used under /chosen, unless explicitely stated
> 
> typo: "explicitly".
> 
>> +otherwise.
>> +
>> +- stdout-path
> 
> What's the default for the Secure world if (a) the secure-chosen
> node doesn't exist at all or (b) it does exist but doesn't
> define stdout-path? Presumably it should be "fall back to
> using the chosen node's stdout-path", to match the way we
> do fallback for other secure world properties, but it would
> be good to say so explicitly I think.

I'd agree that it would be reasonable for the secure OS to fall back to
parsing /chosen in the absence of /secure-chosen, but if the latter is
present I would (at least naively) expect it to be authoritative. I can
imagine the case of a cut-down version of some system implementing only
one UART, where you might want to tell the Normal world OS to use that
and the Secure OS to keep its output to itself - allowing fallbacks at
the individual property level would make that harder than it needs to
be, and in general seems like it might be more confusing than useful.

Maintaining the principal that "secure-X" takes complete precedence over
"X" seems to me to be the least surprising; it's just that in this one
case X gets to be an entire node rather than a property because /chosen
is special.

Robin.

> 
> thanks
> -- PMM
> 

[PATCH] Documentation: dt: Add binding for /secure-chosen/stdout-path

[Jerome Forissier]

On 03/01/2017 07:42 PM, Robin Murphy wrote:
> On 01/03/17 17:43, Peter Maydell wrote:
>> On 1 March 2017 at 17:08, Jerome Forissier <jerome.forissier@linaro.org> wrote:
>>> Some platforms may use a single device tree to describe two address
>>> spaces, as described in d9f43babb998 ("Documentation: dt: Add bindings
>>> for Secure-only devices"). For these platforms it makes sense to define
>>> a secure counterpart of /chosen, namely: /secure-chosen. This new node
>>> is meant to be used by the secure firmware to pass data to the secure
>>> OS. Only the stdout-path property is supported for now.
>>>
>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/arm/secure.txt | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/secure.txt b/Documentation/devicetree/bindings/arm/secure.txt
>>> index e31303f..e7c596a 100644
>>> --- a/Documentation/devicetree/bindings/arm/secure.txt
>>> +++ b/Documentation/devicetree/bindings/arm/secure.txt
>>> @@ -32,7 +32,8 @@ describe the view of Secure world using the standard bindings. These
>>>  secure- bindings only need to be used where both the Secure and Normal
>>>  world views need to be described in a single device tree.
>>>
>>> -Valid Secure world properties:
>>> +Valid Secure world properties
>>> +-----------------------------
>>>
>>>  - secure-status : specifies whether the device is present and usable
>>>    in the secure world. The combination of this with "status" allows
>>> @@ -51,3 +52,15 @@ Valid Secure world properties:
>>>     status = "disabled"; secure-status = "okay";     /* S-only */
>>>     status = "disabled";                             /* disabled in both */
>>>     status = "disabled"; secure-status = "disabled"; /* disabled in both */
>>> +
>>> +The secure-chosen node
>>> +----------------------
>>> +
>>> +Similar to the /chosen node which serves as a place for passing data
>>> +between firmware and the operating system, the /secure-chosen node may
>>> +be used to pass data to the secure OS. Only the properties defined
>>> +below may appear in the /secure-chosen node. They have the same
>>> +definition as when used under /chosen, unless explicitely stated
>>
>> typo: "explicitly".
>>
>>> +otherwise.
>>> +
>>> +- stdout-path
>>
>> What's the default for the Secure world if (a) the secure-chosen
>> node doesn't exist at all or (b) it does exist but doesn't
>> define stdout-path? Presumably it should be "fall back to
>> using the chosen node's stdout-path", to match the way we
>> do fallback for other secure world properties, but it would
>> be good to say so explicitly I think.
> 
> I'd agree that it would be reasonable for the secure OS to fall back to
> parsing /chosen in the absence of /secure-chosen,

I don't think that "fall back to using stuff from /chosen" is generally
useful. Indeed, of all the properties I can see mentioned for /chosen
("stdout-path", "linux,syrq-reset-seq", "linux,pci-probe-only",
"bootargs", "initrd_start" and "initrd_end"), only stdout-path is likely
to be usable by the Secure OS as a fall back. So I'd rather make this an
exception rather than the rule.

> but if the latter is
> present I would (at least naively) expect it to be authoritative.

Agreed.

> I can
> imagine the case of a cut-down version of some system implementing only
> one UART, where you might want to tell the Normal world OS to use that
> and the Secure OS to keep its output to itself - allowing fallbacks at
> the individual property level would make that harder than it needs to
> be, and in general seems like it might be more confusing than useful.

Yup.

> Maintaining the principal that "secure-X" takes complete precedence over
> "X" seems to me to be the least surprising; it's just that in this one
> case X gets to be an entire node rather than a property because /chosen
> is special.

OK, can we agree on the following?

- The secure OS is supposed to get its boot data from /secure-chosen
- Only the stdout-path path property is defined currently. Its
definition is the same as for /chosen. If not present but /secure-chosen
is present, it takes no default value. If /secure-chosen does not exist
however, it defaults to the value of /chosen/stdout-path.
- As we add properties to /secure-chosen we'll see if it makes sense to
allow fall back to their counterparts in /chosen (which I don't expect
to happen too often).

If that sounds good to you I'll send a V2.

Thanks,
-- 
Jerome

> 
> Robin.
> 
>>
>> thanks
>> -- PMM
>>
>