[U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses
york sun
york.sun at nxp.com
Tue Feb 23 21:36:11 CET 2016
On 02/16/2016 08:01 AM, Simon Glass wrote:
> Hi York,
>
> On 12 February 2016 at 13:59, York Sun <york.sun at 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 at 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
More information about the U-Boot
mailing list