[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