diff mbox series

[u-boot-marvell,09/11] tools: kwbimage: Fill the real header size into the main header

Message ID 20211108171251.25382-10-kabel@kernel.org
State Accepted
Commit 2b0980c24027ee00987b04c0c4dc18546a704c84
Delegated to: Stefan Roese
Headers show
Series Another kwbimage series | expand

Commit Message

Marek Behún Nov. 8, 2021, 5:12 p.m. UTC 
From: Pali Rohár <pali@kernel.org>

Fill the real header size without padding into the main header

This allows to reduce final image when converting image to another format
which does not need additional padding.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Stefan Roese Nov. 10, 2021, 8:29 a.m. UTC | #1 
On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Fill the real header size without padding into the main header
> 
> This allows to reduce final image when converting image to another format
> which does not need additional padding.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 8dcfebcb57..114b313b4d 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1220,6 +1220,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   {
>   	struct image_cfg_element *e;
>   	struct main_hdr_v1 *main_hdr;
> +	struct opt_hdr_v1 *ohdr;
>   	struct register_set_hdr_v1 *register_set_hdr;
>   	struct secure_hdr_v1 *secure_hdr = NULL;
>   	size_t headersz;
> @@ -1370,6 +1371,14 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	main_hdr->checksum = image_checksum8(main_hdr, headersz);
>   
>   	*imagesz = headersz;
> +
> +	/* Fill the real header size without padding into the main header */
> +	headersz = sizeof(*main_hdr);
> +	for_each_opt_hdr_v1 (ohdr, main_hdr)
> +		headersz += opt_hdr_v1_size(ohdr);
> +	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
> +	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
> +
>   	return image;
>   }
>   
> 

Viele Grüße,
Stefan Roese
Pierre Bourdon Dec. 25, 2021, 5:48 p.m. UTC | #2 
Hi Marek, Pali,

On Mon, Nov 8, 2021 at 6:14 PM Marek Behún <kabel@kernel.org> wrote:
> Fill the real header size without padding into the main header
>
> This allows to reduce final image when converting image to another format
> which does not need additional padding.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

This patch seems to cause mkimage to generate v1 images with invalid
checksums (which fail to verify with kwboot or mkimage -l). Could you
double check whether you can reproduce on the latest u-boot master? I
don't really understand how this patch is supposed to work
(headersz_lsb/headersz_msb get updated *after* csum8 has already been
computed!).

$ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
-a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
Image Type:   MVEBU Boot from nand Image
Image version:1
BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
Load Address: 00800000
Entry Point:  00800000

$ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
kwboot version 2022.01-rc4-00075-g3bd6c62cf4-dirty
u-boot.kwb: Invalid image.

(The specific kwbimage.cfg/board is still a WIP, I can try to provide
some more repro if somehow there is something specific to my setup,
but I doubt it.)

Best,
Pali Rohár Dec. 25, 2021, 5:57 p.m. UTC | #3 
On Saturday 25 December 2021 18:48:22 Pierre Bourdon wrote:
> Hi Marek, Pali,
> 
> On Mon, Nov 8, 2021 at 6:14 PM Marek Behún <kabel@kernel.org> wrote:
> > Fill the real header size without padding into the main header
> >
> > This allows to reduce final image when converting image to another format
> > which does not need additional padding.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> 
> This patch seems to cause mkimage to generate v1 images with invalid
> checksums (which fail to verify with kwboot or mkimage -l). Could you
> double check whether you can reproduce on the latest u-boot master?

Hello! I'm using this patch for more than month and I have not seen any
boot issue related to this patch A385 board.

> I don't really understand how this patch is supposed to work
> (headersz_lsb/headersz_msb get updated *after* csum8 has already been
> computed!).

Ou. Now I see, this is of course wrong in this patch! Checksum in
main_hdr->checksum must be filled *after* updating main_hdr->headersz_*
fields.

Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
should be enough. Are you going to prepare a fixup for master branch?

I'm not sure how it could happen... but probably "real header size" is
the same as it was before and therefore checksum did not change.

> $ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
> -a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
> Image Type:   MVEBU Boot from nand Image
> Image version:1
> BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
> Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
> Load Address: 00800000
> Entry Point:  00800000
> 
> $ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
> kwboot version 2022.01-rc4-00075-g3bd6c62cf4-dirty
> u-boot.kwb: Invalid image.
> 
> (The specific kwbimage.cfg/board is still a WIP, I can try to provide
> some more repro if somehow there is something specific to my setup,
> but I doubt it.)
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/
Pierre Bourdon Dec. 25, 2021, 6:06 p.m. UTC | #4 
On Sat, Dec 25, 2021 at 6:57 PM Pali Rohár <pali@kernel.org> wrote:
> Hello! I'm using this patch for more than month and I have not seen any
> boot issue related to this patch A385 board.

FWIW I'm testing this on a Prestera board, so Armada XP based.

> Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
> after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
> should be enough. Are you going to prepare a fixup for master branch?

I've actually tested this earlier -- csum8 works, but csum32 still
doesn't pass in kwboot / mkimage. Note that I haven't checked whether
the hardware accepts it (I know it didn't like the broken csum8), so
this could be that the csum32 verification code is not tolerant to
your modified headersz.

Thanks for the really quick response!

Best,
Pali Rohár Dec. 25, 2021, 6:10 p.m. UTC | #5 
On Saturday 25 December 2021 19:06:13 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 6:57 PM Pali Rohár <pali@kernel.org> wrote:
> > Hello! I'm using this patch for more than month and I have not seen any
> > boot issue related to this patch A385 board.
> 
> FWIW I'm testing this on a Prestera board, so Armada XP based.
> 
> > Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
> > after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
> > should be enough. Are you going to prepare a fixup for master branch?
> 
> I've actually tested this earlier -- csum8 works,

Ok!

> but csum32 still
> doesn't pass in kwboot / mkimage. Note that I haven't checked whether
> the hardware accepts it (I know it didn't like the broken csum8), so
> this could be that the csum32 verification code is not tolerant to
> your modified headersz.

csum32 is checksum of data, not including header. If generated image
does not pass kwboot verification then it is invalid.

Has it worked with some previous version? If yes, can you bisect git
commit which broke it?

> Thanks for the really quick response!
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/
Pierre Bourdon Dec. 25, 2021, 6:18 p.m. UTC | #6 
On Sat, Dec 25, 2021 at 7:10 PM Pali Rohár <pali@kernel.org> wrote:
> csum32 is checksum of data, not including header. If generated image
> does not pass kwboot verification then it is invalid.

Agreed, but this is how it gets computed later in the caller function:

/* Build and add image data checksum */
checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
datasz));

Since headersz is now different (headersz += opt_hdr_v1_size(ohdr); in
this patch), presumably different (and maybe wrong?) data is now
getting checksummed.

> Has it worked with some previous version? If yes, can you bisect git
> commit which broke it?

Just reverting this one specific commit is enough to fix the issue.
After revert:

$ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
-a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
Image Type:   MVEBU Boot from nand Image
Image version:1
BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
Load Address: 00800000
Entry Point:  00800000

$ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
kwboot version 2022.01-rc4-00076-g34df634003-dirty
Patching image boot signature to UART
Sending boot message. Please reboot the target...-
Waiting 2s and flushing tty
Sending boot image header (76288 bytes)...
  <snip>
Done

General initialization - Version: 1.0.0
Serdes initialization - Version: 1.0.2
DDR3 Training Sequence - Ver TIP-1.55.0
DDR3 Training Sequence - Switching XBAR Window to FastPath Window
DDR3 Training Sequence - Ended Successfully

Sending boot image data (735768 bytes)...
  <snip>
Done
Finishing transfer
[Type Ctrl-\ + c to quit]

U-Boot 2022.01-rc4-00076-g34df634003-dirty (Jan 01 1980 - 00:00:00 +0000)

SoC:   98DX3236-A1 at 800 MHz
Model: QNAP QSW-M408S
Board: qsw-98dx3236
DRAM:  512 MiB (800 MHz, 16-bit, ECC not enabled)
NAND:  512 MiB

Best,
Pali Rohár Dec. 25, 2021, 7:11 p.m. UTC | #7 
On Saturday 25 December 2021 19:18:05 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 7:10 PM Pali Rohár <pali@kernel.org> wrote:
> > csum32 is checksum of data, not including header. If generated image
> > does not pass kwboot verification then it is invalid.
> 
> Agreed, but this is how it gets computed later in the caller function:
> 
> /* Build and add image data checksum */
> checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
> datasz));
> 
> Since headersz is now different (headersz += opt_hdr_v1_size(ohdr); in
> this patch), presumably different (and maybe wrong?) data is now
> getting checksummed.

headersz is not different. See:

static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
			     uint8_t *ptr, int payloadsz)
{
...
	*imagesz = headersz;
...
	headersz = sizeof(*main_hdr);
...
}

static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
				struct image_tool_params *params)
{
...
	case 1:
		image = image_create_v1(&headersz, params, ptr, datasz + 4);
		break;
...
	/* Build and add image data checksum */
	checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
						datasz));
}

"headersz" in kwbimage_set_header() should not be affected by this patch
as first argument of image_create_v1() is not modified by this patch.

I do not see here logical error. Any idea?

> > Has it worked with some previous version? If yes, can you bisect git
> > commit which broke it?
> 
> Just reverting this one specific commit is enough to fix the issue.

Ah :-(

> After revert:
> 
> $ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage

You you send a link to this kwbimage.cfg file?

And ideally, could you send me "broken" u-boot.kwb file (with this
patch) and "working" u-boot.kwb (without this patch)?

> -a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
> Image Type:   MVEBU Boot from nand Image
> Image version:1
> BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
> Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
> Load Address: 00800000
> Entry Point:  00800000
> 
> $ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
> kwboot version 2022.01-rc4-00076-g34df634003-dirty
> Patching image boot signature to UART
> Sending boot message. Please reboot the target...-
> Waiting 2s and flushing tty
> Sending boot image header (76288 bytes)...
>   <snip>
> Done
> 
> General initialization - Version: 1.0.0
> Serdes initialization - Version: 1.0.2
> DDR3 Training Sequence - Ver TIP-1.55.0
> DDR3 Training Sequence - Switching XBAR Window to FastPath Window
> DDR3 Training Sequence - Ended Successfully
> 
> Sending boot image data (735768 bytes)...
>   <snip>
> Done
> Finishing transfer
> [Type Ctrl-\ + c to quit]
> 
> U-Boot 2022.01-rc4-00076-g34df634003-dirty (Jan 01 1980 - 00:00:00 +0000)
> 
> SoC:   98DX3236-A1 at 800 MHz
> Model: QNAP QSW-M408S
> Board: qsw-98dx3236
> DRAM:  512 MiB (800 MHz, 16-bit, ECC not enabled)
> NAND:  512 MiB
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/
Pierre Bourdon Dec. 25, 2021, 7:48 p.m. UTC | #8 
On Sat, Dec 25, 2021 at 8:11 PM Pali Rohár <pali@kernel.org> wrote:
> "headersz" in kwbimage_set_header() should not be affected by this patch
> as first argument of image_create_v1() is not modified by this patch.
>
> I do not see here logical error. Any idea?

My bad. I moved the code you added in 2b0980c24027 above the checksum
computation, but left "*imagesz = headersz;" at the end of the
function where it was before. So of course this was broken, but it was
my fault :-)

I'll send you the fix patch shortly, tested on my Prestera/Armada XP board.

Best,
Pali Rohár Dec. 25, 2021, 8:01 p.m. UTC | #9 
On Saturday 25 December 2021 20:48:09 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 8:11 PM Pali Rohár <pali@kernel.org> wrote:
> > "headersz" in kwbimage_set_header() should not be affected by this patch
> > as first argument of image_create_v1() is not modified by this patch.
> >
> > I do not see here logical error. Any idea?
> 
> My bad. I moved the code you added in 2b0980c24027 above the checksum
> computation, but left "*imagesz = headersz;" at the end of the
> function where it was before. So of course this was broken, but it was
> my fault :-)

Perfect, problem solved. Anyway, I would be really interested in your
kwb cfg file as it is probably image layout which reveal this issue.

> I'll send you the fix patch shortly, tested on my Prestera/Armada XP board.
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/
Pierre Bourdon Dec. 25, 2021, 8:15 p.m. UTC | #10 
On Sat, Dec 25, 2021 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
> Perfect, problem solved. Anyway, I would be really interested in your
> kwb cfg file as it is probably image layout which reveal this issue.

It's a very basic configuration, inspired from
board/mikrotik/crs3xx-98dx3236/kwbimage.cfg.in but for NAND booting:

VERSION 1
BOOT_FROM nand
NAND_BLKSZ 00020000
NAND_BADBLK_LOCATION 00
BINARY ./board/qnap/qsw-98dx3236/binary.0 0000005b 00000068

I can't upload the actual KWB images I'm using because this is using a
DDR3 training blob extracted from my vendor's build and I'm unsure
about the licensing. But here's a complete hexdump diff of the images
(before: without this patch, after: with this patch + the checksum
fix).

--- /dev/fd/63  2021-12-25 21:11:45.555505526 +0100
+++ /dev/fd/62  2021-12-25 21:11:45.556505539 +0100
@@ -1,5 +1,5 @@
-00000000  8b 00 00 00 40 3a 0b 00  01 01 00 2a 00 2a 01 00
-00000010  00 00 80 00 00 00 80 00  00 02 00 00 00 00 01 19
+00000000  8b 00 00 00 40 3a 0b 00  01 01 f4 29 00 2a 01 00
+00000010  00 00 80 00 00 00 80 00  00 02 00 00 00 00 01 0c
 00000020  02 01 d4 29 02 00 00 00  5b 00 00 00 68 00 00 00
 00000030  ff 5f 2d e9 c1 02 00 fa  00 00 a0 e3 ff 9f bd e8
 00000040  fe 1f 2d e9 36 0f 07 ee  fe 1f bd e8 1e ff 2f e1
Pali Rohár Dec. 25, 2021, 8:42 p.m. UTC | #11 
On Saturday 25 December 2021 21:15:13 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
> > Perfect, problem solved. Anyway, I would be really interested in your
> > kwb cfg file as it is probably image layout which reveal this issue.
> 
> It's a very basic configuration, inspired from
> board/mikrotik/crs3xx-98dx3236/kwbimage.cfg.in but for NAND booting:
> 
> VERSION 1
> BOOT_FROM nand
> NAND_BLKSZ 00020000
> NAND_BADBLK_LOCATION 00

Probably it is because of NAND alignment. Other Armada boards use SPI
for booting...

> BINARY ./board/qnap/qsw-98dx3236/binary.0 0000005b 00000068
> 
> I can't upload the actual KWB images I'm using because this is using a
> DDR3 training blob extracted from my vendor's build and I'm unsure
> about the licensing.

Could you ask vendor about it? Armada DDR3 training code is for a longer
time also under GPL and BSD license.

Marvell U-Boot for 32-bit Armada boards is available on Marvell github
and latest version in u-boot-2013.01-armada-18.06 branch:
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/tree/u-boot-2013.01-armada-18.06

Source code for BINARY header is in tools/marvell/bin_hdr directory:
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/tree/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr

IIRC those prestera switches are marked as Armada "msys" architecture.
diff mbox series

Patch

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 8dcfebcb57..114b313b4d 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1220,6 +1220,7 @@  static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 {
 	struct image_cfg_element *e;
 	struct main_hdr_v1 *main_hdr;
+	struct opt_hdr_v1 *ohdr;
 	struct register_set_hdr_v1 *register_set_hdr;
 	struct secure_hdr_v1 *secure_hdr = NULL;
 	size_t headersz;
@@ -1370,6 +1371,14 @@  static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	main_hdr->checksum = image_checksum8(main_hdr, headersz);
 
 	*imagesz = headersz;
+
+	/* Fill the real header size without padding into the main header */
+	headersz = sizeof(*main_hdr);
+	for_each_opt_hdr_v1 (ohdr, main_hdr)
+		headersz += opt_hdr_v1_size(ohdr);
+	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
+	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
+
 	return image;
 }