[04/10] Add UCLASS_TEE for Trusted Execution Environment

Message ID 20180813155347.13844-5-jens.wiklander@linaro.org
State New
Headers show
Series
  • AVB using OP-TEE
Related show

Commit Message

Jens Wiklander Aug. 13, 2018, 3:53 p.m.
Adds a uclass to interface with a TEE (Trusted Execution Environment).

A TEE driver is a driver that interfaces with a trusted OS running in
some secure environment, for example, TrustZone on ARM cpus, or a
separate secure co-processor etc.

The TEE subsystem can serve a TEE driver for a Global Platform compliant
TEE, but it's not limited to only Global Platform TEEs.

The over all design is based on the TEE subsystem in the Linux kernel,
tailored for U-boot.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

---
 MAINTAINERS              |   6 ++
 drivers/Kconfig          |   2 +
 drivers/Makefile         |   1 +
 drivers/tee/Kconfig      |   8 ++
 drivers/tee/Makefile     |   3 +
 drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/tee.h            | 134 +++++++++++++++++++++++++++++
 8 files changed, 335 insertions(+)
 create mode 100644 drivers/tee/Kconfig
 create mode 100644 drivers/tee/Makefile
 create mode 100644 drivers/tee/tee-uclass.c
 create mode 100644 include/tee.h

-- 
2.17.1

Comments

Igor Opaniuk Aug. 16, 2018, 12:14 p.m. | #1
Tested this on Poplar:

Tested-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 13 August 2018 at 18:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> Adds a uclass to interface with a TEE (Trusted Execution Environment).
>
> A TEE driver is a driver that interfaces with a trusted OS running in
> some secure environment, for example, TrustZone on ARM cpus, or a
> separate secure co-processor etc.
>
> The TEE subsystem can serve a TEE driver for a Global Platform compliant
> TEE, but it's not limited to only Global Platform TEEs.
>
> The over all design is based on the TEE subsystem in the Linux kernel,
> tailored for U-boot.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  MAINTAINERS              |   6 ++
>  drivers/Kconfig          |   2 +
>  drivers/Makefile         |   1 +
>  drivers/tee/Kconfig      |   8 ++
>  drivers/tee/Makefile     |   3 +
>  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |   1 +
>  include/tee.h            | 134 +++++++++++++++++++++++++++++
>  8 files changed, 335 insertions(+)
>  create mode 100644 drivers/tee/Kconfig
>  create mode 100644 drivers/tee/Makefile
>  create mode 100644 drivers/tee/tee-uclass.c
>  create mode 100644 include/tee.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b61ac05882..7458c606ee92 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -571,6 +571,12 @@ TQ GROUP
>  S:     Orphaned (Since 2016-02)
>  T:     git git://git.denx.de/u-boot-tq-group.git
>
> +TEE
> +M:     Jens Wiklander <jens.wiklander@linaro.org>
> +S:     Maintained
> +F:     drivers/tee/
> +F:     include/tee.h
> +
>  UBI
>  M:     Kyungmin Park <kmpark@infradead.org>
>  M:     Heiko Schocher <hs@denx.de>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index c72abf893297..f3249ab1d143 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
>
>  source "drivers/sysreset/Kconfig"
>
> +source "drivers/tee/Kconfig"
> +
>  source "drivers/thermal/Kconfig"
>
>  source "drivers/timer/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index d53208540ea6..0fcae36f50f7 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -103,6 +103,7 @@ obj-y += smem/
>  obj-y += soc/
>  obj-$(CONFIG_REMOTEPROC) += remoteproc/
>  obj-y += thermal/
> +obj-$(CONFIG_TEE) += tee/
>
>  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
>  endif
> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> new file mode 100644
> index 000000000000..817ab331b0f8
> --- /dev/null
> +++ b/drivers/tee/Kconfig
> @@ -0,0 +1,8 @@
> +# Generic Trusted Execution Environment Configuration
> +config TEE
> +       bool "Trusted Execution Environment support"
> +       depends on ARM && (ARM64 || CPU_V7A)
> +       select ARM_SMCCC
> +       help
> +         This implements a generic interface towards a Trusted Execution
> +         Environment (TEE).
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> new file mode 100644
> index 000000000000..b6d8e16e6211
> --- /dev/null
> +++ b/drivers/tee/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +obj-y += tee-uclass.o
> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
> new file mode 100644
> index 000000000000..f0243a0f2e4e
> --- /dev/null
> +++ b/drivers/tee/tee-uclass.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <tee.h>
> +
> +struct tee_uclass_priv {
> +       struct list_head list_shm;
> +};
> +
> +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
> +{
> +       return device_get_ops(dev);
> +}
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
> +{
> +       tee_get_ops(dev)->get_version(dev, vers);
> +}
> +
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
> +}
> +
> +int tee_close_session(struct udevice *dev, u32 session)
> +{
> +       return tee_get_ops(dev)->close_session(dev, session);
> +}
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
> +}
> +
> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags)
> +{
> +       struct tee_shm *shm;
> +       void *p = addr;
> +
> +       if (flags & TEE_SHM_ALLOC) {
> +               if (align)
> +                       p = memalign(align, size);
> +               else
> +                       p = malloc(size);
> +       }
> +       if (!p)
> +               return NULL;
> +
> +       shm = calloc(1, sizeof(*shm));
> +       if (!shm)
> +               goto err;
> +
> +       shm->dev = dev;
> +       shm->addr = p;
> +       shm->size = size;
> +       shm->flags = flags;
> +
> +       if ((flags & TEE_SHM_SEC_REGISTER) &&
> +           tee_get_ops(dev)->shm_register(dev, shm))
> +               goto err;
> +
> +       if (flags & TEE_SHM_REGISTER) {
> +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +               list_add(&shm->link, &priv->list_shm);
> +       }
> +       return shm;
> +err:
> +       free(shm);
> +       if (flags & TEE_SHM_ALLOC)
> +               free(p);
> +       return NULL;
> +}
> +
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
> +                             u32 flags)
> +{
> +       u32 f = flags;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
> +       return __tee_shm_add(dev, 0, NULL, size, f);
> +}
> +
> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong size, u32 flags)
> +{
> +       u32 f = flags & ~TEE_SHM_ALLOC;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
> +       return __tee_shm_add(dev, 0, addr, size, f);
> +}
> +
> +void tee_shm_free(struct tee_shm *shm)
> +{
> +       if (!shm)
> +               return;
> +
> +       if (shm->flags & TEE_SHM_SEC_REGISTER)
> +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
> +
> +       if (shm->flags & TEE_SHM_REGISTER)
> +               list_del(&shm->link);
> +
> +       if (shm->flags & TEE_SHM_ALLOC)
> +               free(shm->addr);
> +
> +       free(shm);
> +}
> +
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *s;
> +
> +       list_for_each_entry(s, &priv->list_shm, link)
> +               if (s == shm)
> +                       return true;
> +
> +       return false;
> +}
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers)
> +{
> +       struct udevice *dev = start;
> +       struct tee_version_data lv;
> +       struct tee_version_data *v = vers ? vers : &lv;
> +
> +       if (!dev)
> +               uclass_first_device(UCLASS_TEE, &dev);
> +
> +       for (; dev; uclass_next_device(&dev)) {
> +               tee_get_ops(dev)->get_version(dev, v);
> +               if (!match || match(v, data))
> +                       return dev;
> +       }
> +
> +       return NULL;
> +}
> +
> +static int tee_pre_probe(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +       INIT_LIST_HEAD(&priv->list_shm);
> +       return 0;
> +}
> +
> +static int tee_pre_remove(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *shm;
> +
> +       while (!list_empty(&priv->list_shm)) {
> +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
> +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
> +                     __func__, (void *)shm, shm->size, shm->flags);
> +               tee_shm_free(shm);
> +       }
> +
> +       return 0;
> +}
> +
> +UCLASS_DRIVER(tee) = {
> +       .id = UCLASS_TEE,
> +       .name = "tee",
> +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
> +       .pre_probe = tee_pre_probe,
> +       .pre_remove = tee_pre_remove,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index a39643ec5eef..955e0a915b87 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -81,6 +81,7 @@ enum uclass_id {
>         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
>         UCLASS_SYSCON,          /* System configuration device */
>         UCLASS_SYSRESET,        /* System reset device */
> +       UCLASS_TEE,             /* Trusted Execution Environment device */
>         UCLASS_THERMAL,         /* Thermal sensor */
>         UCLASS_TIMER,           /* Timer device */
>         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
> diff --git a/include/tee.h b/include/tee.h
> new file mode 100644
> index 000000000000..c2ac13e34128
> --- /dev/null
> +++ b/include/tee.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#ifndef __TEE_H
> +#define __TEE_H
> +
> +#include <common.h>
> +#include <dm.h>
> +
> +#define TEE_UUID_LEN           16
> +
> +#define TEE_GEN_CAP_GP          BIT(0) /* GlobalPlatform compliant TEE */
> +#define TEE_GEN_CAP_REG_MEM     BIT(1) /* Supports registering shared memory */
> +
> +#define TEE_SHM_REGISTER       BIT(0)
> +#define TEE_SHM_SEC_REGISTER   BIT(1)
> +#define TEE_SHM_ALLOC          BIT(2)
> +
> +#define TEE_PARAM_ATTR_TYPE_NONE               0       /* parameter not used */
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT                1
> +#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT       2
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT                3       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT       5
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT      6
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT       7       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MASK               0xff
> +#define TEE_PARAM_ATTR_META                    0x100
> +#define TEE_PARAM_ATTR_MASK                    (TEE_PARAM_ATTR_TYPE_MASK | \
> +                                                TEE_PARAM_ATTR_META)
> +
> +/*
> + * Some Global Platform error codes which has a meaning if the
> + * TEE_GEN_CAP_GP bit is returned by the driver.
> + */
> +#define TEE_SUCCESS                    0x00000000
> +#define TEE_ERROR_GENERIC              0xffff0000
> +#define TEE_ERROR_BAD_PARAMETERS       0xffff0006
> +#define TEE_ERROR_ITEM_NOT_FOUND       0xffff0008
> +#define TEE_ERROR_NOT_IMPLEMENTED      0xffff0009
> +#define TEE_ERROR_NOT_SUPPORTED                0xffff000a
> +#define TEE_ERROR_COMMUNICATION                0xffff000e
> +#define TEE_ERROR_OUT_OF_MEMORY                0xffff000c
> +#define TEE_ERROR_TARGET_DEAD          0xffff3024
> +
> +#define TEE_ORIGIN_COMMS               0x00000002
> +
> +struct tee_driver_ops;
> +
> +struct tee_shm {
> +       struct udevice *dev;
> +       struct list_head link;
> +       void *addr;
> +       ulong size;
> +       u32 flags;
> +};
> +
> +struct tee_param_memref {
> +       ulong shm_offs;
> +       ulong size;
> +       struct tee_shm *shm;
> +};
> +
> +struct tee_param_value {
> +       u64 a;
> +       u64 b;
> +       u64 c;
> +};
> +
> +struct tee_param {
> +       u64 attr;
> +       union {
> +               struct tee_param_memref memref;
> +               struct tee_param_value value;
> +       } u;
> +};
> +
> +struct tee_open_session_arg {
> +       u8 uuid[TEE_UUID_LEN];
> +       u8 clnt_uuid[TEE_UUID_LEN];
> +       u32 clnt_login;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_invoke_arg {
> +       u32 func;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_version_data {
> +       u32 gen_caps;
> +};
> +
> +struct tee_driver_ops {
> +       void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
> +       int (*open_session)(struct udevice *dev,
> +                           struct tee_open_session_arg *arg, ulong num_param,
> +                           struct tee_param *param);
> +       int (*close_session)(struct udevice *dev, u32 session);
> +       int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
> +                          ulong num_param, struct tee_param *param);
> +       int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
> +       int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
> +};
> +
> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags);
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);
> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong length, u32 flags);
> +void tee_shm_free(struct tee_shm *shm);
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers);
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param);
> +
> +int tee_close_session(struct udevice *dev, u32 session);
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param);
> +
> +#endif /*__TEE_H*/
> --
> 2.17.1
>
Simon Glass Aug. 17, 2018, 12:48 p.m. | #2
Hi Jens,

On 13 August 2018 at 09:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> Adds a uclass to interface with a TEE (Trusted Execution Environment).
>
> A TEE driver is a driver that interfaces with a trusted OS running in
> some secure environment, for example, TrustZone on ARM cpus, or a
> separate secure co-processor etc.
>
> The TEE subsystem can serve a TEE driver for a Global Platform compliant
> TEE, but it's not limited to only Global Platform TEEs.
>
> The over all design is based on the TEE subsystem in the Linux kernel,
> tailored for U-boot.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  MAINTAINERS              |   6 ++
>  drivers/Kconfig          |   2 +
>  drivers/Makefile         |   1 +
>  drivers/tee/Kconfig      |   8 ++
>  drivers/tee/Makefile     |   3 +
>  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |   1 +
>  include/tee.h            | 134 +++++++++++++++++++++++++++++
>  8 files changed, 335 insertions(+)
>  create mode 100644 drivers/tee/Kconfig
>  create mode 100644 drivers/tee/Makefile
>  create mode 100644 drivers/tee/tee-uclass.c
>  create mode 100644 include/tee.h
>

Please add a sandbox driver and a test in test/dm for any new uclass.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b61ac05882..7458c606ee92 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -571,6 +571,12 @@ TQ GROUP
>  S:     Orphaned (Since 2016-02)
>  T:     git git://git.denx.de/u-boot-tq-group.git
>
> +TEE
> +M:     Jens Wiklander <jens.wiklander@linaro.org>
> +S:     Maintained
> +F:     drivers/tee/
> +F:     include/tee.h
> +
>  UBI
>  M:     Kyungmin Park <kmpark@infradead.org>
>  M:     Heiko Schocher <hs@denx.de>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index c72abf893297..f3249ab1d143 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
>
>  source "drivers/sysreset/Kconfig"
>
> +source "drivers/tee/Kconfig"
> +
>  source "drivers/thermal/Kconfig"
>
>  source "drivers/timer/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index d53208540ea6..0fcae36f50f7 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -103,6 +103,7 @@ obj-y += smem/
>  obj-y += soc/
>  obj-$(CONFIG_REMOTEPROC) += remoteproc/
>  obj-y += thermal/
> +obj-$(CONFIG_TEE) += tee/
>
>  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
>  endif
> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> new file mode 100644
> index 000000000000..817ab331b0f8
> --- /dev/null
> +++ b/drivers/tee/Kconfig
> @@ -0,0 +1,8 @@
> +# Generic Trusted Execution Environment Configuration
> +config TEE
> +       bool "Trusted Execution Environment support"
> +       depends on ARM && (ARM64 || CPU_V7A)
> +       select ARM_SMCCC
> +       help
> +         This implements a generic interface towards a Trusted Execution
> +         Environment (TEE).
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> new file mode 100644
> index 000000000000..b6d8e16e6211
> --- /dev/null
> +++ b/drivers/tee/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +obj-y += tee-uclass.o
> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
> new file mode 100644
> index 000000000000..f0243a0f2e4e
> --- /dev/null
> +++ b/drivers/tee/tee-uclass.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <tee.h>
> +
> +struct tee_uclass_priv {
> +       struct list_head list_shm;

Struct comment please

> +};
> +
> +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
> +{
> +       return device_get_ops(dev);
> +}
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
> +{
> +       tee_get_ops(dev)->get_version(dev, vers);
> +}
> +
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
> +}
> +
> +int tee_close_session(struct udevice *dev, u32 session)
> +{
> +       return tee_get_ops(dev)->close_session(dev, session);
> +}
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
> +}
> +
> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags)
> +{
> +       struct tee_shm *shm;
> +       void *p = addr;
> +
> +       if (flags & TEE_SHM_ALLOC) {
> +               if (align)
> +                       p = memalign(align, size);
> +               else
> +                       p = malloc(size);
> +       }
> +       if (!p)
> +               return NULL;
> +
> +       shm = calloc(1, sizeof(*shm));

Rather than allocating memory here, can you use driver-model's
auto-alloc features?

> +       if (!shm)
> +               goto err;
> +
> +       shm->dev = dev;
> +       shm->addr = p;
> +       shm->size = size;
> +       shm->flags = flags;
> +
> +       if ((flags & TEE_SHM_SEC_REGISTER) &&
> +           tee_get_ops(dev)->shm_register(dev, shm))
> +               goto err;
> +
> +       if (flags & TEE_SHM_REGISTER) {
> +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +               list_add(&shm->link, &priv->list_shm);
> +       }
> +       return shm;
> +err:
> +       free(shm);
> +       if (flags & TEE_SHM_ALLOC)
> +               free(p);
> +       return NULL;
> +}
> +
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
> +                             u32 flags)
> +{
> +       u32 f = flags;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
> +       return __tee_shm_add(dev, 0, NULL, size, f);
> +}
> +
> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong size, u32 flags)
> +{
> +       u32 f = flags & ~TEE_SHM_ALLOC;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
> +       return __tee_shm_add(dev, 0, addr, size, f);
> +}
> +
> +void tee_shm_free(struct tee_shm *shm)
> +{
> +       if (!shm)
> +               return;
> +
> +       if (shm->flags & TEE_SHM_SEC_REGISTER)
> +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
> +
> +       if (shm->flags & TEE_SHM_REGISTER)
> +               list_del(&shm->link);
> +
> +       if (shm->flags & TEE_SHM_ALLOC)
> +               free(shm->addr);
> +
> +       free(shm);
> +}
> +
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *s;
> +
> +       list_for_each_entry(s, &priv->list_shm, link)
> +               if (s == shm)
> +                       return true;
> +
> +       return false;
> +}
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers)

'find' should find the device, not probe anything. Is that what this does?

> +{
> +       struct udevice *dev = start;
> +       struct tee_version_data lv;
> +       struct tee_version_data *v = vers ? vers : &lv;
> +
> +       if (!dev)
> +               uclass_first_device(UCLASS_TEE, &dev);
> +
> +       for (; dev; uclass_next_device(&dev)) {
> +               tee_get_ops(dev)->get_version(dev, v);
> +               if (!match || match(v, data))
> +                       return dev;
> +       }
> +
> +       return NULL;
> +}
> +
> +static int tee_pre_probe(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +       INIT_LIST_HEAD(&priv->list_shm);

blank line before return

> +       return 0;
> +}
> +
> +static int tee_pre_remove(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *shm;
> +
> +       while (!list_empty(&priv->list_shm)) {
> +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
> +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
> +                     __func__, (void *)shm, shm->size, shm->flags);
> +               tee_shm_free(shm);
> +       }
> +
> +       return 0;
> +}
> +
> +UCLASS_DRIVER(tee) = {
> +       .id = UCLASS_TEE,
> +       .name = "tee",
> +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
> +       .pre_probe = tee_pre_probe,
> +       .pre_remove = tee_pre_remove,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index a39643ec5eef..955e0a915b87 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -81,6 +81,7 @@ enum uclass_id {
>         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
>         UCLASS_SYSCON,          /* System configuration device */
>         UCLASS_SYSRESET,        /* System reset device */
> +       UCLASS_TEE,             /* Trusted Execution Environment device */
>         UCLASS_THERMAL,         /* Thermal sensor */
>         UCLASS_TIMER,           /* Timer device */
>         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
> diff --git a/include/tee.h b/include/tee.h
> new file mode 100644
> index 000000000000..c2ac13e34128
> --- /dev/null
> +++ b/include/tee.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#ifndef __TEE_H
> +#define __TEE_H
> +
> +#include <common.h>
> +#include <dm.h>
> +
> +#define TEE_UUID_LEN           16
> +
> +#define TEE_GEN_CAP_GP          BIT(0) /* GlobalPlatform compliant TEE */
> +#define TEE_GEN_CAP_REG_MEM     BIT(1) /* Supports registering shared memory */
> +
> +#define TEE_SHM_REGISTER       BIT(0)
> +#define TEE_SHM_SEC_REGISTER   BIT(1)
> +#define TEE_SHM_ALLOC          BIT(2)
> +
> +#define TEE_PARAM_ATTR_TYPE_NONE               0       /* parameter not used */
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT                1
> +#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT       2
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT                3       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT       5
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT      6
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT       7       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MASK               0xff
> +#define TEE_PARAM_ATTR_META                    0x100
> +#define TEE_PARAM_ATTR_MASK                    (TEE_PARAM_ATTR_TYPE_MASK | \
> +                                                TEE_PARAM_ATTR_META)
> +
> +/*
> + * Some Global Platform error codes which has a meaning if the
> + * TEE_GEN_CAP_GP bit is returned by the driver.
> + */
> +#define TEE_SUCCESS                    0x00000000
> +#define TEE_ERROR_GENERIC              0xffff0000
> +#define TEE_ERROR_BAD_PARAMETERS       0xffff0006
> +#define TEE_ERROR_ITEM_NOT_FOUND       0xffff0008
> +#define TEE_ERROR_NOT_IMPLEMENTED      0xffff0009
> +#define TEE_ERROR_NOT_SUPPORTED                0xffff000a
> +#define TEE_ERROR_COMMUNICATION                0xffff000e
> +#define TEE_ERROR_OUT_OF_MEMORY                0xffff000c
> +#define TEE_ERROR_TARGET_DEAD          0xffff3024
> +
> +#define TEE_ORIGIN_COMMS               0x00000002
> +
> +struct tee_driver_ops;
> +
> +struct tee_shm {

comments for these structs please.

Also the whole uclass needs a README or more docs. Perhaps I have missed this?

> +       struct udevice *dev;
> +       struct list_head link;
> +       void *addr;
> +       ulong size;
> +       u32 flags;
> +};
> +
> +struct tee_param_memref {
> +       ulong shm_offs;
> +       ulong size;
> +       struct tee_shm *shm;
> +};
> +
> +struct tee_param_value {
> +       u64 a;
> +       u64 b;
> +       u64 c;
> +};
> +
> +struct tee_param {
> +       u64 attr;
> +       union {
> +               struct tee_param_memref memref;
> +               struct tee_param_value value;
> +       } u;
> +};
> +
> +struct tee_open_session_arg {
> +       u8 uuid[TEE_UUID_LEN];
> +       u8 clnt_uuid[TEE_UUID_LEN];
> +       u32 clnt_login;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_invoke_arg {
> +       u32 func;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_version_data {
> +       u32 gen_caps;
> +};
> +
> +struct tee_driver_ops {
> +       void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
> +       int (*open_session)(struct udevice *dev,
> +                           struct tee_open_session_arg *arg, ulong num_param,
> +                           struct tee_param *param);
> +       int (*close_session)(struct udevice *dev, u32 session);
> +       int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
> +                          ulong num_param, struct tee_param *param);
> +       int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
> +       int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);

Each of these needs a full function comment. Without that I cannot
really comment on the API. Same for functions below.

> +};
> +
> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags);
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);
> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong length, u32 flags);
> +void tee_shm_free(struct tee_shm *shm);
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers);
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param);
> +
> +int tee_close_session(struct udevice *dev, u32 session);
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param);
> +
> +#endif /*__TEE_H*/
> --
> 2.17.1
>

Regards,
Simon
Jens Wiklander Aug. 21, 2018, 9:20 a.m. | #3
Hi Simon,

On Fri, Aug 17, 2018 at 06:48:47AM -0600, Simon Glass wrote:
> Hi Jens,
> 
> On 13 August 2018 at 09:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > Adds a uclass to interface with a TEE (Trusted Execution Environment).
> >
> > A TEE driver is a driver that interfaces with a trusted OS running in
> > some secure environment, for example, TrustZone on ARM cpus, or a
> > separate secure co-processor etc.
> >
> > The TEE subsystem can serve a TEE driver for a Global Platform compliant
> > TEE, but it's not limited to only Global Platform TEEs.
> >
> > The over all design is based on the TEE subsystem in the Linux kernel,
> > tailored for U-boot.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  MAINTAINERS              |   6 ++
> >  drivers/Kconfig          |   2 +
> >  drivers/Makefile         |   1 +
> >  drivers/tee/Kconfig      |   8 ++
> >  drivers/tee/Makefile     |   3 +
> >  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h   |   1 +
> >  include/tee.h            | 134 +++++++++++++++++++++++++++++
> >  8 files changed, 335 insertions(+)
> >  create mode 100644 drivers/tee/Kconfig
> >  create mode 100644 drivers/tee/Makefile
> >  create mode 100644 drivers/tee/tee-uclass.c
> >  create mode 100644 include/tee.h
> >
> 
> Please add a sandbox driver and a test in test/dm for any new uclass.

OK

> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 58b61ac05882..7458c606ee92 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -571,6 +571,12 @@ TQ GROUP
> >  S:     Orphaned (Since 2016-02)
> >  T:     git git://git.denx.de/u-boot-tq-group.git
> >
> > +TEE
> > +M:     Jens Wiklander <jens.wiklander@linaro.org>
> > +S:     Maintained
> > +F:     drivers/tee/
> > +F:     include/tee.h
> > +
> >  UBI
> >  M:     Kyungmin Park <kmpark@infradead.org>
> >  M:     Heiko Schocher <hs@denx.de>
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index c72abf893297..f3249ab1d143 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
> >
> >  source "drivers/sysreset/Kconfig"
> >
> > +source "drivers/tee/Kconfig"
> > +
> >  source "drivers/thermal/Kconfig"
> >
> >  source "drivers/timer/Kconfig"
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index d53208540ea6..0fcae36f50f7 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -103,6 +103,7 @@ obj-y += smem/
> >  obj-y += soc/
> >  obj-$(CONFIG_REMOTEPROC) += remoteproc/
> >  obj-y += thermal/
> > +obj-$(CONFIG_TEE) += tee/
> >
> >  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
> >  endif
> > diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> > new file mode 100644
> > index 000000000000..817ab331b0f8
> > --- /dev/null
> > +++ b/drivers/tee/Kconfig
> > @@ -0,0 +1,8 @@
> > +# Generic Trusted Execution Environment Configuration
> > +config TEE
> > +       bool "Trusted Execution Environment support"
> > +       depends on ARM && (ARM64 || CPU_V7A)
> > +       select ARM_SMCCC
> > +       help
> > +         This implements a generic interface towards a Trusted Execution
> > +         Environment (TEE).
> > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > new file mode 100644
> > index 000000000000..b6d8e16e6211
> > --- /dev/null
> > +++ b/drivers/tee/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +
> > +obj-y += tee-uclass.o
> > diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
> > new file mode 100644
> > index 000000000000..f0243a0f2e4e
> > --- /dev/null
> > +++ b/drivers/tee/tee-uclass.c
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2018 Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <tee.h>
> > +
> > +struct tee_uclass_priv {
> > +       struct list_head list_shm;
> 
> Struct comment please
> 
> > +};
> > +
> > +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
> > +{
> > +       return device_get_ops(dev);
> > +}
> > +
> > +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
> > +{
> > +       tee_get_ops(dev)->get_version(dev, vers);
> > +}
> > +
> > +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> > +                    ulong num_param, struct tee_param *param)
> > +{
> > +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
> > +}
> > +
> > +int tee_close_session(struct udevice *dev, u32 session)
> > +{
> > +       return tee_get_ops(dev)->close_session(dev, session);
> > +}
> > +
> > +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> > +                   ulong num_param, struct tee_param *param)
> > +{
> > +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
> > +}
> > +
> > +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> > +                             ulong size, u32 flags)
> > +{
> > +       struct tee_shm *shm;
> > +       void *p = addr;
> > +
> > +       if (flags & TEE_SHM_ALLOC) {
> > +               if (align)
> > +                       p = memalign(align, size);
> > +               else
> > +                       p = malloc(size);
> > +       }
> > +       if (!p)
> > +               return NULL;
> > +
> > +       shm = calloc(1, sizeof(*shm));
> 
> Rather than allocating memory here, can you use driver-model's
> auto-alloc features?

I don't see how that can be done, these shared memory objects are
dynamically allocated by the client. The number of needed objects isn't
easily predicted.

> 
> > +       if (!shm)
> > +               goto err;
> > +
> > +       shm->dev = dev;
> > +       shm->addr = p;
> > +       shm->size = size;
> > +       shm->flags = flags;
> > +
> > +       if ((flags & TEE_SHM_SEC_REGISTER) &&
> > +           tee_get_ops(dev)->shm_register(dev, shm))
> > +               goto err;
> > +
> > +       if (flags & TEE_SHM_REGISTER) {
> > +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> > +
> > +               list_add(&shm->link, &priv->list_shm);
> > +       }
> > +       return shm;
> > +err:
> > +       free(shm);
> > +       if (flags & TEE_SHM_ALLOC)
> > +               free(p);
> > +       return NULL;
> > +}
> > +
> > +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
> > +                             u32 flags)
> > +{
> > +       u32 f = flags;
> > +
> > +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
> > +       return __tee_shm_add(dev, 0, NULL, size, f);
> > +}
> > +
> > +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> > +                                ulong size, u32 flags)
> > +{
> > +       u32 f = flags & ~TEE_SHM_ALLOC;
> > +
> > +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
> > +       return __tee_shm_add(dev, 0, addr, size, f);
> > +}
> > +
> > +void tee_shm_free(struct tee_shm *shm)
> > +{
> > +       if (!shm)
> > +               return;
> > +
> > +       if (shm->flags & TEE_SHM_SEC_REGISTER)
> > +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
> > +
> > +       if (shm->flags & TEE_SHM_REGISTER)
> > +               list_del(&shm->link);
> > +
> > +       if (shm->flags & TEE_SHM_ALLOC)
> > +               free(shm->addr);
> > +
> > +       free(shm);
> > +}
> > +
> > +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
> > +{
> > +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> > +       struct tee_shm *s;
> > +
> > +       list_for_each_entry(s, &priv->list_shm, link)
> > +               if (s == shm)
> > +                       return true;
> > +
> > +       return false;
> > +}
> > +
> > +struct udevice *tee_find_device(struct udevice *start,
> > +                               int (*match)(struct tee_version_data *vers,
> > +                                            const void *data),
> > +                               const void *data,
> > +                               struct tee_version_data *vers)
> 
> 'find' should find the device, not probe anything. Is that what this does?

It probes the device too, I'm adding that in the description of this
function. The probe is needed in order to read out the capabilities of
the TEE.

> 
> > +{
> > +       struct udevice *dev = start;
> > +       struct tee_version_data lv;
> > +       struct tee_version_data *v = vers ? vers : &lv;
> > +
> > +       if (!dev)
> > +               uclass_first_device(UCLASS_TEE, &dev);
> > +
> > +       for (; dev; uclass_next_device(&dev)) {
> > +               tee_get_ops(dev)->get_version(dev, v);
> > +               if (!match || match(v, data))
> > +                       return dev;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +static int tee_pre_probe(struct udevice *dev)
> > +{
> > +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> > +
> > +       INIT_LIST_HEAD(&priv->list_shm);
> 
> blank line before return
> 
> > +       return 0;
> > +}
> > +
> > +static int tee_pre_remove(struct udevice *dev)
> > +{
> > +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> > +       struct tee_shm *shm;
> > +
> > +       while (!list_empty(&priv->list_shm)) {
> > +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
> > +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
> > +                     __func__, (void *)shm, shm->size, shm->flags);
> > +               tee_shm_free(shm);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +UCLASS_DRIVER(tee) = {
> > +       .id = UCLASS_TEE,
> > +       .name = "tee",
> > +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
> > +       .pre_probe = tee_pre_probe,
> > +       .pre_remove = tee_pre_remove,
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index a39643ec5eef..955e0a915b87 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -81,6 +81,7 @@ enum uclass_id {
> >         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
> >         UCLASS_SYSCON,          /* System configuration device */
> >         UCLASS_SYSRESET,        /* System reset device */
> > +       UCLASS_TEE,             /* Trusted Execution Environment device */
> >         UCLASS_THERMAL,         /* Thermal sensor */
> >         UCLASS_TIMER,           /* Timer device */
> >         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
> > diff --git a/include/tee.h b/include/tee.h
> > new file mode 100644
> > index 000000000000..c2ac13e34128
> > --- /dev/null
> > +++ b/include/tee.h
> > @@ -0,0 +1,134 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2018 Linaro Limited
> > + */
> > +
> > +#ifndef __TEE_H
> > +#define __TEE_H
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +
> > +#define TEE_UUID_LEN           16
> > +
> > +#define TEE_GEN_CAP_GP          BIT(0) /* GlobalPlatform compliant TEE */
> > +#define TEE_GEN_CAP_REG_MEM     BIT(1) /* Supports registering shared memory */
> > +
> > +#define TEE_SHM_REGISTER       BIT(0)
> > +#define TEE_SHM_SEC_REGISTER   BIT(1)
> > +#define TEE_SHM_ALLOC          BIT(2)
> > +
> > +#define TEE_PARAM_ATTR_TYPE_NONE               0       /* parameter not used */
> > +#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT                1
> > +#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT       2
> > +#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT                3       /* input and output */
> > +#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT       5
> > +#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT      6
> > +#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT       7       /* input and output */
> > +#define TEE_PARAM_ATTR_TYPE_MASK               0xff
> > +#define TEE_PARAM_ATTR_META                    0x100
> > +#define TEE_PARAM_ATTR_MASK                    (TEE_PARAM_ATTR_TYPE_MASK | \
> > +                                                TEE_PARAM_ATTR_META)
> > +
> > +/*
> > + * Some Global Platform error codes which has a meaning if the
> > + * TEE_GEN_CAP_GP bit is returned by the driver.
> > + */
> > +#define TEE_SUCCESS                    0x00000000
> > +#define TEE_ERROR_GENERIC              0xffff0000
> > +#define TEE_ERROR_BAD_PARAMETERS       0xffff0006
> > +#define TEE_ERROR_ITEM_NOT_FOUND       0xffff0008
> > +#define TEE_ERROR_NOT_IMPLEMENTED      0xffff0009
> > +#define TEE_ERROR_NOT_SUPPORTED                0xffff000a
> > +#define TEE_ERROR_COMMUNICATION                0xffff000e
> > +#define TEE_ERROR_OUT_OF_MEMORY                0xffff000c
> > +#define TEE_ERROR_TARGET_DEAD          0xffff3024
> > +
> > +#define TEE_ORIGIN_COMMS               0x00000002
> > +
> > +struct tee_driver_ops;
> > +
> > +struct tee_shm {
> 
> comments for these structs please.
> 
> Also the whole uclass needs a README or more docs. Perhaps I have missed this?

I'm sorry, didn't supply that. I'll add it in the v2.

> 
> > +       struct udevice *dev;
> > +       struct list_head link;
> > +       void *addr;
> > +       ulong size;
> > +       u32 flags;
> > +};
> > +
> > +struct tee_param_memref {
> > +       ulong shm_offs;
> > +       ulong size;
> > +       struct tee_shm *shm;
> > +};
> > +
> > +struct tee_param_value {
> > +       u64 a;
> > +       u64 b;
> > +       u64 c;
> > +};
> > +
> > +struct tee_param {
> > +       u64 attr;
> > +       union {
> > +               struct tee_param_memref memref;
> > +               struct tee_param_value value;
> > +       } u;
> > +};
> > +
> > +struct tee_open_session_arg {
> > +       u8 uuid[TEE_UUID_LEN];
> > +       u8 clnt_uuid[TEE_UUID_LEN];
> > +       u32 clnt_login;
> > +       u32 session;
> > +       u32 ret;
> > +       u32 ret_origin;
> > +};
> > +
> > +struct tee_invoke_arg {
> > +       u32 func;
> > +       u32 session;
> > +       u32 ret;
> > +       u32 ret_origin;
> > +};
> > +
> > +struct tee_version_data {
> > +       u32 gen_caps;
> > +};
> > +
> > +struct tee_driver_ops {
> > +       void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
> > +       int (*open_session)(struct udevice *dev,
> > +                           struct tee_open_session_arg *arg, ulong num_param,
> > +                           struct tee_param *param);
> > +       int (*close_session)(struct udevice *dev, u32 session);
> > +       int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
> > +                          ulong num_param, struct tee_param *param);
> > +       int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
> > +       int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
> 
> Each of these needs a full function comment. Without that I cannot
> really comment on the API. Same for functions below.

OK

> 
> > +};
> > +
> > +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> > +                             ulong size, u32 flags);
> > +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);
> > +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> > +                                ulong length, u32 flags);
> > +void tee_shm_free(struct tee_shm *shm);
> > +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
> > +
> > +struct udevice *tee_find_device(struct udevice *start,
> > +                               int (*match)(struct tee_version_data *vers,
> > +                                            const void *data),
> > +                               const void *data,
> > +                               struct tee_version_data *vers);
> > +
> > +void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
> > +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> > +                    ulong num_param, struct tee_param *param);
> > +
> > +int tee_close_session(struct udevice *dev, u32 session);
> > +
> > +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> > +                   ulong num_param, struct tee_param *param);
> > +
> > +#endif /*__TEE_H*/
> > --
> > 2.17.1
> >

Thanks for the review. I'll address all the comments in the comming v2
patch set.

--
Jens
Simon Glass Aug. 23, 2018, 10:45 a.m. | #4
Hi Jens,

On 13 August 2018 at 09:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> Adds a uclass to interface with a TEE (Trusted Execution Environment).
>
> A TEE driver is a driver that interfaces with a trusted OS running in
> some secure environment, for example, TrustZone on ARM cpus, or a
> separate secure co-processor etc.
>
> The TEE subsystem can serve a TEE driver for a Global Platform compliant
> TEE, but it's not limited to only Global Platform TEEs.
>
> The over all design is based on the TEE subsystem in the Linux kernel,
> tailored for U-boot.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  MAINTAINERS              |   6 ++
>  drivers/Kconfig          |   2 +
>  drivers/Makefile         |   1 +
>  drivers/tee/Kconfig      |   8 ++
>  drivers/tee/Makefile     |   3 +
>  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |   1 +
>  include/tee.h            | 134 +++++++++++++++++++++++++++++
>  8 files changed, 335 insertions(+)
>  create mode 100644 drivers/tee/Kconfig
>  create mode 100644 drivers/tee/Makefile
>  create mode 100644 drivers/tee/tee-uclass.c
>  create mode 100644 include/tee.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b61ac05882..7458c606ee92 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -571,6 +571,12 @@ TQ GROUP
>  S:     Orphaned (Since 2016-02)
>  T:     git git://git.denx.de/u-boot-tq-group.git
>
> +TEE
> +M:     Jens Wiklander <jens.wiklander@linaro.org>
> +S:     Maintained
> +F:     drivers/tee/
> +F:     include/tee.h
> +
>  UBI
>  M:     Kyungmin Park <kmpark@infradead.org>
>  M:     Heiko Schocher <hs@denx.de>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index c72abf893297..f3249ab1d143 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
>
>  source "drivers/sysreset/Kconfig"
>
> +source "drivers/tee/Kconfig"
> +
>  source "drivers/thermal/Kconfig"
>
>  source "drivers/timer/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index d53208540ea6..0fcae36f50f7 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -103,6 +103,7 @@ obj-y += smem/
>  obj-y += soc/
>  obj-$(CONFIG_REMOTEPROC) += remoteproc/
>  obj-y += thermal/
> +obj-$(CONFIG_TEE) += tee/
>
>  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
>  endif
> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> new file mode 100644
> index 000000000000..817ab331b0f8
> --- /dev/null
> +++ b/drivers/tee/Kconfig
> @@ -0,0 +1,8 @@
> +# Generic Trusted Execution Environment Configuration
> +config TEE
> +       bool "Trusted Execution Environment support"
> +       depends on ARM && (ARM64 || CPU_V7A)
> +       select ARM_SMCCC
> +       help
> +         This implements a generic interface towards a Trusted Execution
> +         Environment (TEE).
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> new file mode 100644
> index 000000000000..b6d8e16e6211
> --- /dev/null
> +++ b/drivers/tee/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +obj-y += tee-uclass.o
> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
> new file mode 100644
> index 000000000000..f0243a0f2e4e
> --- /dev/null
> +++ b/drivers/tee/tee-uclass.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <tee.h>
> +
> +struct tee_uclass_priv {
> +       struct list_head list_shm;
> +};
> +
> +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
> +{
> +       return device_get_ops(dev);
> +}
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
> +{
> +       tee_get_ops(dev)->get_version(dev, vers);
> +}
> +
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
> +}
> +
> +int tee_close_session(struct udevice *dev, u32 session)
> +{
> +       return tee_get_ops(dev)->close_session(dev, session);
> +}
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param)
> +{
> +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
> +}
> +
> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags)
> +{
> +       struct tee_shm *shm;
> +       void *p = addr;
> +
> +       if (flags & TEE_SHM_ALLOC) {
> +               if (align)
> +                       p = memalign(align, size);
> +               else
> +                       p = malloc(size);
> +       }
> +       if (!p)
> +               return NULL;
> +
> +       shm = calloc(1, sizeof(*shm));
> +       if (!shm)
> +               goto err;
> +
> +       shm->dev = dev;
> +       shm->addr = p;
> +       shm->size = size;
> +       shm->flags = flags;
> +
> +       if ((flags & TEE_SHM_SEC_REGISTER) &&
> +           tee_get_ops(dev)->shm_register(dev, shm))
> +               goto err;

It seems like this can return errors other than -ENOMEM. How about
changing this function to return an int?

> +
> +       if (flags & TEE_SHM_REGISTER) {
> +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +               list_add(&shm->link, &priv->list_shm);
> +       }
> +       return shm;
> +err:
> +       free(shm);
> +       if (flags & TEE_SHM_ALLOC)
> +               free(p);
> +       return NULL;
> +}
> +
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
> +                             u32 flags)
> +{
> +       u32 f = flags;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;

blank line before return

> +       return __tee_shm_add(dev, 0, NULL, size, f);
> +}
> +
> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong size, u32 flags)
> +{
> +       u32 f = flags & ~TEE_SHM_ALLOC;
> +
> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
> +       return __tee_shm_add(dev, 0, addr, size, f);
> +}
> +
> +void tee_shm_free(struct tee_shm *shm)
> +{
> +       if (!shm)
> +               return;
> +
> +       if (shm->flags & TEE_SHM_SEC_REGISTER)
> +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
> +
> +       if (shm->flags & TEE_SHM_REGISTER)
> +               list_del(&shm->link);
> +
> +       if (shm->flags & TEE_SHM_ALLOC)
> +               free(shm->addr);
> +
> +       free(shm);
> +}
> +
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *s;
> +
> +       list_for_each_entry(s, &priv->list_shm, link)
> +               if (s == shm)
> +                       return true;
> +
> +       return false;
> +}
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers)
> +{
> +       struct udevice *dev = start;
> +       struct tee_version_data lv;
> +       struct tee_version_data *v = vers ? vers : &lv;
> +
> +       if (!dev)
> +               uclass_first_device(UCLASS_TEE, &dev);
> +
> +       for (; dev; uclass_next_device(&dev)) {
> +               tee_get_ops(dev)->get_version(dev, v);
> +               if (!match || match(v, data))
> +                       return dev;

This will probe each device as it looks through them. Is that what you want?

> +       }
> +
> +       return NULL;
> +}
> +
> +static int tee_pre_probe(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +
> +       INIT_LIST_HEAD(&priv->list_shm);
> +       return 0;
> +}
> +
> +static int tee_pre_remove(struct udevice *dev)
> +{
> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
> +       struct tee_shm *shm;
> +
> +       while (!list_empty(&priv->list_shm)) {
> +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
> +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
> +                     __func__, (void *)shm, shm->size, shm->flags);
> +               tee_shm_free(shm);
> +       }
> +
> +       return 0;
> +}
> +
> +UCLASS_DRIVER(tee) = {
> +       .id = UCLASS_TEE,
> +       .name = "tee",
> +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
> +       .pre_probe = tee_pre_probe,
> +       .pre_remove = tee_pre_remove,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index a39643ec5eef..955e0a915b87 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -81,6 +81,7 @@ enum uclass_id {
>         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
>         UCLASS_SYSCON,          /* System configuration device */
>         UCLASS_SYSRESET,        /* System reset device */
> +       UCLASS_TEE,             /* Trusted Execution Environment device */

We need a bit more information about what this is for. It could go in
a README or in the uclass header file.

>         UCLASS_THERMAL,         /* Thermal sensor */
>         UCLASS_TIMER,           /* Timer device */
>         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
> diff --git a/include/tee.h b/include/tee.h
> new file mode 100644
> index 000000000000..c2ac13e34128
> --- /dev/null
> +++ b/include/tee.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2018 Linaro Limited
> + */
> +
> +#ifndef __TEE_H
> +#define __TEE_H
> +
> +#include <common.h>
> +#include <dm.h>

These should be included by .c files.

> +
> +#define TEE_UUID_LEN           16
> +
> +#define TEE_GEN_CAP_GP          BIT(0) /* GlobalPlatform compliant TEE */
> +#define TEE_GEN_CAP_REG_MEM     BIT(1) /* Supports registering shared memory */
> +
> +#define TEE_SHM_REGISTER       BIT(0)
> +#define TEE_SHM_SEC_REGISTER   BIT(1)
> +#define TEE_SHM_ALLOC          BIT(2)
> +
> +#define TEE_PARAM_ATTR_TYPE_NONE               0       /* parameter not used */
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT                1
> +#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT       2
> +#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT                3       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT       5
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT      6
> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT       7       /* input and output */
> +#define TEE_PARAM_ATTR_TYPE_MASK               0xff
> +#define TEE_PARAM_ATTR_META                    0x100
> +#define TEE_PARAM_ATTR_MASK                    (TEE_PARAM_ATTR_TYPE_MASK | \
> +                                                TEE_PARAM_ATTR_META)
> +
> +/*
> + * Some Global Platform error codes which has a meaning if the
> + * TEE_GEN_CAP_GP bit is returned by the driver.
> + */
> +#define TEE_SUCCESS                    0x00000000
> +#define TEE_ERROR_GENERIC              0xffff0000
> +#define TEE_ERROR_BAD_PARAMETERS       0xffff0006
> +#define TEE_ERROR_ITEM_NOT_FOUND       0xffff0008
> +#define TEE_ERROR_NOT_IMPLEMENTED      0xffff0009
> +#define TEE_ERROR_NOT_SUPPORTED                0xffff000a
> +#define TEE_ERROR_COMMUNICATION                0xffff000e
> +#define TEE_ERROR_OUT_OF_MEMORY                0xffff000c
> +#define TEE_ERROR_TARGET_DEAD          0xffff3024
> +
> +#define TEE_ORIGIN_COMMS               0x00000002
> +
> +struct tee_driver_ops;
> +
> +struct tee_shm {

Comment structure, and below

> +       struct udevice *dev;
> +       struct list_head link;
> +       void *addr;
> +       ulong size;
> +       u32 flags;
> +};
> +
> +struct tee_param_memref {
> +       ulong shm_offs;
> +       ulong size;
> +       struct tee_shm *shm;
> +};
> +
> +struct tee_param_value {
> +       u64 a;
> +       u64 b;
> +       u64 c;
> +};
> +
> +struct tee_param {
> +       u64 attr;
> +       union {
> +               struct tee_param_memref memref;
> +               struct tee_param_value value;
> +       } u;
> +};
> +
> +struct tee_open_session_arg {
> +       u8 uuid[TEE_UUID_LEN];
> +       u8 clnt_uuid[TEE_UUID_LEN];
> +       u32 clnt_login;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_invoke_arg {
> +       u32 func;
> +       u32 session;
> +       u32 ret;
> +       u32 ret_origin;
> +};
> +
> +struct tee_version_data {
> +       u32 gen_caps;
> +};
> +
> +struct tee_driver_ops {

These all need full comments.

> +       void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
> +       int (*open_session)(struct udevice *dev,
> +                           struct tee_open_session_arg *arg, ulong num_param,
> +                           struct tee_param *param);
> +       int (*close_session)(struct udevice *dev, u32 session);
> +       int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
> +                          ulong num_param, struct tee_param *param);
> +       int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
> +       int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
> +};
> +

And these

> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
> +                             ulong size, u32 flags);
> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);

What are flags? All of this needs documentation please.

> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
> +                                ulong length, u32 flags);
> +void tee_shm_free(struct tee_shm *shm);
> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
> +
> +struct udevice *tee_find_device(struct udevice *start,
> +                               int (*match)(struct tee_version_data *vers,
> +                                            const void *data),
> +                               const void *data,
> +                               struct tee_version_data *vers);
> +
> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
> +                    ulong num_param, struct tee_param *param);
> +
> +int tee_close_session(struct udevice *dev, u32 session);
> +
> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
> +                   ulong num_param, struct tee_param *param);
> +
> +#endif /*__TEE_H*/
> --
> 2.17.1
>

Regards,
Simon
Jens Wiklander Aug. 23, 2018, 11:11 a.m. | #5
Hi Simon,

On Thu, Aug 23, 2018 at 12:45 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jens,
>
> On 13 August 2018 at 09:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>> Adds a uclass to interface with a TEE (Trusted Execution Environment).
>>
>> A TEE driver is a driver that interfaces with a trusted OS running in
>> some secure environment, for example, TrustZone on ARM cpus, or a
>> separate secure co-processor etc.
>>
>> The TEE subsystem can serve a TEE driver for a Global Platform compliant
>> TEE, but it's not limited to only Global Platform TEEs.
>>
>> The over all design is based on the TEE subsystem in the Linux kernel,
>> tailored for U-boot.
>>
>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>> ---
>>  MAINTAINERS              |   6 ++
>>  drivers/Kconfig          |   2 +
>>  drivers/Makefile         |   1 +
>>  drivers/tee/Kconfig      |   8 ++
>>  drivers/tee/Makefile     |   3 +
>>  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
>>  include/dm/uclass-id.h   |   1 +
>>  include/tee.h            | 134 +++++++++++++++++++++++++++++
>>  8 files changed, 335 insertions(+)
>>  create mode 100644 drivers/tee/Kconfig
>>  create mode 100644 drivers/tee/Makefile
>>  create mode 100644 drivers/tee/tee-uclass.c
>>  create mode 100644 include/tee.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 58b61ac05882..7458c606ee92 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -571,6 +571,12 @@ TQ GROUP
>>  S:     Orphaned (Since 2016-02)
>>  T:     git git://git.denx.de/u-boot-tq-group.git
>>
>> +TEE
>> +M:     Jens Wiklander <jens.wiklander@linaro.org>
>> +S:     Maintained
>> +F:     drivers/tee/
>> +F:     include/tee.h
>> +
>>  UBI
>>  M:     Kyungmin Park <kmpark@infradead.org>
>>  M:     Heiko Schocher <hs@denx.de>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index c72abf893297..f3249ab1d143 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
>>
>>  source "drivers/sysreset/Kconfig"
>>
>> +source "drivers/tee/Kconfig"
>> +
>>  source "drivers/thermal/Kconfig"
>>
>>  source "drivers/timer/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index d53208540ea6..0fcae36f50f7 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -103,6 +103,7 @@ obj-y += smem/
>>  obj-y += soc/
>>  obj-$(CONFIG_REMOTEPROC) += remoteproc/
>>  obj-y += thermal/
>> +obj-$(CONFIG_TEE) += tee/
>>
>>  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
>>  endif
>> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
>> new file mode 100644
>> index 000000000000..817ab331b0f8
>> --- /dev/null
>> +++ b/drivers/tee/Kconfig
>> @@ -0,0 +1,8 @@
>> +# Generic Trusted Execution Environment Configuration
>> +config TEE
>> +       bool "Trusted Execution Environment support"
>> +       depends on ARM && (ARM64 || CPU_V7A)
>> +       select ARM_SMCCC
>> +       help
>> +         This implements a generic interface towards a Trusted Execution
>> +         Environment (TEE).
>> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
>> new file mode 100644
>> index 000000000000..b6d8e16e6211
>> --- /dev/null
>> +++ b/drivers/tee/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +
>> +obj-y += tee-uclass.o
>> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
>> new file mode 100644
>> index 000000000000..f0243a0f2e4e
>> --- /dev/null
>> +++ b/drivers/tee/tee-uclass.c
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2018 Linaro Limited
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <tee.h>
>> +
>> +struct tee_uclass_priv {
>> +       struct list_head list_shm;
>> +};
>> +
>> +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
>> +{
>> +       return device_get_ops(dev);
>> +}
>> +
>> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
>> +{
>> +       tee_get_ops(dev)->get_version(dev, vers);
>> +}
>> +
>> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
>> +                    ulong num_param, struct tee_param *param)
>> +{
>> +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
>> +}
>> +
>> +int tee_close_session(struct udevice *dev, u32 session)
>> +{
>> +       return tee_get_ops(dev)->close_session(dev, session);
>> +}
>> +
>> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
>> +                   ulong num_param, struct tee_param *param)
>> +{
>> +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
>> +}
>> +
>> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
>> +                             ulong size, u32 flags)
>> +{
>> +       struct tee_shm *shm;
>> +       void *p = addr;
>> +
>> +       if (flags & TEE_SHM_ALLOC) {
>> +               if (align)
>> +                       p = memalign(align, size);
>> +               else
>> +                       p = malloc(size);
>> +       }
>> +       if (!p)
>> +               return NULL;
>> +
>> +       shm = calloc(1, sizeof(*shm));
>> +       if (!shm)
>> +               goto err;
>> +
>> +       shm->dev = dev;
>> +       shm->addr = p;
>> +       shm->size = size;
>> +       shm->flags = flags;
>> +
>> +       if ((flags & TEE_SHM_SEC_REGISTER) &&
>> +           tee_get_ops(dev)->shm_register(dev, shm))
>> +               goto err;
>
> It seems like this can return errors other than -ENOMEM. How about
> changing this function to return an int?

OK, I'll fix.

>
>> +
>> +       if (flags & TEE_SHM_REGISTER) {
>> +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>> +
>> +               list_add(&shm->link, &priv->list_shm);
>> +       }
>> +       return shm;
>> +err:
>> +       free(shm);
>> +       if (flags & TEE_SHM_ALLOC)
>> +               free(p);
>> +       return NULL;
>> +}
>> +
>> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
>> +                             u32 flags)
>> +{
>> +       u32 f = flags;
>> +
>> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
>
> blank line before return

OK

>
>> +       return __tee_shm_add(dev, 0, NULL, size, f);
>> +}
>> +
>> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
>> +                                ulong size, u32 flags)
>> +{
>> +       u32 f = flags & ~TEE_SHM_ALLOC;
>> +
>> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
>> +       return __tee_shm_add(dev, 0, addr, size, f);
>> +}
>> +
>> +void tee_shm_free(struct tee_shm *shm)
>> +{
>> +       if (!shm)
>> +               return;
>> +
>> +       if (shm->flags & TEE_SHM_SEC_REGISTER)
>> +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
>> +
>> +       if (shm->flags & TEE_SHM_REGISTER)
>> +               list_del(&shm->link);
>> +
>> +       if (shm->flags & TEE_SHM_ALLOC)
>> +               free(shm->addr);
>> +
>> +       free(shm);
>> +}
>> +
>> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
>> +{
>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>> +       struct tee_shm *s;
>> +
>> +       list_for_each_entry(s, &priv->list_shm, link)
>> +               if (s == shm)
>> +                       return true;
>> +
>> +       return false;
>> +}
>> +
>> +struct udevice *tee_find_device(struct udevice *start,
>> +                               int (*match)(struct tee_version_data *vers,
>> +                                            const void *data),
>> +                               const void *data,
>> +                               struct tee_version_data *vers)
>> +{
>> +       struct udevice *dev = start;
>> +       struct tee_version_data lv;
>> +       struct tee_version_data *v = vers ? vers : &lv;
>> +
>> +       if (!dev)
>> +               uclass_first_device(UCLASS_TEE, &dev);
>> +
>> +       for (; dev; uclass_next_device(&dev)) {
>> +               tee_get_ops(dev)->get_version(dev, v);
>> +               if (!match || match(v, data))
>> +                       return dev;
>
> This will probe each device as it looks through them. Is that what you want?

Yes, in practice there will be only one or perhaps in some special
cases two. The capabilities reported by secure world are needed tee.

>
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static int tee_pre_probe(struct udevice *dev)
>> +{
>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>> +
>> +       INIT_LIST_HEAD(&priv->list_shm);
>> +       return 0;
>> +}
>> +
>> +static int tee_pre_remove(struct udevice *dev)
>> +{
>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>> +       struct tee_shm *shm;
>> +
>> +       while (!list_empty(&priv->list_shm)) {
>> +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
>> +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
>> +                     __func__, (void *)shm, shm->size, shm->flags);
>> +               tee_shm_free(shm);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +UCLASS_DRIVER(tee) = {
>> +       .id = UCLASS_TEE,
>> +       .name = "tee",
>> +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
>> +       .pre_probe = tee_pre_probe,
>> +       .pre_remove = tee_pre_remove,
>> +};
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index a39643ec5eef..955e0a915b87 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -81,6 +81,7 @@ enum uclass_id {
>>         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
>>         UCLASS_SYSCON,          /* System configuration device */
>>         UCLASS_SYSRESET,        /* System reset device */
>> +       UCLASS_TEE,             /* Trusted Execution Environment device */
>
> We need a bit more information about what this is for. It could go in
> a README or in the uclass header file.
>
>>         UCLASS_THERMAL,         /* Thermal sensor */
>>         UCLASS_TIMER,           /* Timer device */
>>         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
>> diff --git a/include/tee.h b/include/tee.h
>> new file mode 100644
>> index 000000000000..c2ac13e34128
>> --- /dev/null
>> +++ b/include/tee.h
>> @@ -0,0 +1,134 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (c) 2018 Linaro Limited
>> + */
>> +
>> +#ifndef __TEE_H
>> +#define __TEE_H
>> +
>> +#include <common.h>
>> +#include <dm.h>
>
> These should be included by .c files.

Yes, but what those provides is also needed by this .h file.
Aren't all .h files supposed to include what they depend on?

>
>> +
>> +#define TEE_UUID_LEN           16
>> +
>> +#define TEE_GEN_CAP_GP          BIT(0) /* GlobalPlatform compliant TEE */
>> +#define TEE_GEN_CAP_REG_MEM     BIT(1) /* Supports registering shared memory */
>> +
>> +#define TEE_SHM_REGISTER       BIT(0)
>> +#define TEE_SHM_SEC_REGISTER   BIT(1)
>> +#define TEE_SHM_ALLOC          BIT(2)
>> +
>> +#define TEE_PARAM_ATTR_TYPE_NONE               0       /* parameter not used */
>> +#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT                1
>> +#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT       2
>> +#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT                3       /* input and output */
>> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT       5
>> +#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT      6
>> +#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT       7       /* input and output */
>> +#define TEE_PARAM_ATTR_TYPE_MASK               0xff
>> +#define TEE_PARAM_ATTR_META                    0x100
>> +#define TEE_PARAM_ATTR_MASK                    (TEE_PARAM_ATTR_TYPE_MASK | \
>> +                                                TEE_PARAM_ATTR_META)
>> +
>> +/*
>> + * Some Global Platform error codes which has a meaning if the
>> + * TEE_GEN_CAP_GP bit is returned by the driver.
>> + */
>> +#define TEE_SUCCESS                    0x00000000
>> +#define TEE_ERROR_GENERIC              0xffff0000
>> +#define TEE_ERROR_BAD_PARAMETERS       0xffff0006
>> +#define TEE_ERROR_ITEM_NOT_FOUND       0xffff0008
>> +#define TEE_ERROR_NOT_IMPLEMENTED      0xffff0009
>> +#define TEE_ERROR_NOT_SUPPORTED                0xffff000a
>> +#define TEE_ERROR_COMMUNICATION                0xffff000e
>> +#define TEE_ERROR_OUT_OF_MEMORY                0xffff000c
>> +#define TEE_ERROR_TARGET_DEAD          0xffff3024
>> +
>> +#define TEE_ORIGIN_COMMS               0x00000002
>> +
>> +struct tee_driver_ops;
>> +
>> +struct tee_shm {
>
> Comment structure, and below

I've added that in the V2 that I posted as you replied here.

>
>> +       struct udevice *dev;
>> +       struct list_head link;
>> +       void *addr;
>> +       ulong size;
>> +       u32 flags;
>> +};
>> +
>> +struct tee_param_memref {
>> +       ulong shm_offs;
>> +       ulong size;
>> +       struct tee_shm *shm;
>> +};
>> +
>> +struct tee_param_value {
>> +       u64 a;
>> +       u64 b;
>> +       u64 c;
>> +};
>> +
>> +struct tee_param {
>> +       u64 attr;
>> +       union {
>> +               struct tee_param_memref memref;
>> +               struct tee_param_value value;
>> +       } u;
>> +};
>> +
>> +struct tee_open_session_arg {
>> +       u8 uuid[TEE_UUID_LEN];
>> +       u8 clnt_uuid[TEE_UUID_LEN];
>> +       u32 clnt_login;
>> +       u32 session;
>> +       u32 ret;
>> +       u32 ret_origin;
>> +};
>> +
>> +struct tee_invoke_arg {
>> +       u32 func;
>> +       u32 session;
>> +       u32 ret;
>> +       u32 ret_origin;
>> +};
>> +
>> +struct tee_version_data {
>> +       u32 gen_caps;
>> +};
>> +
>> +struct tee_driver_ops {
>
> These all need full comments.
>
>> +       void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
>> +       int (*open_session)(struct udevice *dev,
>> +                           struct tee_open_session_arg *arg, ulong num_param,
>> +                           struct tee_param *param);
>> +       int (*close_session)(struct udevice *dev, u32 session);
>> +       int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
>> +                          ulong num_param, struct tee_param *param);
>> +       int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
>> +       int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
>> +};
>> +
>
> And these
>
>> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
>> +                             ulong size, u32 flags);
>> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);
>
> What are flags? All of this needs documentation please.
>
>> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
>> +                                ulong length, u32 flags);
>> +void tee_shm_free(struct tee_shm *shm);
>> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
>> +
>> +struct udevice *tee_find_device(struct udevice *start,
>> +                               int (*match)(struct tee_version_data *vers,
>> +                                            const void *data),
>> +                               const void *data,
>> +                               struct tee_version_data *vers);
>> +
>> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
>> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
>> +                    ulong num_param, struct tee_param *param);
>> +
>> +int tee_close_session(struct udevice *dev, u32 session);
>> +
>> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
>> +                   ulong num_param, struct tee_param *param);
>> +
>> +#endif /*__TEE_H*/
>> --
>> 2.17.1
>>

Thanks for the review. I posted V2 of this patch set at the same time
as you replied, I'll address what's not already done in V2 in the
coming V3.

--
Jens
Simon Glass Aug. 23, 2018, 4:31 p.m. | #6
Hi Jens,

On 23 August 2018 at 05:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> Hi Simon,
>
> On Thu, Aug 23, 2018 at 12:45 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jens,
>>
>> On 13 August 2018 at 09:53, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>> Adds a uclass to interface with a TEE (Trusted Execution Environment).
>>>
>>> A TEE driver is a driver that interfaces with a trusted OS running in
>>> some secure environment, for example, TrustZone on ARM cpus, or a
>>> separate secure co-processor etc.
>>>
>>> The TEE subsystem can serve a TEE driver for a Global Platform compliant
>>> TEE, but it's not limited to only Global Platform TEEs.
>>>
>>> The over all design is based on the TEE subsystem in the Linux kernel,
>>> tailored for U-boot.
>>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>>  MAINTAINERS              |   6 ++
>>>  drivers/Kconfig          |   2 +
>>>  drivers/Makefile         |   1 +
>>>  drivers/tee/Kconfig      |   8 ++
>>>  drivers/tee/Makefile     |   3 +
>>>  drivers/tee/tee-uclass.c | 180 +++++++++++++++++++++++++++++++++++++++
>>>  include/dm/uclass-id.h   |   1 +
>>>  include/tee.h            | 134 +++++++++++++++++++++++++++++
>>>  8 files changed, 335 insertions(+)
>>>  create mode 100644 drivers/tee/Kconfig
>>>  create mode 100644 drivers/tee/Makefile
>>>  create mode 100644 drivers/tee/tee-uclass.c
>>>  create mode 100644 include/tee.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 58b61ac05882..7458c606ee92 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -571,6 +571,12 @@ TQ GROUP
>>>  S:     Orphaned (Since 2016-02)
>>>  T:     git git://git.denx.de/u-boot-tq-group.git
>>>
>>> +TEE
>>> +M:     Jens Wiklander <jens.wiklander@linaro.org>
>>> +S:     Maintained
>>> +F:     drivers/tee/
>>> +F:     include/tee.h
>>> +
>>>  UBI
>>>  M:     Kyungmin Park <kmpark@infradead.org>
>>>  M:     Heiko Schocher <hs@denx.de>
>>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>>> index c72abf893297..f3249ab1d143 100644
>>> --- a/drivers/Kconfig
>>> +++ b/drivers/Kconfig
>>> @@ -94,6 +94,8 @@ source "drivers/spmi/Kconfig"
>>>
>>>  source "drivers/sysreset/Kconfig"
>>>
>>> +source "drivers/tee/Kconfig"
>>> +
>>>  source "drivers/thermal/Kconfig"
>>>
>>>  source "drivers/timer/Kconfig"
>>> diff --git a/drivers/Makefile b/drivers/Makefile
>>> index d53208540ea6..0fcae36f50f7 100644
>>> --- a/drivers/Makefile
>>> +++ b/drivers/Makefile
>>> @@ -103,6 +103,7 @@ obj-y += smem/
>>>  obj-y += soc/
>>>  obj-$(CONFIG_REMOTEPROC) += remoteproc/
>>>  obj-y += thermal/
>>> +obj-$(CONFIG_TEE) += tee/
>>>
>>>  obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
>>>  endif
>>> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
>>> new file mode 100644
>>> index 000000000000..817ab331b0f8
>>> --- /dev/null
>>> +++ b/drivers/tee/Kconfig
>>> @@ -0,0 +1,8 @@
>>> +# Generic Trusted Execution Environment Configuration
>>> +config TEE
>>> +       bool "Trusted Execution Environment support"
>>> +       depends on ARM && (ARM64 || CPU_V7A)
>>> +       select ARM_SMCCC
>>> +       help
>>> +         This implements a generic interface towards a Trusted Execution
>>> +         Environment (TEE).
>>> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
>>> new file mode 100644
>>> index 000000000000..b6d8e16e6211
>>> --- /dev/null
>>> +++ b/drivers/tee/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0+
>>> +
>>> +obj-y += tee-uclass.o
>>> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
>>> new file mode 100644
>>> index 000000000000..f0243a0f2e4e
>>> --- /dev/null
>>> +++ b/drivers/tee/tee-uclass.c
>>> @@ -0,0 +1,180 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2018 Linaro Limited
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <dm.h>
>>> +#include <tee.h>
>>> +
>>> +struct tee_uclass_priv {
>>> +       struct list_head list_shm;
>>> +};
>>> +
>>> +static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
>>> +{
>>> +       return device_get_ops(dev);
>>> +}
>>> +
>>> +void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
>>> +{
>>> +       tee_get_ops(dev)->get_version(dev, vers);
>>> +}
>>> +
>>> +int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
>>> +                    ulong num_param, struct tee_param *param)
>>> +{
>>> +       return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
>>> +}
>>> +
>>> +int tee_close_session(struct udevice *dev, u32 session)
>>> +{
>>> +       return tee_get_ops(dev)->close_session(dev, session);
>>> +}
>>> +
>>> +int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
>>> +                   ulong num_param, struct tee_param *param)
>>> +{
>>> +       return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
>>> +}
>>> +
>>> +struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
>>> +                             ulong size, u32 flags)
>>> +{
>>> +       struct tee_shm *shm;
>>> +       void *p = addr;
>>> +
>>> +       if (flags & TEE_SHM_ALLOC) {
>>> +               if (align)
>>> +                       p = memalign(align, size);
>>> +               else
>>> +                       p = malloc(size);
>>> +       }
>>> +       if (!p)
>>> +               return NULL;
>>> +
>>> +       shm = calloc(1, sizeof(*shm));
>>> +       if (!shm)
>>> +               goto err;
>>> +
>>> +       shm->dev = dev;
>>> +       shm->addr = p;
>>> +       shm->size = size;
>>> +       shm->flags = flags;
>>> +
>>> +       if ((flags & TEE_SHM_SEC_REGISTER) &&
>>> +           tee_get_ops(dev)->shm_register(dev, shm))
>>> +               goto err;
>>
>> It seems like this can return errors other than -ENOMEM. How about
>> changing this function to return an int?
>
> OK, I'll fix.
>
>>
>>> +
>>> +       if (flags & TEE_SHM_REGISTER) {
>>> +               struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>>> +
>>> +               list_add(&shm->link, &priv->list_shm);
>>> +       }
>>> +       return shm;
>>> +err:
>>> +       free(shm);
>>> +       if (flags & TEE_SHM_ALLOC)
>>> +               free(p);
>>> +       return NULL;
>>> +}
>>> +
>>> +struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
>>> +                             u32 flags)
>>> +{
>>> +       u32 f = flags;
>>> +
>>> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
>>
>> blank line before return
>
> OK
>
>>
>>> +       return __tee_shm_add(dev, 0, NULL, size, f);
>>> +}
>>> +
>>> +struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
>>> +                                ulong size, u32 flags)
>>> +{
>>> +       u32 f = flags & ~TEE_SHM_ALLOC;
>>> +
>>> +       f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
>>> +       return __tee_shm_add(dev, 0, addr, size, f);
>>> +}
>>> +
>>> +void tee_shm_free(struct tee_shm *shm)
>>> +{
>>> +       if (!shm)
>>> +               return;
>>> +
>>> +       if (shm->flags & TEE_SHM_SEC_REGISTER)
>>> +               tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
>>> +
>>> +       if (shm->flags & TEE_SHM_REGISTER)
>>> +               list_del(&shm->link);
>>> +
>>> +       if (shm->flags & TEE_SHM_ALLOC)
>>> +               free(shm->addr);
>>> +
>>> +       free(shm);
>>> +}
>>> +
>>> +bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
>>> +{
>>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>>> +       struct tee_shm *s;
>>> +
>>> +       list_for_each_entry(s, &priv->list_shm, link)
>>> +               if (s == shm)
>>> +                       return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>> +struct udevice *tee_find_device(struct udevice *start,
>>> +                               int (*match)(struct tee_version_data *vers,
>>> +                                            const void *data),
>>> +                               const void *data,
>>> +                               struct tee_version_data *vers)
>>> +{
>>> +       struct udevice *dev = start;
>>> +       struct tee_version_data lv;
>>> +       struct tee_version_data *v = vers ? vers : &lv;
>>> +
>>> +       if (!dev)
>>> +               uclass_first_device(UCLASS_TEE, &dev);
>>> +
>>> +       for (; dev; uclass_next_device(&dev)) {
>>> +               tee_get_ops(dev)->get_version(dev, v);
>>> +               if (!match || match(v, data))
>>> +                       return dev;
>>
>> This will probe each device as it looks through them. Is that what you want?
>
> Yes, in practice there will be only one or perhaps in some special
> cases two. The capabilities reported by secure world are needed tee.
>
>>
>>> +       }
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>> +static int tee_pre_probe(struct udevice *dev)
>>> +{
>>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>>> +
>>> +       INIT_LIST_HEAD(&priv->list_shm);
>>> +       return 0;
>>> +}
>>> +
>>> +static int tee_pre_remove(struct udevice *dev)
>>> +{
>>> +       struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
>>> +       struct tee_shm *shm;
>>> +
>>> +       while (!list_empty(&priv->list_shm)) {
>>> +               shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
>>> +               debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
>>> +                     __func__, (void *)shm, shm->size, shm->flags);
>>> +               tee_shm_free(shm);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +UCLASS_DRIVER(tee) = {
>>> +       .id = UCLASS_TEE,
>>> +       .name = "tee",
>>> +       .per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
>>> +       .pre_probe = tee_pre_probe,
>>> +       .pre_remove = tee_pre_remove,
>>> +};
>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>> index a39643ec5eef..955e0a915b87 100644
>>> --- a/include/dm/uclass-id.h
>>> +++ b/include/dm/uclass-id.h
>>> @@ -81,6 +81,7 @@ enum uclass_id {
>>>         UCLASS_SPI_GENERIC,     /* Generic SPI flash target */
>>>         UCLASS_SYSCON,          /* System configuration device */
>>>         UCLASS_SYSRESET,        /* System reset device */
>>> +       UCLASS_TEE,             /* Trusted Execution Environment device */
>>
>> We need a bit more information about what this is for. It could go in
>> a README or in the uclass header file.
>>
>>>         UCLASS_THERMAL,         /* Thermal sensor */
>>>         UCLASS_TIMER,           /* Timer device */
>>>         UCLASS_TPM,             /* Trusted Platform Module TIS interface */
>>> diff --git a/include/tee.h b/include/tee.h
>>> new file mode 100644
>>> index 000000000000..c2ac13e34128
>>> --- /dev/null
>>> +++ b/include/tee.h
>>> @@ -0,0 +1,134 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Copyright (c) 2018 Linaro Limited
>>> + */
>>> +
>>> +#ifndef __TEE_H
>>> +#define __TEE_H
>>> +
>>> +#include <common.h>
>>> +#include <dm.h>
>>
>> These should be included by .c files.
>
> Yes, but what those provides is also needed by this .h file.
> Aren't all .h files supposed to include what they depend on?

common.h should be included first by any .c file. I suppose it is not
essential if it does not use CONFIG options (e.g. just a library file
for a 3rd-party library), but that is the general rule. So it should
not be in header files.

For dm.h, I have adopted a similar convention. If you like you can add
things like 'struct udevice;' I have done that in cases where dm.h is
needed. I believe reducing unnecessary includes helps to reduce the
amount of code that goes through the preprocessor, but perhaps I am
superstitious. Chrome C++ does the same thing.

> Thanks for the review. I posted V2 of this patch set at the same time
> as you replied, I'll address what's not already done in V2 in the
> coming V3.

Yes I think I lost an email as I thought I had already replied on some
of these points, and apparently I had.

Regards,
Simon

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 58b61ac05882..7458c606ee92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -571,6 +571,12 @@  TQ GROUP
 S:	Orphaned (Since 2016-02)
 T:	git git://git.denx.de/u-boot-tq-group.git
 
+TEE
+M:	Jens Wiklander <jens.wiklander@linaro.org>
+S:	Maintained
+F:	drivers/tee/
+F:	include/tee.h
+
 UBI
 M:	Kyungmin Park <kmpark@infradead.org>
 M:	Heiko Schocher <hs@denx.de>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index c72abf893297..f3249ab1d143 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -94,6 +94,8 @@  source "drivers/spmi/Kconfig"
 
 source "drivers/sysreset/Kconfig"
 
+source "drivers/tee/Kconfig"
+
 source "drivers/thermal/Kconfig"
 
 source "drivers/timer/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index d53208540ea6..0fcae36f50f7 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -103,6 +103,7 @@  obj-y += smem/
 obj-y += soc/
 obj-$(CONFIG_REMOTEPROC) += remoteproc/
 obj-y += thermal/
+obj-$(CONFIG_TEE) += tee/
 
 obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
 endif
diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
new file mode 100644
index 000000000000..817ab331b0f8
--- /dev/null
+++ b/drivers/tee/Kconfig
@@ -0,0 +1,8 @@ 
+# Generic Trusted Execution Environment Configuration
+config TEE
+	bool "Trusted Execution Environment support"
+	depends on ARM && (ARM64 || CPU_V7A)
+	select ARM_SMCCC
+	help
+	  This implements a generic interface towards a Trusted Execution
+	  Environment (TEE).
diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
new file mode 100644
index 000000000000..b6d8e16e6211
--- /dev/null
+++ b/drivers/tee/Makefile
@@ -0,0 +1,3 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+
+obj-y += tee-uclass.o
diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
new file mode 100644
index 000000000000..f0243a0f2e4e
--- /dev/null
+++ b/drivers/tee/tee-uclass.c
@@ -0,0 +1,180 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2018 Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <tee.h>
+
+struct tee_uclass_priv {
+	struct list_head list_shm;
+};
+
+static const struct tee_driver_ops *tee_get_ops(struct udevice *dev)
+{
+	return device_get_ops(dev);
+}
+
+void tee_get_version(struct udevice *dev, struct tee_version_data *vers)
+{
+	tee_get_ops(dev)->get_version(dev, vers);
+}
+
+int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
+		     ulong num_param, struct tee_param *param)
+{
+	return tee_get_ops(dev)->open_session(dev, arg, num_param, param);
+}
+
+int tee_close_session(struct udevice *dev, u32 session)
+{
+	return tee_get_ops(dev)->close_session(dev, session);
+}
+
+int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
+		    ulong num_param, struct tee_param *param)
+{
+	return tee_get_ops(dev)->invoke_func(dev, arg, num_param, param);
+}
+
+struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
+			      ulong size, u32 flags)
+{
+	struct tee_shm *shm;
+	void *p = addr;
+
+	if (flags & TEE_SHM_ALLOC) {
+		if (align)
+			p = memalign(align, size);
+		else
+			p = malloc(size);
+	}
+	if (!p)
+		return NULL;
+
+	shm = calloc(1, sizeof(*shm));
+	if (!shm)
+		goto err;
+
+	shm->dev = dev;
+	shm->addr = p;
+	shm->size = size;
+	shm->flags = flags;
+
+	if ((flags & TEE_SHM_SEC_REGISTER) &&
+	    tee_get_ops(dev)->shm_register(dev, shm))
+		goto err;
+
+	if (flags & TEE_SHM_REGISTER) {
+		struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
+
+		list_add(&shm->link, &priv->list_shm);
+	}
+	return shm;
+err:
+	free(shm);
+	if (flags & TEE_SHM_ALLOC)
+		free(p);
+	return NULL;
+}
+
+struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size,
+			      u32 flags)
+{
+	u32 f = flags;
+
+	f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER | TEE_SHM_ALLOC;
+	return __tee_shm_add(dev, 0, NULL, size, f);
+}
+
+struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
+				 ulong size, u32 flags)
+{
+	u32 f = flags & ~TEE_SHM_ALLOC;
+
+	f |= TEE_SHM_SEC_REGISTER | TEE_SHM_REGISTER;
+	return __tee_shm_add(dev, 0, addr, size, f);
+}
+
+void tee_shm_free(struct tee_shm *shm)
+{
+	if (!shm)
+		return;
+
+	if (shm->flags & TEE_SHM_SEC_REGISTER)
+		tee_get_ops(shm->dev)->shm_unregister(shm->dev, shm);
+
+	if (shm->flags & TEE_SHM_REGISTER)
+		list_del(&shm->link);
+
+	if (shm->flags & TEE_SHM_ALLOC)
+		free(shm->addr);
+
+	free(shm);
+}
+
+bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev)
+{
+	struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
+	struct tee_shm *s;
+
+	list_for_each_entry(s, &priv->list_shm, link)
+		if (s == shm)
+			return true;
+
+	return false;
+}
+
+struct udevice *tee_find_device(struct udevice *start,
+				int (*match)(struct tee_version_data *vers,
+					     const void *data),
+				const void *data,
+				struct tee_version_data *vers)
+{
+	struct udevice *dev = start;
+	struct tee_version_data lv;
+	struct tee_version_data *v = vers ? vers : &lv;
+
+	if (!dev)
+		uclass_first_device(UCLASS_TEE, &dev);
+
+	for (; dev; uclass_next_device(&dev)) {
+		tee_get_ops(dev)->get_version(dev, v);
+		if (!match || match(v, data))
+			return dev;
+	}
+
+	return NULL;
+}
+
+static int tee_pre_probe(struct udevice *dev)
+{
+	struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
+
+	INIT_LIST_HEAD(&priv->list_shm);
+	return 0;
+}
+
+static int tee_pre_remove(struct udevice *dev)
+{
+	struct tee_uclass_priv *priv = dev_get_uclass_priv(dev);
+	struct tee_shm *shm;
+
+	while (!list_empty(&priv->list_shm)) {
+		shm = list_first_entry(&priv->list_shm, struct tee_shm, link);
+		debug("%s: freeing leftover shm %p (size %lu, flags %#x)\n",
+		      __func__, (void *)shm, shm->size, shm->flags);
+		tee_shm_free(shm);
+	}
+
+	return 0;
+}
+
+UCLASS_DRIVER(tee) = {
+	.id = UCLASS_TEE,
+	.name = "tee",
+	.per_device_auto_alloc_size = sizeof(struct tee_uclass_priv),
+	.pre_probe = tee_pre_probe,
+	.pre_remove = tee_pre_remove,
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index a39643ec5eef..955e0a915b87 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -81,6 +81,7 @@  enum uclass_id {
 	UCLASS_SPI_GENERIC,	/* Generic SPI flash target */
 	UCLASS_SYSCON,		/* System configuration device */
 	UCLASS_SYSRESET,	/* System reset device */
+	UCLASS_TEE,		/* Trusted Execution Environment device */
 	UCLASS_THERMAL,		/* Thermal sensor */
 	UCLASS_TIMER,		/* Timer device */
 	UCLASS_TPM,		/* Trusted Platform Module TIS interface */
diff --git a/include/tee.h b/include/tee.h
new file mode 100644
index 000000000000..c2ac13e34128
--- /dev/null
+++ b/include/tee.h
@@ -0,0 +1,134 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2018 Linaro Limited
+ */
+
+#ifndef __TEE_H
+#define __TEE_H
+
+#include <common.h>
+#include <dm.h>
+
+#define TEE_UUID_LEN		16
+
+#define TEE_GEN_CAP_GP          BIT(0)	/* GlobalPlatform compliant TEE */
+#define TEE_GEN_CAP_REG_MEM     BIT(1)	/* Supports registering shared memory */
+
+#define TEE_SHM_REGISTER	BIT(0)
+#define TEE_SHM_SEC_REGISTER	BIT(1)
+#define TEE_SHM_ALLOC		BIT(2)
+
+#define TEE_PARAM_ATTR_TYPE_NONE		0	/* parameter not used */
+#define TEE_PARAM_ATTR_TYPE_VALUE_INPUT		1
+#define TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT	2
+#define TEE_PARAM_ATTR_TYPE_VALUE_INOUT		3	/* input and output */
+#define TEE_PARAM_ATTR_TYPE_MEMREF_INPUT	5
+#define TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT	6
+#define TEE_PARAM_ATTR_TYPE_MEMREF_INOUT	7	/* input and output */
+#define TEE_PARAM_ATTR_TYPE_MASK		0xff
+#define TEE_PARAM_ATTR_META			0x100
+#define TEE_PARAM_ATTR_MASK			(TEE_PARAM_ATTR_TYPE_MASK | \
+						 TEE_PARAM_ATTR_META)
+
+/*
+ * Some Global Platform error codes which has a meaning if the
+ * TEE_GEN_CAP_GP bit is returned by the driver.
+ */
+#define TEE_SUCCESS			0x00000000
+#define TEE_ERROR_GENERIC		0xffff0000
+#define TEE_ERROR_BAD_PARAMETERS	0xffff0006
+#define TEE_ERROR_ITEM_NOT_FOUND	0xffff0008
+#define TEE_ERROR_NOT_IMPLEMENTED	0xffff0009
+#define TEE_ERROR_NOT_SUPPORTED		0xffff000a
+#define TEE_ERROR_COMMUNICATION		0xffff000e
+#define TEE_ERROR_OUT_OF_MEMORY		0xffff000c
+#define TEE_ERROR_TARGET_DEAD		0xffff3024
+
+#define TEE_ORIGIN_COMMS		0x00000002
+
+struct tee_driver_ops;
+
+struct tee_shm {
+	struct udevice *dev;
+	struct list_head link;
+	void *addr;
+	ulong size;
+	u32 flags;
+};
+
+struct tee_param_memref {
+	ulong shm_offs;
+	ulong size;
+	struct tee_shm *shm;
+};
+
+struct tee_param_value {
+	u64 a;
+	u64 b;
+	u64 c;
+};
+
+struct tee_param {
+	u64 attr;
+	union {
+		struct tee_param_memref memref;
+		struct tee_param_value value;
+	} u;
+};
+
+struct tee_open_session_arg {
+	u8 uuid[TEE_UUID_LEN];
+	u8 clnt_uuid[TEE_UUID_LEN];
+	u32 clnt_login;
+	u32 session;
+	u32 ret;
+	u32 ret_origin;
+};
+
+struct tee_invoke_arg {
+	u32 func;
+	u32 session;
+	u32 ret;
+	u32 ret_origin;
+};
+
+struct tee_version_data {
+	u32 gen_caps;
+};
+
+struct tee_driver_ops {
+	void (*get_version)(struct udevice *dev, struct tee_version_data *vers);
+	int (*open_session)(struct udevice *dev,
+			    struct tee_open_session_arg *arg, ulong num_param,
+			    struct tee_param *param);
+	int (*close_session)(struct udevice *dev, u32 session);
+	int (*invoke_func)(struct udevice *dev, struct tee_invoke_arg *arg,
+			   ulong num_param, struct tee_param *param);
+	int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
+	int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
+};
+
+struct tee_shm *__tee_shm_add(struct udevice *dev, ulong align, void *addr,
+			      ulong size, u32 flags);
+struct tee_shm *tee_shm_alloc(struct udevice *dev, ulong size, u32 flags);
+struct tee_shm *tee_shm_register(struct udevice *dev, void *addr,
+				 ulong length, u32 flags);
+void tee_shm_free(struct tee_shm *shm);
+bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
+
+struct udevice *tee_find_device(struct udevice *start,
+				int (*match)(struct tee_version_data *vers,
+					     const void *data),
+				const void *data,
+				struct tee_version_data *vers);
+
+void tee_get_version(struct udevice *dev, struct tee_version_data *vers);
+int tee_open_session(struct udevice *dev, struct tee_open_session_arg *arg,
+		     ulong num_param, struct tee_param *param);
+
+int tee_close_session(struct udevice *dev, u32 session);
+
+int tee_invoke_func(struct udevice *dev, struct tee_invoke_arg *arg,
+		    ulong num_param, struct tee_param *param);
+
+#endif /*__TEE_H*/