Hi Simon, On Fri, 8 Jun 2018 13:59:07 -0800, Simon Glass <s...@chromium.org> wrote:
> Hi Miquel, > > On 7 June 2018 at 23:36, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > > Hi Simon, > > > > On Thu, 7 Jun 2018 16:25:28 -0800, Simon Glass <s...@chromium.org> wrote: > > > >> Hi Miquel, > >> > >> On 6 June 2018 at 23:38, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > >> > Hello, > >> > > >> > Sorry for the delay. > >> > > >> > On Sat, 2 Jun 2018 10:15:17 -0600, Simon Glass <s...@chromium.org> wrote: > >> > > >> >> Hi Tom, > >> >> > >> >> On 1 June 2018 at 11:55, Tom Rini <tr...@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 <tr...@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 <s...@chromium.org> > >> >> > > > Signed-off-by: Tom Rini <tr...@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. Ok. > > > > > 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 meant for all the platforms. But you are right that this double selection should only happen for Sandbox. > > > > >> > >> 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. Maybe an enum would be better. 0 would still mean a v1 driver. > > > > >> 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. Thanks! > > Regards, > Simon Regards, Miquèl -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
