| Message ID | 20240317061730.654893-1-marek.vasut+renesas@mailbox.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/3] boot: fdt: Change type of env_get_bootm_low() to phys_addr_t | expand |
On 3/17/24 07:16, Marek Vasut wrote: > Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low(). > The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t, > while the function itself still returns ulong. This is potentially dangerous > on 64bit systems, where ulong might not be large enough to hold the content Isn't long always 64bit when building with gcc or llvm? > of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to > what env_get_bootm_size() does, which returns phys_size_t . > > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> > --- > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > boot/bootm.c | 2 +- > boot/image-board.c | 11 ++++++----- > boot/image-fdt.c | 9 +++++---- > include/image.h | 2 +- > 4 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/boot/bootm.c b/boot/bootm.c > index d071537d692..2e818264f5f 100644 > --- a/boot/bootm.c > +++ b/boot/bootm.c > @@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images, > #ifdef CONFIG_LMB > static void boot_start_lmb(struct bootm_headers *images) > { > - ulong mem_start; > + phys_addr_t mem_start; > phys_size_t mem_size; > > mem_start = env_get_bootm_low(); Please, remove the conversion to phys_addr_t in the lmb_init_and_reserve_range() invocation. > diff --git a/boot/image-board.c b/boot/image-board.c > index 75f6906cd56..447ced2678a 100644 > --- a/boot/image-board.c > +++ b/boot/image-board.c > @@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char *value, enum env_op op, > } > U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr); > > -ulong env_get_bootm_low(void) > +phys_addr_t env_get_bootm_low(void) > { > char *s = env_get("bootm_low"); > + phys_size_t tmp; > > if (s) { > - ulong tmp = hextoul(s, NULL); > + tmp = (phys_addr_t)simple_strtoull(s, NULL, 16); In C the conversion is superfluous. Please, remove '(phys_addr_t)'. Why do we need tmp? return simple_strtoull(s, NULL, 16); > return tmp; > } > > @@ -538,7 +539,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, > ulong *initrd_start, ulong *initrd_end) > { > char *s; > - ulong initrd_high; > + phys_addr_t initrd_high; > int initrd_copy_to_ram = 1; > > s = env_get("initrd_high"); > @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, > initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); > } > > - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", > - initrd_high, initrd_copy_to_ram); > + debug("## initrd_high = 0x%p, copy_to_ram = %d\n", > + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram); Void * may be narrower than phys_addr_t. Shouldn't we convert to unsigned long long for printing. Best regards Heinrich > > if (rd_data) { > if (!initrd_copy_to_ram) { /* zero-copy ramdisk support */ > diff --git a/boot/image-fdt.c b/boot/image-fdt.c > index 5e4aa9de0d2..boot/image-fdt.c > @@ -160,9 +160,10 @@c2571b22244 100644 > --- a/boot/image-fdt.c > +++ b/ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > { > void *fdt_blob = *of_flat_tree; > void *of_start = NULL; > - u64 start, size, usable; > + phys_addr_t start, size, usable; > char *fdt_high; > - ulong mapsize, low; > + phys_addr_t low; > + phys_size_t mapsize; > ulong of_len = 0; > int bank; > int err; > @@ -217,7 +218,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > if (start + size < low) > continue; > > - usable = min(start + size, (u64)(low + mapsize)); > + usable = min(start + size, low + mapsize); > > /* > * At least part of this DRAM bank is usable, try > @@ -233,7 +234,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > * Reduce the mapping size in the next bank > * by the size of attempt in current bank. > */ > - mapsize -= usable - max(start, (u64)low); > + mapsize -= usable - max(start, low); > if (!mapsize) > break; > } > diff --git a/include/image.h b/include/image.h > index 21de70f0c9e..acffd17e0df 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -946,7 +946,7 @@ static inline void image_set_name(struct legacy_img_hdr *hdr, const char *name) > int image_check_hcrc(const struct legacy_img_hdr *hdr); > int image_check_dcrc(const struct legacy_img_hdr *hdr); > #ifndef USE_HOSTCC > -ulong env_get_bootm_low(void); > +phys_addr_t env_get_bootm_low(void); > phys_size_t env_get_bootm_size(void); > phys_size_t env_get_bootm_mapsize(void); > #endif
Hi Marek, Thank you for the patch. On Sun, Mar 17, 2024 at 07:16:29AM +0100, Marek Vasut wrote: > Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low(). > The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t, > while the function itself still returns ulong. This is potentially dangerous > on 64bit systems, where ulong might not be large enough to hold the content > of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to > what env_get_bootm_size() does, which returns phys_size_t . > > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> > --- > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > boot/bootm.c | 2 +- > boot/image-board.c | 11 ++++++----- > boot/image-fdt.c | 9 +++++---- > include/image.h | 2 +- > 4 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/boot/bootm.c b/boot/bootm.c > index d071537d692..2e818264f5f 100644 > --- a/boot/bootm.c > +++ b/boot/bootm.c > @@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images, > #ifdef CONFIG_LMB > static void boot_start_lmb(struct bootm_headers *images) > { > - ulong mem_start; > + phys_addr_t mem_start; > phys_size_t mem_size; > > mem_start = env_get_bootm_low(); > diff --git a/boot/image-board.c b/boot/image-board.c > index 75f6906cd56..447ced2678a 100644 > --- a/boot/image-board.c > +++ b/boot/image-board.c > @@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char *value, enum env_op op, > } > U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr); > > -ulong env_get_bootm_low(void) > +phys_addr_t env_get_bootm_low(void) > { > char *s = env_get("bootm_low"); > + phys_size_t tmp; > > if (s) { > - ulong tmp = hextoul(s, NULL); > + tmp = (phys_addr_t)simple_strtoull(s, NULL, 16); > return tmp; Can't you write return (phys_addr_t)simple_strtoull(s, NULL, 16); and avoid the temporary variable ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > @@ -538,7 +539,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, > ulong *initrd_start, ulong *initrd_end) > { > char *s; > - ulong initrd_high; > + phys_addr_t initrd_high; > int initrd_copy_to_ram = 1; > > s = env_get("initrd_high"); > @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, > initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); > } > > - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", > - initrd_high, initrd_copy_to_ram); > + debug("## initrd_high = 0x%p, copy_to_ram = %d\n", > + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram); > > if (rd_data) { > if (!initrd_copy_to_ram) { /* zero-copy ramdisk support */ > diff --git a/boot/image-fdt.c b/boot/image-fdt.c > index 5e4aa9de0d2..c2571b22244 100644 > --- a/boot/image-fdt.c > +++ b/boot/image-fdt.c > @@ -160,9 +160,10 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > { > void *fdt_blob = *of_flat_tree; > void *of_start = NULL; > - u64 start, size, usable; > + phys_addr_t start, size, usable; > char *fdt_high; > - ulong mapsize, low; > + phys_addr_t low; > + phys_size_t mapsize; > ulong of_len = 0; > int bank; > int err; > @@ -217,7 +218,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > if (start + size < low) > continue; > > - usable = min(start + size, (u64)(low + mapsize)); > + usable = min(start + size, low + mapsize); > > /* > * At least part of this DRAM bank is usable, try > @@ -233,7 +234,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) > * Reduce the mapping size in the next bank > * by the size of attempt in current bank. > */ > - mapsize -= usable - max(start, (u64)low); > + mapsize -= usable - max(start, low); > if (!mapsize) > break; > } > diff --git a/include/image.h b/include/image.h > index 21de70f0c9e..acffd17e0df 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -946,7 +946,7 @@ static inline void image_set_name(struct legacy_img_hdr *hdr, const char *name) > int image_check_hcrc(const struct legacy_img_hdr *hdr); > int image_check_dcrc(const struct legacy_img_hdr *hdr); > #ifndef USE_HOSTCC > -ulong env_get_bootm_low(void); > +phys_addr_t env_get_bootm_low(void); > phys_size_t env_get_bootm_size(void); > phys_size_t env_get_bootm_mapsize(void); > #endif
On 3/17/24 10:08 AM, Heinrich Schuchardt wrote: > On 3/17/24 07:16, Marek Vasut wrote: >> Change type of ulong env_get_bootm_low() to phys_addr_t >> env_get_bootm_low(). >> The PPC/LS systems already treat env_get_bootm_low() result as >> phys_addr_t, >> while the function itself still returns ulong. This is potentially >> dangerous >> on 64bit systems, where ulong might not be large enough to hold the >> content > > Isn't long always 64bit when building with gcc or llvm? C99 spec says: 5.2.4.2.1 Sizes of integer types <limits.h> ... - maximum value for an object of type unsigned long int ULONG_MAX 4294967295 // 2^32 - 1 ... Simpler table: https://en.wikipedia.org/wiki/C_data_types So, to answer the question, it might, but we cannot rely on that. Also, phys_addr_t represents physical addresses, which this is. [...] >> @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong >> rd_data, ulong rd_len, >> initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); >> } >> >> - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", >> - initrd_high, initrd_copy_to_ram); >> + debug("## initrd_high = 0x%p, copy_to_ram = %d\n", >> + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram); > > Void * may be narrower than phys_addr_t. When would that happen, on PAE systems ? > Shouldn't we convert to unsigned long long for printing. Printing phys_addr_t always confuses me. I was under the impression that turning the value into uintptr_t (platform pointer type represented as integer) and then actually using that as a pointer for printing should not suffer from any type width problems. Does it ? Since this is a debug print, I can just upcast it to u64.
On 3/17/24 12:18 PM, Laurent Pinchart wrote: > Hi Marek, > > Thank you for the patch. > > On Sun, Mar 17, 2024 at 07:16:29AM +0100, Marek Vasut wrote: >> Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low(). >> The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t, >> while the function itself still returns ulong. This is potentially dangerous >> on 64bit systems, where ulong might not be large enough to hold the content >> of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to >> what env_get_bootm_size() does, which returns phys_size_t . >> >> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> >> --- >> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Tom Rini <trini@konsulko.com> >> --- >> boot/bootm.c | 2 +- >> boot/image-board.c | 11 ++++++----- >> boot/image-fdt.c | 9 +++++---- >> include/image.h | 2 +- >> 4 files changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/boot/bootm.c b/boot/bootm.c >> index d071537d692..2e818264f5f 100644 >> --- a/boot/bootm.c >> +++ b/boot/bootm.c >> @@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images, >> #ifdef CONFIG_LMB >> static void boot_start_lmb(struct bootm_headers *images) >> { >> - ulong mem_start; >> + phys_addr_t mem_start; >> phys_size_t mem_size; >> >> mem_start = env_get_bootm_low(); >> diff --git a/boot/image-board.c b/boot/image-board.c >> index 75f6906cd56..447ced2678a 100644 >> --- a/boot/image-board.c >> +++ b/boot/image-board.c >> @@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char *value, enum env_op op, >> } >> U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr); >> >> -ulong env_get_bootm_low(void) >> +phys_addr_t env_get_bootm_low(void) >> { >> char *s = env_get("bootm_low"); >> + phys_size_t tmp; >> >> if (s) { >> - ulong tmp = hextoul(s, NULL); >> + tmp = (phys_addr_t)simple_strtoull(s, NULL, 16); >> return tmp; > > Can't you write > > return (phys_addr_t)simple_strtoull(s, NULL, 16); > > and avoid the temporary variable ? Fixed in V2, thanks
On 17.03.24 17:58, Marek Vasut wrote: > On 3/17/24 10:08 AM, Heinrich Schuchardt wrote: >> On 3/17/24 07:16, Marek Vasut wrote: >>> Change type of ulong env_get_bootm_low() to phys_addr_t >>> env_get_bootm_low(). >>> The PPC/LS systems already treat env_get_bootm_low() result as >>> phys_addr_t, >>> while the function itself still returns ulong. This is potentially >>> dangerous >>> on 64bit systems, where ulong might not be large enough to hold the >>> content >> >> Isn't long always 64bit when building with gcc or llvm? > > C99 spec says: > > 5.2.4.2.1 Sizes of integer types <limits.h> > ... > - maximum value for an object of type unsigned long int > ULONG_MAX 4294967295 // 2^32 - 1 > ... > > Simpler table: > https://en.wikipedia.org/wiki/C_data_types > > So, to answer the question, it might, but we cannot rely on that. > > Also, phys_addr_t represents physical addresses, which this is. > > [...] > >>> @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong >>> rd_data, ulong rd_len, >>> initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); >>> } >>> >>> - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", >>> - initrd_high, initrd_copy_to_ram); >>> + debug("## initrd_high = 0x%p, copy_to_ram = %d\n", >>> + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram); >> >> Void * may be narrower than phys_addr_t. > > When would that happen, on PAE systems ? > >> Shouldn't we convert to unsigned long long for printing. > > Printing phys_addr_t always confuses me. I was under the impression that > turning the value into uintptr_t (platform pointer type represented as > integer) and then actually using that as a pointer for printing should > not suffer from any type width problems. Does it ? As you already remarked on LPAE this may happen. Though you may not be able load initrd outside the address range of void* the variables might exceed it. Best regards Heinrich > > Since this is a debug print, I can just upcast it to u64.
On 3/17/24 6:18 PM, Heinrich Schuchardt wrote: [...] >>> Shouldn't we convert to unsigned long long for printing. >> >> Printing phys_addr_t always confuses me. I was under the impression that >> turning the value into uintptr_t (platform pointer type represented as >> integer) and then actually using that as a pointer for printing should >> not suffer from any type width problems. Does it ? > > As you already remarked on LPAE this may happen. > > Though you may not be able load initrd outside the address range of > void* the variables might exceed it. ACK, thanks for the confirmation.
diff --git a/boot/bootm.c b/boot/bootm.c index d071537d692..2e818264f5f 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images, #ifdef CONFIG_LMB static void boot_start_lmb(struct bootm_headers *images) { - ulong mem_start; + phys_addr_t mem_start; phys_size_t mem_size; mem_start = env_get_bootm_low(); diff --git a/boot/image-board.c b/boot/image-board.c index 75f6906cd56..447ced2678a 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char *value, enum env_op op, } U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr); -ulong env_get_bootm_low(void) +phys_addr_t env_get_bootm_low(void) { char *s = env_get("bootm_low"); + phys_size_t tmp; if (s) { - ulong tmp = hextoul(s, NULL); + tmp = (phys_addr_t)simple_strtoull(s, NULL, 16); return tmp; } @@ -538,7 +539,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, ulong *initrd_start, ulong *initrd_end) { char *s; - ulong initrd_high; + phys_addr_t initrd_high; int initrd_copy_to_ram = 1; s = env_get("initrd_high"); @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); } - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", - initrd_high, initrd_copy_to_ram); + debug("## initrd_high = 0x%p, copy_to_ram = %d\n", + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram); if (rd_data) { if (!initrd_copy_to_ram) { /* zero-copy ramdisk support */ diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 5e4aa9de0d2..c2571b22244 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -160,9 +160,10 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) { void *fdt_blob = *of_flat_tree; void *of_start = NULL; - u64 start, size, usable; + phys_addr_t start, size, usable; char *fdt_high; - ulong mapsize, low; + phys_addr_t low; + phys_size_t mapsize; ulong of_len = 0; int bank; int err; @@ -217,7 +218,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue; - usable = min(start + size, (u64)(low + mapsize)); + usable = min(start + size, low + mapsize); /* * At least part of this DRAM bank is usable, try @@ -233,7 +234,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * Reduce the mapping size in the next bank * by the size of attempt in current bank. */ - mapsize -= usable - max(start, (u64)low); + mapsize -= usable - max(start, low); if (!mapsize) break; } diff --git a/include/image.h b/include/image.h index 21de70f0c9e..acffd17e0df 100644 --- a/include/image.h +++ b/include/image.h @@ -946,7 +946,7 @@ static inline void image_set_name(struct legacy_img_hdr *hdr, const char *name) int image_check_hcrc(const struct legacy_img_hdr *hdr); int image_check_dcrc(const struct legacy_img_hdr *hdr); #ifndef USE_HOSTCC -ulong env_get_bootm_low(void); +phys_addr_t env_get_bootm_low(void); phys_size_t env_get_bootm_size(void); phys_size_t env_get_bootm_mapsize(void); #endif
Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low(). The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t, while the function itself still returns ulong. This is potentially dangerous on 64bit systems, where ulong might not be large enough to hold the content of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to what env_get_bootm_size() does, which returns phys_size_t . Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> --- Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> --- boot/bootm.c | 2 +- boot/image-board.c | 11 ++++++----- boot/image-fdt.c | 9 +++++---- include/image.h | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-)