Hi Simon, On Wed, 1 Jan 2025 at 17:15, Simon Glass <s...@chromium.org> wrote:
> Hi Tom, > > On Fri, 20 Dec 2024 at 05:36, Tom Rini <tr...@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 <s...@chromium.org> wrote: > > > > > > > Hi, > > > > > > > > On Mon, 16 Dec 2024 at 11:30, Tom Rini <tr...@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....@linaro.org> > > > > wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Mon, 16 Dec 2024 at 10:43, Simon Glass <s...@chromium.org> > wrote: > > > > > > >> > > > > > > >> Hi Raymond, > > > > > > >> > > > > > > >> On Mon, 16 Dec 2024 at 08:29, Raymond Mao < > raymond....@linaro.org> > > > > wrote: > > > > > > >> > > > > > > > >> > Hi Simon, > > > > > > >> > > > > > > > >> > On Mon, 16 Dec 2024 at 10:20, Simon Glass <s...@chromium.org > > > > > > wrote: > > > > > > >> >> > > > > > > >> >> Hi Raymond, > > > > > > >> >> > > > > > > >> >> On Mon, 16 Dec 2024 at 07:52, Raymond Mao < > > > > raymond....@linaro.org> wrote: > > > > > > >> >> > > > > > > > >> >> > Hi Simon, > > > > > > >> >> > > > > > > > >> >> > On Sun, 15 Dec 2024 at 19:25, Simon Glass < > s...@chromium.org> > > > > wrote: > > > > > > >> >> >> > > > > > > >> >> >> Hi Raymond, > > > > > > >> >> >> > > > > > > >> >> >> On Fri, 13 Dec 2024 at 15:56, Raymond Mao < > > > > raymond....@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-tr...@konsulko.com/ > > > > > > >> >> >> > > > > > > > >> >> >> > Signed-off-by: Raymond Mao <raymond....@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
