[PATCH u-boot-mvebu 0/5] mvebu: Fix UART booting

[PATCH u-boot-mvebu 0/5] mvebu: Fix UART booting

[Pali Rohár]

This patch series contains kwboot fixes for booting non-UART-generated
images over UART.

Pali Rohár (5):
  tools: kwbimage: Fix invalid UART kwbimage v1 headersz
  tools: kwboot: Fix invalid UART kwbimage v1 headersz
  tools: kwboot: Fix inserting UART data checksum without -B option
  tools: kwboot: Fix sending very small images
  tools: kwboot: Workaround A38x BootROM bug for images with a gap

 tools/kwbimage.c | 10 ++++++++++
 tools/kwboot.c   | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

-- 
2.20.1

[PATCH u-boot-mvebu 1/5] tools: kwbimage: Fix invalid UART kwbimage v1 headersz

[Pali Rohár]

Armada 385 BootROM ignores low 7 bits of headersz when parsing kwbimage
header of UART type, which effectively means that headersz is rounded down
to multiply of 128 bytes. For all other image types BootROM reads and use
all bits of headersz. Therefore fill into UART type of kwbimage v1 headersz
aligned to 128 bytes.

Fixes: 2b0980c24027 ("tools: kwbimage: Fill the real header size into the main header")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwbimage.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 309657a5637b..177084adf825 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1231,6 +1231,16 @@ static size_t image_headersz_v1(int *hasext)
 	if (count > 0)
 		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 
+	/*
+	 * For all images except UART, headersz stored in header itself should
+	 * contains header size without padding. For UART image BootROM rounds
+	 * down headersz to multiply of 128 bytes. Therefore align UART headersz
+	 * to multiply of 128 bytes to ensure that remaining UART header bytes
+	 * are not ignored by BootROM.
+	 */
+	if (image_get_bootfrom() == IBR_HDR_UART_ID)
+		headersz = ALIGN(headersz, 128);
+
 	return headersz;
 }
 
-- 
2.20.1

Re: [PATCH u-boot-mvebu 1/5] tools: kwbimage: Fix invalid UART kwbimage v1 headersz

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> Armada 385 BootROM ignores low 7 bits of headersz when parsing kwbimage
> header of UART type, which effectively means that headersz is rounded down
> to multiply of 128 bytes. For all other image types BootROM reads and use
> all bits of headersz. Therefore fill into UART type of kwbimage v1 headersz
> aligned to 128 bytes.
> 
> Fixes: 2b0980c24027 ("tools: kwbimage: Fill the real header size into the main header")
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 309657a5637b..177084adf825 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1231,6 +1231,16 @@ static size_t image_headersz_v1(int *hasext)
>   	if (count > 0)
>   		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
>   
> +	/*
> +	 * For all images except UART, headersz stored in header itself should
> +	 * contains header size without padding. For UART image BootROM rounds
> +	 * down headersz to multiply of 128 bytes. Therefore align UART headersz
> +	 * to multiply of 128 bytes to ensure that remaining UART header bytes
> +	 * are not ignored by BootROM.
> +	 */
> +	if (image_get_bootfrom() == IBR_HDR_UART_ID)
> +		headersz = ALIGN(headersz, 128);
> +
>   	return headersz;
>   }
>   

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-mvebu 2/5] tools: kwboot: Fix invalid UART kwbimage v1 headersz

[Pali Rohár]

Ensure that UART aligned header size is always stored into kwbimage v1
header. It is needed for proper UART booting. Calculation of headersz field
was broken in commit d656f5a0ee22 ("tools: kwboot: Calculate real used
space in kwbimage header when calling kwboot_img_grow_hdr()") which
introduced optimization of kwboot_img_grow_hdr() function.

Fixes: d656f5a0ee22 ("tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwboot.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index c131711444ec..dd894e80db1c 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -2171,6 +2171,17 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 
 		kwboot_printv("Aligning image header to Xmodem block size\n");
 		kwboot_img_grow_hdr(img, size, grow);
+		hdrsz += grow;
+
+		/*
+		 * kwbimage v1 contains header size field and for UART type it
+		 * must be set to the aligned xmodem header size because BootROM
+		 * rounds header size down to xmodem block size.
+		 */
+		if (kwbimage_version(img) == 1) {
+			hdr->headersz_msb = hdrsz >> 16;
+			hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
+		}
 	}
 
 	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;
-- 
2.20.1

Re: [PATCH u-boot-mvebu 2/5] tools: kwboot: Fix invalid UART kwbimage v1 headersz

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> Ensure that UART aligned header size is always stored into kwbimage v1
> header. It is needed for proper UART booting. Calculation of headersz field
> was broken in commit d656f5a0ee22 ("tools: kwboot: Calculate real used
> space in kwbimage header when calling kwboot_img_grow_hdr()") which
> introduced optimization of kwboot_img_grow_hdr() function.
> 
> Fixes: d656f5a0ee22 ("tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()")
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index c131711444ec..dd894e80db1c 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -2171,6 +2171,17 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   
>   		kwboot_printv("Aligning image header to Xmodem block size\n");
>   		kwboot_img_grow_hdr(img, size, grow);
> +		hdrsz += grow;
> +
> +		/*
> +		 * kwbimage v1 contains header size field and for UART type it
> +		 * must be set to the aligned xmodem header size because BootROM
> +		 * rounds header size down to xmodem block size.
> +		 */
> +		if (kwbimage_version(img) == 1) {
> +			hdr->headersz_msb = hdrsz >> 16;
> +			hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
> +		}
>   	}
>   
>   	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-mvebu 3/5] tools: kwboot: Fix inserting UART data checksum without -B option

[Pali Rohár]

Commit 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data
checksum") added fixup code to insert place for data checksum if UART image
does not have it. Together with option -B (change baudrate), kwboot
calculates this checksum. Without option -B, it inserts only place for
checksum but does not calculate it.

This commit fix above logic and calculate data checksum also when kwboot is
used without -B option.

Fixes: 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data checksum")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwboot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index dd894e80db1c..23a893a9b9f8 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -2082,6 +2082,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 			goto err;
 		}
 		kwboot_img_grow_data_right(img, size, sizeof(uint32_t));
+		/* Update the 32-bit data checksum */
+		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
 	}
 
 	if (!kwboot_img_has_ddr_init(img) &&
-- 
2.20.1

Re: [PATCH u-boot-mvebu 3/5] tools: kwboot: Fix inserting UART data checksum without -B option

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> Commit 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data
> checksum") added fixup code to insert place for data checksum if UART image
> does not have it. Together with option -B (change baudrate), kwboot
> calculates this checksum. Without option -B, it inserts only place for
> checksum but does not calculate it.
> 
> This commit fix above logic and calculate data checksum also when kwboot is
> used without -B option.
> 
> Fixes: 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data checksum")
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index dd894e80db1c..23a893a9b9f8 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -2082,6 +2082,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   			goto err;
>   		}
>   		kwboot_img_grow_data_right(img, size, sizeof(uint32_t));
> +		/* Update the 32-bit data checksum */
> +		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
>   	}
>   
>   	if (!kwboot_img_has_ddr_init(img) &&

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-mvebu 4/5] tools: kwboot: Fix sending very small images

[Pali Rohár]

Sending of very small images (smaller than 128 bytes = xmodem block size)
cause out-of-bound memory read access. Fix this issue by ensuring that
hdrsz when sending image is not larger than total size of the image.
Issue was introduced in commit f8017c37799c ("tools: kwboot: Fix sending
Kirkwood v0 images"). Special case when total image is smaller than header
size aligned to multiply of xmodem size is already handled since that
commit.

Fixes: f8017c37799c ("tools: kwboot: Fix sending Kirkwood v0 images")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwboot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 23a893a9b9f8..1cf78dda6755 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1458,6 +1458,8 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
 	 * followed by the header. So align header size to xmodem block size.
 	 */
 	hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
+	if (hdrsz > size)
+		hdrsz = size;
 
 	pnum = 1;
 
-- 
2.20.1

Re: [PATCH u-boot-mvebu 4/5] tools: kwboot: Fix sending very small images

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> Sending of very small images (smaller than 128 bytes = xmodem block size)
> cause out-of-bound memory read access. Fix this issue by ensuring that
> hdrsz when sending image is not larger than total size of the image.
> Issue was introduced in commit f8017c37799c ("tools: kwboot: Fix sending
> Kirkwood v0 images"). Special case when total image is smaller than header
> size aligned to multiply of xmodem size is already handled since that
> commit.
> 
> Fixes: f8017c37799c ("tools: kwboot: Fix sending Kirkwood v0 images")
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 23a893a9b9f8..1cf78dda6755 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1458,6 +1458,8 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
>   	 * followed by the header. So align header size to xmodem block size.
>   	 */
>   	hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
> +	if (hdrsz > size)
> +		hdrsz = size;
>   
>   	pnum = 1;
>   

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-mvebu 5/5] tools: kwboot: Workaround A38x BootROM bug for images with a gap

[Pali Rohár]

A38x BootROM has a bug which cause that BootROM loads data part of UART
image into RAM target address increased by one byte when source address
and header size stored in the image header are not same.

Workaround this bug by completely removing a gap between header and data
part of the UART image. Without gap, this BootROM bug is not triggered.

This gap can be present in SDIO or SATA image types which have aligned
start of the data part to the media sector size. With this workaround
kwboot should be able to convert and send SDIO or SATA images for UART
booting.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwboot.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 1cf78dda6755..2b92966919da 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -78,6 +78,17 @@
  *
  * - IBR_HDR_UART_ID (0x69):
  *   UART image can be transfered via xmodem protocol over first UART.
+ *   Unlike all other image types, header size stored in the image must be
+ *   multiply of the 128 bytes (for all other image types it can be any size)
+ *   and data part of the image does not have to contain 32-bit checksum
+ *   (all other image types must have valid 32-bit checksum in its data part).
+ *   And data size stored in the image is ignored. A38x BootROM determinates
+ *   size of the data part implicitly by the end of the xmodem transfer.
+ *   A38x BootROM has a bug which cause that BootROM loads data part of UART
+ *   image into RAM target address increased by one byte when source address
+ *   and header size stored in the image header are not same. So UART image
+ *   should be constructed in a way that there is no gap between header and
+ *   data part.
  *
  * - IBR_HDR_I2C_ID (0x4D):
  *   It is unknown for what kind of storage is used this image. It is not
@@ -2188,6 +2199,18 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 		}
 	}
 
+	/* Header size and source address must be same for UART type due to A38x BootROM bug */
+	if (hdrsz != le32_to_cpu(hdr->srcaddr)) {
+		if (is_secure) {
+			fprintf(stderr, "Cannot align image with secure header\n");
+			goto err;
+		}
+
+		kwboot_printv("Removing gap between image header and data\n");
+		memmove(img + hdrsz, img + le32_to_cpu(hdr->srcaddr), le32_to_cpu(hdr->blocksize));
+		hdr->srcaddr = cpu_to_le32(hdrsz);
+	}
+
 	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;
 
 	*size = le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize);
-- 
2.20.1

Re: [PATCH u-boot-mvebu 5/5] tools: kwboot: Workaround A38x BootROM bug for images with a gap

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> A38x BootROM has a bug which cause that BootROM loads data part of UART
> image into RAM target address increased by one byte when source address
> and header size stored in the image header are not same.
> 
> Workaround this bug by completely removing a gap between header and data
> part of the UART image. Without gap, this BootROM bug is not triggered.
> 
> This gap can be present in SDIO or SATA image types which have aligned
> start of the data part to the media sector size. With this workaround
> kwboot should be able to convert and send SDIO or SATA images for UART
> booting.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 1cf78dda6755..2b92966919da 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -78,6 +78,17 @@
>    *
>    * - IBR_HDR_UART_ID (0x69):
>    *   UART image can be transfered via xmodem protocol over first UART.
> + *   Unlike all other image types, header size stored in the image must be
> + *   multiply of the 128 bytes (for all other image types it can be any size)
> + *   and data part of the image does not have to contain 32-bit checksum
> + *   (all other image types must have valid 32-bit checksum in its data part).
> + *   And data size stored in the image is ignored. A38x BootROM determinates
> + *   size of the data part implicitly by the end of the xmodem transfer.
> + *   A38x BootROM has a bug which cause that BootROM loads data part of UART
> + *   image into RAM target address increased by one byte when source address
> + *   and header size stored in the image header are not same. So UART image
> + *   should be constructed in a way that there is no gap between header and
> + *   data part.
>    *
>    * - IBR_HDR_I2C_ID (0x4D):
>    *   It is unknown for what kind of storage is used this image. It is not
> @@ -2188,6 +2199,18 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   		}
>   	}
>   
> +	/* Header size and source address must be same for UART type due to A38x BootROM bug */
> +	if (hdrsz != le32_to_cpu(hdr->srcaddr)) {
> +		if (is_secure) {
> +			fprintf(stderr, "Cannot align image with secure header\n");
> +			goto err;
> +		}
> +
> +		kwboot_printv("Removing gap between image header and data\n");
> +		memmove(img + hdrsz, img + le32_to_cpu(hdr->srcaddr), le32_to_cpu(hdr->blocksize));
> +		hdr->srcaddr = cpu_to_le32(hdrsz);
> +	}
> +
>   	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;
>   
>   	*size = le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize);

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu 0/5] mvebu: Fix UART booting

[Martin Rowe]

On Thu, 23 Mar 2023 at 19:58, Pali Rohár <pali@kernel.org> wrote:
>
> This patch series contains kwboot fixes for booting non-UART-generated
> images over UART.
>
> Pali Rohár (5):
>   tools: kwbimage: Fix invalid UART kwbimage v1 headersz
>   tools: kwboot: Fix invalid UART kwbimage v1 headersz
>   tools: kwboot: Fix inserting UART data checksum without -B option
>   tools: kwboot: Fix sending very small images
>   tools: kwboot: Workaround A38x BootROM bug for images with a gap
>
>  tools/kwbimage.c | 10 ++++++++++
>  tools/kwboot.c   | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> --
> 2.20.1
>

Tested-by: Martin Rowe <martin.p.rowe@gmail.com>

Re: [PATCH u-boot-mvebu 0/5] mvebu: Fix UART booting

[Stefan Roese]

On 3/23/23 20:57, Pali Rohár wrote:
> This patch series contains kwboot fixes for booting non-UART-generated
> images over UART.
> 
> Pali Rohár (5):
>    tools: kwbimage: Fix invalid UART kwbimage v1 headersz
>    tools: kwboot: Fix invalid UART kwbimage v1 headersz
>    tools: kwboot: Fix inserting UART data checksum without -B option
>    tools: kwboot: Fix sending very small images
>    tools: kwboot: Workaround A38x BootROM bug for images with a gap
> 
>   tools/kwbimage.c | 10 ++++++++++
>   tools/kwboot.c   | 38 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 48 insertions(+)
> 

Applied to u-boot-marvell/next

Thanks,
Stefan