[RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[Elena Popa]

When Falcon Mode is enabled, spl_start_uboot() function must be defined. If this function returns 0 then SPL should start the kernel, if it returns 1 then U-Boot must be started. 

When spl_start_uboot() returns 1, then U-Boot must be loaded, and as far as I can tell spl_image->os should be set to IH_OS_U_BOOT before:

	if (os_takes_devicetree(spl_image->os)) {
		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);

in common/spl/spl_fit.c/ int spl_load_simple_fit(...)

this is not the case currently, and the loading fails. If set manually spl_image->os = IH_OS_U_BOOT, it works fine.

The only place I could see where it could be set is a few lines above in:

if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
	debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
	spl_image->os = IH_OS_U_BOOT;

but the second "if" does not execute, obviously (since we are in falcon, CONFIG_SPL_OS_BOOT is enabled). I see a few options, but I am not sure how best to fix this:

1) add something like:
else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT) || spl_start_uboot())
	spl_image->os = IH_OS_U_BOOT;
but this would mean calling spl_start_uboot() twice, requiring it to return the same value. It works in my case.

2) set spl_image->os to IH_OS_U_BOOT when the spl_start_uboot() is called the first time, in:
common/spl/spl_mmc.c/spl_mmc_do_fs_boot(..)    - in several places:
#ifdef CONFIG_SPL_FS_FAT
	if (!spl_start_uboot()) {
		err = spl_load_image_fat_os(spl_image, bootdev, mmc_get_blk_desc(mmc),
...

#ifdef CONFIG_SPL_FS_EXT4
	if (!spl_start_uboot()) {
		err = spl_load_image_ext_os(spl_image, bootdev, mmc_get_blk_desc(mmc),

...

Any insight on how to proceed? I've implemented spl_start_uboot() to decide on whether to boot the kernel or the U-Boot based on key presses on the console.

Thank you,
Elena. 

Re: [RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

On Tue, Aug 08, 2023 at 03:40:44PM +0000, Elena Popa wrote:
> When Falcon Mode is enabled, spl_start_uboot() function must be defined. If this function returns 0 then SPL should start the kernel, if it returns 1 then U-Boot must be started. 
> 
> When spl_start_uboot() returns 1, then U-Boot must be loaded, and as far as I can tell spl_image->os should be set to IH_OS_U_BOOT before:
> 
> 	if (os_takes_devicetree(spl_image->os)) {
> 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> 
> in common/spl/spl_fit.c/ int spl_load_simple_fit(...)
> 
> this is not the case currently, and the loading fails. If set manually spl_image->os = IH_OS_U_BOOT, it works fine.
> 
> The only place I could see where it could be set is a few lines above in:
> 
> if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
> 	debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
> else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
> 	spl_image->os = IH_OS_U_BOOT;
> 
> but the second "if" does not execute, obviously (since we are in falcon, CONFIG_SPL_OS_BOOT is enabled). I see a few options, but I am not sure how best to fix this:
> 
> 1) add something like:
> else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT) || spl_start_uboot())
> 	spl_image->os = IH_OS_U_BOOT;
> but this would mean calling spl_start_uboot() twice, requiring it to return the same value. It works in my case.
> 
> 2) set spl_image->os to IH_OS_U_BOOT when the spl_start_uboot() is called the first time, in:
> common/spl/spl_mmc.c/spl_mmc_do_fs_boot(..)    - in several places:
> #ifdef CONFIG_SPL_FS_FAT
> 	if (!spl_start_uboot()) {
> 		err = spl_load_image_fat_os(spl_image, bootdev, mmc_get_blk_desc(mmc),
> ...
> 
> #ifdef CONFIG_SPL_FS_EXT4
> 	if (!spl_start_uboot()) {
> 		err = spl_load_image_ext_os(spl_image, bootdev, mmc_get_blk_desc(mmc),
> 
> ...
> 
> Any insight on how to proceed? I've implemented spl_start_uboot() to decide on whether to boot the kernel or the U-Boot based on key presses on the console.

Are you talking about the case where we tried to load (or verify) the
kernel image and that failed, and so should fall back to U-Boot? Or the
case where we're going to U-Boot instead of trying the kernel. For the
second case there, we should be doing the same thing we do in the case
of not-falcon mode. In the first case however, we might well have a bug
or two there and aren't falling back to reporting the error and then
loading U-Boot just like we hadn't tried to load the kernel before.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [EXT] Re: [RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[Elena Popa]

> > When Falcon Mode is enabled, spl_start_uboot() function must be defined.
> If this function returns 0 then SPL should start the kernel, if it returns 1 then
> U-Boot must be started.
> >
> > When spl_start_uboot() returns 1, then U-Boot must be loaded, and as far
> as I can tell spl_image->os should be set to IH_OS_U_BOOT before:
> >
> > 	if (os_takes_devicetree(spl_image->os)) {
> > 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> >
> > in common/spl/spl_fit.c/ int spl_load_simple_fit(...)
> >
> > this is not the case currently, and the loading fails. If set manually
> spl_image->os = IH_OS_U_BOOT, it works fine.
> >
> > The only place I could see where it could be set is a few lines above in:
> >
> > if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
> > 	debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
> > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
> > 	spl_image->os = IH_OS_U_BOOT;
> >
> > but the second "if" does not execute, obviously (since we are in falcon,
> CONFIG_SPL_OS_BOOT is enabled). I see a few options, but I am not sure how
> best to fix this:
> >
> > 1) add something like:
> > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT) || spl_start_uboot())
> > 	spl_image->os = IH_OS_U_BOOT;
> > but this would mean calling spl_start_uboot() twice, requiring it to return
> the same value. It works in my case.
> >
> > 2) set spl_image->os to IH_OS_U_BOOT when the spl_start_uboot() is called
> the first time, in:
> > common/spl/spl_mmc.c/spl_mmc_do_fs_boot(..)    - in several places:
> > #ifdef CONFIG_SPL_FS_FAT
> > 	if (!spl_start_uboot()) {
> > 		err = spl_load_image_fat_os(spl_image, bootdev,
> > mmc_get_blk_desc(mmc), ...
> >
> > #ifdef CONFIG_SPL_FS_EXT4
> > 	if (!spl_start_uboot()) {
> > 		err = spl_load_image_ext_os(spl_image, bootdev,
> > mmc_get_blk_desc(mmc),
> >
> > ...
> >
> > Any insight on how to proceed? I've implemented spl_start_uboot() to
> decide on whether to boot the kernel or the U-Boot based on key presses on
> the console.
> 
> Are you talking about the case where we tried to load (or verify) the kernel
> image and that failed, and so should fall back to U-Boot? Or the case where
> we're going to U-Boot instead of trying the kernel. For the second case there,
> we should be doing the same thing we do in the case of not-falcon mode. In
> the first case however, we might well have a bug or two there and aren't
> falling back to reporting the error and then loading U-Boot just like we hadn't
> tried to load the kernel before.
> 
> --
> Tom

I was referring to the second case: we are in Falcon Mode, but spl_start_uboot() returns 1, so we should load U-Boot instead of the Kernel. However, I found a solution that works with the current implementation: adding the "os" property into the u-boot node from U-Boot FIT image. Then, spl_fit_image_get_os(...) sets spl_image->os correctly.

Re: [EXT] Re: [RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]

On Fri, Aug 18, 2023 at 05:00:05PM +0000, Elena Popa wrote:
> 
> 
> > > When Falcon Mode is enabled, spl_start_uboot() function must be defined.
> > If this function returns 0 then SPL should start the kernel, if it returns 1 then
> > U-Boot must be started.
> > >
> > > When spl_start_uboot() returns 1, then U-Boot must be loaded, and as far
> > as I can tell spl_image->os should be set to IH_OS_U_BOOT before:
> > >
> > > 	if (os_takes_devicetree(spl_image->os)) {
> > > 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > >
> > > in common/spl/spl_fit.c/ int spl_load_simple_fit(...)
> > >
> > > this is not the case currently, and the loading fails. If set manually
> > spl_image->os = IH_OS_U_BOOT, it works fine.
> > >
> > > The only place I could see where it could be set is a few lines above in:
> > >
> > > if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
> > > 	debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
> > > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
> > > 	spl_image->os = IH_OS_U_BOOT;
> > >
> > > but the second "if" does not execute, obviously (since we are in falcon,
> > CONFIG_SPL_OS_BOOT is enabled). I see a few options, but I am not sure how
> > best to fix this:
> > >
> > > 1) add something like:
> > > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT) || spl_start_uboot())
> > > 	spl_image->os = IH_OS_U_BOOT;
> > > but this would mean calling spl_start_uboot() twice, requiring it to return
> > the same value. It works in my case.
> > >
> > > 2) set spl_image->os to IH_OS_U_BOOT when the spl_start_uboot() is called
> > the first time, in:
> > > common/spl/spl_mmc.c/spl_mmc_do_fs_boot(..)    - in several places:
> > > #ifdef CONFIG_SPL_FS_FAT
> > > 	if (!spl_start_uboot()) {
> > > 		err = spl_load_image_fat_os(spl_image, bootdev,
> > > mmc_get_blk_desc(mmc), ...
> > >
> > > #ifdef CONFIG_SPL_FS_EXT4
> > > 	if (!spl_start_uboot()) {
> > > 		err = spl_load_image_ext_os(spl_image, bootdev,
> > > mmc_get_blk_desc(mmc),
> > >
> > > ...
> > >
> > > Any insight on how to proceed? I've implemented spl_start_uboot() to
> > decide on whether to boot the kernel or the U-Boot based on key presses on
> > the console.
> > 
> > Are you talking about the case where we tried to load (or verify) the kernel
> > image and that failed, and so should fall back to U-Boot? Or the case where
> > we're going to U-Boot instead of trying the kernel. For the second case there,
> > we should be doing the same thing we do in the case of not-falcon mode. In
> > the first case however, we might well have a bug or two there and aren't
> > falling back to reporting the error and then loading U-Boot just like we hadn't
> > tried to load the kernel before.
> 
> I was referring to the second case: we are in Falcon Mode, but
> spl_start_uboot() returns 1, so we should load U-Boot instead of the
> Kernel. However, I found a solution that works with the current
> implementation: adding the "os" property into the u-boot node from
> U-Boot FIT image. Then, spl_fit_image_get_os(...) sets spl_image->os
> correctly.

Ah, OK.  I think this may be partly that the fall-back case wasn't
tested as much with FIT images as it was with legacy ones at the time,
so it might well be that the code needs a bit of fixing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]