[U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses

Simon Glass sjg at chromium.org
Wed Feb 24 00:28:08 CET 2016


Hi York,

On 23 February 2016 at 13:36, york sun <york.sun at 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.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".

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


More information about the U-Boot mailing list