On 02/16/2016 08:01 AM, Simon Glass wrote:
Hi York,
On 12 February 2016 at 13:59, York Sun york.sun@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.sun@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