[PATCH v2 0/7] Asynchronous notifications from secure world

[PATCH v2 0/7] 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 interrupt is used to notify the driver that there are asynchronous
notifications pending.

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 (7):
  docs: staging/tee.rst: add a section on OP-TEE notifications
  dt-bindings: arm: Convert optee binding to json-schema
  dt-bindings: arm: optee: add interrupt property
  tee: fix put order in teedev_close_context()
  tee: add tee_dev_open_helper() primitive
  optee: separate notification functions
  optee: add asynchronous notifications

 .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
 .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
 Documentation/staging/tee.rst                 |  27 +++
 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                        |  37 ++-
 include/linux/tee_drv.h                       |  27 +++
 14 files changed, 576 insertions(+), 155 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
 create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
 create mode 100644 drivers/tee/optee/notif.c

-- 
2.31.1

[PATCH v2 1/7] 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.

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

diff --git a/Documentation/staging/tee.rst b/Documentation/staging/tee.rst
index 4d4b5f889603..37bdd097336f 100644
--- a/Documentation/staging/tee.rst
+++ b/Documentation/staging/tee.rst
@@ -184,6 +184,33 @@ 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
+   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 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``.
+
+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 v2 2/7] dt-bindings: arm: Convert optee binding to json-schema

[Jens Wiklander]

Converts the optee binding to use DT schema format.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 .../bindings/arm/firmware/linaro,optee-tz.txt | 31 ------------
 .../arm/firmware/linaro,optee-tz.yaml         | 49 +++++++++++++++++++
 2 files changed, 49 insertions(+), 31 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
 create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml

diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
deleted file mode 100644
index d38834c67dff..000000000000
--- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
+++ /dev/null
@@ -1,31 +0,0 @@
-OP-TEE Device Tree Bindings
-
-OP-TEE is a piece of software using hardware features to provide a Trusted
-Execution Environment. The security can be provided with ARM TrustZone, but
-also by virtualization or a separate chip.
-
-We're using "linaro" as the first part of the compatible property for
-the reference implementation maintained by Linaro.
-
-* OP-TEE based on ARM TrustZone required properties:
-
-- compatible     : should contain "linaro,optee-tz"
-
-- method         : The method of calling the OP-TEE Trusted OS. Permitted
-                   values are:
-
-                   "smc" : SMC #0, with the register assignments specified
-		           in drivers/tee/optee/optee_smc.h
-
-                   "hvc" : HVC #0, with the register assignments specified
-		           in drivers/tee/optee/optee_smc.h
-
-
-
-Example:
-	firmware {
-		optee {
-			compatible = "linaro,optee-tz";
-			method = "smc";
-		};
-	};
diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
new file mode 100644
index 000000000000..c931b030057f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/firmware/linaro,optee-tz.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OP-TEE Device Tree Bindings
+
+maintainers:
+  - Jens Wiklander <jens.wiklander@linaro.org>
+
+description: |
+  OP-TEE is a piece of software using hardware features to provide a Trusted
+  Execution Environment. The security can be provided with ARM TrustZone, but
+  also by virtualization or a separate chip.
+
+properties:
+  $nodename:
+    const: optee
+  compatible:
+      oneOf:
+        - description:
+            We're using "linaro" as the first part of the compatible property
+            for the reference implementation maintained by Linaro.
+          const: linaro,optee-tz
+
+  method:
+    description: The method of calling the OP-TEE Trusted OS.
+    $ref: /schemas/types.yaml#/definitions/string-array
+    oneOf:
+      - description: |
+          SMC #0, with the register assignments specified in
+          in drivers/tee/optee/optee_smc.h
+        const: smc
+      - description: |
+          HVC #0, with the register assignments specified in
+          in drivers/tee/optee/optee_smc.h
+        const: hvc
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware {
+      optee {
+        compatible = "linaro,optee-tz";
+        method = "smc";
+      };
+    };
-- 
2.31.1

Re: [PATCH v2 2/7] dt-bindings: arm: Convert optee binding to json-schema

[Rob Herring]

On Wed, 16 Jun 2021 12:36:44 +0200, Jens Wiklander wrote:
> Converts the optee binding to use DT schema format.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../bindings/arm/firmware/linaro,optee-tz.txt | 31 ------------
>  .../arm/firmware/linaro,optee-tz.yaml         | 49 +++++++++++++++++++
>  2 files changed, 49 insertions(+), 31 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml:21:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

dtschema/dtc warnings/errors:
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1492865

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 2/7] dt-bindings: arm: Convert optee binding to json-schema

[Rob Herring]

On Wed, Jun 16, 2021 at 12:36:44PM +0200, Jens Wiklander wrote:
> Converts the optee binding to use DT schema format.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../bindings/arm/firmware/linaro,optee-tz.txt | 31 ------------
>  .../arm/firmware/linaro,optee-tz.yaml         | 49 +++++++++++++++++++
>  2 files changed, 49 insertions(+), 31 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml

Other than the indentation,

Reviewed-by: Rob Herring <robh@kernel.org> 

Re: [PATCH v2 2/7] dt-bindings: arm: Convert optee binding to json-schema

[Jens Wiklander]

On Wed, Jun 16, 2021 at 6:05 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 16, 2021 at 12:36:44PM +0200, Jens Wiklander wrote:
> > Converts the optee binding to use DT schema format.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  .../bindings/arm/firmware/linaro,optee-tz.txt | 31 ------------
> >  .../arm/firmware/linaro,optee-tz.yaml         | 49 +++++++++++++++++++
> >  2 files changed, 49 insertions(+), 31 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
>
> Other than the indentation,
>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks Rob. Nishanth reminded me of
https://lore.kernel.org/linux-arm-kernel/20210503191327.GA2217487@robh.at.kernel.org/,
aka be78329717e4 ("dt-bindings: arm: firmware: Convert linaro,optee-tz
to json schema") in linux-next. I'll wait for that one to land
upstream instead.

Cheers,
Jens

[PATCH v2 3/7] dt-bindings: arm: optee: add interrupt property

[Jens Wiklander]

Adds an optional interrupt property to the optee binding.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
index c931b030057f..3efbe11b637d 100644
--- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
+++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
@@ -24,6 +24,9 @@ properties:
             for the reference implementation maintained by Linaro.
           const: linaro,optee-tz
 
+  interrupts:
+    maxItems: 1
+
   method:
     description: The method of calling the OP-TEE Trusted OS.
     $ref: /schemas/types.yaml#/definitions/string-array
@@ -37,6 +40,10 @@ properties:
           in drivers/tee/optee/optee_smc.h
         const: hvc
 
+required:
+  - compatible
+  - method
+
 additionalProperties: false
 
 examples:
@@ -45,5 +52,6 @@ examples:
       optee {
         compatible = "linaro,optee-tz";
         method = "smc";
+        interrupts = <0 187 4>;
       };
     };
-- 
2.31.1

Re: [PATCH v2 3/7] dt-bindings: arm: optee: add interrupt property

[Rob Herring]

On Wed, Jun 16, 2021 at 12:36:45PM +0200, Jens Wiklander wrote:
> Adds an optional interrupt property to the optee binding.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index c931b030057f..3efbe11b637d 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -24,6 +24,9 @@ properties:
>              for the reference implementation maintained by Linaro.
>            const: linaro,optee-tz
>  
> +  interrupts:
> +    maxItems: 1
> +
>    method:
>      description: The method of calling the OP-TEE Trusted OS.
>      $ref: /schemas/types.yaml#/definitions/string-array
> @@ -37,6 +40,10 @@ properties:
>            in drivers/tee/optee/optee_smc.h
>          const: hvc
>  
> +required:
> +  - compatible
> +  - method
> +

This should go in the first patch.

>  additionalProperties: false
>  
>  examples:
> @@ -45,5 +52,6 @@ examples:
>        optee {
>          compatible = "linaro,optee-tz";
>          method = "smc";
> +        interrupts = <0 187 4>;
>        };
>      };
> -- 
> 2.31.1

Re: [PATCH v2 3/7] dt-bindings: arm: optee: add interrupt property

[Jens Wiklander]

On Wed, Jun 16, 2021 at 6:06 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 16, 2021 at 12:36:45PM +0200, Jens Wiklander wrote:
> > Adds an optional interrupt property to the optee binding.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  .../devicetree/bindings/arm/firmware/linaro,optee-tz.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > index c931b030057f..3efbe11b637d 100644
> > --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > @@ -24,6 +24,9 @@ properties:
> >              for the reference implementation maintained by Linaro.
> >            const: linaro,optee-tz
> >
> > +  interrupts:
> > +    maxItems: 1
> > +
> >    method:
> >      description: The method of calling the OP-TEE Trusted OS.
> >      $ref: /schemas/types.yaml#/definitions/string-array
> > @@ -37,6 +40,10 @@ properties:
> >            in drivers/tee/optee/optee_smc.h
> >          const: hvc
> >
> > +required:
> > +  - compatible
> > +  - method
> > +
>
> This should go in the first patch.

OK, that will be covered when I rebase the next patch set on what's
now be78329717e4 ("dt-bindings: arm: firmware: Convert linaro,optee-tz
to json schema") in linux-next.

Thanks,
Jens

>
> >  additionalProperties: false
> >
> >  examples:
> > @@ -45,5 +52,6 @@ examples:
> >        optee {
> >          compatible = "linaro,optee-tz";
> >          method = "smc";
> > +        interrupts = <0 187 4>;
> >        };
> >      };
> > --
> > 2.31.1

[PATCH v2 4/7] 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 480d294a23ab..f97d95b50773 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 v2 5/7] tee: add tee_dev_open_helper() primitive

[Jens Wiklander]

Adds tee_dev_open_helper() and tee_dev_ctx_put() to make it easier to
create a driver internal struct tee_context without the usual
tee_device_get() on the struct tee_device as that adds a circular
reference counter dependency and would prevent the struct tee_device
from ever being released again.

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

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index f97d95b50773..6d81f6268b99 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -43,14 +43,11 @@ 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 *tee_dev_open_helper(struct tee_device *teedev)
 {
 	int rc;
 	struct tee_context *ctx;
 
-	if (!tee_device_get(teedev))
-		return ERR_PTR(-EINVAL);
-
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		rc = -ENOMEM;
@@ -66,10 +63,30 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
 	return ctx;
 err:
 	kfree(ctx);
-	tee_device_put(teedev);
 	return ERR_PTR(rc);
 
 }
+EXPORT_SYMBOL_GPL(tee_dev_open_helper);
+
+void tee_dev_ctx_put(struct tee_context *ctx)
+{
+	teedev_ctx_put(ctx);
+}
+EXPORT_SYMBOL_GPL(tee_dev_ctx_put);
+
+static struct tee_context *teedev_open(struct tee_device *teedev)
+{
+	struct tee_context *ctx;
+
+	if (!tee_device_get(teedev))
+		return ERR_PTR(-EINVAL);
+
+	ctx = tee_dev_open_helper(teedev);
+	if (IS_ERR(ctx))
+		tee_device_put(teedev);
+
+	return ctx;
+}
 
 void teedev_ctx_get(struct tee_context *ctx)
 {
@@ -90,10 +107,8 @@ static void teedev_ctx_release(struct kref *ref)
 
 void teedev_ctx_put(struct tee_context *ctx)
 {
-	if (ctx->releasing)
-		return;
-
-	kref_put(&ctx->refcount, teedev_ctx_release);
+	if (ctx && !ctx->releasing)
+		kref_put(&ctx->refcount, teedev_ctx_release);
 }
 
 static void teedev_close_context(struct tee_context *ctx)
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 54269e47ac9a..f592ba4e9561 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -456,6 +456,33 @@ static inline int tee_shm_get_id(struct tee_shm *shm)
  */
 struct tee_shm *tee_shm_get_from_id(struct tee_context *ctx, int id);
 
+/**
+ * tee_dev_open_helper() - helper function to make a struct tee_context
+ * @teedev:	Device to open
+ *
+ * Creates the struct tee_context without increasing the reference counter
+ * on @teedev. This is needed for instance when a driver need an internal
+ * struct tee_context to operate on. By skipping the reference counter
+ * the circular dependency is broken.
+ *
+ * Note that this struct tee_context need special care when freeing in
+ * order to avoid the normal put on the struct tee_device.
+ * tee_dev_ctx_put() is the best choice for this.
+ *
+ * @returns a pointer 'struct tee_context' on success or an ERR_PTR on failure
+ */
+struct tee_context *tee_dev_open_helper(struct tee_device *teedev);
+
+/**
+ * tee_dev_ctx_put() - helper function to release a struct tee_context
+ * @ctx:	The struct tee_context to release
+ *
+ * Note that this function doesn't do a tee_device_put() on the internal
+ * struct tee_device so this function should normal only be used when
+ * releasing a struct tee_context obtained with tee_dev_open_helper().
+ */
+void tee_dev_ctx_put(struct tee_context *ctx);
+
 /**
  * tee_client_open_context() - Open a TEE context
  * @start:	if not NULL, continue search after this context
-- 
2.31.1

[PATCH v2 6/7] 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          |  13 +++-
 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, 171 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 ddb8f9ecf307..2272696ac986 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -583,6 +583,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.
@@ -593,7 +594,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);
 
@@ -681,18 +681,23 @@ 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;
+	}
+
 	optee_enable_shm_cache(optee);
 
 	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 e25b216a14ef..7dc058d008b2 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 1849180b0278..e5b931f50db2 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 v2 7/7] 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.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/call.c          |  27 ++++++++
 drivers/tee/optee/core.c          |  82 +++++++++++++++-------
 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, 276 insertions(+), 32 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 6132cc8d014c..9da66acac828 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -390,6 +390,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 2272696ac986..e3c80505cc88 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -7,9 +7,12 @@
 
 #include <linux/arm-smccc.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>
@@ -353,6 +356,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;
@@ -402,7 +416,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;
@@ -425,6 +439,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;
 }
 
@@ -609,6 +624,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;
 
@@ -628,7 +644,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;
 	}
@@ -651,7 +668,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;
@@ -660,24 +677,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);
@@ -687,10 +704,30 @@ static int optee_probe(struct platform_device *pdev)
 
 	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;
+		}
 	}
 
 	optee_enable_shm_cache(optee);
@@ -706,20 +743,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..ecfa82797695 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 = tee_dev_open_helper(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_put_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_put_ctx:
+	tee_dev_ctx_put(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.
+		 */
+		tee_dev_ctx_put(optee->notif.ctx);
+	}
+
 	kfree(optee->notif.bitmap);
 }
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 81ff593ac4ec..35970932de34 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -291,6 +291,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
@@ -298,6 +305,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 7dc058d008b2..62365912a70b 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

Re: [PATCH v2 7/7] optee: add asynchronous notifications

[Ard Biesheuvel]

On Wed, 16 Jun 2021 at 12:37, 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.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  drivers/tee/optee/call.c          |  27 ++++++++
>  drivers/tee/optee/core.c          |  82 +++++++++++++++-------
>  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, 276 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 6132cc8d014c..9da66acac828 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -390,6 +390,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 2272696ac986..e3c80505cc88 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -7,9 +7,12 @@
>
>  #include <linux/arm-smccc.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>
> @@ -353,6 +356,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;
> @@ -402,7 +416,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;
> @@ -425,6 +439,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;
>  }
>
> @@ -609,6 +624,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;
>
> @@ -628,7 +644,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;
>         }
> @@ -651,7 +668,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;
> @@ -660,24 +677,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);
> @@ -687,10 +704,30 @@ static int optee_probe(struct platform_device *pdev)
>
>         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;
> +               }
>         }
>
>         optee_enable_shm_cache(optee);
> @@ -706,20 +743,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..ecfa82797695 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 = tee_dev_open_helper(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_put_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_put_ctx:
> +       tee_dev_ctx_put(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.
> +                */
> +               tee_dev_ctx_put(optee->notif.ctx);
> +       }
> +
>         kfree(optee->notif.bitmap);
>  }
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 81ff593ac4ec..35970932de34 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -291,6 +291,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
> @@ -298,6 +305,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 7dc058d008b2..62365912a70b 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
>

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sumit Garg]

Hi Jens,

On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> 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 interrupt is used to notify the driver that there are asynchronous
> notifications pending.
>

It looks like a nice feature. I would like to get hands on with this.
Can I test this feature on Qemu?

-Sumit

> 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 (7):
>   docs: staging/tee.rst: add a section on OP-TEE notifications
>   dt-bindings: arm: Convert optee binding to json-schema
>   dt-bindings: arm: optee: add interrupt property
>   tee: fix put order in teedev_close_context()
>   tee: add tee_dev_open_helper() primitive
>   optee: separate notification functions
>   optee: add asynchronous notifications
>
>  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
>  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
>  Documentation/staging/tee.rst                 |  27 +++
>  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                        |  37 ++-
>  include/linux/tee_drv.h                       |  27 +++
>  14 files changed, 576 insertions(+), 155 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
>  create mode 100644 drivers/tee/optee/notif.c
>
> --
> 2.31.1
>

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Jens Wiklander]

Hi Sumit,

On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Jens,
>
> On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > 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 interrupt is used to notify the driver that there are asynchronous
> > notifications pending.
> >
>
> It looks like a nice feature. I would like to get hands on with this.
> Can I test this feature on Qemu?

Absolutely, you can get this into the normal OP-TEE development repo setup with:
repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
repo sync
Update optee_os with
https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
cd build
make all -j...
make run-only

If you type anything at the secure console you'll notice how it
changes behaviour once the Linux kernel has booted.

Cheers,
Jens

>
> -Sumit
>
> > 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 (7):
> >   docs: staging/tee.rst: add a section on OP-TEE notifications
> >   dt-bindings: arm: Convert optee binding to json-schema
> >   dt-bindings: arm: optee: add interrupt property
> >   tee: fix put order in teedev_close_context()
> >   tee: add tee_dev_open_helper() primitive
> >   optee: separate notification functions
> >   optee: add asynchronous notifications
> >
> >  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
> >  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
> >  Documentation/staging/tee.rst                 |  27 +++
> >  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                        |  37 ++-
> >  include/linux/tee_drv.h                       |  27 +++
> >  14 files changed, 576 insertions(+), 155 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> >  create mode 100644 drivers/tee/optee/notif.c
> >
> > --
> > 2.31.1
> >

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sumit Garg]

On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Jens,
> >
> > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > 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 interrupt is used to notify the driver that there are asynchronous
> > > notifications pending.
> > >
> >
> > It looks like a nice feature. I would like to get hands on with this.
> > Can I test this feature on Qemu?
>
> Absolutely, you can get this into the normal OP-TEE development repo setup with:
> repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> repo sync
> Update optee_os with
> https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> cd build
> make all -j...
> make run-only
>
> If you type anything at the secure console you'll notice how it
> changes behaviour once the Linux kernel has booted.
>

Thanks for sharing instructions as I now got some time to test and
deep dive into this feature. It looks like a pretty useful feature to
realize interrupt support in the secure world in its true sense. This
feature works for me as per your instructions.

I could recognise it's requirement from the time while I was playing
with secure timer interrupt support for OP-TEE RNG driver on
Developerbox. In that case I had to strip down the secure interrupt
handler to a minimum that would just collect entropy and dump into the
secure buffer. But with asynchronous notifications support, I could
add more functionality like entropy health tests in the bottom half
instead of doing those health tests while retrieving entropy from the
secure world.

Given that, have you explored the possibility to leverage SGI rather
than a platform specific SPI for notifying the normal world? If it's
possible to leverage Architecture specific SGI for this purpose then I
think this feature will come automatically enabled for every platform
without the need to reserve a platform specific SPI.

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > 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 (7):
> > >   docs: staging/tee.rst: add a section on OP-TEE notifications
> > >   dt-bindings: arm: Convert optee binding to json-schema
> > >   dt-bindings: arm: optee: add interrupt property
> > >   tee: fix put order in teedev_close_context()
> > >   tee: add tee_dev_open_helper() primitive
> > >   optee: separate notification functions
> > >   optee: add asynchronous notifications
> > >
> > >  .../bindings/arm/firmware/linaro,optee-tz.txt |  31 ---
> > >  .../arm/firmware/linaro,optee-tz.yaml         |  57 +++++
> > >  Documentation/staging/tee.rst                 |  27 +++
> > >  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                        |  37 ++-
> > >  include/linux/tee_drv.h                       |  27 +++
> > >  14 files changed, 576 insertions(+), 155 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> > >  create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > >  create mode 100644 drivers/tee/optee/notif.c
> > >
> > > --
> > > 2.31.1
> > >

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Marc Zyngier]

On Tue, 06 Jul 2021 08:25:26 +0100,
Sumit Garg <sumit.garg@linaro.org> wrote:
> 
> On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> > On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Jens,
> > >
> > > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > 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 interrupt is used to notify the driver that there are asynchronous
> > > > notifications pending.
> > > >
> > >
> > > It looks like a nice feature. I would like to get hands on with this.
> > > Can I test this feature on Qemu?
> >
> > Absolutely, you can get this into the normal OP-TEE development repo setup with:
> > repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> > repo sync
> > Update optee_os with
> > https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> > Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> > cd build
> > make all -j...
> > make run-only
> >
> > If you type anything at the secure console you'll notice how it
> > changes behaviour once the Linux kernel has booted.
> >
> 
> Thanks for sharing instructions as I now got some time to test and
> deep dive into this feature. It looks like a pretty useful feature to
> realize interrupt support in the secure world in its true sense. This
> feature works for me as per your instructions.
> 
> I could recognise it's requirement from the time while I was playing
> with secure timer interrupt support for OP-TEE RNG driver on
> Developerbox. In that case I had to strip down the secure interrupt
> handler to a minimum that would just collect entropy and dump into the
> secure buffer. But with asynchronous notifications support, I could
> add more functionality like entropy health tests in the bottom half
> instead of doing those health tests while retrieving entropy from the
> secure world.
> 
> Given that, have you explored the possibility to leverage SGI rather
> than a platform specific SPI for notifying the normal world? If it's
> possible to leverage Architecture specific SGI for this purpose then I

What does "Architecture specific SGI" mean?

> think this feature will come automatically enabled for every platform
> without the need to reserve a platform specific SPI.

That old chestnut again...

- How do you discover that the secure side has graced you with a
  Group-1 SGI (no, you can't use one of the first 8)? for both DT and
  ACPI?

- How do you find which CPUs are targeted by this SGI? All? One? A
  subset? What is the expected behaviour with CPU hotplug? How can the
  NS side (Linux) can inform the secure side about the CPUs it wants
  to use?

- Is there any case where you would instead need a level interrupt
  (which a SGI cannot provide)?

In general, cross world SGIs are a really bad idea. Yes, some people
like them. I still think they are misguided, and I don't intend to
provide a generic request interface for this.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sumit Garg]

Hi Marc,

On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 06 Jul 2021 08:25:26 +0100,
> Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Thu, 17 Jun 2021 at 11:40, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Thu, Jun 17, 2021 at 6:33 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Jens,
> > > >
> > > > On Wed, 16 Jun 2021 at 16:07, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > 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 interrupt is used to notify the driver that there are asynchronous
> > > > > notifications pending.
> > > > >
> > > >
> > > > It looks like a nice feature. I would like to get hands on with this.
> > > > Can I test this feature on Qemu?
> > >
> > > Absolutely, you can get this into the normal OP-TEE development repo setup with:
> > > repo init -u https://github.com/OP-TEE/manifest.git -m default.xml
> > > repo sync
> > > Update optee_os with
> > > https://github.com/jenswi-linaro/optee_os/tree/async_notif_v2
> > > Update linux with https://github.com/jenswi-linaro/linux-1/tree/async_notif_v2
> > > cd build
> > > make all -j...
> > > make run-only
> > >
> > > If you type anything at the secure console you'll notice how it
> > > changes behaviour once the Linux kernel has booted.
> > >
> >
> > Thanks for sharing instructions as I now got some time to test and
> > deep dive into this feature. It looks like a pretty useful feature to
> > realize interrupt support in the secure world in its true sense. This
> > feature works for me as per your instructions.
> >
> > I could recognise it's requirement from the time while I was playing
> > with secure timer interrupt support for OP-TEE RNG driver on
> > Developerbox. In that case I had to strip down the secure interrupt
> > handler to a minimum that would just collect entropy and dump into the
> > secure buffer. But with asynchronous notifications support, I could
> > add more functionality like entropy health tests in the bottom half
> > instead of doing those health tests while retrieving entropy from the
> > secure world.
> >
> > Given that, have you explored the possibility to leverage SGI rather
> > than a platform specific SPI for notifying the normal world? If it's
> > possible to leverage Architecture specific SGI for this purpose then I
>
> What does "Architecture specific SGI" mean?
>

Here I meant that SGI is specific to Arm architecture and doesn't
require to be specific to per platform like an SPI.

> > think this feature will come automatically enabled for every platform
> > without the need to reserve a platform specific SPI.
>
> That old chestnut again...

Okay, can you provide reference to earlier threads?

>
> - How do you discover that the secure side has graced you with a
>   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
>   ACPI?

I think the secure world can be probed for that during the OP-TEE
driver probe. And I agree with you that the first 7 SGIs are already
pre-occupied and I guess you remember mine patch-set that tried to
leverage 8th SGI as pseudo NMI for kernel debug purposes.

So yes for this use-case, the secure world can reserve one of the
latter 8 SGIs (8 to 15) for cross world notification and I guess your
earlier work to make SGIs to be requested as normal IRQs should make
it easier to implement this as well.

>
> - How do you find which CPUs are targeted by this SGI? All? One? A
>   subset? What is the expected behaviour with CPU hotplug? How can the
>   NS side (Linux) can inform the secure side about the CPUs it wants
>   to use?

For the current OP-TEE use-case, I think targeting all CPUs would be
efficient. So wouldn't it be possible for the CPU which receives the
secure interrupt to raise that SGI to self that would in turn notify
the normal world (Linux) to create a thread for OP-TEE to do bottom
half processing?

>
> - Is there any case where you would instead need a level interrupt
>   (which a SGI cannot provide)?

I think SGI should be sufficient to suffice OP-TEE notifications use-case.

>
> In general, cross world SGIs are a really bad idea. Yes, some people
> like them. I still think they are misguided, and I don't intend to
> provide a generic request interface for this.

Okay, as I mentioned above having it specific to OP-TEE driver
requesting secure world donated SGI would work for you?

-Sumit

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Marc Zyngier]

Sumit,

On Tue, 06 Jul 2021 12:39:13 +0100,
Sumit Garg <sumit.garg@linaro.org> wrote:
> 
> Hi Marc,
> 
> On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 06 Jul 2021 08:25:26 +0100,
> > Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > I could recognise it's requirement from the time while I was playing
> > > with secure timer interrupt support for OP-TEE RNG driver on
> > > Developerbox. In that case I had to strip down the secure interrupt
> > > handler to a minimum that would just collect entropy and dump into the
> > > secure buffer. But with asynchronous notifications support, I could
> > > add more functionality like entropy health tests in the bottom half
> > > instead of doing those health tests while retrieving entropy from the
> > > secure world.
> > >
> > > Given that, have you explored the possibility to leverage SGI rather
> > > than a platform specific SPI for notifying the normal world? If it's
> > > possible to leverage Architecture specific SGI for this purpose then I
> >
> > What does "Architecture specific SGI" mean?
> >
> 
> Here I meant that SGI is specific to Arm architecture and doesn't
> require to be specific to per platform like an SPI.

SGIs are, by definition *software* specific (the clue is in the name),
and the architecture spec has *zero* say into what they are used for.
It says even less when it comes to specifying cross-world signalling.

> 
> > > think this feature will come automatically enabled for every platform
> > > without the need to reserve a platform specific SPI.
> >
> > That old chestnut again...
> 
> Okay, can you provide reference to earlier threads?

They show up every other year. Lore is your friend.

> 
> >
> > - How do you discover that the secure side has graced you with a
> >   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
> >   ACPI?
> 
> I think the secure world can be probed

How? With what guarantees?

> for that during the OP-TEE driver probe.

Oh, so it is only for the benefit of a single driver?

> And I agree with you that the first 7 SGIs are already
> pre-occupied and I guess you remember mine patch-set that tried to
> leverage 8th SGI as pseudo NMI for kernel debug purposes.

I do remember, and I'm definitely not keen on spending this last SGI
on this feature.

> So yes for this use-case, the secure world can reserve one of the
> latter 8 SGIs (8 to 15) for cross world notification and I guess your
> earlier work to make SGIs to be requested as normal IRQs should make
> it easier to implement this as well.
>
> >
> > - How do you find which CPUs are targeted by this SGI? All? One? A
> >   subset? What is the expected behaviour with CPU hotplug? How can the
> >   NS side (Linux) can inform the secure side about the CPUs it wants
> >   to use?
> 
> For the current OP-TEE use-case, I think targeting all CPUs would be
> efficient.

Efficient? How? Broadcast? One of N? Random?

> So wouldn't it be possible for the CPU which receives the
> secure interrupt to raise that SGI to self that would in turn notify
> the normal world (Linux) to create a thread for OP-TEE to do bottom
> half processing?

You are assuming that this is the way the NS side wants to work, and I
question this assumption.

> 
> >
> > - Is there any case where you would instead need a level interrupt
> >   (which a SGI cannot provide)?
> 
> I think SGI should be sufficient to suffice OP-TEE notifications use-case.

I don't care about OP-TEE. If you are proposing a contract between S
and NS, it has to be TEE and OS independent. That's how the
architecture works.

> >
> > In general, cross world SGIs are a really bad idea. Yes, some people
> > like them. I still think they are misguided, and I don't intend to
> > provide a generic request interface for this.
> 
> Okay, as I mentioned above having it specific to OP-TEE driver
> requesting secure world donated SGI would work for you?

No. I want a proper architecture between secure and non-secure that
explain how messages are conveyed between the two world, how
signalling is done, how CPU PM is handled, how targeting is
negotiated. And at the end of the day, this is starting to look a lot
like FFA.

If you want a custom OP-TEE hack, you don't need my blessing for
that. You'll even get to keep the pieces once it breaks. But if you
are going to invent a new universal way of signalling things across
world, you'd better start specifying things the right way, taking into
considerations systems where the interrupt controller doesn't allow
cross-world signalling.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sumit Garg]

On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
>
> Sumit,
>
> On Tue, 06 Jul 2021 12:39:13 +0100,
> Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Marc,
> >
> > On Tue, 6 Jul 2021 at 16:06, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Tue, 06 Jul 2021 08:25:26 +0100,
> > > Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > I could recognise it's requirement from the time while I was playing
> > > > with secure timer interrupt support for OP-TEE RNG driver on
> > > > Developerbox. In that case I had to strip down the secure interrupt
> > > > handler to a minimum that would just collect entropy and dump into the
> > > > secure buffer. But with asynchronous notifications support, I could
> > > > add more functionality like entropy health tests in the bottom half
> > > > instead of doing those health tests while retrieving entropy from the
> > > > secure world.
> > > >
> > > > Given that, have you explored the possibility to leverage SGI rather
> > > > than a platform specific SPI for notifying the normal world? If it's
> > > > possible to leverage Architecture specific SGI for this purpose then I
> > >
> > > What does "Architecture specific SGI" mean?
> > >
> >
> > Here I meant that SGI is specific to Arm architecture and doesn't
> > require to be specific to per platform like an SPI.
>
> SGIs are, by definition *software* specific (the clue is in the name),
> and the architecture spec has *zero* say into what they are used for.
> It says even less when it comes to specifying cross-world signalling.
>

Agree.

> >
> > > > think this feature will come automatically enabled for every platform
> > > > without the need to reserve a platform specific SPI.
> > >
> > > That old chestnut again...
> >
> > Okay, can you provide reference to earlier threads?
>
> They show up every other year. Lore is your friend.
>

Okay.

> >
> > >
> > > - How do you discover that the secure side has graced you with a
> > >   Group-1 SGI (no, you can't use one of the first 8)? for both DT and
> > >   ACPI?
> >
> > I think the secure world can be probed
>
> How? With what guarantees?
>

It can simply be a fast SMC call to OP-TEE to retrieve the SGI to be
used for notification using similar SMC as
OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE that Jens has used in this
patch-set.

I am not sure how that would fail as we do maintain backwards
compatibility with prior OP-TEE versions.

> > for that during the OP-TEE driver probe.
>
> Oh, so it is only for the benefit of a single driver?
>

Yeah.

> > And I agree with you that the first 7 SGIs are already
> > pre-occupied and I guess you remember mine patch-set that tried to
> > leverage 8th SGI as pseudo NMI for kernel debug purposes.
>
> I do remember, and I'm definitely not keen on spending this last SGI
> on this feature.

Agree and that's why we allowed that last SGI for debug purposes if it
is not used anywhere else. Let's keep this discussion to the
corresponding patch-set only as otherwise we would unnecessarily
derail discussion for this OP-TEE specific feature.

>
> > So yes for this use-case, the secure world can reserve one of the
> > latter 8 SGIs (8 to 15) for cross world notification and I guess your
> > earlier work to make SGIs to be requested as normal IRQs should make
> > it easier to implement this as well.
> >
> > >
> > > - How do you find which CPUs are targeted by this SGI? All? One? A
> > >   subset? What is the expected behaviour with CPU hotplug? How can the
> > >   NS side (Linux) can inform the secure side about the CPUs it wants
> > >   to use?
> >
> > For the current OP-TEE use-case, I think targeting all CPUs would be
> > efficient.
>
> Efficient? How? Broadcast? One of N? Random?
>

By efficient here I meant that we would enable that SGI for every CPU
rather than a subset so that any CPU which receives a secure interrupt
(PPI or SPI) would be able to raise this SGI to itself in order to
notify Linux to create a thread for OP-TEE.

> > So wouldn't it be possible for the CPU which receives the
> > secure interrupt to raise that SGI to self that would in turn notify
> > the normal world (Linux) to create a thread for OP-TEE to do bottom
> > half processing?
>
> You are assuming that this is the way the NS side wants to work, and I
> question this assumption.
>

Actually this is the way that Jens has implemented notifications among
Linux and OP-TEE using a SPI in this patch-set. The only difference
with SGI is that it's a per CPU interrupt.

> >
> > >
> > > - Is there any case where you would instead need a level interrupt
> > >   (which a SGI cannot provide)?
> >
> > I think SGI should be sufficient to suffice OP-TEE notifications use-case.
>
> I don't care about OP-TEE. If you are proposing a contract between S
> and NS, it has to be TEE and OS independent. That's how the
> architecture works.
>

Agree, here we are not proposing a common contract among the S and NS
world that every TEE (based on Arm TrustZone) will use to communicate
with REE (Linux in our case) but rather an OP-TEE specific
notifications feature that is built on top of OP-TEE specific ABIs.

And I can see your arguments coming from an FFA perspective but there
are platforms like the ones based on Armv7 which don't support FFA
ABI. Maybe Jens can elaborate how this feature will fit in when FFA
comes into picture?

> > >
> > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > like them. I still think they are misguided, and I don't intend to
> > > provide a generic request interface for this.
> >
> > Okay, as I mentioned above having it specific to OP-TEE driver
> > requesting secure world donated SGI would work for you?
>
> No. I want a proper architecture between secure and non-secure that
> explain how messages are conveyed between the two world, how
> signalling is done, how CPU PM is handled, how targeting is
> negotiated. And at the end of the day, this is starting to look a lot
> like FFA.

AFAIK when FFA comes in picture than OP-TEE will use the standard
interface provided by FFA ABIs but if FFA isn't supported by a
particular platform (eg. based on Armv7) then we need to rely on TEE
specific ABI like what OP-TEE currently provides:

1. how messages are conveyed between the two worlds -> OP-TEE specific
ABI (yielding SMC calls).
2. how signalling is done -> OP-TEE specific ABI (fast SMC calls).
3. how CPU PM is handled -> OP-TEE is notified on PSCI CPU ON, OFF and
SUSPEND calls.
4. how targeting is negotiated -> SGI would be targeted to the same
CPU which receives the secure interrupt (PPI or SPI).

>
> If you want a custom OP-TEE hack, you don't need my blessing for
> that. You'll even get to keep the pieces once it breaks. But if you
> are going to invent a new universal way of signalling things across
> world, you'd better start specifying things the right way, taking into
> considerations systems where the interrupt controller doesn't allow
> cross-world signalling.

As I mentioned above, this patch-set adds an OP-TEE specific
notifications feature. AFAIK, the interrupt controllers supported by
OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.

So given the explanation above, if you still think requesting an SGI
as an IRQ by drivers isn't allowed then I am fine with the approach
that Jens has already implemented in this patch-set to use platform
specific SPI.

-Sumit

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Jens Wiklander]

Hi,

On Wed, Jul 7, 2021 at 7:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> >
[snip]
> > > > - Is there any case where you would instead need a level interrupt
> > > >   (which a SGI cannot provide)?
> > >
> > > I think SGI should be sufficient to suffice OP-TEE notifications use-case.
> >
> > I don't care about OP-TEE. If you are proposing a contract between S
> > and NS, it has to be TEE and OS independent. That's how the
> > architecture works.
> >
>
> Agree, here we are not proposing a common contract among the S and NS
> world that every TEE (based on Arm TrustZone) will use to communicate
> with REE (Linux in our case) but rather an OP-TEE specific
> notifications feature that is built on top of OP-TEE specific ABIs.
>
> And I can see your arguments coming from an FFA perspective but there
> are platforms like the ones based on Armv7 which don't support FFA
> ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> comes into picture?

OP-TEE has one official ABI at the moment, the SMC based one. It's
about to get another one based on FF-A instead. The two ABIs will
never be used at the same time. It's a build time option for the
OP-TEE firmware to either use SMC or FF-A based communication.

The patches I've posted here concern the SMC based ABI. Asynchronous
notification in OP-TEE with a FF-A based ABI will use the notification
framework provided by FF-A instead to implement that counterpart
provided by these patches. So the OP-TEE driver here in the kernel
will use the FF-A framework in the kernel instead of registering an
interrupt handler directly.

Cheers,
Jens

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sudeep Holla]

Hi Sumit,

I was holding off you reply as I didn't have all the background on this.
Achin did mention that this is preparatory work for FFA notifications.
I did mention to him that this is more than that, it is custom extension
to address what FF-A notification is trying to in standard way.

I share same opinion as Marc Z.

On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:

[...]

> >
> > I don't care about OP-TEE. If you are proposing a contract between S
> > and NS, it has to be TEE and OS independent. That's how the
> > architecture works.
> >
> 
> Agree, here we are not proposing a common contract among the S and NS
> world that every TEE (based on Arm TrustZone) will use to communicate
> with REE (Linux in our case) but rather an OP-TEE specific
> notifications feature that is built on top of OP-TEE specific ABIs.
> 
> And I can see your arguments coming from an FFA perspective but there
> are platforms like the ones based on Armv7 which don't support FFA
> ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> comes into picture?
>

I can understand that but won't those platforms add the support both in
the kernel(current series) and secure world to address notifications.
While you could argue that it is small extension to what is already present
but I prefer they support FF-A is they need such a support instead of adding
custom mechanisms. It is hard to maintain and each vendor will deviate
from this custom mechanism and soon we will have bunch of them to handle.

> > > >
> > > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > > like them. I still think they are misguided, and I don't intend to
> > > > provide a generic request interface for this.
> > >
> > > Okay, as I mentioned above having it specific to OP-TEE driver
> > > requesting secure world donated SGI would work for you?
> >
> > No. I want a proper architecture between secure and non-secure that
> > explain how messages are conveyed between the two world, how
> > signalling is done, how CPU PM is handled, how targeting is
> > negotiated. And at the end of the day, this is starting to look a lot
> > like FFA.
> 
> AFAIK when FFA comes in picture than OP-TEE will use the standard
> interface provided by FFA ABIs but if FFA isn't supported by a
> particular platform (eg. based on Armv7) then we need to rely on TEE
> specific ABI like what OP-TEE currently provides:
>

Who are asking for this ? Can we ask them to migrate to FF-A if this
(new) notification support is needed on their platforms ? It is help to
know the requesters so that they can be included in FF-A spec discussions.

> > that. You'll even get to keep the pieces once it breaks. But if you
> > are going to invent a new universal way of signalling things across
> > world, you'd better start specifying things the right way, taking into
> > considerations systems where the interrupt controller doesn't allow
> > cross-world signalling.
> 
> As I mentioned above, this patch-set adds an OP-TEE specific
> notifications feature. AFAIK, the interrupt controllers supported by
> OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.
> 
> So given the explanation above, if you still think requesting an SGI
> as an IRQ by drivers isn't allowed then I am fine with the approach
> that Jens has already implemented in this patch-set to use platform
> specific SPI.
>

And I assume these platforms in question have SPI to spare and way to
trigger it from secure world ?

-- 
Regards,
Sudeep

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sumit Garg]

Hi Sudeep,

On Wed, 7 Jul 2021 at 23:22, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi Sumit,
>
> I was holding off you reply as I didn't have all the background on this.
> Achin did mention that this is preparatory work for FFA notifications.
> I did mention to him that this is more than that, it is custom extension
> to address what FF-A notification is trying to in standard way.
>
> I share same opinion as Marc Z.
>
> On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
>
> [...]
>
> > >
> > > I don't care about OP-TEE. If you are proposing a contract between S
> > > and NS, it has to be TEE and OS independent. That's how the
> > > architecture works.
> > >
> >
> > Agree, here we are not proposing a common contract among the S and NS
> > world that every TEE (based on Arm TrustZone) will use to communicate
> > with REE (Linux in our case) but rather an OP-TEE specific
> > notifications feature that is built on top of OP-TEE specific ABIs.
> >
> > And I can see your arguments coming from an FFA perspective but there
> > are platforms like the ones based on Armv7 which don't support FFA
> > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > comes into picture?
> >
>
> I can understand that but won't those platforms add the support both in
> the kernel(current series) and secure world to address notifications.

Agree.

> While you could argue that it is small extension to what is already present
> but I prefer they support FF-A is they need such a support instead of adding
> custom mechanisms. It is hard to maintain and each vendor will deviate
> from this custom mechanism and soon we will have bunch of them to handle.
>

I haven't had a deep dive into FF-A spec, maybe you can clarify on the
following queries regarding Armv7 compatibility:
- As you may be aware, secure monitor implementation on Armv7 is
tightly coupled to trusted OS (part of the same code base), so would
you like each trusted OS vendor to implement a common FF-A interface?
- IIRC, FF-A spec has the notion of multiple secure partitions, are
those supported on Armv7? If yes then how?

> > > > >
> > > > > In general, cross world SGIs are a really bad idea. Yes, some people
> > > > > like them. I still think they are misguided, and I don't intend to
> > > > > provide a generic request interface for this.
> > > >
> > > > Okay, as I mentioned above having it specific to OP-TEE driver
> > > > requesting secure world donated SGI would work for you?
> > >
> > > No. I want a proper architecture between secure and non-secure that
> > > explain how messages are conveyed between the two world, how
> > > signalling is done, how CPU PM is handled, how targeting is
> > > negotiated. And at the end of the day, this is starting to look a lot
> > > like FFA.
> >
> > AFAIK when FFA comes in picture than OP-TEE will use the standard
> > interface provided by FFA ABIs but if FFA isn't supported by a
> > particular platform (eg. based on Armv7) then we need to rely on TEE
> > specific ABI like what OP-TEE currently provides:
> >
>
> Who are asking for this ? Can we ask them to migrate to FF-A if this
> (new) notification support is needed on their platforms ? It is help to
> know the requesters so that they can be included in FF-A spec discussions.
>

I would let Jens answer that.

> > > that. You'll even get to keep the pieces once it breaks. But if you
> > > are going to invent a new universal way of signalling things across
> > > world, you'd better start specifying things the right way, taking into
> > > considerations systems where the interrupt controller doesn't allow
> > > cross-world signalling.
> >
> > As I mentioned above, this patch-set adds an OP-TEE specific
> > notifications feature. AFAIK, the interrupt controllers supported by
> > OP-TEE (GICv2, GICv3 etc.) don't restrict cross-world signaling.
> >
> > So given the explanation above, if you still think requesting an SGI
> > as an IRQ by drivers isn't allowed then I am fine with the approach
> > that Jens has already implemented in this patch-set to use platform
> > specific SPI.
> >
>
> And I assume these platforms in question have SPI to spare and way to
> trigger it from secure world ?
>

Yeah, that is the requirement on the platform if we rely on SPI (Qemu
test example [1]) which wouldn't be the case if we use secure world
donated SGI.

BTW, is this notification mechanism discussed in the case of FF-A? If
yes, can you throw some light on that?

[1] https://github.com/jenswi-linaro/optee_os/commit/9007f8184deb9b7995da8d590779cb3ba2783394

-Sumit

> --
> Regards,
> Sudeep

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Etienne CARRIERE]

Hello Sudeep and all,

On Wed, 7 Jul 2021 at 19:52, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi Sumit,
>
> I was holding off you reply as I didn't have all the background on this.
> Achin did mention that this is preparatory work for FFA notifications.
> I did mention to him that this is more than that, it is custom extension
> to address what FF-A notification is trying to in standard way.
>
> I share same opinion as Marc Z.
>
> On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
>
> [...]
>
> > >
> > > I don't care about OP-TEE. If you are proposing a contract between S
> > > and NS, it has to be TEE and OS independent. That's how the
> > > architecture works.
> > >
> >
> > Agree, here we are not proposing a common contract among the S and NS
> > world that every TEE (based on Arm TrustZone) will use to communicate
> > with REE (Linux in our case) but rather an OP-TEE specific
> > notifications feature that is built on top of OP-TEE specific ABIs.
> >
> > And I can see your arguments coming from an FFA perspective but there
> > are platforms like the ones based on Armv7 which don't support FFA
> > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > comes into picture?
> >
>
> I can understand that but won't those platforms add the support both in
> the kernel(current series) and secure world to address notifications.
> While you could argue that it is small extension to what is already present
> but I prefer they support FF-A is they need such a support instead of adding
> custom mechanisms. It is hard to maintain and each vendor will deviate
> from this custom mechanism and soon we will have bunch of them to handle.


There exist armv7-a platforms that expect OP-TEE notification support and will not move the FF-A, like the stm32mp15. This platform won't move to FF-A mainly due to the memory cost of the added SPM layer and the device physical constraints.
We have a usecase for OP-TEE notification. We're working on the integration of an SCMI server in OP-TEE. SCMI notification is a feature needed is this scope and it requires OP-TEE async notification means as those proposed here.

This OP-TEE async notif also brings a lot of value in OP-TEE as it allows a OP-TEE secure thread (i.e. executing a trusted application service) to gently wait on a secure interrupt (as a slow bus transaction completion or many other usecase) with the CPU relaxed. This support is provided by the proposed series. I believe existing device should be able to leverage this OP-TEE feature without needing their OP-TEE to move to the new FF-A interface.

Regards,
Etienne

>
> [...]

ST Restricted

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sudeep Holla]

On Fri, Jul 09, 2021 at 08:05:57AM +0000, Etienne CARRIERE wrote:
> Hello Sudeep and all,
> 
> On Wed, 7 Jul 2021 at 19:52, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Hi Sumit,
> >
> > I was holding off you reply as I didn't have all the background on this.
> > Achin did mention that this is preparatory work for FFA notifications.
> > I did mention to him that this is more than that, it is custom extension
> > to address what FF-A notification is trying to in standard way.
> >
> > I share same opinion as Marc Z.
> >
> > On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> >
> > [...]
> >
> > > >
> > > > I don't care about OP-TEE. If you are proposing a contract between S
> > > > and NS, it has to be TEE and OS independent. That's how the
> > > > architecture works.
> > > >
> > >
> > > Agree, here we are not proposing a common contract among the S and NS
> > > world that every TEE (based on Arm TrustZone) will use to communicate
> > > with REE (Linux in our case) but rather an OP-TEE specific
> > > notifications feature that is built on top of OP-TEE specific ABIs.
> > >
> > > And I can see your arguments coming from an FFA perspective but there
> > > are platforms like the ones based on Armv7 which don't support FFA
> > > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > > comes into picture?
> > >
> >
> > I can understand that but won't those platforms add the support both in
> > the kernel(current series) and secure world to address notifications.
> > While you could argue that it is small extension to what is already present
> > but I prefer they support FF-A is they need such a support instead of adding
> > custom mechanisms. It is hard to maintain and each vendor will deviate
> > from this custom mechanism and soon we will have bunch of them to handle.
>
> There exist armv7-a platforms that expect OP-TEE notification support and
> will not move the FF-A, like the stm32mp15. This platform won't move to FF-A
> mainly due to the memory cost of the added SPM layer and the device physical
> constraints.

Fair enough on the use-case and the analysis for not being able to use FF-A.
As you may already know it doesn't simply this problem. This has been
discussed for years and FF-A was assumed to be the solution when FF-A
spec work started.

> We have a usecase for OP-TEE notification. We're working on the integration
> of an SCMI server in OP-TEE. SCMI notification is a feature needed is this
> scope and it requires OP-TEE async notification means as those proposed
> here.
>

I am aware of this use-case, I understand. But I can only share rants
which I know doesn't help much.

> This OP-TEE async notif also brings a lot of value in OP-TEE as it allows a
> OP-TEE secure thread (i.e. executing a trusted application service) to
> gently wait on a secure interrupt (as a slow bus transaction completion or
> many other usecase) with the CPU relaxed. This support is provided by the
> proposed series. I believe existing device should be able to leverage this
> OP-TEE feature without needing their OP-TEE to move to the new FF-A
> interface.
>

While I agree these are nice to have in OPTEE, the timing is just odd.

We are trying hard to push FF-A as standard solution to address all such
issues that couldn't be solved with OPTEE + DT, now we are back to address
the same in parallel to FF-A.

--
Regards,
Sudeep

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Jens Wiklander]

On Tue, Jul 13, 2021 at 1:12 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Jul 09, 2021 at 08:05:57AM +0000, Etienne CARRIERE wrote:
> > Hello Sudeep and all,
> >
> > On Wed, 7 Jul 2021 at 19:52, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > Hi Sumit,
> > >
> > > I was holding off you reply as I didn't have all the background on this.
> > > Achin did mention that this is preparatory work for FFA notifications.
> > > I did mention to him that this is more than that, it is custom extension
> > > to address what FF-A notification is trying to in standard way.

Are you suggesting that we should use a hybrid implementation with
FF-A for notifications and keep the rest as is for armv7-a?

> > >
> > > I share same opinion as Marc Z.

From what I've read in this thread this has mainly been about using
SGI notification and not whether asynchronous notification from OP-TEE
on non-FF-A systems is good or bad. I assume Sumit was asking about
SGI to find out why that wasn't used. This patch set uses SPI.

> > >
> > > On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > > > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > [...]
> > >
> > > > >
> > > > > I don't care about OP-TEE. If you are proposing a contract between S
> > > > > and NS, it has to be TEE and OS independent. That's how the
> > > > > architecture works.
> > > > >
> > > >
> > > > Agree, here we are not proposing a common contract among the S and NS
> > > > world that every TEE (based on Arm TrustZone) will use to communicate
> > > > with REE (Linux in our case) but rather an OP-TEE specific
> > > > notifications feature that is built on top of OP-TEE specific ABIs.
> > > >
> > > > And I can see your arguments coming from an FFA perspective but there
> > > > are platforms like the ones based on Armv7 which don't support FFA
> > > > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > > > comes into picture?
> > > >
> > >
> > > I can understand that but won't those platforms add the support both in
> > > the kernel(current series) and secure world to address notifications.
> > > While you could argue that it is small extension to what is already present
> > > but I prefer they support FF-A is they need such a support instead of adding
> > > custom mechanisms. It is hard to maintain and each vendor will deviate
> > > from this custom mechanism and soon we will have bunch of them to handle.

Regarding deviation, are we still talking about the OP-TEE driver? So
far I haven't seen any vendor extensions at all in that driver.

> >
> > There exist armv7-a platforms that expect OP-TEE notification support and
> > will not move the FF-A, like the stm32mp15. This platform won't move to FF-A
> > mainly due to the memory cost of the added SPM layer and the device physical
> > constraints.
>
> Fair enough on the use-case and the analysis for not being able to use FF-A.
> As you may already know it doesn't simply this problem. This has been
> discussed for years and FF-A was assumed to be the solution when FF-A
> spec work started.
>
> > We have a usecase for OP-TEE notification. We're working on the integration
> > of an SCMI server in OP-TEE. SCMI notification is a feature needed is this
> > scope and it requires OP-TEE async notification means as those proposed
> > here.
> >
>
> I am aware of this use-case, I understand. But I can only share rants
> which I know doesn't help much.
>
> > This OP-TEE async notif also brings a lot of value in OP-TEE as it allows a
> > OP-TEE secure thread (i.e. executing a trusted application service) to
> > gently wait on a secure interrupt (as a slow bus transaction completion or
> > many other usecase) with the CPU relaxed. This support is provided by the
> > proposed series. I believe existing device should be able to leverage this
> > OP-TEE feature without needing their OP-TEE to move to the new FF-A
> > interface.
> >
>
> While I agree these are nice to have in OPTEE, the timing is just odd.
>
> We are trying hard to push FF-A as standard solution to address all such
> issues that couldn't be solved with OPTEE + DT, now we are back to address
> the same in parallel to FF-A.

It's not exactly the same since the primary target here is armv7-a
where introducing FF-A isn't an obvious choice in all cases. For
OP-TEE armv7-a is special in the way that all secure world processing
is handled by OP-TEE. The internal secure monitor already takes care
of what's implemented in TF-A at EL3 for armv8-a.

This isn't meant to compete with FF-A, it's to make sure that the
OP-TEE armv7-a user base isn't left behind. This doesn't rule out FF-A
support for armv7-a for those prepared to take that step.

Cheers,
Jens

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Sudeep Holla]

On Tue, Jul 20, 2021 at 08:45:59AM +0200, Jens Wiklander wrote:
> On Tue, Jul 13, 2021 at 1:12 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Jul 09, 2021 at 08:05:57AM +0000, Etienne CARRIERE wrote:
> > > Hello Sudeep and all,
> > >
> > > On Wed, 7 Jul 2021 at 19:52, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > I was holding off you reply as I didn't have all the background on this.
> > > > Achin did mention that this is preparatory work for FFA notifications.
> > > > I did mention to him that this is more than that, it is custom extension
> > > > to address what FF-A notification is trying to in standard way.
>
> Are you suggesting that we should use a hybrid implementation with
> FF-A for notifications and keep the rest as is for armv7-a?
>

No I was just mentioning that this patch series addresses notifications from
secure world(optee in this case) which is very similar to what FF-A is trying
to address too.

Anyways, you brought up interesting idea of hybrid model, it would be good if
that is possible and the specification allows for that. I don't think it does
in the current form, may need some amendments to allow that I think.

> > > >
> > > > I share same opinion as Marc Z.
>
> From what I've read in this thread this has mainly been about using
> SGI notification and not whether asynchronous notification from OP-TEE
> on non-FF-A systems is good or bad. I assume Sumit was asking about
> SGI to find out why that wasn't used. This patch set uses SPI.
>

I understand. I was trying(ineffectively) to tell why it is not so trivial
to use SGI and how FF-A is enabling that.

On SPI, so it is expected that platform has SPI available for this ?

> > > >
> > > > On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > > > > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > [...]
> > > >
> > > > > >
> > > > > > I don't care about OP-TEE. If you are proposing a contract between S
> > > > > > and NS, it has to be TEE and OS independent. That's how the
> > > > > > architecture works.
> > > > > >
> > > > >
> > > > > Agree, here we are not proposing a common contract among the S and NS
> > > > > world that every TEE (based on Arm TrustZone) will use to communicate
> > > > > with REE (Linux in our case) but rather an OP-TEE specific
> > > > > notifications feature that is built on top of OP-TEE specific ABIs.
> > > > >
> > > > > And I can see your arguments coming from an FFA perspective but there
> > > > > are platforms like the ones based on Armv7 which don't support FFA
> > > > > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > > > > comes into picture?
> > > > >
> > > >
> > > > I can understand that but won't those platforms add the support both in
> > > > the kernel(current series) and secure world to address notifications.
> > > > While you could argue that it is small extension to what is already present
> > > > but I prefer they support FF-A is they need such a support instead of adding
> > > > custom mechanisms. It is hard to maintain and each vendor will deviate
> > > > from this custom mechanism and soon we will have bunch of them to handle.
>
> Regarding deviation, are we still talking about the OP-TEE driver? So
> far I haven't seen any vendor extensions at all in that driver.
>

Yes, I was referring to addition of notification support in both worlds.
I was trying to emphasize that both OPTEE and FF-A needs changes in the
secure world. OPTEE changes could be small compared to starting with FF-A
but it may result in deviation in notification hadling(in both worlds).

> > >
> > > There exist armv7-a platforms that expect OP-TEE notification support and
> > > will not move the FF-A, like the stm32mp15. This platform won't move to FF-A
> > > mainly due to the memory cost of the added SPM layer and the device physical
> > > constraints.
> >
> > Fair enough on the use-case and the analysis for not being able to use FF-A.
> > As you may already know it doesn't simply this problem. This has been
> > discussed for years and FF-A was assumed to be the solution when FF-A
> > spec work started.
> >
> > > We have a usecase for OP-TEE notification. We're working on the integration
> > > of an SCMI server in OP-TEE. SCMI notification is a feature needed is this
> > > scope and it requires OP-TEE async notification means as those proposed
> > > here.
> > >
> >
> > I am aware of this use-case, I understand. But I can only share rants
> > which I know doesn't help much.
> >
> > > This OP-TEE async notif also brings a lot of value in OP-TEE as it allows a
> > > OP-TEE secure thread (i.e. executing a trusted application service) to
> > > gently wait on a secure interrupt (as a slow bus transaction completion or
> > > many other usecase) with the CPU relaxed. This support is provided by the
> > > proposed series. I believe existing device should be able to leverage this
> > > OP-TEE feature without needing their OP-TEE to move to the new FF-A
> > > interface.
> > >
> >
> > While I agree these are nice to have in OPTEE, the timing is just odd.
> >
> > We are trying hard to push FF-A as standard solution to address all such
> > issues that couldn't be solved with OPTEE + DT, now we are back to address
> > the same in parallel to FF-A.
>
> It's not exactly the same since the primary target here is armv7-a
> where introducing FF-A isn't an obvious choice in all cases. For
> OP-TEE armv7-a is special in the way that all secure world processing
> is handled by OP-TEE. The internal secure monitor already takes care
> of what's implemented in TF-A at EL3 for armv8-a.
>

Fair enough.

> This isn't meant to compete with FF-A, it's to make sure that the
> OP-TEE armv7-a user base isn't left behind. This doesn't rule out FF-A
> support for armv7-a for those prepared to take that step.
>

Sure, as long as that is conveyed to the adopters of this, it should be
fine. Do you have plans to disable this feature for armv8-a ? I see that
as safe approach to avoid any kind of conflicts.

I just don't want similar arguments used as excuse on armv8-a.

--
Regards,
Sudeep

Re: [PATCH v2 0/7] Asynchronous notifications from secure world

[Jens Wiklander]

On Tue, Jul 20, 2021 at 9:51 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jul 20, 2021 at 08:45:59AM +0200, Jens Wiklander wrote:
> > On Tue, Jul 13, 2021 at 1:12 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Fri, Jul 09, 2021 at 08:05:57AM +0000, Etienne CARRIERE wrote:
> > > > Hello Sudeep and all,
> > > >
> > > > On Wed, 7 Jul 2021 at 19:52, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > I was holding off you reply as I didn't have all the background on this.
> > > > > Achin did mention that this is preparatory work for FFA notifications.
> > > > > I did mention to him that this is more than that, it is custom extension
> > > > > to address what FF-A notification is trying to in standard way.
> >
> > Are you suggesting that we should use a hybrid implementation with
> > FF-A for notifications and keep the rest as is for armv7-a?
> >
>
> No I was just mentioning that this patch series addresses notifications from
> secure world(optee in this case) which is very similar to what FF-A is trying
> to address too.

I was hoping that it would be easy to integrate with FF-A in the
OP-TEE driver if it was done in this way.

>
> Anyways, you brought up interesting idea of hybrid model, it would be good if
> that is possible and the specification allows for that. I don't think it does
> in the current form, may need some amendments to allow that I think.

Agree, the scheduling part will need something too.

>
> > > > >
> > > > > I share same opinion as Marc Z.
> >
> > From what I've read in this thread this has mainly been about using
> > SGI notification and not whether asynchronous notification from OP-TEE
> > on non-FF-A systems is good or bad. I assume Sumit was asking about
> > SGI to find out why that wasn't used. This patch set uses SPI.
> >
>
> I understand. I was trying(ineffectively) to tell why it is not so trivial
> to use SGI and how FF-A is enabling that.

OK, thanks for clarifying.

>
> On SPI, so it is expected that platform has SPI available for this ?

Yes, either that or some other source usable with platform_get_irq().

>
> > > > >
> > > > > On Wed, Jul 07, 2021 at 11:22:23AM +0530, Sumit Garg wrote:
> > > > > > On Tue, 6 Jul 2021 at 18:16, Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > >
> > > > > > > I don't care about OP-TEE. If you are proposing a contract between S
> > > > > > > and NS, it has to be TEE and OS independent. That's how the
> > > > > > > architecture works.
> > > > > > >
> > > > > >
> > > > > > Agree, here we are not proposing a common contract among the S and NS
> > > > > > world that every TEE (based on Arm TrustZone) will use to communicate
> > > > > > with REE (Linux in our case) but rather an OP-TEE specific
> > > > > > notifications feature that is built on top of OP-TEE specific ABIs.
> > > > > >
> > > > > > And I can see your arguments coming from an FFA perspective but there
> > > > > > are platforms like the ones based on Armv7 which don't support FFA
> > > > > > ABI. Maybe Jens can elaborate how this feature will fit in when FFA
> > > > > > comes into picture?
> > > > > >
> > > > >
> > > > > I can understand that but won't those platforms add the support both in
> > > > > the kernel(current series) and secure world to address notifications.
> > > > > While you could argue that it is small extension to what is already present
> > > > > but I prefer they support FF-A is they need such a support instead of adding
> > > > > custom mechanisms. It is hard to maintain and each vendor will deviate
> > > > > from this custom mechanism and soon we will have bunch of them to handle.
> >
> > Regarding deviation, are we still talking about the OP-TEE driver? So
> > far I haven't seen any vendor extensions at all in that driver.
> >
>
> Yes, I was referring to addition of notification support in both worlds.
> I was trying to emphasize that both OPTEE and FF-A needs changes in the
> secure world. OPTEE changes could be small compared to starting with FF-A
> but it may result in deviation in notification hadling(in both worlds).

Yes they will be different, but they will also be either or. In a FF-A
system it wouldn't make sense to use these OP-TEE notifications. In
fact it wouldn't be possible since it can only be enabled if probed
via the raw SMCs based interface.

>
> > > >
> > > > There exist armv7-a platforms that expect OP-TEE notification support and
> > > > will not move the FF-A, like the stm32mp15. This platform won't move to FF-A
> > > > mainly due to the memory cost of the added SPM layer and the device physical
> > > > constraints.
> > >
> > > Fair enough on the use-case and the analysis for not being able to use FF-A.
> > > As you may already know it doesn't simply this problem. This has been
> > > discussed for years and FF-A was assumed to be the solution when FF-A
> > > spec work started.
> > >
> > > > We have a usecase for OP-TEE notification. We're working on the integration
> > > > of an SCMI server in OP-TEE. SCMI notification is a feature needed is this
> > > > scope and it requires OP-TEE async notification means as those proposed
> > > > here.
> > > >
> > >
> > > I am aware of this use-case, I understand. But I can only share rants
> > > which I know doesn't help much.
> > >
> > > > This OP-TEE async notif also brings a lot of value in OP-TEE as it allows a
> > > > OP-TEE secure thread (i.e. executing a trusted application service) to
> > > > gently wait on a secure interrupt (as a slow bus transaction completion or
> > > > many other usecase) with the CPU relaxed. This support is provided by the
> > > > proposed series. I believe existing device should be able to leverage this
> > > > OP-TEE feature without needing their OP-TEE to move to the new FF-A
> > > > interface.
> > > >
> > >
> > > While I agree these are nice to have in OPTEE, the timing is just odd.
> > >
> > > We are trying hard to push FF-A as standard solution to address all such
> > > issues that couldn't be solved with OPTEE + DT, now we are back to address
> > > the same in parallel to FF-A.
> >
> > It's not exactly the same since the primary target here is armv7-a
> > where introducing FF-A isn't an obvious choice in all cases. For
> > OP-TEE armv7-a is special in the way that all secure world processing
> > is handled by OP-TEE. The internal secure monitor already takes care
> > of what's implemented in TF-A at EL3 for armv8-a.
> >
>
> Fair enough.
>
> > This isn't meant to compete with FF-A, it's to make sure that the
> > OP-TEE armv7-a user base isn't left behind. This doesn't rule out FF-A
> > support for armv7-a for those prepared to take that step.
> >
>
> Sure, as long as that is conveyed to the adopters of this, it should be
> fine. Do you have plans to disable this feature for armv8-a ? I see that
> as safe approach to avoid any kind of conflicts.

I'm not so keen on artificial limitations that encourage strange
workarounds. I'm not sure what would be the best way of disabling this
for armv8-a, it would for instance still make sense for armv8-a
aarch32.

>
> I just don't want similar arguments used as excuse on armv8-a.

As long as there are no technical arguments it shouldn't happen.

Cheers,
Jens