On 02/16/2016 08:01 AM, Simon Glass wrote: > Hi York, > > On 12 February 2016 at 13:59, York Sun <york....@nxp.com> wrote: >> When dealing with image addresses, ulong has been used. Some files >> are used by both host and target. It is OK for the target, but not >> always enough for host tools including mkimage. This patch replaces >> "ulong" with "phys_addr_t" to make sure addresses are correct for >> both the target and the host. >> >> This issue was found on 32-bit host when compiling for 64-bit target >> to support images with address higher than 32-bit space. >> >> Signed-off-by: York Sun <york....@nxp.com> >> >> --- >> >> Changes in v4: >> New patch, separated from fixing FIT image. >> >> Changes in v3: None >> Changes in v2: None >> >> arch/powerpc/lib/bootm.c | 4 ++-- >> cmd/ximg.c | 9 +++++---- >> common/bootm.c | 21 +++++++++++---------- >> common/bootm_os.c | 14 ++++++++------ >> common/image-android.c | 6 +++--- >> common/image-fdt.c | 16 ++++++++-------- >> common/image-fit.c | 27 ++++++++++++++------------- >> common/image.c | 17 ++++++++++------- >> include/bootm.h | 6 +++--- >> include/image.h | 30 ++++++++++++++++++------------ >> tools/default_image.c | 2 +- >> 11 files changed, 83 insertions(+), 69 deletions(-) >> >> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c >> index ef15e7a..794382a 100644 >> --- a/arch/powerpc/lib/bootm.c >> +++ b/arch/powerpc/lib/bootm.c >> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images) >> #endif >> >> kernel = (void (*)(bd_t *, ulong, ulong, ulong, >> - ulong, ulong, ulong))images->ep; >> + ulong, ulong, ulong))(uintptr_t)images->ep; >> debug ("## Transferring control to Linux (at address %08lx) ...\n", >> (ulong)kernel); >> >> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images) >> WATCHDOG_RESET(); >> >> ((void (*)(void *, ulong, ulong, ulong, >> - ulong, ulong, ulong))images->ep)(images->ft_addr, >> + ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr, >> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0); >> } >> #endif >> diff --git a/cmd/ximg.c b/cmd/ximg.c >> index d033c15..70d6d14 100644 >> --- a/cmd/ximg.c >> +++ b/cmd/ximg.c >> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char >> * const argv[]) >> { >> ulong addr = load_addr; >> ulong dest = 0; >> - ulong data, len; >> + phys_addr_t data; >> + ulong len; >> int verify; >> int part = 0; >> #if defined(CONFIG_IMAGE_FORMAT_LEGACY) >> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, >> char * const argv[]) >> return 1; >> } >> >> - data = (ulong)fit_data; >> + data = (phys_addr_t)(uintptr_t)fit_data; >> len = (ulong)fit_len; >> break; >> #endif >> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, >> char * const argv[]) >> } >> #else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */ >> printf(" Loading part %d ... ", part); >> - memmove((char *) dest, (char *)data, len); >> + memmove((char *)dest, (char *)(uintptr_t)data, len); >> #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */ >> break; >> #ifdef CONFIG_GZIP >> case IH_COMP_GZIP: >> printf(" Uncompressing part %d ... ", part); >> if (gunzip((void *) dest, unc_len, >> - (uchar *) data, &len) != 0) { >> + (uchar *)(uintptr_t)data, &len) != 0) { > > The uintptr_t cast presumably defeats the warning. Is the intention to > convert a 64-bit value to 32-bit?
Yes. Before this patch, all these addresses are ulong. It is enough to hold the addresses _used_ for images on 32- and 64-bit targets. Please note, the key here is the address used. On system with 36 or more bits for physical addresses, the phys_addr_t is "unsigned long long". If the core is in 32-bit, the images are actually put in 32-bit space. So it is safe to case the 64-bit phys_addr_t to a 32-bit pointer. The idea behind this change is to make the host tool (such as mkimge) to support 64-bit address, even on a 32-bit host. My solution is to always use 64-bit variable on host. I didn't find a better way to deal with this issue. > > Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target? No. On some arch (eg arm, mips, powerpc), phys_addr_t can be either "unsigned long long", or "unsigned long". > >> puts("GUNZIP ERROR - image not loaded\n"); >> return 1; >> } >> diff --git a/common/bootm.c b/common/bootm.c >> index 99d574d..785858c 100644 >> --- a/common/bootm.c >> +++ b/common/bootm.c >> @@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR; >> >> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, >> char * const argv[], bootm_headers_t >> *images, >> - ulong *os_data, ulong *os_len); >> + phys_addr_t *os_data, ulong *os_len); >> >> #ifdef CONFIG_LMB >> static void boot_start_lmb(bootm_headers_t *images) >> @@ -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; >> >> @@ -767,7 +767,7 @@ static image_header_t *image_get_kernel(ulong img_addr, >> int verify) >> >> /** >> * boot_get_kernel - find kernel image >> - * @os_data: pointer to a ulong variable, will hold os data start address >> + * @os_data: pointer to a phys_addr_t variable, will hold os data start >> address >> * @os_len: pointer to a ulong variable, will hold os data length >> * >> * boot_get_kernel() tries to find a kernel image, verifies its integrity >> @@ -779,7 +779,7 @@ static image_header_t *image_get_kernel(ulong img_addr, >> int verify) >> */ >> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, >> char * const argv[], bootm_headers_t >> *images, >> - ulong *os_data, ulong *os_len) >> + phys_addr_t *os_data, ulong *os_len) >> { >> #if defined(CONFIG_IMAGE_FORMAT_LEGACY) >> image_header_t *hdr; >> @@ -879,7 +879,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int >> flag, int argc, >> return NULL; >> } >> >> - debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n", >> + debug(" kernel data at " PRIpa ", len = 0x%08lx (%ld)\n", >> *os_data, *os_len, *os_len); >> >> return buf; >> @@ -894,7 +894,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; > > This looks suspicious. Why is it changing to a pointer? It does look suspicious. I must carried the change from an earlier patch. > >> + ulong len; >> bootm_headers_t images; >> int noffset; >> ulong load_end; >> @@ -908,7 +909,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); > > Won't this pass a NULL pointer? This must be wrong. Let me debug it. > >> if (noffset < 0) >> return noffset; >> if (fit_image_get_type(fit, noffset, &image_type)) { >> @@ -923,7 +924,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/bootm_os.c b/common/bootm_os.c >> index cb83f4a..74276f6 100644 >> --- a/common/bootm_os.c >> +++ b/common/bootm_os.c >> @@ -26,7 +26,7 @@ static int do_bootm_standalone(int flag, int argc, char * >> const argv[], >> setenv_hex("filesize", images->os.image_len); >> return 0; >> } >> - appl = (int (*)(int, char * const []))images->ep; >> + appl = (int (*)(int, char * const []))(uintptr_t)images->ep; >> appl(argc, argv); >> return 0; >> } >> @@ -55,7 +55,8 @@ static int do_bootm_netbsd(int flag, int argc, char * >> const argv[], >> { >> void (*loader)(bd_t *, image_header_t *, char *, char *); >> image_header_t *os_hdr, *hdr; >> - ulong kernel_data, kernel_len; >> + phys_addr_t kernel_data; >> + ulong kernel_len; >> char *consdev; >> char *cmdline; >> >> @@ -113,7 +114,8 @@ static int do_bootm_netbsd(int flag, int argc, char * >> const argv[], >> cmdline = ""; >> } >> >> - loader = (void (*)(bd_t *, image_header_t *, char *, char >> *))images->ep; >> + loader = (void (*)(bd_t *, image_header_t *, char *, char *)) >> + (uintptr_t)images->ep; >> >> printf("## Transferring control to NetBSD stage-2 loader (at address >> %08lx) ...\n", >> (ulong)loader); >> @@ -171,7 +173,7 @@ static int do_bootm_rtems(int flag, int argc, char * >> const argv[], >> } >> #endif >> >> - entry_point = (void (*)(bd_t *))images->ep; >> + entry_point = (void (*)(bd_t *))(uintptr_t)images->ep; >> >> printf("## Transferring control to RTEMS (at address %08lx) ...\n", >> (ulong)entry_point); >> @@ -252,7 +254,7 @@ static int do_bootm_plan9(int flag, int argc, char * >> const argv[], >> } >> } >> >> - entry_point = (void (*)(void))images->ep; >> + entry_point = (void (*)(void))(uintptr_t)images->ep; >> >> printf("## Transferring control to Plan 9 (at address %08lx) ...\n", >> (ulong)entry_point); >> @@ -364,7 +366,7 @@ static int do_bootm_qnxelf(int flag, int argc, char * >> const argv[], >> } >> #endif >> >> - sprintf(str, "%lx", images->ep); /* write entry-point into string */ >> + sprintf(str, PRIpa, images->ep); /* write entry-point into string */ >> local_args[0] = argv[0]; >> local_args[1] = str; /* and provide it via the arguments */ >> do_bootelf(NULL, 0, 2, local_args); >> diff --git a/common/image-android.c b/common/image-android.c >> index b6a94b3..7c574f8 100644 >> --- a/common/image-android.c >> +++ b/common/image-android.c >> @@ -38,7 +38,7 @@ static ulong android_image_get_kernel_addr(const struct >> andr_img_hdr *hdr) >> * @hdr: Pointer to image header, which is at the start >> * of the image. >> * @verify: Checksum verification flag. Currently unimplemented. >> - * @os_data: Pointer to a ulong variable, will hold os data start >> + * @os_data: Pointer to a phys_addr_t variable, will hold os data start >> * address. >> * @os_len: Pointer to a ulong variable, will hold os data length. >> * >> @@ -49,7 +49,7 @@ static ulong android_image_get_kernel_addr(const struct >> andr_img_hdr *hdr) >> * otherwise on failure. >> */ >> int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify, >> - ulong *os_data, ulong *os_len) >> + phys_addr_t *os_data, ulong *os_len) >> { >> u32 kernel_addr = android_image_get_kernel_addr(hdr); >> >> @@ -93,7 +93,7 @@ int android_image_get_kernel(const struct andr_img_hdr >> *hdr, int verify, >> setenv("bootargs", newbootargs); >> >> if (os_data) { >> - *os_data = (ulong)hdr; >> + *os_data = (phys_addr_t)hdr; >> *os_data += hdr->page_size; >> } >> if (os_len) >> diff --git a/common/image-fdt.c b/common/image-fdt.c >> index 5e4e5bd..bb637d7 100644 >> --- a/common/image-fdt.c >> +++ b/common/image-fdt.c >> @@ -223,11 +223,14 @@ error: >> int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, >> bootm_headers_t *images, char **of_flat_tree, ulong *of_size) >> { >> + phys_addr_t load; >> #if defined(CONFIG_IMAGE_FORMAT_LEGACY) >> const image_header_t *fdt_hdr; >> - ulong load, load_end; >> + phys_addr_t load_end; >> ulong image_start, image_data, image_end; >> #endif >> + phys_addr_t fdt_data; >> + ulong fdt_len; >> ulong fdt_addr; >> char *fdt_blob = NULL; >> void *buf; >> @@ -236,6 +239,7 @@ int boot_get_fdt(int flag, int argc, char * const >> argv[], uint8_t arch, >> const char *fit_uname_fdt = NULL; >> ulong default_addr; >> int fdt_noffset; >> + ulong len; >> #endif >> const char *select = NULL; >> int ok_no_fdt = 0; >> @@ -335,10 +339,10 @@ int boot_get_fdt(int flag, int argc, char * const >> argv[], uint8_t arch, >> goto error; >> } >> >> - debug(" Loading FDT from 0x%08lx to 0x%08lx\n", >> + debug(" Loading FDT from 0x%08lx to " PRIpa "\n", >> image_data, load); >> >> - memmove((void *)load, >> + memmove((void *)(uintptr_t)load, >> (void *)image_data, >> image_get_data_size(fdt_hdr)); >> >> @@ -354,8 +358,6 @@ int boot_get_fdt(int flag, int argc, char * const >> argv[], uint8_t arch, >> #if defined(CONFIG_FIT) >> /* check FDT blob vs FIT blob */ >> if (fit_check_format(buf)) { >> - ulong load, len; >> - >> fdt_noffset = fit_image_load(images, >> fdt_addr, &fit_uname_fdt, >> &fit_uname_config, >> @@ -389,8 +391,6 @@ int boot_get_fdt(int flag, int argc, char * const >> argv[], uint8_t arch, >> } else if (images->legacy_hdr_valid && >> image_check_type(&images->legacy_hdr_os_copy, >> IH_TYPE_MULTI)) { >> - ulong fdt_data, fdt_len; >> - >> /* >> * Now check if we have a legacy multi-component image, >> * get second entry data start address and len. >> @@ -401,7 +401,7 @@ int boot_get_fdt(int flag, int argc, char * const >> argv[], uint8_t arch, >> image_multi_getimg(images->legacy_hdr_os, 2, &fdt_data, >> &fdt_len); >> if (fdt_len) { >> - fdt_blob = (char *)fdt_data; >> + fdt_blob = (char *)(uintptr_t)fdt_data; >> printf(" Booting using the fdt at 0x%p\n", >> fdt_blob); >> >> if (fdt_check_header(fdt_blob) != 0) { >> diff --git a/common/image-fit.c b/common/image-fit.c >> index c531ee7..bfa76a2 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,7 +428,7 @@ 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(PRIpa "\n", load); >> } >> >> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) || >> @@ -438,7 +438,7 @@ void fit_image_print(const void *fit, int image_noffset, >> const char *p) >> if (ret) >> printf("unavailable\n"); >> else >> - printf("0x%08lx\n", entry); >> + printf(PRIpa "\n", entry); >> } >> >> /* 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; >> @@ -707,7 +707,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 +720,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; >> @@ -1556,7 +1556,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; >> @@ -1565,7 +1565,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; >> @@ -1721,7 +1722,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; >> >> /* >> @@ -1738,8 +1739,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); > > Do you need to cast? Why not use your magic printf() string #define? Sure. Let me respin the patch and double confirm on an ARMv8 board. York _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot