[U-Boot] [PATCH v1] common: Fix load and entry addresses in FIT image

[York Sun]

On 09/02/2015 09:05 AM, Simon Glass wrote:
> Hi York,
> 
> On 1 September 2015 at 22:01, York Sun <yorksun at freescale.com> wrote:
>> FIT image supports more than 32 bits in addresses by using #address-cell
>> field. However the address length is not handled when parsing FIT images.
>> Beside, the variable used to host address has "ulong" type. It is OK for
>> the target, but not always enough for host tools such as mkimage. This
>> patch replaces "ulong" with "phys_addr_t" to make sure the address is
>> correct for both the target and the host.
> 
> This looks right to me but I have a few comments.
> 
>>
>> Signed-off-by: York Sun <yorksun at freescale.com>
>>
>> ---
>>
>>  common/bootm.c     |   13 +++++++------
>>  common/image-fit.c |   55 +++++++++++++++++++++++++++++++++++++---------------
>>  include/bootm.h    |    6 +++---
>>  include/image.h    |   12 ++++++++----
>>  4 files changed, 57 insertions(+), 29 deletions(-)
>>
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 667c934..0672e73 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
>>         return BOOTM_ERR_RESET;
>>  }
>>
>> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
>> -                      void *load_buf, void *image_buf, ulong image_len,
>> -                      uint unc_len, ulong *load_end)
>> +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
>> +                      int type, void *load_buf, void *image_buf,
>> +                      ulong image_len, uint unc_len, ulong *load_end)
>>  {
>>         int ret = 0;
>>
>> @@ -883,7 +883,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
>>  static int bootm_host_load_image(const void *fit, int req_image_type)
>>  {
>>         const char *fit_uname_config = NULL;
>> -       ulong data, len;
>> +       phys_addr_t *data = NULL;
>> +       ulong len;
>>         bootm_headers_t images;
>>         int noffset;
>>         ulong load_end;
>> @@ -897,7 +898,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>>         noffset = fit_image_load(&images, (ulong)fit,
>>                 NULL, &fit_uname_config,
>>                 IH_ARCH_DEFAULT, req_image_type, -1,
>> -               FIT_LOAD_IGNORED, &data, &len);
>> +               FIT_LOAD_IGNORED, data, &len);
>>         if (noffset < 0)
>>                 return noffset;
>>         if (fit_image_get_type(fit, noffset, &image_type)) {
>> @@ -912,7 +913,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>>
>>         /* Allow the image to expand by a factor of 4, should be safe */
>>         load_buf = malloc((1 << 20) + len * 4);
>> -       ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf,
>> +       ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf,
>>                                  (void *)data, len, CONFIG_SYS_BOOTM_LEN,
>>                                  &load_end);
>>         free(load_buf);
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index 28f7aa8..513cfdc 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>         char *desc;
>>         uint8_t type, arch, os, comp;
>>         size_t size;
>> -       ulong load, entry;
>> +       phys_addr_t load, entry;
>>         const void *data;
>>         int noffset;
>>         int ndepth;
>> @@ -428,17 +428,17 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>                 if (ret)
>>                         printf("unavailable\n");
>>                 else
>> -                       printf("0x%08lx\n", load);
>> +                       printf("0x%08llx\n", (uint64_t)load);
>>         }
>>
>>         if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>>             (type == IH_TYPE_RAMDISK)) {
>> -               fit_image_get_entry(fit, image_noffset, &entry);
>> +               ret = fit_image_get_entry(fit, image_noffset, &entry);
>>                 printf("%s  Entry Point:  ", p);
>>                 if (ret)
>>                         printf("unavailable\n");
>>                 else
>> -                       printf("0x%08lx\n", entry);
>> +                       printf("0x%08llx\n", (uint64_t)entry);
> 
> If the address is 32-bit we cast it to 64-bit and print 8 digits. If
> it is 64-bit we print as many digits as we can find.
> 
> I think this behaviour is reasonable - and avoids hopelessly confusing
> 16-character hex strings with lots of leading zeros.
> 
> But the code looks a bit odd. Do you think we should add a % formatter
> to print a phys_addr_t?
> 

Do you mean %pa? I tried and the result looks weird on mkimage. I didn't spend
time on it, thinking it could be host CC issue.

I will work on your other comments.

York