Hi York,
On 23 February 2016 at 13:36, york sun <york....@nxp.com> wrote:
> 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".
OK - can you please add a comment in the code here? I understand what
you are saying but it will not be obvious from the code.
>
>>
>>> 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.
OK
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
