core: arm: tee to linux i2c trampoline driver #3905

ldts · 2020-05-31T12:07:25Z

core: arm: tee to linux i2c trampoline driver

Gives OP-TEE access to the i2c buses initialized and controlled by the
REE kernel. This is done by memory mapping a buffer from the thread's
cache where the input or output data is transferred.

Using this mechanism, OP-TEE clients do not have to worry about REE
RUNTIME_PM features switching off clocks from the controllers or
collisions with other bus masters.

This driver assumes that the I2C chip is on a REE statically assigned
bus which value is known to OP-TEE (it will not query/probe the REE).

jenswi-linaro · 2020-06-08T06:50:00Z

core/arch/arm/include/kernel/thread.h

@@ -596,6 +596,22 @@ struct mobj *thread_rpc_alloc_payload(size_t size);
 */
 void thread_rpc_free_payload(struct mobj *mobj);

+/**


I'd prefer this in a separate patch (in this PR in OK).

jenswi-linaro · 2020-06-08T06:52:20Z

core/include/optee_rpc_cmd.h

@@ -107,8 +107,7 @@
 */
 #define OPTEE_RPC_CMD_SHM_FREE		7

-/* Was OPTEE_RPC_CMD_SQL_FS, which isn't supported any longer */
-#define OPTEE_RPC_CMD_SQL_FS_RESERVED	8
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER	8


Please don't reuse this id, allocate a new.

jenswi-linaro · 2020-06-08T06:53:02Z

lib/libutee/include/tee_api_types.h

@@ -162,6 +162,21 @@ typedef struct {
 	uint32_t millis;
 } TEE_Time;

+/* I2C Api API */


Why in this file?

I assume that nothing of this is exposed to secure user space, so I'd suggest core/arch/arm/include/kernel/rpc_i2c.h.

By the way, tee_api_types.h only provides whats specified in GlobalPlatform Core Internal API.

jenswi-linaro · 2020-06-08T06:54:11Z

core/arch/arm/kernel/tee_i2c.c

+#define I2C_BUFFER_LENGTH 512
+static struct mobj *mobj;
+
+TEE_Result tee_i2c_transfer(struct TEE_I2CRequest *req, size_t *bytes)


I'd prefer a rpc_io prefix for function names and file names.

jenswi-linaro · 2020-06-08T06:54:54Z

core/arch/arm/kernel/tee_i2c.c

+		return TEE_ERROR_BAD_PARAMETERS;
+
+	if (req->bufferLen > I2C_BUFFER_LENGTH) {
+		EMSG("increase buffer size (%d)", (uint32_t)req->bufferLen);


Normally you'd print "buffer to large" or something like that.

jenswi-linaro · 2020-06-08T06:55:55Z

core/arch/arm/kernel/tee_i2c.c

+	if (!mobj) {
+		mobj = thread_rpc_alloc_kernel_payload(I2C_BUFFER_LENGTH);
+		if (!mobj) {
+			EMSG("increase linux CONFIG_OPTEE_SHM_NUM_PRIV_PAGES");


We are agnostic to the OS in normal world, why not just say that the kernel memory allocation failed?

I thought it would help others...took me a few hours to find out how this config was affecting the driver. maybe I just add a comment in the code? would it be ok?

jenswi-linaro · 2020-06-08T06:56:48Z

core/arch/arm/kernel/tee_i2c.c

+	if (req->mode == TEE_MODE_WRITE)
+		memcpy(mobj_get_va(mobj, 0), req->buffer, req->bufferLen);
+
+	struct thread_param p[] = {


Please declare variables at the start of a block (don't mix code and variables).

um ok. I followed what was being done in tee_time.c (seemed cleaner to me). but ok. will change.

ldts · 2020-06-15T07:51:49Z

thanks @jenswi-linaro ; sorry it took me a bit to respond but had to work on the i2c data encryption for the se050 comms (btw I hope to be able to send an initial PR sometime this week).

etienne-lms

Comments for commit "core: arm: allocate kernel payload"

etienne-lms · 2020-06-15T11:08:42Z

core/arch/arm/include/kernel/thread.h

+ * @mobj:	mobj that describes the buffer
+ */
+void thread_rpc_free_kernel_payload(struct mobj *mobj);
+


can remove the extra empty line.

etienne-lms · 2020-06-15T11:09:11Z

core/arch/arm/include/kernel/thread.h

@@ -605,6 +605,23 @@ struct mobj *thread_rpc_alloc_payload(size_t size);
 */
 void thread_rpc_free_payload(struct mobj *mobj);

+/**
+ * Allocates data for payload buffers only shared with the non secure kernel


Pefer s/Allocates/Allocate/
s/non secure/non-secure/

replacing allocates with allocate seems intuitively wrong to me but will do.

Adding my two cents here: we have a mix of both forms currently, and TBH both seem OK to me as long as the description is proper English.
For the commit subjects however we insist on using the imperative mood, but it is a different topic.

etienne-lms · 2020-06-15T11:15:27Z

core/arch/arm/kernel/thread_optee_smc.c

+void thread_rpc_free_kernel_payload(struct mobj *mobj)
+{
+	thread_rpc_free(OPTEE_RPC_SHM_TYPE_KERNEL, mobj_get_cookie(mobj),
+			mobj);


minor: stands in a single 80 char line:

thread_rpc_free(OPTEE_RPC_SHM_TYPE_KERNEL, mobj_get_cookie(mobj), mobj);

etienne-lms · 2020-06-15T11:22:21Z

core/include/kernel/rpc_io_i2c.h

+
+#include <tee_api_types.h>
+
+enum TEE_I2CMode {


prefer sanke_case:

enum rpc_i2c_mode { TEE_RPC_I2C_MODE_WRITE = 0, TEE_RPC_I2C_READ = 1, };

etienne-lms · 2020-06-15T11:22:42Z

core/include/kernel/rpc_io_i2c.h

+	TEE_MODE_READ = 1,
+};
+
+struct TEE_I2CRequest {


etienne-lms · 2020-06-15T11:22:56Z

core/include/kernel/rpc_io_i2c.h

+	uint8_t bus;
+	uint8_t chip;
+	uint8_t *buffer;
+	size_t bufferLen;


buffer_len

etienne-lms · 2020-06-15T11:27:39Z

core/arch/arm/kernel/rpc_io_i2c.c

+	if (res != TEE_SUCCESS)
+		return res;
+
+	*bytes = (size_t)p[2].u.value.a;


Maybe bytes can be optional, and skipped here if NULL

etienne-lms · 2020-06-15T11:32:20Z

core/arch/arm/kernel/rpc_io_i2c.c

+TEE_Result rpc_io_i2c_transfer(struct TEE_I2CRequest *req, size_t *bytes)
+{
+	TEE_Result res = TEE_SUCCESS;
+	struct thread_param p[3] = { 0 };


for struct thread_param flexibility, prefer struct thread_param p[3] = { };

jenswi-linaro

For commit "core: arm: allocate kernel payload", please apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro

Comments for "core: arm: rpc i2c trampoline driver"

jenswi-linaro · 2020-06-15T15:16:09Z

core/arch/arm/kernel/rpc_io_i2c.c

+#include <string.h>
+
+#define I2C_BUFFER_LENGTH 512 /* enough for most use cases */
+static struct mobj *mobj;


Why is this needed?

what is needed? the mobj? it is persistent and never deallocated (as you noticed below)

jenswi-linaro · 2020-06-15T15:16:47Z

core/arch/arm/kernel/rpc_io_i2c.c

+	}
+
+	if (!mobj) {
+		mobj = thread_rpc_alloc_kernel_payload(I2C_BUFFER_LENGTH);


How is this freed? For instance when switching driver from U-Boot to Linux.

it is not freed. once the buffer is allocated to Linux is left there for any i2c comms that might be required. The performance price to allocate/deallocate on every transfer would be too expensive for some i2c devices. I was hoping this would not be an issue for a 512 bytes allocation.

@jenswi-linaro maybe we should kick a timer to free the memory if no transfers occur in a period?

That will not take care of the transition from U-Boot to Linux kernel. This doesn't seem to take concurrency into account. At:

optee_os/core/arch/arm/include/kernel/thread.h

Lines 235 to 237 in 35e770d

void *rpc_fs_payload;

struct mobj *rpc_fs_payload_mobj;

size_t rpc_fs_payload_size;

There's a way of caching a shared memory allocation for the current invoke, per thread. This could perhaps be extended to be able so handle this kind of shared memory allocation too.

I dont understand your point: what is your concern? why do you mention the transition from uboot to linux as an issue?

before linux boots, the user (uboot or spl) must not call this "service" and use the imx_i2c driver instead ; once linux is up, the user can safely use this service -which will allocate the required buffer- and must stop using the imx_i2c driver..

Considering a boot scenario with:

(secure) early-boot -> (secure) OP-TEE -> (non-secure) U-Boot -> (non-secure) Linux

OP-TEE could very well use the I2C trampoline when U-Boot has booted. This of course expects an OP-TEE I2C RPC driver is implemented in U-Boot, as you did for the Linux kernel.

ah I am with you now -and thanks for clarifying- I did fail to establish the link since I didn't submit U-boot support.

So if we are discussing how things might evolve for a future user wanting to integrate with U-Boot - or some other form of pipeline- it would not be too hard to build some form of state machine for those users wanting synchronization between different i2c scenarios. Doing that was beyond the scope of this PR really but who knows, maybe the need arises sooner rather than later.

This PR is about an interface exposed to normal world. It's not enough to just look at Linux then, we must have the big picture from the start.

honestly I dont have a clue what you are asking. let me know if I should abandon the PR, this is getting very time consuming for something this trivial.

Adding a new interface between secure and normal world is not a trivial task. Since we're going to be stuck with it once it's there we'd like to get it right the first time.

etienne-lms · 2020-06-15T15:46:36Z

core/include/optee_rpc_cmd.h

+ * [in]     value[0].c	    The I2C chip to transfer from/to.
+ *
+ * [in/out] memref[1]	    Buffer used to transfer the data.
+ * [in]     value[2].a	    Number of bytes transferred by the kernel driver.


Is value[2] needed? memref[1].size already provides the input and output sizes.

yes it is needed: it reports the actual number of bytes processed by the linux driver.

ok.

Note that output memref[1].size will also provide the information for read case.
As for the input case (write), you don't really care how many bytes were written, you expect TEE_SUCCESS <=> all bytes written.

Discussed from #3905 (comment) and https://github.com/etienne-lms.

By the way, replace kernel by non-secure world.

etienne-lms · 2020-06-15T15:47:57Z

core/arch/arm/kernel/rpc_io_i2c.c

+	TEE_Result res = TEE_SUCCESS;
+	struct thread_param p[3] = { };
+
+	if (!req)


is this really needed?

not sure what to say to this. it is not needed if nobody calls the function with a null pointer...seems on the same line of thought of having to initialize all the stack variables even when there is only one.

It is ok as is but I think assert(req); would be enough.

I think you are right. ok

etienne-lms

commit message:

"This is done by sharing a ~~one~~ small memory buffer where the input and output data is are transferred."
s/worrry/worry/
"OP-TEE clients" usually refers to client applications. Maybe use 'OP-TEE Core" instead.
"The default I2C buffer...": use either i2c only or I2C only.

etienne-lms · 2020-06-18T12:03:07Z

core/arch/arm/kernel/rpc_io_i2c.c

+	if (!mobj) {
+		mobj = thread_rpc_alloc_kernel_payload(I2C_BUFFER_LENGTH);
+		if (!mobj) {
+			/* if running Linux, check the value of the kernel


/* * If running Linux, check the value of the kernel

etienne-lms · 2020-06-18T12:07:20Z

core/arch/arm/kernel/rpc_io_i2c.c

+#include <optee_rpc_cmd.h>
+#include <string.h>
+
+#define I2C_BUFFER_LENGTH 512 /* enough for most use cases */


Maybe make it configurable: CFG_RPC_I2C_BUFFER_SIZE ?= 512 in mk/config.mk.

good point. will do.

etienne-lms · 2020-06-18T12:08:21Z

core/arch/arm/kernel/rpc_io_i2c.c

+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright 2020 Foundries Ltd <jorge@foundries.io>
+ *


remove empty line

etienne-lms · 2020-06-18T12:09:57Z

core/include/optee_rpc_cmd.h

+/*
+ * Transfer data in/out via I2C
+ *
+ * [in]     value[0].a	    Transfer mode (read/write).


s/(read/write)/(write = 0, read = 1)/

why 0 and 1? and how is this different from the statement just above (copied here for you to compare).
unless you want me to also send a patch to fix OPTEE_RPC_CMD_BENCH_REG?

/*

Register timestamp buffer in the linux kernel optee driver

[in] value[0].a Subcommand (register buffer, unregister buffer)

[in] value[0].b Physical address of timestamp buffer

[in] value[0].c Size of buffer
*/
#define OPTEE_RPC_CMD_BENCH_REG 2

This header file should be the reference for the API. It should better describe it explicitly.

For example, you could go this way:

/* * Transfer data in/out via I2C * * [in] value[0].a Transfer mode: enum rpc_i2c_mode. * (...) */ #define OPTEE_RPC_CMD_I2C_TRANSFER 21 enum rpc_i2c_mode { RPC_I2C_MODE_WRITE = 0, RPC_I2C_MODE_READ = 1, };

etienne-lms · 2020-06-18T12:13:07Z

core/include/kernel/rpc_io_i2c.h

+	TEE_RPC_I2C_MODE_READ = 1,
+};
+
+struct rpc_i2c_request {


Add explicit description of the field, mainly bus and chip which may not be obvious.

not needed. it is obvious from the context - If we need to explain what a bus or a chip are, then maybe the reader should read an I2C protocol primer first (I could add a comment with a link if you think it helps)

From my understanding, chip is an I2C address (address of a target I2C slave device), and bus is an I2C interface ID.
I guess in your case, bus 1 will stand for i.MX8M I2C1, 2 for for i.MX8M I2C2. I think this part is not very generic. Or maybe i miss something.

I agree with @etienne-lms, this has to be clarified. To me it looks like this will be part of the ABI with normal world. How are these values determined? Are they platform specific? Do they depend on which version of the Linux kernel use used in normal world?

these values do not depend on which version of the linux kernel is in use.
these values are not platform specific; of course there is nothing preventing an REE to number the i2c buses in any range (instead of 0 to n, they could do 1 to n)

the bus id will be on the schematic for the board (Linux uses numbers from 0, 1, 2, 3 to identify the i2c buses. I stick to that convention)

the chip id will be on the data sheet for the chip (from 0 to 128 as per the data sheet)

the buffer_len depends on how many bytes the client wants to access from the i2c chip.

Do you have examples of Linux kernel or U-Boot RPC handlers that bind such a bus identifier with a I2C device driver? Seems still very platforms specific for a generic OP-TEE RPC handler. Mayne the REE DTB could define the I2C devices OP-TEE RPC handler should bind to upon such a given bus identifier?

This is the only part of this P-R that looks not nice to me.
@jenswi-linaro, @jforissier, @jbech-linaro: if you feel this is ok as a start point, then, I'm fine.

ldts · 2020-06-19T11:07:08Z

commit message:

"This is done by sharing a ~~one~~ small memory buffer where the input and output data is are transferred."

please do send me a link to a reliable grammar source for that. I am curious.

s/worrry/worry/

"OP-TEE clients" usually refers to client applications. Maybe use 'OP-TEE Core" instead.

"The default I2C buffer...": use either i2c only or I2C only.

sure

etienne-lms

commit message:

"This is done by sharing a ~~one~~ small memory buffer where the input and output data is are transferred."

please do send me a link to a reliable grammar source for that. I am curious.

I'm not a English native speaker. If you feel your sentence is fine then ignore my comment :)

etienne-lms · 2020-06-19T12:15:49Z

core/include/kernel/rpc_io_i2c.h

+	TEE_RPC_I2C_MODE_READ = 1,
+};
+
+struct rpc_i2c_request {


From my understanding, chip is an I2C address (address of a target I2C slave device), and bus is an I2C interface ID.
I guess in your case, bus 1 will stand for i.MX8M I2C1, 2 for for i.MX8M I2C2. I think this part is not very generic. Or maybe i miss something.

etienne-lms · 2020-06-19T12:19:16Z

core/include/optee_rpc_cmd.h

+/*
+ * Transfer data in/out via I2C
+ *
+ * [in]     value[0].a	    Transfer mode (read/write).


This header file should be the reference for the API. It should better describe it explicitly.

For example, you could go this way:

/* * Transfer data in/out via I2C * * [in] value[0].a Transfer mode: enum rpc_i2c_mode. * (...) */ #define OPTEE_RPC_CMD_I2C_TRANSFER 21 enum rpc_i2c_mode { RPC_I2C_MODE_WRITE = 0, RPC_I2C_MODE_READ = 1, };

etienne-lms · 2020-06-19T12:23:29Z

core/include/optee_rpc_cmd.h

+ * [in]     value[0].c	    The I2C chip to transfer from/to.
+ *
+ * [in/out] memref[1]	    Buffer used to transfer the data.
+ * [in]     value[2].a	    Number of bytes transferred by the kernel driver.


ok.

Note that output memref[1].size will also provide the information for read case.
As for the input case (write), you don't really care how many bytes were written, you expect TEE_SUCCESS <=> all bytes written.

ldts · 2020-06-19T19:20:45Z

@etienne-lms responding with an error when not the exact number of bytes were transferred (during read or write operations) would mean deviating from what the kernel does which IMO would be inconsistent.

If you check the linux part of this "service", we invoke i2c_master_send and i2c_master_receive: both of those functions return either an error or the number of bytes handled being those not necessarily the ones requested.

So with this service being only some sort of trampoline/wrap to those, I chose not to alter the meaning of the functions it calls.

In fact, IMO and for coherency with the kernel, it is the one calling this service who should decide if it needs to raise an error or not. But not the trampoline.

etienne-lms · 2020-06-22T07:53:23Z

In fact, IMO and for coherency with the kernel, it is the one calling this service who should decide if it needs to raise an error or not. But not the trampoline.

Make sense but consider that OP-TEE Core is not exclisively designed for the Linux kernel. Consider U-Boot which may invoke OP-TEE which in turn may use the I2C trampoline. That said, I guess decoupling the number of bytes transfered from the transfer status makes this interface more flexible.

etienne-lms · 2020-06-22T09:39:24Z

core/arch/arm/kernel/rpc_io_i2c.c

+
+		assert(mobj_get_va(mobj, 0));
+	}
+


Need to add a mutex (OP-TEE condvar) to protect allocated buffer from multi-thread access.

absolutely. I forgot I had disabled multithread support on op-tee for my work. will do.

not anymore since the buffer is now thread specific

fine, thanks.

ldts · 2020-07-20T14:33:22Z

I need to reopen this pull request since the NXP Se050 upstreaming would be pretty hard to justify without it. Could you give me a set of requirements of what you need beyond the review comments done so far?

linux support - was posted and I can move it along (should not be an issue)
u-boot support- do you need me to post u-boot support before going ahead with this proposal
anything else ?

or if this PR is not a good solution for you, could you recommend me what sort of solution would you rather see and I will implement to that requirement? having said that, performance wise, this PR is pretty solid (been using it for months without issues)

jenswi-linaro · 2020-07-22T09:19:48Z

#3987 is merged now.

ldts · 2020-07-22T10:50:58Z

thanks.
I also updated https://lore.kernel.org/patchwork/patch/1277559/

jenswi-linaro · 2020-07-22T12:52:07Z

core/include/optee_rpc_cmd.h

+ * [in]     value[0].c	    The I2C chip to transfer from/to (a.k.a address)
+ *
+ * [in/out] memref[1]	    Buffer used to transfer the data
+ * [in]     value[2].a	    Number of bytes transferred by the actual driver


Shouldn't this be tagged out?

jenswi-linaro · 2020-07-22T12:54:13Z

core/arch/arm/kernel/rpc_io_i2c.c

+	if (res != TEE_SUCCESS)
+		return res;
+
+	*len = (size_t)p[2].u.value.a;


What if p[2].u.value.a is larger than req->buffer_len?

ok will protect against bugs in the service executing the transfer command.

The casting isn't needed.

ldts · 2020-07-27T15:57:41Z

@jenswi-linaro any further comments?

ldts · 2020-07-30T14:09:30Z

@jenswi-linaro @jforissier it would be great if this could be included in the 3.10 release.

jforissier · 2020-07-30T14:24:23Z

@ldts I agree. I believe both Jens and Etienne are on vacation, but I'm still confident that the majority of the currently open PRs can make it in the release.

jenswi-linaro · 2020-07-31T08:52:36Z

core/arch/arm/kernel/rpc_io_i2c.c

@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: BSD-3-Clause


Any reason to not use BSD-2-Clause instead?

no not really. is BSD-2 your preferred license?

@ldts yes: https://optee.readthedocs.io/en/latest/faq/faq.html#license

sure will change. having said that, any issues with accepting third-party BSD-3 licensed libraries?

No problem.

jenswi-linaro · 2020-07-31T08:55:54Z

core/arch/arm/kernel/rpc_io_i2c.c

+	if (res != TEE_SUCCESS)
+		return res;
+
+	*len = (size_t)p[2].u.value.a;


The casting isn't needed.

core/arch/arm/kernel/rpc_io_i2c.c

jenswi-linaro · 2020-07-31T09:07:03Z

core/include/kernel/rpc_io_i2c.h

+#include <tee_api_types.h>
+
+enum rpc_i2c_mode {
+	TEE_RPC_I2C_MODE_WRITE = 0,


You can drop the TEE_ prefix.

jenswi-linaro · 2020-07-31T09:07:38Z

core/include/kernel/rpc_io_i2c.h

+ * Copyright (c) 2020 Foundries Ltd <jorge@foundries.io>
+ */
+
+#ifndef RPC_IO_I2C_H


Please use leading __ (double underscore).

jenswi-linaro · 2020-07-31T09:13:40Z

core/include/kernel/rpc_io_i2c.h

+	TEE_RPC_I2C_MODE_READ = 1,
+};
+
+struct rpc_i2c_request {


I agree with @etienne-lms, this has to be clarified. To me it looks like this will be part of the ABI with normal world. How are these values determined? Are they platform specific? Do they depend on which version of the Linux kernel use used in normal world?

ldts · 2020-08-03T06:44:06Z

these values do not depend on which version of the linux kernel is in use.
these values are not platform specific; of course there is nothing preventing an REE to number the i2c buses in any range (instead of 0 to n, they could do 1 to n)

the bus id will be on the schematic for the board (Linux uses numbers from 0, 1, 2, 3 to identify the i2c buses.)
the chip id will be on the data sheet for the chip (from 0 to 128 as per the data sheet)
the buffer_len depends on how many bytes the client wants to access from the i2c chip.

jenswi-linaro · 2020-08-03T08:56:12Z

these values do not depend on which version of the linux kernel is in use.
these values are not platform specific; of course there is nothing preventing an REE to number the i2c buses in any range (instead of 0 to n, they could do 1 to n)

OK, so how to pick these numbers in a way that REE and TEE mean the same thing?
Chip ID seems to be clear, there's only one way to represent that since it's wired in the hardware.
Bus ID seems a bit more troublesome since that's a logical construction.

the bus id will be on the schematic for the board (Linux uses numbers from 0, 1, 2, 3 to identify the i2c buses.)
the chip id will be on the data sheet for the chip (from 0 to 128 as per the data sheet)

Thanks for the clarification.

the buffer_len depends on how many bytes the client wants to access from the i2c chip.

Makes sense.

ldts · 2020-08-03T10:02:53Z

We could add complexity by making an additional call to the REE asking for the first bus id and then apply the necessary offset
But is it worth it making this extra call?

Because what would we do about the documentation? ie, in the iMX8 Referenfe Manual i2c buses are named i2c1, i2c2, i2c3. But to access i2c3 as defined in the iMX8 RM from OP-TEE with Linux as REE, the bus id to request would be 2.

In my view, we could simply say that i2c buses in OP-TEE are values ranging from 0 to n. REE side of the implementation would need to do the necessary translations.

If that is not enough, we can also complicate the REE implementation to not require the bus id in the call at all and make the driver scan all buses looking for the chip id before performing any operation.

It wont be nice for performance - which is quite bad as it stands for secure elements running a 3.4mbps (to the point that running hash and aes ecb makes opening a TA x30 times slower- but it would definitively remove the ambiguity since the user wont care where the device is...wont even have to read the schematics.

jenswi-linaro · 2020-08-03T10:26:16Z

We could add complexity by making an additional call to the REE asking for the first bus id and then apply the necessary offset
But is it worth it making this extra call?

No

Because what would we do about the documentation? ie, in the iMX8 Referenfe Manual i2c buses are named i2c1, i2c2, i2c3. But to access i2c3 as defined in the iMX8 RM from OP-TEE with Linux as REE, the bus id to request would be 2.

In my view, we could simply say that i2c buses in OP-TEE are values ranging from 0 to n. REE side of the implementation would need to do the necessary translations.

Doesn't this assume statically assigned bus IDs? These can be assigned dynamically if I've understood it correctly.

If that is not enough, we can also complicate the REE implementation to not require the bus id in the call at all and make the driver scan all buses looking for the chip id before performing any operation.

You're saying that the Bus ID is not needed, it's just a shortcut to the Chip ID?

It wont be nice for performance - which is quite bad as it stands for secure elements running a 3.4mbps (to the point that running hash and aes ecb makes opening a TA x30 times slower- but it would definitively remove the ambiguity since the user wont care where the device is...wont even have to read the schematics.

I suppose you could cache the Bus ID. I'm not saying you should do that. I'm still trying to understand this ABI.

ldts · 2020-08-03T12:21:07Z

Yes this assumes statically assigned bus IDs - just as done per the device tree definitions. So the OP-TEE client would need to know in advance in which bus the REE has the i2c chip.

I suggested that the REE could scan all buses looking for the chip requested by TEE but then how would it resolve what to do if the same chip is on several buses? So maybe - even though trivial to implement- is not that great idea performance issues aside.

As a use case, the way I am using this service is by inspecting the schematic, noting that the chip (NXP SE050) is on I2C3 (per the NXP documentation) and then sending requests to Linux for bus ID 2 (since linux enumerates from 0 to n)

But I agree with you, solving the issue for chips on dynamically assigned buses need more thought and more interaction with the REE. do you think we should delay this PR until that is resolved?

jenswi-linaro · 2020-08-03T12:52:30Z

Yes this assumes statically assigned bus IDs - just as done per the device tree definitions. So the OP-TEE client would need to know in advance in which bus the REE has the i2c chip.

That's good to know.

I suggested that the REE could scan all buses looking for the chip requested by TEE but then how would it resolve what to do if the same chip is on several buses? So maybe - even though trivial to implement- is not that great idea performance issues aside.

Yeah, I suspected there would be some more downside.

As a use case, the way I am using this service is by inspecting the schematic, noting that the chip (NXP SE050) is on I2C3 (per the NXP documentation) and then sending requests to Linux for bus ID 2 (since linux enumerates from 0 to n)

A assume that this happens to match with the DTB passed to the kernel and in effect it's the DTB that assigns the Bus IDs not the schematic.

But I agree with you, solving the issue for chips on dynamically assigned buses need more thought and more interaction with the REE. do you think we should delay this PR until that is resolved?

No need for that. But it should be noted in the ABI that the caller is expected to rely on statically assigned Bus IDs and that the caller needs platform specific configuration for those IDs or something like that.

ldts · 2020-08-03T13:39:42Z

well a device tree is just a 1:1 representation of the schematics: if the schematics say that the i2c chip is the second bus, we better hope that the device tree is doing exactly that or the chip will not be found.

I'll add some more information to the PR to indicate that it only supports statically assigned buses.

ldts · 2020-08-04T08:36:26Z

I added additional comments to the commit message and to rpc_io_i2c.h where the request message is defined.

etienne-lms · 2020-08-05T07:29:12Z

core/arch/arm/kernel/rpc_io_i2c.c

+ *
+ * @param req:	mode: transfer mode is either read or write,
+ *		chip: the i2c device address (0..128)
+ *		bus: the i2c adapter (REE bus id where the chip sits)


etienne-lms · 2020-08-05T07:29:40Z

core/arch/arm/kernel/rpc_io_i2c.c

+ * to validate the number of bytes processed by the call.
+ *
+ * @param req:	mode: transfer mode is either read or write,
+ *		chip: the i2c device address (0..128)


etienne-lms · 2020-08-05T12:42:11Z

core/arch/arm/kernel/rpc_io_i2c.c

+ *		buffer: the memory to access during the transfer,
+ *		buffer_len: the number of bytes to be processed,
+ * @param len: the number of bytes processed by the driver
+ * @returns res: TEE_SUCCESS on success, TEE_ERROR_XXX on error


remove res: .

etienne-lms · 2020-08-05T12:43:42Z

core/arch/arm/kernel/rpc_io_i2c.c

+	res = thread_rpc_cmd(OPTEE_RPC_CMD_I2C_TRANSFER, ARRAY_SIZE(p), p);
+	if (res != TEE_SUCCESS)
+		return res;
+	/*


minor: add an empty line above, and a terminal dot to inline comment below.

etienne-lms · 2020-08-05T13:27:47Z

core/include/kernel/rpc_io_i2c.h

+
+enum rpc_i2c_mode {
+	RPC_I2C_MODE_WRITE = 0,
+	RPC_I2C_MODE_READ = 1,


This addresses master read/write transfer. Do you plan to support combined transfer (aka register/memory read/write, where a master emits write to specify target address read/written in slave, followed by effective read/write transfer? Or maybe caller shall issue several of these nominal i2c rpc transfers to do so.

no not really, this was supposed to be the simplest possible way to communicate with an i2c device like an HSM. it is not more ambitious than that.

I guess this is ok then.

etienne-lms · 2020-08-05T13:29:46Z

core/include/kernel/rpc_io_i2c.h

+struct rpc_i2c_request {
+	enum rpc_i2c_mode mode;
+	uint8_t bus; /* bus identifier used by the REE [0..n] */
+	uint8_t chip; /* chip identifier from the device data sheet [0..0x7f] */


Not considering 10 bit slave address support?
LGTM but should maybe be stated in rpc_io_i2c.h.

I didnt need it. I suppose that if this ends up being merged (not so sure anymore) and someone has a need and a device to test it should be easy to add.

How will 10 bit slave address support affect the ABI?

On HW, this is a I2C protocol matter in the transfered data.
On linux side, 10bit or 7bit address is an attribute of the I2C client driver. Adding 10bit address support needs a u16 for ~~bus~~ chip and an attribute stating the slave I2C address size.

(edited)

right, it is not about the technical complexity of the implementation really but I have no means of testing it with an actual device. if that is ok with you - accept that I wont be testing access to those chips- I am ok increasing the size for bus and chip to unsigned short for the chip and int for the bus to match linux sizes. do you prefer if I do that?

So no impact on the ABI since we're transmitting those as uint32_t.

right, it would be just on this side of the world

fine with me then as for the field types.

etienne-lms · 2020-08-05T13:33:35Z

core/include/kernel/rpc_io_i2c.h

+
+/*
+ * The bus identifier defines an implicit ABI with the REE.
+ * Using this service to access I2C chips on REE dynamically assigned buses is


minor: prefer always i2c or always I2C in inline comments.

etienne-lms · 2020-08-05T13:38:58Z

core/include/kernel/rpc_io_i2c.h

+	TEE_RPC_I2C_MODE_READ = 1,
+};
+
+struct rpc_i2c_request {


Do you have examples of Linux kernel or U-Boot RPC handlers that bind such a bus identifier with a I2C device driver? Seems still very platforms specific for a generic OP-TEE RPC handler. Mayne the REE DTB could define the I2C devices OP-TEE RPC handler should bind to upon such a given bus identifier?

ldts · 2020-08-05T20:04:32Z

I am sorry I dont understand. do you mean to ask how does linux declares the bus that the i2c chip sits on? if so this below would be the way to do it (these are chips on i2c bus 0)

&i2c0 {
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins>;
clock-frequency = <400000>;
status = "okay";

    tps: pmic@2d {                                                                                                                                          
            reg = <0x2d>;                                                                                                                                   
    };                                                                                                                                                      
                                                                                                                                                            
    i2c_tmp102: temp@4b {                                                                                                                                   
            compatible = "ti,tmp102";                                                                                                                       
            reg = <0x4b>;                                                                                                                                   
            status = "disabled";                                                                                                                            
    };                                                                                                                                                      
                                                                                                                                                            
    i2c_eeprom: eeprom@52 {                                                                                                                                 
            compatible = "atmel,24c32";                                                                                                                     
            pagesize = <32>;                                                                                                                                
            reg = <0x52>;                                                                                                                                   
            status = "disabled";                                                                                                                            
    };                                                                                                                                                      
                                                                                                                                                            
    i2c_rtc: rtc@68 {                                                                                                                                       
            compatible = "microcrystal,rv4162";                                                                                                             
            reg = <0x68>;                                                                                                                                   
            status = "disabled";                                                                                                                            
    };

};

ldts · 2020-08-05T20:19:09Z

unless you are referring to the kernel side of this feature
https://lore.kernel.org/patchwork/patch/1277843/

ldts · 2020-08-06T06:17:37Z

@etienne-lms review comments addressed

etienne-lms · 2020-08-07T07:58:19Z

core/include/kernel/rpc_io_i2c.h

+	TEE_RPC_I2C_MODE_READ = 1,
+};
+
+struct rpc_i2c_request {


This is the only part of this P-R that looks not nice to me.
@jenswi-linaro, @jforissier, @jbech-linaro: if you feel this is ok as a start point, then, I'm fine.

etienne-lms · 2020-08-07T07:59:26Z

core/include/optee_rpc_cmd.h

@@ -148,6 +148,18 @@
 */
 #define OPTEE_RPC_CMD_BENCH_REG		20

+/*
+ * Transfer data in/out via I2C


...via I2C in master mode with a 7-bit address

well no, this structure is not limiting to 7-bit addresses so I dont think that would be accurate.
And as to having to specify master it is clearly a redundancy...but of course I can do it if it is a condition to get accepted.

IMO you/maintainers have surrounded the few lines of code in this commit with an unnecessary amount of redundant comments...I think it is a mistake but please let me know what needs doing so this gets merged.

Ok the ABI does not limit to 7bit, but the REE side needs to know whether to use 7bit or 10bit address mode. If you expect/implement the 7bit mode on REE side, it lloks simpler to state this command is designed for 7bit. An extra 10bit address mode command can be later added if needed.

As for master mode, maybe that's not a wording found in I2C literature, feel free to reword.

Clear and concise is the motto. It should be possible to understand this without consulting you afterwards. If you can word the description better please do, in fact it is preferable if the one writing the code comments it himself.

but precisely for that it would be incorrect to declare it here: the actual dependency with the 7bit length is not defined here (in the command) but in rpc_io_i2c_transfer as already documented btw .

So doing it here is not only unnecessary but also not correct: we will have to edit this file - when we add support to 10 bits in rpc_io_i2c_transfer.c - just to change the comment but not the code (and isn't the code supposed to do what the comment clarifies?...I would think twice about a file that requires comments to be modified but not their source code because something changed on some other file in the tree....a strong smell that something is not properly abstracted.

anyway...sorry for the explanation ...let me just make the changes and push...it will save us time.

that will not matter - at least not in Linux AFAIK (7 bit addresses are stored in the lower 7 bits of a 16 bit field); I cant speak for other REEs but IMO it would be weird having to add a flag to distinguish between 7 and 10 bit address....would make the core stack ugly

This is the Linux side of the RPC call for reference

client.addr = params[0].u.value.c; client.adapter = i2c_get_adapter(params[0].u.value.b); if (!client.adapter) goto bad; snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr); buf = params[1].u.memref.shm->kaddr; len = params[1].u.memref.size; switch (params[0].u.value.a) { case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD: ret = i2c_master_recv(&client, buf, len); break; case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR: ret = i2c_master_send(&client, buf, len); break; default: i2c_put_adapter(client.adapter); goto bad; }

Clear and concise is the motto. It should be possible to understand this without consulting you afterwards. If you can word the description better please do, in fact it is preferable if the one writing the code comments it himself.

I would have no trouble adding myself as maintainer of this section of code if that helps. This code is the cornerstone for secured elements controlled from the root of trust so its reliability is critical to us.

So for 10 bit mode the lower 10 bits (of params[0].u.value.c) carry the 10bit address and the rest are zero (MBZ or must be zero).
In 7 bit mode the lower 7 bits carry the 7bit address and the rest are zero.

Is that enough or is a flag needed too in order to explicitly tell if it's a 10bit or 7bit address?

If you could update the ABI description to cover this I think this should be good enough to handle eventual future extensions. I guess this is what the discussion here has been about.

I'll add support for 10 bit addresses on linux. will be better.

(didnt answer your question: we could reuse and use lower 16 for address and higher 16 for flags - those are the lengths in linux). but since there is no real need I'll just add another THREAD_PARAM_VALUE indicating a 10 bit address. if in the future we need to support more flags, we can add to this thread param and will look cleaner IMO

etienne-lms · 2020-08-07T08:01:06Z

core/include/kernel/rpc_io_i2c.h

+struct rpc_i2c_request {
+	enum rpc_i2c_mode mode;
+	uint8_t bus; /* bus identifier used by the REE [0..n] */
+	uint8_t chip; /* chip identifier from the device data sheet [0..0x7f] */


fine with me then as for the field types.

etienne-lms · 2020-08-07T08:02:13Z

core/arch/arm/kernel/rpc_io_i2c.c

+	res = thread_rpc_cmd(OPTEE_RPC_CMD_I2C_TRANSFER, ARRAY_SIZE(p), p);
+	if (res != TEE_SUCCESS)
+		return res;
+	/*


Gives OP-TEE access to the i2c buses initialized and controlled by the REE kernel. This is done by memory mapping a buffer from the thread's cache where the input or output data is transferred. Using this mechanism, OP-TEE clients do not have to worry about REE RUNTIME_PM features switching off clocks from the controllers or collisions with other bus masters. This driver assumes that the I2C chip is on a REE statically assigned bus which value is known to OP-TEE (it will not query/probe the REE). Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

ldts · 2020-08-10T09:26:27Z

closing now. will reopen once done.

ldts force-pushed the i2c-trampoline branch 7 times, most recently from 73ea7ab to 3c3a480 Compare May 31, 2020 23:36

ldts mentioned this pull request Jun 2, 2020

when is op-tee first aware that the normal world is running #3910

Closed

jenswi-linaro reviewed Jun 8, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 3c3a480 to 84d8466 Compare June 15, 2020 09:17

etienne-lms reviewed Jun 15, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 84d8466 to be3d5d4 Compare June 15, 2020 13:14

etienne-lms reviewed Jun 15, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from be3d5d4 to 389e66c Compare June 15, 2020 14:30

jenswi-linaro reviewed Jun 15, 2020

View reviewed changes

etienne-lms reviewed Jun 15, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 389e66c to 9a691ee Compare June 17, 2020 08:44

etienne-lms reviewed Jun 18, 2020

View reviewed changes

etienne-lms reviewed Jun 19, 2020

View reviewed changes

etienne-lms reviewed Jun 22, 2020

View reviewed changes

ldts closed this Jun 23, 2020

ldts deleted the i2c-trampoline branch June 23, 2020 09:59

ldts restored the i2c-trampoline branch July 20, 2020 14:28

ldts reopened this Jul 20, 2020

ldts force-pushed the i2c-trampoline branch from 9a691ee to 1d251fa Compare July 22, 2020 10:49

jenswi-linaro reviewed Jul 22, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 1d251fa to 94e892c Compare July 22, 2020 13:18

jenswi-linaro reviewed Jul 31, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 94e892c to ad47551 Compare August 3, 2020 06:59

ldts force-pushed the i2c-trampoline branch from ad47551 to 724f327 Compare August 4, 2020 08:35

etienne-lms reviewed Aug 5, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 724f327 to 9604e6e Compare August 6, 2020 06:16

etienne-lms reviewed Aug 7, 2020

View reviewed changes

ldts force-pushed the i2c-trampoline branch from 9604e6e to 9f01bd5 Compare August 7, 2020 09:59

ldts closed this Aug 10, 2020

	void *rpc_fs_payload;
	struct mobj *rpc_fs_payload_mobj;
	size_t rpc_fs_payload_size;

core: arm: tee to linux i2c trampoline driver #3905

core: arm: tee to linux i2c trampoline driver #3905

Conversation

ldts commented May 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts commented Jun 15, 2020

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts Jun 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts commented May 31, 2020 •

edited

ldts Jun 15, 2020 •

edited

ldts Jun 15, 2020 •

edited

ldts Jun 22, 2020 •

edited

ldts Jun 19, 2020 •

edited

ldts commented Jun 19, 2020 •

edited