From: Peng Fan <[email protected]>
The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
actions in software layers running in the EL2 or EL3 exception levels.
The term "ARM" here relates to the SMC instruction as part of the ARM
instruction set, not as a standard endorsed by ARM Ltd.
Signed-off-by: Peng Fan <[email protected]>
---
.../devicetree/bindings/mailbox/arm-smc.yaml | 96 ++++++++++++++++++++++
1 file changed, 96 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
new file mode 100644
index 000000000000..bf01bec035fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM SMC Mailbox Interface
+
+maintainers:
+ - Peng Fan <[email protected]>
+
+description: |
+ This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
+ call) instruction to trigger a mailbox-connected activity in firmware,
+ executing on the very same core as the caller. The value of r0/w0/x0
+ the firmware returns after the smc call is delivered as a received
+ message to the mailbox framework, so synchronous communication can be
+ established. The exact meaning of the action the mailbox triggers as
+ well as the return value is defined by their users and is not subject
+ to this binding.
+
+ One use case of this mailbox is the SCMI interface, which uses shared
+ memory to transfer commands and parameters, and a mailbox to trigger a
+ function call. This allows SoCs without a separate management processor
+ (or when such a processor is not available or used) to use this
+ standardized interface anyway.
+
+ This binding describes no hardware, but establishes a firmware interface.
+ Upon receiving an SMC using one of the described SMC function identifiers,
+ the firmware is expected to trigger some mailbox connected functionality.
+ The communication follows the ARM SMC calling convention.
+ Firmware expects an SMC function identifier in r0 or w0. The supported
+ identifiers are passed from consumers, or listed in the the arm,func-ids
+ properties as described below. The firmware can return one value in
+ the first SMC result register, it is expected to be an error value,
+ which shall be propagated to the mailbox client.
+
+ Any core which supports the SMC or HVC instruction can be used, as long
+ as a firmware component running in EL3 or EL2 is handling these calls.
+
+properties:
+ compatible:
+ oneOf:
+ - description:
+ For implementations using ARM SMC instruction.
+ const: arm,smc-mbox
+
+ - description:
+ For implementations using ARM HVC instruction.
+ const: arm,hvc-mbox
+
+ "#mbox-cells":
+ const: 1
+
+ arm,func-id:
+ description: |
+ An 32-bit value specifying the function ID used by the mailbox.
+ The function ID follow the ARM SMC calling convention standard [1].
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+ - compatible
+ - "#mbox-cells"
+
+examples:
+ - |
+ sram@93f000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x93f000 0x0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x93f000 0x1000>;
+
+ cpu_scp_lpri: scp-shmem@0 {
+ compatible = "arm,scmi-shmem";
+ reg = <0x0 0x200>;
+ };
+ };
+
+ smc_tx_mbox: tx_mbox {
+ #mbox-cells = <1>;
+ compatible = "arm,smc-mbox";
+ /* optional */
+ arm,func-id = <0xc20000fe>;
+ };
+
+ firmware {
+ scmi {
+ compatible = "arm,scmi";
+ mboxes = <&smc_tx_mbox 0>;
+ mbox-names = "tx";
+ shmem = <&cpu_scp_lpri>;
+ };
+ };
+
+...
--
2.16.4
On Mon, 16 Sep 2019 09:44:37 +0000
Peng Fan <[email protected]> wrote:
Hi,
> From: Peng Fan <[email protected]>
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> .../devicetree/bindings/mailbox/arm-smc.yaml | 96 ++++++++++++++++++++++
> 1 file changed, 96 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..bf01bec035fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan <[email protected]>
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
I think "or" instead of "and" is less confusing.
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. The value of r0/w0/x0
> + the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so synchronous communication can be
> + established. The exact meaning of the action the mailbox triggers as
> + well as the return value is defined by their users and is not subject
> + to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared
One example use case of this mailbox ...
(to make it more obvious that it's not restricted to this)
> + memory to transfer commands and parameters, and a mailbox to trigger a
> + function call. This allows SoCs without a separate management processor
> + (or when such a processor is not available or used) to use this
> + standardized interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
... the described SMC function identifier,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers,
identifier
"passed from consumers": How? Where?
But I want to repeat: We should not allow this. This is a binding for a mailbox controller driver, not a generic firmware backdoor.
We should be as strict as possible to avoid any security issues.
The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
What would be the use case of this functionality?
> or listed in the the arm,func-ids
arm,func-id
> + properties as described below. The firmware can return one value in
property
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long
> + as a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> + oneOf:
> + - description:
> + For implementations using ARM SMC instruction.
> + const: arm,smc-mbox
> +
> + - description:
> + For implementations using ARM HVC instruction.
> + const: arm,hvc-mbox
I am not particularly happy with this, but well ...
> +
> + "#mbox-cells":
> + const: 1
Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
> +
> + arm,func-id:
> + description: |
> + An 32-bit value specifying the function ID used by the mailbox.
A single 32-bit value ...
> + The function ID follow the ARM SMC calling convention standard [1].
follows
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> +
> +examples:
> + - |
> + sram@93f000 {
> + compatible = "mmio-sram";
> + reg = <0x0 0x93f000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x93f000 0x1000>;
> +
> + cpu_scp_lpri: scp-shmem@0 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x0 0x200>;
> + };
> + };
> +
> + smc_tx_mbox: tx_mbox {
> + #mbox-cells = <1>;
As mentioned above, should be 0.
> + compatible = "arm,smc-mbox";
> + /* optional */
First: having "optional" in a specific example is not helpful, just confusing.
Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
> + arm,func-id = <0xc20000fe>;
> + };
> +
> + firmware {
> + scmi {
> + compatible = "arm,scmi";
> + mboxes = <&smc_tx_mbox 0>;
... and here just <&smc_tx_mbox>; would suffice.
> + mbox-names = "tx";
> + shmem = <&cpu_scp_lpri>;
> + };
> + };
> +
> +...
Cheers,
Andre.
On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara <[email protected]> wrote:
>
> On Mon, 16 Sep 2019 09:44:37 +0000
> Peng Fan <[email protected]> wrote:
>
> Hi,
>
> > From: Peng Fan <[email protected]>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> > actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > .../devicetree/bindings/mailbox/arm-smc.yaml | 96 ++++++++++++++++++++++
> > 1 file changed, 96 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..bf01bec035fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,96 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > + - Peng Fan <[email protected]>
> > +
> > +description: |
> > + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
>
> I think "or" instead of "and" is less confusing.
>
> > + call) instruction to trigger a mailbox-connected activity in firmware,
> > + executing on the very same core as the caller. The value of r0/w0/x0
> > + the firmware returns after the smc call is delivered as a received
> > + message to the mailbox framework, so synchronous communication can be
> > + established. The exact meaning of the action the mailbox triggers as
> > + well as the return value is defined by their users and is not subject
> > + to this binding.
> > +
> > + One use case of this mailbox is the SCMI interface, which uses shared
>
> One example use case of this mailbox ...
> (to make it more obvious that it's not restricted to this)
>
> > + memory to transfer commands and parameters, and a mailbox to trigger a
> > + function call. This allows SoCs without a separate management processor
> > + (or when such a processor is not available or used) to use this
> > + standardized interface anyway.
> > +
> > + This binding describes no hardware, but establishes a firmware interface.
> > + Upon receiving an SMC using one of the described SMC function identifiers,
>
> ... the described SMC function identifier,
>
> > + the firmware is expected to trigger some mailbox connected functionality.
> > + The communication follows the ARM SMC calling convention.
> > + Firmware expects an SMC function identifier in r0 or w0. The supported
> > + identifiers are passed from consumers,
>
> identifier
>
> "passed from consumers": How? Where?
> But I want to repeat: We should not allow this.
> This is a binding for a mailbox controller driver, not a generic firmware backdoor.
>
Exactly. The mailbox controller here is the SMC/HVC instruction,
which needs 9 arguments to work. The fact that the fist argument is
always going to be same on a platform is just the way we use this
instruction.
> We should be as strict as possible to avoid any security issues.
>
Any example of such a security issue?
> The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
>
> What would be the use case of this functionality?
>
At least for flexibility and consistency.
> > or listed in the the arm,func-ids
>
> arm,func-id
>
> > + properties as described below. The firmware can return one value in
>
> property
>
> > + the first SMC result register, it is expected to be an error value,
> > + which shall be propagated to the mailbox client.
> > +
> > + Any core which supports the SMC or HVC instruction can be used, as long
> > + as a firmware component running in EL3 or EL2 is handling these calls.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - description:
> > + For implementations using ARM SMC instruction.
> > + const: arm,smc-mbox
> > +
> > + - description:
> > + For implementations using ARM HVC instruction.
> > + const: arm,hvc-mbox
>
> I am not particularly happy with this, but well ...
>
> > +
> > + "#mbox-cells":
> > + const: 1
>
> Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
>
Yes. I overlooked it and actually queued the patch for pull request.
But I think the bindings should not carry a 'fix' patch later. Also I
realise this revision of binding hasn't been reviewed by Rob. Maybe I
should drop the patch for now.
> > +
> > + arm,func-id:
> > + description: |
> > + An 32-bit value specifying the function ID used by the mailbox.
>
> A single 32-bit value ...
>
> > + The function ID follow the ARM SMC calling convention standard [1].
>
> follows
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > + - compatible
> > + - "#mbox-cells"
> > +
> > +examples:
> > + - |
> > + sram@93f000 {
> > + compatible = "mmio-sram";
> > + reg = <0x0 0x93f000 0x0 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0x0 0x93f000 0x1000>;
> > +
> > + cpu_scp_lpri: scp-shmem@0 {
> > + compatible = "arm,scmi-shmem";
> > + reg = <0x0 0x200>;
> > + };
> > + };
> > +
> > + smc_tx_mbox: tx_mbox {
> > + #mbox-cells = <1>;
>
> As mentioned above, should be 0.
>
> > + compatible = "arm,smc-mbox";
> > + /* optional */
>
> First: having "optional" in a specific example is not helpful, just confusing.
> Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
> I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
>
Again, the mailbox controller here is the SMC/HVC _instruction_, which
doesn't care what value the first argument carry.
Cheers!
Hi Jassi,
> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
>
> On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara
> <[email protected]> wrote:
> >
> > On Mon, 16 Sep 2019 09:44:37 +0000
> > Peng Fan <[email protected]> wrote:
> >
> > Hi,
> >
> > > From: Peng Fan <[email protected]>
> > >
> > > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > > trigger actions in software layers running in the EL2 or EL3 exception
> levels.
> > > The term "ARM" here relates to the SMC instruction as part of the
> > > ARM instruction set, not as a standard endorsed by ARM Ltd.
> > >
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > > .../devicetree/bindings/mailbox/arm-smc.yaml | 96
> ++++++++++++++++++++++
> > > 1 file changed, 96 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > new file mode 100644
> > > index 000000000000..bf01bec035fc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > @@ -0,0 +1,96 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > >
> +vicetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&data=02%
> 7C01
> > >
> +%7Cpeng.fan%40nxp.com%7Cf8065d24dd474238baf008d73bf8dc7a%7C686
> ea1d3
> > >
> +bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637043812342903260&sd
> ata=vC3
> > >
> +S8hvYDxDhNbIQoC44hpO5bw1yYZdBwu%2B%2Fp8mV0hI%3D&reserv
> ed=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > >
> +vicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7C
> peng.
> > >
> +fan%40nxp.com%7Cf8065d24dd474238baf008d73bf8dc7a%7C686ea1d3bc2
> b4c6f
> > >
> +a92cd99c5c301635%7C0%7C1%7C637043812342903260&sdata=IDHd
> vf1Mgw1
> > > +BR%2Bo4XJ%2BjQS%2Bx1pSBzADnW44B2hZLzKw%3D&reserved=0
> > > +
> > > +title: ARM SMC Mailbox Interface
> > > +
> > > +maintainers:
> > > + - Peng Fan <[email protected]>
> > > +
> > > +description: |
> > > + This mailbox uses the ARM smc (secure monitor call) and hvc
> > > +(hypervisor
> >
> > I think "or" instead of "and" is less confusing.
> >
> > > + call) instruction to trigger a mailbox-connected activity in
> > > + firmware, executing on the very same core as the caller. The
> > > + value of r0/w0/x0 the firmware returns after the smc call is
> > > + delivered as a received message to the mailbox framework, so
> > > + synchronous communication can be established. The exact meaning
> > > + of the action the mailbox triggers as well as the return value is
> > > + defined by their users and is not subject to this binding.
> > > +
> > > + One use case of this mailbox is the SCMI interface, which uses
> > > + shared
> >
> > One example use case of this mailbox ...
> > (to make it more obvious that it's not restricted to this)
> >
> > > + memory to transfer commands and parameters, and a mailbox to
> > > + trigger a function call. This allows SoCs without a separate
> > > + management processor (or when such a processor is not available
> > > + or used) to use this standardized interface anyway.
> > > +
> > > + This binding describes no hardware, but establishes a firmware
> interface.
> > > + Upon receiving an SMC using one of the described SMC function
> > > + identifiers,
> >
> > ... the described SMC function
> > identifier,
> >
> > > + the firmware is expected to trigger some mailbox connected
> functionality.
> > > + The communication follows the ARM SMC calling convention.
> > > + Firmware expects an SMC function identifier in r0 or w0. The
> > > + supported identifiers are passed from consumers,
> >
> > identifier
> >
> > "passed from consumers": How? Where?
> > But I want to repeat: We should not allow this.
> > This is a binding for a mailbox controller driver, not a generic firmware
> backdoor.
> >
> Exactly. The mailbox controller here is the SMC/HVC instruction, which
> needs 9 arguments to work. The fact that the fist argument is always going to
> be same on a platform is just the way we use this instruction.
>
> > We should be as strict as possible to avoid any security issues.
> >
> Any example of such a security issue?
>
> > The firmware certainly knows the function ID it implements. The firmware
> controls the DT. So it is straight-forward to put the ID into the DT. The
> firmware could even do this at boot time, dynamically, before passing on the
> DT to the non-secure world (bootloader or kernel).
> >
> > What would be the use case of this functionality?
> >
> At least for flexibility and consistency.
>
> > > or listed in the the arm,func-ids
> >
> > arm,func-id
> >
> > > + properties as described below. The firmware can return one value
> > > + in
> >
> > property
> >
> > > + the first SMC result register, it is expected to be an error
> > > + value, which shall be propagated to the mailbox client.
> > > +
> > > + Any core which supports the SMC or HVC instruction can be used,
> > > + as long as a firmware component running in EL3 or EL2 is handling
> these calls.
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - description:
> > > + For implementations using ARM SMC instruction.
> > > + const: arm,smc-mbox
> > > +
> > > + - description:
> > > + For implementations using ARM HVC instruction.
> > > + const: arm,hvc-mbox
> >
> > I am not particularly happy with this, but well ...
> >
> > > +
> > > + "#mbox-cells":
> > > + const: 1
> >
> > Why is this "1"? What is this number used for? It used to be the channel ID,
> but since you are describing a single channel controller only, this should be 0
> now.
> >
> Yes. I overlooked it and actually queued the patch for pull request.
In Documentation/devicetree/bindings/mailbox/mailbox.txt
#mbox-cells: Must be at least 1.
So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1 here.
So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
and add xlate for smc mailbox?
Thanks,
Peng.
> But I think the bindings should not carry a 'fix' patch later. Also I realise this
> revision of binding hasn't been reviewed by Rob. Maybe I should drop the
> patch for now.
>
> > > +
> > > + arm,func-id:
> > > + description: |
> > > + An 32-bit value specifying the function ID used by the mailbox.
> >
> > A single 32-bit value ...
> >
> > > + The function ID follow the ARM SMC calling convention standard
> [1].
> >
> > follows
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +required:
> > > + - compatible
> > > + - "#mbox-cells"
> > > +
> > > +examples:
> > > + - |
> > > + sram@93f000 {
> > > + compatible = "mmio-sram";
> > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + ranges = <0x0 0x93f000 0x1000>;
> > > +
> > > + cpu_scp_lpri: scp-shmem@0 {
> > > + compatible = "arm,scmi-shmem";
> > > + reg = <0x0 0x200>;
> > > + };
> > > + };
> > > +
> > > + smc_tx_mbox: tx_mbox {
> > > + #mbox-cells = <1>;
> >
> > As mentioned above, should be 0.
> >
> > > + compatible = "arm,smc-mbox";
> > > + /* optional */
> >
> > First: having "optional" in a specific example is not helpful, just confusing.
> > Second: It is actually *not* optional in this case, as there is no other way of
> propagating the function ID. The SCMI driver as the mailbox client has
> certainly no clue about this.
> > I think I said this previously: Relying on the mailbox client to pass the
> function ID sounds broken, as this is a property of the mailbox controller driver.
> The mailbox client does not care about this mailbox communication detail, it
> just wants to trigger the mailbox.
> >
> Again, the mailbox controller here is the SMC/HVC _instruction_, which
> doesn't care what value the first argument carry.
>
> Cheers!
Hi Andre,
> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
>
> On Mon, 16 Sep 2019 09:44:37 +0000
> Peng Fan <[email protected]> wrote:
>
> Hi,
>
> > From: Peng Fan <[email protected]>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > trigger actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > .../devicetree/bindings/mailbox/arm-smc.yaml | 96
> ++++++++++++++++++++++
> > 1 file changed, 96 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..bf01bec035fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,96 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&data=02%7
> C01%7Cp
> >
> +eng.fan%40nxp.com%7Cff378bc3d622436c39ba08d73b94dfcc%7C686ea1d
> 3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C1%7C637043382928045369&sdata=rnx
> KdDGjPPd
> > +8VBI5WmgnZ3jxIjL2hcRYzbljfFxDkA0%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cpe
> ng.fan%
> >
> +40nxp.com%7Cff378bc3d622436c39ba08d73b94dfcc%7C686ea1d3bc2b4c6
> fa92cd9
> >
> +9c5c301635%7C0%7C1%7C637043382928045369&sdata=R02nWzpp9
> %2BrDYG9tA
> > +ot4pdWb8tGGHet1MOjrD0dEjwA%3D&reserved=0
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > + - Peng Fan <[email protected]>
> > +
> > +description: |
> > + This mailbox uses the ARM smc (secure monitor call) and hvc
> > +(hypervisor
>
> I think "or" instead of "and" is less confusing.
ok
>
> > + call) instruction to trigger a mailbox-connected activity in
> > + firmware, executing on the very same core as the caller. The value
> > + of r0/w0/x0 the firmware returns after the smc call is delivered as
> > + a received message to the mailbox framework, so synchronous
> > + communication can be established. The exact meaning of the action
> > + the mailbox triggers as well as the return value is defined by
> > + their users and is not subject to this binding.
> > +
> > + One use case of this mailbox is the SCMI interface, which uses
> > + shared
>
> One example use case of this mailbox ...
> (to make it more obvious that it's not restricted to this)
ok
>
> > + memory to transfer commands and parameters, and a mailbox to
> > + trigger a function call. This allows SoCs without a separate
> > + management processor (or when such a processor is not available or
> > + used) to use this standardized interface anyway.
> > +
> > + This binding describes no hardware, but establishes a firmware
> interface.
> > + Upon receiving an SMC using one of the described SMC function
> > + identifiers,
>
> ... the described SMC function identifier,
ok
>
> > + the firmware is expected to trigger some mailbox connected
> functionality.
> > + The communication follows the ARM SMC calling convention.
> > + Firmware expects an SMC function identifier in r0 or w0. The
> > + supported identifiers are passed from consumers,
>
> identifier
ok
>
> "passed from consumers": How? Where?
> But I want to repeat: We should not allow this. This is a binding for a mailbox
> controller driver, not a generic firmware backdoor.
As Jassi suggested the function identifier as an optional for mailbox driver.
The driver should support function id passed from consumers.
Currently there is no users for such case that passed from consumers,
so I have no idea how.
> We should be as strict as possible to avoid any security issues.
> The firmware certainly knows the function ID it implements. The firmware
> controls the DT. So it is straight-forward to put the ID into the DT. The
> firmware could even do this at boot time, dynamically, before passing on the
> DT to the non-secure world (bootloader or kernel).
>
> What would be the use case of this functionality?
>
> > or listed in the the arm,func-ids
>
> arm,func-id
ok
>
> > + properties as described below. The firmware can return one value in
>
> property
ok
>
> > + the first SMC result register, it is expected to be an error value,
> > + which shall be propagated to the mailbox client.
> > +
> > + Any core which supports the SMC or HVC instruction can be used, as
> > + long as a firmware component running in EL3 or EL2 is handling these
> calls.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - description:
> > + For implementations using ARM SMC instruction.
> > + const: arm,smc-mbox
> > +
> > + - description:
> > + For implementations using ARM HVC instruction.
> > + const: arm,hvc-mbox
>
> I am not particularly happy with this, but well ...
>
> > +
> > + "#mbox-cells":
> > + const: 1
>
> Why is this "1"? What is this number used for? It used to be the channel ID,
> but since you are describing a single channel controller only, this should be 0
> now.
Mailbox bindings requires it at least 1, as replied to Jassi in the other mail.
>
> > +
> > + arm,func-id:
> > + description: |
> > + An 32-bit value specifying the function ID used by the mailbox.
>
> A single 32-bit value ...
>
> > + The function ID follow the ARM SMC calling convention standard
> [1].
>
> follows
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > + - compatible
> > + - "#mbox-cells"
> > +
> > +examples:
> > + - |
> > + sram@93f000 {
> > + compatible = "mmio-sram";
> > + reg = <0x0 0x93f000 0x0 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0x0 0x93f000 0x1000>;
> > +
> > + cpu_scp_lpri: scp-shmem@0 {
> > + compatible = "arm,scmi-shmem";
> > + reg = <0x0 0x200>;
> > + };
> > + };
> > +
> > + smc_tx_mbox: tx_mbox {
> > + #mbox-cells = <1>;
>
> As mentioned above, should be 0.
>
> > + compatible = "arm,smc-mbox";
> > + /* optional */
>
> First: having "optional" in a specific example is not helpful, just confusing.
> Second: It is actually *not* optional in this case, as there is no other way of
> propagating the function ID. The SCMI driver as the mailbox client has
> certainly no clue about this.
I'll drop "/*optinal*/" since it is required in the example.
> I think I said this previously: Relying on the mailbox client to pass the function
> ID sounds broken, as this is a property of the mailbox controller driver. The
> mailbox client does not care about this mailbox communication detail, it just
> wants to trigger the mailbox.
>
> > + arm,func-id = <0xc20000fe>;
> > + };
> > +
> > + firmware {
> > + scmi {
> > + compatible = "arm,scmi";
> > + mboxes = <&smc_tx_mbox 0>;
>
> ... and here just <&smc_tx_mbox>; would suffice.
Mailbox requires mbox-cells at least 1, it must have one arg.
Otherwise of_mbox_index_xlate not work.
Thanks,
Peng.
>
> > + mbox-names = "tx";
> > + shmem = <&cpu_scp_lpri>;
> > + };
> > + };
> > +
> > +...
>
> Cheers,
> Andre.
On Wed, 18 Sep 2019 00:27:00 -0500
Jassi Brar <[email protected]> wrote:
Hi,
> On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara <[email protected]> wrote:
> >
> > On Mon, 16 Sep 2019 09:44:37 +0000
> > Peng Fan <[email protected]> wrote:
> >
> > Hi,
> >
> > > From: Peng Fan <[email protected]>
> > >
> > > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> > > actions in software layers running in the EL2 or EL3 exception levels.
> > > The term "ARM" here relates to the SMC instruction as part of the ARM
> > > instruction set, not as a standard endorsed by ARM Ltd.
> > >
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > > .../devicetree/bindings/mailbox/arm-smc.yaml | 96 ++++++++++++++++++++++
> > > 1 file changed, 96 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > new file mode 100644
> > > index 000000000000..bf01bec035fc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > @@ -0,0 +1,96 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ARM SMC Mailbox Interface
> > > +
> > > +maintainers:
> > > + - Peng Fan <[email protected]>
> > > +
> > > +description: |
> > > + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
> >
> > I think "or" instead of "and" is less confusing.
> >
> > > + call) instruction to trigger a mailbox-connected activity in firmware,
> > > + executing on the very same core as the caller. The value of r0/w0/x0
> > > + the firmware returns after the smc call is delivered as a received
> > > + message to the mailbox framework, so synchronous communication can be
> > > + established. The exact meaning of the action the mailbox triggers as
> > > + well as the return value is defined by their users and is not subject
> > > + to this binding.
> > > +
> > > + One use case of this mailbox is the SCMI interface, which uses shared
> >
> > One example use case of this mailbox ...
> > (to make it more obvious that it's not restricted to this)
> >
> > > + memory to transfer commands and parameters, and a mailbox to trigger a
> > > + function call. This allows SoCs without a separate management processor
> > > + (or when such a processor is not available or used) to use this
> > > + standardized interface anyway.
> > > +
> > > + This binding describes no hardware, but establishes a firmware interface.
> > > + Upon receiving an SMC using one of the described SMC function identifiers,
> >
> > ... the described SMC function identifier,
> >
> > > + the firmware is expected to trigger some mailbox connected functionality.
> > > + The communication follows the ARM SMC calling convention.
> > > + Firmware expects an SMC function identifier in r0 or w0. The supported
> > > + identifiers are passed from consumers,
> >
> > identifier
> >
> > "passed from consumers": How? Where?
> > But I want to repeat: We should not allow this.
> > This is a binding for a mailbox controller driver, not a generic firmware backdoor.
> >
> Exactly. The mailbox controller here is the SMC/HVC instruction,
No, the mailbox controller is an *SMCCC compliant* smc/hvc call, targeting a very specific function ID. SMC calls are used for PSCI already, for instance, and you don't want to mess with that. Also some platforms define a vendor specific smc interface, again using a well constructed function ID complying to SMCCC.
So we definitely need to stay within SMCCC for this kind of generic interface, *and* to let firmware specify the function ID via the DT, to not clash with any other function ID.
> which needs 9 arguments to work. The fact that the fist argument is
> always going to be same on a platform is just the way we use this
> instruction.
>
> > We should be as strict as possible to avoid any security issues.
> >
> Any example of such a security issue?
Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
Do you have any example of a use case where the mailbox client needs to provide the function ID?
> > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> >
> > What would be the use case of this functionality?
> >
> At least for flexibility and consistency.
I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
> > > or listed in the the arm,func-ids
> >
> > arm,func-id
> >
> > > + properties as described below. The firmware can return one value in
> >
> > property
> >
> > > + the first SMC result register, it is expected to be an error value,
> > > + which shall be propagated to the mailbox client.
> > > +
> > > + Any core which supports the SMC or HVC instruction can be used, as long
> > > + as a firmware component running in EL3 or EL2 is handling these calls.
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - description:
> > > + For implementations using ARM SMC instruction.
> > > + const: arm,smc-mbox
> > > +
> > > + - description:
> > > + For implementations using ARM HVC instruction.
> > > + const: arm,hvc-mbox
> >
> > I am not particularly happy with this, but well ...
> >
> > > +
> > > + "#mbox-cells":
> > > + const: 1
> >
> > Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
> >
> Yes. I overlooked it and actually queued the patch for pull request.
> But I think the bindings should not carry a 'fix' patch later. Also I
> realise this revision of binding hasn't been reviewed by Rob. Maybe I
> should drop the patch for now.
Yes, please do. I would like to make sure that the binding is correct, as it serves as a specification for people implementing both firmware services and other drivers (like *BSD).
> > > +
> > > + arm,func-id:
> > > + description: |
> > > + An 32-bit value specifying the function ID used by the mailbox.
> >
> > A single 32-bit value ...
> >
> > > + The function ID follow the ARM SMC calling convention standard [1].
> >
> > follows
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +required:
> > > + - compatible
> > > + - "#mbox-cells"
> > > +
> > > +examples:
> > > + - |
> > > + sram@93f000 {
> > > + compatible = "mmio-sram";
> > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + ranges = <0x0 0x93f000 0x1000>;
> > > +
> > > + cpu_scp_lpri: scp-shmem@0 {
> > > + compatible = "arm,scmi-shmem";
> > > + reg = <0x0 0x200>;
> > > + };
> > > + };
> > > +
> > > + smc_tx_mbox: tx_mbox {
> > > + #mbox-cells = <1>;
> >
> > As mentioned above, should be 0.
> >
> > > + compatible = "arm,smc-mbox";
> > > + /* optional */
> >
> > First: having "optional" in a specific example is not helpful, just confusing.
> > Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
> > I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
> >
> Again, the mailbox controller here is the SMC/HVC _instruction_, which
> doesn't care what value the first argument carry.
That is not true. Just check Peng's example implementation he mentioned in the cover letter:
#define FSL_SIP_SCMI_1 0xc20000fe
#define FSL_SIP_SCMI_2 0xc20000ff
....
case FSL_SIP_SCMI_1:
case FSL_SIP_SCMI_2:
SMC_RET1(handle, scmi_handler(smc_fid, x1, x2, x3));
Definitely the function ID is crucial here.
Cheers,
Andre.
> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
>
> On Wed, Sep 18, 2019 at 3:53 AM Peng Fan <[email protected]> wrote:
>
> > > >
> > > > > +
> > > > > + "#mbox-cells":
> > > > > + const: 1
> > > >
> > > > Why is this "1"? What is this number used for? It used to be the
> > > > channel ID,
> > > but since you are describing a single channel controller only, this
> > > should be 0 now.
> > > >
> > > Yes. I overlooked it and actually queued the patch for pull request.
> >
> > In Documentation/devicetree/bindings/mailbox/mailbox.txt
> > #mbox-cells: Must be at least 1.
> >
> > So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1
> here.
> > So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
> > and add xlate for smc mailbox?
> >
> No, you just can not use the generic xlate() provided by the api.
> Please implement your own xlate() that requires no argument.
ok. Will add xlate.
Thanks,
Peng.
>
> Cheers!
On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <[email protected]> wrote:
>
> > which needs 9 arguments to work. The fact that the fist argument is
> > always going to be same on a platform is just the way we use this
> > instruction.
> >
> > > We should be as strict as possible to avoid any security issues.
> > >
> > Any example of such a security issue?
>
> Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
>
What if someone finds a way to trick the block layer to erase 'sda' ?
That is called "bug in the code".
It does happen in every subsystem but we don't stop implementing new
features .... we write flexible code and then fix any bug.
> Do you have any example of a use case where the mailbox client needs to provide the function ID?
>
FSL_SIP_SCMI_1/2 ?
But that is not the main point, which is to be consistent (not
ignoring first argument because someone may find a bug to exploit) and
flexible.
> > > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> > >
> > > What would be the use case of this functionality?
> > >
> > At least for flexibility and consistency.
>
> I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
>
I really don't see how there can be clashes with more complete and
flexible implementation.
Thanks.
On Wed, 18 Sep 2019 09:19:46 -0500
Jassi Brar <[email protected]> wrote:
Hi,
> On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <[email protected]> wrote:
>
> >
> > > which needs 9 arguments to work. The fact that the fist argument is
> > > always going to be same on a platform is just the way we use this
> > > instruction.
> > >
> > > > We should be as strict as possible to avoid any security issues.
> > > >
> > > Any example of such a security issue?
> >
> > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> >
> What if someone finds a way to trick the block layer to erase 'sda' ?
Yes, the Linux block driver control the whole block device, it can do whatever it wants.
The firmware provides an interface for multiple users, and the mailbox controller just uses *one part* of it. So we should make sure that it's the right part.
> That is called "bug in the code".
> It does happen in every subsystem but we don't stop implementing new
> features .... we write flexible code and then fix any bug.
>
>
> > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> >
> FSL_SIP_SCMI_1/2 ?
Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.
> But that is not the main point, which is to be consistent (not
> ignoring first argument because someone may find a bug to exploit) and
> flexible.
Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.
Please keep in mind that we should *NOT* do smc calls without following the SMCCC spec.
> > > > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> > > >
> > > > What would be the use case of this functionality?
> > > >
> > > At least for flexibility and consistency.
> >
> > I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
> >
> I really don't see how there can be clashes with more complete and
> flexible implementation.
Platform A uses PSCI, but no other SMCCC services, so in your scenario you assign a valid function ID say from the SIP range and tell the mailbox *client* to use it. Now you want to run this client on platform B, which happens to have chosen this very function ID for the "set fire on the device" SMCCC service.
So now you would need to assign different IDs to the *client*, depending on the platform? That doesn't make sense. The solution is that you just assign the function ID to the controller, in the (platform specific) DT, so it naturally matches the platform requirements. You can even change it over time, as long as you change both the DT and firmware at the same time.
The client (SCPI, for instance) is totally ignorant about this communication detail, it just needs some mailbox to request services.
That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.
Cheers,
Andre.
[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
On Wed, Sep 18, 2019 at 9:46 AM Andre Przywara <[email protected]> wrote:
>
> On Wed, 18 Sep 2019 09:19:46 -0500
> Jassi Brar <[email protected]> wrote:
>
> Hi,
>
> > On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <[email protected]> wrote:
> >
> > >
> > > > which needs 9 arguments to work. The fact that the fist argument is
> > > > always going to be same on a platform is just the way we use this
> > > > instruction.
> > > >
> > > > > We should be as strict as possible to avoid any security issues.
> > > > >
> > > > Any example of such a security issue?
> > >
> > > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> > >
> > What if someone finds a way to trick the block layer to erase 'sda' ?
>
> Yes, the Linux block driver control the whole block device, it can do whatever it wants.
>
Sorry, it doesn't make any sense.
> > That is called "bug in the code".
> > It does happen in every subsystem but we don't stop implementing new
> > features .... we write flexible code and then fix any bug.
> >
> >
> > > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> > >
> > FSL_SIP_SCMI_1/2 ?
>
> Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.
>
SCPI/SCMI implementation is broken. I did NAK it.
With the 'smc' mailbox you may get away without have to program the
channel before transmit, but not every controller is natively so.
> > But that is not the main point, which is to be consistent (not
> > ignoring first argument because someone may find a bug to exploit) and
> > flexible.
>
> Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.
>
The first argument of smc call is the function-id
arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, 0, &res);
>
> That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.
>
Actually that is the very reason func-id should be a client thing and
passed via client's DT node :)
It is general understanding that protocol specific bits should not be
a part of controller driver, but the client(protocol) driver.
Page-7 Function-ID specifies :-
1) The service to be invoked.
2) The function to be invoked.
3) The calling convention (32-bit or 64-bit) that is in use.
4) The call type (fast or yielding) that is in use.
Even if we turn blind to 2,3 & 4, but (1) shouts like a runtime property.
Thanks.
On Wed, 18 Sep 2019 10:31:57 -0500
Jassi Brar <[email protected]> wrote:
Hi,
> On Wed, Sep 18, 2019 at 9:46 AM Andre Przywara <[email protected]> wrote:
> >
> > On Wed, 18 Sep 2019 09:19:46 -0500
> > Jassi Brar <[email protected]> wrote:
> >
> > Hi,
> >
> > > On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <[email protected]> wrote:
> > >
> > > >
> > > > > which needs 9 arguments to work. The fact that the fist argument is
> > > > > always going to be same on a platform is just the way we use this
> > > > > instruction.
> > > > >
> > > > > > We should be as strict as possible to avoid any security issues.
> > > > > >
> > > > > Any example of such a security issue?
> > > >
> > > > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> > > >
> > > What if someone finds a way to trick the block layer to erase 'sda' ?
> >
> > Yes, the Linux block driver control the whole block device, it can do whatever it wants.
> >
> Sorry, it doesn't make any sense.
What I meant is that in contrast to a block driver the SMC interface is a shared resource. The mailbox controller is just using a tiny part of that. We must make sure to not interfere with the other services offered by firmware.
> > > That is called "bug in the code".
> > > It does happen in every subsystem but we don't stop implementing new
> > > features .... we write flexible code and then fix any bug.
> > >
> > >
> > > > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> > > >
> > > FSL_SIP_SCMI_1/2 ?
> >
> > Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.
> >
> SCPI/SCMI implementation is broken. I did NAK it.
> With the 'smc' mailbox you may get away without have to program the
> channel before transmit, but not every controller is natively so.
>
> > > But that is not the main point, which is to be consistent (not
> > > ignoring first argument because someone may find a bug to exploit) and
> > > flexible.
> >
> > Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.
> >
> The first argument of smc call is the function-id
> arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, 0, &res);
As I wrote we can't safely use a bare smc call. We must comply with the SMCCC, because there are a possible multitude of services the firmware offers. PSCI is a one obvious example.
And SMCCC says:
When an SMC32/HVC32 call is made from AArch32:
• A Function Identifier is passed in register R0.
• Arguments are passed in registers R1-R6.
• Results are returned in R0-R3.
...
(similar for SMC64)
So arguments start from r1/x1.
> >
> > That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.
> >
> Actually that is the very reason func-id should be a client thing and
> passed via client's DT node :)
> It is general understanding that protocol specific bits should not be
> a part of controller driver, but the client(protocol) driver.
There are *two* protocols to consider here:
One is the outer SMCCC protocol, which establishes communication between the mailbox controller driver and the firmware. SMCCC defines very precisely the meaning of r0/w0 for that, but leaves the rest (x1-x6) to the user. Think of the function ID being the equivalent of an MMIO base address. You can use it to select several instances of one type of mailbox. It's the task of the controller driver to abstract from that. Surely you wouldn't provide the MMIO base address of the particular mailbox in the client's DT node.
The *client* protocol is then wrapped inside this, using the six argument registers that SMCCC gives us. This is indeed up to the client to use and it dictates its meaning.
Maybe there is some misunderstanding that this mailbox is really a SMCCC mailbox rather than a pure SMC mailbox? Should we use a different naming like smccc_mailbox or firmware_mbox instead?
> Page-7 Function-ID specifies :-
> 1) The service to be invoked.
> 2) The function to be invoked.
"Service" is the term used for a "group of functions". PSCI is one service, for instance. It's typically determined by some upper bits. Inside this service there are several functions, like CPU_ON or SYSTEM_RESET, typically using low order bits.
In our case the service is the mailbox, and there is just one function, the actual doorbell. We could have introduced more functions (like disocvery, information or statistics), but there is no real need for that.
Cheers,
Andre.
> 3) The calling convention (32-bit or 64-bit) that is in use.
> 4) The call type (fast or yielding) that is in use.
>
> Even if we turn blind to 2,3 & 4, but (1) shouts like a runtime property.
>
> Thanks.
On Wed, Sep 18, 2019 at 3:53 AM Peng Fan <[email protected]> wrote:
> > >
> > > > +
> > > > + "#mbox-cells":
> > > > + const: 1
> > >
> > > Why is this "1"? What is this number used for? It used to be the channel ID,
> > but since you are describing a single channel controller only, this should be 0
> > now.
> > >
> > Yes. I overlooked it and actually queued the patch for pull request.
>
> In Documentation/devicetree/bindings/mailbox/mailbox.txt
> #mbox-cells: Must be at least 1.
>
> So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1 here.
> So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
> and add xlate for smc mailbox?
>
No, you just can not use the generic xlate() provided by the api.
Please implement your own xlate() that requires no argument.
Cheers!