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_SMCCChelpThis implements a generic interface towards a Trusted ExecutionEnvironment (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);elsep = 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