From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 7 Jan 2021 05:36:12 -0700 Subject: [PATCH] drivers: tee: i2c trampoline driver In-Reply-To: <20210106172348.GA1412@trex> References: <20201221181540.17949-1-jorge@foundries.io> <20201229083005.GA15607@trex> <20210106172348.GA1412@trex> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Jorge, On Wed, 6 Jan 2021 at 10:23, Jorge Ramirez-Ortiz, Foundries wrote: > > On 29/12/20, Simon Glass wrote: > > Hi Jorge, > > > > On Tue, 29 Dec 2020 at 01:30, Jorge Ramirez-Ortiz, Foundries > > wrote: > > > > > > On 28/12/20, Simon Glass wrote: > > > > Hi Jorge, > > > > > > > > On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz 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