Re: [PATCH] drivers: tee: i2c trampoline driver

On 29/12/20, Simon Glass wrote:

Hi Jorge,

On Mon, 21 Dec 2020 at 11:15, 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 (tipically those would be secure elements like the NXP SE050).

Since this code is ported from linux it might be worth adding a link to the linux commit or patch.

sure, will do that.

...

Tested on imx8mmevk.

Signed-off-by: Jorge Ramirez-Ortiz jorge@foundries.io

drivers/tee/optee/Makefile | 1 + drivers/tee/optee/i2c.c | 88 ++++++++++++++++++++++++ drivers/tee/optee/optee_msg.h | 22 ++++++ drivers/tee/optee/optee_msg_supplicant.h | 5 ++ drivers/tee/optee/optee_private.h | 12 ++++ drivers/tee/optee/supplicant.c | 3 + 6 files changed, 131 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..2ebbf1ff7c --- /dev/null +++ b/drivers/tee/optee/i2c.c @@ -0,0 +1,88 @@ +// 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 struct {

comments on members, but see below

...

•   struct udevice *dev;
  

•   int chip;
  

•   int bus;
  

+} xfer;

How come this is not a local variable? Is it an optimisation? Does it make any difference in execution time? If so I think it would be better to drop it as state should be kept in driver rmodel. If you really want it, then perhaps just keep the dev, since you can use:

dev_seq(dev_get_parent(dev) - to get the bus number the device is on

struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);

then use chip->chip_addr to get the chip address

then store 'dev' in priv data in your dev (I think this is struct optee_private), the one passed to the function below:

I will remove this optimization instead. it is really not necessary for a few i2c transfers (I think it will obfuscate the code which I'd rather keep this trampoline code as simple as possible)

...

+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,

•                             struct optee_msg_arg *arg)
  

+{

•   const uint64_t 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 = NULL;
  

•   struct tee_shm *shm = NULL;
  

•   uint8_t *buf = NULL;
  

Shouldn't init vars that don't need to be

ok

...

•   size_t len = 0;
  

•   int chip = -1;
  

•   int bus = -1;
  

•   int ret = -1;
  

•   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]) {
  

•           arg->ret = TEE_ERROR_BAD_PARAMETERS;
  

•           return;
  

•   }
  

•   len = arg->params[2].u.tmem.size;
  

•   shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
  

•   buf = shm->addr;
  

•   if (!buf || !len)
  

•           goto bad;
  

•   bus = (int)arg->params[0].u.value.b;
  

•   chip = (int)arg->params[0].u.value.c;
  

•   if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
  

•           if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
  

•                   goto bad;
  

•           xfer.dev = chip_dev;
  

•           xfer.chip = chip;
  

•           xfer.bus = bus;
  

•   }
  

•   if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
  

•           if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
  

•                   goto bad;
  

Is this flag defined in the devicetree? If so we could read it in i2c_chip_ofdata_to_platdata() (which will be i2c_chip_of_to_plat() when the next merge window opens - see upstream/next).

It just seems odd that optee is controlling this, since presumably U-Boot knows about it?

U-boot should take care of having set this bit for the chip device. so I'll modify the code to just check for the flag was set since OP-TEE also knows about it. In case of mismatch, I'll report the error from u-boot and abort the transfer.

We can then discuss how u-boot sets this value and follow up with the necessary changes to the driver model if needed. Maybe the i2c maintainers want to comment on this as well.

...

•   switch (arg->params[0].u.value.a) {
  

•   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
  

•           ret = dm_i2c_read(xfer.dev, 0, buf, len);
  

•           break;
  

•   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
  

•           ret = dm_i2c_write(xfer.dev, 0, buf, len);
  

This code should run on sandbox and you can use a suitable i2c emulator (UCLASS_I2C_EMUL), only three at present) or write a new one. Then your test can arrange for sandbox to send an RPC (e.g. by calling a function directly in that driver to tell it to do that next time it has a chance), and your test can check that the i2c read/write happened.

does this mean you wont accept the commit without emulation code to go with it? is this a new rule? just trying to understand here.

This code is already tested on imx8mm, imx6ull and imx6ulz with the upstream/tip of OP-TEE

...

•           break;
  

•   default:
  

•           goto bad;
  

•   }
  

•   if (ret) {
  

•           arg->ret = TEE_ERROR_COMMUNICATION;
  

•   } else {
  

•           arg->params[3].u.value.a = len;
  

•           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..7cedb59a82 100644 --- a/drivers/tee/optee/optee_msg.h +++ b/drivers/tee/optee/optee_msg.h @@ -422,4 +422,26 @@ 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] param[1].u.value.b i2c retry (optional)
• • [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)

Jens, perhaps OPTEE_MSG_RPC_CMD_ could be renamed to OPTEE_RPC_ as this is way too long.

...

#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..d7ab1f593f 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev) } #endif

+#ifdef CONFIG_DM_I2C

We can probably assume DM_I2C is used for any recent boards, but I'd prefer to avoid cluttering up the header file when DM_I2C should be supported. See below.

I am sorry, I am not quite sure what you are asking. This just restrict the changes to DM_I2C. what do you propose we do?

...

+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,

•                             struct optee_msg_arg *arg);
  

Function comment please

...

+#else

Yuk, please don't do this..

sorry dont do what? isnt it just the same logic/pattern to what is already in the file? how should we handle this otherwise?

...

+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,

•                             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..f7738983cd 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,

It seems a comment in the header file was missed. Can you please add it while you are here?

sure

...

    case OPTEE_MSG_RPC_CMD_RPMB:
            optee_suppl_cmd_rpmb(dev, arg);
            break;

•   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
  

if (IS_ENABLED(DM_I2C))

...

•           optee_suppl_cmd_i2c_transfer(dev, arg);
  

or if permitted, make TEE depend on DM_I2C

...

•           break;
    default:
            arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
    }
  

-- 2.17.1

Regards, Simon