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

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

[Etienne Carriere]

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
+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 */
+
 /**
  * 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
+
 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
 	/*
 	 * 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 */
-- 
2.25.1

[PATCH 2/2] drivers: rng: optee_rng: register to CONFIG_OPTEE_SERVICE_DISCOVERY

[Etienne Carriere]

Changes optee_rng driver to register itself has a OP-TEE service so
that a device is bound for the driver when OP-TEE enumerates the
PTA RNG service.

Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/rng/Kconfig     |  1 +
 drivers/rng/optee_rng.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index c10f7d345b..14e95a6213 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -34,6 +34,7 @@ config RNG_MSM
 config RNG_OPTEE
 	bool "OP-TEE based Random Number Generator support"
 	depends on DM_RNG && OPTEE
+	default y if OPTEE_SERVICE_DISCOVERY
 	help
 	  This driver provides support for the OP-TEE based Random Number
 	  Generator on ARM SoCs where hardware entropy sources are not
diff --git a/drivers/rng/optee_rng.c b/drivers/rng/optee_rng.c
index aa8ce864d3..90d9434395 100644
--- a/drivers/rng/optee_rng.c
+++ b/drivers/rng/optee_rng.c
@@ -11,6 +11,9 @@
 #include <dm/device.h>
 #include <dm/device_compat.h>
 #include <linux/sizes.h>
+#include <tee/optee_service.h>
+
+#define DRIVER_NAME	"optee-rng"
 
 #define TEE_ERROR_HEALTH_TEST_FAIL	0x00000001
 
@@ -35,6 +38,13 @@
 #define TA_HWRNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \
 			{ 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } }
 
+#ifdef CONFIG_OPTEE_SERVICE_DISCOVERY
+OPTEE_SERVICE_DRIVER(optee_rng) = {
+	.uuid = TA_HWRNG_UUID,
+	.driver_name = DRIVER_NAME,
+};
+#endif
+
 /** open_session_ta_hwrng() - Open session with hwrng Trusted App
  *
  * @dev:		device
@@ -177,7 +187,7 @@ static const struct dm_rng_ops optee_rng_ops = {
 };
 
 U_BOOT_DRIVER(optee_rng) = {
-	.name = "optee-rng",
+	.name = DRIVER_NAME,
 	.id = UCLASS_RNG,
 	.ops = &optee_rng_ops,
 	.probe = optee_rng_probe,
-- 
2.25.1

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

[Patrick DELAUNAY]

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

Re: [PATCH 2/2] drivers: rng: optee_rng: register to CONFIG_OPTEE_SERVICE_DISCOVERY

[Patrick DELAUNAY]

Hi Etienne,

On 6/1/22 10:27, Etienne Carriere wrote:
> Changes optee_rng driver to register itself has a OP-TEE service so
> that a device is bound for the driver when OP-TEE enumerates the
> PTA RNG service.
>
> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>   drivers/rng/Kconfig     |  1 +
>   drivers/rng/optee_rng.c | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c10f7d345b..14e95a6213 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -34,6 +34,7 @@ config RNG_MSM
>   config RNG_OPTEE
>   	bool "OP-TEE based Random Number Generator support"
>   	depends on DM_RNG && OPTEE
> +	default y if OPTEE_SERVICE_DISCOVERY
>   	help
>   	  This driver provides support for the OP-TEE based Random Number
>   	  Generator on ARM SoCs where hardware entropy sources are not
> diff --git a/drivers/rng/optee_rng.c b/drivers/rng/optee_rng.c
> index aa8ce864d3..90d9434395 100644
> --- a/drivers/rng/optee_rng.c
> +++ b/drivers/rng/optee_rng.c
> @@ -11,6 +11,9 @@
>   #include <dm/device.h>
>   #include <dm/device_compat.h>
>   #include <linux/sizes.h>
> +#include <tee/optee_service.h>
> +
> +#define DRIVER_NAME	"optee-rng"
>   
>   #define TEE_ERROR_HEALTH_TEST_FAIL	0x00000001
>   
> @@ -35,6 +38,13 @@
>   #define TA_HWRNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \
>   			{ 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } }
>   
> +#ifdef CONFIG_OPTEE_SERVICE_DISCOVERY


#ifdef is really needed here


> +OPTEE_SERVICE_DRIVER(optee_rng) = {
> +	.uuid = TA_HWRNG_UUID,
> +	.driver_name = DRIVER_NAME,
> +};
> +#endif
> +
>   /** open_session_ta_hwrng() - Open session with hwrng Trusted App
>    *
>    * @dev:		device
> @@ -177,7 +187,7 @@ static const struct dm_rng_ops optee_rng_ops = {
>   };
>   
>   U_BOOT_DRIVER(optee_rng) = {
> -	.name = "optee-rng",
> +	.name = DRIVER_NAME,
>   	.id = UCLASS_RNG,
>   	.ops = &optee_rng_ops,
>   	.probe = optee_rng_probe,



Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

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

[Ilias Apalodimas]

Hi Etienne, 

On Wed, Jun 01, 2022 at 10:27:51AM +0200, 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
>  
 
[...]

> +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);

I'd prefer if we handled this a bit differently.  Instead of freeing the
buffer here, just release it on bind_service_drivers() always

> +	else
> +		*count = shm_size / sizeof(struct tee_optee_ta_uuid);
> +
> +	return ret;
> +}
> +
> +
>  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
>  	/*
>  	 * 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);

The same principle applies for fTPM.  Moreover the linux kernel supports
bus scanning, which creates a conflict when the fTPM is added on the .dts
(for u-boot to scan it).  

Can we make this a bit more generic, even though only the rng is added on
this patch?

something like 
struct devices {
	const char *drv_name;
	const char *dev_name;
} tee_bus_devices = {
	{
		"optee-rng",
		"optee-rng",
	},
}
and add an array of the 'scanable' devices?  It would make adding the ftpm
and other devices trivial

>  		if (ret)
>  			return ret;
>  	}
> +#endif
[...]


Thanks!
/Ilias

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

[Etienne Carriere]

Hi Ilias,

On Mon, 6 Jun 2022 at 11:49, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Etienne,
>
> On Wed, Jun 01, 2022 at 10:27:51AM +0200, 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
> >
>
> [...]
>
> > +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);
>
> I'd prefer if we handled this a bit differently.  Instead of freeing the
> buffer here, just release it on bind_service_drivers() always

Ok, i'll change this in patch v3.

>
> > +     else
> > +             *count = shm_size / sizeof(struct tee_optee_ta_uuid);
> > +
> > +     return ret;
> > +}
> > +
> > +
> >  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
> >       /*
> >        * 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);
>
> The same principle applies for fTPM.  Moreover the linux kernel supports
> bus scanning, which creates a conflict when the fTPM is added on the .dts
> (for u-boot to scan it).

Do you mean you would like fTPM driver to NOT be probed upon its
related DT compatible node and only probed from the fTPM TA discovery
(optee so-called devices enumeration)?

Another issue here is that current fTPM implementation [1] does not
set flag TA_FLAG_DEVICE_ENUM [2] that makes a built-in TA (so-called
early TA) to be enumerated by OP-TEE.

[1] https://github.com/microsoft/ms-tpm-20-ref/blob/d638536d0fe01acd5e39ffa1bd100b3da82d92c7/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
[2] https://github.com/OP-TEE/optee_os/blob/3.17.0/lib/libutee/include/user_ta_header.h#L26-L32

>
> Can we make this a bit more generic, even though only the rng is added on
> this patch?
>
> something like
> struct devices {
>         const char *drv_name;
>         const char *dev_name;
> } tee_bus_devices = {
>         {
>                 "optee-rng",
>                 "optee-rng",
>         },
> }
> and add an array of the 'scanable' devices?  It would make adding the ftpm
> and other devices trivial

Assuming fTPM TA is enumerated, i don't think we need to add a device
name here. fTPM service could be proved straight based on the driver
name. fTPM driver in u-boot expects there is only 1 TEE firmware,
hence only 1 fTPM TA instance.

For info, i'll send a patch v3 without changes on fTPM.

Best regards,
etienne

>
> >               if (ret)
> >                       return ret;
> >       }
> > +#endif
> [...]
>
>
> Thanks!
> /Ilias

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

[Ilias Apalodimas]

Hi Etienne,

[...]

> > > +
> > > +#ifndef CONFIG_OPTEE_SERVICE_DISCOVERY
> > >       /*
> > >        * 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);
> >
> > The same principle applies for fTPM.  Moreover the linux kernel supports
> > bus scanning, which creates a conflict when the fTPM is added on the .dts
> > (for u-boot to scan it).
>
> Do you mean you would like fTPM driver to NOT be probed upon its
> related DT compatible node and only probed from the fTPM TA discovery
> (optee so-called devices enumeration)?

That should be a user selected option.  If the dt entry is there we
should scan it as we do today.  However if the DT entry is not there I
believe we should try to scan the device from the tree bus.

>
> Another issue here is that current fTPM implementation [1] does not
> set flag TA_FLAG_DEVICE_ENUM [2] that makes a built-in TA (so-called
> early TA) to be enumerated by OP-TEE.
>
> [1] https://github.com/microsoft/ms-tpm-20-ref/blob/d638536d0fe01acd5e39ffa1bd100b3da82d92c7/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
> [2] https://github.com/OP-TEE/optee_os/blob/3.17.0/lib/libutee/include/user_ta_header.h#L26-L32

Yea I know there's  a PR fixing that but was posted on the initial
fTPM project  [1].  We need to refresh that

[1] https://github.com/microsoft/MSRSec/pull/34

>
> >
> > Can we make this a bit more generic, even though only the rng is added on
> > this patch?
> >
> > something like
> > struct devices {
> >         const char *drv_name;
> >         const char *dev_name;
> > } tee_bus_devices = {
> >         {
> >                 "optee-rng",
> >                 "optee-rng",
> >         },
> > }
> > and add an array of the 'scanable' devices?  It would make adding the ftpm
> > and other devices trivial
>
> Assuming fTPM TA is enumerated, i don't think we need to add a device
> name here. fTPM service could be proved straight based on the driver
> name. fTPM driver in u-boot expects there is only 1 TEE firmware,
> hence only 1 fTPM TA instance.
>
> For info, i'll send a patch v3 without changes on fTPM.

Yea don't add the ftpm now.  I only wanted to convert this to an
array, so we plug in new devices easier in the future.

Cheers
/Ilias
>
> Best regards,
> etienne
>
> >
> > >               if (ret)
> > >                       return ret;
> > >       }
> > > +#endif
> > [...]
> >
> >
> > Thanks!
> > /Ilias