From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Sat, 14 Jul 2018 13:51:14 +0200 Subject: [U-Boot] [PATCH] sandbox_flattree: Switch to TPMv2 support In-Reply-To: <20180713233523.7eb63083@xps13> References: <1527251266-30014-1-git-send-email-trini@konsulko.com> <20180601175543.GG21194@bill-the-cat.ec.rr.com> <20180607093828.2700e3f4@xps13> <20180608093651.2bc8e3d3@xps13> <20180713233523.7eb63083@xps13> Message-ID: <20180714135114.532c2696@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de Hi Miquel, Miquel Raynal wrote on Fri, 13 Jul 2018 23:35:23 +0200: > Hi Simon, >=20 >=20 > > >> >> > > > In order to have the test.py tests for TPMv2 run automatica= lly we need > > >> >> > > > to have one of our sandbox builds use TPMv2 rather than TPM= v1. Switch > > >> >> > > > sandbox_flattree over to this style of TPM. =20 > > >> >> > > > > >> >> > > 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. =20 > > >> >> > > > >> >> > Right. But we don't have any run-time automatic tests for TPMv= 1 as the > > >> >> > 'tpm test' command needs to be done manually, at least today (u= nless I'm > > >> >> > missing something under test/py/tests/). And we can't (functio= nally in > > >> >> > real uses) have both TPM types available. Perhaps we should ma= ke TPMv2 > > >> >> > the default for sandbox? All of the TPMv1 code will still be g= etting > > >> >> > build-time exercised due to platforms with TPMv1 on them. =20 > > >> >> > > >> >> I'll take a look at this. It should actually be quite easy to hav= e two > > >> >> TPMs in sandbox, one v1 and one v2. At least I don't know of any > > >> >> impediment. > > >> >> =20 > > >> >> > =20 > > >> >> > > 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 defin= ed as > > >> >> > > inline. We need this build so that we can test those inline f= unctions, > > >> >> > > and we cannot build with both the inline functions and the no= n-inline > > >> >> > > functions since they are named the same > > >> >> > > sandbox_noblk - builds without CONFIG_BLK, which means the le= gacy > > >> >> > > block drivers are used. We cannot use both the legacy and dri= ver-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. W= e could > > >> >> > > probably remove this and add SPL support to the vanilla sandb= ox build, > > >> >> > > since people can still run ./u-boot directly > > >> >> > > > > >> >> > > At present there are unnecessary config differences between t= hese > > >> >> > > builds. This is explained by the fact that it is a pain for p= eople to > > >> >> > > have to add configs separately to each defconfig. But we shou= ld > > >> >> > > probably make them more common. I will take a look. =20 > > >> >> > > > >> >> > OK. > > >> >> > =20 > > >> >> > > What do you think about dropping sandbox_spl and make sandbox= build > > >> >> > > SPL? It does take slightly longer to build, perhaps 25%. =20 > > >> >> > > > >> >> > That's fine with me. > > >> >> > =20 > > >> >> > > > Cc: Simon Glass > > >> >> > > > Signed-off-by: Tom Rini > > >> >> > > > --- > > >> >> > > > 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 automaticall= y. Would > > >> >> > > > that be a better idea? > > >> >> > > > --- =20 > > >> >> > > > > >> >> > > Miquel, can we adjust the code to build both TPMv1 and v2 for= sandbox, > > >> >> > > and select at run-time? =20 > > >> >> > > > >> >> > 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 covera= ge sake. =20 > > >> >> > > >> >> Yes, I am hoping that it is just different drivers with the same = API > > >> >> but perhaps I am going to be disappointed. =20 > > >> > > > >> > 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 an= d v2 > > >> > code. Trying to compile them both with just some Kconfig hacks wou= ld > > >> > simply not work IMHO. > > >> > > > >> > My apologies for not being helpful at all... As Tom said, there ar= e 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? =20 > > >> > > >> 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' =20 > > > > > > 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. =20 > >=20 > > I think you can just remove the #ifdef from inside struct > > tpm_chip_priv - it's not really a nice thing to do anyway. > > =20 > > > > > > 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? =20 > >=20 > > 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. > > =20 > > > =20 > > >> > > >> 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= . =20 > > > > > > It means patching all drivers if we want to do it cleanly. =20 > >=20 > > 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. > > =20 > > > =20 > > >> As to whether the driver is v1 or v2, I wonder if the driver could s= et > > >> a 'version' flag in tpm_chip_priv() ? =20 > > > > > > Probably. > > > =20 > > >> I really don't like the idea of having mutually exclusive code in > > >> driver model, so it would be good to fix this. =20 > > > > > > 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? =20 > >=20 > > Yes I pushed a patch to u-boot-dm branch tpm-working. =20 >=20 > I think I got a proper patch for the above request. >=20 > 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. >=20 > Do you have a solution in mind? >=20 > 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... >=20 > What do you think? >=20 > Here is the patch: > https://github.com/miquelraynal/u-boot/commit/549822f82d797d6125a25af207e= ae12d45737fb7 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=C3=A8l