[PATCHv3 0/4] OP-TEE I2C trampoline and associated tests

[PATCHv3 0/4] OP-TEE I2C trampoline and associated tests

[Jorge Ramirez-Ortiz]

This patchset allows OP-TEE to communicate with I2C devices; a typical
use case would be servicing U-Boot requests that require underlying
cryptographic operations implemented by an I2C chip.

On a board fitted with the NXP SE050 I2C secure element, OP-TEE can
route some of the cryptographic operations it needs to that device (ie
RSA, ECC, CTR..).

Before the REE executes, OP-TEE would use its own I2C drivers to
communicate with the device on the bus; later on, once the REE is up,
accesses to the I2C bus should be coordinated with the REE to avoid
collisions. However instead of implementing such a synchronization
mechanism, this trampoline service permits OP-TEE to route those I2C
requests back to U-boot without then having to worry about collisions.

Lets suppose that U-Boot executes the trusted application Android
Verified Boot; when OP-TEE receives the request - and before executing
the application - it uses RSA to verify it. So on the back of the TA
function invocation, OP-TEE returns to U-boot with a sequence of RPC
calls requesting I2C transfers (check carefully the implementation in
do_call_with_arg(...) implemented in drivers/tee/optee/core.c and
notice the while loop)

When using sandbox testing, RPC is called directly to validate its
actual implementation; however as succintly described above, these
calls will always be originated in OP-TEE.

Igor Opaniuk (3):
  test: py: add pygit2 and pyelftools to requirements.txt
  drivers: tee: sandbox: add rpc test ta emulation
  test: dm: tee: extend with RPC test

Jorge Ramirez-Ortiz (1):
  drivers: tee: i2c trampoline driver

 drivers/tee/Makefile                     |   2 +
 drivers/tee/optee/Kconfig                |   9 ++
 drivers/tee/optee/Makefile               |   1 +
 drivers/tee/optee/i2c.c                  |  90 +++++++++++++++
 drivers/tee/optee/optee_msg.h            |  21 ++++
 drivers/tee/optee/optee_msg_supplicant.h |   5 +
 drivers/tee/optee/optee_private.h        |  17 +++
 drivers/tee/optee/supplicant.c           |   3 +
 drivers/tee/sandbox.c                    | 137 ++++++++++++++++++++++-
 include/tee/optee_ta_rpc_test.h          |  28 +++++
 test/dm/tee.c                            | 109 +++++++++++++++++-
 test/py/requirements.txt                 |   2 +
 12 files changed, 415 insertions(+), 9 deletions(-)
 create mode 100644 drivers/tee/optee/i2c.c
 create mode 100644 include/tee/optee_ta_rpc_test.h

-- 
2.17.1

[PATCHv3 1/4] drivers: tee: i2c trampoline driver

[Jorge Ramirez-Ortiz]

This commit gives the secure world access to the I2C bus so it can
communicate with I2C slaves (typically those would be secure elements
like the NXP SE050).

A similar service implementation has been merged in linux:
c05210ab ("drivers: optee: allow op-tee to access devices on the i2c
bus")

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 drivers/tee/optee/Makefile               |  1 +
 drivers/tee/optee/i2c.c                  | 90 ++++++++++++++++++++++++
 drivers/tee/optee/optee_msg.h            | 21 ++++++
 drivers/tee/optee/optee_msg_supplicant.h |  5 ++
 drivers/tee/optee/optee_private.h        | 17 +++++
 drivers/tee/optee/supplicant.c           |  3 +
 6 files changed, 137 insertions(+)
 create mode 100644 drivers/tee/optee/i2c.c

diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
index 928d3f8002..068c6e7aa1 100644
--- a/drivers/tee/optee/Makefile
+++ b/drivers/tee/optee/Makefile
@@ -2,4 +2,5 @@
 
 obj-y += core.o
 obj-y += supplicant.o
+obj-$(CONFIG_DM_I2C) += i2c.o
 obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
new file mode 100644
index 0000000000..ef4e10f991
--- /dev/null
+++ b/drivers/tee/optee/i2c.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright (c) 2020 Foundries.io Ltd
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+#include <tee.h>
+#include "optee_msg.h"
+#include "optee_private.h"
+
+static int check_xfer_flags(struct udevice *chip, uint tee_flags)
+{
+	uint flags;
+	int ret;
+
+	ret = i2c_get_chip_flags(chip, &flags);
+	if (ret)
+		return ret;
+
+	if (tee_flags & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT) {
+		if (!(flags & DM_I2C_CHIP_10BIT))
+			return -EINVAL;
+	} else {
+		if (flags & DM_I2C_CHIP_10BIT)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+void optee_suppl_cmd_i2c_transfer(struct optee_msg_arg *arg)
+{
+	const u8 attr[] = {
+		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+		OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
+		OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
+	};
+	struct udevice *chip_dev;
+	struct tee_shm *shm;
+	u8 *buf;
+	int ret;
+
+	if (arg->num_params != ARRAY_SIZE(attr) ||
+	    arg->params[0].attr != attr[0] ||
+	    arg->params[1].attr != attr[1] ||
+	    arg->params[2].attr != attr[2] ||
+	    arg->params[3].attr != attr[3]) {
+		goto bad;
+	}
+
+	shm = (struct tee_shm *)(unsigned long)arg->params[2].u.rmem.shm_ref;
+	buf = shm->addr;
+	if (!buf)
+		goto bad;
+
+	if (i2c_get_chip_for_busnum((int)arg->params[0].u.value.b,
+				    (int)arg->params[0].u.value.c,
+				    0, &chip_dev))
+		goto bad;
+
+	if (check_xfer_flags(chip_dev, arg->params[1].u.value.a))
+		goto bad;
+
+	switch (arg->params[0].u.value.a) {
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+		ret = dm_i2c_read(chip_dev, 0, buf,
+				  (size_t)arg->params[2].u.rmem.size);
+		break;
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+		ret = dm_i2c_write(chip_dev, 0, buf,
+				   (size_t)arg->params[2].u.rmem.size);
+		break;
+	default:
+		goto bad;
+	}
+
+	if (ret) {
+		arg->ret = TEE_ERROR_COMMUNICATION;
+	} else {
+		arg->params[3].u.value.a = arg->params[2].u.rmem.size;
+		arg->ret = TEE_SUCCESS;
+	}
+
+	return;
+bad:
+	arg->ret = TEE_ERROR_BAD_PARAMETERS;
+}
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 24c60960fc..8d40ce60c2 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -422,4 +422,25 @@ struct optee_msg_arg {
  */
 #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
 
+/*
+ * Access a device on an i2c bus
+ *
+ * [in]  param[0].u.value.a		mode: RD(0), WR(1)
+ * [in]  param[0].u.value.b		i2c adapter
+ * [in]  param[0].u.value.c		i2c chip
+ *
+ * [in]  param[1].u.value.a		i2c control flags
+ *
+ * [in/out] memref[2]			buffer to exchange the transfer data
+ *					with the secure world
+ *
+ * [out]  param[3].u.value.a		bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+/* I2C master transfer modes */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+/* I2C master control flags */
+#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
+
 #endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
index a0fb8063c8..963cfd4782 100644
--- a/drivers/tee/optee/optee_msg_supplicant.h
+++ b/drivers/tee/optee/optee_msg_supplicant.h
@@ -147,6 +147,11 @@
 #define OPTEE_MSG_RPC_CMD_SHM_ALLOC	6
 #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
 
+/*
+ * I2C bus access
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+
 /*
  * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
  */
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 9442d1c176..1f07a27ee4 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -60,6 +60,23 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
 }
 #endif
 
+#ifdef CONFIG_DM_I2C
+/**
+ * optee_suppl_cmd_i2c_transfer() - route I2C requests to an I2C chip
+ * @arg:	OP-TEE message (layout specified in optee_msg.h) defining the
+ *		transfer mode (read/write), adapter, chip and control flags.
+ *
+ * Handles OP-TEE requests to transfer data to the I2C chip on the I2C adapter.
+ */
+void optee_suppl_cmd_i2c_transfer(struct optee_msg_arg *arg);
+#else
+static inline void optee_suppl_cmd_i2c_transfer(struct optee_msg_arg *arg)
+{
+	debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
+	arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
+}
+#endif
+
 void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
 
 #endif /* __OPTEE_PRIVATE_H */
diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
index ae042b9a20..f9dd874b59 100644
--- a/drivers/tee/optee/supplicant.c
+++ b/drivers/tee/optee/supplicant.c
@@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
 	case OPTEE_MSG_RPC_CMD_RPMB:
 		optee_suppl_cmd_rpmb(dev, arg);
 		break;
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+		optee_suppl_cmd_i2c_transfer(arg);
+		break;
 	default:
 		arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
 	}
-- 
2.17.1

[PATCHv3 1/4] drivers: tee: i2c trampoline driver

[Simon Glass]

On Tue, 12 Jan 2021 at 01:43, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> This commit gives the secure world access to the I2C bus so it can
> communicate with I2C slaves (typically those would be secure elements
> like the NXP SE050).
>
> A similar service implementation has been merged in linux:
> c05210ab ("drivers: optee: allow op-tee to access devices on the i2c
> bus")
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/tee/optee/Makefile               |  1 +
>  drivers/tee/optee/i2c.c                  | 90 ++++++++++++++++++++++++
>  drivers/tee/optee/optee_msg.h            | 21 ++++++
>  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
>  drivers/tee/optee/optee_private.h        | 17 +++++
>  drivers/tee/optee/supplicant.c           |  3 +
>  6 files changed, 137 insertions(+)
>  create mode 100644 drivers/tee/optee/i2c.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCHv3 2/4] test: py: add pygit2 and pyelftools to requirements.txt

[Jorge Ramirez-Ortiz]

From: Igor Opaniuk <igor.opaniuk@foundries.io>

Add pygit2 and pyelftools to the list of packages for virtualenv
needed to run all sets of pytests.This fixes warnings like:

binman.elf_test.TestElf.testDecodeElf (subunit.RemotedTestCase):
Python elftools not available

Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
---
 test/py/requirements.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/py/requirements.txt b/test/py/requirements.txt
index cf251186f4..926bccad69 100644
--- a/test/py/requirements.txt
+++ b/test/py/requirements.txt
@@ -10,6 +10,8 @@ packaging==19.2
 pbr==5.4.3
 pluggy==0.13.0
 py==1.8.0
+pyelftools==0.27
+pygit2==1.4.0
 pyparsing==2.4.2
 pytest==5.2.1
 python-mimeparse==1.6.0
-- 
2.17.1

[PATCHv3 2/4] test: py: add pygit2 and pyelftools to requirements.txt

[Simon Glass]

On Tue, 12 Jan 2021 at 01:43, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
>
> Add pygit2 and pyelftools to the list of packages for virtualenv
> needed to run all sets of pytests.This fixes warnings like:
>
> binman.elf_test.TestElf.testDecodeElf (subunit.RemotedTestCase):
> Python elftools not available
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
>  test/py/requirements.txt | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCHv3 3/4] drivers: tee: sandbox: add rpc test ta emulation

[Jorge Ramirez-Ortiz]

From: Igor Opaniuk <igor.opaniuk@foundries.io>

This adds support for RPC test trusted application emulation, which
permits to test reverse RPC calls to TEE supplicant. Currently it covers
requests to the I2C bus from TEE.

Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
---
 drivers/tee/Makefile            |   2 +
 drivers/tee/optee/Kconfig       |   9 +++
 drivers/tee/sandbox.c           | 137 +++++++++++++++++++++++++++++++-
 include/tee/optee_ta_rpc_test.h |  28 +++++++
 4 files changed, 172 insertions(+), 4 deletions(-)
 create mode 100644 include/tee/optee_ta_rpc_test.h

diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
index 5c8ffdbce8..ff844195ae 100644
--- a/drivers/tee/Makefile
+++ b/drivers/tee/Makefile
@@ -2,5 +2,7 @@
 
 obj-y += tee-uclass.o
 obj-$(CONFIG_SANDBOX) += sandbox.o
+obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/supplicant.o
+obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/i2c.o
 obj-$(CONFIG_OPTEE) += optee/
 obj-y += broadcom/
diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
index d489834df9..65622f30b1 100644
--- a/drivers/tee/optee/Kconfig
+++ b/drivers/tee/optee/Kconfig
@@ -22,6 +22,15 @@ config OPTEE_TA_AVB
 	  The TA can support the "avb" subcommands "read_rb", "write"rb"
 	  and "is_unlocked".
 
+config OPTEE_TA_RPC_TEST
+	bool "Support RPC TEST TA"
+	depends on SANDBOX_TEE
+	default y
+	help
+	  Enables support for RPC test trusted application emulation, which
+	  permits to test reverse RPC calls to TEE supplicant. Should
+	  be used only in sandbox env.
+
 endmenu
 
 endif
diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
index 4b91e7db1b..1cacd443f4 100644
--- a/drivers/tee/sandbox.c
+++ b/drivers/tee/sandbox.c
@@ -7,11 +7,15 @@
 #include <sandboxtee.h>
 #include <tee.h>
 #include <tee/optee_ta_avb.h>
+#include <tee/optee_ta_rpc_test.h>
+
+#include "optee/optee_msg.h"
+#include "optee/optee_private.h"
 
 /*
  * The sandbox tee driver tries to emulate a generic Trusted Exectution
- * Environment (TEE) with the Trusted Application (TA) OPTEE_TA_AVB
- * available.
+ * Environment (TEE) with the Trusted Applications (TA) OPTEE_TA_AVB and
+ * OPTEE_TA_RPC_TEST available.
  */
 
 static const u32 pstorage_max = 16;
@@ -32,7 +36,32 @@ struct ta_entry {
 			   struct tee_param *params);
 };
 
-#ifdef CONFIG_OPTEE_TA_AVB
+static int get_msg_arg(struct udevice *dev, uint num_params,
+		       struct tee_shm **shmp, struct optee_msg_arg **msg_arg)
+{
+	int rc;
+	struct optee_msg_arg *ma;
+
+	rc = __tee_shm_add(dev, OPTEE_MSG_NONCONTIG_PAGE_SIZE, NULL,
+			   OPTEE_MSG_GET_ARG_SIZE(num_params), TEE_SHM_ALLOC,
+			   shmp);
+	if (rc)
+		return rc;
+
+	ma = (*shmp)->addr;
+	memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
+	ma->num_params = num_params;
+	*msg_arg = ma;
+
+	return 0;
+}
+
+inline void *optee_alloc_and_init_page_list(void *buf, ulong len,
+					   u64 *phys_buf_ptr)
+{
+	return 0;
+}
+
 static u32 get_attr(uint n, uint num_params, struct tee_param *params)
 {
 	if (n >= num_params)
@@ -63,6 +92,7 @@ bad_params:
 	return TEE_ERROR_BAD_PARAMETERS;
 }
 
+#ifdef CONFIG_OPTEE_TA_AVB
 static u32 ta_avb_open_session(struct udevice *dev, uint num_params,
 			       struct tee_param *params)
 {
@@ -214,7 +244,100 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
 		return TEE_ERROR_NOT_SUPPORTED;
 	}
 }
-#endif /*OPTEE_TA_AVB*/
+#endif /* OPTEE_TA_AVB */
+
+#ifdef CONFIG_OPTEE_TA_RPC_TEST
+static u32 ta_rpc_test_open_session(struct udevice *dev, uint num_params,
+				    struct tee_param *params)
+{
+	/*
+	 * We don't expect additional parameters when opening a session to
+	 * this TA.
+	 */
+	return check_params(TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
+			    TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
+			    num_params, params);
+}
+
+static void fill_i2c_rpc_params(struct optee_msg_arg *msg_arg, u64 bus_num,
+				u64 chip_addr, u64 op,
+				struct tee_param_memref memref)
+{
+	msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
+	msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
+	msg_arg->params[2].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INOUT;
+	msg_arg->params[3].attr = OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT;
+
+	/* trigger I2C services of TEE supplicant */
+	msg_arg->cmd = OPTEE_MSG_RPC_CMD_I2C_TRANSFER;
+
+	msg_arg->params[0].u.value.a = op;
+	msg_arg->params[0].u.value.b = bus_num;
+	msg_arg->params[0].u.value.c = chip_addr;
+
+	/* buffer to read/write data */
+	msg_arg->params[2].u.rmem.shm_ref = (ulong)memref.shm;
+	msg_arg->params[2].u.rmem.size = memref.size;
+	msg_arg->params[2].u.rmem.offs = memref.shm_offs;
+
+	msg_arg->num_params = 4;
+}
+
+static u32 ta_rpc_test_invoke_func(struct udevice *dev, u32 func,
+				   uint num_params,
+				   struct tee_param *params)
+{
+	struct tee_shm *shm;
+	struct tee_param_memref memref_data;
+	struct optee_msg_arg *msg_arg;
+	int chip_addr, bus_num, op;
+	int res;
+
+	res = check_params(TEE_PARAM_ATTR_TYPE_VALUE_INPUT,
+			   TEE_PARAM_ATTR_TYPE_MEMREF_INOUT,
+			   TEE_PARAM_ATTR_TYPE_NONE,
+			   TEE_PARAM_ATTR_TYPE_NONE,
+			   num_params, params);
+	if (res)
+		return TEE_ERROR_BAD_PARAMETERS;
+
+	bus_num = params[0].u.value.a;
+	chip_addr = params[0].u.value.b;
+	memref_data = params[1].u.memref;
+
+	switch (func) {
+	case TA_RPC_TEST_CMD_I2C_READ:
+		op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD;
+		break;
+	case TA_RPC_TEST_CMD_I2C_WRITE:
+		op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR;
+		break;
+	default:
+		return TEE_ERROR_NOT_SUPPORTED;
+	}
+
+	/*
+	 * Fill params for an RPC call to tee supplicant
+	 */
+	res = get_msg_arg(dev, 4, &shm, &msg_arg);
+	if (res)
+		goto bad;
+
+	fill_i2c_rpc_params(msg_arg, bus_num, chip_addr, op, memref_data);
+
+	/* Make an RPC call to tee supplicant */
+	optee_suppl_cmd(dev, shm, 0);
+	res = msg_arg->ret;
+
+bad:
+	tee_shm_free(shm);
+
+	if (res)
+		return res;
+
+	return TEE_SUCCESS;
+}
+#endif /* CONFIG_OPTEE_TA_RPC_TEST */
 
 static const struct ta_entry ta_entries[] = {
 #ifdef CONFIG_OPTEE_TA_AVB
@@ -223,6 +346,12 @@ static const struct ta_entry ta_entries[] = {
 	  .invoke_func = ta_avb_invoke_func,
 	},
 #endif
+#ifdef CONFIG_OPTEE_TA_RPC_TEST
+	{ .uuid = TA_RPC_TEST_UUID,
+	  .open_session = ta_rpc_test_open_session,
+	  .invoke_func = ta_rpc_test_invoke_func,
+	},
+#endif
 };
 
 static void sandbox_tee_get_version(struct udevice *dev,
diff --git a/include/tee/optee_ta_rpc_test.h b/include/tee/optee_ta_rpc_test.h
new file mode 100644
index 0000000000..cae2fb04b4
--- /dev/null
+++ b/include/tee/optee_ta_rpc_test.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+/* Copyright (c) 2020 Foundries Ltd */
+
+#ifndef __TA_RPC_TEST_H
+#define __TA_RPC_TEST_H
+
+#define TA_RPC_TEST_UUID { 0x48420575, 0x96ca, 0x401a, \
+		      { 0x89, 0x91, 0x1e, 0xfd, 0xce, 0xbd, 0x7d, 0x04 } }
+
+/*
+ * Does a reverse RPC call for I2C read
+ *
+ * in		params[0].value.a:	bus number
+ * in		params[0].value.b:	chip address
+ * inout	params[1].u.memref:	buffer to read data
+ */
+#define TA_RPC_TEST_CMD_I2C_READ	0
+
+/*
+ * Does a reverse RPC call for I2C write
+ *
+ * in		params[0].value.a:	bus number
+ * in		params[0].value.b:	chip address
+ * inout	params[1].u.memref:	buffer with data to write
+ */
+#define TA_RPC_TEST_CMD_I2C_WRITE	1
+
+#endif /* __TA_RPC_TEST_H */
-- 
2.17.1

[PATCHv3 3/4] drivers: tee: sandbox: add rpc test ta emulation

[Simon Glass]

On Tue, 12 Jan 2021 at 01:44, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
>
> This adds support for RPC test trusted application emulation, which
> permits to test reverse RPC calls to TEE supplicant. Currently it covers
> requests to the I2C bus from TEE.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
>  drivers/tee/Makefile            |   2 +
>  drivers/tee/optee/Kconfig       |   9 +++
>  drivers/tee/sandbox.c           | 137 +++++++++++++++++++++++++++++++-
>  include/tee/optee_ta_rpc_test.h |  28 +++++++
>  4 files changed, 172 insertions(+), 4 deletions(-)
>  create mode 100644 include/tee/optee_ta_rpc_test.h

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCHv3 3/4] drivers: tee: sandbox: add rpc test ta emulation

[Jens Wiklander]

On Tue, Jan 12, 2021 at 09:43:39AM +0100, Jorge Ramirez-Ortiz wrote:
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
> 
> This adds support for RPC test trusted application emulation, which
> permits to test reverse RPC calls to TEE supplicant. Currently it covers
> requests to the I2C bus from TEE.
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
>  drivers/tee/Makefile            |   2 +
>  drivers/tee/optee/Kconfig       |   9 +++
>  drivers/tee/sandbox.c           | 137 +++++++++++++++++++++++++++++++-
>  include/tee/optee_ta_rpc_test.h |  28 +++++++
>  4 files changed, 172 insertions(+), 4 deletions(-)
>  create mode 100644 include/tee/optee_ta_rpc_test.h
> 
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> index 5c8ffdbce8..ff844195ae 100644
> --- a/drivers/tee/Makefile
> +++ b/drivers/tee/Makefile
> @@ -2,5 +2,7 @@
>  
>  obj-y += tee-uclass.o
>  obj-$(CONFIG_SANDBOX) += sandbox.o
> +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/supplicant.o
> +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/i2c.o
>  obj-$(CONFIG_OPTEE) += optee/
>  obj-y += broadcom/
> diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> index d489834df9..65622f30b1 100644
> --- a/drivers/tee/optee/Kconfig
> +++ b/drivers/tee/optee/Kconfig
> @@ -22,6 +22,15 @@ config OPTEE_TA_AVB
>  	  The TA can support the "avb" subcommands "read_rb", "write"rb"
>  	  and "is_unlocked".
>  
> +config OPTEE_TA_RPC_TEST
> +	bool "Support RPC TEST TA"
> +	depends on SANDBOX_TEE
> +	default y
> +	help
> +	  Enables support for RPC test trusted application emulation, which
> +	  permits to test reverse RPC calls to TEE supplicant. Should
> +	  be used only in sandbox env.
> +
>  endmenu
>  
>  endif
> diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
> index 4b91e7db1b..1cacd443f4 100644
> --- a/drivers/tee/sandbox.c
> +++ b/drivers/tee/sandbox.c
> @@ -7,11 +7,15 @@
>  #include <sandboxtee.h>
>  #include <tee.h>
>  #include <tee/optee_ta_avb.h>
> +#include <tee/optee_ta_rpc_test.h>
> +
> +#include "optee/optee_msg.h"
> +#include "optee/optee_private.h"
>  
>  /*
>   * The sandbox tee driver tries to emulate a generic Trusted Exectution
> - * Environment (TEE) with the Trusted Application (TA) OPTEE_TA_AVB
> - * available.
> + * Environment (TEE) with the Trusted Applications (TA) OPTEE_TA_AVB and
> + * OPTEE_TA_RPC_TEST available.
>   */
>  
>  static const u32 pstorage_max = 16;
> @@ -32,7 +36,32 @@ struct ta_entry {
>  			   struct tee_param *params);
>  };
>  
> -#ifdef CONFIG_OPTEE_TA_AVB
> +static int get_msg_arg(struct udevice *dev, uint num_params,
> +		       struct tee_shm **shmp, struct optee_msg_arg **msg_arg)
> +{
> +	int rc;
> +	struct optee_msg_arg *ma;
> +
> +	rc = __tee_shm_add(dev, OPTEE_MSG_NONCONTIG_PAGE_SIZE, NULL,
> +			   OPTEE_MSG_GET_ARG_SIZE(num_params), TEE_SHM_ALLOC,
> +			   shmp);
> +	if (rc)
> +		return rc;
> +
> +	ma = (*shmp)->addr;
> +	memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
> +	ma->num_params = num_params;
> +	*msg_arg = ma;
> +
> +	return 0;
> +}
> +
> +inline void *optee_alloc_and_init_page_list(void *buf, ulong len,
> +					   u64 *phys_buf_ptr)

Why "inline"?

> +{
> +	return 0;

How can this work? I'd expect an eventual caller to get a bit
disappointed. By the way aren't we usually using NULL for pointers in
U-Boot?

> +}
> +
>  static u32 get_attr(uint n, uint num_params, struct tee_param *params)
>  {
>  	if (n >= num_params)
> @@ -63,6 +92,7 @@ bad_params:
>  	return TEE_ERROR_BAD_PARAMETERS;
>  }
>  
> +#ifdef CONFIG_OPTEE_TA_AVB
>  static u32 ta_avb_open_session(struct udevice *dev, uint num_params,
>  			       struct tee_param *params)
>  {
> @@ -214,7 +244,100 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
>  		return TEE_ERROR_NOT_SUPPORTED;
>  	}
>  }
> -#endif /*OPTEE_TA_AVB*/
> +#endif /* OPTEE_TA_AVB */
> +
> +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> +static u32 ta_rpc_test_open_session(struct udevice *dev, uint num_params,
> +				    struct tee_param *params)
> +{
> +	/*
> +	 * We don't expect additional parameters when opening a session to
> +	 * this TA.
> +	 */
> +	return check_params(TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> +			    TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> +			    num_params, params);
> +}
> +
> +static void fill_i2c_rpc_params(struct optee_msg_arg *msg_arg, u64 bus_num,
> +				u64 chip_addr, u64 op,
> +				struct tee_param_memref memref)
> +{
> +	msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> +	msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> +	msg_arg->params[2].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INOUT;
> +	msg_arg->params[3].attr = OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT;
> +
> +	/* trigger I2C services of TEE supplicant */
> +	msg_arg->cmd = OPTEE_MSG_RPC_CMD_I2C_TRANSFER;
> +
> +	msg_arg->params[0].u.value.a = op;
> +	msg_arg->params[0].u.value.b = bus_num;
> +	msg_arg->params[0].u.value.c = chip_addr;
> +
> +	/* buffer to read/write data */
> +	msg_arg->params[2].u.rmem.shm_ref = (ulong)memref.shm;
> +	msg_arg->params[2].u.rmem.size = memref.size;
> +	msg_arg->params[2].u.rmem.offs = memref.shm_offs;
> +
> +	msg_arg->num_params = 4;
> +}
> +
> +static u32 ta_rpc_test_invoke_func(struct udevice *dev, u32 func,
> +				   uint num_params,
> +				   struct tee_param *params)
> +{
> +	struct tee_shm *shm;
> +	struct tee_param_memref memref_data;
> +	struct optee_msg_arg *msg_arg;
> +	int chip_addr, bus_num, op;
> +	int res;
> +
> +	res = check_params(TEE_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			   TEE_PARAM_ATTR_TYPE_MEMREF_INOUT,
> +			   TEE_PARAM_ATTR_TYPE_NONE,
> +			   TEE_PARAM_ATTR_TYPE_NONE,
> +			   num_params, params);
> +	if (res)
> +		return TEE_ERROR_BAD_PARAMETERS;
> +
> +	bus_num = params[0].u.value.a;
> +	chip_addr = params[0].u.value.b;
> +	memref_data = params[1].u.memref;
> +
> +	switch (func) {
> +	case TA_RPC_TEST_CMD_I2C_READ:
> +		op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD;
> +		break;
> +	case TA_RPC_TEST_CMD_I2C_WRITE:
> +		op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR;
> +		break;
> +	default:
> +		return TEE_ERROR_NOT_SUPPORTED;
> +	}
> +
> +	/*
> +	 * Fill params for an RPC call to tee supplicant
> +	 */
> +	res = get_msg_arg(dev, 4, &shm, &msg_arg);
> +	if (res)
> +		goto bad;
> +
> +	fill_i2c_rpc_params(msg_arg, bus_num, chip_addr, op, memref_data);
> +
> +	/* Make an RPC call to tee supplicant */
> +	optee_suppl_cmd(dev, shm, 0);
> +	res = msg_arg->ret;
> +
> +bad:
> +	tee_shm_free(shm);
> +
> +	if (res)
> +		return res;
> +
> +	return TEE_SUCCESS;
> +}
> +#endif /* CONFIG_OPTEE_TA_RPC_TEST */
>  
>  static const struct ta_entry ta_entries[] = {
>  #ifdef CONFIG_OPTEE_TA_AVB
> @@ -223,6 +346,12 @@ static const struct ta_entry ta_entries[] = {
>  	  .invoke_func = ta_avb_invoke_func,
>  	},
>  #endif
> +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> +	{ .uuid = TA_RPC_TEST_UUID,
> +	  .open_session = ta_rpc_test_open_session,
> +	  .invoke_func = ta_rpc_test_invoke_func,
> +	},
> +#endif
>  };
>  
>  static void sandbox_tee_get_version(struct udevice *dev,
> diff --git a/include/tee/optee_ta_rpc_test.h b/include/tee/optee_ta_rpc_test.h
> new file mode 100644
> index 0000000000..cae2fb04b4
> --- /dev/null
> +++ b/include/tee/optee_ta_rpc_test.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/* Copyright (c) 2020 Foundries Ltd */
> +
> +#ifndef __TA_RPC_TEST_H
> +#define __TA_RPC_TEST_H
> +
> +#define TA_RPC_TEST_UUID { 0x48420575, 0x96ca, 0x401a, \
> +		      { 0x89, 0x91, 0x1e, 0xfd, 0xce, 0xbd, 0x7d, 0x04 } }
> +
> +/*
> + * Does a reverse RPC call for I2C read
> + *
> + * in		params[0].value.a:	bus number
> + * in		params[0].value.b:	chip address
> + * inout	params[1].u.memref:	buffer to read data
> + */
> +#define TA_RPC_TEST_CMD_I2C_READ	0
> +
> +/*
> + * Does a reverse RPC call for I2C write
> + *
> + * in		params[0].value.a:	bus number
> + * in		params[0].value.b:	chip address
> + * inout	params[1].u.memref:	buffer with data to write
> + */
> +#define TA_RPC_TEST_CMD_I2C_WRITE	1
> +
> +#endif /* __TA_RPC_TEST_H */
> -- 
> 2.17.1
> 

[PATCHv3 3/4] drivers: tee: sandbox: add rpc test ta emulation

[Igor Opaniuk]

Hi Jens,

On Wed, Jan 20, 2021 at 10:49 AM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> On Tue, Jan 12, 2021 at 09:43:39AM +0100, Jorge Ramirez-Ortiz wrote:
> > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> >
> > This adds support for RPC test trusted application emulation, which
> > permits to test reverse RPC calls to TEE supplicant. Currently it covers
> > requests to the I2C bus from TEE.
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > ---
> >  drivers/tee/Makefile            |   2 +
> >  drivers/tee/optee/Kconfig       |   9 +++
> >  drivers/tee/sandbox.c           | 137 +++++++++++++++++++++++++++++++-
> >  include/tee/optee_ta_rpc_test.h |  28 +++++++
> >  4 files changed, 172 insertions(+), 4 deletions(-)
> >  create mode 100644 include/tee/optee_ta_rpc_test.h
> >
> > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > index 5c8ffdbce8..ff844195ae 100644
> > --- a/drivers/tee/Makefile
> > +++ b/drivers/tee/Makefile
> > @@ -2,5 +2,7 @@
> >
> >  obj-y += tee-uclass.o
> >  obj-$(CONFIG_SANDBOX) += sandbox.o
> > +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/supplicant.o
> > +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/i2c.o
> >  obj-$(CONFIG_OPTEE) += optee/
> >  obj-y += broadcom/
> > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > index d489834df9..65622f30b1 100644
> > --- a/drivers/tee/optee/Kconfig
> > +++ b/drivers/tee/optee/Kconfig
> > @@ -22,6 +22,15 @@ config OPTEE_TA_AVB
> >         The TA can support the "avb" subcommands "read_rb", "write"rb"
> >         and "is_unlocked".
> >
> > +config OPTEE_TA_RPC_TEST
> > +     bool "Support RPC TEST TA"
> > +     depends on SANDBOX_TEE
> > +     default y
> > +     help
> > +       Enables support for RPC test trusted application emulation, which
> > +       permits to test reverse RPC calls to TEE supplicant. Should
> > +       be used only in sandbox env.
> > +
> >  endmenu
> >
> >  endif
> > diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
> > index 4b91e7db1b..1cacd443f4 100644
> > --- a/drivers/tee/sandbox.c
> > +++ b/drivers/tee/sandbox.c
> > @@ -7,11 +7,15 @@
> >  #include <sandboxtee.h>
> >  #include <tee.h>
> >  #include <tee/optee_ta_avb.h>
> > +#include <tee/optee_ta_rpc_test.h>
> > +
> > +#include "optee/optee_msg.h"
> > +#include "optee/optee_private.h"
> >
> >  /*
> >   * The sandbox tee driver tries to emulate a generic Trusted Exectution
> > - * Environment (TEE) with the Trusted Application (TA) OPTEE_TA_AVB
> > - * available.
> > + * Environment (TEE) with the Trusted Applications (TA) OPTEE_TA_AVB and
> > + * OPTEE_TA_RPC_TEST available.
> >   */
> >
> >  static const u32 pstorage_max = 16;
> > @@ -32,7 +36,32 @@ struct ta_entry {
> >                          struct tee_param *params);
> >  };
> >
> > -#ifdef CONFIG_OPTEE_TA_AVB
> > +static int get_msg_arg(struct udevice *dev, uint num_params,
> > +                    struct tee_shm **shmp, struct optee_msg_arg **msg_arg)
> > +{
> > +     int rc;
> > +     struct optee_msg_arg *ma;
> > +
> > +     rc = __tee_shm_add(dev, OPTEE_MSG_NONCONTIG_PAGE_SIZE, NULL,
> > +                        OPTEE_MSG_GET_ARG_SIZE(num_params), TEE_SHM_ALLOC,
> > +                        shmp);
> > +     if (rc)
> > +             return rc;
> > +
> > +     ma = (*shmp)->addr;
> > +     memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
> > +     ma->num_params = num_params;
> > +     *msg_arg = ma;
> > +
> > +     return 0;
> > +}
> > +
> > +inline void *optee_alloc_and_init_page_list(void *buf, ulong len,
> > +                                        u64 *phys_buf_ptr)
>
> Why "inline"?
That should not be there, will fix in new patch series.

>
> > +{
> > +     return 0;
>
> How can this work? I'd expect an eventual caller to get a bit
> disappointed. By the way aren't we usually using NULL for pointers in
> U-Boot?

I didn't want to pull-in the full core.c of OP-TEE driver, so I kept a
simple stub for
optee_alloc_and_init_page_list() (taking into account that currently
it's not being called anyware)
to avoid compilation issues in supplicant.c . As you already said, if
someone decides to extend RPC
tests and test OPTEE_MSG_RPC_CMD_SHM_ALLOC etc - we will definitely
have a problem here (but why do that?).

As a suggestion I can put a simple panic() call in
optee_alloc_and_init_page_list() to cover that case.

Personally I don't like either the idea of testing OP-TEE supplicant
functionality from sandbox driver and
I believed from the very beginning that it's not a good idea at all,
as initial sandbox driver is supposed to be
used to test TEE Client API  and not to validate bits and pieces of
another driver (OP-TEE).

>
> > +}
> > +
> >  static u32 get_attr(uint n, uint num_params, struct tee_param *params)
> >  {
> >       if (n >= num_params)
> > @@ -63,6 +92,7 @@ bad_params:
> >       return TEE_ERROR_BAD_PARAMETERS;
> >  }
> >
> > +#ifdef CONFIG_OPTEE_TA_AVB
> >  static u32 ta_avb_open_session(struct udevice *dev, uint num_params,
> >                              struct tee_param *params)
> >  {
> > @@ -214,7 +244,100 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
> >               return TEE_ERROR_NOT_SUPPORTED;
> >       }
> >  }
> > -#endif /*OPTEE_TA_AVB*/
> > +#endif /* OPTEE_TA_AVB */
> > +
> > +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> > +static u32 ta_rpc_test_open_session(struct udevice *dev, uint num_params,
> > +                                 struct tee_param *params)
> > +{
> > +     /*
> > +      * We don't expect additional parameters when opening a session to
> > +      * this TA.
> > +      */
> > +     return check_params(TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> > +                         TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> > +                         num_params, params);
> > +}
> > +
> > +static void fill_i2c_rpc_params(struct optee_msg_arg *msg_arg, u64 bus_num,
> > +                             u64 chip_addr, u64 op,
> > +                             struct tee_param_memref memref)
> > +{
> > +     msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> > +     msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> > +     msg_arg->params[2].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INOUT;
> > +     msg_arg->params[3].attr = OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT;
> > +
> > +     /* trigger I2C services of TEE supplicant */
> > +     msg_arg->cmd = OPTEE_MSG_RPC_CMD_I2C_TRANSFER;
> > +
> > +     msg_arg->params[0].u.value.a = op;
> > +     msg_arg->params[0].u.value.b = bus_num;
> > +     msg_arg->params[0].u.value.c = chip_addr;
> > +
> > +     /* buffer to read/write data */
> > +     msg_arg->params[2].u.rmem.shm_ref = (ulong)memref.shm;
> > +     msg_arg->params[2].u.rmem.size = memref.size;
> > +     msg_arg->params[2].u.rmem.offs = memref.shm_offs;
> > +
> > +     msg_arg->num_params = 4;
> > +}
> > +
> > +static u32 ta_rpc_test_invoke_func(struct udevice *dev, u32 func,
> > +                                uint num_params,
> > +                                struct tee_param *params)
> > +{
> > +     struct tee_shm *shm;
> > +     struct tee_param_memref memref_data;
> > +     struct optee_msg_arg *msg_arg;
> > +     int chip_addr, bus_num, op;
> > +     int res;
> > +
> > +     res = check_params(TEE_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +                        TEE_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > +                        TEE_PARAM_ATTR_TYPE_NONE,
> > +                        TEE_PARAM_ATTR_TYPE_NONE,
> > +                        num_params, params);
> > +     if (res)
> > +             return TEE_ERROR_BAD_PARAMETERS;
> > +
> > +     bus_num = params[0].u.value.a;
> > +     chip_addr = params[0].u.value.b;
> > +     memref_data = params[1].u.memref;
> > +
> > +     switch (func) {
> > +     case TA_RPC_TEST_CMD_I2C_READ:
> > +             op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD;
> > +             break;
> > +     case TA_RPC_TEST_CMD_I2C_WRITE:
> > +             op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR;
> > +             break;
> > +     default:
> > +             return TEE_ERROR_NOT_SUPPORTED;
> > +     }
> > +
> > +     /*
> > +      * Fill params for an RPC call to tee supplicant
> > +      */
> > +     res = get_msg_arg(dev, 4, &shm, &msg_arg);
> > +     if (res)
> > +             goto bad;
> > +
> > +     fill_i2c_rpc_params(msg_arg, bus_num, chip_addr, op, memref_data);
> > +
> > +     /* Make an RPC call to tee supplicant */
> > +     optee_suppl_cmd(dev, shm, 0);
> > +     res = msg_arg->ret;
> > +
> > +bad:
> > +     tee_shm_free(shm);
> > +
> > +     if (res)
> > +             return res;
> > +
> > +     return TEE_SUCCESS;
> > +}
> > +#endif /* CONFIG_OPTEE_TA_RPC_TEST */
> >
> >  static const struct ta_entry ta_entries[] = {
> >  #ifdef CONFIG_OPTEE_TA_AVB
> > @@ -223,6 +346,12 @@ static const struct ta_entry ta_entries[] = {
> >         .invoke_func = ta_avb_invoke_func,
> >       },
> >  #endif
> > +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> > +     { .uuid = TA_RPC_TEST_UUID,
> > +       .open_session = ta_rpc_test_open_session,
> > +       .invoke_func = ta_rpc_test_invoke_func,
> > +     },
> > +#endif
> >  };
> >
> >  static void sandbox_tee_get_version(struct udevice *dev,
> > diff --git a/include/tee/optee_ta_rpc_test.h b/include/tee/optee_ta_rpc_test.h
> > new file mode 100644
> > index 0000000000..cae2fb04b4
> > --- /dev/null
> > +++ b/include/tee/optee_ta_rpc_test.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/* Copyright (c) 2020 Foundries Ltd */
> > +
> > +#ifndef __TA_RPC_TEST_H
> > +#define __TA_RPC_TEST_H
> > +
> > +#define TA_RPC_TEST_UUID { 0x48420575, 0x96ca, 0x401a, \
> > +                   { 0x89, 0x91, 0x1e, 0xfd, 0xce, 0xbd, 0x7d, 0x04 } }
> > +
> > +/*
> > + * Does a reverse RPC call for I2C read
> > + *
> > + * in                params[0].value.a:      bus number
> > + * in                params[0].value.b:      chip address
> > + * inout     params[1].u.memref:     buffer to read data
> > + */
> > +#define TA_RPC_TEST_CMD_I2C_READ     0
> > +
> > +/*
> > + * Does a reverse RPC call for I2C write
> > + *
> > + * in                params[0].value.a:      bus number
> > + * in                params[0].value.b:      chip address
> > + * inout     params[1].u.memref:     buffer with data to write
> > + */
> > +#define TA_RPC_TEST_CMD_I2C_WRITE    1
> > +
> > +#endif /* __TA_RPC_TEST_H */
> > --
> > 2.17.1
> >



-- 
Best regards - Freundliche Gr?sse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk at foundries.io
W: www.foundries.io

[PATCHv3 3/4] drivers: tee: sandbox: add rpc test ta emulation

[Jens Wiklander]

Hi Igor,

On Wed, Jan 20, 2021 at 11:39:35AM +0200, Igor Opaniuk wrote:
> Hi Jens,
> 
> On Wed, Jan 20, 2021 at 10:49 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > On Tue, Jan 12, 2021 at 09:43:39AM +0100, Jorge Ramirez-Ortiz wrote:
> > > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> > >
> > > This adds support for RPC test trusted application emulation, which
> > > permits to test reverse RPC calls to TEE supplicant. Currently it covers
> > > requests to the I2C bus from TEE.
> > >
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > > ---
> > >  drivers/tee/Makefile            |   2 +
> > >  drivers/tee/optee/Kconfig       |   9 +++
> > >  drivers/tee/sandbox.c           | 137 +++++++++++++++++++++++++++++++-
> > >  include/tee/optee_ta_rpc_test.h |  28 +++++++
> > >  4 files changed, 172 insertions(+), 4 deletions(-)
> > >  create mode 100644 include/tee/optee_ta_rpc_test.h
> > >
> > > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > > index 5c8ffdbce8..ff844195ae 100644
> > > --- a/drivers/tee/Makefile
> > > +++ b/drivers/tee/Makefile
> > > @@ -2,5 +2,7 @@
> > >
> > >  obj-y += tee-uclass.o
> > >  obj-$(CONFIG_SANDBOX) += sandbox.o
> > > +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/supplicant.o
> > > +obj-$(CONFIG_OPTEE_TA_RPC_TEST) += optee/i2c.o
> > >  obj-$(CONFIG_OPTEE) += optee/
> > >  obj-y += broadcom/
> > > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > > index d489834df9..65622f30b1 100644
> > > --- a/drivers/tee/optee/Kconfig
> > > +++ b/drivers/tee/optee/Kconfig
> > > @@ -22,6 +22,15 @@ config OPTEE_TA_AVB
> > >         The TA can support the "avb" subcommands "read_rb", "write"rb"
> > >         and "is_unlocked".
> > >
> > > +config OPTEE_TA_RPC_TEST
> > > +     bool "Support RPC TEST TA"
> > > +     depends on SANDBOX_TEE
> > > +     default y
> > > +     help
> > > +       Enables support for RPC test trusted application emulation, which
> > > +       permits to test reverse RPC calls to TEE supplicant. Should
> > > +       be used only in sandbox env.
> > > +
> > >  endmenu
> > >
> > >  endif
> > > diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
> > > index 4b91e7db1b..1cacd443f4 100644
> > > --- a/drivers/tee/sandbox.c
> > > +++ b/drivers/tee/sandbox.c
> > > @@ -7,11 +7,15 @@
> > >  #include <sandboxtee.h>
> > >  #include <tee.h>
> > >  #include <tee/optee_ta_avb.h>
> > > +#include <tee/optee_ta_rpc_test.h>
> > > +
> > > +#include "optee/optee_msg.h"
> > > +#include "optee/optee_private.h"
> > >
> > >  /*
> > >   * The sandbox tee driver tries to emulate a generic Trusted Exectution
> > > - * Environment (TEE) with the Trusted Application (TA) OPTEE_TA_AVB
> > > - * available.
> > > + * Environment (TEE) with the Trusted Applications (TA) OPTEE_TA_AVB and
> > > + * OPTEE_TA_RPC_TEST available.
> > >   */
> > >
> > >  static const u32 pstorage_max = 16;
> > > @@ -32,7 +36,32 @@ struct ta_entry {
> > >                          struct tee_param *params);
> > >  };
> > >
> > > -#ifdef CONFIG_OPTEE_TA_AVB
> > > +static int get_msg_arg(struct udevice *dev, uint num_params,
> > > +                    struct tee_shm **shmp, struct optee_msg_arg **msg_arg)
> > > +{
> > > +     int rc;
> > > +     struct optee_msg_arg *ma;
> > > +
> > > +     rc = __tee_shm_add(dev, OPTEE_MSG_NONCONTIG_PAGE_SIZE, NULL,
> > > +                        OPTEE_MSG_GET_ARG_SIZE(num_params), TEE_SHM_ALLOC,
> > > +                        shmp);
> > > +     if (rc)
> > > +             return rc;
> > > +
> > > +     ma = (*shmp)->addr;
> > > +     memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
> > > +     ma->num_params = num_params;
> > > +     *msg_arg = ma;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +inline void *optee_alloc_and_init_page_list(void *buf, ulong len,
> > > +                                        u64 *phys_buf_ptr)
> >
> > Why "inline"?
> That should not be there, will fix in new patch series.
> 
> >
> > > +{
> > > +     return 0;
> >
> > How can this work? I'd expect an eventual caller to get a bit
> > disappointed. By the way aren't we usually using NULL for pointers in
> > U-Boot?
> 
> I didn't want to pull-in the full core.c of OP-TEE driver, so I kept a
> simple stub for
> optee_alloc_and_init_page_list() (taking into account that currently
> it's not being called anyware)
> to avoid compilation issues in supplicant.c . As you already said, if
> someone decides to extend RPC
> tests and test OPTEE_MSG_RPC_CMD_SHM_ALLOC etc - we will definitely
> have a problem here (but why do that?).
> 
> As a suggestion I can put a simple panic() call in
> optee_alloc_and_init_page_list() to cover that case.

Maybe just a comment noting that this function isn't supposed to be
called from the test code but needed for linking to succeed. Reporting
an error in the test suite rather than aborting the whole thing seems
more friendly.

Cheers,
Jens

> 
> Personally I don't like either the idea of testing OP-TEE supplicant
> functionality from sandbox driver and
> I believed from the very beginning that it's not a good idea at all,
> as initial sandbox driver is supposed to be
> used to test TEE Client API  and not to validate bits and pieces of
> another driver (OP-TEE).
> 
> >
> > > +}
> > > +
> > >  static u32 get_attr(uint n, uint num_params, struct tee_param *params)
> > >  {
> > >       if (n >= num_params)
> > > @@ -63,6 +92,7 @@ bad_params:
> > >       return TEE_ERROR_BAD_PARAMETERS;
> > >  }
> > >
> > > +#ifdef CONFIG_OPTEE_TA_AVB
> > >  static u32 ta_avb_open_session(struct udevice *dev, uint num_params,
> > >                              struct tee_param *params)
> > >  {
> > > @@ -214,7 +244,100 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
> > >               return TEE_ERROR_NOT_SUPPORTED;
> > >       }
> > >  }
> > > -#endif /*OPTEE_TA_AVB*/
> > > +#endif /* OPTEE_TA_AVB */
> > > +
> > > +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> > > +static u32 ta_rpc_test_open_session(struct udevice *dev, uint num_params,
> > > +                                 struct tee_param *params)
> > > +{
> > > +     /*
> > > +      * We don't expect additional parameters when opening a session to
> > > +      * this TA.
> > > +      */
> > > +     return check_params(TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> > > +                         TEE_PARAM_ATTR_TYPE_NONE, TEE_PARAM_ATTR_TYPE_NONE,
> > > +                         num_params, params);
> > > +}
> > > +
> > > +static void fill_i2c_rpc_params(struct optee_msg_arg *msg_arg, u64 bus_num,
> > > +                             u64 chip_addr, u64 op,
> > > +                             struct tee_param_memref memref)
> > > +{
> > > +     msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> > > +     msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> > > +     msg_arg->params[2].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INOUT;
> > > +     msg_arg->params[3].attr = OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT;
> > > +
> > > +     /* trigger I2C services of TEE supplicant */
> > > +     msg_arg->cmd = OPTEE_MSG_RPC_CMD_I2C_TRANSFER;
> > > +
> > > +     msg_arg->params[0].u.value.a = op;
> > > +     msg_arg->params[0].u.value.b = bus_num;
> > > +     msg_arg->params[0].u.value.c = chip_addr;
> > > +
> > > +     /* buffer to read/write data */
> > > +     msg_arg->params[2].u.rmem.shm_ref = (ulong)memref.shm;
> > > +     msg_arg->params[2].u.rmem.size = memref.size;
> > > +     msg_arg->params[2].u.rmem.offs = memref.shm_offs;
> > > +
> > > +     msg_arg->num_params = 4;
> > > +}
> > > +
> > > +static u32 ta_rpc_test_invoke_func(struct udevice *dev, u32 func,
> > > +                                uint num_params,
> > > +                                struct tee_param *params)
> > > +{
> > > +     struct tee_shm *shm;
> > > +     struct tee_param_memref memref_data;
> > > +     struct optee_msg_arg *msg_arg;
> > > +     int chip_addr, bus_num, op;
> > > +     int res;
> > > +
> > > +     res = check_params(TEE_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > +                        TEE_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > > +                        TEE_PARAM_ATTR_TYPE_NONE,
> > > +                        TEE_PARAM_ATTR_TYPE_NONE,
> > > +                        num_params, params);
> > > +     if (res)
> > > +             return TEE_ERROR_BAD_PARAMETERS;
> > > +
> > > +     bus_num = params[0].u.value.a;
> > > +     chip_addr = params[0].u.value.b;
> > > +     memref_data = params[1].u.memref;
> > > +
> > > +     switch (func) {
> > > +     case TA_RPC_TEST_CMD_I2C_READ:
> > > +             op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD;
> > > +             break;
> > > +     case TA_RPC_TEST_CMD_I2C_WRITE:
> > > +             op = OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR;
> > > +             break;
> > > +     default:
> > > +             return TEE_ERROR_NOT_SUPPORTED;
> > > +     }
> > > +
> > > +     /*
> > > +      * Fill params for an RPC call to tee supplicant
> > > +      */
> > > +     res = get_msg_arg(dev, 4, &shm, &msg_arg);
> > > +     if (res)
> > > +             goto bad;
> > > +
> > > +     fill_i2c_rpc_params(msg_arg, bus_num, chip_addr, op, memref_data);
> > > +
> > > +     /* Make an RPC call to tee supplicant */
> > > +     optee_suppl_cmd(dev, shm, 0);
> > > +     res = msg_arg->ret;
> > > +
> > > +bad:
> > > +     tee_shm_free(shm);
> > > +
> > > +     if (res)
> > > +             return res;
> > > +
> > > +     return TEE_SUCCESS;
> > > +}
> > > +#endif /* CONFIG_OPTEE_TA_RPC_TEST */
> > >
> > >  static const struct ta_entry ta_entries[] = {
> > >  #ifdef CONFIG_OPTEE_TA_AVB
> > > @@ -223,6 +346,12 @@ static const struct ta_entry ta_entries[] = {
> > >         .invoke_func = ta_avb_invoke_func,
> > >       },
> > >  #endif
> > > +#ifdef CONFIG_OPTEE_TA_RPC_TEST
> > > +     { .uuid = TA_RPC_TEST_UUID,
> > > +       .open_session = ta_rpc_test_open_session,
> > > +       .invoke_func = ta_rpc_test_invoke_func,
> > > +     },
> > > +#endif
> > >  };
> > >
> > >  static void sandbox_tee_get_version(struct udevice *dev,
> > > diff --git a/include/tee/optee_ta_rpc_test.h b/include/tee/optee_ta_rpc_test.h
> > > new file mode 100644
> > > index 0000000000..cae2fb04b4
> > > --- /dev/null
> > > +++ b/include/tee/optee_ta_rpc_test.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/* Copyright (c) 2020 Foundries Ltd */
> > > +
> > > +#ifndef __TA_RPC_TEST_H
> > > +#define __TA_RPC_TEST_H
> > > +
> > > +#define TA_RPC_TEST_UUID { 0x48420575, 0x96ca, 0x401a, \
> > > +                   { 0x89, 0x91, 0x1e, 0xfd, 0xce, 0xbd, 0x7d, 0x04 } }
> > > +
> > > +/*
> > > + * Does a reverse RPC call for I2C read
> > > + *
> > > + * in                params[0].value.a:      bus number
> > > + * in                params[0].value.b:      chip address
> > > + * inout     params[1].u.memref:     buffer to read data
> > > + */
> > > +#define TA_RPC_TEST_CMD_I2C_READ     0
> > > +
> > > +/*
> > > + * Does a reverse RPC call for I2C write
> > > + *
> > > + * in                params[0].value.a:      bus number
> > > + * in                params[0].value.b:      chip address
> > > + * inout     params[1].u.memref:     buffer with data to write
> > > + */
> > > +#define TA_RPC_TEST_CMD_I2C_WRITE    1
> > > +
> > > +#endif /* __TA_RPC_TEST_H */
> > > --
> > > 2.17.1
> > >
> 
> 
> 
> -- 
> Best regards - Freundliche Gr?sse - Meilleures salutations
> 
> Igor Opaniuk
> Embedded Software Engineer
> T:  +380 938364067
> E: igor.opaniuk at foundries.io
> W: www.foundries.io

[PATCHv3 4/4] test: dm: tee: extend with RPC test

[Jorge Ramirez-Ortiz]

From: Igor Opaniuk <igor.opaniuk@foundries.io>

Extend existing DM tee tests adding test coverage for reverse RPC calls.
Currently this commit only adds tests for I2C requests from TEE driver
to TEE supplicant, for instance reading/writing data to emulated i2c
eeprom defines in standard sandbox test device tree
(arch/sandbox/dts/test.dtb):

=> i2c bus
Bus 0:	i2c at 0  (active 0)
   2c: eeprom at 2c, offset len 1, flags 0
   ...

Running TEE tests:
=> ut dm tee
Test: dm_test_tee: tee.c
Test: dm_test_tee: tee.c (flat tree)
Failures: 0

Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
---
 test/dm/tee.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 104 insertions(+), 5 deletions(-)

diff --git a/test/dm/tee.c b/test/dm/tee.c
index ddbdcfb0cf..2cb54c68fb 100644
--- a/test/dm/tee.c
+++ b/test/dm/tee.c
@@ -13,11 +13,12 @@
 #include <test/test.h>
 #include <test/ut.h>
 #include <tee/optee_ta_avb.h>
+#include <tee/optee_ta_rpc_test.h>
 
-static int open_session(struct udevice *dev, u32 *session)
+static int open_session(struct udevice *dev, u32 *session,
+			struct tee_optee_ta_uuid uuid)
 {
 	struct tee_open_session_arg arg;
-	const struct tee_optee_ta_uuid uuid = TA_AVB_UUID;
 	int rc;
 
 	memset(&arg, 0, sizeof(arg));
@@ -32,7 +33,7 @@ static int open_session(struct udevice *dev, u32 *session)
 	return 0;
 }
 
-static int invoke_func(struct udevice *dev, u32 session)
+static int invoke_func_avb(struct udevice *dev, u32 session)
 {
 	struct tee_param param = { .attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT };
 	struct tee_invoke_arg arg;
@@ -47,6 +48,47 @@ static int invoke_func(struct udevice *dev, u32 session)
 	return 0;
 }
 
+static int invoke_func_rpc_test(struct udevice *dev, u32 session,
+				u64 op, u64 busnum, u64 chip_addr,
+				u8 *buf, size_t buf_size)
+{
+	struct tee_param param[2];
+	struct tee_invoke_arg arg;
+	struct tee_shm *shm_buf;
+	int rc;
+
+	memset(&arg, 0, sizeof(arg));
+	arg.session = session;
+	arg.func = op;
+
+	rc = tee_shm_alloc(dev, buf_size,
+			   TEE_SHM_ALLOC, &shm_buf);
+	if (rc)
+		return rc;
+
+	if (op == TA_RPC_TEST_CMD_I2C_WRITE)
+		memcpy(shm_buf->addr, buf, buf_size);
+
+	memset(param, 0, sizeof(param));
+	param[0].attr = TEE_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = busnum;
+	param[0].u.value.b = chip_addr;
+	param[1].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
+	param[1].u.memref.shm = shm_buf;
+	param[1].u.memref.size = buf_size;
+
+	if (tee_invoke_func(dev, &arg, 2, param) || arg.ret) {
+		goto out;
+		rc = -1;
+	}
+
+	if (op == TA_RPC_TEST_CMD_I2C_READ)
+		memcpy(buf, shm_buf->addr, buf_size);
+out:
+	tee_shm_free(shm_buf);
+	return rc;
+}
+
 static int match(struct tee_version_data *vers, const void *data)
 {
 	return vers->gen_caps & TEE_GEN_CAP_GP;
@@ -62,6 +104,7 @@ static int test_tee(struct unit_test_state *uts, struct test_tee_vars *vars)
 	struct tee_version_data vers;
 	struct udevice *dev;
 	struct sandbox_tee_state *state;
+	struct tee_optee_ta_uuid avb_uuid = TA_AVB_UUID;
 	u32 session = 0;
 	int rc;
 	u8 data[128];
@@ -71,11 +114,11 @@ static int test_tee(struct unit_test_state *uts, struct test_tee_vars *vars)
 	state = dev_get_priv(dev);
 	ut_assert(!state->session);
 
-	rc = open_session(dev, &session);
+	rc = open_session(dev, &session, avb_uuid);
 	ut_assert(!rc);
 	ut_assert(session == state->session);
 
-	rc = invoke_func(dev, session);
+	rc = invoke_func_avb(dev, session);
 	ut_assert(!rc);
 
 	rc = tee_close_session(dev, session);
@@ -103,11 +146,67 @@ static int test_tee(struct unit_test_state *uts, struct test_tee_vars *vars)
 	return 0;
 }
 
+#ifdef CONFIG_OPTEE_TA_RPC_TEST
+#define BUF_SIZE 64
+static int test_tee_rpc(struct unit_test_state *uts)
+{
+	struct tee_version_data vers;
+	struct udevice *dev;
+	struct sandbox_tee_state *state;
+	struct tee_optee_ta_uuid rpc_test_uuid = TA_RPC_TEST_UUID;
+	u32 session = 0;
+	int rc;
+
+	char *test_str = "Test string";
+	u8 data[BUF_SIZE] = {0};
+	u8 data_from_eeprom[BUF_SIZE] = {0};
+
+	/* Use sandbox I2C EEPROM emulation; bus: 0, chip: 0x2c */
+	u64 bus = 0;
+	u64 chip = 0x2c;
+
+	dev = tee_find_device(NULL, match, NULL, &vers);
+	ut_assert(dev);
+	state = dev_get_priv(dev);
+	ut_assert(!state->session);
+
+	/* Test RPC call asking for I2C sevice */
+	rc = open_session(dev, &session, rpc_test_uuid);
+	ut_assert(!rc);
+	ut_assert(session == state->session);
+
+	/* Write buffer */
+	strncpy((char *)data, test_str, strlen(test_str));
+	rc = invoke_func_rpc_test(dev, session, TA_RPC_TEST_CMD_I2C_WRITE,
+				  bus, chip, data, sizeof(data));
+	ut_assert(!rc);
+
+	/* Read buffer */
+	rc = invoke_func_rpc_test(dev, session, TA_RPC_TEST_CMD_I2C_READ,
+				  bus, chip, data_from_eeprom,
+				  sizeof(data_from_eeprom));
+	ut_assert(!rc);
+
+	/* Compare */
+	ut_assert(!memcmp(data, data_from_eeprom, BUF_SIZE));
+
+	rc = tee_close_session(dev, session);
+	ut_assert(!rc);
+	ut_assert(!state->session);
+
+	return 0;
+}
+#endif /* CONFIG_OPTEE_TA_RPC_TEST */
+
 static int dm_test_tee(struct unit_test_state *uts)
 {
 	struct test_tee_vars vars = { NULL, NULL };
 	int rc = test_tee(uts, &vars);
 
+#ifdef CONFIG_OPTEE_TA_RPC_TEST
+	rc = test_tee_rpc(uts);
+#endif /* CONFIG_OPTEE_TA_RPC_TEST */
+
 	/* In case test_tee() asserts these may still remain allocated */
 	tee_shm_free(vars.reg_shm);
 	tee_shm_free(vars.alloc_shm);
-- 
2.17.1

[PATCHv3 4/4] test: dm: tee: extend with RPC test

[Simon Glass]

On Tue, 12 Jan 2021 at 01:44, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
>
> Extend existing DM tee tests adding test coverage for reverse RPC calls.
> Currently this commit only adds tests for I2C requests from TEE driver
> to TEE supplicant, for instance reading/writing data to emulated i2c
> eeprom defines in standard sandbox test device tree
> (arch/sandbox/dts/test.dtb):
>
> => i2c bus
> Bus 0:  i2c at 0  (active 0)
>    2c: eeprom at 2c, offset len 1, flags 0
>    ...
>
> Running TEE tests:
> => ut dm tee
> Test: dm_test_tee: tee.c
> Test: dm_test_tee: tee.c (flat tree)
> Failures: 0
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
>  test/dm/tee.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 104 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

I wonder if the #ifdefs could go away, or at least be reduced. Perhaps
just assuming that the option is enabled on sandbox?

[PATCHv3 4/4] test: dm: tee: extend with RPC test

[Igor Opaniuk]

Hi Simon,

On Wed, Jan 13, 2021 at 6:12 PM Simon Glass <sjg@chromium.org> wrote:
>
> On Tue, 12 Jan 2021 at 01:44, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> >
> > Extend existing DM tee tests adding test coverage for reverse RPC calls.
> > Currently this commit only adds tests for I2C requests from TEE driver
> > to TEE supplicant, for instance reading/writing data to emulated i2c
> > eeprom defines in standard sandbox test device tree
> > (arch/sandbox/dts/test.dtb):
> >
> > => i2c bus
> > Bus 0:  i2c at 0  (active 0)
> >    2c: eeprom at 2c, offset len 1, flags 0
> >    ...
> >
> > Running TEE tests:
> > => ut dm tee
> > Test: dm_test_tee: tee.c
> > Test: dm_test_tee: tee.c (flat tree)
> > Failures: 0
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > ---
> >  test/dm/tee.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 104 insertions(+), 5 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I wonder if the #ifdefs could go away, or at least be reduced. Perhaps
> just assuming that the option is enabled on sandbox?

I've switched to "if (IS_ENABLED(CONFIG_*))" in test/dm/tee.c, however
for dropping all ifdefs in sandbox.c likely we have to do a complete overhaul
of tee sandbox driver structure, moving all TA specific code to separate files
(`sandbox-avb.c` etc), which IMHO is out of scope of the current patchset.
I'll address that in another patchset if you don't have objections.

Jorge will send another version of this patchset including my changes
in test/dm/tee.c.

-- 
Best regards - Freundliche Gr?sse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk at foundries.io
W: www.foundries.io

[PATCHv3 4/4] test: dm: tee: extend with RPC test

[Simon Glass]

Hi Igor,

On Thu, 14 Jan 2021 at 02:41, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> Hi Simon,
>
> On Wed, Jan 13, 2021 at 6:12 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > On Tue, 12 Jan 2021 at 01:44, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> > >
> > > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> > >
> > > Extend existing DM tee tests adding test coverage for reverse RPC calls.
> > > Currently this commit only adds tests for I2C requests from TEE driver
> > > to TEE supplicant, for instance reading/writing data to emulated i2c
> > > eeprom defines in standard sandbox test device tree
> > > (arch/sandbox/dts/test.dtb):
> > >
> > > => i2c bus
> > > Bus 0:  i2c at 0  (active 0)
> > >    2c: eeprom at 2c, offset len 1, flags 0
> > >    ...
> > >
> > > Running TEE tests:
> > > => ut dm tee
> > > Test: dm_test_tee: tee.c
> > > Test: dm_test_tee: tee.c (flat tree)
> > > Failures: 0
> > >
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > > ---
> > >  test/dm/tee.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 104 insertions(+), 5 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I wonder if the #ifdefs could go away, or at least be reduced. Perhaps
> > just assuming that the option is enabled on sandbox?
>
> I've switched to "if (IS_ENABLED(CONFIG_*))" in test/dm/tee.c, however
> for dropping all ifdefs in sandbox.c likely we have to do a complete overhaul
> of tee sandbox driver structure, moving all TA specific code to separate files
> (`sandbox-avb.c` etc), which IMHO is out of scope of the current patchset.
> I'll address that in another patchset if you don't have objections.

I don't see a great need for it as we do have #ifdefs in tests. But it
might work out nicely. See how you go.

>
> Jorge will send another version of this patchset including my changes
> in test/dm/tee.c.

OK.

Regards,
Simon