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. > > 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. > > In both cases, full U-Boot will copy the bloblist to where it likes. > > And this is separate from "bloblist has device tree or we hang()". Yes, well that has been the case in mainline for over a year with several of my boards, so it is a bit late to start worrying about it now. I have OF_BLOBLIST and it works fine. Regards, Simon
