[U-Boot] [PATCH] tools: zynqmpimage: round up partition size

[Michael Tretter]

On Mon, 17 Dec 2018 14:27:38 +0100, Michal Simek wrote:
> On 03. 12. 18 14:42, Alexander Graf wrote:
> > On 03.12.18 14:14, Michal Simek wrote:  
> >> +Alex
> >>
> >> On 28. 11. 18 11:47, Michael Tretter wrote:  
> >>> The FSBL copies "Total Partition Word Length" * 4 bytes from the boot.bin,
> >>> which implies that the partition size is 4 byte aligned. When writing the
> >>> partition, mkimage calculates "Total Partition Word Length" by dividing
> >>> the size by 4. This implicitly cuts unaligned bytes at the end of the
> >>> added binary.
> >>>
> >>> Instead of rounding down, the size must be round up to 4 bytes and the
> >>> binary padded accordingly.
> >>>
> >>> Signed-off-by: Michael Tretter <m.tretter at pengutronix.de>
> >>> ---
> >>>  tools/zynqmpbif.c | 18 ++++++++++++++----
> >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
> >>> index 6c8f66055d..885a037da6 100644
> >>> --- a/tools/zynqmpbif.c
> >>> +++ b/tools/zynqmpbif.c
> >>> @@ -316,19 +316,29 @@ static int bif_add_pmufw(struct bif_entry *bf, const char *data, size_t len)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> -static int bif_add_part(struct bif_entry *bf, const char *data, size_t len)
> >>> +static int bif_add_part(struct bif_entry *bf, char *data, size_t len)
> >>>  {
> >>>  	size_t parthdr_offset = 0;
> >>> +	size_t len_padded = ROUND(len, 4);
> >>> +
> >>>  	struct partition_header parthdr = {
> >>> -		.len_enc = cpu_to_le32(len / 4),
> >>> -		.len_unenc = cpu_to_le32(len / 4),
> >>> -		.len = cpu_to_le32(len / 4),
> >>> +		.len_enc = cpu_to_le32(len_padded / 4),
> >>> +		.len_unenc = cpu_to_le32(len_padded / 4),
> >>> +		.len = cpu_to_le32(len_padded / 4),
> >>>  		.entry_point = cpu_to_le64(bf->entry),
> >>>  		.load_address = cpu_to_le64(bf->load),
> >>>  	};
> >>>  	int r;
> >>>  	uint32_t csum;
> >>>  
> >>> +	if (len != len_padded) {
> >>> +		data = realloc(data, len_padded);
> >>> +		while (len < len_padded) {
> >>> +			data[len] = 0;
> >>> +			len++;
> >>> +		}
> >>> +	}
> >>> +
> >>>  	if (bf->flags & (1ULL << BIF_FLAG_PMUFW_IMAGE))
> >>>  		return bif_add_pmufw(bf, data, len);
> >>>  
> >>>  
> >>
> >> I am not using this for creating boot image but I have tested it on
> >> regular example and it is still working.
> >>
> >> I have applied this to my branch.
> >> Alex: If you have any comment on this please let us know and I can
> >> remove it from my queue.  
> > 
> > I don't think I've ever had a not-multiple-of-4 payload, so I never ran
> > into this.  
> 
> I was playing with U-Boot on Friday and I found one thing related to
> this commit. You can get "not-multiple-of-4 payload" quite simply just
> but enabling OF_SEPARATE for SPL which is generating (I am not 100% sure
> if all case) aligned u-boot-spl-nodtb.bin and then appends cat
> <filterred dtb> >> u-boot-spl-nodtb.bin which is ending up in
> "not-multiple-of-4 payload" very often.
> 
> I was trying to enable OF_SEPARATE because of
> "fdt: Add warning about CONFIG_OF_EMBED"
> (sha1: 841d5fbae4e993476fa87d8933db0cd58d3c2d41)
> which is working fine for jtag bootmode but not for SD one (I expect
> also others will be broken).
> 
> But I found that you are still pure len(non aligned) in
> image_size/image_stored_size instead of aligned one. Which caused that
> boot.bin with "not-multiple-of-4 payload" is not handled properly by
> bootrom.
> 
> After this patch, boot.bin started to work.
> 
> --- a/tools/zynqmpbif.c
> +++ b/tools/zynqmpbif.c
> @@ -425,8 +425,8 @@ static int bif_add_part(struct bif_entry *bf, const
> char *data, size_t len)
>                 if (!bif_output.header->image_offset)
>                         bif_output.header->image_offset =
>                                 cpu_to_le32(bf->offset);
> -               bif_output.header->image_size = cpu_to_le32(len);
> -               bif_output.header->image_stored_size = cpu_to_le32(len);
> +               bif_output.header->image_size = cpu_to_le32(len_padded);
> +               bif_output.header->image_stored_size =
> cpu_to_le32(len_padded);
> 
> 
> Anyway xilinx bootgen has issue with this too and also zynqmpimage.c.
> 
> Can you please retest it again but I am quite sure you need to also
> update header with padded len?

The image_size affects only the first stage bootloader. As I use the
FSBL for that, I didn't run into this issue.

I can reproduce the issue by writing a size that is not multiple of 4
into either image_size or image_stored_size. The FSBL does not
boot, if the image_size is larger than the size of the image, but not 4
byte aligned. It does boot, if image_size is larger and 4 byte aligned.

I would have expected that the bootrom ignores the bytes between
image_size and partition_size and just reads image_size bytes.
Therefore, aligning the image_size looks like a proper fix, but the
reason for the patch is different.

Michael

> 
> Thanks,
> Michal
> 
>