| Message ID | 20240414183759.146925-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | 16da853114efa97682920356a0eaa6a34270477f |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | boot: fdt: Turn all addresses and sizes into u64 | expand |
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!
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;
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(-)