Hi Jens,
On 23 August 2018 at 05:11, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
> Hi Simon,
>
> On Thu, Aug 23, 2018 at 12:45 PM, Simon Glass <s...@chromium.org> wrote:
>> Hi Jens,
>>
>> On 13 August 2018 at 09:53, 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.
>>>
>>> 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 | 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.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).
>>> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
