On Wed, Aug 29, 2018 at 06:28:48PM -0600, Simon Glass wrote:
> Hi Jens,
>
> On 23 August 2018 at 04:43, Jens Wiklander <jens.wiklan...@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.
>
> U-Boot
>
> >
> > Tested-by: Igor Opaniuk <igor.opan...@linaro.org>
> > Signed-off-by: Jens Wiklander <jens.wiklan...@linaro.org>
> > ---
> > MAINTAINERS | 6 +
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/tee/Kconfig | 8 ++
> > drivers/tee/Makefile | 3 +
> > drivers/tee/tee-uclass.c | 192 ++++++++++++++++++++++++++
> > include/dm/uclass-id.h | 1 +
> > include/tee.h | 290 +++++++++++++++++++++++++++++++++++++++
> > 8 files changed, 503 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
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> Various nits below.
>
> >
> > 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.wiklan...@linaro.org>
> > +S: Maintained
> > +F: drivers/tee/
> > +F: include/tee.h
> > +
> > UBI
> > M: Kyungmin Park <kmp...@infradead.org>
> > M: Heiko Schocher <h...@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).
>
> Please expand this. What is it? Why would I use it? Also perhaps add a
> web link?
I'll expand it. I'll skip the why part though.
>
> > 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..0209983491a0
> > --- /dev/null
> > +++ b/drivers/tee/tee-uclass.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2018 Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <tee.h>
> > +
> > +/**
> > + * struct tee_uclass_priv - information of a TEE, stored by the uclass
> > + *
> > + * @list_shm: list of structe tee_shm representing memory blocks shared
> > + * with the TEE.
> > + */
> > +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;
>
> blank line here
>
> > + 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;
>
> and here
>
> > + 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)
>
> Please add function comment
There is one in include/tee.h
> > +{
> > + 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);
>
> This can return an error if it finds a device but it tails to probe.
> If you don't care about that, I suppose it is OK.
Thanks. I need the first device that probes OK and then the next devices
that probes OK and so on. I'll need to make some small changes here.
> > +
> > + 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;
> > +
> > + /*
> > + * Any remaining shared memory must be unregistered now as U-boot
>
> U-Boot
>
> > + * is about to hand over to the next stage and that memory will be
> > + * reused.
> > + */
> > + 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..3e6771123ef0
> > --- /dev/null
> > +++ b/include/tee.h
> > @@ -0,0 +1,290 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2018 Linaro Limited
> > + */
> > +
> > +#ifndef __TEE_H
> > +#define __TEE_H
> > +
> > +#include <common.h>
> > +#include <dm.h>
>
> Please drop both of those.
>
> > +
> > +#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) /* In list of shared memory */
> > +#define TEE_SHM_SEC_REGISTER BIT(1) /* TEE notified of this memory */
> > +#define TEE_SHM_ALLOC BIT(2) /* The memory is malloced() and
> > must */
> > + /* be freed() */
> > +
> > +#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 in
> > + * struct tee_version_data::gen_caps
> > + */
> > +#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_shm - memory shared with the TEE
> > + * @dev: The TEE device
> > + * @link: List node in the list in struct struct tee_uclass_priv
> > + * @addr: Pointer to the shared memory
> > + * @size: Size of the the shared memory
> > + * @flags: TEE_SHM_* above
> > + */
> > +struct tee_shm {
> > + struct udevice *dev;
> > + struct list_head link;
> > + void *addr;
> > + ulong size;
> > + u32 flags;
> > +};
> > +
> > +/**
> > + * struct tee_param_memref - parameter memory reference
>
> Used for ...?
>
> > + * @shm_offs: Offset in bytes into the shared memory object @shm
> > + * @size: Size in bytes of the memory reference
> > + * @shm: Pointer to a shared memory object for the buffer
> > + */
> > +struct tee_param_memref {
> > + ulong shm_offs;
> > + ulong size;
> > + struct tee_shm *shm;
> > +};
> > +
> > +/**
> > + * struct tee_param_value - value parameter
>
> ??
>
> This doesn't really explain anything. What is it for?
I'll expand and refer to struct tee_param below.
>
> > + * @a, @b, @c: Parameters passed by value
> > + */
> > +struct tee_param_value {
> > + u64 a;
> > + u64 b;
> > + u64 c;
> > +};
> > +
> > +/**
> > + * struct tee_param - invoke parameter
> > + * @attr: Attributes
> > + * @u.memref: Memref parameter if (@attr & TEE_PARAM_ATTR_MASK) is one of
> > + * TEE_PARAM_ATTR_TYPE_MEMREF_* above
> > + * @u.value: Value parameter if (@attr & TEE_PARAM_ATTR_MASK) is one of
> > + * TEE_PARAM_ATTR_TYPE_VALUE_* above
> > + */
> > +struct tee_param {
> > + u64 attr;
> > + union {
> > + struct tee_param_memref memref;
> > + struct tee_param_value value;
> > + } u;
> > +};
> > +
> > +/**
> > + * struct tee_open_session_arg - extra arguments for tee_open_session()
> > + * @uuid: [in] UUID of the Trusted Application
> > + * @clnt_uuid: [in] Normally zeroes
> > + * @clnt_login: [in] Normally 0
> > + * @session: [out] Session id
> > + * @ret: [out] return value
> > + * @ret_origin: [out] origin of the return value
> > + */
> > +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;
>
> Do these use the sized u32 type because it is an API of some sort?
> Otherwise, why not uint?
Yes, the minimum width is 32-bits and wider than that is waste.
>
> > +};
> > +
> > +/**
> > + * struct tee_invoke_arg - extra arguments for tee_invoke_func()
> > + * @func: [in] Trusted Application function, specific to the TA
> > + * @session: [in] Session id, from open session
> > + * @ret: [out] return value
> > + * @ret_origin: [out] origin of the return value
> > + */
> > +struct tee_invoke_arg {
> > + u32 func;
> > + u32 session;
> > + u32 ret;
> > + u32 ret_origin;
> > +};
> > +
> > +/**
> > + * struct tee_version_data - description of TEE
> > + * @gen_caps: Generic capabilities, TEE_GEN_CAP_* above
> > + */
> > +struct tee_version_data {
> > + u32 gen_caps;
> > +};
> > +
> > +/**
> > + * struct tee_driver_ops - TEE driver operations
> > + * @get_version: Query capabilities of TEE device,
> > + see tee_get_version()
> > + * @open_session: Opens a session to a Trusted Application in the TEE,
> > + * see tee_open_session().
> > + * @close_session: Closes a session to Trusted Application,
> > + * see tee_close_session()
> > + * @invoke_func: Invokes a function in a Trusted Application,
> > + see tee_invoke_func()
>
> These comments should be against each function in the struct below.
> You should repeat the comment from the exported functions there too.
I'll fix.
>
> > + */
> > +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);
> > + /**
> > + * shm_register() - Registers memory shared with the TEE
> > + * @dev: The TEE device
> > + * @shm: Pointer to a shared memory object
> > + * Returns 0 on success or < 0 on failure.
> > + */
> > + int (*shm_register)(struct udevice *dev, struct tee_shm *shm);
> > + /**
> > + * shm_unregister() - Unregisters memory shared with the TEE
> > + * @dev: The TEE device
> > + * @shm: Pointer to a shared memory object
> > + * Returns 0 on success or < 0 on failure.
> > + */
> > + int (*shm_unregister)(struct udevice *dev, struct tee_shm *shm);
> > +};
> > +
> [...]
Thanks for the review.
--
Jens
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
