[PATCH] boot: fdt: Turn all addresses and sizes into u64
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types. This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
Fixes: a4df06e41fa2 ("boot: fdt: Change type of env_get_bootm_low() to phys_addr_t") Signed-off-by: Marek Vasut marex@denx.de --- Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Matthias Schiffer matthias.schiffer@ew.tq-group.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- boot/image-fdt.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 2b92bdaff16..f09716cba30 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -158,13 +158,10 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob) */ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) { + u64 start, size, usable, addr, low, mapsize; void *fdt_blob = *of_flat_tree; 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; int bank; int err;
Hi Marek,
Thank you for the patch.
On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote:
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types.
I'm not sure I like this much, as it removes a useful indication regarding what the variables store. Wouldn't it be better if the code's logic could be modified to avoid those overflows ?
This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
Shouldn't phys_addr_t be a 64-bit type on PAE systems ?
Fixes: a4df06e41fa2 ("boot: fdt: Change type of env_get_bootm_low() to phys_addr_t") Signed-off-by: Marek Vasut marex@denx.de
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Matthias Schiffer matthias.schiffer@ew.tq-group.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
boot/image-fdt.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 2b92bdaff16..f09716cba30 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -158,13 +158,10 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob) */ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) {
- u64 start, size, usable, addr, low, mapsize; void *fdt_blob = *of_flat_tree; 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; int bank; int err;
On 4/14/24 9:29 PM, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote:
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types.
I'm not sure I like this much, as it removes a useful indication regarding what the variables store.
That's what the variable name is for, not variable type.
Wouldn't it be better if the code's logic could be modified to avoid those overflows ?
I'd prefer to keep the code simple and blanket avoid the overflows for a very long time, rather than play whack-a-mole with various odd corner cases here.
Note that this is a fix for a previous series which changed from u64/ulong to phys_addr/size_t , which clearly was incorrect .
This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
Shouldn't phys_addr_t be a 64-bit type on PAE systems ?
That depends on CONFIG_PHYS_64BIT , on am57xx this is not set for example, so there phys_addr_t is 32bit .
On Sun, Apr 14, 2024 at 11:25:06PM +0200, Marek Vasut wrote:
On 4/14/24 9:29 PM, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote:
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types.
I'm not sure I like this much, as it removes a useful indication regarding what the variables store.
That's what the variable name is for, not variable type.
Wouldn't it be better if the code's logic could be modified to avoid those overflows ?
I'd prefer to keep the code simple and blanket avoid the overflows for a very long time, rather than play whack-a-mole with various odd corner cases here.
Up to you.
Note that this is a fix for a previous series which changed from u64/ulong to phys_addr/size_t , which clearly was incorrect .
This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
Shouldn't phys_addr_t be a 64-bit type on PAE systems ?
That depends on CONFIG_PHYS_64BIT , on am57xx this is not set for example, so there phys_addr_t is 32bit .
The system won't be able to address more than 32 bits of memory in that case, would it ?
On 4/14/24 11:28 PM, Laurent Pinchart wrote:
On Sun, Apr 14, 2024 at 11:25:06PM +0200, Marek Vasut wrote:
On 4/14/24 9:29 PM, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote:
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types.
I'm not sure I like this much, as it removes a useful indication regarding what the variables store.
That's what the variable name is for, not variable type.
Wouldn't it be better if the code's logic could be modified to avoid those overflows ?
I'd prefer to keep the code simple and blanket avoid the overflows for a very long time, rather than play whack-a-mole with various odd corner cases here.
Up to you.
Note that this is a fix for a previous series which changed from u64/ulong to phys_addr/size_t , which clearly was incorrect .
This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
Shouldn't phys_addr_t be a 64-bit type on PAE systems ?
That depends on CONFIG_PHYS_64BIT , on am57xx this is not set for example, so there phys_addr_t is 32bit .
The system won't be able to address more than 32 bits of memory in that case, would it ?
It might do so through some memory window, like PCIe, but I never used the am57xx so I cannot tell what it really does.
On Sun, 14 Apr 2024 20:37:20 +0200, Marek Vasut wrote:
In case of systems where DRAM bank ends at the edge of 32bit boundary, start + size calculations would overflow. This happens on STM32MP15xx with 1 DRAM bank starting at 0xc0000000 and 1 GiB of DRAM. This is a usual 32bit system DRAM size overflow, fix it by doing all DRAM size and offset calculations using u64 types. This also covers a case where a 32bit PAE system might be able to address up to 36bits of DRAM.
[...]
Applied to u-boot/master, thanks!
participants (3)
-
Laurent Pinchart -
Marek Vasut -
Tom Rini