[U-Boot] [PATCH] sandbox_flattree: Switch to TPMv2 support

Sat Jul 14 11:51:14 UTC 2018

Hi Miquel,

Miquel Raynal <miquel.raynal at bootlin.com> wrote on Fri, 13 Jul 2018
23:35:23 +0200:

> Hi Simon,
> 
> 
> > >> >> > > > In order to have the test.py tests for TPMv2 run automatically we need
> > >> >> > > > to have one of our sandbox builds use TPMv2 rather than TPMv1.  Switch
> > >> >> > > > sandbox_flattree over to this style of TPM.    
> > >> >> > >
> > >> >> > > The problem seems to be that the sandbox driver is only built with
> > >> >> > > either TPMv1 or TPMv2. It needs to be able to build with both, so we
> > >> >> > > can run tests with both.    
> > >> >> >
> > >> >> > Right.  But we don't have any run-time automatic tests for TPMv1 as the
> > >> >> > 'tpm test' command needs to be done manually, at least today (unless I'm
> > >> >> > missing something under test/py/tests/).  And we can't (functionally in
> > >> >> > real uses) have both TPM types available.  Perhaps we should make TPMv2
> > >> >> > the default for sandbox?  All of the TPMv1 code will still be getting
> > >> >> > build-time exercised due to platforms with TPMv1 on them.    
> > >> >>
> > >> >> I'll take a look at this. It should actually be quite easy to have two
> > >> >> TPMs in sandbox, one v1 and one v2. At least I don't know of any
> > >> >> impediment.
> > >> >>    
> > >> >> >    
> > >> >> > > It really doesn't make any sense to have build-time branches for sandbox.
> > >> >> > >
> > >> >> > > We currently have:
> > >> >> > >
> > >> >> > > sandbox - should be used for most tests
> > >> >> > > sandbox64 - special build that forces a 64-bit host
> > >> >> > > sandbox_flattree - builds with dev_read_...() functions defined as
> > >> >> > > inline. We need this build so that we can test those inline functions,
> > >> >> > > and we cannot build with both the inline functions and the non-inline
> > >> >> > > functions since they are named the same
> > >> >> > > sandbox_noblk - builds without CONFIG_BLK, which means the legacy
> > >> >> > > block drivers are used. We cannot use both the legacy and driver-model
> > >> >> > > block drivers since they implement the same functions
> > >> >> > > sandbox_spl - builds sandbox with SPL support, so you can run
> > >> >> > > spl/u-boot-spl and it will start up and then load ./u-boot. We could
> > >> >> > > probably remove this and add SPL support to the vanilla sandbox build,
> > >> >> > > since people can still run ./u-boot directly
> > >> >> > >
> > >> >> > > At present there are unnecessary config differences between these
> > >> >> > > builds. This is explained by the fact that it is a pain for people to
> > >> >> > > have to add configs separately to each defconfig. But we should
> > >> >> > > probably make them more common. I will take a look.    
> > >> >> >
> > >> >> > OK.
> > >> >> >    
> > >> >> > > What do you think about dropping sandbox_spl and make sandbox build
> > >> >> > > SPL? It does take slightly longer to build, perhaps 25%.    
> > >> >> >
> > >> >> > That's fine with me.
> > >> >> >    
> > >> >> > > > Cc: Simon Glass <sjg at chromium.org>
> > >> >> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > >> >> > > > ---
> > >> >> > > > I'm tempted to switch the main sandbox target over instead as I don't
> > >> >> > > > quite see where we're running the tpm1.x tests automatically.  Would
> > >> >> > > > that be a better idea?
> > >> >> > > > ---    
> > >> >> > >
> > >> >> > > Miquel, can we adjust the code to build both TPMv1 and v2 for sandbox,
> > >> >> > > and select at run-time?    
> > >> >> >
> > >> >> > I thought we had talked about that before and couldn't easily?  One
> > >> >> > thing I am a bit wary of is adding indirection for build coverage sake.    
> > >> >>
> > >> >> Yes, I am hoping that it is just different drivers with the same API
> > >> >> but perhaps I am going to be disappointed.    
> > >> >
> > >> > Indeed, both versions share the same 'architecture' but quite a few
> > >> > structures/functions are defined differently for each TPM flavour in
> > >> > different files. What makes the magic are the
> > >> > #ifdef TPM_V1
> > >> > #else
> > >> > #endif
> > >> > blocks around includes, making them mutually exclusive.
> > >> >
> > >> > Choice has been made not to use both flavours at the same time in the
> > >> > second series, when I clearly made a separation between v1 code and v2
> > >> > code. Trying to compile them both with just some Kconfig hacks would
> > >> > simply not work IMHO.
> > >> >
> > >> > My apologies for not being helpful at all... As Tom said, there are no
> > >> > tests running on v1 code so maybe it's better to exercise v2 code in
> > >> > Sandbox and let people compile-test the former on their own?    
> > >>
> > >> I had a play with this and it does not seem too tricky.
> > >>
> > >> With a bit of fiddling I got it to build except for this:
> > >>
> > >> /home/sjg/c/src/third_party/u-boot/files/cmd/tpm-v2.c:324: multiple
> > >> definition of `get_tpm_commands'    
> > >
> > > That's one problem. I'm pretty sure at some point we will need to
> > > declare differently tpm_chip_priv depending on the version. Using two
> > > structures in an enumeration could be the way to handle it.    
> > 
> > I think you can just remove the #ifdef from inside struct
> > tpm_chip_priv - it's not really a nice thing to do anyway.
> >   
> > >
> > > Another point is that doing so, you embed twice the code and symbols
> > > than what's really needed. Is not having mutually exclusive
> > > code better than enlarging U-Boot binary?    
> > 
> > The sandbox binary is enormous since it enables as many features as it
> > can. We can always create a minimal sandbox if it becomes useful, but
> > for now sandbox is mostly for testing.
> >   
> > >    
> > >>
> > >> I think if you adjust it to check the driver version (v1 or v2), then
> > >> you can use either the v1 or v2 command set. You could move the
> > >> get_tpm_commands function into the uclass so it can check the driver.    
> > >
> > > It means patching all drivers if we want to do it cleanly.    
> > 
> > Yes, you could have a field that needs to be set so that the uclass
> > knows which version it is. Alternatively if you want to save a patch
> > you could have an is_v2 bool, which defaults to 0 for v1 drivers.
> >   
> > >    
> > >> As to whether the driver is v1 or v2, I wonder if the driver could set
> > >> a 'version' flag in tpm_chip_priv() ?    
> > >
> > > Probably.
> > >    
> > >> I really don't like the idea of having mutually exclusive code in
> > >> driver model, so it would be good to fix this.    
> > >
> > > I'll be away the next weeks, so I won't work on it before end of June.
> > > Can you share a diff of your changes?    
> > 
> > Yes I pushed a patch to u-boot-dm branch tpm-working.  
> 
> I think I got a proper patch for the above request.
> 
> One thing I could not figure out:
> There is one U_BOOT_CMD() definition per file (v1 and v2). Because now
> I have to call them 'tpm' and 'tpm2', it creates two commands instead of
> one. I've got the whole logic behind for the driver to tell which stack
> it wants to use, but the user still has to use either one command or the
> other because now both are defined (and we can't define twice
> U_BOOT_CMD() with 'tpm' as command name.
> 
> Do you have a solution in mind?
> 
> Otherwise people already using TPM v2 (if any) will have to change their
> scripts to use 'tpm2' instead of 'tpm' (the tests in test/py/tests
> will also change). I really dislike this change just for compile
> testing while I had both stacks hidden behind a single command...
> 
> What do you think?
> 
> Here is the patch:
> https://github.com/miquelraynal/u-boot/commit/549822f82d797d6125a25af207eae12d45737fb7

Actually it will still work for people using 'tpm' instead of 'tpm2' if
CONFIG_TPM_V1 is not set in their configuration.

I fixed a compilation issue in the above commit, I think the patch is
ready, will send it after a few more tests.

Thanks,
Miquèl