[PATCH] spl: return kernel image header size in os boot

[PATCH] spl: return kernel image header size in os boot

[Anshul Dalal]

During kernel build process the header size is computed including the
BSS whereas it's removed when creating the uncompressed image. Therefore
the size of the uncompressed image on filesystem will be smaller than
the size specifiedin the header.

Therefore it makes sense to return the header size back instead of the
file size in falcon boot.

More info:
https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/

Signed-off-by: Anshul Dalal <anshuld@ti.com>
---
 common/spl/spl_ext.c | 4 ++++
 common/spl/spl_fat.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
index c5478820a9b..6d8d6544092 100644
--- a/common/spl/spl_ext.c
+++ b/common/spl/spl_ext.c
@@ -17,6 +17,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
 	ret = ext4fs_read(buf, file_offset, size, &actlen);
 	if (ret)
 		return ret;
+
+	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
+		return size;
+
 	return actlen;
 }
 
diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
index fce451b7664..ddf85e2cece 100644
--- a/common/spl/spl_fat.c
+++ b/common/spl/spl_fat.c
@@ -55,6 +55,9 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
 	if (ret)
 		return ret;
 
+	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
+		return size;
+
 	return actread;
 }
 
-- 
2.43.0

Re: [PATCH] spl: return kernel image header size in os boot

[Tom Rini]

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

On Fri, Feb 21, 2025 at 08:47:51PM +0530, Anshul Dalal wrote:

> During kernel build process the header size is computed including the
> BSS whereas it's removed when creating the uncompressed image. Therefore
> the size of the uncompressed image on filesystem will be smaller than
> the size specifiedin the header.
> 
> Therefore it makes sense to return the header size back instead of the
> file size in falcon boot.
> 
> More info:
> https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/
> 
> Signed-off-by: Anshul Dalal <anshuld@ti.com>
> ---
>  common/spl/spl_ext.c | 4 ++++
>  common/spl/spl_fat.c | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> index c5478820a9b..6d8d6544092 100644
> --- a/common/spl/spl_ext.c
> +++ b/common/spl/spl_ext.c
> @@ -17,6 +17,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
>  	ret = ext4fs_read(buf, file_offset, size, &actlen);
>  	if (ret)
>  		return ret;
> +
> +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> +		return size;
> +
>  	return actlen;
>  }
>  
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index fce451b7664..ddf85e2cece 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -55,6 +55,9 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
>  	if (ret)
>  		return ret;
>  
> +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> +		return size;
> +
>  	return actread;
>  }

Some image formats include information about the BSS as well, and others
do not. But this is just having some parts of SPL return the filesize
and not doing some deeper probing. Given that usually problems like this
arise from loading contents in to memory too close together and then
getting wiped out by the kernel BSS, what's the problem situation you've
run in to, where, and with what loaded where in memory?

-- 
Tom

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

Re: [EXTERNAL] Re: [PATCH] spl: return kernel image header size in os boot

[Anshul Dalal]

On Sat Feb 22, 2025 at 1:34 AM IST, Tom Rini wrote:
> On Fri, Feb 21, 2025 at 08:47:51PM +0530, Anshul Dalal wrote:
>
> > During kernel build process the header size is computed including the
> > BSS whereas it's removed when creating the uncompressed image. Therefore
> > the size of the uncompressed image on filesystem will be smaller than
> > the size specifiedin the header.
> > 
> > Therefore it makes sense to return the header size back instead of the
> > file size in falcon boot.
> > 
> > More info:
> > https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/
> > 
> > Signed-off-by: Anshul Dalal <anshuld@ti.com>
> > ---
> >  common/spl/spl_ext.c | 4 ++++
> >  common/spl/spl_fat.c | 3 +++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> > index c5478820a9b..6d8d6544092 100644
> > --- a/common/spl/spl_ext.c
> > +++ b/common/spl/spl_ext.c
> > @@ -17,6 +17,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> >  	ret = ext4fs_read(buf, file_offset, size, &actlen);
> >  	if (ret)
> >  		return ret;
> > +
> > +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> > +		return size;
> > +
> >  	return actlen;
> >  }
> >  
> > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> > index fce451b7664..ddf85e2cece 100644
> > --- a/common/spl/spl_fat.c
> > +++ b/common/spl/spl_fat.c
> > @@ -55,6 +55,9 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> > +		return size;
> > +
> >  	return actread;
> >  }
>
> Some image formats include information about the BSS as well, and others
> do not. But this is just having some parts of SPL return the filesize
> and not doing some deeper probing. Given that usually problems like this
> arise from loading contents in to memory too close together and then
> getting wiped out by the kernel BSS, what's the problem situation you've
> run in to, where, and with what loaded where in memory?

The problem is not about the allocation of BSS section but when loading
an uncompressed kernel image with the check in _spl_load at
`include/spl_load.h:95` which fails to -EIO even when the image is loaded
properly because it compares the size form the header to the size of the
file read from the FS:

	return read < spl_image->size ? -EIO : 0;

This doesn't work for uncompressed kernel image because the size from
the header (the parameter ulong size for spl_fit_read) includes BSS
which is not part of the final image file. And there is no way of
knowing the file size based on the header. Hence why we check for
CONFIG_CMD_BOOTI for returning the header size directly.

Although it might be better to have a different function altogether for
reading uncompressed image that can be passed to spl_load_init so we are
not using spl_**fit**_read to read non fit images.

You can find the bug report here:
https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/

- Anshul

Re: [EXTERNAL] Re: [PATCH] spl: return kernel image header size in os boot

[Tom Rini]

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

On Sat, Feb 22, 2025 at 04:05:55PM +0530, Anshul Dalal wrote:
> On Sat Feb 22, 2025 at 1:34 AM IST, Tom Rini wrote:
> > On Fri, Feb 21, 2025 at 08:47:51PM +0530, Anshul Dalal wrote:
> >
> > > During kernel build process the header size is computed including the
> > > BSS whereas it's removed when creating the uncompressed image. Therefore
> > > the size of the uncompressed image on filesystem will be smaller than
> > > the size specifiedin the header.
> > > 
> > > Therefore it makes sense to return the header size back instead of the
> > > file size in falcon boot.
> > > 
> > > More info:
> > > https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/
> > > 
> > > Signed-off-by: Anshul Dalal <anshuld@ti.com>
> > > ---
> > >  common/spl/spl_ext.c | 4 ++++
> > >  common/spl/spl_fat.c | 3 +++
> > >  2 files changed, 7 insertions(+)
> > > 
> > > diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> > > index c5478820a9b..6d8d6544092 100644
> > > --- a/common/spl/spl_ext.c
> > > +++ b/common/spl/spl_ext.c
> > > @@ -17,6 +17,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> > >  	ret = ext4fs_read(buf, file_offset, size, &actlen);
> > >  	if (ret)
> > >  		return ret;
> > > +
> > > +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> > > +		return size;
> > > +
> > >  	return actlen;
> > >  }
> > >  
> > > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> > > index fce451b7664..ddf85e2cece 100644
> > > --- a/common/spl/spl_fat.c
> > > +++ b/common/spl/spl_fat.c
> > > @@ -55,6 +55,9 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> > > +		return size;
> > > +
> > >  	return actread;
> > >  }
> >
> > Some image formats include information about the BSS as well, and others
> > do not. But this is just having some parts of SPL return the filesize
> > and not doing some deeper probing. Given that usually problems like this
> > arise from loading contents in to memory too close together and then
> > getting wiped out by the kernel BSS, what's the problem situation you've
> > run in to, where, and with what loaded where in memory?
> 
> The problem is not about the allocation of BSS section but when loading
> an uncompressed kernel image with the check in _spl_load at
> `include/spl_load.h:95` which fails to -EIO even when the image is loaded
> properly because it compares the size form the header to the size of the
> file read from the FS:
> 
> 	return read < spl_image->size ? -EIO : 0;
> 
> This doesn't work for uncompressed kernel image because the size from
> the header (the parameter ulong size for spl_fit_read) includes BSS
> which is not part of the final image file. And there is no way of
> knowing the file size based on the header. Hence why we check for
> CONFIG_CMD_BOOTI for returning the header size directly.
> 
> Although it might be better to have a different function altogether for
> reading uncompressed image that can be passed to spl_load_init so we are
> not using spl_**fit**_read to read non fit images.
> 
> You can find the bug report here:
> https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/

Thanks for explaining. You should include a fixes tag and explain a bit
more about the problem being fixed in spl_load, thanks.

-- 
Tom

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

Re: [PATCH] spl: return kernel image header size in os boot

[Sean Anderson]

On 2/21/25 10:17, Anshul Dalal wrote:
> During kernel build process the header size is computed including the
> BSS whereas it's removed when creating the uncompressed image. Therefore
> the size of the uncompressed image on filesystem will be smaller than
> the size specifiedin the header.
> 
> Therefore it makes sense to return the header size back instead of the
> file size in falcon boot.
> 
> More info:
> https://lore.kernel.org/u-boot/20250214111656.2358748-1-anshuld@ti.com/
> 
> Signed-off-by: Anshul Dalal <anshuld@ti.com>
> ---
>   common/spl/spl_ext.c | 4 ++++
>   common/spl/spl_fat.c | 3 +++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> index c5478820a9b..6d8d6544092 100644
> --- a/common/spl/spl_ext.c
> +++ b/common/spl/spl_ext.c
> @@ -17,6 +17,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
>   	ret = ext4fs_read(buf, file_offset, size, &actlen);
>   	if (ret)
>   		return ret;
> +
> +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> +		return size;
> +
>   	return actlen;
>   }
>   
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index fce451b7664..ddf85e2cece 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -55,6 +55,9 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
>   	if (ret)
>   		return ret;
>   
> +	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI))
> +		return size;
> +
>   	return actread;
>   }
>   

Reviewed-by: Sean Anderson <seanga2@gmail.com>