[PATCH 1/2] bloblist: add api to get blob with size
bloblist_find function only returns the pointer of blob data, which is fine for those self-describing data like FDT. But as a common scenario, an interface is needed to retrieve both the pointer and the size of the blob data.
Signed-off-by: Raymond Mao raymond.mao@linaro.org --- common/bloblist.c | 17 +++++++++++++++-- include/bloblist.h | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/common/bloblist.c b/common/bloblist.c index ec6ff7a5a9..efc1905da3 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag, struct bloblist_rec **recp, int size, }
void *bloblist_find(uint tag, int size) +{ + void *blob = NULL; + int blob_size; + + blob = bloblist_get_blob(tag, &blob_size); + + if (size && size != blob_size) + return NULL; + + return blob; +} + +void *bloblist_get_blob(uint tag, int *size) { struct bloblist_rec *rec;
rec = bloblist_findrec(tag); if (!rec) return NULL; - if (size && size != rec->size) - return NULL; + + *size = rec->size;
return (void *)rec + rec_hdr_size(rec); } diff --git a/include/bloblist.h b/include/bloblist.h index ff32d3fecf..a04c7c80be 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -250,6 +250,24 @@ static inline void *bloblist_check_magic(ulong addr) return ptr; }
+#if CONFIG_IS_ENABLED(BLOBLIST) +/** + * bloblist_get_blob() - Find a blob and get the size of it + * + * Searches the bloblist and returns the blob with the matching tag + * + * @tag: Tag to search for (enum bloblist_tag_t) + * @size: Size of the blob found + * Return: pointer to bloblist if found, or NULL if not found + */ +void *bloblist_get_blob(uint tag, int *size); +#else +static inline void *bloblist_get_blob(uint tag, int *size) +{ + return NULL; +} +#endif + /** * bloblist_find() - Find a blob *
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()) { + *addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size); + if (*addr && *size) + return 0; + } + addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize); if (!addr_prop) addr_prop = dev_read_prop(dev, "linux,sml-base", &asize);
On Sat, 14 Dec 2024 at 00: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()) {*addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size);if (*addr && *size)return 0;}addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize); if (!addr_prop) addr_prop = dev_read_prop(dev, "linux,sml-base", &asize);-- 2.25.1
Tom, this is a mix of TPM & bloblist series. Do you want me to carry them via the TPM tree?
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On Sun, Dec 15, 2024 at 09:34:32AM +0200, Ilias Apalodimas wrote:
On Sat, 14 Dec 2024 at 00: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()) {*addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size);if (*addr && *size)return 0;}addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize); if (!addr_prop) addr_prop = dev_read_prop(dev, "linux,sml-base", &asize);-- 2.25.1
Tom, this is a mix of TPM & bloblist series. Do you want me to carry them via the TPM tree?
Yeah, that works, thanks.
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.
Also please drop the first term. If you find it in the bloblist, just use it.
In other words, this code should be unconditional.
*addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size);if (*addr && *size)return 0;}addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize); if (!addr_prop) addr_prop = dev_read_prop(dev, "linux,sml-base", &asize);-- 2.25.1
Regards, Simon
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.
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.
[snip]
Regards, Raymond
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.
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.
[snip]
REgards, Simon
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.
[snip]
REgards, Simon
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.
This is the sort of confusion that I find very frustrating. At least with OF_BLOBLIST it was clear that it related to devicetree only...
Regards, Simon
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.
[snip]
Regards, Raymond
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.
Perhaps I should ask, what problem are you trying to solve by making it conditional on a Kconfig?
[snip]
Regards, Simon
Hi Simon,
On Mon, 16 Dec 2024 at 12:37, Simon Glass sjg@chromium.org 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.
Perhaps I should ask, what problem are you trying to solve by making it conditional on a Kconfig?
My idea is to have one kconfig to enable/disable all the stuff related to the Firmware Handoff spec implementations. When this kconfig is enabled, U-Boot will use bloblist (aka. Transfer List, described in Firmware Handoff spec) to handover everything (e.g. FDT, eventlog, ...) from the previous boot stage instead of DT. If this kconfig is disabled, or any errors during parsing the blob, fallback to the legacy handoff way. Surely, in the future we can remove the fallback to restrict the rule. At the moment, I think the 'BLOBLIST_PRIOR_STAGE' kconfig is best aligned to this idea.
Regards. Raymond
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. 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.
And that said, yes, I will be v2'ing what I did, in some manner or another as it's not quite right either in terms of being clear about what the condition is.
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.
And that said, yes, I will be v2'ing what I did, in some manner or another as it's not quite right either in terms of being clear about what the condition is.
Regards, Simon
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.
And that said, yes, I will be v2'ing what I did, in some manner or another as it's not quite right either in terms of being clear about what the condition is.
Regards, Simon
Raymond
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.
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.
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()".
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.
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
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
On Thu, Jan 02, 2025 at 10:25:15AM -0500, Raymond Mao wrote:
[snip]
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.
We do we need this knob? I don't think that we do. The case of bloblist not existing where we looked for it needs to work. And the case of the bloblist not having an entry needs to work (or if it *must* exist, that's a separate option to add, ie CONFIG_TPM_BLOBLIST_LOG_REQUIRED).
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.
Simon doesn't speak for the U-Boot project, he speaks for himself. I don't see why U-Boot shouldn't look for an use a bloblist for things that need to be passed from one stage of the boot process to another.
Hi Tom,
On Thu, 2 Jan 2025 at 10:48, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 02, 2025 at 10:25:15AM -0500, Raymond Mao wrote:
[snip]
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.
We do we need this knob? I don't think that we do. The case of bloblist not existing where we looked for it needs to work. And the case of the bloblist not having an entry needs to work (or if it *must* exist, that's a separate option to add, ie CONFIG_TPM_BLOBLIST_LOG_REQUIRED).
I think this kconfig should not be only for TPM log, but for all general
blob tags which are required to be handed over. User should have a choice to hand over *all* required handoff data from a blobllist (if exists) or to stay in each data's own legacy way (from DT or whatever) Aka, the switch should be general and "one for all", otherwise, we have to add multiple CONFIG_XXX_BLOBLIST_REQUIRED in the future.
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.
Simon doesn't speak for the U-Boot project, he speaks for himself. I don't see why U-Boot shouldn't look for an use a bloblist for things that need to be passed from one stage of the boot process to another.
Thanks for clarifying this.
Thanks and regards, Raymond
On Thu, Jan 02, 2025 at 11:00:02AM -0500, Raymond Mao wrote:
Hi Tom,
On Thu, 2 Jan 2025 at 10:48, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 02, 2025 at 10:25:15AM -0500, Raymond Mao wrote:
[snip]
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.
We do we need this knob? I don't think that we do. The case of bloblist not existing where we looked for it needs to work. And the case of the bloblist not having an entry needs to work (or if it *must* exist, that's a separate option to add, ie CONFIG_TPM_BLOBLIST_LOG_REQUIRED).
I think this kconfig should not be only for TPM log, but for all general blob tags which are required to be handed over. User should have a choice to hand over *all* required handoff data from a blobllist (if exists) or to stay in each data's own legacy way (from DT or whatever) Aka, the switch should be general and "one for all", otherwise, we have to add multiple CONFIG_XXX_BLOBLIST_REQUIRED in the future.
Since we're just getting this effort really moving forward now, I'd like to go with the assumption that bloblists will be complete if passed. So if the TPM code wants to do: if (IS_ENABLED(CONFIG_BLOBLIST)) ... no event log found ... hang("No eventlog in bloblist!") ... That's fine and how we can enforce requirements. But we don't know for certain what a previous to U-Boot stage will or will not have done. It could be TF-A, it could be U-Boot, it could be something else. We also don't have a list of strictly required tags, so that too is perhaps part of the problem.
Hi Tom,
On Thu, 2 Jan 2025 at 11:53, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 02, 2025 at 11:00:02AM -0500, Raymond Mao wrote:
Hi Tom,
On Thu, 2 Jan 2025 at 10:48, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 02, 2025 at 10:25:15AM -0500, Raymond Mao wrote:
[snip]
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.
We do we need this knob? I don't think that we do. The case of bloblist not existing where we looked for it needs to work. And the case of the bloblist not having an entry needs to work (or if it *must* exist, that's a separate option to add, ie CONFIG_TPM_BLOBLIST_LOG_REQUIRED).
I think this kconfig should not be only for TPM log, but for all general blob tags which are required to be handed over. User should have a choice to hand over *all* required handoff data from a blobllist (if exists) or to stay in each data's own legacy way (from DT
or
whatever) Aka, the switch should be general and "one for all", otherwise, we have
to
add multiple CONFIG_XXX_BLOBLIST_REQUIRED in the future.
Since we're just getting this effort really moving forward now, I'd like to go with the assumption that bloblists will be complete if passed. So if the TPM code wants to do: if (IS_ENABLED(CONFIG_BLOBLIST)) ... no event log found ... hang("No eventlog in bloblist!") ... That's fine and how we can enforce requirements. But we don't know for certain what a previous to U-Boot stage will or will not have done. It could be TF-A, it could be U-Boot, it could be something else. We also don't have a list of strictly required tags, so that too is perhaps part of the problem.
OK. I can update my patch with this logic as a temporary solution.
Actually the tags before "BLOBLISTT_AREA_TF = 0x100" can be regarded as strictly-required, and, when any of them does not exist, another kconfig can be introduced for the behaviour whether to hang or to fall back to each one's legacy handoff method. Both kconfigs can be general and one-for-all. But yes, I agree we can solve this later when we have a clearer picture.
Thanks and regards, Raymond
Hi Raymond, Tom,
On Fri, 3 Jan 2025 at 04:25, Raymond Mao raymond.mao@linaro.org wrote:
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.
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
On Thu, Jan 02, 2025 at 11:14:51AM +1300, Simon Glass 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.
Yes, the case of bloblist found, TPM log isn't found needs to work.
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.
No. The normal case is that no one is using a bloblist. The rare case is the platforms you developed bloblist on. The new and growing case is other platforms using the standard mechanism you designed to communicate across projects in a not ad-hoc manner.
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.
You mean your boards that are broken in mainline, because no one quite understood how you were trying to use the bloblist? And that are AFAIK end of life platforms? No, they aren't the important use cases for a bloblist. They're apparently not ideal references for demonstrating how to use a bloblist.
On Sat, 14 Dec 2024 at 00:55, Raymond Mao raymond.mao@linaro.org wrote:
bloblist_find function only returns the pointer of blob data, which is fine for those self-describing data like FDT. But as a common scenario, an interface is needed to retrieve both the pointer and the size of the blob data.
Signed-off-by: Raymond Mao raymond.mao@linaro.org
common/bloblist.c | 17 +++++++++++++++-- include/bloblist.h | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/common/bloblist.c b/common/bloblist.c index ec6ff7a5a9..efc1905da3 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag, struct bloblist_rec **recp, int size, }
void *bloblist_find(uint tag, int size) +{
void *blob = NULL;int blob_size;blob = bloblist_get_blob(tag, &blob_size);if (size && size != blob_size)return NULL;return blob;+}
+void *bloblist_get_blob(uint tag, int *size) { struct bloblist_rec *rec;
rec = bloblist_findrec(tag); if (!rec) return NULL;
if (size && size != rec->size)return NULL;
*size = rec->size; return (void *)rec + rec_hdr_size(rec);} diff --git a/include/bloblist.h b/include/bloblist.h index ff32d3fecf..a04c7c80be 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -250,6 +250,24 @@ static inline void *bloblist_check_magic(ulong addr) return ptr; }
+#if CONFIG_IS_ENABLED(BLOBLIST) +/**
- bloblist_get_blob() - Find a blob and get the size of it
- Searches the bloblist and returns the blob with the matching tag
- @tag: Tag to search for (enum bloblist_tag_t)
- @size: Size of the blob found
- Return: pointer to bloblist if found, or NULL if not found
- */
+void *bloblist_get_blob(uint tag, int *size); +#else +static inline void *bloblist_get_blob(uint tag, int *size) +{
return NULL;+} +#endif
/**
- bloblist_find() - Find a blob
-- 2.25.1
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Hi Raymond,
On Fri, 13 Dec 2024 at 15:55, Raymond Mao raymond.mao@linaro.org wrote:
bloblist_find function only returns the pointer of blob data, which is fine for those self-describing data like FDT. But as a common scenario, an interface is needed to retrieve both the pointer and the size of the blob data.
Signed-off-by: Raymond Mao raymond.mao@linaro.org
common/bloblist.c | 17 +++++++++++++++-- include/bloblist.h | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)
Please could you add a test? Otherwise looks good.
diff --git a/common/bloblist.c b/common/bloblist.c index ec6ff7a5a9..efc1905da3 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag, struct
bloblist_rec **recp, int size,
}
void *bloblist_find(uint tag, int size) +{
void *blob = NULL;int blob_size;blob = bloblist_get_blob(tag, &blob_size);if (size && size != blob_size)return NULL;return blob;+}
+void *bloblist_get_blob(uint tag, int *size)
Can you use sizep instead, like the other functions, so it is clear within the function that it is a pointer?
{ struct bloblist_rec *rec;
rec = bloblist_findrec(tag); if (!rec) return NULL;
if (size && size != rec->size)return NULL;
*size = rec->size; return (void *)rec + rec_hdr_size(rec);} diff --git a/include/bloblist.h b/include/bloblist.h index ff32d3fecf..a04c7c80be 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -250,6 +250,24 @@ static inline void *bloblist_check_magic(ulong addr) return ptr; }
+#if CONFIG_IS_ENABLED(BLOBLIST) +/**
- bloblist_get_blob() - Find a blob and get the size of it
- Searches the bloblist and returns the blob with the matching tag
- @tag: Tag to search for (enum bloblist_tag_t)
- @size: Size of the blob found
- Return: pointer to bloblist if found, or NULL if not found
- */
+void *bloblist_get_blob(uint tag, int *size); +#else +static inline void *bloblist_get_blob(uint tag, int *size) +{
return NULL;+} +#endif
/**
- bloblist_find() - Find a blob
-- 2.25.1
Regards, Simon
participants (4)
-
Ilias Apalodimas -
Raymond Mao -
Simon Glass -
Tom Rini