[PATCH] fdt_support: add optional board_rng_seed() hook

[PATCH] fdt_support: add optional board_rng_seed() hook

[Rasmus Villemoes]

A recurring theme on LKML is the boot process deadlocking due to some
process blocking waiting for random numbers, while the kernel's
Cryptographic Random Number Generator (crng) is not initalized yet,
but that very blocking means no activity happens that would generate
the entropy necessary to finalize seeding the crng.

This is not a problem on boards that have a good hwrng (when the
kernel is configured to trust it), whether in the CPU or in a TPM or
elsewhere. However, that's far from all boards out there. Moreover,
there are consumers in the kernel that try to obtain random numbers
very early, before the kernel has had any chance to initialize any
hwrng or other peripherals.

Allow a board to provide a board_rng_seed() function, which is
responsible for providing a value to be put into the rng-seed property
under the /chosen node.

The board code is responsible for how to actually obtain those
bytes.

- One possibility is for the board to load a seed "file" from
  somewhere (it need not be a file in a filesystem of course), and
  then ensure that that the same seed file does not get used on
  subsequent boots.

  * One way to do that is to delete the file, or otherwise mark it as
    invalid, then rely on userspace to create a new one, and living
    with the possibility of not finding a seed file during some boots.

  * Another is to use the scheme used by systemd-boot and create a new
    seed file immediately, but in a way that the seed passed to the
    kernel and the new (i.e. next) seed cannot be deduced from each
    other, see the explanation at
    https://lore.kernel.org/lkml/20190929090512.GB13049@gardel-login/
    and the current code at
    https://github.com/systemd/systemd/blob/main/src/boot/efi/random-seed.c

- The board may have an hwrng from which some bytes can be read; while
  the kernel can also do that, doing it in U-Boot and providing a seed
  ensures that even very early users in the kernel get good random
  numbers.

- If the board has a sensor of some sort (temperature, humidity, GPS,
  RTC, whatever), mixing in a reading of that doesn't hurt.

- etc. etc.

These can of course be combined.

The rng-seed property is mixed into the pool used by the linux
kernel's CRNG very early during boot. Whether it then actually
contributes towards the kernel considering the CRNG initialized
depends on whether the kernel has been configured with
CONFIG_RANDOM_TRUST_BOOTLOADER (nowadays overridable via the
random.trust_bootloader command line option). But that's for the BSP
developer to ultimately decide.

So, if the board needs to have all that logic, why not also just have
it do the actual population of /chosen/rng-seed in ft_board_setup(),
which is not that many extra lines of code?

I considered that, but decided handling this logically belongs in
fdt_chosen(). Also, apart from saving the board code from the few
lines of boilerplate, doing it in ft_board_setup() is too late for at
least some use cases. For example, I want to allow the board logic to
decide

  ok, let's pass back this buffer and use that as seed, but also let's
  set random.trust_bootloader=n so no entropy is credited.

This requires the rng-seed handling to happen before bootargs
handling. For example, during the very first boot, the board might not
have a proper seed file, but the board could still return (a hash of)
some CPU serial# or whatnot, so that at least no two boards ever get
the same seed - the kernel always mixes in the value passed in
rng-seed, but if it is not "trusted", the kernel would still go
through the same motions as it would if no rng-seed was passed before
considering its CRNG initialized. I.e., by returning that
unique-to-this-board value and setting random.trust_bootloader=n, the
board would be no worse off than if board_rng_seed() returned nothing
at all.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/Kconfig        | 14 ++++++++++++++
 common/fdt_support.c  | 13 +++++++++++++
 include/fdt_support.h | 13 +++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index e7914ca750..ebee856e56 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -768,6 +768,20 @@ config TPL_STACKPROTECTOR
 	bool "Stack Protector buffer overflow detection for TPL"
 	depends on STACKPROTECTOR && TPL
 
+config BOARD_RNG_SEED
+	bool "Provide /chosen/rng-seed property to the linux kernel"
+	help
+	  Selecting this option requires the board to define a
+	  board_rng_seed() function, which should return a buffer
+	  which will be used to populate the /chosen/rng-seed property
+	  in the device tree for the OS being booted.
+
+	  It is up to the board code (and more generally the whole
+	  BSP) where and how to store (or generate) such a seed, how
+	  to ensure a given seed is only used once, how to create a
+	  new seed for use on subsequent boots, and whether or not the
+	  kernel should account any entropy from the given seed.
+
 endmenu
 
 menu "Update support"
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 8c18af2ce1..baf7fb7065 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -7,6 +7,7 @@
  */
 
 #include <common.h>
+#include <abuf.h>
 #include <env.h>
 #include <log.h>
 #include <mapmem.h>
@@ -279,6 +280,7 @@ __weak char *board_fdt_chosen_bootargs(void)
 
 int fdt_chosen(void *fdt)
 {
+	struct abuf buf = {};
 	int   nodeoffset;
 	int   err;
 	char  *str;		/* used to set string properties */
@@ -294,6 +296,17 @@ int fdt_chosen(void *fdt)
 	if (nodeoffset < 0)
 		return nodeoffset;
 
+	if (IS_ENABLED(CONFIG_BOARD_RNG_SEED) && !board_rng_seed(&buf)) {
+		err = fdt_setprop(fdt, nodeoffset, "rng-seed",
+				  abuf_data(&buf), abuf_size(&buf));
+		abuf_uninit(&buf);
+		if (err < 0) {
+			printf("WARNING: could not set rng-seed %s.\n",
+			       fdt_strerror(err));
+			return err;
+		}
+	}
+
 	str = board_fdt_chosen_bootargs();
 
 	if (str) {
diff --git a/include/fdt_support.h b/include/fdt_support.h
index ac76939e81..b8380716f3 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -11,6 +11,7 @@
 
 #include <asm/u-boot.h>
 #include <linux/libfdt.h>
+#include <abuf.h>
 
 /**
  * arch_fixup_fdt() - Write arch-specific information to fdt
@@ -186,6 +187,18 @@ int fdt_find_or_add_subnode(void *fdt, int parentoffset, const char *name);
  */
 int ft_board_setup(void *blob, struct bd_info *bd);
 
+/**
+ * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
+ *
+ * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
+ * be provided by the board. It should return, via @buf, some suitable
+ * seed value to pass to the kernel.
+ *
+ * @param buf         A struct abuf for returning the seed and its size.
+ * @return            0 if ok, negative on error.
+ */
+int board_rng_seed(struct abuf *buf);
+
 /**
  * board_fdt_chosen_bootargs() - Arbitrarily amend fdt kernel command line
  *
-- 
2.31.1

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

?Hi,

On Mon, 22 Aug 2022 at 01:34, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> A recurring theme on LKML is the boot process deadlocking due to some
> process blocking waiting for random numbers, while the kernel's
> Cryptographic Random Number Generator (crng) is not initalized yet,
> but that very blocking means no activity happens that would generate
> the entropy necessary to finalize seeding the crng.
>
> This is not a problem on boards that have a good hwrng (when the
> kernel is configured to trust it), whether in the CPU or in a TPM or
> elsewhere. However, that's far from all boards out there. Moreover,
> there are consumers in the kernel that try to obtain random numbers
> very early, before the kernel has had any chance to initialize any
> hwrng or other peripherals.
>
> Allow a board to provide a board_rng_seed() function, which is
> responsible for providing a value to be put into the rng-seed property
> under the /chosen node.
>
> The board code is responsible for how to actually obtain those
> bytes.
>
> - One possibility is for the board to load a seed "file" from
>   somewhere (it need not be a file in a filesystem of course), and
>   then ensure that that the same seed file does not get used on
>   subsequent boots.
>
>   * One way to do that is to delete the file, or otherwise mark it as
>     invalid, then rely on userspace to create a new one, and living
>     with the possibility of not finding a seed file during some boots.
>
>   * Another is to use the scheme used by systemd-boot and create a new
>     seed file immediately, but in a way that the seed passed to the
>     kernel and the new (i.e. next) seed cannot be deduced from each
>     other, see the explanation at
>     https://lore.kernel.org/lkml/20190929090512.GB13049@gardel-login/
>     and the current code at
>     https://github.com/systemd/systemd/blob/main/src/boot/efi/random-seed.c
>
> - The board may have an hwrng from which some bytes can be read; while
>   the kernel can also do that, doing it in U-Boot and providing a seed
>   ensures that even very early users in the kernel get good random
>   numbers.
>
> - If the board has a sensor of some sort (temperature, humidity, GPS,
>   RTC, whatever), mixing in a reading of that doesn't hurt.
>
> - etc. etc.
>
> These can of course be combined.
>
> The rng-seed property is mixed into the pool used by the linux
> kernel's CRNG very early during boot. Whether it then actually
> contributes towards the kernel considering the CRNG initialized
> depends on whether the kernel has been configured with
> CONFIG_RANDOM_TRUST_BOOTLOADER (nowadays overridable via the
> random.trust_bootloader command line option). But that's for the BSP
> developer to ultimately decide.
>
> So, if the board needs to have all that logic, why not also just have
> it do the actual population of /chosen/rng-seed in ft_board_setup(),
> which is not that many extra lines of code?
>
> I considered that, but decided handling this logically belongs in
> fdt_chosen(). Also, apart from saving the board code from the few
> lines of boilerplate, doing it in ft_board_setup() is too late for at
> least some use cases. For example, I want to allow the board logic to
> decide
>
>   ok, let's pass back this buffer and use that as seed, but also let's
>   set random.trust_bootloader=n so no entropy is credited.
>
> This requires the rng-seed handling to happen before bootargs
> handling. For example, during the very first boot, the board might not
> have a proper seed file, but the board could still return (a hash of)
> some CPU serial# or whatnot, so that at least no two boards ever get
> the same seed - the kernel always mixes in the value passed in
> rng-seed, but if it is not "trusted", the kernel would still go
> through the same motions as it would if no rng-seed was passed before
> considering its CRNG initialized. I.e., by returning that
> unique-to-this-board value and setting random.trust_bootloader=n, the
> board would be no worse off than if board_rng_seed() returned nothing
> at all.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  common/Kconfig        | 14 ++++++++++++++
>  common/fdt_support.c  | 13 +++++++++++++
>  include/fdt_support.h | 13 +++++++++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/common/Kconfig b/common/Kconfig
> index e7914ca750..ebee856e56 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -768,6 +768,20 @@ config TPL_STACKPROTECTOR
>         bool "Stack Protector buffer overflow detection for TPL"
>         depends on STACKPROTECTOR && TPL
>
> +config BOARD_RNG_SEED
> +       bool "Provide /chosen/rng-seed property to the linux kernel"
> +       help
> +         Selecting this option requires the board to define a
> +         board_rng_seed() function, which should return a buffer
> +         which will be used to populate the /chosen/rng-seed property
> +         in the device tree for the OS being booted.
> +
> +         It is up to the board code (and more generally the whole
> +         BSP) where and how to store (or generate) such a seed, how
> +         to ensure a given seed is only used once, how to create a
> +         new seed for use on subsequent boots, and whether or not the
> +         kernel should account any entropy from the given seed.
> +
>  endmenu
>
>  menu "Update support"
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 8c18af2ce1..baf7fb7065 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <common.h>
> +#include <abuf.h>
>  #include <env.h>
>  #include <log.h>
>  #include <mapmem.h>
> @@ -279,6 +280,7 @@ __weak char *board_fdt_chosen_bootargs(void)
>
>  int fdt_chosen(void *fdt)
>  {
> +       struct abuf buf = {};
>         int   nodeoffset;
>         int   err;
>         char  *str;             /* used to set string properties */
> @@ -294,6 +296,17 @@ int fdt_chosen(void *fdt)
>         if (nodeoffset < 0)
>                 return nodeoffset;
>
> +       if (IS_ENABLED(CONFIG_BOARD_RNG_SEED) && !board_rng_seed(&buf)) {
> +               err = fdt_setprop(fdt, nodeoffset, "rng-seed",
> +                                 abuf_data(&buf), abuf_size(&buf));
> +               abuf_uninit(&buf);
> +               if (err < 0) {
> +                       printf("WARNING: could not set rng-seed %s.\n",
> +                              fdt_strerror(err));
> +                       return err;
> +               }
> +       }
> +
>         str = board_fdt_chosen_bootargs();
>
>         if (str) {
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index ac76939e81..b8380716f3 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -11,6 +11,7 @@
>
>  #include <asm/u-boot.h>
>  #include <linux/libfdt.h>
> +#include <abuf.h>
>
>  /**
>   * arch_fixup_fdt() - Write arch-specific information to fdt
> @@ -186,6 +187,18 @@ int fdt_find_or_add_subnode(void *fdt, int parentoffset, const char *name);
>   */
>  int ft_board_setup(void *blob, struct bd_info *bd);
>
> +/**
> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> + *
> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> + * be provided by the board. It should return, via @buf, some suitable
> + * seed value to pass to the kernel.
> + *
> + * @param buf         A struct abuf for returning the seed and its size.
> + * @return            0 if ok, negative on error.
> + */
> +int board_rng_seed(struct abuf *buf);

Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
method to obtain the information.


> +
>  /**
>   * board_fdt_chosen_bootargs() - Arbitrarily amend fdt kernel command line
>   *
> --
> 2.31.1
>

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Rasmus Villemoes]

On 23/08/2022 15.38, Simon Glass wrote:

>> +/**
>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
>> + *
>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
>> + * be provided by the board. It should return, via @buf, some suitable
>> + * seed value to pass to the kernel.
>> + *
>> + * @param buf         A struct abuf for returning the seed and its size.
>> + * @return            0 if ok, negative on error.
>> + */
>> +int board_rng_seed(struct abuf *buf);
> 
> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> method to obtain the information.

I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
it seems suffer the same problem as ft_board_setup() as I mention,
namely running after the command line (aka /chosen/bootargs) has been
set up.

Also, I can't see how it can actually affect the blob being passed to
the kernel, doesn't

                fixup.tree = oftree_default();
                ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));

mean that fixup.tree points at U-Boot's control fdt rather than the blob
that will be passed as the kernel's fdt? That seems wrong.

Rasmus

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

Hi Rasmus,

On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 23/08/2022 15.38, Simon Glass wrote:
>
> >> +/**
> >> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> >> + *
> >> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> >> + * be provided by the board. It should return, via @buf, some suitable
> >> + * seed value to pass to the kernel.
> >> + *
> >> + * @param buf         A struct abuf for returning the seed and its size.
> >> + * @return            0 if ok, negative on error.
> >> + */
> >> +int board_rng_seed(struct abuf *buf);
> >
> > Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> > option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> > method to obtain the information.
>
> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
> it seems suffer the same problem as ft_board_setup() as I mention,
> namely running after the command line (aka /chosen/bootargs) has been
> set up.

If that is the only problem, then you could add another event for
doing an earlier fixup.

>
> Also, I can't see how it can actually affect the blob being passed to
> the kernel, doesn't
>
>                 fixup.tree = oftree_default();
>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
>
> mean that fixup.tree points at U-Boot's control fdt rather than the blob
> that will be passed as the kernel's fdt? That seems wrong.

Yes that is wrong for many platforms. We should probably just change
it, but there is a complication.

My recent series made a start at supporting writing to a DT using the
ofnode interface. See vbe_simple_test_base() for some comments on the
current state. You could require OF_LIVE to be enabled for your new
feature.

Ideally I'd like to see ofnode used for all devicetree access, but it
will need to be done in stages. In the meantime we should try to head
in that direction.

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Rasmus Villemoes]

On 23/08/2022 16.35, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 23/08/2022 15.38, Simon Glass wrote:
>>
>>>> +/**
>>>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
>>>> + *
>>>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
>>>> + * be provided by the board. It should return, via @buf, some suitable
>>>> + * seed value to pass to the kernel.
>>>> + *
>>>> + * @param buf         A struct abuf for returning the seed and its size.
>>>> + * @return            0 if ok, negative on error.
>>>> + */
>>>> +int board_rng_seed(struct abuf *buf);
>>>
>>> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
>>> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
>>> method to obtain the information.
>>
>> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
>> it seems suffer the same problem as ft_board_setup() as I mention,
>> namely running after the command line (aka /chosen/bootargs) has been
>> set up.
> 
> If that is the only problem, then you could add another event for
> doing an earlier fixup.

Then I'd much rather just add a board_fdt_chosen() hook called early in
fdt_chosen(), rather than having to enable yet another overcomplicated
generic framework. But this was very much specifically targeted at
rng-seed, because that's a generic, defined binding in /chosen, and I
wanted to support that explicitly rather than having each board
implement the logic for populating that - even if, due to its nature,
the board must supply the actual value to put there.

>> Also, I can't see how it can actually affect the blob being passed to
>> the kernel, doesn't
>>
>>                 fixup.tree = oftree_default();
>>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
>>
>> mean that fixup.tree points at U-Boot's control fdt rather than the blob
>> that will be passed as the kernel's fdt? That seems wrong.
> 
> Yes that is wrong for many platforms. We should probably just change
> it, but there is a complication.
> 
> My recent series made a start at supporting writing to a DT using the
> ofnode interface. See vbe_simple_test_base() for some comments on the
> current state. You could require OF_LIVE to be enabled for your new
> feature.
> 
> Ideally I'd like to see ofnode used for all devicetree access, but it
> will need to be done in stages. In the meantime we should try to head
> in that direction.

Huh? You'd need to deserialize the blob we've loaded (from FIT or uImage
or given directly to a bootm command), then have _all_ the various fixup
functions (setting mac addresses, populating /chosen, all the various
arch and board fixups etc.) modify that deserialized tree, and then at
the end of the day, you need to serialize the tree again to pass to
linux. I don't see how that could happen incrementally, and I don't see
what advantage this would bring anyway.

All that has nothing at all to do with how U-Boot code accesses U-Boot's
control DT.

Rasmus

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

Hi Rasmus,

On Tue, 23 Aug 2022 at 23:57, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 23/08/2022 16.35, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 23/08/2022 15.38, Simon Glass wrote:
> >>
> >>>> +/**
> >>>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> >>>> + *
> >>>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> >>>> + * be provided by the board. It should return, via @buf, some suitable
> >>>> + * seed value to pass to the kernel.
> >>>> + *
> >>>> + * @param buf         A struct abuf for returning the seed and its size.
> >>>> + * @return            0 if ok, negative on error.
> >>>> + */
> >>>> +int board_rng_seed(struct abuf *buf);
> >>>
> >>> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> >>> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> >>> method to obtain the information.
> >>
> >> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
> >> it seems suffer the same problem as ft_board_setup() as I mention,
> >> namely running after the command line (aka /chosen/bootargs) has been
> >> set up.
> >
> > If that is the only problem, then you could add another event for
> > doing an earlier fixup.
>
> Then I'd much rather just add a board_fdt_chosen() hook called early in
> fdt_chosen(), rather than having to enable yet another overcomplicated
> generic framework. But this was very much specifically targeted at
> rng-seed, because that's a generic, defined binding in /chosen, and I
> wanted to support that explicitly rather than having each board
> implement the logic for populating that - even if, due to its nature,
> the board must supply the actual value to put there.

You can put the event spy in generic code if you like. I am trying to
think more generic ways to handle things, since we have so many
'hooks'.

>
> >> Also, I can't see how it can actually affect the blob being passed to
> >> the kernel, doesn't
> >>
> >>                 fixup.tree = oftree_default();
> >>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> >>
> >> mean that fixup.tree points at U-Boot's control fdt rather than the blob
> >> that will be passed as the kernel's fdt? That seems wrong.
> >
> > Yes that is wrong for many platforms. We should probably just change
> > it, but there is a complication.
> >
> > My recent series made a start at supporting writing to a DT using the
> > ofnode interface. See vbe_simple_test_base() for some comments on the
> > current state. You could require OF_LIVE to be enabled for your new
> > feature.
> >
> > Ideally I'd like to see ofnode used for all devicetree access, but it
> > will need to be done in stages. In the meantime we should try to head
> > in that direction.
>
> Huh? You'd need to deserialize the blob we've loaded (from FIT or uImage
> or given directly to a bootm command),

yes

> then have _all_ the various fixup
> functions (setting mac addresses, populating /chosen, all the various
> arch and board fixups etc.) modify that deserialized tree,

Well they only need to use ofnode.

>  and then at
> the end of the day, you need to serialize the tree again to pass to
> linux. I don't see how that could happen incrementally, and I don't see
> what advantage this would bring anyway.

Yes that's right. It can actually happen incrementally.

Unless there is very little going on, it is faster to modify the live
tree and then flatten it before calling Linux.

>
> All that has nothing at all to do with how U-Boot code accesses U-Boot's
> control DT.

As I mentioned, ofnode can now access any tree, at least when OF_LIVE
is enabled. But as you point out, there is lots of work to do here.

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

Hi Rasmus,

On Wed, 24 Aug 2022 at 19:25, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rasmus,
>
> On Tue, 23 Aug 2022 at 23:57, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
> >
> > On 23/08/2022 16.35, Simon Glass wrote:
> > > Hi Rasmus,
> > >
> > > On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
> > > <rasmus.villemoes@prevas.dk> wrote:
> > >>
> > >> On 23/08/2022 15.38, Simon Glass wrote:
> > >>
> > >>>> +/**
> > >>>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> > >>>> + *
> > >>>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> > >>>> + * be provided by the board. It should return, via @buf, some suitable
> > >>>> + * seed value to pass to the kernel.
> > >>>> + *
> > >>>> + * @param buf         A struct abuf for returning the seed and its size.
> > >>>> + * @return            0 if ok, negative on error.
> > >>>> + */
> > >>>> +int board_rng_seed(struct abuf *buf);
> > >>>
> > >>> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> > >>> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> > >>> method to obtain the information.
> > >>
> > >> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
> > >> it seems suffer the same problem as ft_board_setup() as I mention,
> > >> namely running after the command line (aka /chosen/bootargs) has been
> > >> set up.
> > >
> > > If that is the only problem, then you could add another event for
> > > doing an earlier fixup.
> >
> > Then I'd much rather just add a board_fdt_chosen() hook called early in
> > fdt_chosen(), rather than having to enable yet another overcomplicated
> > generic framework. But this was very much specifically targeted at
> > rng-seed, because that's a generic, defined binding in /chosen, and I
> > wanted to support that explicitly rather than having each board
> > implement the logic for populating that - even if, due to its nature,
> > the board must supply the actual value to put there.
>
> You can put the event spy in generic code if you like. I am trying to
> think more generic ways to handle things, since we have so many
> 'hooks'.
>
> >
> > >> Also, I can't see how it can actually affect the blob being passed to
> > >> the kernel, doesn't
> > >>
> > >>                 fixup.tree = oftree_default();
> > >>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> > >>
> > >> mean that fixup.tree points at U-Boot's control fdt rather than the blob
> > >> that will be passed as the kernel's fdt? That seems wrong.
> > >
> > > Yes that is wrong for many platforms. We should probably just change
> > > it, but there is a complication.
> > >
> > > My recent series made a start at supporting writing to a DT using the
> > > ofnode interface. See vbe_simple_test_base() for some comments on the
> > > current state. You could require OF_LIVE to be enabled for your new
> > > feature.
> > >
> > > Ideally I'd like to see ofnode used for all devicetree access, but it
> > > will need to be done in stages. In the meantime we should try to head
> > > in that direction.
> >
> > Huh? You'd need to deserialize the blob we've loaded (from FIT or uImage
> > or given directly to a bootm command),
>
> yes
>
> > then have _all_ the various fixup
> > functions (setting mac addresses, populating /chosen, all the various
> > arch and board fixups etc.) modify that deserialized tree,
>
> Well they only need to use ofnode.
>
> >  and then at
> > the end of the day, you need to serialize the tree again to pass to
> > linux. I don't see how that could happen incrementally, and I don't see
> > what advantage this would bring anyway.
>
> Yes that's right. It can actually happen incrementally.
>
> Unless there is very little going on, it is faster to modify the live
> tree and then flatten it before calling Linux.
>
> >
> > All that has nothing at all to do with how U-Boot code accesses U-Boot's
> > control DT.
>
> As I mentioned, ofnode can now access any tree, at least when OF_LIVE
> is enabled. But as you point out, there is lots of work to do here.

I'm looking at adding full support to ofnode for reading/writing any
tree, not just the control FDT. I should have a series out before the
end of next week.

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

Hi Rasmus,

On Sat, 27 Aug 2022 at 19:52, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rasmus,
>
> On Wed, 24 Aug 2022 at 19:25, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rasmus,
> >
> > On Tue, 23 Aug 2022 at 23:57, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> > >
> > > On 23/08/2022 16.35, Simon Glass wrote:
> > > > Hi Rasmus,
> > > >
> > > > On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
> > > > <rasmus.villemoes@prevas.dk> wrote:
> > > >>
> > > >> On 23/08/2022 15.38, Simon Glass wrote:
> > > >>
> > > >>>> +/**
> > > >>>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> > > >>>> + *
> > > >>>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> > > >>>> + * be provided by the board. It should return, via @buf, some suitable
> > > >>>> + * seed value to pass to the kernel.
> > > >>>> + *
> > > >>>> + * @param buf         A struct abuf for returning the seed and its size.
> > > >>>> + * @return            0 if ok, negative on error.
> > > >>>> + */
> > > >>>> +int board_rng_seed(struct abuf *buf);
> > > >>>
> > > >>> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> > > >>> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> > > >>> method to obtain the information.
> > > >>
> > > >> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
> > > >> it seems suffer the same problem as ft_board_setup() as I mention,
> > > >> namely running after the command line (aka /chosen/bootargs) has been
> > > >> set up.
> > > >
> > > > If that is the only problem, then you could add another event for
> > > > doing an earlier fixup.
> > >
> > > Then I'd much rather just add a board_fdt_chosen() hook called early in
> > > fdt_chosen(), rather than having to enable yet another overcomplicated
> > > generic framework. But this was very much specifically targeted at
> > > rng-seed, because that's a generic, defined binding in /chosen, and I
> > > wanted to support that explicitly rather than having each board
> > > implement the logic for populating that - even if, due to its nature,
> > > the board must supply the actual value to put there.
> >
> > You can put the event spy in generic code if you like. I am trying to
> > think more generic ways to handle things, since we have so many
> > 'hooks'.
> >
> > >
> > > >> Also, I can't see how it can actually affect the blob being passed to
> > > >> the kernel, doesn't
> > > >>
> > > >>                 fixup.tree = oftree_default();
> > > >>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> > > >>
> > > >> mean that fixup.tree points at U-Boot's control fdt rather than the blob
> > > >> that will be passed as the kernel's fdt? That seems wrong.
> > > >
> > > > Yes that is wrong for many platforms. We should probably just change
> > > > it, but there is a complication.
> > > >
> > > > My recent series made a start at supporting writing to a DT using the
> > > > ofnode interface. See vbe_simple_test_base() for some comments on the
> > > > current state. You could require OF_LIVE to be enabled for your new
> > > > feature.
> > > >
> > > > Ideally I'd like to see ofnode used for all devicetree access, but it
> > > > will need to be done in stages. In the meantime we should try to head
> > > > in that direction.
> > >
> > > Huh? You'd need to deserialize the blob we've loaded (from FIT or uImage
> > > or given directly to a bootm command),
> >
> > yes
> >
> > > then have _all_ the various fixup
> > > functions (setting mac addresses, populating /chosen, all the various
> > > arch and board fixups etc.) modify that deserialized tree,
> >
> > Well they only need to use ofnode.
> >
> > >  and then at
> > > the end of the day, you need to serialize the tree again to pass to
> > > linux. I don't see how that could happen incrementally, and I don't see
> > > what advantage this would bring anyway.
> >
> > Yes that's right. It can actually happen incrementally.
> >
> > Unless there is very little going on, it is faster to modify the live
> > tree and then flatten it before calling Linux.
> >
> > >
> > > All that has nothing at all to do with how U-Boot code accesses U-Boot's
> > > control DT.
> >
> > As I mentioned, ofnode can now access any tree, at least when OF_LIVE
> > is enabled. But as you point out, there is lots of work to do here.
>
> I'm looking at adding full support to ofnode for reading/writing any
> tree, not just the control FDT. I should have a series out before the
> end of next week.

Well, I sent it and I think you saw it.

What shall we do with this patch? Apply it?

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Rasmus Villemoes]

On 02/09/2022 01.52, Simon Glass wrote:
> Hi Rasmus,
> 
> On Sat, 27 Aug 2022 at 19:52, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Rasmus,
>>
>> On Wed, 24 Aug 2022 at 19:25, Simon Glass <sjg@chromium.org> wrote:

>> I'm looking at adding full support to ofnode for reading/writing any
>> tree, not just the control FDT. I should have a series out before the
>> end of next week.
> 
> Well, I sent it and I think you saw it.

Yeah, I saw it, but unfortunately I don't have time to give it a
thorough review. But please do keep me cc'ed in case you send a new
revision.

> What shall we do with this patch? Apply it?

Well, that's probably not for me to decide (I guess Tom is), but I'd
still like it applied. It's simple, and works now, and it can always be
removed again if there's a demonstrated viable alternative.

Rasmus

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

Hi Rasmus,

On Fri, 2 Sept 2022 at 01:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 02/09/2022 01.52, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Sat, 27 Aug 2022 at 19:52, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Rasmus,
> >>
> >> On Wed, 24 Aug 2022 at 19:25, Simon Glass <sjg@chromium.org> wrote:
>
> >> I'm looking at adding full support to ofnode for reading/writing any
> >> tree, not just the control FDT. I should have a series out before the
> >> end of next week.
> >
> > Well, I sent it and I think you saw it.
>
> Yeah, I saw it, but unfortunately I don't have time to give it a
> thorough review. But please do keep me cc'ed in case you send a new
> revision.
>
> > What shall we do with this patch? Apply it?
>
> Well, that's probably not for me to decide (I guess Tom is), but I'd
> still like it applied. It's simple, and works now, and it can always be
> removed again if there's a demonstrated viable alternative.

OK I'll apply it for this release and we'll see how much progress is
made with ofnode in the next one.

Are you OK with reworking it to use events when possible?

Regards,
Simon

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Rasmus Villemoes]

On 02/09/2022 21.59, Simon Glass wrote:
> Hi Rasmus,
> 
> On Fri, 2 Sept 2022 at 01:00, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>>> What shall we do with this patch? Apply it?
>>
>> Well, that's probably not for me to decide (I guess Tom is), but I'd
>> still like it applied. It's simple, and works now, and it can always be
>> removed again if there's a demonstrated viable alternative.
> 
> OK I'll apply it for this release and we'll see how much progress is
> made with ofnode in the next one.

Thanks.

> Are you OK with reworking it to use events when possible?
> 

Yes, if it turns out to be a mostly drop-in replacement I'm fine with
this board_rng_seed hook vanishing again.

Rasmus

Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass]

On 02/09/2022 21.59, Simon Glass wrote:
> Hi Rasmus,
>
> On Fri, 2 Sept 2022 at 01:00, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>>> What shall we do with this patch? Apply it?
>>
>> Well, that's probably not for me to decide (I guess Tom is), but I'd
>> still like it applied. It's simple, and works now, and it can always be
>> removed again if there's a demonstrated viable alternative.
>
> OK I'll apply it for this release and we'll see how much progress is
> made with ofnode in the next one.

Thanks.

> Are you OK with reworking it to use events when possible?
>

Yes, if it turns out to be a mostly drop-in replacement I'm fine with
this board_rng_seed hook vanishing again.

Rasmus

Applied to u-boot-dm, thanks!