Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1561818 > unrolled thread

[PATCH v14 0/5] generic TEE subsystem

Back to article view | Back to linux.kernel

Contents

  [PATCH v14 0/5] generic TEE subsystem Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-18 15:00 +0100
    [PATCH v14 4/5] Documentation: tee subsystem and op-tee driver Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-18 15:10 +0100
      Re: [PATCH v14 4/5] Documentation: tee subsystem and op-tee driver Scott Branden <scott.branden@broadcom.com> - 2017-01-19 01:00 +0100
    Re: [PATCH v14 3/5] tee: add OP-TEE driver Arnd Bergmann <arnd@arndb.de> - 2017-01-18 17:50 +0100
      Re: [PATCH v14 3/5] tee: add OP-TEE driver Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-19 16:00 +0100
        Re: [PATCH v14 3/5] tee: add OP-TEE driver Arnd Bergmann <arnd@arndb.de> - 2017-01-20 18:00 +0100
          Re: [PATCH v14 3/5] tee: add OP-TEE driver Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-23 10:10 +0100
            Re: [PATCH v14 3/5] tee: add OP-TEE driver Arnd Bergmann <arnd@arndb.de> - 2017-01-23 17:20 +0100
              Re: [PATCH v14 3/5] tee: add OP-TEE driver Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-24 14:00 +0100
                Re: [PATCH v14 3/5] tee: add OP-TEE driver Jens Wiklander <jens.wiklander@linaro.org> - 2017-01-25 10:50 +0100
                  Re: [PATCH v14 3/5] tee: add OP-TEE driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-25 11:10 +0100
                    Re: [PATCH v14 3/5] tee: add OP-TEE driver Arnd Bergmann <arnd@arndb.de> - 2017-01-25 12:10 +0100

#1561818 — [PATCH v14 0/5] generic TEE subsystem

Hi,

This patch set introduces a generic TEE subsystem. These patches are used
on HiKey in AOSP. It's currently supported by roughly 20 platforms
(https://github.com/OP-TEE/optee_os#3-platforms-supported), to name a few,
Renesas RCAR H3, Sequitur Labs CoreTEE, Wind River VxWorks etc.

I’m not sure how to progress this. I’ve had only positive feedback on the
previous v12 version. In the previous versions I’ve addressed all feedback
(which has been getting more and more minor) and I haven’t had any response
from Greg since February. Is there anything else we can be doing here to
help progress this?

The TEE subsystem will contain drivers for various TEE implementations. A
TEE (Trusted Execution Environment) is a trusted OS running in some secure
environment, for example, TrustZone on ARM CPUs, or a separate secure
co-processor etc.

Regarding use cases, TrustZone has traditionally been used for
offloading secure tasks to the secure world. Examples include: 
- Secure key handling where the OS may or may not have direct access to key
  material.
- E-commerce and payment technologies. Credentials, credit card numbers etc
  could be stored in a more secure environment.
- Trusted User Interface (TUI) to ensure that no-one can snoop PIN-codes
  etc.
- Secure boot to ensure that loaded binaries haven’t been tampered with.
  It’s not strictly needed for secure boot, but you could enhance security
  by leveraging a TEE during boot.
- Digital Rights Management (DRM), the studios provides content with
  different resolution depending on the security of the device. Higher
  security means higher resolution.

A TEE could also be used in existing and new technologies. For example IMA
(Integrity Measurement Architecture) which has been in the kernel for quite
a while. Today you can enhance security by using a TPM-chip to sign the IMA
measurement list. This is something that also has been done in practical
systems by leveraging a TEE.

Another example could be in 2-factor authentication which is becoming
increasingly more important. FIDO (https://fidoalliance.org) for example
are using public key cryptography in their 2-factor authentication standard
(U2F). With FIDO, a private and public key pair will be generated for every
site you visit and the private key should never leave the local device.
This is an example where you could use secure storage in a TEE for the
private key.

Today you will find a quite a few different out of tree implementations of
TEE drivers which tends to fragment the TEE ecosystem and development. We
think it would be a good idea to have a generic TEE driver integrated in
the kernel which would serve as a base for several different TEE solutions,
no matter if they are on-chip like TrustZone or if they are on a separate
crypto co-processor.

To develop this TEE subsystem we have been using the open source TEE called
OP-TEE (https://www.op-tee.org/) and therefore this would be the first TEE
solution supported by this new subsystem. OP-TEE is a GlobalPlatform
compliant TEE, however this TEE subsystem is not limited to only
GlobalPlatform TEEs, instead we have tried to design it so that it should
work with other TEE solutions also. Since the first version (2015 April) of
this patchset we’ve talked about it at Linaro Connect, GlobalPlatform
annual TEE conference and last time at Linux Plumbers, so we think there
has been both exposure and plenty of time to be able to get involved in
this work.

"tee: generic TEE subsystem" brings in the generic TEE subsystem which
helps when writing a driver for a specific TEE, for example, OP-TEE.

"tee: add OP-TEE driver" is an OP-TEE driver which uses the subsystem to do
its work.

This patch set has been prepared in cooperation with Javier González who
proposed "Generic TrustZone Driver in Linux Kernel" patches 28 Nov 2014,
https://lwn.net/Articles/623380/ . We've since then changed the scope to
TEE instead of TrustZone.

We have discussed the design on tee-dev@lists.linaro.org (archive at
https://lists.linaro.org/pipermail/tee-dev/) with people from other
companies, including Valentin Manea <valentin.manea@huawei.com>,
Emmanuel MICHEL <emmanuel.michel@st.com>,
Jean-michel DELORME <jean-michel.delorme@st.com>,
and Joakim Bech <joakim.bech@linaro.org>. Our main concern has been to
agree on something that is generic enough to support many different
TEEs while still keeping the interface together.

v14:
* Rebased on v4.10-rc4
* Fixed checkpatch warning in OPTEE_SMC_RETURN_IS_RPC() macro

v13:
* Rebased on v4.9-rc5
* Added Hikey DT patch
* Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey)
* Tested-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> (RCAR H3)
* Tested-by: Andrew F. Davis <afd@ti.com> 

v12-resend:
* Rebased on v4.9-rc2

v12:
* Rebased on v4.8-rc5
* Addressed review comments from Andrew F. Davis
* Removed Acked-by: Andreas Dannenberg <dannenberg@ti.com> as the
  mail bounces
* Bugfix possible null dereference in error cleanup path of
  optee_probe().
* Bugfix optee_from_msg_param() when calculating offset of memref
  into a shared memory object

v11:
* Rebased on v4.8-rc3
* Addressed review comments from Nishanth Menon
* Made the TEE framework available as a loadable module.
* Reviewed-by: Javier González <javier@javigon.com>
* Zeroes shared memory on allocation to avoid information leakage
* Links shared memory objects to context to avoid stealing of shared memory
  object from an unrelated process
* Allow RPC interruption if supplicant is unavailable

v10:
* Rebased on v4.7-rc1
* Addressed private review comments from Nishanth Menon
* Optee driver only accepts one supplicant process on the privileged device
* Optee driver avoids long delayed releases of shm objects
* Added more comments on functions and structs

v9:
* Rebased on v4.6-rc1
* Acked-by: Andreas Dannenberg <dannenberg@ti.com>
* Addressed comments from Al Viro how file descriptors are passed to
  user space
* Addressed comments from Randy Dunlap on documentation
* Changed license for include/uapi/linux/tee.h

v8:
* Rebased on v4.5-rc3
* dt/bindings: add bindings for optee
  Acked-by: Rob Herring <robh@kernel.org>
* Fixes build error for X86
* Fixes spell error in "dt/bindings: add bindings for optee"

v7:
* Rebased on v4.5-rc2
* Moved the ARM SMC Calling Convention support into a separate patch
  set, which is now merged

v6:
* Rebased on v4.3-rc7
* Changed smccc interface to let the compiler marshal most of the
  parameters
* Added ARCH64 capability for smccc interface
* Changed the PSCI firmware calls (both arm and arm64) to use the new
  generic smccc interface instead instead of own assembly functions.
* Move optee DT bindings to below arm/firmware
* Defines method for OP-TEE driver to call secure world in DT, smc or hvc
* Exposes implementation id of a TEE driver in sysfs
  to easily spawn corresponding tee-supplicant when device is ready
* Update OP-TEE Message Protocol to better cope with fragmented physical
  memory
* Read time directly from OP-TEE driver instead of forwarding the RPC
  request to tee-supplicant

v5:
* Replaced kref reference counting for the device with a size_t instead as
  the counter is always protected by a mutex

v4:
* Rebased on 4.1
* Redesigned the synchronization around entry exit of normal SMC
* Replaced rwsem on the driver instance with kref and completion since
  rwsem wasn't intended to be used in this way
* Expanded the TEE_IOCTL_PARAM_ATTR_TYPE_MASK to make room for
  future additional parameter types
* Documents TEE subsystem and OP-TEE driver
* Replaced TEE_IOC_CMD with TEE_IOC_OPEN_SESSION, TEE_IOC_INVOKE,
  TEE_IOC_CANCEL and TEE_IOC_CLOSE_SESSION
* DT bindings in a separate patch
* Assembly parts moved to arch/arm and arch/arm64 respectively, in a
  separate patch
* Redefined/clarified the meaning of OPTEE_SMC_SHM_CACHED
* Removed CMA usage to limit the scope of the patch set

v3:
* Rebased on 4.1-rc3 (dma_buf_export() API change)
* A couple of small sparse fixes
* Documents bindings for OP-TEE driver
* Updated MAINTAINERS

v2:
* Replaced the stubbed OP-TEE driver with a real OP-TEE driver
* Removed most APIs not needed by OP-TEE in current state
* Update Documentation/ioctl/ioctl-number.txt with correct path to tee.h
* Rename tee_shm_pool_alloc_cma() to tee_shm_pool_alloc()
* Moved tee.h into include/uapi/linux/
* Redefined tee.h IOCTL macros to be directly based on _IOR and friends
* Removed version info on the API to user space, a data blob which
  can contain an UUID is left for user space to be able to tell which
  protocol to use in TEE_IOC_CMD
* Changed user space exposed structures to only have types with __ prefix
* Dropped THIS_MODULE from tee_fops
* Reworked how the driver is registered and ref counted:
  - moved from using an embedded struct miscdevice to an embedded struct
    device.
  - uses an struct rw_semaphore as synchronization for driver detachment
  - uses alloc/register pattern from TPM

Thanks,
Jens

Jens Wiklander (4):
  dt/bindings: add bindings for optee
  tee: generic TEE subsystem
  tee: add OP-TEE driver
  Documentation: tee subsystem and op-tee driver

Jerome Forissier (1):
  arm64: dt: hikey: Add optee node

 Documentation/00-INDEX                             |   2 +
 .../bindings/arm/firmware/linaro,optee-tz.txt      |  31 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 Documentation/ioctl/ioctl-number.txt               |   1 +
 Documentation/tee.txt                              | 118 +++
 MAINTAINERS                                        |  13 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |   7 +
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/tee/Kconfig                                |  18 +
 drivers/tee/Makefile                               |   5 +
 drivers/tee/optee/Kconfig                          |   7 +
 drivers/tee/optee/Makefile                         |   5 +
 drivers/tee/optee/call.c                           | 435 ++++++++++
 drivers/tee/optee/core.c                           | 598 ++++++++++++++
 drivers/tee/optee/optee_msg.h                      | 435 ++++++++++
 drivers/tee/optee/optee_private.h                  | 185 +++++
 drivers/tee/optee/optee_smc.h                      | 450 ++++++++++
 drivers/tee/optee/rpc.c                            | 404 +++++++++
 drivers/tee/optee/supp.c                           | 273 +++++++
 drivers/tee/tee_core.c                             | 901 +++++++++++++++++++++
 drivers/tee/tee_private.h                          | 129 +++
 drivers/tee/tee_shm.c                              | 357 ++++++++
 drivers/tee/tee_shm_pool.c                         | 158 ++++
 include/linux/tee_drv.h                            | 278 +++++++
 include/uapi/linux/tee.h                           | 401 +++++++++
 26 files changed, 5215 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
 create mode 100644 Documentation/tee.txt
 create mode 100644 drivers/tee/Kconfig
 create mode 100644 drivers/tee/Makefile
 create mode 100644 drivers/tee/optee/Kconfig
 create mode 100644 drivers/tee/optee/Makefile
 create mode 100644 drivers/tee/optee/call.c
 create mode 100644 drivers/tee/optee/core.c
 create mode 100644 drivers/tee/optee/optee_msg.h
 create mode 100644 drivers/tee/optee/optee_private.h
 create mode 100644 drivers/tee/optee/optee_smc.h
 create mode 100644 drivers/tee/optee/rpc.c
 create mode 100644 drivers/tee/optee/supp.c
 create mode 100644 drivers/tee/tee_core.c
 create mode 100644 drivers/tee/tee_private.h
 create mode 100644 drivers/tee/tee_shm.c
 create mode 100644 drivers/tee/tee_shm_pool.c
 create mode 100644 include/linux/tee_drv.h
 create mode 100644 include/uapi/linux/tee.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1561836 — [PATCH v14 4/5] Documentation: tee subsystem and op-tee driver

Acked-by: Andreas Dannenberg <dannenberg@ti.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 Documentation/00-INDEX |   2 +
 Documentation/tee.txt  | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS            |   1 +
 3 files changed, 121 insertions(+)
 create mode 100644 Documentation/tee.txt

diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
index c8a8eb1a2b11..fc6a3cb83d97 100644
--- a/Documentation/00-INDEX
+++ b/Documentation/00-INDEX
@@ -412,6 +412,8 @@ sysctl/
 	- directory with info on the /proc/sys/* files.
 target/
 	- directory with info on generating TCM v4 fabric .ko modules
+tee.txt
+	- info on the TEE subsystem and drivers
 this_cpu_ops.txt
 	- List rationale behind and the way to use this_cpu operations.
 thermal/
diff --git a/Documentation/tee.txt b/Documentation/tee.txt
new file mode 100644
index 000000000000..718599357596
--- /dev/null
+++ b/Documentation/tee.txt
@@ -0,0 +1,118 @@
+TEE subsystem
+This document describes the TEE subsystem in Linux.
+
+A TEE (Trusted Execution Environment) is a trusted OS running in some
+secure environment, for example, TrustZone on ARM CPUs, or a separate
+secure co-processor etc. A TEE driver handles the details needed to
+communicate with the TEE.
+
+This subsystem deals with:
+
+- Registration of TEE drivers
+
+- Managing shared memory between Linux and the TEE
+
+- Providing a generic API to the TEE
+
+The TEE interface
+=================
+
+include/uapi/linux/tee.h defines the generic interface to a TEE.
+
+User space (the client) connects to the driver by opening /dev/tee[0-9]* or
+/dev/teepriv[0-9]*.
+
+- TEE_IOC_SHM_ALLOC allocates shared memory and returns a file descriptor
+  which user space can mmap. When user space doesn't need the file
+  descriptor any more, it should be closed. When shared memory isn't needed
+  any longer it should be unmapped with munmap() to allow the reuse of
+  memory.
+
+- TEE_IOC_VERSION lets user space know which TEE this driver handles and
+  the its capabilities.
+
+- TEE_IOC_OPEN_SESSION opens a new session to a Trusted Application.
+
+- TEE_IOC_INVOKE invokes a function in a Trusted Application.
+
+- TEE_IOC_CANCEL may cancel an ongoing TEE_IOC_OPEN_SESSION or TEE_IOC_INVOKE.
+
+- TEE_IOC_CLOSE_SESSION closes a session to a Trusted Application.
+
+There are two classes of clients, normal clients and supplicants. The latter is
+a helper process for the TEE to access resources in Linux, for example file
+system access. A normal client opens /dev/tee[0-9]* and a supplicant opens
+/dev/teepriv[0-9].
+
+Much of the communication between clients and the TEE is opaque to the
+driver. The main job for the driver is to receive requests from the
+clients, forward them to the TEE and send back the results. In the case of
+supplicants the communication goes in the other direction, the TEE sends
+requests to the supplicant which then sends back the result.
+
+OP-TEE driver
+=============
+
+The OP-TEE driver handles OP-TEE [1] based TEEs. Currently it is only the ARM
+TrustZone based OP-TEE solution that is supported.
+
+Lowest level of communication with OP-TEE builds on ARM SMC Calling
+Convention (SMCCC) [2], which is the foundation for OP-TEE's SMC interface
+[3] used internally by the driver. Stacked on top of that is OP-TEE Message
+Protocol [4].
+
+OP-TEE SMC interface provides the basic functions required by SMCCC and some
+additional functions specific for OP-TEE. The most interesting functions are:
+
+- OPTEE_SMC_FUNCID_CALLS_UID (part of SMCCC) returns the version information
+  which is then returned by TEE_IOC_VERSION
+
+- OPTEE_SMC_CALL_GET_OS_UUID returns the particular OP-TEE implementation, used
+  to tell, for instance, a TrustZone OP-TEE apart from an OP-TEE running on a
+  separate secure co-processor.
+
+- OPTEE_SMC_CALL_WITH_ARG drives the OP-TEE message protocol
+
+- OPTEE_SMC_GET_SHM_CONFIG lets the driver and OP-TEE agree on which memory
+  range to used for shared memory between Linux and OP-TEE.
+
+The GlobalPlatform TEE Client API [5] is implemented on top of the generic
+TEE API.
+
+Picture of the relationship between the different components in the
+OP-TEE architecture.
+
+    User space                  Kernel                   Secure world
+    ~~~~~~~~~~                  ~~~~~~                   ~~~~~~~~~~~~
+ +--------+                                             +-------------+
+ | Client |                                             | Trusted     |
+ +--------+                                             | Application |
+    /\                                                  +-------------+
+    || +----------+                                           /\
+    || |tee-      |                                           ||
+    || |supplicant|                                           \/
+    || +----------+                                     +-------------+
+    \/      /\                                          | TEE Internal|
+ +-------+  ||                                          | API         |
+ + TEE   |  ||            +--------+--------+           +-------------+
+ | Client|  ||            | TEE    | OP-TEE |           | OP-TEE      |
+ | API   |  \/            | subsys | driver |           | Trusted OS  |
+ +-------+----------------+----+-------+----+-----------+-------------+
+ |      Generic TEE API        |       |     OP-TEE MSG               |
+ |      IOCTL (TEE_IOC_*)      |       |     SMCCC (OPTEE_SMC_CALL_*) |
+ +-----------------------------+       +------------------------------+
+
+RPC (Remote Procedure Call) are requests from secure world to kernel driver
+or tee-supplicant. An RPC is identified by a special range of SMCCC return
+values from OPTEE_SMC_CALL_WITH_ARG. RPC messages which are intended for the
+kernel are handled by the kernel driver. Other RPC messages will be forwarded to
+tee-supplicant without further involvement of the driver, except switching
+shared memory buffer representation.
+
+References:
+[1] https://github.com/OP-TEE/optee_os
+[2] http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+[3] drivers/tee/optee/optee_smc.h
+[4] drivers/tee/optee/optee_msg.h
+[5] http://www.globalplatform.org/specificationsdevice.asp look for
+    "TEE Client API Specification v1.0" and click download.
diff --git a/MAINTAINERS b/MAINTAINERS
index 975fd339c026..eec9bee0d752 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10895,6 +10895,7 @@ S:	Maintained
 F:	include/linux/tee_drv.h
 F:	include/uapi/linux/tee.h
 F:	drivers/tee/
+F:	Documentation/tee.txt
 
 THUNDERBOLT DRIVER
 M:	Andreas Noever <andreas.noever@gmail.com>
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1562308 — Re: [PATCH v14 4/5] Documentation: tee subsystem and op-tee driver

Hi Jens,

Documentation looks good.

On 17-01-18 04:58 AM, Jens Wiklander wrote:
> Acked-by: Andreas Dannenberg <dannenberg@ti.com>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  Documentation/00-INDEX |   2 +
>  Documentation/tee.txt  | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS            |   1 +
>  3 files changed, 121 insertions(+)
>  create mode 100644 Documentation/tee.txt
>
> diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
> index c8a8eb1a2b11..fc6a3cb83d97 100644
> --- a/Documentation/00-INDEX
> +++ b/Documentation/00-INDEX
> @@ -412,6 +412,8 @@ sysctl/
>  	- directory with info on the /proc/sys/* files.
>  target/
>  	- directory with info on generating TCM v4 fabric .ko modules
> +tee.txt
> +	- info on the TEE subsystem and drivers
>  this_cpu_ops.txt
>  	- List rationale behind and the way to use this_cpu operations.
>  thermal/
> diff --git a/Documentation/tee.txt b/Documentation/tee.txt
> new file mode 100644
> index 000000000000..718599357596
> --- /dev/null
> +++ b/Documentation/tee.txt
> @@ -0,0 +1,118 @@
> +TEE subsystem
> +This document describes the TEE subsystem in Linux.
> +
> +A TEE (Trusted Execution Environment) is a trusted OS running in some
> +secure environment, for example, TrustZone on ARM CPUs, or a separate
> +secure co-processor etc. A TEE driver handles the details needed to
> +communicate with the TEE.
> +
> +This subsystem deals with:
> +
> +- Registration of TEE drivers
> +
> +- Managing shared memory between Linux and the TEE
> +
> +- Providing a generic API to the TEE
> +
> +The TEE interface
> +=================
> +
> +include/uapi/linux/tee.h defines the generic interface to a TEE.
> +
> +User space (the client) connects to the driver by opening /dev/tee[0-9]* or
> +/dev/teepriv[0-9]*.
> +
> +- TEE_IOC_SHM_ALLOC allocates shared memory and returns a file descriptor
> +  which user space can mmap. When user space doesn't need the file
> +  descriptor any more, it should be closed. When shared memory isn't needed
> +  any longer it should be unmapped with munmap() to allow the reuse of
> +  memory.
> +
> +- TEE_IOC_VERSION lets user space know which TEE this driver handles and
> +  the its capabilities.
> +
> +- TEE_IOC_OPEN_SESSION opens a new session to a Trusted Application.
> +
> +- TEE_IOC_INVOKE invokes a function in a Trusted Application.
> +
> +- TEE_IOC_CANCEL may cancel an ongoing TEE_IOC_OPEN_SESSION or TEE_IOC_INVOKE.
> +
> +- TEE_IOC_CLOSE_SESSION closes a session to a Trusted Application.
> +
> +There are two classes of clients, normal clients and supplicants. The latter is
> +a helper process for the TEE to access resources in Linux, for example file
> +system access. A normal client opens /dev/tee[0-9]* and a supplicant opens
> +/dev/teepriv[0-9].
> +
> +Much of the communication between clients and the TEE is opaque to the
> +driver. The main job for the driver is to receive requests from the
> +clients, forward them to the TEE and send back the results. In the case of
> +supplicants the communication goes in the other direction, the TEE sends
> +requests to the supplicant which then sends back the result.
> +
> +OP-TEE driver
> +=============
> +
> +The OP-TEE driver handles OP-TEE [1] based TEEs. Currently it is only the ARM
> +TrustZone based OP-TEE solution that is supported.
> +
> +Lowest level of communication with OP-TEE builds on ARM SMC Calling
> +Convention (SMCCC) [2], which is the foundation for OP-TEE's SMC interface
> +[3] used internally by the driver. Stacked on top of that is OP-TEE Message
> +Protocol [4].
> +
> +OP-TEE SMC interface provides the basic functions required by SMCCC and some
> +additional functions specific for OP-TEE. The most interesting functions are:
> +
> +- OPTEE_SMC_FUNCID_CALLS_UID (part of SMCCC) returns the version information
> +  which is then returned by TEE_IOC_VERSION
> +
> +- OPTEE_SMC_CALL_GET_OS_UUID returns the particular OP-TEE implementation, used
> +  to tell, for instance, a TrustZone OP-TEE apart from an OP-TEE running on a
> +  separate secure co-processor.
> +
> +- OPTEE_SMC_CALL_WITH_ARG drives the OP-TEE message protocol
> +
> +- OPTEE_SMC_GET_SHM_CONFIG lets the driver and OP-TEE agree on which memory
> +  range to used for shared memory between Linux and OP-TEE.
> +
> +The GlobalPlatform TEE Client API [5] is implemented on top of the generic
> +TEE API.
> +
> +Picture of the relationship between the different components in the
> +OP-TEE architecture.
> +
> +    User space                  Kernel                   Secure world
> +    ~~~~~~~~~~                  ~~~~~~                   ~~~~~~~~~~~~
> + +--------+                                             +-------------+
> + | Client |                                             | Trusted     |
> + +--------+                                             | Application |
> +    /\                                                  +-------------+
> +    || +----------+                                           /\
> +    || |tee-      |                                           ||
> +    || |supplicant|                                           \/
> +    || +----------+                                     +-------------+
> +    \/      /\                                          | TEE Internal|
> + +-------+  ||                                          | API         |
> + + TEE   |  ||            +--------+--------+           +-------------+
> + | Client|  ||            | TEE    | OP-TEE |           | OP-TEE      |
> + | API   |  \/            | subsys | driver |           | Trusted OS  |
> + +-------+----------------+----+-------+----+-----------+-------------+
> + |      Generic TEE API        |       |     OP-TEE MSG               |
> + |      IOCTL (TEE_IOC_*)      |       |     SMCCC (OPTEE_SMC_CALL_*) |
> + +-----------------------------+       +------------------------------+
> +
> +RPC (Remote Procedure Call) are requests from secure world to kernel driver
> +or tee-supplicant. An RPC is identified by a special range of SMCCC return
> +values from OPTEE_SMC_CALL_WITH_ARG. RPC messages which are intended for the
> +kernel are handled by the kernel driver. Other RPC messages will be forwarded to
> +tee-supplicant without further involvement of the driver, except switching
> +shared memory buffer representation.
> +
> +References:
> +[1] https://github.com/OP-TEE/optee_os
> +[2] http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
> +[3] drivers/tee/optee/optee_smc.h
> +[4] drivers/tee/optee/optee_msg.h
> +[5] http://www.globalplatform.org/specificationsdevice.asp look for
> +    "TEE Client API Specification v1.0" and click download.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 975fd339c026..eec9bee0d752 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10895,6 +10895,7 @@ S:	Maintained
>  F:	include/linux/tee_drv.h
>  F:	include/uapi/linux/tee.h
>  F:	drivers/tee/
> +F:	Documentation/tee.txt
>
>  THUNDERBOLT DRIVER
>  M:	Andreas Noever <andreas.noever@gmail.com>
>

[toc] | [prev] | [next] | [standalone]

#1561953 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Wednesday, January 18, 2017 1:58:14 PM CET Jens Wiklander wrote:
> Adds a OP-TEE driver which also can be compiled as a loadable module.
> 
> * Targets ARM and ARM64
> * Supports using reserved memory from OP-TEE as shared memory
> * Probes OP-TEE version using SMCs
> * Accepts requests on privileged and unprivileged device
> * Uses OPTEE message protocol version 2 to communicate with secure world

I had not really followed the last versions, and I've looked through
it now for things that seemed odd to me, either because I don't understand
them or because they could be improved. I'll try to read it again after
I've seen clarifications on these points.

Generally speaking I haven't seen any show-stoppers so far.

> +struct optee_call_waiter {
> +	struct list_head list_node;
> +	struct completion c;
> +	bool completed;
> +};

It seems wrong to have both a 'struct completion' and 'bool completed' here,
as completion already contains such a flag and is designed to update that
atomically.

> +static void optee_cq_complete_one(struct optee_call_queue *cq)
> +{
> +	struct optee_call_waiter *w;
> +
> +	list_for_each_entry(w, &cq->waiters, list_node) {
> +		if (!w->completed) {
> +			complete(&w->c);
> +			w->completed = true;
> +			break;
> +		}
> +	}
> +}
> +
> +static void optee_cq_wait_final(struct optee_call_queue *cq,
> +				struct optee_call_waiter *w)
> +{
> +	mutex_lock(&cq->mutex);
> +
> +	/* Get out of the list */
> +	list_del(&w->list_node);
> +
> +	optee_cq_complete_one(cq);
> +	/*
> +	 * If we're completed we've got a completion that some other task
> +	 * could have used instead.
> +	 */
> +	if (w->completed)
> +		optee_cq_complete_one(cq);
> +
> +	mutex_unlock(&cq->mutex);
> +}

This deserves some more comments: the function name suggests that you are
waiting for a specific optee_call_waiter, but then it calls
optee_cq_complete_one(), which unconditionally completes the first
incomplete completion and it never waits.

> +static struct tee_shm_pool *
> +optee_config_shm_ioremap(struct device *dev, optee_invoke_fn *invoke_fn,
> +			 void __iomem **ioremaped_shm)
> +{
> +	union {
> +		struct arm_smccc_res smccc;
> +		struct optee_smc_get_shm_config_result result;
> +	} res;
> +	struct tee_shm_pool *pool;
> +	unsigned long vaddr;
> +	phys_addr_t paddr;
> +	size_t size;
> +	phys_addr_t begin;
> +	phys_addr_t end;
> +	void __iomem *va;
> +	struct tee_shm_pool_mem_info priv_info;
> +	struct tee_shm_pool_mem_info dmabuf_info;
> +
> +	invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res.smccc);
> +	if (res.result.status != OPTEE_SMC_RETURN_OK) {
> +		dev_info(dev, "shm service not available\n");
> +		return ERR_PTR(-ENOENT);
> +	}
> +
> +	if (res.result.settings != OPTEE_SMC_SHM_CACHED) {
> +		dev_err(dev, "only normal cached shared memory supported\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	begin = roundup(res.result.start, PAGE_SIZE);
> +	end = rounddown(res.result.start + res.result.size, PAGE_SIZE);
> +	paddr = begin;
> +	size = end - begin;
> +
> +	if (size < 2 * OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE) {
> +		dev_err(dev, "too small shared memory area\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	va = ioremap_cache(paddr, size);
> +	if (!va) {
> +		dev_err(dev, "shared memory ioremap failed\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +	vaddr = (unsigned long)va;

I think you should call memremap() instead of ioremap_cache() here and assume
that you are talking to actual RAM.

> +static int __init optee_driver_init(void)
> +{
> +	struct device_node *node;
> +
> +	/*
> +	 * Preferred path is /firmware/optee, but it's the matching that
> +	 * matters.
> +	 */
> +	for_each_matching_node(node, optee_match)
> +		of_platform_device_create(node, NULL, NULL);
> +
> +	return platform_driver_register(&optee_driver);
> +}
> +module_init(optee_driver_init);
> +
> +static void __exit optee_driver_exit(void)
> +{
> +	platform_driver_unregister(&optee_driver);
> +}
> +module_exit(optee_driver_exit);

What is the platform driver good for if the same module has to create the
platform devices itself?

I'd just skip it and do

	for_each_matching_node(node, optee_match)
		optee_probe(node);

I also suspect that module unloading is broken here if you don't clean
up the platform devices in the end, so you should already remove the
exit function to prevent unloading.

> +struct optee_msg_arg {
> +	u32 cmd;
> +	u32 func;
> +	u32 session;
> +	u32 cancel_id;
> +	u32 pad;
> +	u32 ret;
> +	u32 ret_origin;
> +	u32 num_params;
> +
> +	/*
> +	 * this struct is 8 byte aligned since the 'struct optee_msg_param'
> +	 * which follows requires 8 byte alignment.
> +	 *
> +	 * Commented out element used to visualize the layout dynamic part
> +	 * of the struct. This field is not available at all if
> +	 * num_params == 0.
> +	 *
> +	 * params is accessed through the macro OPTEE_MSG_GET_PARAMS
> +	 *
> +	 * struct optee_msg_param params[num_params];
> +	 */
> +} __aligned(8);
> +
> +/**
> + * OPTEE_MSG_GET_PARAMS - return pointer to struct optee_msg_param *
> + *
> + * @x: Pointer to a struct optee_msg_arg
> + *
> + * Returns a pointer to the params[] inside a struct optee_msg_arg.
> + */
> +#define OPTEE_MSG_GET_PARAMS(x) \
> +	(struct optee_msg_param *)(((struct optee_msg_arg *)(x)) + 1)

If you make the last member of optee_msg_arg

	struct optee_msg_param params[0];

then you can remove both the macro here and the alignment attribute.

> +/*****************************************************************************
> + * Part 2 - requests from normal world
> + *****************************************************************************/
> +
> +/*
> + * Return the following UID if using API specified in this file without
> + * further extensions:
> + * 384fb3e0-e7f8-11e3-af63-0002a5d5c51b.
> + * Represented in 4 32-bit words in OPTEE_MSG_UID_0, OPTEE_MSG_UID_1,
> + * OPTEE_MSG_UID_2, OPTEE_MSG_UID_3.
> + */
> +#define OPTEE_MSG_UID_0			0x384fb3e0
> +#define OPTEE_MSG_UID_1			0xe7f811e3
> +#define OPTEE_MSG_UID_2			0xaf630002
> +#define OPTEE_MSG_UID_3			0xa5d5c51b
> +#define OPTEE_MSG_FUNCID_CALLS_UID	0xFF01
> +
> +/*
> + * Returns 2.0 if using API specified in this file without further
> + * extensions. Represented in 2 32-bit words in OPTEE_MSG_REVISION_MAJOR
> + * and OPTEE_MSG_REVISION_MINOR
> + */
> +#define OPTEE_MSG_REVISION_MAJOR	2
> +#define OPTEE_MSG_REVISION_MINOR	0
> +#define OPTEE_MSG_FUNCID_CALLS_REVISION	0xFF03
> +
> +/*
> + * Get UUID of Trusted OS.
> + *
> + * Used by non-secure world to figure out which Trusted OS is installed.
> + * Note that returned UUID is the UUID of the Trusted OS, not of the API.
> + *
> + * Returns UUID in 4 32-bit words in the same way as
> + * OPTEE_MSG_FUNCID_CALLS_UID described above.
> + */
> +#define OPTEE_MSG_OS_OPTEE_UUID_0	0x486178e0
> +#define OPTEE_MSG_OS_OPTEE_UUID_1	0xe7f811e3
> +#define OPTEE_MSG_OS_OPTEE_UUID_2	0xbc5e0002
> +#define OPTEE_MSG_OS_OPTEE_UUID_3	0xa5d5c51b
> +#define OPTEE_MSG_FUNCID_GET_OS_UUID	0x0000
> +
> +/*
> + * Get revision of Trusted OS.
> + *
> + * Used by non-secure world to figure out which version of the Trusted OS
> + * is installed. Note that the returned revision is the revision of the
> + * Trusted OS, not of the API.
> + *
> + * Returns revision in 2 32-bit words in the same way as
> + * OPTEE_MSG_CALLS_REVISION described above.
> + */
> +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR	1
> +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR	0
> +#define OPTEE_MSG_FUNCID_GET_OS_REVISION	0x0001

Just for my understanding, what is the significance of these numbers,
i.e. which code (user space, kernel driver, trusted OS) provides
the uuid and which one provides the version? The code comments almost
make sense to me, but I don't see why specific versions are listed
in this header.

What is the expected behavior when one side reports a version that
is unknown? Can one side claim to be backwards compatible with
a previous version, or does each new version need support on
all three sides?

> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> new file mode 100644
> index 000000000000..0b9c1a2accd0
> --- /dev/null
> +++ b/drivers/tee/optee/rpc.c
> +static void handle_rpc_func_cmd_wq(struct optee *optee,
> +				   struct optee_msg_arg *arg)
> +{
> +	struct optee_msg_param *params;
> +
> +	if (arg->num_params != 1)
> +		goto bad;
> +
> +	params = OPTEE_MSG_GET_PARAMS(arg);
> +	if ((params->attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> +			OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> +		goto bad;
> +
> +	switch (params->u.value.a) {
> +	case OPTEE_MSG_RPC_WAIT_QUEUE_SLEEP:
> +		wq_sleep(&optee->wait_queue, params->u.value.b);
> +		break;
> +	case OPTEE_MSG_RPC_WAIT_QUEUE_WAKEUP:
> +		wq_wakeup(&optee->wait_queue, params->u.value.b);
> +		break;
> +	default:
> +		goto bad;
> +	}
> +
> +	arg->ret = TEEC_SUCCESS;
> +	return;
> +bad:
> +	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +}
> +

I'm trying to understand what this is good for. What I can see is that
you have a user space process calling into the kernel asking the tee
to do some command, and then the tee can ask the kernel to wait for
something to happen, or notify it that something has happened.

If we wait here, the user process gets suspended until this has
actually happened.

Am I reading this correctly? If yes, what is the intended use case?
Is there some process that is meant to always wait here? What
if we ever need to wait for more than one thing at a time (think
select or poll?)

> +	params = OPTEE_MSG_GET_PARAMS(arg);
> +	if ((params->attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> +			OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> +		goto bad;
> +
> +	msec_to_wait = params->u.value.a;
> +
> +	/* set task's state to interruptible sleep */
> +	set_current_state(TASK_INTERRUPTIBLE);
> +
> +	/* take a nap */
> +	schedule_timeout(msecs_to_jiffies(msec_to_wait));

This can be done simpler with msleep();

	Arnd

[toc] | [prev] | [next] | [standalone]

#1562800 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> On Wednesday, January 18, 2017 1:58:14 PM CET Jens Wiklander wrote:
> > Adds a OP-TEE driver which also can be compiled as a loadable module.
> > 
> > * Targets ARM and ARM64
> > * Supports using reserved memory from OP-TEE as shared memory
> > * Probes OP-TEE version using SMCs
> > * Accepts requests on privileged and unprivileged device
> > * Uses OPTEE message protocol version 2 to communicate with secure world
> 
> I had not really followed the last versions, and I've looked through
> it now for things that seemed odd to me, either because I don't understand
> them or because they could be improved. I'll try to read it again after
> I've seen clarifications on these points.
> 
> Generally speaking I haven't seen any show-stoppers so far.
> 
> > +struct optee_call_waiter {
> > +	struct list_head list_node;
> > +	struct completion c;
> > +	bool completed;
> > +};
> 
> It seems wrong to have both a 'struct completion' and 'bool completed' here,
> as completion already contains such a flag and is designed to update that
> atomically.

You're right, I'll remove the 'bool completed'.

> 
> > +static void optee_cq_complete_one(struct optee_call_queue *cq)
> > +{
> > +	struct optee_call_waiter *w;
> > +
> > +	list_for_each_entry(w, &cq->waiters, list_node) {
> > +		if (!w->completed) {
> > +			complete(&w->c);
> > +			w->completed = true;
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +static void optee_cq_wait_final(struct optee_call_queue *cq,
> > +				struct optee_call_waiter *w)
> > +{
> > +	mutex_lock(&cq->mutex);
> > +
> > +	/* Get out of the list */
> > +	list_del(&w->list_node);
> > +
> > +	optee_cq_complete_one(cq);
> > +	/*
> > +	 * If we're completed we've got a completion that some other task
> > +	 * could have used instead.
> > +	 */
> > +	if (w->completed)
> > +		optee_cq_complete_one(cq);
> > +
> > +	mutex_unlock(&cq->mutex);
> > +}
> 
> This deserves some more comments: the function name suggests that you are
> waiting for a specific optee_call_waiter, but then it calls
> optee_cq_complete_one(), which unconditionally completes the first
> incomplete completion and it never waits.

OK, I'm updating these functions with more comments. The purpose of
these functions is to deal with resource shortage in secure world.
There's a limit on how many threads that execute concurrently in secure
world. optee_cq_wait_for_completion() waits for any task returning from
a call to secure world, optee_cq_complete_one() doesn't care who's
completed as long as tasks aren't stuck when there's available resources
in secure world.

> 
> > +static struct tee_shm_pool *
> > +optee_config_shm_ioremap(struct device *dev, optee_invoke_fn *invoke_fn,
> > +			 void __iomem **ioremaped_shm)
> > +{
> > +	union {
> > +		struct arm_smccc_res smccc;
> > +		struct optee_smc_get_shm_config_result result;
> > +	} res;
> > +	struct tee_shm_pool *pool;
> > +	unsigned long vaddr;
> > +	phys_addr_t paddr;
> > +	size_t size;
> > +	phys_addr_t begin;
> > +	phys_addr_t end;
> > +	void __iomem *va;
> > +	struct tee_shm_pool_mem_info priv_info;
> > +	struct tee_shm_pool_mem_info dmabuf_info;
> > +
> > +	invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res.smccc);
> > +	if (res.result.status != OPTEE_SMC_RETURN_OK) {
> > +		dev_info(dev, "shm service not available\n");
> > +		return ERR_PTR(-ENOENT);
> > +	}
> > +
> > +	if (res.result.settings != OPTEE_SMC_SHM_CACHED) {
> > +		dev_err(dev, "only normal cached shared memory supported\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	begin = roundup(res.result.start, PAGE_SIZE);
> > +	end = rounddown(res.result.start + res.result.size, PAGE_SIZE);
> > +	paddr = begin;
> > +	size = end - begin;
> > +
> > +	if (size < 2 * OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE) {
> > +		dev_err(dev, "too small shared memory area\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	va = ioremap_cache(paddr, size);
> > +	if (!va) {
> > +		dev_err(dev, "shared memory ioremap failed\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +	vaddr = (unsigned long)va;
> 
> I think you should call memremap() instead of ioremap_cache() here and assume
> that you are talking to actual RAM.

Thanks, I'll update.

> 
> > +static int __init optee_driver_init(void)
> > +{
> > +	struct device_node *node;
> > +
> > +	/*
> > +	 * Preferred path is /firmware/optee, but it's the matching that
> > +	 * matters.
> > +	 */
> > +	for_each_matching_node(node, optee_match)
> > +		of_platform_device_create(node, NULL, NULL);
> > +
> > +	return platform_driver_register(&optee_driver);
> > +}
> > +module_init(optee_driver_init);
> > +
> > +static void __exit optee_driver_exit(void)
> > +{
> > +	platform_driver_unregister(&optee_driver);
> > +}
> > +module_exit(optee_driver_exit);
> 
> What is the platform driver good for if the same module has to create the
> platform devices itself?

The platform device(s) are created here because the optee node is below
"/firmware" instead of the root where it would have had the platform
device created automatically.

I think it's useful to be able to unload the module, the early reviews
of this patch set was much focused around that. Regardless I'll need
some device as parent for the devices created during optee_probe() and
using a platform device for that seems natural.

I'd rather keep the platform driver. Perhaps some variant of the pattern
in qcom_scm_init() (drivers/firmware/qcom_scm.c) is useful, except that
I need to find out what to do about the life cycle of the objects
created with of_platform_populate().

> 
> I'd just skip it and do
> 
> 	for_each_matching_node(node, optee_match)
> 		optee_probe(node);
> 
> I also suspect that module unloading is broken here if you don't clean
> up the platform devices in the end, so you should already remove the
> exit function to prevent unloading.

Does the platform devices really need cleaning? I mean
of_platform_default_populate_init() creates a bunch of platform devices
which are just left there even if unused. Here we're doing the same
thing except that we're doing it for a specific node in the DT.

> 
> > +struct optee_msg_arg {
> > +	u32 cmd;
> > +	u32 func;
> > +	u32 session;
> > +	u32 cancel_id;
> > +	u32 pad;
> > +	u32 ret;
> > +	u32 ret_origin;
> > +	u32 num_params;
> > +
> > +	/*
> > +	 * this struct is 8 byte aligned since the 'struct optee_msg_param'
> > +	 * which follows requires 8 byte alignment.
> > +	 *
> > +	 * Commented out element used to visualize the layout dynamic part
> > +	 * of the struct. This field is not available at all if
> > +	 * num_params == 0.
> > +	 *
> > +	 * params is accessed through the macro OPTEE_MSG_GET_PARAMS
> > +	 *
> > +	 * struct optee_msg_param params[num_params];
> > +	 */
> > +} __aligned(8);
> > +
> > +/**
> > + * OPTEE_MSG_GET_PARAMS - return pointer to struct optee_msg_param *
> > + *
> > + * @x: Pointer to a struct optee_msg_arg
> > + *
> > + * Returns a pointer to the params[] inside a struct optee_msg_arg.
> > + */
> > +#define OPTEE_MSG_GET_PARAMS(x) \
> > +	(struct optee_msg_param *)(((struct optee_msg_arg *)(x)) + 1)
> 
> If you make the last member of optee_msg_arg
> 
> 	struct optee_msg_param params[0];
> 
> then you can remove both the macro here and the alignment attribute.

OK.

> 
> > +/*****************************************************************************
> > + * Part 2 - requests from normal world
> > + *****************************************************************************/
> > +
> > +/*
> > + * Return the following UID if using API specified in this file without
> > + * further extensions:
> > + * 384fb3e0-e7f8-11e3-af63-0002a5d5c51b.
> > + * Represented in 4 32-bit words in OPTEE_MSG_UID_0, OPTEE_MSG_UID_1,
> > + * OPTEE_MSG_UID_2, OPTEE_MSG_UID_3.
> > + */
> > +#define OPTEE_MSG_UID_0			0x384fb3e0
> > +#define OPTEE_MSG_UID_1			0xe7f811e3
> > +#define OPTEE_MSG_UID_2			0xaf630002
> > +#define OPTEE_MSG_UID_3			0xa5d5c51b
> > +#define OPTEE_MSG_FUNCID_CALLS_UID	0xFF01
> > +
> > +/*
> > + * Returns 2.0 if using API specified in this file without further
> > + * extensions. Represented in 2 32-bit words in OPTEE_MSG_REVISION_MAJOR
> > + * and OPTEE_MSG_REVISION_MINOR
> > + */
> > +#define OPTEE_MSG_REVISION_MAJOR	2
> > +#define OPTEE_MSG_REVISION_MINOR	0
> > +#define OPTEE_MSG_FUNCID_CALLS_REVISION	0xFF03
> > +
> > +/*
> > + * Get UUID of Trusted OS.
> > + *
> > + * Used by non-secure world to figure out which Trusted OS is installed.
> > + * Note that returned UUID is the UUID of the Trusted OS, not of the API.
> > + *
> > + * Returns UUID in 4 32-bit words in the same way as
> > + * OPTEE_MSG_FUNCID_CALLS_UID described above.
> > + */
> > +#define OPTEE_MSG_OS_OPTEE_UUID_0	0x486178e0
> > +#define OPTEE_MSG_OS_OPTEE_UUID_1	0xe7f811e3
> > +#define OPTEE_MSG_OS_OPTEE_UUID_2	0xbc5e0002
> > +#define OPTEE_MSG_OS_OPTEE_UUID_3	0xa5d5c51b
> > +#define OPTEE_MSG_FUNCID_GET_OS_UUID	0x0000
> > +
> > +/*
> > + * Get revision of Trusted OS.
> > + *
> > + * Used by non-secure world to figure out which version of the Trusted OS
> > + * is installed. Note that the returned revision is the revision of the
> > + * Trusted OS, not of the API.
> > + *
> > + * Returns revision in 2 32-bit words in the same way as
> > + * OPTEE_MSG_CALLS_REVISION described above.
> > + */
> > +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR	1
> > +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR	0
> > +#define OPTEE_MSG_FUNCID_GET_OS_REVISION	0x0001
> 
> Just for my understanding, what is the significance of these numbers,
> i.e. which code (user space, kernel driver, trusted OS) provides
> the uuid and which one provides the version? The code comments almost
> make sense to me, but I don't see why specific versions are listed
> in this header.

You're right, OPTEE_MSG_OS_OPTEE_REVISION_* should be removed. The
actual version the secure OS is of a mostly informational nature. The
same goes the OS UUID, but I suppose the actual UUID used by the
upstream version of OP-TEE OS could be interesting to know.

> 
> What is the expected behavior when one side reports a version that
> is unknown? Can one side claim to be backwards compatible with
> a previous version, or does each new version need support on
> all three sides?

The UUID and version of the message protocol are important to match
correctly as otherwise it could mean that there's something unexpected
in secure world that following the message protocol would be undefined
behaviour. All changes to the message protocol should be backwards
compatible in the sense that the driver and secure world need to
negotiate eventual extensions while probing. That's what we're doing in
optee_msg_exchange_capabilities().

> 
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > new file mode 100644
> > index 000000000000..0b9c1a2accd0
> > --- /dev/null
> > +++ b/drivers/tee/optee/rpc.c
> > +static void handle_rpc_func_cmd_wq(struct optee *optee,
> > +				   struct optee_msg_arg *arg)
> > +{
> > +	struct optee_msg_param *params;
> > +
> > +	if (arg->num_params != 1)
> > +		goto bad;
> > +
> > +	params = OPTEE_MSG_GET_PARAMS(arg);
> > +	if ((params->attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> > +			OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> > +		goto bad;
> > +
> > +	switch (params->u.value.a) {
> > +	case OPTEE_MSG_RPC_WAIT_QUEUE_SLEEP:
> > +		wq_sleep(&optee->wait_queue, params->u.value.b);
> > +		break;
> > +	case OPTEE_MSG_RPC_WAIT_QUEUE_WAKEUP:
> > +		wq_wakeup(&optee->wait_queue, params->u.value.b);
> > +		break;
> > +	default:
> > +		goto bad;
> > +	}
> > +
> > +	arg->ret = TEEC_SUCCESS;
> > +	return;
> > +bad:
> > +	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > +}
> > +
> 
> I'm trying to understand what this is good for. What I can see is that
> you have a user space process calling into the kernel asking the tee
> to do some command, and then the tee can ask the kernel to wait for
> something to happen, or notify it that something has happened.
> 
> If we wait here, the user process gets suspended until this has
> actually happened.
> 
> Am I reading this correctly? If yes, what is the intended use case?
> Is there some process that is meant to always wait here? What
> if we ever need to wait for more than one thing at a time (think
> select or poll?)

I'm updating the comments for OPTEE_MSG_RPC_CMD_WAIT_QUEUE with:
"If secure world need 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."

The way we're waking up a sleeping thread is a bit limiting in some
circumstances.

One case is where we need to do it from a secure interrupt handler.
Because there's no way of doing this kind of RPC to normal world from a
secure interrupt handler. In that case it wouldn't be mutex the thread
is waiting for though.

Another case is where there's several guest running in the system and
more than one guests has access to secure world. If guest2 waits for
mutex which guest1 is releasing, how can guest2 be notified? Normal RPC
is impossible here also.

The only way around this limitation I've come up with so far is by doing
the wakeup via a software generated interrupt destined to the correct
guest. However this problem is beyond the scope of this patch set.

> 
> > +	params = OPTEE_MSG_GET_PARAMS(arg);
> > +	if ((params->attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> > +			OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> > +		goto bad;
> > +
> > +	msec_to_wait = params->u.value.a;
> > +
> > +	/* set task's state to interruptible sleep */
> > +	set_current_state(TASK_INTERRUPTIBLE);
> > +
> > +	/* take a nap */
> > +	schedule_timeout(msecs_to_jiffies(msec_to_wait));
> 
> This can be done simpler with msleep();

OK.

Thanks,
Jens

[toc] | [prev] | [next] | [standalone]

#1563794 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> > On Wednesday, January 18, 2017 1:58:14 PM CET Jens Wiklander wrote:

> > > +static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > +{
> > > +	struct optee_call_waiter *w;
> > > +
> > > +	list_for_each_entry(w, &cq->waiters, list_node) {
> > > +		if (!w->completed) {
> > > +			complete(&w->c);
> > > +			w->completed = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void optee_cq_wait_final(struct optee_call_queue *cq,
> > > +				struct optee_call_waiter *w)
> > > +{
> > > +	mutex_lock(&cq->mutex);
> > > +
> > > +	/* Get out of the list */
> > > +	list_del(&w->list_node);
> > > +
> > > +	optee_cq_complete_one(cq);
> > > +	/*
> > > +	 * If we're completed we've got a completion that some other task
> > > +	 * could have used instead.
> > > +	 */
> > > +	if (w->completed)
> > > +		optee_cq_complete_one(cq);
> > > +
> > > +	mutex_unlock(&cq->mutex);
> > > +}
> > 
> > This deserves some more comments: the function name suggests that you are
> > waiting for a specific optee_call_waiter, but then it calls
> > optee_cq_complete_one(), which unconditionally completes the first
> > incomplete completion and it never waits.
> 
> OK, I'm updating these functions with more comments. The purpose of
> these functions is to deal with resource shortage in secure world.
> There's a limit on how many threads that execute concurrently in secure
> world. optee_cq_wait_for_completion() waits for any task returning from
> a call to secure world, optee_cq_complete_one() doesn't care who's
> completed as long as tasks aren't stuck when there's available resources
> in secure world.

Ok, that is indeed not clear from the code but your explanation makes
perfect sense, thanks!

> > > +static int __init optee_driver_init(void)
> > > +{
> > > +	struct device_node *node;
> > > +
> > > +	/*
> > > +	 * Preferred path is /firmware/optee, but it's the matching that
> > > +	 * matters.
> > > +	 */
> > > +	for_each_matching_node(node, optee_match)
> > > +		of_platform_device_create(node, NULL, NULL);
> > > +
> > > +	return platform_driver_register(&optee_driver);
> > > +}
> > > +module_init(optee_driver_init);
> > > +
> > > +static void __exit optee_driver_exit(void)
> > > +{
> > > +	platform_driver_unregister(&optee_driver);
> > > +}
> > > +module_exit(optee_driver_exit);
> > 
> > What is the platform driver good for if the same module has to create the
> > platform devices itself?
> 
> The platform device(s) are created here because the optee node is below
> "/firmware" instead of the root where it would have had the platform
> device created automatically.
> 
> I think it's useful to be able to unload the module, the early reviews
> of this patch set was much focused around that. Regardless I'll need
> some device as parent for the devices created during optee_probe() and
> using a platform device for that seems natural.
> 
> I'd rather keep the platform driver. Perhaps some variant of the pattern
> in qcom_scm_init() (drivers/firmware/qcom_scm.c) is useful, except that
> I need to find out what to do about the life cycle of the objects
> created with of_platform_populate().

My point was that I don't think we need devices here at all. It's different
when you talk to external hardware that has register resource etc that
can be best abstracted as a real device, but for other firmware features
we don't normally add one.

Module unloading can also be done without the device.

> > 
> > I'd just skip it and do
> > 
> > 	for_each_matching_node(node, optee_match)
> > 		optee_probe(node);
> > 
> > I also suspect that module unloading is broken here if you don't clean
> > up the platform devices in the end, so you should already remove the
> > exit function to prevent unloading.
> 
> Does the platform devices really need cleaning? I mean
> of_platform_default_populate_init() creates a bunch of platform devices
> which are just left there even if unused. Here we're doing the same
> thing except that we're doing it for a specific node in the DT.

I think it will work if you don't clean them up, but it feels wrong
to have a loadable module that creates devices when loaded but doesn't
remove them when unloaded.

This could be done differently by having the device creation done in
one driver and the the user of that device in another driver, but I
think just killing off the device achieves the same in a simpler way.

> > > +/*
> > > + * Get revision of Trusted OS.
> > > + *
> > > + * Used by non-secure world to figure out which version of the Trusted OS
> > > + * is installed. Note that the returned revision is the revision of the
> > > + * Trusted OS, not of the API.
> > > + *
> > > + * Returns revision in 2 32-bit words in the same way as
> > > + * OPTEE_MSG_CALLS_REVISION described above.
> > > + */
> > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR	1
> > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR	0
> > > +#define OPTEE_MSG_FUNCID_GET_OS_REVISION	0x0001
> > 
> > Just for my understanding, what is the significance of these numbers,
> > i.e. which code (user space, kernel driver, trusted OS) provides
> > the uuid and which one provides the version? The code comments almost
> > make sense to me, but I don't see why specific versions are listed
> > in this header.
> 
> You're right, OPTEE_MSG_OS_OPTEE_REVISION_* should be removed. The
> actual version the secure OS is of a mostly informational nature. The
> same goes the OS UUID, but I suppose the actual UUID used by the
> upstream version of OP-TEE OS could be interesting to know.
...
> > What is the expected behavior when one side reports a version that
> > is unknown? Can one side claim to be backwards compatible with
> > a previous version, or does each new version need support on
> > all three sides?
> 
> The UUID and version of the message protocol are important to match
> correctly as otherwise it could mean that there's something unexpected
> in secure world that following the message protocol would be undefined
> behaviour. All changes to the message protocol should be backwards
> compatible in the sense that the driver and secure world need to
> negotiate eventual extensions while probing. That's what we're doing in
> optee_msg_exchange_capabilities().

Ok, then maybe the "compatible" identifier in DT should be sufficient
to ensure that the capability exchange works, and the rest be based
on that?

We tend to avoid version checks for APIs in the kernel because they
never work in practice, but the capability check should be fine.

> > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > new file mode 100644
> > > index 000000000000..0b9c1a2accd0
> > > --- /dev/null
> > > +++ b/drivers/tee/optee/rpc.c
> > > +static void handle_rpc_func_cmd_wq(struct optee *optee,
> > > +				   struct optee_msg_arg *arg)
> > > +{
> > > +	struct optee_msg_param *params;
> > > +
> > > +	if (arg->num_params != 1)
> > > +		goto bad;
> > > +
> > > +	params = OPTEE_MSG_GET_PARAMS(arg);
> > > +	if ((params->attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> > > +			OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> > > +		goto bad;
> > > +
> > > +	switch (params->u.value.a) {
> > > +	case OPTEE_MSG_RPC_WAIT_QUEUE_SLEEP:
> > > +		wq_sleep(&optee->wait_queue, params->u.value.b);
> > > +		break;
> > > +	case OPTEE_MSG_RPC_WAIT_QUEUE_WAKEUP:
> > > +		wq_wakeup(&optee->wait_queue, params->u.value.b);
> > > +		break;
> > > +	default:
> > > +		goto bad;
> > > +	}
> > > +
> > > +	arg->ret = TEEC_SUCCESS;
> > > +	return;
> > > +bad:
> > > +	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > +}
> > > +
> > 
> > I'm trying to understand what this is good for. What I can see is that
> > you have a user space process calling into the kernel asking the tee
> > to do some command, and then the tee can ask the kernel to wait for
> > something to happen, or notify it that something has happened.
> > 
> > If we wait here, the user process gets suspended until this has
> > actually happened.
> > 
> > Am I reading this correctly? If yes, what is the intended use case?
> > Is there some process that is meant to always wait here? What
> > if we ever need to wait for more than one thing at a time (think
> > select or poll?)
> 
> I'm updating the comments for OPTEE_MSG_RPC_CMD_WAIT_QUEUE with:
> "If secure world need 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."
> 
> The way we're waking up a sleeping thread is a bit limiting in some
> circumstances.
> 
> One case is where we need to do it from a secure interrupt handler.
> Because there's no way of doing this kind of RPC to normal world from a
> secure interrupt handler. In that case it wouldn't be mutex the thread
> is waiting for though.
> 
> Another case is where there's several guest running in the system and
> more than one guests has access to secure world. If guest2 waits for
> mutex which guest1 is releasing, how can guest2 be notified? Normal RPC
> is impossible here also.
> 
> The only way around this limitation I've come up with so far is by doing
> the wakeup via a software generated interrupt destined to the correct
> guest. However this problem is beyond the scope of this patch set.

Ok, I see.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1564793 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote:
> On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> > > On Wednesday, January 18, 2017 1:58:14 PM CET Jens Wiklander wrote:
> 
> > > > +static int __init optee_driver_init(void)
> > > > +{
> > > > +	struct device_node *node;
> > > > +
> > > > +	/*
> > > > +	 * Preferred path is /firmware/optee, but it's the matching that
> > > > +	 * matters.
> > > > +	 */
> > > > +	for_each_matching_node(node, optee_match)
> > > > +		of_platform_device_create(node, NULL, NULL);
> > > > +
> > > > +	return platform_driver_register(&optee_driver);
> > > > +}
> > > > +module_init(optee_driver_init);
> > > > +
> > > > +static void __exit optee_driver_exit(void)
> > > > +{
> > > > +	platform_driver_unregister(&optee_driver);
> > > > +}
> > > > +module_exit(optee_driver_exit);
> > > 
> > > What is the platform driver good for if the same module has to create the
> > > platform devices itself?
> > 
> > The platform device(s) are created here because the optee node is below
> > "/firmware" instead of the root where it would have had the platform
> > device created automatically.
> > 
> > I think it's useful to be able to unload the module, the early reviews
> > of this patch set was much focused around that. Regardless I'll need
> > some device as parent for the devices created during optee_probe() and
> > using a platform device for that seems natural.
> > 
> > I'd rather keep the platform driver. Perhaps some variant of the pattern
> > in qcom_scm_init() (drivers/firmware/qcom_scm.c) is useful, except that
> > I need to find out what to do about the life cycle of the objects
> > created with of_platform_populate().
> 
> My point was that I don't think we need devices here at all. It's different
> when you talk to external hardware that has register resource etc that
> can be best abstracted as a real device, but for other firmware features
> we don't normally add one.
> 
> Module unloading can also be done without the device.
> 
> > > 
> > > I'd just skip it and do
> > > 
> > > 	for_each_matching_node(node, optee_match)
> > > 		optee_probe(node);
> > > 
> > > I also suspect that module unloading is broken here if you don't clean
> > > up the platform devices in the end, so you should already remove the
> > > exit function to prevent unloading.
> > 
> > Does the platform devices really need cleaning? I mean
> > of_platform_default_populate_init() creates a bunch of platform devices
> > which are just left there even if unused. Here we're doing the same
> > thing except that we're doing it for a specific node in the DT.
> 
> I think it will work if you don't clean them up, but it feels wrong
> to have a loadable module that creates devices when loaded but doesn't
> remove them when unloaded.
> 
> This could be done differently by having the device creation done in
> one driver and the the user of that device in another driver, but I
> think just killing off the device achieves the same in a simpler way.

I see your point. My final concern here is that with device we got
entries in sysfs and uevents that could be used to automatically start
the correct supplicant. Different drivers are likely to require
different supplicants. Starting the correct supplicant based on uevents
is a quite elegant solution which I'm not sure how to support when
skipping devices. Perhaps I could create an object below
<sysfs>/firmware/tee ?

> 
> > > > +/*
> > > > + * Get revision of Trusted OS.
> > > > + *
> > > > + * Used by non-secure world to figure out which version of the Trusted OS
> > > > + * is installed. Note that the returned revision is the revision of the
> > > > + * Trusted OS, not of the API.
> > > > + *
> > > > + * Returns revision in 2 32-bit words in the same way as
> > > > + * OPTEE_MSG_CALLS_REVISION described above.
> > > > + */
> > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR	1
> > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR	0
> > > > +#define OPTEE_MSG_FUNCID_GET_OS_REVISION	0x0001
> > > 
> > > Just for my understanding, what is the significance of these numbers,
> > > i.e. which code (user space, kernel driver, trusted OS) provides
> > > the uuid and which one provides the version? The code comments almost
> > > make sense to me, but I don't see why specific versions are listed
> > > in this header.
> > 
> > You're right, OPTEE_MSG_OS_OPTEE_REVISION_* should be removed. The
> > actual version the secure OS is of a mostly informational nature. The
> > same goes the OS UUID, but I suppose the actual UUID used by the
> > upstream version of OP-TEE OS could be interesting to know.
> ...
> > > What is the expected behavior when one side reports a version that
> > > is unknown? Can one side claim to be backwards compatible with
> > > a previous version, or does each new version need support on
> > > all three sides?
> > 
> > The UUID and version of the message protocol are important to match
> > correctly as otherwise it could mean that there's something unexpected
> > in secure world that following the message protocol would be undefined
> > behaviour. All changes to the message protocol should be backwards
> > compatible in the sense that the driver and secure world need to
> > negotiate eventual extensions while probing. That's what we're doing in
> > optee_msg_exchange_capabilities().
> 
> Ok, then maybe the "compatible" identifier in DT should be sufficient
> to ensure that the capability exchange works, and the rest be based
> on that?
> 
> We tend to avoid version checks for APIs in the kernel because they
> never work in practice, but the capability check should be fine.

UUID and version of the message protocol is required by ARM SMC Calling
Convention. It will be there anyway so we could just as well check it in
the probe function to catch eventual mismatches in configuration. Since
we're using capabilities to manage extensions of the protocol I think
the minor version could be ignored by probe.

Thanks,
Jens

[toc] | [prev] | [next] | [standalone]

#1565108 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Monday, January 23, 2017 10:08:53 AM CET Jens Wiklander wrote:
> On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote:
> > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:

> > > Does the platform devices really need cleaning? I mean
> > > of_platform_default_populate_init() creates a bunch of platform devices
> > > which are just left there even if unused. Here we're doing the same
> > > thing except that we're doing it for a specific node in the DT.
> > 
> > I think it will work if you don't clean them up, but it feels wrong
> > to have a loadable module that creates devices when loaded but doesn't
> > remove them when unloaded.
> > 
> > This could be done differently by having the device creation done in
> > one driver and the the user of that device in another driver, but I
> > think just killing off the device achieves the same in a simpler way.
> 
> I see your point. My final concern here is that with device we got
> entries in sysfs and uevents that could be used to automatically start
> the correct supplicant. Different drivers are likely to require
> different supplicants. Starting the correct supplicant based on uevents
> is a quite elegant solution which I'm not sure how to support when
> skipping devices. Perhaps I could create an object below
> <sysfs>/firmware/tee ?

Putting the objects somewhere other than /sys/devices sounds good, yes.
This would also help with TEE implementations that might get probed
differently.

I think the natural place would be /sys/class/tee/, as we normally
require something in /sys/class anyway to support the character
device.

/sys/firmware/tee/ sounds less fitting, as there other TEE implementations
are not necessarily firmware based, as you point out. 
/sys/firmware/op-tee certainly makes sense for anything that is specific
to OP-TEE in particular, while /sys/class/tee would be for anything
that uses the ioctl interface. This part is particularly important to
get right from the start, just like the ioctls themselves we can't make
incompatible changes here later once there are users relying on the
upstream kernel interfaces.

> > > > > +/*
> > > > > + * Get revision of Trusted OS.
> > > > > + *
> > > > > + * Used by non-secure world to figure out which version of the Trusted OS
> > > > > + * is installed. Note that the returned revision is the revision of the
> > > > > + * Trusted OS, not of the API.
> > > > > + *
> > > > > + * Returns revision in 2 32-bit words in the same way as
> > > > > + * OPTEE_MSG_CALLS_REVISION described above.
> > > > > + */
> > > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR	1
> > > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR	0
> > > > > +#define OPTEE_MSG_FUNCID_GET_OS_REVISION	0x0001
> > > > 
> > > > Just for my understanding, what is the significance of these numbers,
> > > > i.e. which code (user space, kernel driver, trusted OS) provides
> > > > the uuid and which one provides the version? The code comments almost
> > > > make sense to me, but I don't see why specific versions are listed
> > > > in this header.
> > > 
> > > You're right, OPTEE_MSG_OS_OPTEE_REVISION_* should be removed. The
> > > actual version the secure OS is of a mostly informational nature. The
> > > same goes the OS UUID, but I suppose the actual UUID used by the
> > > upstream version of OP-TEE OS could be interesting to know.
> > ...
> > > > What is the expected behavior when one side reports a version that
> > > > is unknown? Can one side claim to be backwards compatible with
> > > > a previous version, or does each new version need support on
> > > > all three sides?
> > > 
> > > The UUID and version of the message protocol are important to match
> > > correctly as otherwise it could mean that there's something unexpected
> > > in secure world that following the message protocol would be undefined
> > > behaviour. All changes to the message protocol should be backwards
> > > compatible in the sense that the driver and secure world need to
> > > negotiate eventual extensions while probing. That's what we're doing in
> > > optee_msg_exchange_capabilities().
> > 
> > Ok, then maybe the "compatible" identifier in DT should be sufficient
> > to ensure that the capability exchange works, and the rest be based
> > on that?
> > 
> > We tend to avoid version checks for APIs in the kernel because they
> > never work in practice, but the capability check should be fine.
> 
> UUID and version of the message protocol is required by ARM SMC Calling
> Convention. It will be there anyway so we could just as well check it in
> the probe function to catch eventual mismatches in configuration. Since
> we're using capabilities to manage extensions of the protocol I think
> the minor version could be ignored by probe.

Ok, makes sense.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1565833 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Mon, Jan 23, 2017 at 05:16:15PM +0100, Arnd Bergmann wrote:
> On Monday, January 23, 2017 10:08:53 AM CET Jens Wiklander wrote:
> > On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote:
> > > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> > > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> 
> > > > Does the platform devices really need cleaning? I mean
> > > > of_platform_default_populate_init() creates a bunch of platform devices
> > > > which are just left there even if unused. Here we're doing the same
> > > > thing except that we're doing it for a specific node in the DT.
> > > 
> > > I think it will work if you don't clean them up, but it feels wrong
> > > to have a loadable module that creates devices when loaded but doesn't
> > > remove them when unloaded.
> > > 
> > > This could be done differently by having the device creation done in
> > > one driver and the the user of that device in another driver, but I
> > > think just killing off the device achieves the same in a simpler way.
> > 
> > I see your point. My final concern here is that with device we got
> > entries in sysfs and uevents that could be used to automatically start
> > the correct supplicant. Different drivers are likely to require
> > different supplicants. Starting the correct supplicant based on uevents
> > is a quite elegant solution which I'm not sure how to support when
> > skipping devices. Perhaps I could create an object below
> > <sysfs>/firmware/tee ?
> 
> Putting the objects somewhere other than /sys/devices sounds good, yes.
> This would also help with TEE implementations that might get probed
> differently.
> 
> I think the natural place would be /sys/class/tee/, as we normally
> require something in /sys/class anyway to support the character
> device.
> 
> /sys/firmware/tee/ sounds less fitting, as there other TEE implementations
> are not necessarily firmware based, as you point out. 
> /sys/firmware/op-tee certainly makes sense for anything that is specific
> to OP-TEE in particular, while /sys/class/tee would be for anything
> that uses the ioctl interface. This part is particularly important to
> get right from the start, just like the ioctls themselves we can't make
> incompatible changes here later once there are users relying on the
> upstream kernel interfaces.

/sys/class/tee/ sounds good, I'll use that. It's more or less what we
also have today.

Thanks for the help with this review.

Jens

[toc] | [prev] | [next] | [standalone]

#1566427 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Tue, Jan 24, 2017 at 01:53:30PM +0100, Jens Wiklander wrote:
> On Mon, Jan 23, 2017 at 05:16:15PM +0100, Arnd Bergmann wrote:
> > On Monday, January 23, 2017 10:08:53 AM CET Jens Wiklander wrote:
> > > On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote:
> > > > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> > > > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> > 
> > > > > Does the platform devices really need cleaning? I mean
> > > > > of_platform_default_populate_init() creates a bunch of platform devices
> > > > > which are just left there even if unused. Here we're doing the same
> > > > > thing except that we're doing it for a specific node in the DT.
> > > > 
> > > > I think it will work if you don't clean them up, but it feels wrong
> > > > to have a loadable module that creates devices when loaded but doesn't
> > > > remove them when unloaded.
> > > > 
> > > > This could be done differently by having the device creation done in
> > > > one driver and the the user of that device in another driver, but I
> > > > think just killing off the device achieves the same in a simpler way.
> > > 
> > > I see your point. My final concern here is that with device we got
> > > entries in sysfs and uevents that could be used to automatically start
> > > the correct supplicant. Different drivers are likely to require
> > > different supplicants. Starting the correct supplicant based on uevents
> > > is a quite elegant solution which I'm not sure how to support when
> > > skipping devices. Perhaps I could create an object below
> > > <sysfs>/firmware/tee ?
> > 
> > Putting the objects somewhere other than /sys/devices sounds good, yes.
> > This would also help with TEE implementations that might get probed
> > differently.
> > 
> > I think the natural place would be /sys/class/tee/, as we normally
> > require something in /sys/class anyway to support the character
> > device.
> > 
> > /sys/firmware/tee/ sounds less fitting, as there other TEE implementations
> > are not necessarily firmware based, as you point out. 
> > /sys/firmware/op-tee certainly makes sense for anything that is specific
> > to OP-TEE in particular, while /sys/class/tee would be for anything
> > that uses the ioctl interface. This part is particularly important to
> > get right from the start, just like the ioctls themselves we can't make
> > incompatible changes here later once there are users relying on the
> > upstream kernel interfaces.
> 
> /sys/class/tee/ sounds good, I'll use that. It's more or less what we
> also have today.

I'm sorry, it seems a struct device has to be used in order to put stuff
under /sys/class/tee/. Or am I missing something?

Thanks,
Jens

[toc] | [prev] | [next] | [standalone]

#1566443 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Wed, Jan 25, 2017 at 10:47:45AM +0100, Jens Wiklander wrote:
> On Tue, Jan 24, 2017 at 01:53:30PM +0100, Jens Wiklander wrote:
> > On Mon, Jan 23, 2017 at 05:16:15PM +0100, Arnd Bergmann wrote:
> > > On Monday, January 23, 2017 10:08:53 AM CET Jens Wiklander wrote:
> > > > On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote:
> > > > > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote:
> > > > > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote:
> > > 
> > > > > > Does the platform devices really need cleaning? I mean
> > > > > > of_platform_default_populate_init() creates a bunch of platform devices
> > > > > > which are just left there even if unused. Here we're doing the same
> > > > > > thing except that we're doing it for a specific node in the DT.
> > > > > 
> > > > > I think it will work if you don't clean them up, but it feels wrong
> > > > > to have a loadable module that creates devices when loaded but doesn't
> > > > > remove them when unloaded.
> > > > > 
> > > > > This could be done differently by having the device creation done in
> > > > > one driver and the the user of that device in another driver, but I
> > > > > think just killing off the device achieves the same in a simpler way.
> > > > 
> > > > I see your point. My final concern here is that with device we got
> > > > entries in sysfs and uevents that could be used to automatically start
> > > > the correct supplicant. Different drivers are likely to require
> > > > different supplicants. Starting the correct supplicant based on uevents
> > > > is a quite elegant solution which I'm not sure how to support when
> > > > skipping devices. Perhaps I could create an object below
> > > > <sysfs>/firmware/tee ?
> > > 
> > > Putting the objects somewhere other than /sys/devices sounds good, yes.
> > > This would also help with TEE implementations that might get probed
> > > differently.
> > > 
> > > I think the natural place would be /sys/class/tee/, as we normally
> > > require something in /sys/class anyway to support the character
> > > device.
> > > 
> > > /sys/firmware/tee/ sounds less fitting, as there other TEE implementations
> > > are not necessarily firmware based, as you point out. 
> > > /sys/firmware/op-tee certainly makes sense for anything that is specific
> > > to OP-TEE in particular, while /sys/class/tee would be for anything
> > > that uses the ioctl interface. This part is particularly important to
> > > get right from the start, just like the ioctls themselves we can't make
> > > incompatible changes here later once there are users relying on the
> > > upstream kernel interfaces.
> > 
> > /sys/class/tee/ sounds good, I'll use that. It's more or less what we
> > also have today.
> 
> I'm sorry, it seems a struct device has to be used in order to put stuff
> under /sys/class/tee/. Or am I missing something?

Nope, that is correct.

[toc] | [prev] | [next] | [standalone]

#1566495 — Re: [PATCH v14 3/5] tee: add OP-TEE driver

On Wed, Jan 25, 2017 at 11:02 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 25, 2017 at 10:47:45AM +0100, Jens Wiklander wrote:
>> On Tue, Jan 24, 2017 at 01:53:30PM +0100, Jens Wiklander wrote:
>> >
>> > /sys/class/tee/ sounds good, I'll use that. It's more or less what we
>> > also have today.
>>
>> I'm sorry, it seems a struct device has to be used in order to put stuff
>> under /sys/class/tee/. Or am I missing something?
>
> Nope, that is correct.

But that's a different device: this device represents the "class" of
device for the
user space interface, nothing wrong with that. However, we don't need both
the class device and a platform device that is made up by the optee driver
for a firmware feature to represent the same entity that we have the
class device
for.

     Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web