[PATCH v6 0/6] Asynchronous notifications from secure world

[PATCH v6 0/6] Asynchronous notifications from secure world

[Jens Wiklander]

Hi all,

This adds support for asynchronous notifications from OP-TEE in secure
world to the OP-TEE driver. This allows a design with a top half and bottom
half type of driver where the top half runs in secure interrupt context and
a notifications tells normal world to schedule a yielding call to do the
bottom half processing.

An edge-triggered interrupt is used to notify the driver that there are
asynchronous notifications pending.

The documentation and DT bindings patches are now well reviewed, but
the patches with code would do with some more attention.

v5->v6:
* Rebased on v5.15-rc2
* Replaced "tee: add tee_dev_open_helper() primitive" with "tee: export
  teedev_open() and teedev_close_context()" since it turned out that the
  normal teedev functions could be used instead as noted by Sumit.
* Changed "optee: add asynchronous notifications" to use the exported
  teedev_open() and teedev_close_context() functions instead.

v4->v5:
* Rebased on v5.14-rc7
* Updated documentation to clarify that one interrupt may represent multiple
  notifications as requested.
* Applied Marc's and Rob's tags

v3->v4:
* Clarfied the expected type of interrypt is edge-triggered, both in
  the normal documentation and in the DT bindings as requested.

v2->v3:
* Rebased on v5.14-rc2 which made the patch "dt-bindings: arm: Convert
  optee binding to json-schema" from the V2 patch set obsolete.
* Applied Ard's Acked-by on "optee: add asynchronous notifications"

v1->v2:
* Added documentation
* Converted optee bindings to json-schema and added interrupt property
* Configure notification interrupt from DT instead of getting it
  from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.

Thanks,
Jens

Jens Wiklander (6):
  docs: staging/tee.rst: add a section on OP-TEE notifications
  dt-bindings: arm: optee: add interrupt property
  tee: fix put order in teedev_close_context()
  tee: export teedev_open() and teedev_close_context()
  optee: separate notification functions
  optee: add asynchronous notifications

 .../arm/firmware/linaro,optee-tz.yaml         |   7 +
 Documentation/staging/tee.rst                 |  30 +++
 drivers/tee/optee/Makefile                    |   1 +
 drivers/tee/optee/call.c                      |  27 +++
 drivers/tee/optee/core.c                      |  87 +++++--
 drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
 drivers/tee/optee/optee_msg.h                 |   9 +
 drivers/tee/optee/optee_private.h             |  23 +-
 drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
 drivers/tee/optee/optee_smc.h                 |  75 +++++-
 drivers/tee/optee/rpc.c                       |  73 +-----
 drivers/tee/tee_core.c                        |  10 +-
 include/linux/tee_drv.h                       |  14 ++
 13 files changed, 496 insertions(+), 117 deletions(-)
 create mode 100644 drivers/tee/optee/notif.c

-- 
2.31.1

[PATCH v6 0/6] Asynchronous notifications from secure world

[Jens Wiklander]

Hi all,

This adds support for asynchronous notifications from OP-TEE in secure
world to the OP-TEE driver. This allows a design with a top half and bottom
half type of driver where the top half runs in secure interrupt context and
a notifications tells normal world to schedule a yielding call to do the
bottom half processing.

An edge-triggered interrupt is used to notify the driver that there are
asynchronous notifications pending.

The documentation and DT bindings patches are now well reviewed, but
the patches with code would do with some more attention.

v5->v6:
* Rebased on v5.15-rc2
* Replaced "tee: add tee_dev_open_helper() primitive" with "tee: export
  teedev_open() and teedev_close_context()" since it turned out that the
  normal teedev functions could be used instead as noted by Sumit.
* Changed "optee: add asynchronous notifications" to use the exported
  teedev_open() and teedev_close_context() functions instead.

v4->v5:
* Rebased on v5.14-rc7
* Updated documentation to clarify that one interrupt may represent multiple
  notifications as requested.
* Applied Marc's and Rob's tags

v3->v4:
* Clarfied the expected type of interrypt is edge-triggered, both in
  the normal documentation and in the DT bindings as requested.

v2->v3:
* Rebased on v5.14-rc2 which made the patch "dt-bindings: arm: Convert
  optee binding to json-schema" from the V2 patch set obsolete.
* Applied Ard's Acked-by on "optee: add asynchronous notifications"

v1->v2:
* Added documentation
* Converted optee bindings to json-schema and added interrupt property
* Configure notification interrupt from DT instead of getting it
  from secure world, suggested by Ard Biesheuvel <ardb@kernel.org>.

Thanks,
Jens

Jens Wiklander (6):
  docs: staging/tee.rst: add a section on OP-TEE notifications
  dt-bindings: arm: optee: add interrupt property
  tee: fix put order in teedev_close_context()
  tee: export teedev_open() and teedev_close_context()
  optee: separate notification functions
  optee: add asynchronous notifications

 .../arm/firmware/linaro,optee-tz.yaml         |   7 +
 Documentation/staging/tee.rst                 |  30 +++
 drivers/tee/optee/Makefile                    |   1 +
 drivers/tee/optee/call.c                      |  27 +++
 drivers/tee/optee/core.c                      |  87 +++++--
 drivers/tee/optee/notif.c                     | 226 ++++++++++++++++++
 drivers/tee/optee/optee_msg.h                 |   9 +
 drivers/tee/optee/optee_private.h             |  23 +-
 drivers/tee/optee/optee_rpc_cmd.h             |  31 +--
 drivers/tee/optee/optee_smc.h                 |  75 +++++-
 drivers/tee/optee/rpc.c                       |  73 +-----
 drivers/tee/tee_core.c                        |  10 +-
 include/linux/tee_drv.h                       |  14 ++
 13 files changed, 496 insertions(+), 117 deletions(-)
 create mode 100644 drivers/tee/optee/notif.c

-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 1/6] docs: staging/tee.rst: add a section on OP-TEE notifications

[Jens Wiklander]

Adds a section on notifications used by OP-TEE, synchronous and
asynchronous.

Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 Documentation/staging/tee.rst | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/staging/tee.rst b/Documentation/staging/tee.rst
index 4d4b5f889603..3c63d8dcd61e 100644
--- a/Documentation/staging/tee.rst
+++ b/Documentation/staging/tee.rst
@@ -184,6 +184,36 @@ order to support device enumeration. In other words, OP-TEE driver invokes this
 application to retrieve a list of Trusted Applications which can be registered
 as devices on the TEE bus.
 
+OP-TEE notifications
+--------------------
+
+There are two kinds of notifications that secure world can use to make
+normal world aware of some event.
+
+1. Synchronous notifications delivered with ``OPTEE_RPC_CMD_NOTIFICATION``
+   using the ``OPTEE_RPC_NOTIFICATION_SEND`` parameter.
+2. Asynchronous notifications delivered with a combination of a non-secure
+   edge-triggered interrupt and a fast call from the non-secure interrupt
+   handler.
+
+Synchronous notifications are limited by depending on RPC for delivery,
+this is only usable when secure world is entered with a yielding call via
+``OPTEE_SMC_CALL_WITH_ARG``. This excludes such notifications from secure
+world interrupt handlers.
+
+An asynchronous notification is delivered via a non-secure edge-triggered
+interrupt to an interrupt handler registered in the OP-TEE driver. The
+actual notification value are retrieved with the fast call
+``OPTEE_SMC_GET_ASYNC_NOTIF_VALUE``. Note that one interrupt can represent
+multiple notifications.
+
+One notification value ``OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF`` has a
+special meaning. When this value is received it means that normal world is
+supposed to make a yielding call ``OPTEE_MSG_CMD_DO_BOTTOM_HALF``. This
+call is done from the thread assisting the interrupt handler. This is a
+building block for OP-TEE OS in secure world to implement the top half and
+bottom half style of device drivers.
+
 AMD-TEE driver
 ==============
 
-- 
2.31.1

[PATCH v6 1/6] docs: staging/tee.rst: add a section on OP-TEE notifications

[Jens Wiklander]

Adds a section on notifications used by OP-TEE, synchronous and
asynchronous.

Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 Documentation/staging/tee.rst | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/staging/tee.rst b/Documentation/staging/tee.rst
index 4d4b5f889603..3c63d8dcd61e 100644
--- a/Documentation/staging/tee.rst
+++ b/Documentation/staging/tee.rst
@@ -184,6 +184,36 @@ order to support device enumeration. In other words, OP-TEE driver invokes this
 application to retrieve a list of Trusted Applications which can be registered
 as devices on the TEE bus.
 
+OP-TEE notifications
+--------------------
+
+There are two kinds of notifications that secure world can use to make
+normal world aware of some event.
+
+1. Synchronous notifications delivered with ``OPTEE_RPC_CMD_NOTIFICATION``
+   using the ``OPTEE_RPC_NOTIFICATION_SEND`` parameter.
+2. Asynchronous notifications delivered with a combination of a non-secure
+   edge-triggered interrupt and a fast call from the non-secure interrupt
+   handler.
+
+Synchronous notifications are limited by depending on RPC for delivery,
+this is only usable when secure world is entered with a yielding call via
+``OPTEE_SMC_CALL_WITH_ARG``. This excludes such notifications from secure
+world interrupt handlers.
+
+An asynchronous notification is delivered via a non-secure edge-triggered
+interrupt to an interrupt handler registered in the OP-TEE driver. The
+actual notification value are retrieved with the fast call
+``OPTEE_SMC_GET_ASYNC_NOTIF_VALUE``. Note that one interrupt can represent
+multiple notifications.
+
+One notification value ``OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF`` has a
+special meaning. When this value is received it means that normal world is
+supposed to make a yielding call ``OPTEE_MSG_CMD_DO_BOTTOM_HALF``. This
+call is done from the thread assisting the interrupt handler. This is a
+building block for OP-TEE OS in secure world to implement the top half and
+bottom half style of device drivers.
+
 AMD-TEE driver
 ==============
 
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 1/6] docs: staging/tee.rst: add a section on OP-TEE notifications

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds a section on notifications used by OP-TEE, synchronous and
> asynchronous.
>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  Documentation/staging/tee.rst | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/Documentation/staging/tee.rst b/Documentation/staging/tee.rst
> index 4d4b5f889603..3c63d8dcd61e 100644
> --- a/Documentation/staging/tee.rst
> +++ b/Documentation/staging/tee.rst
> @@ -184,6 +184,36 @@ order to support device enumeration. In other words, OP-TEE driver invokes this
>  application to retrieve a list of Trusted Applications which can be registered
>  as devices on the TEE bus.
>
> +OP-TEE notifications
> +--------------------
> +
> +There are two kinds of notifications that secure world can use to make
> +normal world aware of some event.
> +
> +1. Synchronous notifications delivered with ``OPTEE_RPC_CMD_NOTIFICATION``
> +   using the ``OPTEE_RPC_NOTIFICATION_SEND`` parameter.
> +2. Asynchronous notifications delivered with a combination of a non-secure
> +   edge-triggered interrupt and a fast call from the non-secure interrupt
> +   handler.
> +
> +Synchronous notifications are limited by depending on RPC for delivery,
> +this is only usable when secure world is entered with a yielding call via
> +``OPTEE_SMC_CALL_WITH_ARG``. This excludes such notifications from secure
> +world interrupt handlers.
> +
> +An asynchronous notification is delivered via a non-secure edge-triggered
> +interrupt to an interrupt handler registered in the OP-TEE driver. The
> +actual notification value are retrieved with the fast call
> +``OPTEE_SMC_GET_ASYNC_NOTIF_VALUE``. Note that one interrupt can represent
> +multiple notifications.
> +
> +One notification value ``OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF`` has a
> +special meaning. When this value is received it means that normal world is
> +supposed to make a yielding call ``OPTEE_MSG_CMD_DO_BOTTOM_HALF``. This
> +call is done from the thread assisting the interrupt handler. This is a
> +building block for OP-TEE OS in secure world to implement the top half and
> +bottom half style of device drivers.
> +
>  AMD-TEE driver
>  ==============
>
> --
> 2.31.1
>

Re: [PATCH v6 1/6] docs: staging/tee.rst: add a section on OP-TEE notifications

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds a section on notifications used by OP-TEE, synchronous and
> asynchronous.
>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  Documentation/staging/tee.rst | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/Documentation/staging/tee.rst b/Documentation/staging/tee.rst
> index 4d4b5f889603..3c63d8dcd61e 100644
> --- a/Documentation/staging/tee.rst
> +++ b/Documentation/staging/tee.rst
> @@ -184,6 +184,36 @@ order to support device enumeration. In other words, OP-TEE driver invokes this
>  application to retrieve a list of Trusted Applications which can be registered
>  as devices on the TEE bus.
>
> +OP-TEE notifications
> +--------------------
> +
> +There are two kinds of notifications that secure world can use to make
> +normal world aware of some event.
> +
> +1. Synchronous notifications delivered with ``OPTEE_RPC_CMD_NOTIFICATION``
> +   using the ``OPTEE_RPC_NOTIFICATION_SEND`` parameter.
> +2. Asynchronous notifications delivered with a combination of a non-secure
> +   edge-triggered interrupt and a fast call from the non-secure interrupt
> +   handler.
> +
> +Synchronous notifications are limited by depending on RPC for delivery,
> +this is only usable when secure world is entered with a yielding call via
> +``OPTEE_SMC_CALL_WITH_ARG``. This excludes such notifications from secure
> +world interrupt handlers.
> +
> +An asynchronous notification is delivered via a non-secure edge-triggered
> +interrupt to an interrupt handler registered in the OP-TEE driver. The
> +actual notification value are retrieved with the fast call
> +``OPTEE_SMC_GET_ASYNC_NOTIF_VALUE``. Note that one interrupt can represent
> +multiple notifications.
> +
> +One notification value ``OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF`` has a
> +special meaning. When this value is received it means that normal world is
> +supposed to make a yielding call ``OPTEE_MSG_CMD_DO_BOTTOM_HALF``. This
> +call is done from the thread assisting the interrupt handler. This is a
> +building block for OP-TEE OS in secure world to implement the top half and
> +bottom half style of device drivers.
> +
>  AMD-TEE driver
>  ==============
>
> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 2/6] dt-bindings: arm: optee: add interrupt property

[Jens Wiklander]

Adds an optional interrupt property to the optee binding.

Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml  | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
index c24047c1fdd5..40c9f1901e3f 100644
--- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
+++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
@@ -24,6 +24,12 @@ properties:
   compatible:
     const: linaro,optee-tz
 
+  interrupts:
+    maxItems: 1
+    description: |
+      This interrupt which is used to signal an event by the secure world
+      software is expected to be edge-triggered.
+
   method:
     enum: [smc, hvc]
     description: |
@@ -46,6 +52,7 @@ examples:
         optee  {
             compatible = "linaro,optee-tz";
             method = "smc";
+            interrupts = <0 187 1>;
         };
     };
 
-- 
2.31.1

[PATCH v6 2/6] dt-bindings: arm: optee: add interrupt property

[Jens Wiklander]

Adds an optional interrupt property to the optee binding.

Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml  | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
index c24047c1fdd5..40c9f1901e3f 100644
--- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
+++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
@@ -24,6 +24,12 @@ properties:
   compatible:
     const: linaro,optee-tz
 
+  interrupts:
+    maxItems: 1
+    description: |
+      This interrupt which is used to signal an event by the secure world
+      software is expected to be edge-triggered.
+
   method:
     enum: [smc, hvc]
     description: |
@@ -46,6 +52,7 @@ examples:
         optee  {
             compatible = "linaro,optee-tz";
             method = "smc";
+            interrupts = <0 187 1>;
         };
     };
 
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/6] dt-bindings: arm: optee: add interrupt property

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds an optional interrupt property to the optee binding.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml  | 7 +++++++
>  1 file changed, 7 insertions(+)
>

Apart from minor nit below:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index c24047c1fdd5..40c9f1901e3f 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -24,6 +24,12 @@ properties:
>    compatible:
>      const: linaro,optee-tz
>
> +  interrupts:
> +    maxItems: 1
> +    description: |
> +      This interrupt which is used to signal an event by the secure world
> +      software is expected to be edge-triggered.
> +
>    method:
>      enum: [smc, hvc]
>      description: |
> @@ -46,6 +52,7 @@ examples:
>          optee  {
>              compatible = "linaro,optee-tz";
>              method = "smc";
> +            interrupts = <0 187 1>;

nit: It would be better to use macros here:

 +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;

-Sumit

>          };
>      };
>
> --
> 2.31.1
>

Re: [PATCH v6 2/6] dt-bindings: arm: optee: add interrupt property

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds an optional interrupt property to the optee binding.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml  | 7 +++++++
>  1 file changed, 7 insertions(+)
>

Apart from minor nit below:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index c24047c1fdd5..40c9f1901e3f 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -24,6 +24,12 @@ properties:
>    compatible:
>      const: linaro,optee-tz
>
> +  interrupts:
> +    maxItems: 1
> +    description: |
> +      This interrupt which is used to signal an event by the secure world
> +      software is expected to be edge-triggered.
> +
>    method:
>      enum: [smc, hvc]
>      description: |
> @@ -46,6 +52,7 @@ examples:
>          optee  {
>              compatible = "linaro,optee-tz";
>              method = "smc";
> +            interrupts = <0 187 1>;

nit: It would be better to use macros here:

 +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;

-Sumit

>          };
>      };
>
> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 3/6] tee: fix put order in teedev_close_context()

[Jens Wiklander]

Prior to this patch was teedev_close_context() calling tee_device_put()
before teedev_ctx_put() leading to teedev_ctx_release() accessing
ctx->teedev just after the reference counter was decreased on the
teedev. Fix this by calling teedev_ctx_put() before tee_device_put().

Fixes: 217e0250cccb ("tee: use reference counting for tee_context")
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 2b37bc408fc3..85102d12d716 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -98,8 +98,10 @@ void teedev_ctx_put(struct tee_context *ctx)
 
 static void teedev_close_context(struct tee_context *ctx)
 {
-	tee_device_put(ctx->teedev);
+	struct tee_device *teedev = ctx->teedev;
+
 	teedev_ctx_put(ctx);
+	tee_device_put(teedev);
 }
 
 static int tee_open(struct inode *inode, struct file *filp)
-- 
2.31.1

[PATCH v6 3/6] tee: fix put order in teedev_close_context()

[Jens Wiklander]

Prior to this patch was teedev_close_context() calling tee_device_put()
before teedev_ctx_put() leading to teedev_ctx_release() accessing
ctx->teedev just after the reference counter was decreased on the
teedev. Fix this by calling teedev_ctx_put() before tee_device_put().

Fixes: 217e0250cccb ("tee: use reference counting for tee_context")
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 2b37bc408fc3..85102d12d716 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -98,8 +98,10 @@ void teedev_ctx_put(struct tee_context *ctx)
 
 static void teedev_close_context(struct tee_context *ctx)
 {
-	tee_device_put(ctx->teedev);
+	struct tee_device *teedev = ctx->teedev;
+
 	teedev_ctx_put(ctx);
+	tee_device_put(teedev);
 }
 
 static int tee_open(struct inode *inode, struct file *filp)
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 3/6] tee: fix put order in teedev_close_context()

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Prior to this patch was teedev_close_context() calling tee_device_put()
> before teedev_ctx_put() leading to teedev_ctx_release() accessing
> ctx->teedev just after the reference counter was decreased on the
> teedev. Fix this by calling teedev_ctx_put() before tee_device_put().
>
> Fixes: 217e0250cccb ("tee: use reference counting for tee_context")
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 2b37bc408fc3..85102d12d716 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -98,8 +98,10 @@ void teedev_ctx_put(struct tee_context *ctx)
>
>  static void teedev_close_context(struct tee_context *ctx)
>  {
> -       tee_device_put(ctx->teedev);
> +       struct tee_device *teedev = ctx->teedev;
> +
>         teedev_ctx_put(ctx);
> +       tee_device_put(teedev);
>  }
>
>  static int tee_open(struct inode *inode, struct file *filp)
> --
> 2.31.1
>

Re: [PATCH v6 3/6] tee: fix put order in teedev_close_context()

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Prior to this patch was teedev_close_context() calling tee_device_put()
> before teedev_ctx_put() leading to teedev_ctx_release() accessing
> ctx->teedev just after the reference counter was decreased on the
> teedev. Fix this by calling teedev_ctx_put() before tee_device_put().
>
> Fixes: 217e0250cccb ("tee: use reference counting for tee_context")
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 2b37bc408fc3..85102d12d716 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -98,8 +98,10 @@ void teedev_ctx_put(struct tee_context *ctx)
>
>  static void teedev_close_context(struct tee_context *ctx)
>  {
> -       tee_device_put(ctx->teedev);
> +       struct tee_device *teedev = ctx->teedev;
> +
>         teedev_ctx_put(ctx);
> +       tee_device_put(teedev);
>  }
>
>  static int tee_open(struct inode *inode, struct file *filp)
> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 4/6] tee: export teedev_open() and teedev_close_context()

[Jens Wiklander]

Exports the two functions teedev_open() and teedev_close_context() in
order to make it easier to create a driver internal struct tee_context.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c  |  6 ++++--
 include/linux/tee_drv.h | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 85102d12d716..3fc426dad2df 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -43,7 +43,7 @@ static DEFINE_SPINLOCK(driver_lock);
 static struct class *tee_class;
 static dev_t tee_devt;
 
-static struct tee_context *teedev_open(struct tee_device *teedev)
+struct tee_context *teedev_open(struct tee_device *teedev)
 {
 	int rc;
 	struct tee_context *ctx;
@@ -70,6 +70,7 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
 	return ERR_PTR(rc);
 
 }
+EXPORT_SYMBOL_GPL(teedev_open);
 
 void teedev_ctx_get(struct tee_context *ctx)
 {
@@ -96,13 +97,14 @@ void teedev_ctx_put(struct tee_context *ctx)
 	kref_put(&ctx->refcount, teedev_ctx_release);
 }
 
-static void teedev_close_context(struct tee_context *ctx)
+void teedev_close_context(struct tee_context *ctx)
 {
 	struct tee_device *teedev = ctx->teedev;
 
 	teedev_ctx_put(ctx);
 	tee_device_put(teedev);
 }
+EXPORT_SYMBOL_GPL(teedev_close_context);
 
 static int tee_open(struct inode *inode, struct file *filp)
 {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 3ebfea0781f1..26f42c4cd7a1 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -582,4 +582,18 @@ struct tee_client_driver {
 #define to_tee_client_driver(d) \
 		container_of(d, struct tee_client_driver, driver)
 
+/**
+ * teedev_open() - Open a struct tee_device
+ * @teedev:	Device to open
+ *
+ * @return a pointer to struct tee_context on success or an ERR_PTR on failure.
+ */
+struct tee_context *teedev_open(struct tee_device *teedev);
+
+/**
+ * teedev_close_context() - closes a struct tee_context
+ * @ctx:	The struct tee_context to close
+ */
+void teedev_close_context(struct tee_context *ctx);
+
 #endif /*__TEE_DRV_H*/
-- 
2.31.1

[PATCH v6 4/6] tee: export teedev_open() and teedev_close_context()

[Jens Wiklander]

Exports the two functions teedev_open() and teedev_close_context() in
order to make it easier to create a driver internal struct tee_context.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c  |  6 ++++--
 include/linux/tee_drv.h | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 85102d12d716..3fc426dad2df 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -43,7 +43,7 @@ static DEFINE_SPINLOCK(driver_lock);
 static struct class *tee_class;
 static dev_t tee_devt;
 
-static struct tee_context *teedev_open(struct tee_device *teedev)
+struct tee_context *teedev_open(struct tee_device *teedev)
 {
 	int rc;
 	struct tee_context *ctx;
@@ -70,6 +70,7 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
 	return ERR_PTR(rc);
 
 }
+EXPORT_SYMBOL_GPL(teedev_open);
 
 void teedev_ctx_get(struct tee_context *ctx)
 {
@@ -96,13 +97,14 @@ void teedev_ctx_put(struct tee_context *ctx)
 	kref_put(&ctx->refcount, teedev_ctx_release);
 }
 
-static void teedev_close_context(struct tee_context *ctx)
+void teedev_close_context(struct tee_context *ctx)
 {
 	struct tee_device *teedev = ctx->teedev;
 
 	teedev_ctx_put(ctx);
 	tee_device_put(teedev);
 }
+EXPORT_SYMBOL_GPL(teedev_close_context);
 
 static int tee_open(struct inode *inode, struct file *filp)
 {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 3ebfea0781f1..26f42c4cd7a1 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -582,4 +582,18 @@ struct tee_client_driver {
 #define to_tee_client_driver(d) \
 		container_of(d, struct tee_client_driver, driver)
 
+/**
+ * teedev_open() - Open a struct tee_device
+ * @teedev:	Device to open
+ *
+ * @return a pointer to struct tee_context on success or an ERR_PTR on failure.
+ */
+struct tee_context *teedev_open(struct tee_device *teedev);
+
+/**
+ * teedev_close_context() - closes a struct tee_context
+ * @ctx:	The struct tee_context to close
+ */
+void teedev_close_context(struct tee_context *ctx);
+
 #endif /*__TEE_DRV_H*/
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 4/6] tee: export teedev_open() and teedev_close_context()

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Exports the two functions teedev_open() and teedev_close_context() in
> order to make it easier to create a driver internal struct tee_context.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c  |  6 ++++--
>  include/linux/tee_drv.h | 14 ++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 85102d12d716..3fc426dad2df 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -43,7 +43,7 @@ static DEFINE_SPINLOCK(driver_lock);
>  static struct class *tee_class;
>  static dev_t tee_devt;
>
> -static struct tee_context *teedev_open(struct tee_device *teedev)
> +struct tee_context *teedev_open(struct tee_device *teedev)
>  {
>         int rc;
>         struct tee_context *ctx;
> @@ -70,6 +70,7 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
>         return ERR_PTR(rc);
>
>  }
> +EXPORT_SYMBOL_GPL(teedev_open);
>
>  void teedev_ctx_get(struct tee_context *ctx)
>  {
> @@ -96,13 +97,14 @@ void teedev_ctx_put(struct tee_context *ctx)
>         kref_put(&ctx->refcount, teedev_ctx_release);
>  }
>
> -static void teedev_close_context(struct tee_context *ctx)
> +void teedev_close_context(struct tee_context *ctx)
>  {
>         struct tee_device *teedev = ctx->teedev;
>
>         teedev_ctx_put(ctx);
>         tee_device_put(teedev);
>  }
> +EXPORT_SYMBOL_GPL(teedev_close_context);
>
>  static int tee_open(struct inode *inode, struct file *filp)
>  {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 3ebfea0781f1..26f42c4cd7a1 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -582,4 +582,18 @@ struct tee_client_driver {
>  #define to_tee_client_driver(d) \
>                 container_of(d, struct tee_client_driver, driver)
>
> +/**
> + * teedev_open() - Open a struct tee_device
> + * @teedev:    Device to open
> + *
> + * @return a pointer to struct tee_context on success or an ERR_PTR on failure.
> + */
> +struct tee_context *teedev_open(struct tee_device *teedev);
> +
> +/**
> + * teedev_close_context() - closes a struct tee_context
> + * @ctx:       The struct tee_context to close
> + */
> +void teedev_close_context(struct tee_context *ctx);
> +
>  #endif /*__TEE_DRV_H*/
> --
> 2.31.1
>

Re: [PATCH v6 4/6] tee: export teedev_open() and teedev_close_context()

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Exports the two functions teedev_open() and teedev_close_context() in
> order to make it easier to create a driver internal struct tee_context.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c  |  6 ++++--
>  include/linux/tee_drv.h | 14 ++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 85102d12d716..3fc426dad2df 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -43,7 +43,7 @@ static DEFINE_SPINLOCK(driver_lock);
>  static struct class *tee_class;
>  static dev_t tee_devt;
>
> -static struct tee_context *teedev_open(struct tee_device *teedev)
> +struct tee_context *teedev_open(struct tee_device *teedev)
>  {
>         int rc;
>         struct tee_context *ctx;
> @@ -70,6 +70,7 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
>         return ERR_PTR(rc);
>
>  }
> +EXPORT_SYMBOL_GPL(teedev_open);
>
>  void teedev_ctx_get(struct tee_context *ctx)
>  {
> @@ -96,13 +97,14 @@ void teedev_ctx_put(struct tee_context *ctx)
>         kref_put(&ctx->refcount, teedev_ctx_release);
>  }
>
> -static void teedev_close_context(struct tee_context *ctx)
> +void teedev_close_context(struct tee_context *ctx)
>  {
>         struct tee_device *teedev = ctx->teedev;
>
>         teedev_ctx_put(ctx);
>         tee_device_put(teedev);
>  }
> +EXPORT_SYMBOL_GPL(teedev_close_context);
>
>  static int tee_open(struct inode *inode, struct file *filp)
>  {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 3ebfea0781f1..26f42c4cd7a1 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -582,4 +582,18 @@ struct tee_client_driver {
>  #define to_tee_client_driver(d) \
>                 container_of(d, struct tee_client_driver, driver)
>
> +/**
> + * teedev_open() - Open a struct tee_device
> + * @teedev:    Device to open
> + *
> + * @return a pointer to struct tee_context on success or an ERR_PTR on failure.
> + */
> +struct tee_context *teedev_open(struct tee_device *teedev);
> +
> +/**
> + * teedev_close_context() - closes a struct tee_context
> + * @ctx:       The struct tee_context to close
> + */
> +void teedev_close_context(struct tee_context *ctx);
> +
>  #endif /*__TEE_DRV_H*/
> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 5/6] optee: separate notification functions

[Jens Wiklander]

Renames struct optee_wait_queue to struct optee_notif and all related
functions to optee_notif_*().

The implementation is changed to allow sending a notification from an
atomic state, that is from the top half of an interrupt handler.

Waiting for keys is currently only used when secure world is waiting for
a mutex or condition variable. The old implementation could handle any
32-bit key while this new implementation is restricted to only 8 bits or
the maximum value 255. A upper value is needed since a bitmap is
allocated to allow an interrupt handler to only set a bit in case the
waiter hasn't had the time yet to allocate and register a completion.

The keys are currently only representing secure world threads which
number usually are never even close to 255 so it should be safe for now.
In future ABI updates the maximum value of the key will be communicated
while the driver is initializing.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/Makefile        |   1 +
 drivers/tee/optee/core.c          |  12 ++-
 drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
 drivers/tee/optee/optee_private.h |  19 +++--
 drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
 drivers/tee/optee/rpc.c           |  73 ++---------------
 6 files changed, 170 insertions(+), 91 deletions(-)
 create mode 100644 drivers/tee/optee/notif.c

diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
index 3aa33ea9e6a6..df55e4ad5370 100644
--- a/drivers/tee/optee/Makefile
+++ b/drivers/tee/optee/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_OPTEE) += optee.o
 optee-objs += core.o
 optee-objs += call.o
+optee-objs += notif.o
 optee-objs += rpc.o
 optee-objs += supp.o
 optee-objs += shm_pool.o
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 5ce13b099d7d..8531184f98f4 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
 	 */
 	optee_disable_shm_cache(optee);
 
+	optee_notif_uninit(optee);
 	/*
 	 * The two devices have to be unregistered before we can free the
 	 * other resources.
@@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
 	tee_shm_pool_free(optee->pool);
 	if (optee->memremaped_shm)
 		memunmap(optee->memremaped_shm);
-	optee_wait_queue_exit(&optee->wait_queue);
 	optee_supp_uninit(&optee->supp);
 	mutex_destroy(&optee->call_queue.mutex);
 
@@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
 
 	mutex_init(&optee->call_queue.mutex);
 	INIT_LIST_HEAD(&optee->call_queue.waiters);
-	optee_wait_queue_init(&optee->wait_queue);
 	optee_supp_init(&optee->supp);
 	optee->memremaped_shm = memremaped_shm;
 	optee->pool = pool;
 
+	platform_set_drvdata(pdev, optee);
+	rc = optee_notif_init(optee, 255);
+	if (rc) {
+		optee_remove(pdev);
+		return rc;
+	}
+
 	/*
 	 * Ensure that there are no pre-existing shm objects before enabling
 	 * the shm cache so that there's no chance of receiving an invalid
@@ -731,8 +737,6 @@ static int optee_probe(struct platform_device *pdev)
 	if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
 		pr_info("dynamic shared memory is enabled\n");
 
-	platform_set_drvdata(pdev, optee);
-
 	rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
 	if (rc) {
 		optee_remove(pdev);
diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
new file mode 100644
index 000000000000..a28fa03dcd0e
--- /dev/null
+++ b/drivers/tee/optee/notif.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2015-2021, Linaro Limited
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/arm-smccc.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/tee_drv.h>
+#include "optee_private.h"
+
+struct notif_entry {
+	struct list_head link;
+	struct completion c;
+	u_int key;
+};
+
+static bool have_key(struct optee *optee, u_int key)
+{
+	struct notif_entry *entry;
+
+	list_for_each_entry(entry, &optee->notif.db, link)
+		if (entry->key == key)
+			return true;
+
+	return false;
+}
+
+int optee_notif_wait(struct optee *optee, u_int key)
+{
+	unsigned long flags;
+	struct notif_entry *entry;
+	int rc = 0;
+
+	if (key > optee->notif.max_key)
+		return -EINVAL;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+	init_completion(&entry->c);
+	entry->key = key;
+
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	/*
+	 * If the bit is already set it means that the key has already
+	 * been posted and we must not wait.
+	 */
+	if (test_bit(key, optee->notif.bitmap)) {
+		clear_bit(key, optee->notif.bitmap);
+		goto out;
+	}
+
+	/*
+	 * Check if someone is already waiting for this key. If there is
+	 * it's a programming error.
+	 */
+	if (have_key(optee, key)) {
+		rc = -EBUSY;
+		goto out;
+	}
+
+	list_add_tail(&entry->link, &optee->notif.db);
+
+	/*
+	 * Unlock temporarily and wait for completion.
+	 */
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+	wait_for_completion(&entry->c);
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	list_del(&entry->link);
+out:
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+
+	kfree(entry);
+
+	return rc;
+}
+
+int optee_notif_send(struct optee *optee, u_int key)
+{
+	unsigned long flags;
+	struct notif_entry *entry;
+
+	if (key > optee->notif.max_key)
+		return -EINVAL;
+
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	list_for_each_entry(entry, &optee->notif.db, link)
+		if (entry->key == key) {
+			complete(&entry->c);
+			goto out;
+		}
+
+	/* Only set the bit in case there where nobody waiting */
+	set_bit(key, optee->notif.bitmap);
+out:
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+
+	return 0;
+}
+
+int optee_notif_init(struct optee *optee, u_int max_key)
+{
+	spin_lock_init(&optee->notif.lock);
+	INIT_LIST_HEAD(&optee->notif.db);
+	optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
+	if (!optee->notif.bitmap)
+		return -ENOMEM;
+
+	optee->notif.max_key = max_key;
+
+	return 0;
+}
+
+void optee_notif_uninit(struct optee *optee)
+{
+	kfree(optee->notif.bitmap);
+}
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index dbdd367be156..76a16d9b6cf4 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -35,10 +35,12 @@ struct optee_call_queue {
 	struct list_head waiters;
 };
 
-struct optee_wait_queue {
-	/* Serializes access to this struct */
-	struct mutex mu;
+struct optee_notif {
+	u_int max_key;
+	/* Serializes access to the elements below in this struct */
+	spinlock_t lock;
 	struct list_head db;
+	u_long *bitmap;
 };
 
 /**
@@ -72,8 +74,7 @@ struct optee_supp {
  * @teedev:		client device
  * @invoke_fn:		function to issue smc or hvc
  * @call_queue:		queue of threads waiting to call @invoke_fn
- * @wait_queue:		queue of threads from secure world waiting for a
- *			secure world sync object
+ * @notif:		notification synchronization struct
  * @supp:		supplicant synchronization struct for RPC to supplicant
  * @pool:		shared memory pool
  * @memremaped_shm	virtual address of memory in shared memory pool
@@ -88,7 +89,7 @@ struct optee {
 	struct tee_device *teedev;
 	optee_invoke_fn *invoke_fn;
 	struct optee_call_queue call_queue;
-	struct optee_wait_queue wait_queue;
+	struct optee_notif notif;
 	struct optee_supp supp;
 	struct tee_shm_pool *pool;
 	void *memremaped_shm;
@@ -131,8 +132,10 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
 		      struct optee_call_ctx *call_ctx);
 void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
 
-void optee_wait_queue_init(struct optee_wait_queue *wq);
-void optee_wait_queue_exit(struct optee_wait_queue *wq);
+int optee_notif_init(struct optee *optee, u_int max_key);
+void optee_notif_uninit(struct optee *optee);
+int optee_notif_wait(struct optee *optee, u_int key);
+int optee_notif_send(struct optee *optee, u_int key);
 
 u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
 			struct tee_param *param);
diff --git a/drivers/tee/optee/optee_rpc_cmd.h b/drivers/tee/optee/optee_rpc_cmd.h
index b8275140cef8..f3f06e0994a7 100644
--- a/drivers/tee/optee/optee_rpc_cmd.h
+++ b/drivers/tee/optee/optee_rpc_cmd.h
@@ -28,24 +28,27 @@
 #define OPTEE_RPC_CMD_GET_TIME		3
 
 /*
- * Wait queue primitive, helper for secure world to implement a wait queue.
+ * Notification from/to secure world.
  *
- * If secure world needs to wait for a secure world mutex it issues a sleep
- * request instead of spinning in secure world. Conversely is a wakeup
- * request issued when a secure world mutex with a thread waiting thread is
- * unlocked.
+ * If secure world needs to wait for something, for instance a mutex, it
+ * does a notification wait request instead of spinning in secure world.
+ * Conversely can a synchronous notification can be sent when a secure
+ * world mutex with a thread waiting thread is unlocked.
  *
- * Waiting on a key
- * [in]    value[0].a	    OPTEE_RPC_WAIT_QUEUE_SLEEP
- * [in]    value[0].b	    Wait key
+ * This interface can also be used to wait for a asynchronous notification
+ * which instead is sent via a non-secure interrupt.
  *
- * Waking up a key
- * [in]    value[0].a	    OPTEE_RPC_WAIT_QUEUE_WAKEUP
- * [in]    value[0].b	    Wakeup key
+ * Waiting on notification
+ * [in]    value[0].a	    OPTEE_RPC_NOTIFICATION_WAIT
+ * [in]    value[0].b	    notification value
+ *
+ * Sending a synchronous notification
+ * [in]    value[0].a	    OPTEE_RPC_NOTIFICATION_SEND
+ * [in]    value[0].b	    notification value
  */
-#define OPTEE_RPC_CMD_WAIT_QUEUE	4
-#define OPTEE_RPC_WAIT_QUEUE_SLEEP	0
-#define OPTEE_RPC_WAIT_QUEUE_WAKEUP	1
+#define OPTEE_RPC_CMD_NOTIFICATION	4
+#define OPTEE_RPC_NOTIFICATION_WAIT	0
+#define OPTEE_RPC_NOTIFICATION_SEND	1
 
 /*
  * Suspend execution
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index efbaff7ad7e5..fa492655843a 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2015-2016, Linaro Limited
+ * Copyright (c) 2015-2021, Linaro Limited
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -14,23 +14,6 @@
 #include "optee_smc.h"
 #include "optee_rpc_cmd.h"
 
-struct wq_entry {
-	struct list_head link;
-	struct completion c;
-	u32 key;
-};
-
-void optee_wait_queue_init(struct optee_wait_queue *priv)
-{
-	mutex_init(&priv->mu);
-	INIT_LIST_HEAD(&priv->db);
-}
-
-void optee_wait_queue_exit(struct optee_wait_queue *priv)
-{
-	mutex_destroy(&priv->mu);
-}
-
 static void handle_rpc_func_cmd_get_time(struct optee_msg_arg *arg)
 {
 	struct timespec64 ts;
@@ -143,48 +126,6 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
 }
 #endif
 
-static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w;
-
-	mutex_lock(&wq->mu);
-
-	list_for_each_entry(w, &wq->db, link)
-		if (w->key == key)
-			goto out;
-
-	w = kmalloc(sizeof(*w), GFP_KERNEL);
-	if (w) {
-		init_completion(&w->c);
-		w->key = key;
-		list_add_tail(&w->link, &wq->db);
-	}
-out:
-	mutex_unlock(&wq->mu);
-	return w;
-}
-
-static void wq_sleep(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w = wq_entry_get(wq, key);
-
-	if (w) {
-		wait_for_completion(&w->c);
-		mutex_lock(&wq->mu);
-		list_del(&w->link);
-		mutex_unlock(&wq->mu);
-		kfree(w);
-	}
-}
-
-static void wq_wakeup(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w = wq_entry_get(wq, key);
-
-	if (w)
-		complete(&w->c);
-}
-
 static void handle_rpc_func_cmd_wq(struct optee *optee,
 				   struct optee_msg_arg *arg)
 {
@@ -196,11 +137,13 @@ static void handle_rpc_func_cmd_wq(struct optee *optee,
 		goto bad;
 
 	switch (arg->params[0].u.value.a) {
-	case OPTEE_RPC_WAIT_QUEUE_SLEEP:
-		wq_sleep(&optee->wait_queue, arg->params[0].u.value.b);
+	case OPTEE_RPC_NOTIFICATION_WAIT:
+		if (optee_notif_wait(optee, arg->params[0].u.value.b))
+			goto bad;
 		break;
-	case OPTEE_RPC_WAIT_QUEUE_WAKEUP:
-		wq_wakeup(&optee->wait_queue, arg->params[0].u.value.b);
+	case OPTEE_RPC_NOTIFICATION_SEND:
+		if (optee_notif_send(optee, arg->params[0].u.value.b))
+			goto bad;
 		break;
 	default:
 		goto bad;
@@ -463,7 +406,7 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
 	case OPTEE_RPC_CMD_GET_TIME:
 		handle_rpc_func_cmd_get_time(arg);
 		break;
-	case OPTEE_RPC_CMD_WAIT_QUEUE:
+	case OPTEE_RPC_CMD_NOTIFICATION:
 		handle_rpc_func_cmd_wq(optee, arg);
 		break;
 	case OPTEE_RPC_CMD_SUSPEND:
-- 
2.31.1

[PATCH v6 5/6] optee: separate notification functions

[Jens Wiklander]

Renames struct optee_wait_queue to struct optee_notif and all related
functions to optee_notif_*().

The implementation is changed to allow sending a notification from an
atomic state, that is from the top half of an interrupt handler.

Waiting for keys is currently only used when secure world is waiting for
a mutex or condition variable. The old implementation could handle any
32-bit key while this new implementation is restricted to only 8 bits or
the maximum value 255. A upper value is needed since a bitmap is
allocated to allow an interrupt handler to only set a bit in case the
waiter hasn't had the time yet to allocate and register a completion.

The keys are currently only representing secure world threads which
number usually are never even close to 255 so it should be safe for now.
In future ABI updates the maximum value of the key will be communicated
while the driver is initializing.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/Makefile        |   1 +
 drivers/tee/optee/core.c          |  12 ++-
 drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
 drivers/tee/optee/optee_private.h |  19 +++--
 drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
 drivers/tee/optee/rpc.c           |  73 ++---------------
 6 files changed, 170 insertions(+), 91 deletions(-)
 create mode 100644 drivers/tee/optee/notif.c

diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
index 3aa33ea9e6a6..df55e4ad5370 100644
--- a/drivers/tee/optee/Makefile
+++ b/drivers/tee/optee/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_OPTEE) += optee.o
 optee-objs += core.o
 optee-objs += call.o
+optee-objs += notif.o
 optee-objs += rpc.o
 optee-objs += supp.o
 optee-objs += shm_pool.o
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 5ce13b099d7d..8531184f98f4 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
 	 */
 	optee_disable_shm_cache(optee);
 
+	optee_notif_uninit(optee);
 	/*
 	 * The two devices have to be unregistered before we can free the
 	 * other resources.
@@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
 	tee_shm_pool_free(optee->pool);
 	if (optee->memremaped_shm)
 		memunmap(optee->memremaped_shm);
-	optee_wait_queue_exit(&optee->wait_queue);
 	optee_supp_uninit(&optee->supp);
 	mutex_destroy(&optee->call_queue.mutex);
 
@@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
 
 	mutex_init(&optee->call_queue.mutex);
 	INIT_LIST_HEAD(&optee->call_queue.waiters);
-	optee_wait_queue_init(&optee->wait_queue);
 	optee_supp_init(&optee->supp);
 	optee->memremaped_shm = memremaped_shm;
 	optee->pool = pool;
 
+	platform_set_drvdata(pdev, optee);
+	rc = optee_notif_init(optee, 255);
+	if (rc) {
+		optee_remove(pdev);
+		return rc;
+	}
+
 	/*
 	 * Ensure that there are no pre-existing shm objects before enabling
 	 * the shm cache so that there's no chance of receiving an invalid
@@ -731,8 +737,6 @@ static int optee_probe(struct platform_device *pdev)
 	if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
 		pr_info("dynamic shared memory is enabled\n");
 
-	platform_set_drvdata(pdev, optee);
-
 	rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
 	if (rc) {
 		optee_remove(pdev);
diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
new file mode 100644
index 000000000000..a28fa03dcd0e
--- /dev/null
+++ b/drivers/tee/optee/notif.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2015-2021, Linaro Limited
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/arm-smccc.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/tee_drv.h>
+#include "optee_private.h"
+
+struct notif_entry {
+	struct list_head link;
+	struct completion c;
+	u_int key;
+};
+
+static bool have_key(struct optee *optee, u_int key)
+{
+	struct notif_entry *entry;
+
+	list_for_each_entry(entry, &optee->notif.db, link)
+		if (entry->key == key)
+			return true;
+
+	return false;
+}
+
+int optee_notif_wait(struct optee *optee, u_int key)
+{
+	unsigned long flags;
+	struct notif_entry *entry;
+	int rc = 0;
+
+	if (key > optee->notif.max_key)
+		return -EINVAL;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+	init_completion(&entry->c);
+	entry->key = key;
+
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	/*
+	 * If the bit is already set it means that the key has already
+	 * been posted and we must not wait.
+	 */
+	if (test_bit(key, optee->notif.bitmap)) {
+		clear_bit(key, optee->notif.bitmap);
+		goto out;
+	}
+
+	/*
+	 * Check if someone is already waiting for this key. If there is
+	 * it's a programming error.
+	 */
+	if (have_key(optee, key)) {
+		rc = -EBUSY;
+		goto out;
+	}
+
+	list_add_tail(&entry->link, &optee->notif.db);
+
+	/*
+	 * Unlock temporarily and wait for completion.
+	 */
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+	wait_for_completion(&entry->c);
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	list_del(&entry->link);
+out:
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+
+	kfree(entry);
+
+	return rc;
+}
+
+int optee_notif_send(struct optee *optee, u_int key)
+{
+	unsigned long flags;
+	struct notif_entry *entry;
+
+	if (key > optee->notif.max_key)
+		return -EINVAL;
+
+	spin_lock_irqsave(&optee->notif.lock, flags);
+
+	list_for_each_entry(entry, &optee->notif.db, link)
+		if (entry->key == key) {
+			complete(&entry->c);
+			goto out;
+		}
+
+	/* Only set the bit in case there where nobody waiting */
+	set_bit(key, optee->notif.bitmap);
+out:
+	spin_unlock_irqrestore(&optee->notif.lock, flags);
+
+	return 0;
+}
+
+int optee_notif_init(struct optee *optee, u_int max_key)
+{
+	spin_lock_init(&optee->notif.lock);
+	INIT_LIST_HEAD(&optee->notif.db);
+	optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
+	if (!optee->notif.bitmap)
+		return -ENOMEM;
+
+	optee->notif.max_key = max_key;
+
+	return 0;
+}
+
+void optee_notif_uninit(struct optee *optee)
+{
+	kfree(optee->notif.bitmap);
+}
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index dbdd367be156..76a16d9b6cf4 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -35,10 +35,12 @@ struct optee_call_queue {
 	struct list_head waiters;
 };
 
-struct optee_wait_queue {
-	/* Serializes access to this struct */
-	struct mutex mu;
+struct optee_notif {
+	u_int max_key;
+	/* Serializes access to the elements below in this struct */
+	spinlock_t lock;
 	struct list_head db;
+	u_long *bitmap;
 };
 
 /**
@@ -72,8 +74,7 @@ struct optee_supp {
  * @teedev:		client device
  * @invoke_fn:		function to issue smc or hvc
  * @call_queue:		queue of threads waiting to call @invoke_fn
- * @wait_queue:		queue of threads from secure world waiting for a
- *			secure world sync object
+ * @notif:		notification synchronization struct
  * @supp:		supplicant synchronization struct for RPC to supplicant
  * @pool:		shared memory pool
  * @memremaped_shm	virtual address of memory in shared memory pool
@@ -88,7 +89,7 @@ struct optee {
 	struct tee_device *teedev;
 	optee_invoke_fn *invoke_fn;
 	struct optee_call_queue call_queue;
-	struct optee_wait_queue wait_queue;
+	struct optee_notif notif;
 	struct optee_supp supp;
 	struct tee_shm_pool *pool;
 	void *memremaped_shm;
@@ -131,8 +132,10 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
 		      struct optee_call_ctx *call_ctx);
 void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
 
-void optee_wait_queue_init(struct optee_wait_queue *wq);
-void optee_wait_queue_exit(struct optee_wait_queue *wq);
+int optee_notif_init(struct optee *optee, u_int max_key);
+void optee_notif_uninit(struct optee *optee);
+int optee_notif_wait(struct optee *optee, u_int key);
+int optee_notif_send(struct optee *optee, u_int key);
 
 u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
 			struct tee_param *param);
diff --git a/drivers/tee/optee/optee_rpc_cmd.h b/drivers/tee/optee/optee_rpc_cmd.h
index b8275140cef8..f3f06e0994a7 100644
--- a/drivers/tee/optee/optee_rpc_cmd.h
+++ b/drivers/tee/optee/optee_rpc_cmd.h
@@ -28,24 +28,27 @@
 #define OPTEE_RPC_CMD_GET_TIME		3
 
 /*
- * Wait queue primitive, helper for secure world to implement a wait queue.
+ * Notification from/to secure world.
  *
- * If secure world needs to wait for a secure world mutex it issues a sleep
- * request instead of spinning in secure world. Conversely is a wakeup
- * request issued when a secure world mutex with a thread waiting thread is
- * unlocked.
+ * If secure world needs to wait for something, for instance a mutex, it
+ * does a notification wait request instead of spinning in secure world.
+ * Conversely can a synchronous notification can be sent when a secure
+ * world mutex with a thread waiting thread is unlocked.
  *
- * Waiting on a key
- * [in]    value[0].a	    OPTEE_RPC_WAIT_QUEUE_SLEEP
- * [in]    value[0].b	    Wait key
+ * This interface can also be used to wait for a asynchronous notification
+ * which instead is sent via a non-secure interrupt.
  *
- * Waking up a key
- * [in]    value[0].a	    OPTEE_RPC_WAIT_QUEUE_WAKEUP
- * [in]    value[0].b	    Wakeup key
+ * Waiting on notification
+ * [in]    value[0].a	    OPTEE_RPC_NOTIFICATION_WAIT
+ * [in]    value[0].b	    notification value
+ *
+ * Sending a synchronous notification
+ * [in]    value[0].a	    OPTEE_RPC_NOTIFICATION_SEND
+ * [in]    value[0].b	    notification value
  */
-#define OPTEE_RPC_CMD_WAIT_QUEUE	4
-#define OPTEE_RPC_WAIT_QUEUE_SLEEP	0
-#define OPTEE_RPC_WAIT_QUEUE_WAKEUP	1
+#define OPTEE_RPC_CMD_NOTIFICATION	4
+#define OPTEE_RPC_NOTIFICATION_WAIT	0
+#define OPTEE_RPC_NOTIFICATION_SEND	1
 
 /*
  * Suspend execution
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index efbaff7ad7e5..fa492655843a 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2015-2016, Linaro Limited
+ * Copyright (c) 2015-2021, Linaro Limited
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -14,23 +14,6 @@
 #include "optee_smc.h"
 #include "optee_rpc_cmd.h"
 
-struct wq_entry {
-	struct list_head link;
-	struct completion c;
-	u32 key;
-};
-
-void optee_wait_queue_init(struct optee_wait_queue *priv)
-{
-	mutex_init(&priv->mu);
-	INIT_LIST_HEAD(&priv->db);
-}
-
-void optee_wait_queue_exit(struct optee_wait_queue *priv)
-{
-	mutex_destroy(&priv->mu);
-}
-
 static void handle_rpc_func_cmd_get_time(struct optee_msg_arg *arg)
 {
 	struct timespec64 ts;
@@ -143,48 +126,6 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
 }
 #endif
 
-static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w;
-
-	mutex_lock(&wq->mu);
-
-	list_for_each_entry(w, &wq->db, link)
-		if (w->key == key)
-			goto out;
-
-	w = kmalloc(sizeof(*w), GFP_KERNEL);
-	if (w) {
-		init_completion(&w->c);
-		w->key = key;
-		list_add_tail(&w->link, &wq->db);
-	}
-out:
-	mutex_unlock(&wq->mu);
-	return w;
-}
-
-static void wq_sleep(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w = wq_entry_get(wq, key);
-
-	if (w) {
-		wait_for_completion(&w->c);
-		mutex_lock(&wq->mu);
-		list_del(&w->link);
-		mutex_unlock(&wq->mu);
-		kfree(w);
-	}
-}
-
-static void wq_wakeup(struct optee_wait_queue *wq, u32 key)
-{
-	struct wq_entry *w = wq_entry_get(wq, key);
-
-	if (w)
-		complete(&w->c);
-}
-
 static void handle_rpc_func_cmd_wq(struct optee *optee,
 				   struct optee_msg_arg *arg)
 {
@@ -196,11 +137,13 @@ static void handle_rpc_func_cmd_wq(struct optee *optee,
 		goto bad;
 
 	switch (arg->params[0].u.value.a) {
-	case OPTEE_RPC_WAIT_QUEUE_SLEEP:
-		wq_sleep(&optee->wait_queue, arg->params[0].u.value.b);
+	case OPTEE_RPC_NOTIFICATION_WAIT:
+		if (optee_notif_wait(optee, arg->params[0].u.value.b))
+			goto bad;
 		break;
-	case OPTEE_RPC_WAIT_QUEUE_WAKEUP:
-		wq_wakeup(&optee->wait_queue, arg->params[0].u.value.b);
+	case OPTEE_RPC_NOTIFICATION_SEND:
+		if (optee_notif_send(optee, arg->params[0].u.value.b))
+			goto bad;
 		break;
 	default:
 		goto bad;
@@ -463,7 +406,7 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
 	case OPTEE_RPC_CMD_GET_TIME:
 		handle_rpc_func_cmd_get_time(arg);
 		break;
-	case OPTEE_RPC_CMD_WAIT_QUEUE:
+	case OPTEE_RPC_CMD_NOTIFICATION:
 		handle_rpc_func_cmd_wq(optee, arg);
 		break;
 	case OPTEE_RPC_CMD_SUSPEND:
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 5/6] optee: separate notification functions

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Renames struct optee_wait_queue to struct optee_notif and all related
> functions to optee_notif_*().
>
> The implementation is changed to allow sending a notification from an
> atomic state, that is from the top half of an interrupt handler.
>
> Waiting for keys is currently only used when secure world is waiting for
> a mutex or condition variable. The old implementation could handle any
> 32-bit key while this new implementation is restricted to only 8 bits or
> the maximum value 255. A upper value is needed since a bitmap is
> allocated to allow an interrupt handler to only set a bit in case the
> waiter hasn't had the time yet to allocate and register a completion.
>
> The keys are currently only representing secure world threads which
> number usually are never even close to 255 so it should be safe for now.
> In future ABI updates the maximum value of the key will be communicated
> while the driver is initializing.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/Makefile        |   1 +
>  drivers/tee/optee/core.c          |  12 ++-
>  drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
>  drivers/tee/optee/optee_private.h |  19 +++--
>  drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
>  drivers/tee/optee/rpc.c           |  73 ++---------------
>  6 files changed, 170 insertions(+), 91 deletions(-)
>  create mode 100644 drivers/tee/optee/notif.c
>

Apart from minor nit below:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index 3aa33ea9e6a6..df55e4ad5370 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_OPTEE) += optee.o
>  optee-objs += core.o
>  optee-objs += call.o
> +optee-objs += notif.o
>  optee-objs += rpc.o
>  optee-objs += supp.o
>  optee-objs += shm_pool.o
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 5ce13b099d7d..8531184f98f4 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
>          */
>         optee_disable_shm_cache(optee);
>
> +       optee_notif_uninit(optee);
>         /*
>          * The two devices have to be unregistered before we can free the
>          * other resources.
> @@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
>         tee_shm_pool_free(optee->pool);
>         if (optee->memremaped_shm)
>                 memunmap(optee->memremaped_shm);
> -       optee_wait_queue_exit(&optee->wait_queue);
>         optee_supp_uninit(&optee->supp);
>         mutex_destroy(&optee->call_queue.mutex);
>
> @@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
>
>         mutex_init(&optee->call_queue.mutex);
>         INIT_LIST_HEAD(&optee->call_queue.waiters);
> -       optee_wait_queue_init(&optee->wait_queue);
>         optee_supp_init(&optee->supp);
>         optee->memremaped_shm = memremaped_shm;
>         optee->pool = pool;
>
> +       platform_set_drvdata(pdev, optee);
> +       rc = optee_notif_init(optee, 255);

nit: Can you use a macro here instead of a constant with a proper
comment similar to the one in commit description?

-Sumit

> +       if (rc) {
> +               optee_remove(pdev);
> +               return rc;
> +       }
> +
>         /*
>          * Ensure that there are no pre-existing shm objects before enabling
>          * the shm cache so that there's no chance of receiving an invalid
> @@ -731,8 +737,6 @@ static int optee_probe(struct platform_device *pdev)
>         if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>                 pr_info("dynamic shared memory is enabled\n");
>
> -       platform_set_drvdata(pdev, optee);
> -
>         rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
>         if (rc) {
>                 optee_remove(pdev);
> diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> new file mode 100644
> index 000000000000..a28fa03dcd0e
> --- /dev/null
> +++ b/drivers/tee/optee/notif.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, Linaro Limited
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/tee_drv.h>
> +#include "optee_private.h"
> +
> +struct notif_entry {
> +       struct list_head link;
> +       struct completion c;
> +       u_int key;
> +};
> +
> +static bool have_key(struct optee *optee, u_int key)
> +{
> +       struct notif_entry *entry;
> +
> +       list_for_each_entry(entry, &optee->notif.db, link)
> +               if (entry->key == key)
> +                       return true;
> +
> +       return false;
> +}
> +
> +int optee_notif_wait(struct optee *optee, u_int key)
> +{
> +       unsigned long flags;
> +       struct notif_entry *entry;
> +       int rc = 0;
> +
> +       if (key > optee->notif.max_key)
> +               return -EINVAL;
> +
> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;
> +       init_completion(&entry->c);
> +       entry->key = key;
> +
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       /*
> +        * If the bit is already set it means that the key has already
> +        * been posted and we must not wait.
> +        */
> +       if (test_bit(key, optee->notif.bitmap)) {
> +               clear_bit(key, optee->notif.bitmap);
> +               goto out;
> +       }
> +
> +       /*
> +        * Check if someone is already waiting for this key. If there is
> +        * it's a programming error.
> +        */
> +       if (have_key(optee, key)) {
> +               rc = -EBUSY;
> +               goto out;
> +       }
> +
> +       list_add_tail(&entry->link, &optee->notif.db);
> +
> +       /*
> +        * Unlock temporarily and wait for completion.
> +        */
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +       wait_for_completion(&entry->c);
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       list_del(&entry->link);
> +out:
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +
> +       kfree(entry);
> +
> +       return rc;
> +}
> +
> +int optee_notif_send(struct optee *optee, u_int key)
> +{
> +       unsigned long flags;
> +       struct notif_entry *entry;
> +
> +       if (key > optee->notif.max_key)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       list_for_each_entry(entry, &optee->notif.db, link)
> +               if (entry->key == key) {
> +                       complete(&entry->c);
> +                       goto out;
> +               }
> +
> +       /* Only set the bit in case there where nobody waiting */
> +       set_bit(key, optee->notif.bitmap);
> +out:
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +
> +       return 0;
> +}
> +
> +int optee_notif_init(struct optee *optee, u_int max_key)
> +{
> +       spin_lock_init(&optee->notif.lock);
> +       INIT_LIST_HEAD(&optee->notif.db);
> +       optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> +       if (!optee->notif.bitmap)
> +               return -ENOMEM;
> +
> +       optee->notif.max_key = max_key;
> +
> +       return 0;
> +}
> +
> +void optee_notif_uninit(struct optee *optee)
> +{
> +       kfree(optee->notif.bitmap);
> +}
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index dbdd367be156..76a16d9b6cf4 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -35,10 +35,12 @@ struct optee_call_queue {
>         struct list_head waiters;
>  };
>
> -struct optee_wait_queue {
> -       /* Serializes access to this struct */
> -       struct mutex mu;
> +struct optee_notif {
> +       u_int max_key;
> +       /* Serializes access to the elements below in this struct */
> +       spinlock_t lock;
>         struct list_head db;
> +       u_long *bitmap;
>  };
>
>  /**
> @@ -72,8 +74,7 @@ struct optee_supp {
>   * @teedev:            client device
>   * @invoke_fn:         function to issue smc or hvc
>   * @call_queue:                queue of threads waiting to call @invoke_fn
> - * @wait_queue:                queue of threads from secure world waiting for a
> - *                     secure world sync object
> + * @notif:             notification synchronization struct
>   * @supp:              supplicant synchronization struct for RPC to supplicant
>   * @pool:              shared memory pool
>   * @memremaped_shm     virtual address of memory in shared memory pool
> @@ -88,7 +89,7 @@ struct optee {
>         struct tee_device *teedev;
>         optee_invoke_fn *invoke_fn;
>         struct optee_call_queue call_queue;
> -       struct optee_wait_queue wait_queue;
> +       struct optee_notif notif;
>         struct optee_supp supp;
>         struct tee_shm_pool *pool;
>         void *memremaped_shm;
> @@ -131,8 +132,10 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>                       struct optee_call_ctx *call_ctx);
>  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
>
> -void optee_wait_queue_init(struct optee_wait_queue *wq);
> -void optee_wait_queue_exit(struct optee_wait_queue *wq);
> +int optee_notif_init(struct optee *optee, u_int max_key);
> +void optee_notif_uninit(struct optee *optee);
> +int optee_notif_wait(struct optee *optee, u_int key);
> +int optee_notif_send(struct optee *optee, u_int key);
>
>  u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
>                         struct tee_param *param);
> diff --git a/drivers/tee/optee/optee_rpc_cmd.h b/drivers/tee/optee/optee_rpc_cmd.h
> index b8275140cef8..f3f06e0994a7 100644
> --- a/drivers/tee/optee/optee_rpc_cmd.h
> +++ b/drivers/tee/optee/optee_rpc_cmd.h
> @@ -28,24 +28,27 @@
>  #define OPTEE_RPC_CMD_GET_TIME         3
>
>  /*
> - * Wait queue primitive, helper for secure world to implement a wait queue.
> + * Notification from/to secure world.
>   *
> - * If secure world needs to wait for a secure world mutex it issues a sleep
> - * request instead of spinning in secure world. Conversely is a wakeup
> - * request issued when a secure world mutex with a thread waiting thread is
> - * unlocked.
> + * If secure world needs to wait for something, for instance a mutex, it
> + * does a notification wait request instead of spinning in secure world.
> + * Conversely can a synchronous notification can be sent when a secure
> + * world mutex with a thread waiting thread is unlocked.
>   *
> - * Waiting on a key
> - * [in]    value[0].a      OPTEE_RPC_WAIT_QUEUE_SLEEP
> - * [in]    value[0].b      Wait key
> + * This interface can also be used to wait for a asynchronous notification
> + * which instead is sent via a non-secure interrupt.
>   *
> - * Waking up a key
> - * [in]    value[0].a      OPTEE_RPC_WAIT_QUEUE_WAKEUP
> - * [in]    value[0].b      Wakeup key
> + * Waiting on notification
> + * [in]    value[0].a      OPTEE_RPC_NOTIFICATION_WAIT
> + * [in]    value[0].b      notification value
> + *
> + * Sending a synchronous notification
> + * [in]    value[0].a      OPTEE_RPC_NOTIFICATION_SEND
> + * [in]    value[0].b      notification value
>   */
> -#define OPTEE_RPC_CMD_WAIT_QUEUE       4
> -#define OPTEE_RPC_WAIT_QUEUE_SLEEP     0
> -#define OPTEE_RPC_WAIT_QUEUE_WAKEUP    1
> +#define OPTEE_RPC_CMD_NOTIFICATION     4
> +#define OPTEE_RPC_NOTIFICATION_WAIT    0
> +#define OPTEE_RPC_NOTIFICATION_SEND    1
>
>  /*
>   * Suspend execution
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index efbaff7ad7e5..fa492655843a 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2015-2016, Linaro Limited
> + * Copyright (c) 2015-2021, Linaro Limited
>   */
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -14,23 +14,6 @@
>  #include "optee_smc.h"
>  #include "optee_rpc_cmd.h"
>
> -struct wq_entry {
> -       struct list_head link;
> -       struct completion c;
> -       u32 key;
> -};
> -
> -void optee_wait_queue_init(struct optee_wait_queue *priv)
> -{
> -       mutex_init(&priv->mu);
> -       INIT_LIST_HEAD(&priv->db);
> -}
> -
> -void optee_wait_queue_exit(struct optee_wait_queue *priv)
> -{
> -       mutex_destroy(&priv->mu);
> -}
> -
>  static void handle_rpc_func_cmd_get_time(struct optee_msg_arg *arg)
>  {
>         struct timespec64 ts;
> @@ -143,48 +126,6 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>  }
>  #endif
>
> -static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w;
> -
> -       mutex_lock(&wq->mu);
> -
> -       list_for_each_entry(w, &wq->db, link)
> -               if (w->key == key)
> -                       goto out;
> -
> -       w = kmalloc(sizeof(*w), GFP_KERNEL);
> -       if (w) {
> -               init_completion(&w->c);
> -               w->key = key;
> -               list_add_tail(&w->link, &wq->db);
> -       }
> -out:
> -       mutex_unlock(&wq->mu);
> -       return w;
> -}
> -
> -static void wq_sleep(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w = wq_entry_get(wq, key);
> -
> -       if (w) {
> -               wait_for_completion(&w->c);
> -               mutex_lock(&wq->mu);
> -               list_del(&w->link);
> -               mutex_unlock(&wq->mu);
> -               kfree(w);
> -       }
> -}
> -
> -static void wq_wakeup(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w = wq_entry_get(wq, key);
> -
> -       if (w)
> -               complete(&w->c);
> -}
> -
>  static void handle_rpc_func_cmd_wq(struct optee *optee,
>                                    struct optee_msg_arg *arg)
>  {
> @@ -196,11 +137,13 @@ static void handle_rpc_func_cmd_wq(struct optee *optee,
>                 goto bad;
>
>         switch (arg->params[0].u.value.a) {
> -       case OPTEE_RPC_WAIT_QUEUE_SLEEP:
> -               wq_sleep(&optee->wait_queue, arg->params[0].u.value.b);
> +       case OPTEE_RPC_NOTIFICATION_WAIT:
> +               if (optee_notif_wait(optee, arg->params[0].u.value.b))
> +                       goto bad;
>                 break;
> -       case OPTEE_RPC_WAIT_QUEUE_WAKEUP:
> -               wq_wakeup(&optee->wait_queue, arg->params[0].u.value.b);
> +       case OPTEE_RPC_NOTIFICATION_SEND:
> +               if (optee_notif_send(optee, arg->params[0].u.value.b))
> +                       goto bad;
>                 break;
>         default:
>                 goto bad;
> @@ -463,7 +406,7 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
>         case OPTEE_RPC_CMD_GET_TIME:
>                 handle_rpc_func_cmd_get_time(arg);
>                 break;
> -       case OPTEE_RPC_CMD_WAIT_QUEUE:
> +       case OPTEE_RPC_CMD_NOTIFICATION:
>                 handle_rpc_func_cmd_wq(optee, arg);
>                 break;
>         case OPTEE_RPC_CMD_SUSPEND:
> --
> 2.31.1
>

Re: [PATCH v6 5/6] optee: separate notification functions

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Renames struct optee_wait_queue to struct optee_notif and all related
> functions to optee_notif_*().
>
> The implementation is changed to allow sending a notification from an
> atomic state, that is from the top half of an interrupt handler.
>
> Waiting for keys is currently only used when secure world is waiting for
> a mutex or condition variable. The old implementation could handle any
> 32-bit key while this new implementation is restricted to only 8 bits or
> the maximum value 255. A upper value is needed since a bitmap is
> allocated to allow an interrupt handler to only set a bit in case the
> waiter hasn't had the time yet to allocate and register a completion.
>
> The keys are currently only representing secure world threads which
> number usually are never even close to 255 so it should be safe for now.
> In future ABI updates the maximum value of the key will be communicated
> while the driver is initializing.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/Makefile        |   1 +
>  drivers/tee/optee/core.c          |  12 ++-
>  drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
>  drivers/tee/optee/optee_private.h |  19 +++--
>  drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
>  drivers/tee/optee/rpc.c           |  73 ++---------------
>  6 files changed, 170 insertions(+), 91 deletions(-)
>  create mode 100644 drivers/tee/optee/notif.c
>

Apart from minor nit below:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index 3aa33ea9e6a6..df55e4ad5370 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_OPTEE) += optee.o
>  optee-objs += core.o
>  optee-objs += call.o
> +optee-objs += notif.o
>  optee-objs += rpc.o
>  optee-objs += supp.o
>  optee-objs += shm_pool.o
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 5ce13b099d7d..8531184f98f4 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
>          */
>         optee_disable_shm_cache(optee);
>
> +       optee_notif_uninit(optee);
>         /*
>          * The two devices have to be unregistered before we can free the
>          * other resources.
> @@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
>         tee_shm_pool_free(optee->pool);
>         if (optee->memremaped_shm)
>                 memunmap(optee->memremaped_shm);
> -       optee_wait_queue_exit(&optee->wait_queue);
>         optee_supp_uninit(&optee->supp);
>         mutex_destroy(&optee->call_queue.mutex);
>
> @@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
>
>         mutex_init(&optee->call_queue.mutex);
>         INIT_LIST_HEAD(&optee->call_queue.waiters);
> -       optee_wait_queue_init(&optee->wait_queue);
>         optee_supp_init(&optee->supp);
>         optee->memremaped_shm = memremaped_shm;
>         optee->pool = pool;
>
> +       platform_set_drvdata(pdev, optee);
> +       rc = optee_notif_init(optee, 255);

nit: Can you use a macro here instead of a constant with a proper
comment similar to the one in commit description?

-Sumit

> +       if (rc) {
> +               optee_remove(pdev);
> +               return rc;
> +       }
> +
>         /*
>          * Ensure that there are no pre-existing shm objects before enabling
>          * the shm cache so that there's no chance of receiving an invalid
> @@ -731,8 +737,6 @@ static int optee_probe(struct platform_device *pdev)
>         if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>                 pr_info("dynamic shared memory is enabled\n");
>
> -       platform_set_drvdata(pdev, optee);
> -
>         rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
>         if (rc) {
>                 optee_remove(pdev);
> diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> new file mode 100644
> index 000000000000..a28fa03dcd0e
> --- /dev/null
> +++ b/drivers/tee/optee/notif.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, Linaro Limited
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/tee_drv.h>
> +#include "optee_private.h"
> +
> +struct notif_entry {
> +       struct list_head link;
> +       struct completion c;
> +       u_int key;
> +};
> +
> +static bool have_key(struct optee *optee, u_int key)
> +{
> +       struct notif_entry *entry;
> +
> +       list_for_each_entry(entry, &optee->notif.db, link)
> +               if (entry->key == key)
> +                       return true;
> +
> +       return false;
> +}
> +
> +int optee_notif_wait(struct optee *optee, u_int key)
> +{
> +       unsigned long flags;
> +       struct notif_entry *entry;
> +       int rc = 0;
> +
> +       if (key > optee->notif.max_key)
> +               return -EINVAL;
> +
> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;
> +       init_completion(&entry->c);
> +       entry->key = key;
> +
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       /*
> +        * If the bit is already set it means that the key has already
> +        * been posted and we must not wait.
> +        */
> +       if (test_bit(key, optee->notif.bitmap)) {
> +               clear_bit(key, optee->notif.bitmap);
> +               goto out;
> +       }
> +
> +       /*
> +        * Check if someone is already waiting for this key. If there is
> +        * it's a programming error.
> +        */
> +       if (have_key(optee, key)) {
> +               rc = -EBUSY;
> +               goto out;
> +       }
> +
> +       list_add_tail(&entry->link, &optee->notif.db);
> +
> +       /*
> +        * Unlock temporarily and wait for completion.
> +        */
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +       wait_for_completion(&entry->c);
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       list_del(&entry->link);
> +out:
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +
> +       kfree(entry);
> +
> +       return rc;
> +}
> +
> +int optee_notif_send(struct optee *optee, u_int key)
> +{
> +       unsigned long flags;
> +       struct notif_entry *entry;
> +
> +       if (key > optee->notif.max_key)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&optee->notif.lock, flags);
> +
> +       list_for_each_entry(entry, &optee->notif.db, link)
> +               if (entry->key == key) {
> +                       complete(&entry->c);
> +                       goto out;
> +               }
> +
> +       /* Only set the bit in case there where nobody waiting */
> +       set_bit(key, optee->notif.bitmap);
> +out:
> +       spin_unlock_irqrestore(&optee->notif.lock, flags);
> +
> +       return 0;
> +}
> +
> +int optee_notif_init(struct optee *optee, u_int max_key)
> +{
> +       spin_lock_init(&optee->notif.lock);
> +       INIT_LIST_HEAD(&optee->notif.db);
> +       optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> +       if (!optee->notif.bitmap)
> +               return -ENOMEM;
> +
> +       optee->notif.max_key = max_key;
> +
> +       return 0;
> +}
> +
> +void optee_notif_uninit(struct optee *optee)
> +{
> +       kfree(optee->notif.bitmap);
> +}
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index dbdd367be156..76a16d9b6cf4 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -35,10 +35,12 @@ struct optee_call_queue {
>         struct list_head waiters;
>  };
>
> -struct optee_wait_queue {
> -       /* Serializes access to this struct */
> -       struct mutex mu;
> +struct optee_notif {
> +       u_int max_key;
> +       /* Serializes access to the elements below in this struct */
> +       spinlock_t lock;
>         struct list_head db;
> +       u_long *bitmap;
>  };
>
>  /**
> @@ -72,8 +74,7 @@ struct optee_supp {
>   * @teedev:            client device
>   * @invoke_fn:         function to issue smc or hvc
>   * @call_queue:                queue of threads waiting to call @invoke_fn
> - * @wait_queue:                queue of threads from secure world waiting for a
> - *                     secure world sync object
> + * @notif:             notification synchronization struct
>   * @supp:              supplicant synchronization struct for RPC to supplicant
>   * @pool:              shared memory pool
>   * @memremaped_shm     virtual address of memory in shared memory pool
> @@ -88,7 +89,7 @@ struct optee {
>         struct tee_device *teedev;
>         optee_invoke_fn *invoke_fn;
>         struct optee_call_queue call_queue;
> -       struct optee_wait_queue wait_queue;
> +       struct optee_notif notif;
>         struct optee_supp supp;
>         struct tee_shm_pool *pool;
>         void *memremaped_shm;
> @@ -131,8 +132,10 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>                       struct optee_call_ctx *call_ctx);
>  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
>
> -void optee_wait_queue_init(struct optee_wait_queue *wq);
> -void optee_wait_queue_exit(struct optee_wait_queue *wq);
> +int optee_notif_init(struct optee *optee, u_int max_key);
> +void optee_notif_uninit(struct optee *optee);
> +int optee_notif_wait(struct optee *optee, u_int key);
> +int optee_notif_send(struct optee *optee, u_int key);
>
>  u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
>                         struct tee_param *param);
> diff --git a/drivers/tee/optee/optee_rpc_cmd.h b/drivers/tee/optee/optee_rpc_cmd.h
> index b8275140cef8..f3f06e0994a7 100644
> --- a/drivers/tee/optee/optee_rpc_cmd.h
> +++ b/drivers/tee/optee/optee_rpc_cmd.h
> @@ -28,24 +28,27 @@
>  #define OPTEE_RPC_CMD_GET_TIME         3
>
>  /*
> - * Wait queue primitive, helper for secure world to implement a wait queue.
> + * Notification from/to secure world.
>   *
> - * If secure world needs to wait for a secure world mutex it issues a sleep
> - * request instead of spinning in secure world. Conversely is a wakeup
> - * request issued when a secure world mutex with a thread waiting thread is
> - * unlocked.
> + * If secure world needs to wait for something, for instance a mutex, it
> + * does a notification wait request instead of spinning in secure world.
> + * Conversely can a synchronous notification can be sent when a secure
> + * world mutex with a thread waiting thread is unlocked.
>   *
> - * Waiting on a key
> - * [in]    value[0].a      OPTEE_RPC_WAIT_QUEUE_SLEEP
> - * [in]    value[0].b      Wait key
> + * This interface can also be used to wait for a asynchronous notification
> + * which instead is sent via a non-secure interrupt.
>   *
> - * Waking up a key
> - * [in]    value[0].a      OPTEE_RPC_WAIT_QUEUE_WAKEUP
> - * [in]    value[0].b      Wakeup key
> + * Waiting on notification
> + * [in]    value[0].a      OPTEE_RPC_NOTIFICATION_WAIT
> + * [in]    value[0].b      notification value
> + *
> + * Sending a synchronous notification
> + * [in]    value[0].a      OPTEE_RPC_NOTIFICATION_SEND
> + * [in]    value[0].b      notification value
>   */
> -#define OPTEE_RPC_CMD_WAIT_QUEUE       4
> -#define OPTEE_RPC_WAIT_QUEUE_SLEEP     0
> -#define OPTEE_RPC_WAIT_QUEUE_WAKEUP    1
> +#define OPTEE_RPC_CMD_NOTIFICATION     4
> +#define OPTEE_RPC_NOTIFICATION_WAIT    0
> +#define OPTEE_RPC_NOTIFICATION_SEND    1
>
>  /*
>   * Suspend execution
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index efbaff7ad7e5..fa492655843a 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2015-2016, Linaro Limited
> + * Copyright (c) 2015-2021, Linaro Limited
>   */
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -14,23 +14,6 @@
>  #include "optee_smc.h"
>  #include "optee_rpc_cmd.h"
>
> -struct wq_entry {
> -       struct list_head link;
> -       struct completion c;
> -       u32 key;
> -};
> -
> -void optee_wait_queue_init(struct optee_wait_queue *priv)
> -{
> -       mutex_init(&priv->mu);
> -       INIT_LIST_HEAD(&priv->db);
> -}
> -
> -void optee_wait_queue_exit(struct optee_wait_queue *priv)
> -{
> -       mutex_destroy(&priv->mu);
> -}
> -
>  static void handle_rpc_func_cmd_get_time(struct optee_msg_arg *arg)
>  {
>         struct timespec64 ts;
> @@ -143,48 +126,6 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>  }
>  #endif
>
> -static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w;
> -
> -       mutex_lock(&wq->mu);
> -
> -       list_for_each_entry(w, &wq->db, link)
> -               if (w->key == key)
> -                       goto out;
> -
> -       w = kmalloc(sizeof(*w), GFP_KERNEL);
> -       if (w) {
> -               init_completion(&w->c);
> -               w->key = key;
> -               list_add_tail(&w->link, &wq->db);
> -       }
> -out:
> -       mutex_unlock(&wq->mu);
> -       return w;
> -}
> -
> -static void wq_sleep(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w = wq_entry_get(wq, key);
> -
> -       if (w) {
> -               wait_for_completion(&w->c);
> -               mutex_lock(&wq->mu);
> -               list_del(&w->link);
> -               mutex_unlock(&wq->mu);
> -               kfree(w);
> -       }
> -}
> -
> -static void wq_wakeup(struct optee_wait_queue *wq, u32 key)
> -{
> -       struct wq_entry *w = wq_entry_get(wq, key);
> -
> -       if (w)
> -               complete(&w->c);
> -}
> -
>  static void handle_rpc_func_cmd_wq(struct optee *optee,
>                                    struct optee_msg_arg *arg)
>  {
> @@ -196,11 +137,13 @@ static void handle_rpc_func_cmd_wq(struct optee *optee,
>                 goto bad;
>
>         switch (arg->params[0].u.value.a) {
> -       case OPTEE_RPC_WAIT_QUEUE_SLEEP:
> -               wq_sleep(&optee->wait_queue, arg->params[0].u.value.b);
> +       case OPTEE_RPC_NOTIFICATION_WAIT:
> +               if (optee_notif_wait(optee, arg->params[0].u.value.b))
> +                       goto bad;
>                 break;
> -       case OPTEE_RPC_WAIT_QUEUE_WAKEUP:
> -               wq_wakeup(&optee->wait_queue, arg->params[0].u.value.b);
> +       case OPTEE_RPC_NOTIFICATION_SEND:
> +               if (optee_notif_send(optee, arg->params[0].u.value.b))
> +                       goto bad;
>                 break;
>         default:
>                 goto bad;
> @@ -463,7 +406,7 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
>         case OPTEE_RPC_CMD_GET_TIME:
>                 handle_rpc_func_cmd_get_time(arg);
>                 break;
> -       case OPTEE_RPC_CMD_WAIT_QUEUE:
> +       case OPTEE_RPC_CMD_NOTIFICATION:
>                 handle_rpc_func_cmd_wq(optee, arg);
>                 break;
>         case OPTEE_RPC_CMD_SUSPEND:
> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 5/6] optee: separate notification functions

[Jens Wiklander]

On Wed, Oct 13, 2021 at 9:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Renames struct optee_wait_queue to struct optee_notif and all related
> > functions to optee_notif_*().
> >
> > The implementation is changed to allow sending a notification from an
> > atomic state, that is from the top half of an interrupt handler.
> >
> > Waiting for keys is currently only used when secure world is waiting for
> > a mutex or condition variable. The old implementation could handle any
> > 32-bit key while this new implementation is restricted to only 8 bits or
> > the maximum value 255. A upper value is needed since a bitmap is
> > allocated to allow an interrupt handler to only set a bit in case the
> > waiter hasn't had the time yet to allocate and register a completion.
> >
> > The keys are currently only representing secure world threads which
> > number usually are never even close to 255 so it should be safe for now.
> > In future ABI updates the maximum value of the key will be communicated
> > while the driver is initializing.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/Makefile        |   1 +
> >  drivers/tee/optee/core.c          |  12 ++-
> >  drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
> >  drivers/tee/optee/optee_private.h |  19 +++--
> >  drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
> >  drivers/tee/optee/rpc.c           |  73 ++---------------
> >  6 files changed, 170 insertions(+), 91 deletions(-)
> >  create mode 100644 drivers/tee/optee/notif.c
> >
>
> Apart from minor nit below:
>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 3aa33ea9e6a6..df55e4ad5370 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,6 +2,7 @@
> >  obj-$(CONFIG_OPTEE) += optee.o
> >  optee-objs += core.o
> >  optee-objs += call.o
> > +optee-objs += notif.o
> >  optee-objs += rpc.o
> >  optee-objs += supp.o
> >  optee-objs += shm_pool.o
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 5ce13b099d7d..8531184f98f4 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
> >          */
> >         optee_disable_shm_cache(optee);
> >
> > +       optee_notif_uninit(optee);
> >         /*
> >          * The two devices have to be unregistered before we can free the
> >          * other resources.
> > @@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
> >         tee_shm_pool_free(optee->pool);
> >         if (optee->memremaped_shm)
> >                 memunmap(optee->memremaped_shm);
> > -       optee_wait_queue_exit(&optee->wait_queue);
> >         optee_supp_uninit(&optee->supp);
> >         mutex_destroy(&optee->call_queue.mutex);
> >
> > @@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
> >
> >         mutex_init(&optee->call_queue.mutex);
> >         INIT_LIST_HEAD(&optee->call_queue.waiters);
> > -       optee_wait_queue_init(&optee->wait_queue);
> >         optee_supp_init(&optee->supp);
> >         optee->memremaped_shm = memremaped_shm;
> >         optee->pool = pool;
> >
> > +       platform_set_drvdata(pdev, optee);
> > +       rc = optee_notif_init(optee, 255);
>
> nit: Can you use a macro here instead of a constant with a proper
> comment similar to the one in commit description?

OK, I'll fix.

Thanks,
Jens

Re: [PATCH v6 5/6] optee: separate notification functions

[Jens Wiklander]

On Wed, Oct 13, 2021 at 9:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Renames struct optee_wait_queue to struct optee_notif and all related
> > functions to optee_notif_*().
> >
> > The implementation is changed to allow sending a notification from an
> > atomic state, that is from the top half of an interrupt handler.
> >
> > Waiting for keys is currently only used when secure world is waiting for
> > a mutex or condition variable. The old implementation could handle any
> > 32-bit key while this new implementation is restricted to only 8 bits or
> > the maximum value 255. A upper value is needed since a bitmap is
> > allocated to allow an interrupt handler to only set a bit in case the
> > waiter hasn't had the time yet to allocate and register a completion.
> >
> > The keys are currently only representing secure world threads which
> > number usually are never even close to 255 so it should be safe for now.
> > In future ABI updates the maximum value of the key will be communicated
> > while the driver is initializing.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/Makefile        |   1 +
> >  drivers/tee/optee/core.c          |  12 ++-
> >  drivers/tee/optee/notif.c         | 125 ++++++++++++++++++++++++++++++
> >  drivers/tee/optee/optee_private.h |  19 +++--
> >  drivers/tee/optee/optee_rpc_cmd.h |  31 ++++----
> >  drivers/tee/optee/rpc.c           |  73 ++---------------
> >  6 files changed, 170 insertions(+), 91 deletions(-)
> >  create mode 100644 drivers/tee/optee/notif.c
> >
>
> Apart from minor nit below:
>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 3aa33ea9e6a6..df55e4ad5370 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,6 +2,7 @@
> >  obj-$(CONFIG_OPTEE) += optee.o
> >  optee-objs += core.o
> >  optee-objs += call.o
> > +optee-objs += notif.o
> >  optee-objs += rpc.o
> >  optee-objs += supp.o
> >  optee-objs += shm_pool.o
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 5ce13b099d7d..8531184f98f4 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -592,6 +592,7 @@ static int optee_remove(struct platform_device *pdev)
> >          */
> >         optee_disable_shm_cache(optee);
> >
> > +       optee_notif_uninit(optee);
> >         /*
> >          * The two devices have to be unregistered before we can free the
> >          * other resources.
> > @@ -602,7 +603,6 @@ static int optee_remove(struct platform_device *pdev)
> >         tee_shm_pool_free(optee->pool);
> >         if (optee->memremaped_shm)
> >                 memunmap(optee->memremaped_shm);
> > -       optee_wait_queue_exit(&optee->wait_queue);
> >         optee_supp_uninit(&optee->supp);
> >         mutex_destroy(&optee->call_queue.mutex);
> >
> > @@ -712,11 +712,17 @@ static int optee_probe(struct platform_device *pdev)
> >
> >         mutex_init(&optee->call_queue.mutex);
> >         INIT_LIST_HEAD(&optee->call_queue.waiters);
> > -       optee_wait_queue_init(&optee->wait_queue);
> >         optee_supp_init(&optee->supp);
> >         optee->memremaped_shm = memremaped_shm;
> >         optee->pool = pool;
> >
> > +       platform_set_drvdata(pdev, optee);
> > +       rc = optee_notif_init(optee, 255);
>
> nit: Can you use a macro here instead of a constant with a proper
> comment similar to the one in commit description?

OK, I'll fix.

Thanks,
Jens

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 6/6] optee: add asynchronous notifications

[Jens Wiklander]

Adds support for asynchronous notifications from secure world to normal
world. This allows a design with a top half and bottom half type of
driver where the top half runs in secure interrupt context and a
notifications tells normal world to schedule a yielding call to do the
bottom half processing.

The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.

A notification consists of a 32-bit value which normal world can
retrieve using a fastcall into secure world. The value
OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
When this value is sent it means that normal world is supposed to make a
yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.

Notification capability is negotiated while the driver is initialized.
If both sides supports these notifications then they are enabled.

An interrupt is used to notify the driver that there are asynchronous
notifications pending. The maximum needed notification value is
communicated at this stage. This allows scaling up when needed.

Acked-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/call.c          |  27 ++++++++
 drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
 drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
 drivers/tee/optee/optee_msg.h     |   9 +++
 drivers/tee/optee/optee_private.h |   6 +-
 drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
 6 files changed, 277 insertions(+), 32 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 945f03da0223..e428e0e9a3af 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 	return 0;
 }
 
+static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
+{
+	struct optee_msg_arg *msg_arg;
+	phys_addr_t msg_parg;
+	struct tee_shm *shm;
+
+	shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
+	if (IS_ERR(shm))
+		return PTR_ERR(shm);
+
+	msg_arg->cmd = cmd;
+	optee_do_call_with_arg(ctx, msg_parg);
+
+	tee_shm_free(shm);
+	return 0;
+}
+
+int optee_do_bottom_half(struct tee_context *ctx)
+{
+	return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
+}
+
+int optee_stop_async_notif(struct tee_context *ctx)
+{
+	return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);
+}
+
 /**
  * optee_enable_shm_cache() - Enables caching of some shared memory allocation
  *			      in OP-TEE
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 8531184f98f4..ccafd7151b45 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -8,9 +8,12 @@
 #include <linux/arm-smccc.h>
 #include <linux/crash_dump.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
 	.flags = TEE_DESC_PRIVILEGED,
 };
 
+static int enable_async_notif(optee_invoke_fn *invoke_fn)
+{
+	struct arm_smccc_res res;
+
+	invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0)
+		return -EINVAL;
+	return 0;
+}
+
 static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
 {
 	struct arm_smccc_res res;
@@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
 }
 
 static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
-					    u32 *sec_caps)
+					    u32 *sec_caps, u32 *max_notif_value)
 {
 	union {
 		struct arm_smccc_res smccc;
@@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
 		return false;
 
 	*sec_caps = res.result.capabilities;
+	*max_notif_value = res.result.max_notif_value;
 	return true;
 }
 
@@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
 	struct optee *optee = NULL;
 	void *memremaped_shm = NULL;
 	struct tee_device *teedev;
+	u32 max_notif_value;
 	u32 sec_caps;
 	int rc;
 
@@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
+	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
+					     &max_notif_value)) {
 		pr_warn("capabilities mismatch\n");
 		return -EINVAL;
 	}
@@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
 	if (!optee) {
 		rc = -ENOMEM;
-		goto err;
+		goto err_free_pool;
 	}
 
 	optee->invoke_fn = invoke_fn;
@@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
 	teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
-		goto err;
+		goto err_free_optee;
 	}
 	optee->teedev = teedev;
 
 	teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
-		goto err;
+		goto err_unreg_teedev;
 	}
 	optee->supp_teedev = teedev;
 
 	rc = tee_device_register(optee->teedev);
 	if (rc)
-		goto err;
+		goto err_unreg_supp_teedev;
 
 	rc = tee_device_register(optee->supp_teedev);
 	if (rc)
-		goto err;
+		goto err_unreg_supp_teedev;
 
 	mutex_init(&optee->call_queue.mutex);
 	INIT_LIST_HEAD(&optee->call_queue.waiters);
@@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
 	optee->pool = pool;
 
 	platform_set_drvdata(pdev, optee);
-	rc = optee_notif_init(optee, 255);
-	if (rc) {
-		optee_remove(pdev);
-		return rc;
+
+	if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
+		unsigned int irq;
+
+		rc = platform_get_irq(pdev, 0);
+		if (rc < 0) {
+			pr_err("platform_get_irq: ret %d\n", rc);
+			goto err_unreg_supp_teedev;
+		}
+		irq = rc;
+
+		rc = optee_notif_init(optee, max_notif_value, irq);
+		if (rc) {
+			irq_dispose_mapping(irq);
+			optee_remove(pdev);
+			return rc;
+		}
+		enable_async_notif(optee->invoke_fn);
+		pr_info("Asynchronous notifications enabled\n");
+	} else {
+		rc = optee_notif_init(optee, 255, 0);
+		if (rc) {
+			optee_remove(pdev);
+			return rc;
+		}
 	}
 
 	/*
@@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
 
 	pr_info("initialized driver\n");
 	return 0;
-err:
-	if (optee) {
-		/*
-		 * tee_device_unregister() is safe to call even if the
-		 * devices hasn't been registered with
-		 * tee_device_register() yet.
-		 */
-		tee_device_unregister(optee->supp_teedev);
-		tee_device_unregister(optee->teedev);
-		kfree(optee);
-	}
-	if (pool)
-		tee_shm_pool_free(pool);
-	if (memremaped_shm)
+err_unreg_supp_teedev:
+	tee_device_unregister(optee->supp_teedev);
+err_unreg_teedev:
+	tee_device_unregister(optee->teedev);
+err_free_optee:
+	kfree(optee);
+err_free_pool:
+	tee_shm_pool_free(pool);
+	if (optee->memremaped_shm)
 		memunmap(memremaped_shm);
 	return rc;
 }
diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
index a28fa03dcd0e..2c888ad87451 100644
--- a/drivers/tee/optee/notif.c
+++ b/drivers/tee/optee/notif.c
@@ -7,10 +7,14 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/tee_drv.h>
 #include "optee_private.h"
+#include "optee_smc.h"
+#include "optee_rpc_cmd.h"
 
 struct notif_entry {
 	struct list_head link;
@@ -18,6 +22,54 @@ struct notif_entry {
 	u_int key;
 };
 
+static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
+				 bool *value_pending)
+{
+	struct arm_smccc_res res;
+
+	invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0)
+		return 0;
+	*value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
+	*value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
+	return res.a1;
+}
+
+static irqreturn_t notif_irq_handler(int irq, void *dev_id)
+{
+	struct optee *optee = dev_id;
+	bool do_bottom_half = false;
+	bool value_valid;
+	bool value_pending;
+	u32 value;
+
+	do {
+		value = get_async_notif_value(optee->invoke_fn, &value_valid,
+					      &value_pending);
+		if (!value_valid)
+			break;
+
+		if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
+			do_bottom_half = true;
+		else
+			optee_notif_send(optee, value);
+	} while (value_pending);
+
+	if (do_bottom_half)
+		return IRQ_WAKE_THREAD;
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
+{
+	struct optee *optee = dev_id;
+
+	optee_do_bottom_half(optee->notif.ctx);
+
+	return IRQ_HANDLED;
+}
+
 static bool have_key(struct optee *optee, u_int key)
 {
 	struct notif_entry *entry;
@@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
 	return 0;
 }
 
-int optee_notif_init(struct optee *optee, u_int max_key)
+int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
 {
+	struct tee_context *ctx;
+	int rc;
+
+	if (irq) {
+		ctx = teedev_open(optee->teedev);
+		if (IS_ERR(ctx))
+			return PTR_ERR(ctx);
+
+		optee->notif.ctx = ctx;
+	}
+
 	spin_lock_init(&optee->notif.lock);
 	INIT_LIST_HEAD(&optee->notif.db);
 	optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
-	if (!optee->notif.bitmap)
-		return -ENOMEM;
-
+	if (!optee->notif.bitmap) {
+		rc = -ENOMEM;
+		goto err_close_ctx;
+	}
 	optee->notif.max_key = max_key;
 
+	if (irq) {
+		rc = request_threaded_irq(irq, notif_irq_handler,
+					  notif_irq_thread_fn,
+					  0, "optee_notification", optee);
+		if (rc)
+			goto err_free_bitmap;
+
+		optee->notif.irq = irq;
+	}
+
 	return 0;
+
+err_free_bitmap:
+	kfree(optee->notif.bitmap);
+err_close_ctx:
+	teedev_close_context(optee->notif.ctx);
+	optee->notif.ctx = NULL;
+
+	return rc;
 }
 
 void optee_notif_uninit(struct optee *optee)
 {
+	if (optee->notif.ctx) {
+		optee_stop_async_notif(optee->notif.ctx);
+		if (optee->notif.irq) {
+			free_irq(optee->notif.irq, optee);
+			irq_dispose_mapping(optee->notif.irq);
+		}
+
+		/*
+		 * The thread normally working with optee->notif.ctx was
+		 * stopped with free_irq() above.
+		 *
+		 * Note we're not using teedev_close_context() or
+		 * tee_client_close_context() since we have already called
+		 * tee_device_put() while initializing to avoid a circular
+		 * reference counting.
+		 */
+		teedev_close_context(optee->notif.ctx);
+	}
+
 	kfree(optee->notif.bitmap);
 }
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index e3d72d09c484..3e09c8386e46 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -293,6 +293,13 @@ struct optee_msg_arg {
  * [in] param[0].u.rmem.shm_ref		holds shared memory reference
  * [in] param[0].u.rmem.offs		0
  * [in] param[0].u.rmem.size		0
+ *
+ * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
+ * of a driver.
+ *
+ * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
+ * normal world unable to process asynchronous notifications. Typically
+ * used when the driver is shut down.
  */
 #define OPTEE_MSG_CMD_OPEN_SESSION	0
 #define OPTEE_MSG_CMD_INVOKE_COMMAND	1
@@ -300,6 +307,8 @@ struct optee_msg_arg {
 #define OPTEE_MSG_CMD_CANCEL		3
 #define OPTEE_MSG_CMD_REGISTER_SHM	4
 #define OPTEE_MSG_CMD_UNREGISTER_SHM	5
+#define OPTEE_MSG_CMD_DO_BOTTOM_HALF	6
+#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF	7
 #define OPTEE_MSG_FUNCID_CALL_WITH_ARG	0x0004
 
 #endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 76a16d9b6cf4..f62a16935c49 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -37,6 +37,8 @@ struct optee_call_queue {
 
 struct optee_notif {
 	u_int max_key;
+	unsigned int irq;
+	struct tee_context *ctx;
 	/* Serializes access to the elements below in this struct */
 	spinlock_t lock;
 	struct list_head db;
@@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
 		      struct optee_call_ctx *call_ctx);
 void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
 
-int optee_notif_init(struct optee *optee, u_int max_key);
+int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
 void optee_notif_uninit(struct optee *optee);
 int optee_notif_wait(struct optee *optee, u_int key);
 int optee_notif_send(struct optee *optee, u_int key);
@@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
 int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 		      struct tee_param *param);
 int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
+int optee_do_bottom_half(struct tee_context *ctx);
+int optee_stop_async_notif(struct tee_context *ctx);
 
 void optee_enable_shm_cache(struct optee *optee);
 void optee_disable_shm_cache(struct optee *optee);
diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 80eb763a8a80..c6eec6b6febf 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
 /*
  * Call with struct optee_msg_arg as argument
  *
+ * When calling this function normal world has a few responsibilities:
+ * 1. It must be able to handle eventual RPCs
+ * 2. Non-secure interrupts should not be masked
+ * 3. If asynchronous notifications has be negotiated successfully, then
+ *    asynchronous notifications should be unmasked during this call.
+ *
  * Call register usage:
  * a0	SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
  * a1	Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
@@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
  * Normal return register usage:
  * a0	OPTEE_SMC_RETURN_OK
  * a1	bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
- * a2-7	Preserved
+ * a2	The maximum secure world notification number
+ * a3-7	Preserved
  *
  * Error return register usage:
  * a0	OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
@@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
 #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION	BIT(3)
 /* Secure world supports Shared Memory with a NULL reference */
 #define OPTEE_SMC_SEC_CAP_MEMREF_NULL		BIT(4)
+/* Secure world supports asynchronous notification of normal world */
+#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
 
 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
 struct optee_smc_exchange_capabilities_result {
 	unsigned long status;
 	unsigned long capabilities;
+	unsigned long max_notif_value;
 	unsigned long reserved0;
-	unsigned long reserved1;
 };
 
 /*
@@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
 #define OPTEE_SMC_GET_THREAD_COUNT \
 	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
 
+/*
+ * Inform OP-TEE that normal world is able to receive asynchronous
+ * notifications.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
+ * a1-6	Not used
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF	16
+#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
+
+/*
+ * Retrieve a value of notifications pended since the last call of this
+ * function.
+ *
+ * OP-TEE keeps a records of all posted values. When an interrupts is
+ * received which indicates that there are posed values this function
+ * should be called until all pended values has been retrieved. When a
+ * value is retrieved it's cleared from the record in secure world.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
+ * a1-6	Not used
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1	value
+ * a2	Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
+ *		valid, else 0 if no values where pending
+ * a2	Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
+ *		pending, else 0.
+ *	Bit[31:2]: MBZ
+ * a3-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID	BIT(0)
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING	BIT(1)
+
+/*
+ * Notification that OP-TEE expects a yielding call to do some bottom half
+ * work in a driver.
+ */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF	0
+
+#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE	17
+#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
+
 /*
  * Resume from RPC (for example after processing a foreign interrupt)
  *
-- 
2.31.1

[PATCH v6 6/6] optee: add asynchronous notifications

[Jens Wiklander]

Adds support for asynchronous notifications from secure world to normal
world. This allows a design with a top half and bottom half type of
driver where the top half runs in secure interrupt context and a
notifications tells normal world to schedule a yielding call to do the
bottom half processing.

The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.

A notification consists of a 32-bit value which normal world can
retrieve using a fastcall into secure world. The value
OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
When this value is sent it means that normal world is supposed to make a
yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.

Notification capability is negotiated while the driver is initialized.
If both sides supports these notifications then they are enabled.

An interrupt is used to notify the driver that there are asynchronous
notifications pending. The maximum needed notification value is
communicated at this stage. This allows scaling up when needed.

Acked-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/call.c          |  27 ++++++++
 drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
 drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
 drivers/tee/optee/optee_msg.h     |   9 +++
 drivers/tee/optee/optee_private.h |   6 +-
 drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
 6 files changed, 277 insertions(+), 32 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 945f03da0223..e428e0e9a3af 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 	return 0;
 }
 
+static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
+{
+	struct optee_msg_arg *msg_arg;
+	phys_addr_t msg_parg;
+	struct tee_shm *shm;
+
+	shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
+	if (IS_ERR(shm))
+		return PTR_ERR(shm);
+
+	msg_arg->cmd = cmd;
+	optee_do_call_with_arg(ctx, msg_parg);
+
+	tee_shm_free(shm);
+	return 0;
+}
+
+int optee_do_bottom_half(struct tee_context *ctx)
+{
+	return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
+}
+
+int optee_stop_async_notif(struct tee_context *ctx)
+{
+	return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);
+}
+
 /**
  * optee_enable_shm_cache() - Enables caching of some shared memory allocation
  *			      in OP-TEE
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 8531184f98f4..ccafd7151b45 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -8,9 +8,12 @@
 #include <linux/arm-smccc.h>
 #include <linux/crash_dump.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
 	.flags = TEE_DESC_PRIVILEGED,
 };
 
+static int enable_async_notif(optee_invoke_fn *invoke_fn)
+{
+	struct arm_smccc_res res;
+
+	invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0)
+		return -EINVAL;
+	return 0;
+}
+
 static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
 {
 	struct arm_smccc_res res;
@@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
 }
 
 static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
-					    u32 *sec_caps)
+					    u32 *sec_caps, u32 *max_notif_value)
 {
 	union {
 		struct arm_smccc_res smccc;
@@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
 		return false;
 
 	*sec_caps = res.result.capabilities;
+	*max_notif_value = res.result.max_notif_value;
 	return true;
 }
 
@@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
 	struct optee *optee = NULL;
 	void *memremaped_shm = NULL;
 	struct tee_device *teedev;
+	u32 max_notif_value;
 	u32 sec_caps;
 	int rc;
 
@@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
+	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
+					     &max_notif_value)) {
 		pr_warn("capabilities mismatch\n");
 		return -EINVAL;
 	}
@@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
 	if (!optee) {
 		rc = -ENOMEM;
-		goto err;
+		goto err_free_pool;
 	}
 
 	optee->invoke_fn = invoke_fn;
@@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
 	teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
-		goto err;
+		goto err_free_optee;
 	}
 	optee->teedev = teedev;
 
 	teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
-		goto err;
+		goto err_unreg_teedev;
 	}
 	optee->supp_teedev = teedev;
 
 	rc = tee_device_register(optee->teedev);
 	if (rc)
-		goto err;
+		goto err_unreg_supp_teedev;
 
 	rc = tee_device_register(optee->supp_teedev);
 	if (rc)
-		goto err;
+		goto err_unreg_supp_teedev;
 
 	mutex_init(&optee->call_queue.mutex);
 	INIT_LIST_HEAD(&optee->call_queue.waiters);
@@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
 	optee->pool = pool;
 
 	platform_set_drvdata(pdev, optee);
-	rc = optee_notif_init(optee, 255);
-	if (rc) {
-		optee_remove(pdev);
-		return rc;
+
+	if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
+		unsigned int irq;
+
+		rc = platform_get_irq(pdev, 0);
+		if (rc < 0) {
+			pr_err("platform_get_irq: ret %d\n", rc);
+			goto err_unreg_supp_teedev;
+		}
+		irq = rc;
+
+		rc = optee_notif_init(optee, max_notif_value, irq);
+		if (rc) {
+			irq_dispose_mapping(irq);
+			optee_remove(pdev);
+			return rc;
+		}
+		enable_async_notif(optee->invoke_fn);
+		pr_info("Asynchronous notifications enabled\n");
+	} else {
+		rc = optee_notif_init(optee, 255, 0);
+		if (rc) {
+			optee_remove(pdev);
+			return rc;
+		}
 	}
 
 	/*
@@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
 
 	pr_info("initialized driver\n");
 	return 0;
-err:
-	if (optee) {
-		/*
-		 * tee_device_unregister() is safe to call even if the
-		 * devices hasn't been registered with
-		 * tee_device_register() yet.
-		 */
-		tee_device_unregister(optee->supp_teedev);
-		tee_device_unregister(optee->teedev);
-		kfree(optee);
-	}
-	if (pool)
-		tee_shm_pool_free(pool);
-	if (memremaped_shm)
+err_unreg_supp_teedev:
+	tee_device_unregister(optee->supp_teedev);
+err_unreg_teedev:
+	tee_device_unregister(optee->teedev);
+err_free_optee:
+	kfree(optee);
+err_free_pool:
+	tee_shm_pool_free(pool);
+	if (optee->memremaped_shm)
 		memunmap(memremaped_shm);
 	return rc;
 }
diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
index a28fa03dcd0e..2c888ad87451 100644
--- a/drivers/tee/optee/notif.c
+++ b/drivers/tee/optee/notif.c
@@ -7,10 +7,14 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/tee_drv.h>
 #include "optee_private.h"
+#include "optee_smc.h"
+#include "optee_rpc_cmd.h"
 
 struct notif_entry {
 	struct list_head link;
@@ -18,6 +22,54 @@ struct notif_entry {
 	u_int key;
 };
 
+static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
+				 bool *value_pending)
+{
+	struct arm_smccc_res res;
+
+	invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
+
+	if (res.a0)
+		return 0;
+	*value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
+	*value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
+	return res.a1;
+}
+
+static irqreturn_t notif_irq_handler(int irq, void *dev_id)
+{
+	struct optee *optee = dev_id;
+	bool do_bottom_half = false;
+	bool value_valid;
+	bool value_pending;
+	u32 value;
+
+	do {
+		value = get_async_notif_value(optee->invoke_fn, &value_valid,
+					      &value_pending);
+		if (!value_valid)
+			break;
+
+		if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
+			do_bottom_half = true;
+		else
+			optee_notif_send(optee, value);
+	} while (value_pending);
+
+	if (do_bottom_half)
+		return IRQ_WAKE_THREAD;
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
+{
+	struct optee *optee = dev_id;
+
+	optee_do_bottom_half(optee->notif.ctx);
+
+	return IRQ_HANDLED;
+}
+
 static bool have_key(struct optee *optee, u_int key)
 {
 	struct notif_entry *entry;
@@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
 	return 0;
 }
 
-int optee_notif_init(struct optee *optee, u_int max_key)
+int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
 {
+	struct tee_context *ctx;
+	int rc;
+
+	if (irq) {
+		ctx = teedev_open(optee->teedev);
+		if (IS_ERR(ctx))
+			return PTR_ERR(ctx);
+
+		optee->notif.ctx = ctx;
+	}
+
 	spin_lock_init(&optee->notif.lock);
 	INIT_LIST_HEAD(&optee->notif.db);
 	optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
-	if (!optee->notif.bitmap)
-		return -ENOMEM;
-
+	if (!optee->notif.bitmap) {
+		rc = -ENOMEM;
+		goto err_close_ctx;
+	}
 	optee->notif.max_key = max_key;
 
+	if (irq) {
+		rc = request_threaded_irq(irq, notif_irq_handler,
+					  notif_irq_thread_fn,
+					  0, "optee_notification", optee);
+		if (rc)
+			goto err_free_bitmap;
+
+		optee->notif.irq = irq;
+	}
+
 	return 0;
+
+err_free_bitmap:
+	kfree(optee->notif.bitmap);
+err_close_ctx:
+	teedev_close_context(optee->notif.ctx);
+	optee->notif.ctx = NULL;
+
+	return rc;
 }
 
 void optee_notif_uninit(struct optee *optee)
 {
+	if (optee->notif.ctx) {
+		optee_stop_async_notif(optee->notif.ctx);
+		if (optee->notif.irq) {
+			free_irq(optee->notif.irq, optee);
+			irq_dispose_mapping(optee->notif.irq);
+		}
+
+		/*
+		 * The thread normally working with optee->notif.ctx was
+		 * stopped with free_irq() above.
+		 *
+		 * Note we're not using teedev_close_context() or
+		 * tee_client_close_context() since we have already called
+		 * tee_device_put() while initializing to avoid a circular
+		 * reference counting.
+		 */
+		teedev_close_context(optee->notif.ctx);
+	}
+
 	kfree(optee->notif.bitmap);
 }
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index e3d72d09c484..3e09c8386e46 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -293,6 +293,13 @@ struct optee_msg_arg {
  * [in] param[0].u.rmem.shm_ref		holds shared memory reference
  * [in] param[0].u.rmem.offs		0
  * [in] param[0].u.rmem.size		0
+ *
+ * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
+ * of a driver.
+ *
+ * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
+ * normal world unable to process asynchronous notifications. Typically
+ * used when the driver is shut down.
  */
 #define OPTEE_MSG_CMD_OPEN_SESSION	0
 #define OPTEE_MSG_CMD_INVOKE_COMMAND	1
@@ -300,6 +307,8 @@ struct optee_msg_arg {
 #define OPTEE_MSG_CMD_CANCEL		3
 #define OPTEE_MSG_CMD_REGISTER_SHM	4
 #define OPTEE_MSG_CMD_UNREGISTER_SHM	5
+#define OPTEE_MSG_CMD_DO_BOTTOM_HALF	6
+#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF	7
 #define OPTEE_MSG_FUNCID_CALL_WITH_ARG	0x0004
 
 #endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 76a16d9b6cf4..f62a16935c49 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -37,6 +37,8 @@ struct optee_call_queue {
 
 struct optee_notif {
 	u_int max_key;
+	unsigned int irq;
+	struct tee_context *ctx;
 	/* Serializes access to the elements below in this struct */
 	spinlock_t lock;
 	struct list_head db;
@@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
 		      struct optee_call_ctx *call_ctx);
 void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
 
-int optee_notif_init(struct optee *optee, u_int max_key);
+int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
 void optee_notif_uninit(struct optee *optee);
 int optee_notif_wait(struct optee *optee, u_int key);
 int optee_notif_send(struct optee *optee, u_int key);
@@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
 int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 		      struct tee_param *param);
 int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
+int optee_do_bottom_half(struct tee_context *ctx);
+int optee_stop_async_notif(struct tee_context *ctx);
 
 void optee_enable_shm_cache(struct optee *optee);
 void optee_disable_shm_cache(struct optee *optee);
diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 80eb763a8a80..c6eec6b6febf 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
 /*
  * Call with struct optee_msg_arg as argument
  *
+ * When calling this function normal world has a few responsibilities:
+ * 1. It must be able to handle eventual RPCs
+ * 2. Non-secure interrupts should not be masked
+ * 3. If asynchronous notifications has be negotiated successfully, then
+ *    asynchronous notifications should be unmasked during this call.
+ *
  * Call register usage:
  * a0	SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
  * a1	Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
@@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
  * Normal return register usage:
  * a0	OPTEE_SMC_RETURN_OK
  * a1	bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
- * a2-7	Preserved
+ * a2	The maximum secure world notification number
+ * a3-7	Preserved
  *
  * Error return register usage:
  * a0	OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
@@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
 #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION	BIT(3)
 /* Secure world supports Shared Memory with a NULL reference */
 #define OPTEE_SMC_SEC_CAP_MEMREF_NULL		BIT(4)
+/* Secure world supports asynchronous notification of normal world */
+#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
 
 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
 struct optee_smc_exchange_capabilities_result {
 	unsigned long status;
 	unsigned long capabilities;
+	unsigned long max_notif_value;
 	unsigned long reserved0;
-	unsigned long reserved1;
 };
 
 /*
@@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
 #define OPTEE_SMC_GET_THREAD_COUNT \
 	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
 
+/*
+ * Inform OP-TEE that normal world is able to receive asynchronous
+ * notifications.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
+ * a1-6	Not used
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF	16
+#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
+
+/*
+ * Retrieve a value of notifications pended since the last call of this
+ * function.
+ *
+ * OP-TEE keeps a records of all posted values. When an interrupts is
+ * received which indicates that there are posed values this function
+ * should be called until all pended values has been retrieved. When a
+ * value is retrieved it's cleared from the record in secure world.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
+ * a1-6	Not used
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1	value
+ * a2	Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
+ *		valid, else 0 if no values where pending
+ * a2	Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
+ *		pending, else 0.
+ *	Bit[31:2]: MBZ
+ * a3-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID	BIT(0)
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING	BIT(1)
+
+/*
+ * Notification that OP-TEE expects a yielding call to do some bottom half
+ * work in a driver.
+ */
+#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF	0
+
+#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE	17
+#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
+
 /*
  * Resume from RPC (for example after processing a foreign interrupt)
  *
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 6/6] optee: add asynchronous notifications

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds support for asynchronous notifications from secure world to normal
> world. This allows a design with a top half and bottom half type of
> driver where the top half runs in secure interrupt context and a
> notifications tells normal world to schedule a yielding call to do the
> bottom half processing.
>
> The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.
>
> A notification consists of a 32-bit value which normal world can
> retrieve using a fastcall into secure world. The value
> OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
> When this value is sent it means that normal world is supposed to make a
> yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.
>
> Notification capability is negotiated while the driver is initialized.
> If both sides supports these notifications then they are enabled.
>
> An interrupt is used to notify the driver that there are asynchronous
> notifications pending. The maximum needed notification value is
> communicated at this stage. This allows scaling up when needed.
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/call.c          |  27 ++++++++
>  drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
>  drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
>  drivers/tee/optee/optee_msg.h     |   9 +++
>  drivers/tee/optee/optee_private.h |   6 +-
>  drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
>  6 files changed, 277 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 945f03da0223..e428e0e9a3af 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
>         return 0;
>  }
>
> +static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
> +{
> +       struct optee_msg_arg *msg_arg;
> +       phys_addr_t msg_parg;
> +       struct tee_shm *shm;
> +
> +       shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
> +       if (IS_ERR(shm))
> +               return PTR_ERR(shm);
> +
> +       msg_arg->cmd = cmd;
> +       optee_do_call_with_arg(ctx, msg_parg);
> +
> +       tee_shm_free(shm);
> +       return 0;
> +}
> +
> +int optee_do_bottom_half(struct tee_context *ctx)
> +{
> +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
> +}
> +
> +int optee_stop_async_notif(struct tee_context *ctx)
> +{
> +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);

Is there any particular reason that this isn't a fast call similar to
OPTEE_SMC_ENABLE_ASYNC_NOTIF?

> +}
> +
>  /**
>   * optee_enable_shm_cache() - Enables caching of some shared memory allocation
>   *                           in OP-TEE
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 8531184f98f4..ccafd7151b45 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -8,9 +8,12 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/crash_dump.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
>         .flags = TEE_DESC_PRIVILEGED,
>  };
>
> +static int enable_async_notif(optee_invoke_fn *invoke_fn)
> +{
> +       struct arm_smccc_res res;
> +
> +       invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +       if (res.a0)
> +               return -EINVAL;
> +       return 0;
> +}
> +
>  static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
>  {
>         struct arm_smccc_res res;
> @@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
>  }
>
>  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> -                                           u32 *sec_caps)
> +                                           u32 *sec_caps, u32 *max_notif_value)
>  {
>         union {
>                 struct arm_smccc_res smccc;
> @@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>                 return false;
>
>         *sec_caps = res.result.capabilities;
> +       *max_notif_value = res.result.max_notif_value;
>         return true;
>  }
>
> @@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
>         struct optee *optee = NULL;
>         void *memremaped_shm = NULL;
>         struct tee_device *teedev;
> +       u32 max_notif_value;
>         u32 sec_caps;
>         int rc;
>
> @@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
> +       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> +                                            &max_notif_value)) {
>                 pr_warn("capabilities mismatch\n");
>                 return -EINVAL;
>         }
> @@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
>         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
>         if (!optee) {
>                 rc = -ENOMEM;
> -               goto err;
> +               goto err_free_pool;
>         }
>
>         optee->invoke_fn = invoke_fn;
> @@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
>         teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
>         if (IS_ERR(teedev)) {
>                 rc = PTR_ERR(teedev);
> -               goto err;
> +               goto err_free_optee;
>         }
>         optee->teedev = teedev;
>
>         teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
>         if (IS_ERR(teedev)) {
>                 rc = PTR_ERR(teedev);
> -               goto err;
> +               goto err_unreg_teedev;
>         }
>         optee->supp_teedev = teedev;
>
>         rc = tee_device_register(optee->teedev);
>         if (rc)
> -               goto err;
> +               goto err_unreg_supp_teedev;
>
>         rc = tee_device_register(optee->supp_teedev);
>         if (rc)
> -               goto err;
> +               goto err_unreg_supp_teedev;
>
>         mutex_init(&optee->call_queue.mutex);
>         INIT_LIST_HEAD(&optee->call_queue.waiters);
> @@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
>         optee->pool = pool;
>
>         platform_set_drvdata(pdev, optee);
> -       rc = optee_notif_init(optee, 255);
> -       if (rc) {
> -               optee_remove(pdev);
> -               return rc;
> +
> +       if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> +               unsigned int irq;
> +
> +               rc = platform_get_irq(pdev, 0);
> +               if (rc < 0) {
> +                       pr_err("platform_get_irq: ret %d\n", rc);
> +                       goto err_unreg_supp_teedev;
> +               }
> +               irq = rc;
> +
> +               rc = optee_notif_init(optee, max_notif_value, irq);
> +               if (rc) {
> +                       irq_dispose_mapping(irq);
> +                       optee_remove(pdev);
> +                       return rc;
> +               }
> +               enable_async_notif(optee->invoke_fn);
> +               pr_info("Asynchronous notifications enabled\n");
> +       } else {
> +               rc = optee_notif_init(optee, 255, 0);
> +               if (rc) {
> +                       optee_remove(pdev);

IMO, multiple usage of optee_remove() in the error path makes error
handling a bit complex. I liked the way you tagged the error paths. So
can't we get rid of optee_remove() from the error path and handle
errors based on tagged error paths only?

> +                       return rc;
> +               }
>         }
>
>         /*
> @@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
>
>         pr_info("initialized driver\n");
>         return 0;
> -err:
> -       if (optee) {
> -               /*
> -                * tee_device_unregister() is safe to call even if the
> -                * devices hasn't been registered with
> -                * tee_device_register() yet.
> -                */
> -               tee_device_unregister(optee->supp_teedev);
> -               tee_device_unregister(optee->teedev);
> -               kfree(optee);
> -       }
> -       if (pool)
> -               tee_shm_pool_free(pool);
> -       if (memremaped_shm)
> +err_unreg_supp_teedev:
> +       tee_device_unregister(optee->supp_teedev);
> +err_unreg_teedev:
> +       tee_device_unregister(optee->teedev);
> +err_free_optee:
> +       kfree(optee);
> +err_free_pool:
> +       tee_shm_pool_free(pool);
> +       if (optee->memremaped_shm)
>                 memunmap(memremaped_shm);
>         return rc;
>  }
> diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> index a28fa03dcd0e..2c888ad87451 100644
> --- a/drivers/tee/optee/notif.c
> +++ b/drivers/tee/optee/notif.c
> @@ -7,10 +7,14 @@
>
>  #include <linux/arm-smccc.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/tee_drv.h>
>  #include "optee_private.h"
> +#include "optee_smc.h"
> +#include "optee_rpc_cmd.h"
>
>  struct notif_entry {
>         struct list_head link;
> @@ -18,6 +22,54 @@ struct notif_entry {
>         u_int key;
>  };
>
> +static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> +                                bool *value_pending)
> +{
> +       struct arm_smccc_res res;
> +
> +       invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +       if (res.a0)
> +               return 0;
> +       *value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
> +       *value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
> +       return res.a1;
> +}
> +
> +static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> +{
> +       struct optee *optee = dev_id;
> +       bool do_bottom_half = false;
> +       bool value_valid;
> +       bool value_pending;
> +       u32 value;
> +
> +       do {
> +               value = get_async_notif_value(optee->invoke_fn, &value_valid,
> +                                             &value_pending);
> +               if (!value_valid)
> +                       break;
> +
> +               if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
> +                       do_bottom_half = true;
> +               else
> +                       optee_notif_send(optee, value);
> +       } while (value_pending);
> +
> +       if (do_bottom_half)
> +               return IRQ_WAKE_THREAD;
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> +{
> +       struct optee *optee = dev_id;
> +
> +       optee_do_bottom_half(optee->notif.ctx);
> +
> +       return IRQ_HANDLED;
> +}
> +
>  static bool have_key(struct optee *optee, u_int key)
>  {
>         struct notif_entry *entry;
> @@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
>         return 0;
>  }
>
> -int optee_notif_init(struct optee *optee, u_int max_key)
> +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
>  {
> +       struct tee_context *ctx;
> +       int rc;
> +
> +       if (irq) {
> +               ctx = teedev_open(optee->teedev);
> +               if (IS_ERR(ctx))
> +                       return PTR_ERR(ctx);
> +
> +               optee->notif.ctx = ctx;
> +       }
> +
>         spin_lock_init(&optee->notif.lock);
>         INIT_LIST_HEAD(&optee->notif.db);
>         optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> -       if (!optee->notif.bitmap)
> -               return -ENOMEM;
> -
> +       if (!optee->notif.bitmap) {
> +               rc = -ENOMEM;
> +               goto err_close_ctx;
> +       }
>         optee->notif.max_key = max_key;
>
> +       if (irq) {
> +               rc = request_threaded_irq(irq, notif_irq_handler,
> +                                         notif_irq_thread_fn,
> +                                         0, "optee_notification", optee);
> +               if (rc)
> +                       goto err_free_bitmap;
> +
> +               optee->notif.irq = irq;
> +       }
> +
>         return 0;
> +
> +err_free_bitmap:
> +       kfree(optee->notif.bitmap);
> +err_close_ctx:
> +       teedev_close_context(optee->notif.ctx);
> +       optee->notif.ctx = NULL;
> +
> +       return rc;
>  }
>
>  void optee_notif_uninit(struct optee *optee)
>  {
> +       if (optee->notif.ctx) {
> +               optee_stop_async_notif(optee->notif.ctx);
> +               if (optee->notif.irq) {
> +                       free_irq(optee->notif.irq, optee);
> +                       irq_dispose_mapping(optee->notif.irq);
> +               }
> +
> +               /*
> +                * The thread normally working with optee->notif.ctx was
> +                * stopped with free_irq() above.
> +                *
> +                * Note we're not using teedev_close_context() or
> +                * tee_client_close_context() since we have already called
> +                * tee_device_put() while initializing to avoid a circular
> +                * reference counting.
> +                */
> +               teedev_close_context(optee->notif.ctx);
> +       }
> +
>         kfree(optee->notif.bitmap);
>  }
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index e3d72d09c484..3e09c8386e46 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -293,6 +293,13 @@ struct optee_msg_arg {
>   * [in] param[0].u.rmem.shm_ref                holds shared memory reference
>   * [in] param[0].u.rmem.offs           0
>   * [in] param[0].u.rmem.size           0
> + *
> + * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
> + * of a driver.
> + *
> + * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
> + * normal world unable to process asynchronous notifications. Typically
> + * used when the driver is shut down.
>   */
>  #define OPTEE_MSG_CMD_OPEN_SESSION     0
>  #define OPTEE_MSG_CMD_INVOKE_COMMAND   1
> @@ -300,6 +307,8 @@ struct optee_msg_arg {
>  #define OPTEE_MSG_CMD_CANCEL           3
>  #define OPTEE_MSG_CMD_REGISTER_SHM     4
>  #define OPTEE_MSG_CMD_UNREGISTER_SHM   5
> +#define OPTEE_MSG_CMD_DO_BOTTOM_HALF   6
> +#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7
>  #define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004
>
>  #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 76a16d9b6cf4..f62a16935c49 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -37,6 +37,8 @@ struct optee_call_queue {
>
>  struct optee_notif {
>         u_int max_key;
> +       unsigned int irq;
> +       struct tee_context *ctx;
>         /* Serializes access to the elements below in this struct */
>         spinlock_t lock;
>         struct list_head db;
> @@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>                       struct optee_call_ctx *call_ctx);
>  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
>
> -int optee_notif_init(struct optee *optee, u_int max_key);
> +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
>  void optee_notif_uninit(struct optee *optee);
>  int optee_notif_wait(struct optee *optee, u_int key);
>  int optee_notif_send(struct optee *optee, u_int key);
> @@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
>  int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
>                       struct tee_param *param);
>  int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> +int optee_do_bottom_half(struct tee_context *ctx);
> +int optee_stop_async_notif(struct tee_context *ctx);
>
>  void optee_enable_shm_cache(struct optee *optee);
>  void optee_disable_shm_cache(struct optee *optee);
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 80eb763a8a80..c6eec6b6febf 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
>  /*
>   * Call with struct optee_msg_arg as argument
>   *
> + * When calling this function normal world has a few responsibilities:
> + * 1. It must be able to handle eventual RPCs
> + * 2. Non-secure interrupts should not be masked
> + * 3. If asynchronous notifications has be negotiated successfully, then

nit: s/has be/have been/

> + *    asynchronous notifications should be unmasked during this call.
> + *
>   * Call register usage:
>   * a0  SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
>   * a1  Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
> @@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
>   * Normal return register usage:
>   * a0  OPTEE_SMC_RETURN_OK
>   * a1  bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
> - * a2-7        Preserved
> + * a2  The maximum secure world notification number
> + * a3-7        Preserved
>   *
>   * Error return register usage:
>   * a0  OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
> @@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION       BIT(3)
>  /* Secure world supports Shared Memory with a NULL reference */
>  #define OPTEE_SMC_SEC_CAP_MEMREF_NULL          BIT(4)
> +/* Secure world supports asynchronous notification of normal world */
> +#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
>  struct optee_smc_exchange_capabilities_result {
>         unsigned long status;
>         unsigned long capabilities;
> +       unsigned long max_notif_value;
>         unsigned long reserved0;
> -       unsigned long reserved1;
>  };
>
>  /*
> @@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
>  #define OPTEE_SMC_GET_THREAD_COUNT \
>         OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
>
> +/*
> + * Inform OP-TEE that normal world is able to receive asynchronous
> + * notifications.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
> + * a1-6        Not used
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF    16
> +#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
> +
> +/*
> + * Retrieve a value of notifications pended since the last call of this

nit: s/pended/pending/

> + * function.
> + *
> + * OP-TEE keeps a records of all posted values. When an interrupts is

nit: s/records/record/

> + * received which indicates that there are posed values this function
> + * should be called until all pended values has been retrieved. When a

nit: s/has/have/

> + * value is retrieved it's cleared from the record in secure world.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
> + * a1-6        Not used
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1  value
> + * a2  Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
> + *             valid, else 0 if no values where pending
> + * a2  Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
> + *             pending, else 0.
> + *     Bit[31:2]: MBZ
> + * a3-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID      BIT(0)
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING    BIT(1)
> +
> +/*
> + * Notification that OP-TEE expects a yielding call to do some bottom half
> + * work in a driver.
> + */
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF     0
> +
> +#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17
> +#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *

Apart from comments above, this patch looks good to me in general.

-Sumit

> --
> 2.31.1
>

Re: [PATCH v6 6/6] optee: add asynchronous notifications

[Sumit Garg]

On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds support for asynchronous notifications from secure world to normal
> world. This allows a design with a top half and bottom half type of
> driver where the top half runs in secure interrupt context and a
> notifications tells normal world to schedule a yielding call to do the
> bottom half processing.
>
> The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.
>
> A notification consists of a 32-bit value which normal world can
> retrieve using a fastcall into secure world. The value
> OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
> When this value is sent it means that normal world is supposed to make a
> yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.
>
> Notification capability is negotiated while the driver is initialized.
> If both sides supports these notifications then they are enabled.
>
> An interrupt is used to notify the driver that there are asynchronous
> notifications pending. The maximum needed notification value is
> communicated at this stage. This allows scaling up when needed.
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/call.c          |  27 ++++++++
>  drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
>  drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
>  drivers/tee/optee/optee_msg.h     |   9 +++
>  drivers/tee/optee/optee_private.h |   6 +-
>  drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
>  6 files changed, 277 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 945f03da0223..e428e0e9a3af 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
>         return 0;
>  }
>
> +static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
> +{
> +       struct optee_msg_arg *msg_arg;
> +       phys_addr_t msg_parg;
> +       struct tee_shm *shm;
> +
> +       shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
> +       if (IS_ERR(shm))
> +               return PTR_ERR(shm);
> +
> +       msg_arg->cmd = cmd;
> +       optee_do_call_with_arg(ctx, msg_parg);
> +
> +       tee_shm_free(shm);
> +       return 0;
> +}
> +
> +int optee_do_bottom_half(struct tee_context *ctx)
> +{
> +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
> +}
> +
> +int optee_stop_async_notif(struct tee_context *ctx)
> +{
> +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);

Is there any particular reason that this isn't a fast call similar to
OPTEE_SMC_ENABLE_ASYNC_NOTIF?

> +}
> +
>  /**
>   * optee_enable_shm_cache() - Enables caching of some shared memory allocation
>   *                           in OP-TEE
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 8531184f98f4..ccafd7151b45 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -8,9 +8,12 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/crash_dump.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
>         .flags = TEE_DESC_PRIVILEGED,
>  };
>
> +static int enable_async_notif(optee_invoke_fn *invoke_fn)
> +{
> +       struct arm_smccc_res res;
> +
> +       invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +       if (res.a0)
> +               return -EINVAL;
> +       return 0;
> +}
> +
>  static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
>  {
>         struct arm_smccc_res res;
> @@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
>  }
>
>  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> -                                           u32 *sec_caps)
> +                                           u32 *sec_caps, u32 *max_notif_value)
>  {
>         union {
>                 struct arm_smccc_res smccc;
> @@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>                 return false;
>
>         *sec_caps = res.result.capabilities;
> +       *max_notif_value = res.result.max_notif_value;
>         return true;
>  }
>
> @@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
>         struct optee *optee = NULL;
>         void *memremaped_shm = NULL;
>         struct tee_device *teedev;
> +       u32 max_notif_value;
>         u32 sec_caps;
>         int rc;
>
> @@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
> +       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> +                                            &max_notif_value)) {
>                 pr_warn("capabilities mismatch\n");
>                 return -EINVAL;
>         }
> @@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
>         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
>         if (!optee) {
>                 rc = -ENOMEM;
> -               goto err;
> +               goto err_free_pool;
>         }
>
>         optee->invoke_fn = invoke_fn;
> @@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
>         teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
>         if (IS_ERR(teedev)) {
>                 rc = PTR_ERR(teedev);
> -               goto err;
> +               goto err_free_optee;
>         }
>         optee->teedev = teedev;
>
>         teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
>         if (IS_ERR(teedev)) {
>                 rc = PTR_ERR(teedev);
> -               goto err;
> +               goto err_unreg_teedev;
>         }
>         optee->supp_teedev = teedev;
>
>         rc = tee_device_register(optee->teedev);
>         if (rc)
> -               goto err;
> +               goto err_unreg_supp_teedev;
>
>         rc = tee_device_register(optee->supp_teedev);
>         if (rc)
> -               goto err;
> +               goto err_unreg_supp_teedev;
>
>         mutex_init(&optee->call_queue.mutex);
>         INIT_LIST_HEAD(&optee->call_queue.waiters);
> @@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
>         optee->pool = pool;
>
>         platform_set_drvdata(pdev, optee);
> -       rc = optee_notif_init(optee, 255);
> -       if (rc) {
> -               optee_remove(pdev);
> -               return rc;
> +
> +       if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> +               unsigned int irq;
> +
> +               rc = platform_get_irq(pdev, 0);
> +               if (rc < 0) {
> +                       pr_err("platform_get_irq: ret %d\n", rc);
> +                       goto err_unreg_supp_teedev;
> +               }
> +               irq = rc;
> +
> +               rc = optee_notif_init(optee, max_notif_value, irq);
> +               if (rc) {
> +                       irq_dispose_mapping(irq);
> +                       optee_remove(pdev);
> +                       return rc;
> +               }
> +               enable_async_notif(optee->invoke_fn);
> +               pr_info("Asynchronous notifications enabled\n");
> +       } else {
> +               rc = optee_notif_init(optee, 255, 0);
> +               if (rc) {
> +                       optee_remove(pdev);

IMO, multiple usage of optee_remove() in the error path makes error
handling a bit complex. I liked the way you tagged the error paths. So
can't we get rid of optee_remove() from the error path and handle
errors based on tagged error paths only?

> +                       return rc;
> +               }
>         }
>
>         /*
> @@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
>
>         pr_info("initialized driver\n");
>         return 0;
> -err:
> -       if (optee) {
> -               /*
> -                * tee_device_unregister() is safe to call even if the
> -                * devices hasn't been registered with
> -                * tee_device_register() yet.
> -                */
> -               tee_device_unregister(optee->supp_teedev);
> -               tee_device_unregister(optee->teedev);
> -               kfree(optee);
> -       }
> -       if (pool)
> -               tee_shm_pool_free(pool);
> -       if (memremaped_shm)
> +err_unreg_supp_teedev:
> +       tee_device_unregister(optee->supp_teedev);
> +err_unreg_teedev:
> +       tee_device_unregister(optee->teedev);
> +err_free_optee:
> +       kfree(optee);
> +err_free_pool:
> +       tee_shm_pool_free(pool);
> +       if (optee->memremaped_shm)
>                 memunmap(memremaped_shm);
>         return rc;
>  }
> diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> index a28fa03dcd0e..2c888ad87451 100644
> --- a/drivers/tee/optee/notif.c
> +++ b/drivers/tee/optee/notif.c
> @@ -7,10 +7,14 @@
>
>  #include <linux/arm-smccc.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/tee_drv.h>
>  #include "optee_private.h"
> +#include "optee_smc.h"
> +#include "optee_rpc_cmd.h"
>
>  struct notif_entry {
>         struct list_head link;
> @@ -18,6 +22,54 @@ struct notif_entry {
>         u_int key;
>  };
>
> +static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> +                                bool *value_pending)
> +{
> +       struct arm_smccc_res res;
> +
> +       invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +       if (res.a0)
> +               return 0;
> +       *value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
> +       *value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
> +       return res.a1;
> +}
> +
> +static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> +{
> +       struct optee *optee = dev_id;
> +       bool do_bottom_half = false;
> +       bool value_valid;
> +       bool value_pending;
> +       u32 value;
> +
> +       do {
> +               value = get_async_notif_value(optee->invoke_fn, &value_valid,
> +                                             &value_pending);
> +               if (!value_valid)
> +                       break;
> +
> +               if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
> +                       do_bottom_half = true;
> +               else
> +                       optee_notif_send(optee, value);
> +       } while (value_pending);
> +
> +       if (do_bottom_half)
> +               return IRQ_WAKE_THREAD;
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> +{
> +       struct optee *optee = dev_id;
> +
> +       optee_do_bottom_half(optee->notif.ctx);
> +
> +       return IRQ_HANDLED;
> +}
> +
>  static bool have_key(struct optee *optee, u_int key)
>  {
>         struct notif_entry *entry;
> @@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
>         return 0;
>  }
>
> -int optee_notif_init(struct optee *optee, u_int max_key)
> +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
>  {
> +       struct tee_context *ctx;
> +       int rc;
> +
> +       if (irq) {
> +               ctx = teedev_open(optee->teedev);
> +               if (IS_ERR(ctx))
> +                       return PTR_ERR(ctx);
> +
> +               optee->notif.ctx = ctx;
> +       }
> +
>         spin_lock_init(&optee->notif.lock);
>         INIT_LIST_HEAD(&optee->notif.db);
>         optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> -       if (!optee->notif.bitmap)
> -               return -ENOMEM;
> -
> +       if (!optee->notif.bitmap) {
> +               rc = -ENOMEM;
> +               goto err_close_ctx;
> +       }
>         optee->notif.max_key = max_key;
>
> +       if (irq) {
> +               rc = request_threaded_irq(irq, notif_irq_handler,
> +                                         notif_irq_thread_fn,
> +                                         0, "optee_notification", optee);
> +               if (rc)
> +                       goto err_free_bitmap;
> +
> +               optee->notif.irq = irq;
> +       }
> +
>         return 0;
> +
> +err_free_bitmap:
> +       kfree(optee->notif.bitmap);
> +err_close_ctx:
> +       teedev_close_context(optee->notif.ctx);
> +       optee->notif.ctx = NULL;
> +
> +       return rc;
>  }
>
>  void optee_notif_uninit(struct optee *optee)
>  {
> +       if (optee->notif.ctx) {
> +               optee_stop_async_notif(optee->notif.ctx);
> +               if (optee->notif.irq) {
> +                       free_irq(optee->notif.irq, optee);
> +                       irq_dispose_mapping(optee->notif.irq);
> +               }
> +
> +               /*
> +                * The thread normally working with optee->notif.ctx was
> +                * stopped with free_irq() above.
> +                *
> +                * Note we're not using teedev_close_context() or
> +                * tee_client_close_context() since we have already called
> +                * tee_device_put() while initializing to avoid a circular
> +                * reference counting.
> +                */
> +               teedev_close_context(optee->notif.ctx);
> +       }
> +
>         kfree(optee->notif.bitmap);
>  }
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index e3d72d09c484..3e09c8386e46 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -293,6 +293,13 @@ struct optee_msg_arg {
>   * [in] param[0].u.rmem.shm_ref                holds shared memory reference
>   * [in] param[0].u.rmem.offs           0
>   * [in] param[0].u.rmem.size           0
> + *
> + * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
> + * of a driver.
> + *
> + * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
> + * normal world unable to process asynchronous notifications. Typically
> + * used when the driver is shut down.
>   */
>  #define OPTEE_MSG_CMD_OPEN_SESSION     0
>  #define OPTEE_MSG_CMD_INVOKE_COMMAND   1
> @@ -300,6 +307,8 @@ struct optee_msg_arg {
>  #define OPTEE_MSG_CMD_CANCEL           3
>  #define OPTEE_MSG_CMD_REGISTER_SHM     4
>  #define OPTEE_MSG_CMD_UNREGISTER_SHM   5
> +#define OPTEE_MSG_CMD_DO_BOTTOM_HALF   6
> +#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7
>  #define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004
>
>  #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 76a16d9b6cf4..f62a16935c49 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -37,6 +37,8 @@ struct optee_call_queue {
>
>  struct optee_notif {
>         u_int max_key;
> +       unsigned int irq;
> +       struct tee_context *ctx;
>         /* Serializes access to the elements below in this struct */
>         spinlock_t lock;
>         struct list_head db;
> @@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>                       struct optee_call_ctx *call_ctx);
>  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
>
> -int optee_notif_init(struct optee *optee, u_int max_key);
> +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
>  void optee_notif_uninit(struct optee *optee);
>  int optee_notif_wait(struct optee *optee, u_int key);
>  int optee_notif_send(struct optee *optee, u_int key);
> @@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
>  int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
>                       struct tee_param *param);
>  int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> +int optee_do_bottom_half(struct tee_context *ctx);
> +int optee_stop_async_notif(struct tee_context *ctx);
>
>  void optee_enable_shm_cache(struct optee *optee);
>  void optee_disable_shm_cache(struct optee *optee);
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 80eb763a8a80..c6eec6b6febf 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
>  /*
>   * Call with struct optee_msg_arg as argument
>   *
> + * When calling this function normal world has a few responsibilities:
> + * 1. It must be able to handle eventual RPCs
> + * 2. Non-secure interrupts should not be masked
> + * 3. If asynchronous notifications has be negotiated successfully, then

nit: s/has be/have been/

> + *    asynchronous notifications should be unmasked during this call.
> + *
>   * Call register usage:
>   * a0  SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
>   * a1  Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
> @@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
>   * Normal return register usage:
>   * a0  OPTEE_SMC_RETURN_OK
>   * a1  bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
> - * a2-7        Preserved
> + * a2  The maximum secure world notification number
> + * a3-7        Preserved
>   *
>   * Error return register usage:
>   * a0  OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
> @@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION       BIT(3)
>  /* Secure world supports Shared Memory with a NULL reference */
>  #define OPTEE_SMC_SEC_CAP_MEMREF_NULL          BIT(4)
> +/* Secure world supports asynchronous notification of normal world */
> +#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
>  struct optee_smc_exchange_capabilities_result {
>         unsigned long status;
>         unsigned long capabilities;
> +       unsigned long max_notif_value;
>         unsigned long reserved0;
> -       unsigned long reserved1;
>  };
>
>  /*
> @@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
>  #define OPTEE_SMC_GET_THREAD_COUNT \
>         OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
>
> +/*
> + * Inform OP-TEE that normal world is able to receive asynchronous
> + * notifications.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
> + * a1-6        Not used
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF    16
> +#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
> +
> +/*
> + * Retrieve a value of notifications pended since the last call of this

nit: s/pended/pending/

> + * function.
> + *
> + * OP-TEE keeps a records of all posted values. When an interrupts is

nit: s/records/record/

> + * received which indicates that there are posed values this function
> + * should be called until all pended values has been retrieved. When a

nit: s/has/have/

> + * value is retrieved it's cleared from the record in secure world.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
> + * a1-6        Not used
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1  value
> + * a2  Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
> + *             valid, else 0 if no values where pending
> + * a2  Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
> + *             pending, else 0.
> + *     Bit[31:2]: MBZ
> + * a3-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID      BIT(0)
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING    BIT(1)
> +
> +/*
> + * Notification that OP-TEE expects a yielding call to do some bottom half
> + * work in a driver.
> + */
> +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF     0
> +
> +#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17
> +#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *

Apart from comments above, this patch looks good to me in general.

-Sumit

> --
> 2.31.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 6/6] optee: add asynchronous notifications

[Jens Wiklander]

On Wed, Oct 13, 2021 at 10:07 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Adds support for asynchronous notifications from secure world to normal
> > world. This allows a design with a top half and bottom half type of
> > driver where the top half runs in secure interrupt context and a
> > notifications tells normal world to schedule a yielding call to do the
> > bottom half processing.
> >
> > The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.
> >
> > A notification consists of a 32-bit value which normal world can
> > retrieve using a fastcall into secure world. The value
> > OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
> > When this value is sent it means that normal world is supposed to make a
> > yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.
> >
> > Notification capability is negotiated while the driver is initialized.
> > If both sides supports these notifications then they are enabled.
> >
> > An interrupt is used to notify the driver that there are asynchronous
> > notifications pending. The maximum needed notification value is
> > communicated at this stage. This allows scaling up when needed.
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/call.c          |  27 ++++++++
> >  drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
> >  drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
> >  drivers/tee/optee/optee_msg.h     |   9 +++
> >  drivers/tee/optee/optee_private.h |   6 +-
> >  drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
> >  6 files changed, 277 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 945f03da0223..e428e0e9a3af 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> >         return 0;
> >  }
> >
> > +static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
> > +{
> > +       struct optee_msg_arg *msg_arg;
> > +       phys_addr_t msg_parg;
> > +       struct tee_shm *shm;
> > +
> > +       shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
> > +       if (IS_ERR(shm))
> > +               return PTR_ERR(shm);
> > +
> > +       msg_arg->cmd = cmd;
> > +       optee_do_call_with_arg(ctx, msg_parg);
> > +
> > +       tee_shm_free(shm);
> > +       return 0;
> > +}
> > +
> > +int optee_do_bottom_half(struct tee_context *ctx)
> > +{
> > +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
> > +}
> > +
> > +int optee_stop_async_notif(struct tee_context *ctx)
> > +{
> > +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);
>
> Is there any particular reason that this isn't a fast call similar to
> OPTEE_SMC_ENABLE_ASYNC_NOTIF?

This is needed to allow the callbacks in the secure world to
synchronize with other threads.

>
> > +}
> > +
> >  /**
> >   * optee_enable_shm_cache() - Enables caching of some shared memory allocation
> >   *                           in OP-TEE
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 8531184f98f4..ccafd7151b45 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -8,9 +8,12 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/crash_dump.h>
> >  #include <linux/errno.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > @@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
> >         .flags = TEE_DESC_PRIVILEGED,
> >  };
> >
> > +static int enable_async_notif(optee_invoke_fn *invoke_fn)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       if (res.a0)
> > +               return -EINVAL;
> > +       return 0;
> > +}
> > +
> >  static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
> >  {
> >         struct arm_smccc_res res;
> > @@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
> >  }
> >
> >  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > -                                           u32 *sec_caps)
> > +                                           u32 *sec_caps, u32 *max_notif_value)
> >  {
> >         union {
> >                 struct arm_smccc_res smccc;
> > @@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> >                 return false;
> >
> >         *sec_caps = res.result.capabilities;
> > +       *max_notif_value = res.result.max_notif_value;
> >         return true;
> >  }
> >
> > @@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
> >         struct optee *optee = NULL;
> >         void *memremaped_shm = NULL;
> >         struct tee_device *teedev;
> > +       u32 max_notif_value;
> >         u32 sec_caps;
> >         int rc;
> >
> > @@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > -       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
> > +       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > +                                            &max_notif_value)) {
> >                 pr_warn("capabilities mismatch\n");
> >                 return -EINVAL;
> >         }
> > @@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
> >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> >         if (!optee) {
> >                 rc = -ENOMEM;
> > -               goto err;
> > +               goto err_free_pool;
> >         }
> >
> >         optee->invoke_fn = invoke_fn;
> > @@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
> >         teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
> >         if (IS_ERR(teedev)) {
> >                 rc = PTR_ERR(teedev);
> > -               goto err;
> > +               goto err_free_optee;
> >         }
> >         optee->teedev = teedev;
> >
> >         teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
> >         if (IS_ERR(teedev)) {
> >                 rc = PTR_ERR(teedev);
> > -               goto err;
> > +               goto err_unreg_teedev;
> >         }
> >         optee->supp_teedev = teedev;
> >
> >         rc = tee_device_register(optee->teedev);
> >         if (rc)
> > -               goto err;
> > +               goto err_unreg_supp_teedev;
> >
> >         rc = tee_device_register(optee->supp_teedev);
> >         if (rc)
> > -               goto err;
> > +               goto err_unreg_supp_teedev;
> >
> >         mutex_init(&optee->call_queue.mutex);
> >         INIT_LIST_HEAD(&optee->call_queue.waiters);
> > @@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
> >         optee->pool = pool;
> >
> >         platform_set_drvdata(pdev, optee);
> > -       rc = optee_notif_init(optee, 255);
> > -       if (rc) {
> > -               optee_remove(pdev);
> > -               return rc;
> > +
> > +       if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> > +               unsigned int irq;
> > +
> > +               rc = platform_get_irq(pdev, 0);
> > +               if (rc < 0) {
> > +                       pr_err("platform_get_irq: ret %d\n", rc);
> > +                       goto err_unreg_supp_teedev;
> > +               }
> > +               irq = rc;
> > +
> > +               rc = optee_notif_init(optee, max_notif_value, irq);
> > +               if (rc) {
> > +                       irq_dispose_mapping(irq);
> > +                       optee_remove(pdev);
> > +                       return rc;
> > +               }
> > +               enable_async_notif(optee->invoke_fn);
> > +               pr_info("Asynchronous notifications enabled\n");
> > +       } else {
> > +               rc = optee_notif_init(optee, 255, 0);
> > +               if (rc) {
> > +                       optee_remove(pdev);
>
> IMO, multiple usage of optee_remove() in the error path makes error
> handling a bit complex. I liked the way you tagged the error paths. So
> can't we get rid of optee_remove() from the error path and handle
> errors based on tagged error paths only?

OK, I'll fix.

>
> > +                       return rc;
> > +               }
> >         }
> >
> >         /*
> > @@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
> >
> >         pr_info("initialized driver\n");
> >         return 0;
> > -err:
> > -       if (optee) {
> > -               /*
> > -                * tee_device_unregister() is safe to call even if the
> > -                * devices hasn't been registered with
> > -                * tee_device_register() yet.
> > -                */
> > -               tee_device_unregister(optee->supp_teedev);
> > -               tee_device_unregister(optee->teedev);
> > -               kfree(optee);
> > -       }
> > -       if (pool)
> > -               tee_shm_pool_free(pool);
> > -       if (memremaped_shm)
> > +err_unreg_supp_teedev:
> > +       tee_device_unregister(optee->supp_teedev);
> > +err_unreg_teedev:
> > +       tee_device_unregister(optee->teedev);
> > +err_free_optee:
> > +       kfree(optee);
> > +err_free_pool:
> > +       tee_shm_pool_free(pool);
> > +       if (optee->memremaped_shm)
> >                 memunmap(memremaped_shm);
> >         return rc;
> >  }
> > diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> > index a28fa03dcd0e..2c888ad87451 100644
> > --- a/drivers/tee/optee/notif.c
> > +++ b/drivers/tee/optee/notif.c
> > @@ -7,10 +7,14 @@
> >
> >  #include <linux/arm-smccc.h>
> >  #include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/tee_drv.h>
> >  #include "optee_private.h"
> > +#include "optee_smc.h"
> > +#include "optee_rpc_cmd.h"
> >
> >  struct notif_entry {
> >         struct list_head link;
> > @@ -18,6 +22,54 @@ struct notif_entry {
> >         u_int key;
> >  };
> >
> > +static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> > +                                bool *value_pending)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       if (res.a0)
> > +               return 0;
> > +       *value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
> > +       *value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
> > +       return res.a1;
> > +}
> > +
> > +static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct optee *optee = dev_id;
> > +       bool do_bottom_half = false;
> > +       bool value_valid;
> > +       bool value_pending;
> > +       u32 value;
> > +
> > +       do {
> > +               value = get_async_notif_value(optee->invoke_fn, &value_valid,
> > +                                             &value_pending);
> > +               if (!value_valid)
> > +                       break;
> > +
> > +               if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
> > +                       do_bottom_half = true;
> > +               else
> > +                       optee_notif_send(optee, value);
> > +       } while (value_pending);
> > +
> > +       if (do_bottom_half)
> > +               return IRQ_WAKE_THREAD;
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> > +{
> > +       struct optee *optee = dev_id;
> > +
> > +       optee_do_bottom_half(optee->notif.ctx);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> >  static bool have_key(struct optee *optee, u_int key)
> >  {
> >         struct notif_entry *entry;
> > @@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
> >         return 0;
> >  }
> >
> > -int optee_notif_init(struct optee *optee, u_int max_key)
> > +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
> >  {
> > +       struct tee_context *ctx;
> > +       int rc;
> > +
> > +       if (irq) {
> > +               ctx = teedev_open(optee->teedev);
> > +               if (IS_ERR(ctx))
> > +                       return PTR_ERR(ctx);
> > +
> > +               optee->notif.ctx = ctx;
> > +       }
> > +
> >         spin_lock_init(&optee->notif.lock);
> >         INIT_LIST_HEAD(&optee->notif.db);
> >         optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> > -       if (!optee->notif.bitmap)
> > -               return -ENOMEM;
> > -
> > +       if (!optee->notif.bitmap) {
> > +               rc = -ENOMEM;
> > +               goto err_close_ctx;
> > +       }
> >         optee->notif.max_key = max_key;
> >
> > +       if (irq) {
> > +               rc = request_threaded_irq(irq, notif_irq_handler,
> > +                                         notif_irq_thread_fn,
> > +                                         0, "optee_notification", optee);
> > +               if (rc)
> > +                       goto err_free_bitmap;
> > +
> > +               optee->notif.irq = irq;
> > +       }
> > +
> >         return 0;
> > +
> > +err_free_bitmap:
> > +       kfree(optee->notif.bitmap);
> > +err_close_ctx:
> > +       teedev_close_context(optee->notif.ctx);
> > +       optee->notif.ctx = NULL;
> > +
> > +       return rc;
> >  }
> >
> >  void optee_notif_uninit(struct optee *optee)
> >  {
> > +       if (optee->notif.ctx) {
> > +               optee_stop_async_notif(optee->notif.ctx);
> > +               if (optee->notif.irq) {
> > +                       free_irq(optee->notif.irq, optee);
> > +                       irq_dispose_mapping(optee->notif.irq);
> > +               }
> > +
> > +               /*
> > +                * The thread normally working with optee->notif.ctx was
> > +                * stopped with free_irq() above.
> > +                *
> > +                * Note we're not using teedev_close_context() or
> > +                * tee_client_close_context() since we have already called
> > +                * tee_device_put() while initializing to avoid a circular
> > +                * reference counting.
> > +                */
> > +               teedev_close_context(optee->notif.ctx);
> > +       }
> > +
> >         kfree(optee->notif.bitmap);
> >  }
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index e3d72d09c484..3e09c8386e46 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -293,6 +293,13 @@ struct optee_msg_arg {
> >   * [in] param[0].u.rmem.shm_ref                holds shared memory reference
> >   * [in] param[0].u.rmem.offs           0
> >   * [in] param[0].u.rmem.size           0
> > + *
> > + * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
> > + * of a driver.
> > + *
> > + * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
> > + * normal world unable to process asynchronous notifications. Typically
> > + * used when the driver is shut down.
> >   */
> >  #define OPTEE_MSG_CMD_OPEN_SESSION     0
> >  #define OPTEE_MSG_CMD_INVOKE_COMMAND   1
> > @@ -300,6 +307,8 @@ struct optee_msg_arg {
> >  #define OPTEE_MSG_CMD_CANCEL           3
> >  #define OPTEE_MSG_CMD_REGISTER_SHM     4
> >  #define OPTEE_MSG_CMD_UNREGISTER_SHM   5
> > +#define OPTEE_MSG_CMD_DO_BOTTOM_HALF   6
> > +#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7
> >  #define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004
> >
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 76a16d9b6cf4..f62a16935c49 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -37,6 +37,8 @@ struct optee_call_queue {
> >
> >  struct optee_notif {
> >         u_int max_key;
> > +       unsigned int irq;
> > +       struct tee_context *ctx;
> >         /* Serializes access to the elements below in this struct */
> >         spinlock_t lock;
> >         struct list_head db;
> > @@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> >                       struct optee_call_ctx *call_ctx);
> >  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
> >
> > -int optee_notif_init(struct optee *optee, u_int max_key);
> > +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
> >  void optee_notif_uninit(struct optee *optee);
> >  int optee_notif_wait(struct optee *optee, u_int key);
> >  int optee_notif_send(struct optee *optee, u_int key);
> > @@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
> >  int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> >                       struct tee_param *param);
> >  int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> > +int optee_do_bottom_half(struct tee_context *ctx);
> > +int optee_stop_async_notif(struct tee_context *ctx);
> >
> >  void optee_enable_shm_cache(struct optee *optee);
> >  void optee_disable_shm_cache(struct optee *optee);
> > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > index 80eb763a8a80..c6eec6b6febf 100644
> > --- a/drivers/tee/optee/optee_smc.h
> > +++ b/drivers/tee/optee/optee_smc.h
> > @@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
> >  /*
> >   * Call with struct optee_msg_arg as argument
> >   *
> > + * When calling this function normal world has a few responsibilities:
> > + * 1. It must be able to handle eventual RPCs
> > + * 2. Non-secure interrupts should not be masked
> > + * 3. If asynchronous notifications has be negotiated successfully, then
>
> nit: s/has be/have been/
>
> > + *    asynchronous notifications should be unmasked during this call.
> > + *
> >   * Call register usage:
> >   * a0  SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
> >   * a1  Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
> > @@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
> >   * Normal return register usage:
> >   * a0  OPTEE_SMC_RETURN_OK
> >   * a1  bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
> > - * a2-7        Preserved
> > + * a2  The maximum secure world notification number
> > + * a3-7        Preserved
> >   *
> >   * Error return register usage:
> >   * a0  OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
> > @@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
> >  #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION       BIT(3)
> >  /* Secure world supports Shared Memory with a NULL reference */
> >  #define OPTEE_SMC_SEC_CAP_MEMREF_NULL          BIT(4)
> > +/* Secure world supports asynchronous notification of normal world */
> > +#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> >
> >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > @@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
> >  struct optee_smc_exchange_capabilities_result {
> >         unsigned long status;
> >         unsigned long capabilities;
> > +       unsigned long max_notif_value;
> >         unsigned long reserved0;
> > -       unsigned long reserved1;
> >  };
> >
> >  /*
> > @@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
> >  #define OPTEE_SMC_GET_THREAD_COUNT \
> >         OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
> >
> > +/*
> > + * Inform OP-TEE that normal world is able to receive asynchronous
> > + * notifications.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
> > + * a1-6        Not used
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF    16
> > +#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
> > +
> > +/*
> > + * Retrieve a value of notifications pended since the last call of this
>
> nit: s/pended/pending/
>
> > + * function.
> > + *
> > + * OP-TEE keeps a records of all posted values. When an interrupts is
>
> nit: s/records/record/
>
> > + * received which indicates that there are posed values this function
> > + * should be called until all pended values has been retrieved. When a
>
> nit: s/has/have/
>
> > + * value is retrieved it's cleared from the record in secure world.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
> > + * a1-6        Not used
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1  value
> > + * a2  Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
> > + *             valid, else 0 if no values where pending
> > + * a2  Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
> > + *             pending, else 0.
> > + *     Bit[31:2]: MBZ
> > + * a3-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID      BIT(0)
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING    BIT(1)
> > +
> > +/*
> > + * Notification that OP-TEE expects a yielding call to do some bottom half
> > + * work in a driver.
> > + */
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF     0
> > +
> > +#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17
> > +#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
> > +
> >  /*
> >   * Resume from RPC (for example after processing a foreign interrupt)
> >   *
>
> Apart from comments above, this patch looks good to me in general.

Thanks, I'll take care of all the nits above.

Cheers,
Jens

Re: [PATCH v6 6/6] optee: add asynchronous notifications

[Jens Wiklander]

On Wed, Oct 13, 2021 at 10:07 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 6 Oct 2021 at 12:46, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Adds support for asynchronous notifications from secure world to normal
> > world. This allows a design with a top half and bottom half type of
> > driver where the top half runs in secure interrupt context and a
> > notifications tells normal world to schedule a yielding call to do the
> > bottom half processing.
> >
> > The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h.
> >
> > A notification consists of a 32-bit value which normal world can
> > retrieve using a fastcall into secure world. The value
> > OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF (0) has a special meaning.
> > When this value is sent it means that normal world is supposed to make a
> > yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF.
> >
> > Notification capability is negotiated while the driver is initialized.
> > If both sides supports these notifications then they are enabled.
> >
> > An interrupt is used to notify the driver that there are asynchronous
> > notifications pending. The maximum needed notification value is
> > communicated at this stage. This allows scaling up when needed.
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/call.c          |  27 ++++++++
> >  drivers/tee/optee/core.c          |  83 ++++++++++++++++-------
> >  drivers/tee/optee/notif.c         | 109 ++++++++++++++++++++++++++++--
> >  drivers/tee/optee/optee_msg.h     |   9 +++
> >  drivers/tee/optee/optee_private.h |   6 +-
> >  drivers/tee/optee/optee_smc.h     |  75 +++++++++++++++++++-
> >  6 files changed, 277 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 945f03da0223..e428e0e9a3af 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -392,6 +392,33 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> >         return 0;
> >  }
> >
> > +static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
> > +{
> > +       struct optee_msg_arg *msg_arg;
> > +       phys_addr_t msg_parg;
> > +       struct tee_shm *shm;
> > +
> > +       shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg);
> > +       if (IS_ERR(shm))
> > +               return PTR_ERR(shm);
> > +
> > +       msg_arg->cmd = cmd;
> > +       optee_do_call_with_arg(ctx, msg_parg);
> > +
> > +       tee_shm_free(shm);
> > +       return 0;
> > +}
> > +
> > +int optee_do_bottom_half(struct tee_context *ctx)
> > +{
> > +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_DO_BOTTOM_HALF);
> > +}
> > +
> > +int optee_stop_async_notif(struct tee_context *ctx)
> > +{
> > +       return simple_call_with_arg(ctx, OPTEE_MSG_CMD_STOP_ASYNC_NOTIF);
>
> Is there any particular reason that this isn't a fast call similar to
> OPTEE_SMC_ENABLE_ASYNC_NOTIF?

This is needed to allow the callbacks in the secure world to
synchronize with other threads.

>
> > +}
> > +
> >  /**
> >   * optee_enable_shm_cache() - Enables caching of some shared memory allocation
> >   *                           in OP-TEE
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 8531184f98f4..ccafd7151b45 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -8,9 +8,12 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/crash_dump.h>
> >  #include <linux/errno.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > @@ -355,6 +358,17 @@ static const struct tee_desc optee_supp_desc = {
> >         .flags = TEE_DESC_PRIVILEGED,
> >  };
> >
> > +static int enable_async_notif(optee_invoke_fn *invoke_fn)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       invoke_fn(OPTEE_SMC_ENABLE_ASYNC_NOTIF, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       if (res.a0)
> > +               return -EINVAL;
> > +       return 0;
> > +}
> > +
> >  static bool optee_msg_api_uid_is_optee_api(optee_invoke_fn *invoke_fn)
> >  {
> >         struct arm_smccc_res res;
> > @@ -404,7 +418,7 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
> >  }
> >
> >  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > -                                           u32 *sec_caps)
> > +                                           u32 *sec_caps, u32 *max_notif_value)
> >  {
> >         union {
> >                 struct arm_smccc_res smccc;
> > @@ -427,6 +441,7 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> >                 return false;
> >
> >         *sec_caps = res.result.capabilities;
> > +       *max_notif_value = res.result.max_notif_value;
> >         return true;
> >  }
> >
> > @@ -630,6 +645,7 @@ static int optee_probe(struct platform_device *pdev)
> >         struct optee *optee = NULL;
> >         void *memremaped_shm = NULL;
> >         struct tee_device *teedev;
> > +       u32 max_notif_value;
> >         u32 sec_caps;
> >         int rc;
> >
> > @@ -659,7 +675,8 @@ static int optee_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > -       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
> > +       if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > +                                            &max_notif_value)) {
> >                 pr_warn("capabilities mismatch\n");
> >                 return -EINVAL;
> >         }
> > @@ -682,7 +699,7 @@ static int optee_probe(struct platform_device *pdev)
> >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> >         if (!optee) {
> >                 rc = -ENOMEM;
> > -               goto err;
> > +               goto err_free_pool;
> >         }
> >
> >         optee->invoke_fn = invoke_fn;
> > @@ -691,24 +708,24 @@ static int optee_probe(struct platform_device *pdev)
> >         teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
> >         if (IS_ERR(teedev)) {
> >                 rc = PTR_ERR(teedev);
> > -               goto err;
> > +               goto err_free_optee;
> >         }
> >         optee->teedev = teedev;
> >
> >         teedev = tee_device_alloc(&optee_supp_desc, NULL, pool, optee);
> >         if (IS_ERR(teedev)) {
> >                 rc = PTR_ERR(teedev);
> > -               goto err;
> > +               goto err_unreg_teedev;
> >         }
> >         optee->supp_teedev = teedev;
> >
> >         rc = tee_device_register(optee->teedev);
> >         if (rc)
> > -               goto err;
> > +               goto err_unreg_supp_teedev;
> >
> >         rc = tee_device_register(optee->supp_teedev);
> >         if (rc)
> > -               goto err;
> > +               goto err_unreg_supp_teedev;
> >
> >         mutex_init(&optee->call_queue.mutex);
> >         INIT_LIST_HEAD(&optee->call_queue.waiters);
> > @@ -717,10 +734,31 @@ static int optee_probe(struct platform_device *pdev)
> >         optee->pool = pool;
> >
> >         platform_set_drvdata(pdev, optee);
> > -       rc = optee_notif_init(optee, 255);
> > -       if (rc) {
> > -               optee_remove(pdev);
> > -               return rc;
> > +
> > +       if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> > +               unsigned int irq;
> > +
> > +               rc = platform_get_irq(pdev, 0);
> > +               if (rc < 0) {
> > +                       pr_err("platform_get_irq: ret %d\n", rc);
> > +                       goto err_unreg_supp_teedev;
> > +               }
> > +               irq = rc;
> > +
> > +               rc = optee_notif_init(optee, max_notif_value, irq);
> > +               if (rc) {
> > +                       irq_dispose_mapping(irq);
> > +                       optee_remove(pdev);
> > +                       return rc;
> > +               }
> > +               enable_async_notif(optee->invoke_fn);
> > +               pr_info("Asynchronous notifications enabled\n");
> > +       } else {
> > +               rc = optee_notif_init(optee, 255, 0);
> > +               if (rc) {
> > +                       optee_remove(pdev);
>
> IMO, multiple usage of optee_remove() in the error path makes error
> handling a bit complex. I liked the way you tagged the error paths. So
> can't we get rid of optee_remove() from the error path and handle
> errors based on tagged error paths only?

OK, I'll fix.

>
> > +                       return rc;
> > +               }
> >         }
> >
> >         /*
> > @@ -745,20 +783,15 @@ static int optee_probe(struct platform_device *pdev)
> >
> >         pr_info("initialized driver\n");
> >         return 0;
> > -err:
> > -       if (optee) {
> > -               /*
> > -                * tee_device_unregister() is safe to call even if the
> > -                * devices hasn't been registered with
> > -                * tee_device_register() yet.
> > -                */
> > -               tee_device_unregister(optee->supp_teedev);
> > -               tee_device_unregister(optee->teedev);
> > -               kfree(optee);
> > -       }
> > -       if (pool)
> > -               tee_shm_pool_free(pool);
> > -       if (memremaped_shm)
> > +err_unreg_supp_teedev:
> > +       tee_device_unregister(optee->supp_teedev);
> > +err_unreg_teedev:
> > +       tee_device_unregister(optee->teedev);
> > +err_free_optee:
> > +       kfree(optee);
> > +err_free_pool:
> > +       tee_shm_pool_free(pool);
> > +       if (optee->memremaped_shm)
> >                 memunmap(memremaped_shm);
> >         return rc;
> >  }
> > diff --git a/drivers/tee/optee/notif.c b/drivers/tee/optee/notif.c
> > index a28fa03dcd0e..2c888ad87451 100644
> > --- a/drivers/tee/optee/notif.c
> > +++ b/drivers/tee/optee/notif.c
> > @@ -7,10 +7,14 @@
> >
> >  #include <linux/arm-smccc.h>
> >  #include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/tee_drv.h>
> >  #include "optee_private.h"
> > +#include "optee_smc.h"
> > +#include "optee_rpc_cmd.h"
> >
> >  struct notif_entry {
> >         struct list_head link;
> > @@ -18,6 +22,54 @@ struct notif_entry {
> >         u_int key;
> >  };
> >
> > +static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> > +                                bool *value_pending)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       invoke_fn(OPTEE_SMC_GET_ASYNC_NOTIF_VALUE, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       if (res.a0)
> > +               return 0;
> > +       *value_valid = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID);
> > +       *value_pending = (res.a2 & OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING);
> > +       return res.a1;
> > +}
> > +
> > +static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct optee *optee = dev_id;
> > +       bool do_bottom_half = false;
> > +       bool value_valid;
> > +       bool value_pending;
> > +       u32 value;
> > +
> > +       do {
> > +               value = get_async_notif_value(optee->invoke_fn, &value_valid,
> > +                                             &value_pending);
> > +               if (!value_valid)
> > +                       break;
> > +
> > +               if (value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF)
> > +                       do_bottom_half = true;
> > +               else
> > +                       optee_notif_send(optee, value);
> > +       } while (value_pending);
> > +
> > +       if (do_bottom_half)
> > +               return IRQ_WAKE_THREAD;
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> > +{
> > +       struct optee *optee = dev_id;
> > +
> > +       optee_do_bottom_half(optee->notif.ctx);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> >  static bool have_key(struct optee *optee, u_int key)
> >  {
> >         struct notif_entry *entry;
> > @@ -106,20 +158,69 @@ int optee_notif_send(struct optee *optee, u_int key)
> >         return 0;
> >  }
> >
> > -int optee_notif_init(struct optee *optee, u_int max_key)
> > +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq)
> >  {
> > +       struct tee_context *ctx;
> > +       int rc;
> > +
> > +       if (irq) {
> > +               ctx = teedev_open(optee->teedev);
> > +               if (IS_ERR(ctx))
> > +                       return PTR_ERR(ctx);
> > +
> > +               optee->notif.ctx = ctx;
> > +       }
> > +
> >         spin_lock_init(&optee->notif.lock);
> >         INIT_LIST_HEAD(&optee->notif.db);
> >         optee->notif.bitmap = bitmap_zalloc(max_key, GFP_KERNEL);
> > -       if (!optee->notif.bitmap)
> > -               return -ENOMEM;
> > -
> > +       if (!optee->notif.bitmap) {
> > +               rc = -ENOMEM;
> > +               goto err_close_ctx;
> > +       }
> >         optee->notif.max_key = max_key;
> >
> > +       if (irq) {
> > +               rc = request_threaded_irq(irq, notif_irq_handler,
> > +                                         notif_irq_thread_fn,
> > +                                         0, "optee_notification", optee);
> > +               if (rc)
> > +                       goto err_free_bitmap;
> > +
> > +               optee->notif.irq = irq;
> > +       }
> > +
> >         return 0;
> > +
> > +err_free_bitmap:
> > +       kfree(optee->notif.bitmap);
> > +err_close_ctx:
> > +       teedev_close_context(optee->notif.ctx);
> > +       optee->notif.ctx = NULL;
> > +
> > +       return rc;
> >  }
> >
> >  void optee_notif_uninit(struct optee *optee)
> >  {
> > +       if (optee->notif.ctx) {
> > +               optee_stop_async_notif(optee->notif.ctx);
> > +               if (optee->notif.irq) {
> > +                       free_irq(optee->notif.irq, optee);
> > +                       irq_dispose_mapping(optee->notif.irq);
> > +               }
> > +
> > +               /*
> > +                * The thread normally working with optee->notif.ctx was
> > +                * stopped with free_irq() above.
> > +                *
> > +                * Note we're not using teedev_close_context() or
> > +                * tee_client_close_context() since we have already called
> > +                * tee_device_put() while initializing to avoid a circular
> > +                * reference counting.
> > +                */
> > +               teedev_close_context(optee->notif.ctx);
> > +       }
> > +
> >         kfree(optee->notif.bitmap);
> >  }
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index e3d72d09c484..3e09c8386e46 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -293,6 +293,13 @@ struct optee_msg_arg {
> >   * [in] param[0].u.rmem.shm_ref                holds shared memory reference
> >   * [in] param[0].u.rmem.offs           0
> >   * [in] param[0].u.rmem.size           0
> > + *
> > + * OPTEE_MSG_CMD_DO_BOTTOM_HALF does the scheduled bottom half processing
> > + * of a driver.
> > + *
> > + * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is
> > + * normal world unable to process asynchronous notifications. Typically
> > + * used when the driver is shut down.
> >   */
> >  #define OPTEE_MSG_CMD_OPEN_SESSION     0
> >  #define OPTEE_MSG_CMD_INVOKE_COMMAND   1
> > @@ -300,6 +307,8 @@ struct optee_msg_arg {
> >  #define OPTEE_MSG_CMD_CANCEL           3
> >  #define OPTEE_MSG_CMD_REGISTER_SHM     4
> >  #define OPTEE_MSG_CMD_UNREGISTER_SHM   5
> > +#define OPTEE_MSG_CMD_DO_BOTTOM_HALF   6
> > +#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7
> >  #define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004
> >
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 76a16d9b6cf4..f62a16935c49 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -37,6 +37,8 @@ struct optee_call_queue {
> >
> >  struct optee_notif {
> >         u_int max_key;
> > +       unsigned int irq;
> > +       struct tee_context *ctx;
> >         /* Serializes access to the elements below in this struct */
> >         spinlock_t lock;
> >         struct list_head db;
> > @@ -132,7 +134,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> >                       struct optee_call_ctx *call_ctx);
> >  void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx);
> >
> > -int optee_notif_init(struct optee *optee, u_int max_key);
> > +int optee_notif_init(struct optee *optee, u_int max_key, u_int irq);
> >  void optee_notif_uninit(struct optee *optee);
> >  int optee_notif_wait(struct optee *optee, u_int key);
> >  int optee_notif_send(struct optee *optee, u_int key);
> > @@ -159,6 +161,8 @@ int optee_close_session(struct tee_context *ctx, u32 session);
> >  int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> >                       struct tee_param *param);
> >  int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> > +int optee_do_bottom_half(struct tee_context *ctx);
> > +int optee_stop_async_notif(struct tee_context *ctx);
> >
> >  void optee_enable_shm_cache(struct optee *optee);
> >  void optee_disable_shm_cache(struct optee *optee);
> > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > index 80eb763a8a80..c6eec6b6febf 100644
> > --- a/drivers/tee/optee/optee_smc.h
> > +++ b/drivers/tee/optee/optee_smc.h
> > @@ -107,6 +107,12 @@ struct optee_smc_call_get_os_revision_result {
> >  /*
> >   * Call with struct optee_msg_arg as argument
> >   *
> > + * When calling this function normal world has a few responsibilities:
> > + * 1. It must be able to handle eventual RPCs
> > + * 2. Non-secure interrupts should not be masked
> > + * 3. If asynchronous notifications has be negotiated successfully, then
>
> nit: s/has be/have been/
>
> > + *    asynchronous notifications should be unmasked during this call.
> > + *
> >   * Call register usage:
> >   * a0  SMC Function ID, OPTEE_SMC*CALL_WITH_ARG
> >   * a1  Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg
> > @@ -195,7 +201,8 @@ struct optee_smc_get_shm_config_result {
> >   * Normal return register usage:
> >   * a0  OPTEE_SMC_RETURN_OK
> >   * a1  bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_*
> > - * a2-7        Preserved
> > + * a2  The maximum secure world notification number
> > + * a3-7        Preserved
> >   *
> >   * Error return register usage:
> >   * a0  OPTEE_SMC_RETURN_ENOTAVAIL, can't use the capabilities from normal world
> > @@ -218,6 +225,8 @@ struct optee_smc_get_shm_config_result {
> >  #define OPTEE_SMC_SEC_CAP_VIRTUALIZATION       BIT(3)
> >  /* Secure world supports Shared Memory with a NULL reference */
> >  #define OPTEE_SMC_SEC_CAP_MEMREF_NULL          BIT(4)
> > +/* Secure world supports asynchronous notification of normal world */
> > +#define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> >
> >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > @@ -226,8 +235,8 @@ struct optee_smc_get_shm_config_result {
> >  struct optee_smc_exchange_capabilities_result {
> >         unsigned long status;
> >         unsigned long capabilities;
> > +       unsigned long max_notif_value;
> >         unsigned long reserved0;
> > -       unsigned long reserved1;
> >  };
> >
> >  /*
> > @@ -319,6 +328,68 @@ struct optee_smc_disable_shm_cache_result {
> >  #define OPTEE_SMC_GET_THREAD_COUNT \
> >         OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_THREAD_COUNT)
> >
> > +/*
> > + * Inform OP-TEE that normal world is able to receive asynchronous
> > + * notifications.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_ENABLE_ASYNC_NOTIF
> > + * a1-6        Not used
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF    16
> > +#define OPTEE_SMC_ENABLE_ASYNC_NOTIF \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_ASYNC_NOTIF)
> > +
> > +/*
> > + * Retrieve a value of notifications pended since the last call of this
>
> nit: s/pended/pending/
>
> > + * function.
> > + *
> > + * OP-TEE keeps a records of all posted values. When an interrupts is
>
> nit: s/records/record/
>
> > + * received which indicates that there are posed values this function
> > + * should be called until all pended values has been retrieved. When a
>
> nit: s/has/have/
>
> > + * value is retrieved it's cleared from the record in secure world.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE
> > + * a1-6        Not used
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1  value
> > + * a2  Bit[0]: OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID if the value in a1 is
> > + *             valid, else 0 if no values where pending
> > + * a2  Bit[1]: OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING if another value is
> > + *             pending, else 0.
> > + *     Bit[31:2]: MBZ
> > + * a3-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_VALID      BIT(0)
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_PENDING    BIT(1)
> > +
> > +/*
> > + * Notification that OP-TEE expects a yielding call to do some bottom half
> > + * work in a driver.
> > + */
> > +#define OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF     0
> > +
> > +#define OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE 17
> > +#define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE)
> > +
> >  /*
> >   * Resume from RPC (for example after processing a foreign interrupt)
> >   *
>
> Apart from comments above, this patch looks good to me in general.

Thanks, I'll take care of all the nits above.

Cheers,
Jens

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel