Hi Raymond, Tom, On Fri, 3 Jan 2025 at 04:25, Raymond Mao <raymond....@linaro.org> wrote: > > 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.
That wasn't my question, though. If you look for the log and it isn't there, what breaks? Why do we need a Kconfig flag for that? In the case of devicetree we need it because we can't (yet, without further patches) look at the bloblist before it has been created. But what is wrong with just creating a new TPM log if there isn't already one in the bloblist, for example? Really just saying that I am trying to understand the use case. > >> >> > >> > 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. I am confused by your response, TBH. Where did I say that 'we should not use the bloblist library from the beginning to hand over data from the previous stage'? Just to be clear, we *should* use the bloblist library from the beginning to hand over data from the previous stage. My comment was entirely directed to making it easy for open source projects to work together, and U-Boot to work with different phases of itself. If we are adding convoluted hooks to U-Boot to deal with a blob, we should be careful. E.g. adding Kconfig options. I'm going to bow out of this conversation as I don't have anything further to add. What Tom says below seems right to me. Regards, SImon
