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

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

[Michael Tretter]

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@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);
 
-- 
2.19.1

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

[Michal Simek]

+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@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.

Thanks,
Michal

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

[Alexander Graf]

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@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.

The one thing that I'm not fully sure of is the realloc(). I don't think
that data is necessarily always a pointer that way allocated using malloc().

If you look at bif_add_bit() for example the data pointer is an offset
into a pointer from read_full_file(). So realloc() won't work on it.

I think it's safer to just allocate a completely new buffer here and
copy the payload if unaligned.


Alex

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

[Michal Simek]

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@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.
> 
> The one thing that I'm not fully sure of is the realloc(). I don't think
> that data is necessarily always a pointer that way allocated using malloc().
> 
> If you look at bif_add_bit() for example the data pointer is an offset
> into a pointer from read_full_file(). So realloc() won't work on it.
> 
> I think it's safer to just allocate a completely new buffer here and
> copy the payload if unaligned.

ok. Let me just wait a little bit to finish this discussion.

Thanks,
Michal

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

[Michael Tretter]

On Mon, 03 Dec 2018 14:42:59 +0100, 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@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.
> 
> The one thing that I'm not fully sure of is the realloc(). I don't think
> that data is necessarily always a pointer that way allocated using malloc().
> 
> If you look at bif_add_bit() for example the data pointer is an offset
> into a pointer from read_full_file(). So realloc() won't work on it.
>
> I think it's safer to just allocate a completely new buffer here and
> copy the payload if unaligned.

Thanks. I thought about aligning data in read_full_file(), but the size
read by read_full_file() is not used in bif_add_elf(). I further
assumed that the bitstream is always correctly aligned (because I never
saw anything else) and will never hit the realloc() path, but I'm
not sure if this is a safe assumption.

As realloc() does not work in bif_add_part(), I can only malloc() a
new aligned buffer and cannot free() the old one. I guess that is fine.

I will send a v2.

Michael

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

[Alexander Graf]

On 03.12.18 16:19, Michael Tretter wrote:
> On Mon, 03 Dec 2018 14:42:59 +0100, 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@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.
>>
>> The one thing that I'm not fully sure of is the realloc(). I don't think
>> that data is necessarily always a pointer that way allocated using malloc().
>>
>> If you look at bif_add_bit() for example the data pointer is an offset
>> into a pointer from read_full_file(). So realloc() won't work on it.
>>
>> I think it's safer to just allocate a completely new buffer here and
>> copy the payload if unaligned.
> 
> Thanks. I thought about aligning data in read_full_file(), but the size
> read by read_full_file() is not used in bif_add_elf(). I further
> assumed that the bitstream is always correctly aligned (because I never
> saw anything else) and will never hit the realloc() path, but I'm
> not sure if this is a safe assumption.
> 
> As realloc() does not work in bif_add_part(), I can only malloc() a
> new aligned buffer and cannot free() the old one. I guess that is fine.

Yes. We're already leaking the old buffer, so leaking yet another one
isn't a big issue either ;).

> I will send a v2.

Thanks! And please CC me on patches to zynqmpbif next time :).


Alex

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

[Michal Simek]

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@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?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

[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@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
> 
> 

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

[Michael Tretter]

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@pengutronix.de>
---
v2:
- do not realloc() data but malloc() and memcpy() new data
---
 tools/zynqmpbif.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 6c8f66055d..a33c15e1f0 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -319,16 +319,25 @@ static int bif_add_pmufw(struct bif_entry *bf, const char *data, size_t len)
 static int bif_add_part(struct bif_entry *bf, const 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) {
+		char *newdata = malloc(len_padded);
+		memcpy(newdata, data, len);
+		memset(newdata + len, 0, len_padded - len);
+		data = newdata;
+	}
+
 	if (bf->flags & (1ULL << BIF_FLAG_PMUFW_IMAGE))
 		return bif_add_pmufw(bf, data, len);
 
-- 
2.19.1

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

[Alexander Graf]

On 03.12.18 16:37, 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@pengutronix.de>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

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

[Michal Simek]

On 03. 12. 18 16:37, 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@pengutronix.de>
> ---
> v2:
> - do not realloc() data but malloc() and memcpy() new data
> ---
>  tools/zynqmpbif.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
> index 6c8f66055d..a33c15e1f0 100644
> --- a/tools/zynqmpbif.c
> +++ b/tools/zynqmpbif.c
> @@ -319,16 +319,25 @@ static int bif_add_pmufw(struct bif_entry *bf, const char *data, size_t len)
>  static int bif_add_part(struct bif_entry *bf, const 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) {
> +		char *newdata = malloc(len_padded);
> +		memcpy(newdata, data, len);
> +		memset(newdata + len, 0, len_padded - len);
> +		data = newdata;
> +	}
> +
>  	if (bf->flags & (1ULL << BIF_FLAG_PMUFW_IMAGE))
>  		return bif_add_pmufw(bf, data, len);
>  
> 

Applied.
M