[PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[Daniel Golle]

It can be useful for the OS (Linux) to know which configuration has
been chosen by U-Boot when launching a FIT image.
Store the name of the FIT configuration node used in a new string
attribute called 'bootconf' in the '/chosen' node in device tree.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/image-fdt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/boot/image-fdt.c b/boot/image-fdt.c
index 692a9ad3e4..4017bc94a6 100644
--- a/boot/image-fdt.c
+++ b/boot/image-fdt.c
@@ -601,6 +601,12 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
 		goto err;
 	}
 
+	/* Store name of configuration node as bootconf in /chosen node */
+	if (images->fit_uname_cfg)
+		fdt_find_and_setprop(blob, "/chosen", "bootconf",
+					images->fit_uname_cfg,
+					strlen(images->fit_uname_cfg) + 1, 1);
+
 	/* Update ethernet nodes */
 	fdt_fixup_ethernet(blob);
 #if CONFIG_IS_ENABLED(CMD_PSTORE)
-- 
2.35.1

Re: [PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[Heinrich Schuchardt]

On 3/22/22 00:22, Daniel Golle wrote:
> It can be useful for the OS (Linux) to know which configuration has
> been chosen by U-Boot when launching a FIT image.
> Store the name of the FIT configuration node used in a new string
> attribute called 'bootconf' in the '/chosen' node in device tree.

Please, point out where you want to use that information. I cannot see
that the Linux kernel will make any use of it.

>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   boot/image-fdt.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 692a9ad3e4..4017bc94a6 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -601,6 +601,12 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
>   		goto err;
>   	}
>
> +	/* Store name of configuration node as bootconf in /chosen node */
> +	if (images->fit_uname_cfg)
> +		fdt_find_and_setprop(blob, "/chosen", "bootconf",
> +					images->fit_uname_cfg,
> +					strlen(images->fit_uname_cfg) + 1, 1);
> +

We should not inject arbitrary properties into the device-tree. Where is
the property /chosen/bootconf defined in the Linux documentation or in
the device-tree specification?

Best regards

Heinrich

>   	/* Update ethernet nodes */
>   	fdt_fixup_ethernet(blob);
>   #if CONFIG_IS_ENABLED(CMD_PSTORE)

Re: [PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[Daniel Golle]

Hi Heinrich,

On Tue, Mar 22, 2022 at 09:29:54AM +0100, Heinrich Schuchardt wrote:
> On 3/22/22 00:22, Daniel Golle wrote:
> > It can be useful for the OS (Linux) to know which configuration has
> > been chosen by U-Boot when launching a FIT image.
> > Store the name of the FIT configuration node used in a new string
> > attribute called 'bootconf' in the '/chosen' node in device tree.
> 
> Please, point out where you want to use that information. I cannot see
> that the Linux kernel will make any use of it.

It can be useful when parsing FIT image in kernel, but can also be
useful in userspace (via /sys/firmware/devicetree/base/chosen/bootconf).
For OpenWrt I've written a uImage.FIT partition parser in order to
allow a single type of firmware image containing kernel, dtb and rootfs
to work accross all storage types (SPI-NOR/mtd, NAND/ubi, eMMC/block).

I haven't yet submitted it upstream, exactly because some edges like
selection of configuration would need to be discussed first.

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/block/partitions/fit.c;h=89b5fb3454f0b69cdcfe53958169fb2a6f9c48a9;hb=HEAD

However, it's doing the job for a bunch of OpenWrt boards already,
incl. some popular devices

https://openwrt.org/toh/linksys/e8450
(boots from SPI-NAND/ubi)

https://openwrt.org/toh/sinovoip/bananapi_bpi-r64_v1.1
(can boot from SD Card, eMMC or SPI-NAND/ubi)

Using uImage.FIT to store the squashfs besides kernel and dtb has
several obvious advantages which are hard to obtain in any other way:
 * single image accross NOR/mtd, NAND/ubi and MMC/block devices
 * dynamically sized sub-partitions for kernel and rootfs
 * hash also for rootfs checked by U-Boot before launching kernel
 * images may include additional filesystems e.g. for localization or
   brandinge (hence the need to parse the boot configuration when
   mapping the 'filesystem' subimages into block partitions).

> 
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >   boot/image-fdt.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 692a9ad3e4..4017bc94a6 100644
> > --- a/boot/image-fdt.c
> > +++ b/boot/image-fdt.c
> > @@ -601,6 +601,12 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
> >   		goto err;
> >   	}
> > 
> > +	/* Store name of configuration node as bootconf in /chosen node */
> > +	if (images->fit_uname_cfg)
> > +		fdt_find_and_setprop(blob, "/chosen", "bootconf",
> > +					images->fit_uname_cfg,
> > +					strlen(images->fit_uname_cfg) + 1, 1);
> > +
> 
> We should not inject arbitrary properties into the device-tree. Where is
> the property /chosen/bootconf defined in the Linux documentation or in
> the device-tree specification?

From Linux Documenation (chosen.txt):
"The chosen node does not represent a real device, but serves as a place
 for passing data between firmware and the operating system, like boot
 arguments."

From Device Tree specification (Release v0.3):
"The /chosen node does not represent a real device in the system but
 describes parameters chosen or specified by the system firmware at run
 time."

So to me it sounds like the right place for an additional attribute
serving the above purpose. It could as well become an additional kernel
cmdline argument in (ie. appended to 'bootargs' in '/chosen'), but that
seemed more complex and harder to deal with.

The name could be changed into something like 'denx,boot-configuration'
if 'bootconf' is perceived to be too generic.


Cheers


Daniel

Re: [PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[Heinrich Schuchardt]

On 3/22/22 14:18, Daniel Golle wrote:
> Hi Heinrich,
>
> On Tue, Mar 22, 2022 at 09:29:54AM +0100, Heinrich Schuchardt wrote:
>> On 3/22/22 00:22, Daniel Golle wrote:
>>> It can be useful for the OS (Linux) to know which configuration has
>>> been chosen by U-Boot when launching a FIT image.
>>> Store the name of the FIT configuration node used in a new string
>>> attribute called 'bootconf' in the '/chosen' node in device tree.
>>
>> Please, point out where you want to use that information. I cannot see
>> that the Linux kernel will make any use of it.
>
> It can be useful when parsing FIT image in kernel, but can also be
> useful in userspace (via /sys/firmware/devicetree/base/chosen/bootconf).
> For OpenWrt I've written a uImage.FIT partition parser in order to
> allow a single type of firmware image containing kernel, dtb and rootfs
> to work accross all storage types (SPI-NOR/mtd, NAND/ubi, eMMC/block).
>
> I haven't yet submitted it upstream, exactly because some edges like
> selection of configuration would need to be discussed first.
>
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/block/partitions/fit.c;h=89b5fb3454f0b69cdcfe53958169fb2a6f9c48a9;hb=HEAD
>
> However, it's doing the job for a bunch of OpenWrt boards already,
> incl. some popular devices
>
> https://openwrt.org/toh/linksys/e8450
> (boots from SPI-NAND/ubi)
>
> https://openwrt.org/toh/sinovoip/bananapi_bpi-r64_v1.1
> (can boot from SD Card, eMMC or SPI-NAND/ubi)
>
> Using uImage.FIT to store the squashfs besides kernel and dtb has
> several obvious advantages which are hard to obtain in any other way:
>   * single image accross NOR/mtd, NAND/ubi and MMC/block devices
>   * dynamically sized sub-partitions for kernel and rootfs
>   * hash also for rootfs checked by U-Boot before launching kernel
>   * images may include additional filesystems e.g. for localization or
>     brandinge (hence the need to parse the boot configuration when
>     mapping the 'filesystem' subimages into block partitions).
>
>>
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>    boot/image-fdt.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index 692a9ad3e4..4017bc94a6 100644
>>> --- a/boot/image-fdt.c
>>> +++ b/boot/image-fdt.c
>>> @@ -601,6 +601,12 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
>>>    		goto err;
>>>    	}
>>>
>>> +	/* Store name of configuration node as bootconf in /chosen node */
>>> +	if (images->fit_uname_cfg)
>>> +		fdt_find_and_setprop(blob, "/chosen", "bootconf",
>>> +					images->fit_uname_cfg,
>>> +					strlen(images->fit_uname_cfg) + 1, 1);
>>> +
>>
>> We should not inject arbitrary properties into the device-tree. Where is
>> the property /chosen/bootconf defined in the Linux documentation or in
>> the device-tree specification?
>
>>From Linux Documenation (chosen.txt):
> "The chosen node does not represent a real device, but serves as a place
>   for passing data between firmware and the operating system, like boot
>   arguments."
>
>>From Device Tree specification (Release v0.3):
> "The /chosen node does not represent a real device in the system but
>   describes parameters chosen or specified by the system firmware at run
>   time."
>
> So to me it sounds like the right place for an additional attribute
> serving the above purpose. It could as well become an additional kernel
> cmdline argument in (ie. appended to 'bootargs' in '/chosen'), but that
> seemed more complex and harder to deal with.
>
> The name could be changed into something like 'denx,boot-configuration'
> if 'bootconf' is perceived to be too generic.

Is this squashfs meant to be served from RAM?
Should U-Boot check a signature of the squashfs?
Should U-Boot already load it into RAM?

Why can't you pack the squashfs into an initrd.img file and do without
any modification?

Best regards

Heinrich

>
> Cheers
>
> Daniel

Re: [PATCH] image-fdt: save name of FIT configuration in '/chosen' node

[Daniel Golle]

On Tue, Mar 22, 2022 at 10:09:38PM +0100, Heinrich Schuchardt wrote:
> On 3/22/22 14:18, Daniel Golle wrote:
> > Hi Heinrich,
> > 
> > On Tue, Mar 22, 2022 at 09:29:54AM +0100, Heinrich Schuchardt wrote:
> > > On 3/22/22 00:22, Daniel Golle wrote:
> > > > It can be useful for the OS (Linux) to know which configuration has
> > > > been chosen by U-Boot when launching a FIT image.
> > > > Store the name of the FIT configuration node used in a new string
> > > > attribute called 'bootconf' in the '/chosen' node in device tree.
> > > 
> > > Please, point out where you want to use that information. I cannot see
> > > that the Linux kernel will make any use of it.
> > 
> > It can be useful when parsing FIT image in kernel, but can also be
> > useful in userspace (via /sys/firmware/devicetree/base/chosen/bootconf).
> > For OpenWrt I've written a uImage.FIT partition parser in order to
> > allow a single type of firmware image containing kernel, dtb and rootfs
> > to work accross all storage types (SPI-NOR/mtd, NAND/ubi, eMMC/block).
> > 
> > I haven't yet submitted it upstream, exactly because some edges like
> > selection of configuration would need to be discussed first.
> > 
> > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/block/partitions/fit.c;h=89b5fb3454f0b69cdcfe53958169fb2a6f9c48a9;hb=HEAD
> > 
> > However, it's doing the job for a bunch of OpenWrt boards already,
> > incl. some popular devices
> > 
> > https://openwrt.org/toh/linksys/e8450
> > (boots from SPI-NAND/ubi)
> > 
> > https://openwrt.org/toh/sinovoip/bananapi_bpi-r64_v1.1
> > (can boot from SD Card, eMMC or SPI-NAND/ubi)
> > 
> > Using uImage.FIT to store the squashfs besides kernel and dtb has
> > several obvious advantages which are hard to obtain in any other way:
> >   * single image accross NOR/mtd, NAND/ubi and MMC/block devices
> >   * dynamically sized sub-partitions for kernel and rootfs
> >   * hash also for rootfs checked by U-Boot before launching kernel
> >   * images may include additional filesystems e.g. for localization or
> >     brandinge (hence the need to parse the boot configuration when
> >     mapping the 'filesystem' subimages into block partitions).
> > 
> > > 
> > > > 
> > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > > > ---
> > > >    boot/image-fdt.c | 6 ++++++
> > > >    1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > > > index 692a9ad3e4..4017bc94a6 100644
> > > > --- a/boot/image-fdt.c
> > > > +++ b/boot/image-fdt.c
> > > > @@ -601,6 +601,12 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
> > > >    		goto err;
> > > >    	}
> > > > 
> > > > +	/* Store name of configuration node as bootconf in /chosen node */
> > > > +	if (images->fit_uname_cfg)
> > > > +		fdt_find_and_setprop(blob, "/chosen", "bootconf",
> > > > +					images->fit_uname_cfg,
> > > > +					strlen(images->fit_uname_cfg) + 1, 1);
> > > > +
> > > 
> > > We should not inject arbitrary properties into the device-tree. Where is
> > > the property /chosen/bootconf defined in the Linux documentation or in
> > > the device-tree specification?
> > 
> > > From Linux Documenation (chosen.txt):
> > "The chosen node does not represent a real device, but serves as a place
> >   for passing data between firmware and the operating system, like boot
> >   arguments."
> > 
> > > From Device Tree specification (Release v0.3):
> > "The /chosen node does not represent a real device in the system but
> >   describes parameters chosen or specified by the system firmware at run
> >   time."
> > 
> > So to me it sounds like the right place for an additional attribute
> > serving the above purpose. It could as well become an additional kernel
> > cmdline argument in (ie. appended to 'bootargs' in '/chosen'), but that
> > seemed more complex and harder to deal with.
> > 
> > The name could be changed into something like 'denx,boot-configuration'
> > if 'bootconf' is perceived to be too generic.
> 
> Is this squashfs meant to be served from RAM?

No, it is part of the FIT image which lives on a block device (which
is typically either a partition on a mmc block device, a ubiblock
device or a mtdblock device, depending on the storage type used).
An additional partition parser (metioned above) then breaks out the
configured filesystem parts into sub-partitions in a generic way, so
the kernel can mount the rootfs (and later on maybe other additional
filesystems e.g. with firmware localization or customization).

> Should U-Boot check a signature of the squashfs?

Yes, as failing to mount it will result in an unusable system,
U-Boot should validate the rootfs before launching the kernel (and
dual-boot into a recovery system in case anything in the FIT image
got damaged).

> Should U-Boot already load it into RAM?

No, that's unneeded. The rootfs can be large and serving the compressed
image from RAM would result in permanently allocating the space needed
for that. (think: total FIT size ~40MiB, total RAM 256MiB; 40MiB
additional allocation is about 15% of all the RAM there is, and that's
too much)

> Why can't you pack the squashfs into an initrd.img file and do without
> any modification?

Because this is not about systems running off a ramdisk or even having
an initrd at all. But that also doesn't matter too much at this point:
Even if we would solve selecting the (large) rootfs sub-image with a
(small) initrd userspace process, also that userspace process would
need to know which boot configuration has been selected, so it could
select the configured 'filesystem' sub-image from FIT (which is stored
on some permanent storage like raw NOR or NAND, or MMC).


Thank you for reviewing!


Cheers


Daniel