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

[Simon Glass]

Hi Miquel,

On 6 June 2018 at 23:38, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> Hello,
>
> Sorry for the delay.
>
> On Sat, 2 Jun 2018 10:15:17 -0600, Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Tom,
>>
>> On 1 June 2018 at 11:55, Tom Rini <trini at konsulko.com> wrote:
>> >
>> > On Fri, Jun 01, 2018 at 09:25:19AM -0600, Simon Glass wrote:
>> > > +Miquel due to sandbox TPM issue
>> > >
>> > > Hi Tom,
>> > >
>> > > On 25 May 2018 at 06:27, Tom Rini <trini at konsulko.com> wrote:
>> > > > 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'

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.
As to whether the driver is v1 or v2, I wonder if the driver could set
a 'version' flag in tpm_chip_priv() ?

I really don't like the idea of having mutually exclusive code in
driver model, so it would be good to fix this.

Regards,
Simon