[PATCH 1/3] boot: fdt: Change type of env_get_bootm_low() to phys_addr_t
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; }
@@ -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
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare $addr as phys_addr_t and get rid of the casts.
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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high; + phys_addr_t addr; phys_addr_t low; phys_size_t mapsize; ulong of_len = 0; @@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL); - ulong addr;
if (desired_addr == ~0UL) { /* All ones means use fdt in place */ @@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * At least part of this DRAM bank is usable, try * using it for LMB allocation. */ - of_start = map_sysmem((ulong)lmb_alloc_base(lmb, - of_len, 0x1000, usable), of_len); + addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); + of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) break;
On 3/17/24 07:16, Marek Vasut wrote:
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare $addr as phys_addr_t and
%s/$addr/addr/
get rid of the casts.
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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high;
- phys_addr_t addr;
Please, keep variables in the narrowest scope where they are used.
Best regards
Heinrich
phys_addr_t low; phys_size_t mapsize; ulong of_len = 0; @@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL);
ulong addr;if (desired_addr == ~0UL) { /* All ones means use fdt in place */
@@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * At least part of this DRAM bank is usable, try * using it for LMB allocation. */
of_start = map_sysmem((ulong)lmb_alloc_base(lmb,of_len, 0x1000, usable), of_len);
addr = lmb_alloc_base(lmb, of_len, 0x1000, usable);
of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) break;
On 3/17/24 10:16 AM, Heinrich Schuchardt wrote:
On 3/17/24 07:16, Marek Vasut wrote:
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare $addr as phys_addr_t and
%s/$addr/addr/
I'm not sure this helps readability, I want to delimit the variable name somehow, so I switched this to 'addr' here and for 'usable' in 3/3.
get rid of the casts.
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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high; + phys_addr_t addr;
Please, keep variables in the narrowest scope where they are used.
Have a look at the entire function, this is the narrowest scope, $addr is used in both branches of the conditional now, in the same way too.
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:30AM +0100, Marek Vasut wrote:
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare $addr as phys_addr_t and
s/$addr/addr/ ?
get rid of the casts.
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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high;
- phys_addr_t addr; phys_addr_t low; phys_size_t mapsize; ulong of_len = 0;
@@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL);
ulong addr;
I would keep addr declared here. With that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
if (desired_addr == ~0UL) { /* All ones means use fdt in place */@@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * At least part of this DRAM bank is usable, try * using it for LMB allocation. */
of_start = map_sysmem((ulong)lmb_alloc_base(lmb,of_len, 0x1000, usable), of_len);
addr = lmb_alloc_base(lmb, of_len, 0x1000, usable);of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) break;
On 3/17/24 12:26 PM, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:30AM +0100, Marek Vasut wrote:
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare $addr as phys_addr_t and
s/$addr/addr/ ?
get rid of the casts.
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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high;
- phys_addr_t addr; phys_addr_t low; phys_size_t mapsize; ulong of_len = 0;
@@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL);
ulong addr;I would keep addr declared here. With that,
I already replied to Heinrich on both parts, let's continue the discussion there.
Move the variable below comment which explains what the variable means. Update the comment. No functional change.
Suggested-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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c37442c9130..16cd15e7e44 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -218,12 +218,12 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue;
- usable = min(start + size, low + mapsize); - /* * At least part of this DRAM bank is usable, try - * using it for LMB allocation. + * using the the DRAM bank up to $usable address + * limit for LMB allocation. */ + usable = min(start + size, low + mapsize); addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:31AM +0100, Marek Vasut wrote:
Move the variable below comment which explains what the variable means. Update the comment. No functional change.
Suggested-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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c37442c9130..16cd15e7e44 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -218,12 +218,12 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue;
usable = min(start + size, low + mapsize);/* * At least part of this DRAM bank is usable, try* using it for LMB allocation.
* using the the DRAM bank up to $usable address
s/the the/the/
Is it a u-boot convention to write $variable to refer to a variable in comments ?
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
* limit for LMB allocation. */usable = min(start + size, low + mapsize); addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */
On 3/17/24 12:29 PM, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:31AM +0100, Marek Vasut wrote:
Move the variable below comment which explains what the variable means. Update the comment. No functional change.
Suggested-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/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c37442c9130..16cd15e7e44 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -218,12 +218,12 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue;
usable = min(start + size, low + mapsize);/* * At least part of this DRAM bank is usable, try* using it for LMB allocation.
* using the the DRAM bank up to $usable addresss/the the/the/
Is it a u-boot convention to write $variable to refer to a variable in comments ?
I don't think there is any convention so far, but some convention would be good to discern what is a variable and what is a regular word, esp. since 'usable' appears in both forms in the comment. Kerneldoc @usable could be an option, shell $usable could also be an option.
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
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 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.
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);
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 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);Can't you write
return (phys_addr_t)simple_strtoull(s, NULL, 16);and avoid the temporary variable ?
Fixed in V2, thanks
participants (4)
-
Heinrich Schuchardt -
Laurent Pinchart -
Marek Vasut -
Marek Vasut