Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

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

Hi All,

While trying to bring up Falcon Mode boot on an ARM64 board, I've
discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
raw kernel image and subsequently call the bootz_setup() function which
resides in spl_parse_image_header().

I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
will skip the mkimage header check, allowing the subsequent
spl_parse_image_header() call to successfully fall through to bootz_setup()
and load/boot a raw kernel image.

Sincerely,
Nathan Barrett-Morrison

[-- Attachment #2: 0001-Allow-Falcon-Mode-boot-to-use-raw-kernel-image-when-.patch --]
[-- Type: text/x-patch, Size: 1279 bytes --]

From e5a15a8ad2fd007e6d8d48dd64767d194bbd1833 Mon Sep 17 00:00:00 2001
From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Date: Mon, 17 Jan 2022 19:42:59 -0500
Subject: [PATCH] Allow Falcon Mode boot to use raw kernel image when
 booting via SPI.  When using Falcon Mode boot with a raw, unwrapped kernel
 image, the bootz_setup() call inside of spl_parse_image_header() is
 unreachable because the mkimage header magic check will never pass.  Adding
 CONFIG_SYS_SPI_KERNEL_SKIP_HEADER gives us the ability to pass through to the
 desired bootz_setup() call.

Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Aneesh V <aneesh@ti.com>
---
 common/spl/spl_spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 4e20a23dea..62dad1d2fb 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -33,8 +33,10 @@ static int spi_load_image_os(struct spl_image_info *spl_image,
 	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
 		       (void *)header);
 
+#ifndef CONFIG_SYS_SPI_KERNEL_SKIP_HEADER
 	if (image_get_magic(header) != IH_MAGIC)
 		return -1;
+#endif
 
 	err = spl_parse_image_header(spl_image, header);
 	if (err)
-- 
2.34.1

Re: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Tom Rini]

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

On Mon, Jan 17, 2022 at 08:02:58PM -0500, Nathan Barrett-Morrison wrote:

> Hi All,
> 
> While trying to bring up Falcon Mode boot on an ARM64 board, I've
> discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
> raw kernel image and subsequently call the bootz_setup() function which
> resides in spl_parse_image_header().
> 
> I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
> will skip the mkimage header check, allowing the subsequent
> spl_parse_image_header() call to successfully fall through to bootz_setup()
> and load/boot a raw kernel image.
> 
> Sincerely,
> Nathan Barrett-Morrison

> From e5a15a8ad2fd007e6d8d48dd64767d194bbd1833 Mon Sep 17 00:00:00 2001
> From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Date: Mon, 17 Jan 2022 19:42:59 -0500
> Subject: [PATCH] Allow Falcon Mode boot to use raw kernel image when
>  booting via SPI.  When using Falcon Mode boot with a raw, unwrapped kernel
>  image, the bootz_setup() call inside of spl_parse_image_header() is
>  unreachable because the mkimage header magic check will never pass.  Adding
>  CONFIG_SYS_SPI_KERNEL_SKIP_HEADER gives us the ability to pass through to the
>  desired bootz_setup() call.
> 
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Aneesh V <aneesh@ti.com>
> ---
>  common/spl/spl_spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index 4e20a23dea..62dad1d2fb 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -33,8 +33,10 @@ static int spi_load_image_os(struct spl_image_info *spl_image,
>  	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
>  		       (void *)header);
>  
> +#ifndef CONFIG_SYS_SPI_KERNEL_SKIP_HEADER
>  	if (image_get_magic(header) != IH_MAGIC)
>  		return -1;
> +#endif
>  
>  	err = spl_parse_image_header(spl_image, header);
>  	if (err)

I'm not sure this is the right path.  Why is this part of the code
checking for IH_MAGIC at all, and should it not instead allow for
graceful falling back so that zImage/Image/etc work?

-- 
Tom

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

Re: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

Hi Tom,

Yes -- I believe this section of code should not be checking for IH_MAGIC
at all, as parse_image_header does it as well.

Would you like me to resubmit this patch with the offending lines deleted?

Sincerely,
Nathan

On Mon, Jan 31, 2022 at 10:14 AM Tom Rini <trini@konsulko.com> wrote:

> On Mon, Jan 17, 2022 at 08:02:58PM -0500, Nathan Barrett-Morrison wrote:
>
> > Hi All,
> >
> > While trying to bring up Falcon Mode boot on an ARM64 board, I've
> > discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
> > raw kernel image and subsequently call the bootz_setup() function which
> > resides in spl_parse_image_header().
> >
> > I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
> > will skip the mkimage header check, allowing the subsequent
> > spl_parse_image_header() call to successfully fall through to
> bootz_setup()
> > and load/boot a raw kernel image.
> >
> > Sincerely,
> > Nathan Barrett-Morrison
>
> > From e5a15a8ad2fd007e6d8d48dd64767d194bbd1833 Mon Sep 17 00:00:00 2001
> > From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> > Date: Mon, 17 Jan 2022 19:42:59 -0500
> > Subject: [PATCH] Allow Falcon Mode boot to use raw kernel image when
> >  booting via SPI.  When using Falcon Mode boot with a raw, unwrapped
> kernel
> >  image, the bootz_setup() call inside of spl_parse_image_header() is
> >  unreachable because the mkimage header magic check will never pass.
> Adding
> >  CONFIG_SYS_SPI_KERNEL_SKIP_HEADER gives us the ability to pass through
> to the
> >  desired bootz_setup() call.
> >
> > Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Aneesh V <aneesh@ti.com>
> > ---
> >  common/spl/spl_spi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> > index 4e20a23dea..62dad1d2fb 100644
> > --- a/common/spl/spl_spi.c
> > +++ b/common/spl/spl_spi.c
> > @@ -33,8 +33,10 @@ static int spi_load_image_os(struct spl_image_info
> *spl_image,
> >       spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
> >                      (void *)header);
> >
> > +#ifndef CONFIG_SYS_SPI_KERNEL_SKIP_HEADER
> >       if (image_get_magic(header) != IH_MAGIC)
> >               return -1;
> > +#endif
> >
> >       err = spl_parse_image_header(spl_image, header);
> >       if (err)
>
> I'm not sure this is the right path.  Why is this part of the code
> checking for IH_MAGIC at all, and should it not instead allow for
> graceful falling back so that zImage/Image/etc work?
>
> --
> Tom
>

Re: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Tom Rini]

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

On Mon, Jan 31, 2022 at 10:16:08AM -0500, Nathan Barrett-Morrison wrote:

> Hi Tom,
> 
> Yes -- I believe this section of code should not be checking for IH_MAGIC
> at all, as parse_image_header does it as well.
> 
> Would you like me to resubmit this patch with the offending lines deleted?

Well, please take a good look over the whole area of code and see if it
really makes sense to do what it's doing there or not.

-- 
Tom

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

Re: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

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

Hi Tom,

I've attached v2 of this patch.  I've also confirmed again that the
offending lines are entirely unnecessary and amended the patch to delete
them instead.

Sincerely,
Nathan

On Mon, Jan 31, 2022 at 10:23 AM Tom Rini <trini@konsulko.com> wrote:

> On Mon, Jan 31, 2022 at 10:16:08AM -0500, Nathan Barrett-Morrison wrote:
>
> > Hi Tom,
> >
> > Yes -- I believe this section of code should not be checking for IH_MAGIC
> > at all, as parse_image_header does it as well.
> >
> > Would you like me to resubmit this patch with the offending lines
> deleted?
>
> Well, please take a good look over the whole area of code and see if it
> really makes sense to do what it's doing there or not.
>
> --
> Tom
>

[-- Attachment #2: 0001-Allow-Falcon-Mode-boot-to-use-raw-kernel-image-when-.patch --]
[-- Type: text/x-patch, Size: 1410 bytes --]

From 65e75debf7813760bf192b477818093e35909081 Mon Sep 17 00:00:00 2001
From: Nathan Barrett Morrison <nathan.morrison@timesys.com>
Date: Wed, 2 Feb 2022 16:56:50 -0500
Subject: [PATCH v2] Allow Falcon Mode boot to use raw kernel image when booting
 via SPI.

When using Falcon Mode boot with a raw, unwrapped kernel image, the bootz_setup() call inside of spl_parse_image_header() is
unreachable because the mkimage header magic check in spi_load_image_os() will never pass.  This check is entirely redundant and unnecessary,
as the spl_parse_image_header() call will also check for IH_MAGIC.

Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Cc: Tom Rini <trini@konsulko.com>
---
Changes for v2:
   - Remove proposed CONFIG_SYS_SPI_KERNEL_SKIP_HEADER option, as we've determined the entire check is redundant and unnecessary.  Just delete it instead.

 common/spl/spl_spi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 4e20a23dea..16e268be50 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -33,9 +33,6 @@ static int spi_load_image_os(struct spl_image_info *spl_image,
 	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
 		       (void *)header);
 
-	if (image_get_magic(header) != IH_MAGIC)
-		return -1;
-
 	err = spl_parse_image_header(spl_image, header);
 	if (err)
 		return err;
-- 
2.30.2