Re: [PATCH 1/2] drivers: tee: optee: discover OP-TEE services

Hi,

minbor remarks on "#ifdef" usage.

On 6/1/22 10:27, Etienne Carriere wrote:

This change defines resources for OP-TEE service drivers to register themselves for being bound to when OP-TEE firmware reports the related service is supported. OP-TEE services are discovered during optee driver probe sequence. Discovery of optee services and binding to related U-Boot drivers is embedded upon configuration switch CONFIG_OPTEE_SERVICE_DISCOVERY.

Cc: Jens Wiklander jens.wiklander@linaro.org Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Etienne Carriere etienne.carriere@linaro.org

---

drivers/tee/optee/Kconfig | 8 ++ drivers/tee/optee/core.c | 187 +++++++++++++++++++++++++++++++++++- include/tee/optee_service.h | 29 ++++++ 3 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 include/tee/optee_service.h

diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig index d03028070b..9dc65b0501 100644 --- a/drivers/tee/optee/Kconfig +++ b/drivers/tee/optee/Kconfig @@ -37,6 +37,14 @@ config OPTEE_TA_SCP03 help Enables support for controlling (enabling, provisioning) the Secure Channel Protocol 03 operation in the OP-TEE SCP03 TA.

+config OPTEE_SERVICE_DISCOVERY

• bool "OP-TEE service discovery"

• default y

• help

•  This implements automated driver binding of OP-TEE service drivers by
  

•  requesting OP-TEE firmware to enumerate its hosted services.
  

• endmenu

  endif

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index a89d62aaf0..86a32f2a81 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -14,6 +14,7 @@ #include <linux/arm-smccc.h> #include <linux/err.h> #include <linux/io.h> +#include <tee/optee_service.h>

#include "optee_smc.h" #include "optee_msg.h" @@ -22,6 +23,25 @@ #define PAGELIST_ENTRIES_PER_PAGE \ ((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)

+/*

• • PTA_DEVICE_ENUM interface exposed by OP-TEE to discover enumerated services
• */

+#define PTA_DEVICE_ENUM { 0x7011a688, 0xddde, 0x4053, \

• 		  { 0xa5, 0xa9, 0x7b, 0x3c, 0x4d, 0xdf, 0x13, 0xb8 } }
  

+/*

• • PTA_CMD_GET_DEVICES - List services without supplicant dependencies
• • [out] memref[0]: List of the UUIDs of service enumerated by OP-TEE
• */

+#define PTA_CMD_GET_DEVICES 0x0

+/*

• • PTA_CMD_GET_DEVICES_SUPP - List services depending on tee supplicant
• • [out] memref[0]: List of the UUIDs of service enumerated by OP-TEE
• */

+#define PTA_CMD_GET_DEVICES_SUPP 0x1

• typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long,

@@ -42,6 +62,152 @@ struct rpc_param { u32 a7; };

+#ifdef CONFIG_OPTEE_SERVICE_DISCOVERY

avoid #ifdef CONFIG whne it is not mandatory

unused function will be dropped by linker

+static struct optee_service *find_service_driver(const struct tee_optee_ta_uuid *uuid) +{

• struct optee_service *service;
• u8 loc_uuid[TEE_UUID_LEN];
• size_t service_cnt, idx;
• service_cnt = ll_entry_count(struct optee_service, optee_service);
• service = ll_entry_start(struct optee_service, optee_service);
• for (idx = 0; idx < service_cnt; idx++, service++) {

• tee_optee_ta_uuid_to_octets(loc_uuid, &service->uuid);
  

• if (!memcmp(uuid, loc_uuid, sizeof(uuid)))
  

• 	return service;
  

• }
• return NULL;

+}

+static int bind_service_list(struct udevice *dev, struct tee_shm *service_list, size_t count) +{

• const struct tee_optee_ta_uuid *service_uuid = (const void *)service_list->addr;
• struct optee_service *service;
• size_t idx;
• int ret;
• for (idx = 0; idx < count; idx++) {

• service = find_service_driver(service_uuid + idx);
  

• if (!service)
  

• 	continue;
  

• ret = device_bind_driver(dev, service->driver_name, service->driver_name, NULL);
  

• if (ret) {
  

• 	dev_warn(dev, "%s was not bound: %d, ignored\n", service->driver_name, ret);
  

• 	continue;
  

• }
  

• }
• return 0;

+}

+static int __enum_services(struct udevice *dev, struct tee_shm *shm, u32 *shm_size, u32 tee_sess) +{

• struct tee_invoke_arg arg = { };
• struct tee_param param = { };
• int ret = 0;
• arg.func = PTA_CMD_GET_DEVICES;
• arg.session = tee_sess;
• /* Fill invoke cmd params */
• param.attr = TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
• param.u.memref.shm = shm;
• param.u.memref.size = *shm_size;
• ret = tee_invoke_func(dev, &arg, 1, &param);
• if (ret || (arg.ret && arg.ret != TEE_ERROR_SHORT_BUFFER)) {

• dev_err(dev, "PTA_CMD_GET_DEVICES invoke function err: 0x%x\n", arg.ret);
  

• return -EINVAL;
  

• }
• *shm_size = param.u.memref.size;
• return 0;

+}

+static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess) +{

• size_t shm_size = 0;
• int ret;
• ret = __enum_services(dev, NULL, &shm_size, tee_sess);
• if (ret)

• return ret;
  

• ret = tee_shm_alloc(dev, shm_size, 0, shm);
• if (ret) {

• dev_err(dev, "Failed to allocated shared memory: %d\n", ret);
  

• return ret;
  

• }
• ret = __enum_services(dev, *shm, &shm_size, tee_sess);
• if (ret)

• tee_shm_free(*shm);
  

• else

• *count = shm_size / sizeof(struct tee_optee_ta_uuid);
  

• return ret;

+}

+static int open_session(struct udevice *dev, u32 *tee_sess) +{

• const struct tee_optee_ta_uuid pta_uuid = PTA_DEVICE_ENUM;
• struct tee_open_session_arg arg = { };
• int ret;
• tee_optee_ta_uuid_to_octets(arg.uuid, &pta_uuid);
• ret = tee_open_session(dev, &arg, 0, NULL);
• if (ret || arg.ret) {

• if (!ret)
  

• 	ret = -EIO;
  

• return ret;
  

• }
• *tee_sess = arg.session;
• return 0;

+}

+static void close_session(struct udevice *dev, u32 tee_sess) +{

• tee_close_session(dev, tee_sess);

+}

+static int bind_service_drivers(struct udevice *dev) +{

• struct tee_shm *service_list = NULL;
• size_t service_count;
• u32 tee_sess;
• int ret;
• ret = open_session(dev, &tee_sess);
• if (ret)

• return ret;
  

• ret = enum_services(dev, &service_list, &service_count, tee_sess);
• if (ret)

• goto close_session;
  

• ret = bind_service_list(dev, service_list, service_count);
• tee_shm_free(service_list);

+close_session:

• close_session(dev, tee_sess);
• return ret;

+} +#else +static int bind_service_drivers(struct udevice *dev) +{

• return 0;

+} +#endif /* CONFIG_OPTEE_SERVICE_DISCOVERY */

always define the function with:

+static int bind_service_drivers(struct udevice *dev) +{ + struct tee_shm *service_list = NULL; + size_t service_count; + u32 tee_sess; + int ret; + + if (!IS_ENABLED(CONFIG_OPTEE_SERVICE_DISCOVERY)) + dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); + + ret = open_session(dev, &tee_sess); + if (ret) + return ret; + + ret = enum_services(dev, &service_list, &service_count, tee_sess); + if (ret) + goto close_session; + + ret = bind_service_list(dev, service_list, service_count); + + tee_shm_free(service_list); + +close_session: + close_session(dev, tee_sess); + + return ret; +}

+static int optee_bind(struct udevice *dev) +{ + if (IS_ENABLED(CONFIG_OPTEE_SERVICE_DISCOVERY)) + dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); + + return 0; +}

• /**
  • reg_pair_to_ptr() - Make a pointer of 2 32-bit values
  • @reg0: High bits of the pointer

@@ -638,11 +804,19 @@ static int optee_of_to_plat(struct udevice *dev) return 0; }

+#ifdef CONFIG_OPTEE_SERVICE_DISCOVERY +static int optee_bind(struct udevice *dev) +{

• dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
• return 0;

+} +#endif

always define the function with:

+static int optee_bind(struct udevice *dev) +{ + if (IS_ENABLED(CONFIG_OPTEE_SERVICE_DISCOVERY)) + dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); + + return 0; +}

• static int optee_probe(struct udevice *dev) { struct optee_pdata *pdata = dev_get_plat(dev); u32 sec_caps;

• struct udevice *child; int ret;

  if (!is_optee_api(pdata->invoke_fn)) {

@@ -668,15 +842,23 @@ static int optee_probe(struct udevice *dev) return -ENOENT; }

• ret = bind_service_drivers(dev);
• if (ret)

• return ret;
  

+#ifndef CONFIG_OPTEE_SERVICE_DISCOVERY

cna be handle wihout any #ifdef with:

+ if (IS_ENABLED(CONFIG_OPTEE_SERVICE_DISCOVERY)) { + ret = bind_service_drivers(dev); + if (ret) + return ret; + } else { + /* + * When the discovery of TA on the TEE bus is not supported: + * only bind the drivers associated to the supported OP-TEE TA + */ + if (IS_ENABLED(CONFIG_RNG_OPTEE)) { + struct udevice *child; + + ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); + if (ret) + return ret; + } + }

/* * in U-Boot, the discovery of TA on the TEE bus is not supported: * only bind the drivers associated to the supported OP-TEE TA */ if (IS_ENABLED(CONFIG_RNG_OPTEE)) {

• struct udevice *child;
  

• ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); if (ret) return ret; }

+#endif

return 0; } @@ -692,6 +874,9 @@ U_BOOT_DRIVER(optee) = { .of_match = optee_match, .of_to_plat = optee_of_to_plat, .probe = optee_probe, +#ifdef CONFIG_OPTEE_SERVICE_DISCOVERY

• .bind = optee_bind,

+#endif .ops = &optee_ops, .plat_auto = sizeof(struct optee_pdata), .priv_auto = sizeof(struct optee_private), diff --git a/include/tee/optee_service.h b/include/tee/optee_service.h new file mode 100644 index 0000000000..31732979da --- /dev/null +++ b/include/tee/optee_service.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/*

• • (C) Copyright 2022 Linaro Limited
• */

+#ifndef _OPTEE_SERVICE_H +#define _OPTEE_SERVICE_H

+/*

• • struct optee_service - Discoverable OP-TEE service
• • @driver_name - Name of the related driver
• • @uuid - UUID of the OP-TEE service related to the driver
• • Use macro OPTEE_SERVICE_DRIVER() to register a driver related to an
• • OP-TEE service discovered when driver asks OP-TEE services enumaration.
• */

+struct optee_service {

• const char *driver_name;
• const struct tee_optee_ta_uuid uuid;

+};

+#define OPTEE_SERVICE_DRIVER(__name) \

• ll_entry_declare(struct optee_service, __name, optee_service)

+#define OPTEE_SERVICE_DRIVER_GET(__name) \

• ll_entry_get(struct optee_service, __name, optee_service)

+#endif /* _OPTEE_SERVICE_H */

Regards

Patrick