[PATCH] drivers: tee: i2c trampoline driver

[Simon Glass]

Hi Jorge,

On Wed, 6 Jan 2021 at 10:23, Jorge Ramirez-Ortiz, Foundries
<jorge at foundries.io> wrote:
>
> On 29/12/20, Simon Glass wrote:
> > Hi Jorge,
> >
> > On Tue, 29 Dec 2020 at 01:30, Jorge Ramirez-Ortiz, Foundries
> > <jorge at foundries.io> wrote:
> > >
> > > On 28/12/20, Simon Glass wrote:
> > > > Hi Jorge,
> > > >
> > > > On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge at 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
> > > >
> > > > typically
> > >
> > > ok
> > >
> > > >
> > > > > like the NXP SE050).
> > > > >
> > > > > Tested on imx8mmevk.
> > > >
> > > > We don't seem to have any optee tests in U-Boot at present. I vaguely
> > > > recall they were coming at some point. I think we need:
> > > >
> > > > - a sandbox fake drive for optee, that understands and responds to the
> > > > 6 uclass calls at a basic level
> > > > - an update to get_invoke_func() that provides a sandbox function too
> > > >
> > > > Then we should be able to run optee tests in CI.
> > > >
> > > > It is not a lot of work, but I don't think we should add to optee
> > > > until this is resolved.
> > >
> > > um, ok but shouldnt this infrastructure better rest on a maintainer's
> > > roadmap rather than on an off-the-blue request? I mean, had I known I
> > > could have done it in parallel but now I'll need to find the time to
> > > do this.
> >
> > We always need tests in U-Boot, so if you are not writing a test it
> > would be a good question to ask as to how you can do that. Actually
> > patman sometimes warns about that, but perhaps only in certain
> > situations.
> >
> > Actually I see that there is a test - it is hidden under the generic
> > unit tests so I didn't see it. See dm/test/tee.c
> >
> > I'll make some comments on the patch.
> >
> > >
> > > also notice that Linux's equivalent patchset was merged back in the
> > > summer (ie, this is not untested code).
> > >
> > > https://lkml.org/lkml/2020/8/12/276
> >
> > I don't see any tests in that patch though...are they somewhere
> else?
>
> hey
>
> no you are right, I didnt post any tests to linux. And the more I
> think about it the less convinced I am that we needed one.
>
> This commit is nothing more than a RPC TEE service that shares a
> buffer with U-boot and then sends requests to an I2C chip.
>
> > Or do you justmean people have been running similar code?
>
> I am not sure many people is using it yet other than a number of our
> customers at Foundries.io across different projects. But the number of
> users is at a steady growth (everybody seems to need this
> functionality).
>
> In our use case this trampoline code is used to access the NXP SE050
> from OP-TEE over I2C via:
> https://github.com/foundriesio/plug-and-trust
>
> (from that link there is access to a lot of information if you are
> interested)
>
> But of course, it can be used by OP-TEE to access (or even just probe)
> any chip.
>
> > If so,
> > that's fair enough but it doesn't really help us much. Lots of people
> > test code manually before submitting patches, at least for their use
> > case, but this is an open-source project. Over time people want to
> > change and expand the code, and it is very hard for them to do that if
> > there are no automated tests.
>
> Right. And I dont disagree (everything should be testable)
> jfyi I maintain a couple of platforms here so I am familiar with this
> project (been using it on/off for a couple of decades already...umm
> time flies, seems like yesterday).
>
> But I dont think we need a test that verifies this service since
> this is just an amalgamation of two other functions that should be
> tested somewhere else (ie TEE RPC and I2C transfers).
>
> IMO, if they dont exist already, u-boot would benefit from:
> - a test that verifies TEE RPC

Do we have that? If not, we need it.

> - a test that verifies TEE buffer sharing with U-BOOT

I'm not sure what that is, but if there is code in U-Boot it should be tested.

> - a test that verifies I2C reading/writing

This is test/dm/i2c.c
>
> But not so much from
> - a test that verifies TEE I2C trampoline service

I suppose with the above this is trivial to write? Often tests are
just a few lines of code but they provide coverage for the feature.

>
> I am going to repost the patch addressing some of your concerns (I
> found some other issues) and if you still think that having a test
> will be convenient sure we can go ahead and work on it.

OK ta.

Regards,
Simon