Hi Simon,
On Wed, 1 Jan 2025 at 17:15, Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Fri, 20 Dec 2024 at 05:36, Tom Rini trini@konsulko.com wrote:
On Thu, Dec 19, 2024 at 10:37:37AM -0500, Raymond Mao wrote:
Hi Simon,
On Thu, 19 Dec 2024 at 10:08, Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 16 Dec 2024 at 11:30, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 16, 2024 at 10:37:30AM -0700, Simon Glass wrote:
Hi Raymond,
On Mon, 16 Dec 2024 at 10:24, Raymond Mao <
raymond.mao@linaro.org>
wrote:
> > Hi Simon, > > On Mon, 16 Dec 2024 at 10:43, Simon Glass sjg@chromium.org
wrote:
>> >> Hi Raymond, >> >> On Mon, 16 Dec 2024 at 08:29, Raymond Mao <
raymond.mao@linaro.org>
wrote:
>> > >> > Hi Simon, >> > >> > On Mon, 16 Dec 2024 at 10:20, Simon Glass <sjg@chromium.org
wrote:
>> >> >> >> Hi Raymond, >> >> >> >> On Mon, 16 Dec 2024 at 07:52, Raymond Mao <
raymond.mao@linaro.org> wrote:
>> >> > >> >> > Hi Simon, >> >> > >> >> > On Sun, 15 Dec 2024 at 19:25, Simon Glass <
sjg@chromium.org>
wrote:
>> >> >> >> >> >> Hi Raymond, >> >> >> >> >> >> On Fri, 13 Dec 2024 at 15:56, Raymond Mao <
raymond.mao@linaro.org> wrote:
>> >> >> > >> >> >> > Get tpm event log from bloblist instead of FDT when
bloblist is
>> >> >> > enabled and valid from previous boot stage. >> >> >> > >> >> >> > Note: >> >> >> > This patch depends on: >> >> >> > [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE
options
>> >> >> >
https://lore.kernel.org/u-boot/20241212151142.1562825-1-trini@konsulko.com/
>> >> >> > >> >> >> > Signed-off-by: Raymond Mao raymond.mao@linaro.org >> >> >> > --- >> >> >> > lib/tpm_tcg2.c | 7 +++++++ >> >> >> > 1 file changed, 7 insertions(+) >> >> >> > >> >> >> > diff --git a/lib/tpm_tcg2.c b/lib/tpm_tcg2.c >> >> >> > index 7f868cc883..acaf0acb88 100644 >> >> >> > --- a/lib/tpm_tcg2.c >> >> >> > +++ b/lib/tpm_tcg2.c >> >> >> > @@ -19,6 +19,7 @@ >> >> >> > #include <linux/unaligned/generic.h> >> >> >> > #include <linux/unaligned/le_byteshift.h> >> >> >> > #include "tpm-utils.h" >> >> >> > +#include <bloblist.h> >> >> >> > >> >> >> > int tcg2_get_pcr_info(struct udevice *dev, u32
*supported_pcr, u32 *active_pcr,
>> >> >> > u32 *pcr_banks) >> >> >> > @@ -672,6 +673,12 @@ __weak int
tcg2_platform_get_log(struct udevice *dev, void **addr, u32 *size)
>> >> >> > *addr = NULL; >> >> >> > *size = 0; >> >> >> > >> >> >> > + if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
!bloblist_maybe_init()) {
>> >> >> >> >> >> Please drop the second term...the bloblist is inited by
U-Boot already.
>> >> >> >> >> > >> >> > As an independent library, from the tpm library's angle,
whether the bloblist init was already called or not is agnostic.
>> >> > I think this is the purpose of the "maybe" function. >> >> >> >> No. U-Boot does init at the start. We don't want each
subsystem
>> >> 'maybe' initing each other subsystem that it uses. If you
want to
>> >> check whether bloblist is available, you can use the GD
flag.
>> >> >> > >> > OK. I can use the gd instead. >> > >> >> >> >> > >> >> >> >> >> >> Also please drop the first term. If you find it in the
bloblist, just use it.
>> >> >> >> >> > >> >> > I think here we should have the same logic as the
existing one
in fdtdec:
>> >> > If bloblist is not enabled or errors observed, do a
fallback
to the original process.
>> >> > At least I want to keep the same logic in different
places
before everything is ready from TF-A to U-boot.
>> >> >> >> Yes, your second line is right. But PRIOR_STAGE doesn't
exist,
nor
>> >> should it. Nor do you need it, since you can just check the
bloblist
>> >> for what you want. If you don't find it, fall back. >> >> >> > >> > Since Firmware Handoff is still an experimental feature and
not
mandatory yet (at least now),
>> > I put everything under PRIOR_STAGE to allow users to
disable it
by their own decisions.
>> > I saw PRIOR_STAGE in one of Tom's recent patches so I have
added
a dependency claim in my commit message.
>> >> Yes, but even if Tom does pick his patch, which I object to,
by the
>> time your code is called, there will be a bloblist. >> >> So the end result of your test is to allow the user to ignore
any
TPM
>> log in the bloblist. If that's what you want (which seems
fine to
me),
>> add a Kconfig for it. >> > > Per my understanding of PRIOR_STAGE, it means we have a
bloblist
from the previous stage and we want to use it (as a Transfer List).
> If this is the case, it is not for DT only but can be applied
for
eventlog or anything for Firmware Handoff in the future.
> That is the reason I use PRIOR_STAGE here, I don't think we
need to
add another kconfig.
Who knows what it is supposed to mean...anyway, it should not be
used
like that. The devicetree is special in that we want to get it
from
the bloblist very early, if available. Nothing else (so far) in
the
bloblist has this constraint, certainly not the TPM log.
Raymond is exactly right, and the device tree is not special.
That is quite wrong IMO. Devicetree is special because we need it early and it might come from bloblist before the bloblist has been
set
up by U-Boot. It also controls the whole operation of U-Boot. There
is
nothing else in U-Boot quite like the devicetree.
The specialness is "do we have a valid bloblist upon entry or not".
Which
means that also no, this series (since it happens late enough)
shouldn't
care about if the previous stage had created it or not, merely as
you
said if the tag for the TPM (or other blob) is found.
I would feel more comfortable having the discussions about what previous stages need to pass U-Boot a bloblist once we have that situation. This particular patch seems clear enough, and well motivated. It just needs to drop the conditions.
I think we still need a condition here indicating that the kconfig of "handoff via bloblist" is enabled. And this "handoff via bloblist" kconfig should mean to handoff
everything
via bloblist, including DT.
No, I don't agree with that at all.
What actually breaks if you look for the TPM log in the bloblist? Please answer this question.
As I said, we need an kconfig here to decide whether a user should look for TPM log (and all other handoff information defined by the Firmware Handoff specification) from the bloblist or not. We don't have such kconfig now.
There's at least two sets of challenges here. One, being solved by vexpress64 right now, is that we didn't have CONFIG_BLOBLIST_PASSAGE as an actual option. And in that case, there's no U-Boot before full U-Boot and the bloblist exists for us. Two, U-Boot is what is creating the bloblist. The contentious parts are *when* it's created and *where* it resides prior to full U-Boot seeing it.
There isn't contention, so far as I am aware. The normal case is that U-Boot creates and uses the bloblist itself. Pre-U-Boot blobs (like TF-A, sadly) are not the normal case and should be discouraged in an open-source project. That doesn't mean we shouldn't support them, but it is the tail wagging the dog.
TBH, I am confused with this statement which means we should not use the
bloblist library from the beginning to hand over data from the previous stage. If U-Boot bloblist only intends to consume the data created by itself, we have to introduce another library to do the handoff, like what was done in TF-A and OP-TEE - then finally we can have a standard handoff library that can be used in all projects and keep bloblist as it was.
[snip]
Regards, Raymond