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

Simon Glass sjg at chromium.org
Wed Sep 2 16:05:37 CEST 2015


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?

>         }
>
>         /* Process all hash subnodes of the component image node */
> @@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>   * fit_image_get_load() - get load addr property for given component image node
>   * @fit: pointer to the FIT format image header
>   * @noffset: component image node offset
> - * @load: pointer to the uint32_t, will hold load address
> + * @load: pointer to the phys_addr_t, will hold load address
>   *
>   * fit_image_get_load() finds load address property in a given component
>   * image node. If the property is found, its value is returned to the caller.
> @@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>   *     0, on success
>   *     -1, on failure
>   */
> -int fit_image_get_load(const void *fit, int noffset, ulong *load)
> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>  {
>         int len;
>         const uint32_t *data;
> @@ -699,7 +699,18 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>                 return -1;
>         }
>
> -       *load = uimage_to_cpu(*data);
> +       if (len == 4) {
> +               *load = uimage_to_cpu(*data);
> +       } else if (len == 8 && sizeof(*load) >= 8) {
> +               *load = uimage_to_cpu(data[0]);
> +               *load <<= 32;
> +               *load |= uimage_to_cpu(data[1]);
> +       } else {
> +               printf("Unsupported load address length %d (%zd)\n",
> +                      len, sizeof(*load));
> +               return -1;

Can this use fdtdec_get_number()? I'm not sure we need this error message.

> +       }
> +
>         return 0;
>  }
>
> @@ -707,7 +718,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>   * fit_image_get_entry() - get entry point address property
>   * @fit: pointer to the FIT format image header
>   * @noffset: component image node offset
> - * @entry: pointer to the uint32_t, will hold entry point address
> + * @entry: pointer to the phys_addr_t, will hold entry point address
>   *
>   * This gets the entry point address property for a given component image
>   * node.
> @@ -720,7 +731,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>   *     0, on success
>   *     -1, on failure
>   */
> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
>  {
>         int len;
>         const uint32_t *data;
> @@ -731,7 +742,18 @@ int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
>                 return -1;
>         }
>
> -       *entry = uimage_to_cpu(*data);
> +       if (len == 4) {
> +               *entry = uimage_to_cpu(*data);
> +       } else if (len == 8 && sizeof(*entry) >= 8) {
> +               *entry = uimage_to_cpu(data[0]);
> +               *entry <<= 32;
> +               *entry |= uimage_to_cpu(data[1]);
> +       } else {
> +               printf("Unsupported entry address length %d (%zd)\n",
> +                      len, sizeof(*entry));
> +               return -1;

Ditto - in fact this code seems duplicated.

> +       }
> +
>         return 0;
>  }
>
> @@ -1554,7 +1576,7 @@ static const char *fit_get_image_type_property(int type)
>  int fit_image_load(bootm_headers_t *images, ulong addr,
>                    const char **fit_unamep, const char **fit_uname_configp,
>                    int arch, int image_type, int bootstage_id,
> -                  enum fit_load_op load_op, ulong *datap, ulong *lenp)
> +                  enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp)
>  {
>         int cfg_noffset, noffset;
>         const char *fit_uname;
> @@ -1563,7 +1585,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>         const void *buf;
>         size_t size;
>         int type_ok, os_ok;
> -       ulong load, data, len;
> +       phys_addr_t load;
> +       ulong data, len;
>         uint8_t os;
>         const char *prop_name;
>         int ret;
> @@ -1719,7 +1742,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>                 }
>         } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
>                 ulong image_start, image_end;
> -               ulong load_end;
> +               phys_addr_t load_end;
>                 void *dst;
>
>                 /*
> @@ -1736,8 +1759,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>                         return -EXDEV;
>                 }
>
> -               printf("   Loading %s from 0x%08lx to 0x%08lx\n",
> -                      prop_name, data, load);
> +               printf("   Loading %s from 0x%08lx to %08llx\n",
> +                      prop_name, data, (uint64_t)load);
>
>                 dst = map_sysmem(load, len);
>                 memmove(dst, buf, len);
> @@ -1756,7 +1779,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>  }
>
>  int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
> -                       ulong *setup_start, ulong *setup_len)
> +                       phys_addr_t *setup_start, ulong *setup_len)
>  {
>         int noffset;
>         ulong addr;
> diff --git a/include/bootm.h b/include/bootm.h
> index 4981377..f280ace 100644
> --- a/include/bootm.h
> +++ b/include/bootm.h
> @@ -69,8 +69,8 @@ void arch_preboot_os(void);
>   * @unc_len:   Available space for decompression
>   * @return 0 if OK, -ve on error (BOOTM_ERR_...)
>   */
> -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);
>
>  #endif
> diff --git a/include/image.h b/include/image.h
> index 63c3d37..bc41ecb 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -33,11 +33,15 @@ struct lmb;
>  #define IMAGE_ENABLE_IGNORE    0
>  #define IMAGE_INDENT_STRING    ""
>
> +/* Be able to hold physical address */
> +typedef unsigned long long phys_addr_t;
> +
>  #else
>
>  #include <lmb.h>
>  #include <asm/u-boot.h>
>  #include <command.h>
> +#include <linux/types.h>
>
>  /* Take notice of the 'ignore' property for hashes */
>  #define IMAGE_ENABLE_IGNORE    1
> @@ -494,7 +498,7 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
>  #endif /* !USE_HOSTCC */
>
>  int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
> -                      ulong *setup_start, ulong *setup_len);
> +                      phys_addr_t *setup_start, ulong *setup_len);
>
>  /**
>   * fit_image_load() - load an image from a FIT
> @@ -529,7 +533,7 @@ int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
>  int fit_image_load(bootm_headers_t *images, ulong addr,
>                    const char **fit_unamep, const char **fit_uname_configp,
>                    int arch, int image_type, int bootstage_id,
> -                  enum fit_load_op load_op, ulong *datap, ulong *lenp);
> +                  enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp);
>
>  #ifndef USE_HOSTCC
>  /**
> @@ -840,8 +844,8 @@ int fit_image_get_os(const void *fit, int noffset, uint8_t *os);
>  int fit_image_get_arch(const void *fit, int noffset, uint8_t *arch);
>  int fit_image_get_type(const void *fit, int noffset, uint8_t *type);
>  int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp);
> -int fit_image_get_load(const void *fit, int noffset, ulong *load);
> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry);
> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load);
> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry);
>  int fit_image_get_data(const void *fit, int noffset,
>                                 const void **data, size_t *size);
>
> --
> 1.7.9.5
>

Regards,
Simon


More information about the U-Boot mailing list