[PATCH] image: fit: Use stack allocation macro

[PATCH] image: fit: Use stack allocation macro

[Joel Stanley]

The documentation above the DEFINE_ALIGN_BUFFER says it's for use
outside functions, but we're inside one.

Instead use ALLOC_CACHE_ALIGN_BUFFER, the stack based macro, which also
includes the cache alignment.

Fixes: b583348ca8c8 ("image: fit: Align hash output buffers")
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
This fixes booting the ast2600-evb image in qemu, which was getting all
zeroes for the output of the FIT hash check.

The 'static' buffer was in BSS but the output image didn't contain a BSS
section. The pointer was left pointing to the text, so the code was
trying to write to the (read only?) text area in SPI NOR memory space.

 tools/mkimage.h  | 3 +--
 boot/image-fit.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/mkimage.h b/tools/mkimage.h
index 7652c8b001c3..f5ca65e2edfd 100644
--- a/tools/mkimage.h
+++ b/tools/mkimage.h
@@ -41,8 +41,7 @@ static inline ulong map_to_sysmem(void *ptr)
 	return (ulong)(uintptr_t)ptr;
 }
 
-#define ARCH_DMA_MINALIGN 1
-#define DEFINE_ALIGN_BUFFER(type, name, size, alugn) type name[size]
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) type name[size]
 
 #define MKIMAGE_TMPFILE_SUFFIX		".tmp"
 #define MKIMAGE_MAX_TMPFILE_LEN		256
diff --git a/boot/image-fit.c b/boot/image-fit.c
index f57d97f55229..df3e5df8836a 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1264,8 +1264,7 @@ int calculate_hash(const void *data, int data_len, const char *name,
 static int fit_image_check_hash(const void *fit, int noffset, const void *data,
 				size_t size, char **err_msgp)
 {
-	DEFINE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN,
-			    ARCH_DMA_MINALIGN);
+	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN);
 	int value_len;
 	const char *algo;
 	uint8_t *fit_value;
-- 
2.35.1

RE: [PATCH] image: fit: Use stack allocation macro

[ChiaWei Wang]

Tested-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

Thanks for the fix.

Driven by the same issue, We also sent another patch moving .BSS section into DRAM.
You may also check it out and any feedback is appreciated.
https://patchwork.ozlabs.org/project/uboot/patch/20220601082115.10799-1-chiawei_wang@aspeedtech.com/

Chiawei

> From: joel.stan@gmail.com <joel.stan@gmail.com> On Behalf Of Joel Stanley
> Sent: Monday, June 20, 2022 3:01 PM
> 
> The documentation above the DEFINE_ALIGN_BUFFER says it's for use outside
> functions, but we're inside one.
> 
> Instead use ALLOC_CACHE_ALIGN_BUFFER, the stack based macro, which also
> includes the cache alignment.
> 
> Fixes: b583348ca8c8 ("image: fit: Align hash output buffers")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> This fixes booting the ast2600-evb image in qemu, which was getting all zeroes
> for the output of the FIT hash check.
> 
> The 'static' buffer was in BSS but the output image didn't contain a BSS section.
> The pointer was left pointing to the text, so the code was trying to write to the
> (read only?) text area in SPI NOR memory space.
> 
>  tools/mkimage.h  | 3 +--
>  boot/image-fit.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/mkimage.h b/tools/mkimage.h index
> 7652c8b001c3..f5ca65e2edfd 100644
> --- a/tools/mkimage.h
> +++ b/tools/mkimage.h
> @@ -41,8 +41,7 @@ static inline ulong map_to_sysmem(void *ptr)
>  	return (ulong)(uintptr_t)ptr;
>  }
> 
> -#define ARCH_DMA_MINALIGN 1
> -#define DEFINE_ALIGN_BUFFER(type, name, size, alugn) type name[size]
> +#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) type name[size]
> 
>  #define MKIMAGE_TMPFILE_SUFFIX		".tmp"
>  #define MKIMAGE_MAX_TMPFILE_LEN		256
> diff --git a/boot/image-fit.c b/boot/image-fit.c index
> f57d97f55229..df3e5df8836a 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -1264,8 +1264,7 @@ int calculate_hash(const void *data, int data_len,
> const char *name,  static int fit_image_check_hash(const void *fit, int noffset,
> const void *data,
>  				size_t size, char **err_msgp)
>  {
> -	DEFINE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN,
> -			    ARCH_DMA_MINALIGN);
> +	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN);
>  	int value_len;
>  	const char *algo;
>  	uint8_t *fit_value;
> --
> 2.35.1

Re: [PATCH] image: fit: Use stack allocation macro

[Sean Anderson]

On 6/20/22 3:01 AM, Joel Stanley wrote:
> The documentation above the DEFINE_ALIGN_BUFFER says it's for use
> outside functions, but we're inside one.
> 
> Instead use ALLOC_CACHE_ALIGN_BUFFER, the stack based macro, which also
> includes the cache alignment.
> 
> Fixes: b583348ca8c8 ("image: fit: Align hash output buffers")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> This fixes booting the ast2600-evb image in qemu, which was getting all
> zeroes for the output of the FIT hash check.
> 
> The 'static' buffer was in BSS but the output image didn't contain a BSS
> section. The pointer was left pointing to the text, so the code was
> trying to write to the (read only?) text area in SPI NOR memory space.
> 
>  tools/mkimage.h  | 3 +--
>  boot/image-fit.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/mkimage.h b/tools/mkimage.h
> index 7652c8b001c3..f5ca65e2edfd 100644
> --- a/tools/mkimage.h
> +++ b/tools/mkimage.h
> @@ -41,8 +41,7 @@ static inline ulong map_to_sysmem(void *ptr)
>  	return (ulong)(uintptr_t)ptr;
>  }
>  
> -#define ARCH_DMA_MINALIGN 1
> -#define DEFINE_ALIGN_BUFFER(type, name, size, alugn) type name[size]
> +#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) type name[size]
>  
>  #define MKIMAGE_TMPFILE_SUFFIX		".tmp"
>  #define MKIMAGE_MAX_TMPFILE_LEN		256
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index f57d97f55229..df3e5df8836a 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -1264,8 +1264,7 @@ int calculate_hash(const void *data, int data_len, const char *name,
>  static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>  				size_t size, char **err_msgp)
>  {
> -	DEFINE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN,
> -			    ARCH_DMA_MINALIGN);
> +	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, value, FIT_MAX_HASH_LEN);
>  	int value_len;
>  	const char *algo;
>  	uint8_t *fit_value;
> 

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Re: [PATCH] image: fit: Use stack allocation macro

[Tom Rini]

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

On Mon, Jun 20, 2022 at 04:31:17PM +0930, Joel Stanley wrote:

> The documentation above the DEFINE_ALIGN_BUFFER says it's for use
> outside functions, but we're inside one.
> 
> Instead use ALLOC_CACHE_ALIGN_BUFFER, the stack based macro, which also
> includes the cache alignment.
> 
> Fixes: b583348ca8c8 ("image: fit: Align hash output buffers")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Tested-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Applied to u-boot/master, thanks!

-- 
Tom

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