Kever Yang <[email protected]> writes:
> Hi Punit,
>
> On 2020/3/16 下午3:28, Punit Agrawal wrote:
>> Kever Yang <[email protected]> writes:
>>
>>> The image is usually stored in block device like emmc, SD card, make the
>>> offset of image data aligned to block(512 byte) can avoid data copy
>>> during boot process.
>>> eg. SPL boot from FIT image with external data:
>>> - SPL read the first block of FIT image, and then parse the header;
>>> - SPL read image data separately;
>>> - The first image offset is the base_offset which is the header size;
>>> - The second image offset is just after the first image;
>>> - If the offset of imge does not aligned, SPL will do memcpy;
>>> The header size is a ramdon number, which is very possible not aligned, so
>>> add '-B' to specify the align size in hex for better performance.
>>>
>>> example usage:
>>> ./tools/mkimage -E -f u-boot.its -B 200 u-boot.itb
>>>
>>> Signed-off-by: Kever Yang <[email protected]>
>>> ---
>>>
>>> Changes in v2:
>>> - use '-B' to take a argument as a align block lenth;
>>> - address commens from Heinrich, Rasmus, Tom, Punit;
>>>
>>> doc/uImage.FIT/source_file_format.txt | 5 +++++
>>> tools/fit_image.c | 26 ++++++++++++++++++++------
>>> tools/imagetool.h | 1 +
>>> tools/mkimage.c | 14 ++++++++++++--
>>> 4 files changed, 38 insertions(+), 8 deletions(-)
>>>
[...]
>>> @@ -430,8 +435,11 @@ static int fit_extract_data(struct image_tool_params
>>> *params, const char *fname)
>>> return -EIO;
>>> fit_size = fdt_totalsize(fdt);
>>> - /* Allocate space to hold the image data we will extract */
>>> - buf = malloc(fit_size);
>>> + /*
>>> + * Allocate space to hold the image data we will extract,
>>> + * 4096 byte extral space allocate for image alignment.
>>> + */
>>> + buf = malloc(fit_size + 4096);
>> I might be missing something but shouldn't the extra allocation depend
>> on -
>>
>> * block size
>> * number of data objects in the FIT
>
> We can get block size here, I will use it in next version, but not
> able to get the number of data
>
> objects before go over the dtb later, so add 8 block may OK for most
> case.
It wouldn't be much effort to get the child count - see
fdtdec_get_child_count() (lib/fdtdec.c) for how it's done.
>>
>>> if (!buf) {
>>> ret = -ENOMEM;
>>> goto err_munmap;
>>> @@ -471,17 +479,23 @@ static int fit_extract_data(struct image_tool_params
>>> *params, const char *fname)
>>> buf_ptr);
>>> }
>>> fdt_setprop_u32(fdt, node, FIT_DATA_SIZE_PROP, len);
>>> -
>>> - buf_ptr += (len + 3) & ~3;
>>> + if (params->bl_len > 0)
>>> + buf_ptr += get_aligned_size(len, params->bl_len);
>>> + else
>>> + buf_ptr += get_aligned_size(len, 4);
>>> }
>>> /* Pack the FDT and place the data after it */
>>> fdt_pack(fdt);
>>> + new_size = fdt_totalsize(fdt);
>>> + if (params->bl_len > 0)
>>> + new_size = get_aligned_size(new_size, params->bl_len);
>>> + else
>>> + new_size = get_aligned_size(new_size, 4);
>> This condition gets duplicated here and in the loop above. It would be
>> better to introduce a variable to track alignment requirement (4 or
>> bl_len) at the start of the function and use them in both places.
>
>
> Introduce a new variable to replace bl_len may not help much for
> logical or code size because
>
> the compare to '0' already very like the use of a tag.
The suggestion was to make the code more readable. I agree it won't have
any significant impact on code size or execution.
Thanks,
Punit
>
>
> Thanks,
>
> - Kever
>
>>
>> What do you think?
>>
>> Thanks,
>> Punit
>>
>>> + fdt_set_totalsize(fdt, new_size);
>>> debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
>>> debug("External data size %x\n", buf_ptr);
>>> - new_size = fdt_totalsize(fdt);
>>> - new_size = (new_size + 3) & ~3;
>>> munmap(fdt, sbuf.st_size);
>>> if (ftruncate(fd, new_size)) {
>>> diff --git a/tools/imagetool.h b/tools/imagetool.h
>>> index e1c778b0df..fd5e4b246c 100644
>>> --- a/tools/imagetool.h
>>> +++ b/tools/imagetool.h
>>> @@ -76,6 +76,7 @@ struct image_tool_params {
>>> bool external_data; /* Store data outside the FIT */
>>> bool quiet; /* Don't output text in normal operation */
>>> unsigned int external_offset; /* Add padding to external data */
>>> + int bl_len; /* Block length in byte for external data */
>>> const char *engine_id; /* Engine to use for signing */
>>> };
>>> diff --git a/tools/mkimage.c b/tools/mkimage.c
>>> index 5f51d2cc89..584aeff907 100644
>>> --- a/tools/mkimage.c
>>> +++ b/tools/mkimage.c
>>> @@ -97,8 +97,9 @@ static void usage(const char *msg)
>>> " -i => input filename for ramdisk file\n");
>>> #ifdef CONFIG_FIT_SIGNATURE
>>> fprintf(stderr,
>>> - "Signing / verified boot options: [-E] [-k keydir] [-K dtb] [
>>> -c <comment>] [-p addr] [-r] [-N engine]\n"
>>> + "Signing / verified boot options: [-E] [-B] [-k keydir] [-K
>>> dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
>>> " -E => place data outside of the FIT structure\n"
>>> + " -B => align FIT structure and header to block\n"
>>> " -k => set directory containing private keys\n"
>>> " -K => write public keys to this .dtb file\n"
>>> " -c => add comment in signature node\n"
>>> @@ -143,7 +144,7 @@ static void process_args(int argc, char **argv)
>>> int opt;
>>> while ((opt = getopt(argc, argv,
>>> - "a:A:b:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qsT:vVx"))
>>> != -1) {
>>> +
>>> "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qsT:vVx")) != -1) {
>>> switch (opt) {
>>> case 'a':
>>> params.addr = strtoull(optarg, &ptr, 16);
>>> @@ -167,6 +168,15 @@ static void process_args(int argc, char **argv)
>>> params.cmdname, optarg);
>>> exit(EXIT_FAILURE);
>>> }
>>> + break;
>>> + case 'B':
>>> + params.bl_len = strtoull(optarg, &ptr, 16);
>>> + if (*ptr) {
>>> + fprintf(stderr, "%s: invalid block length %s\n",
>>> + params.cmdname, optarg);
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> break;
>>> case 'c':
>>> params.comment = optarg;
