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