[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow
When constraining the highest DDR address that U-Boot will use for its data & relocated self, we need to handle the common case in which a 32 bit system with 2GB DDR will have a zero gd->ram_top, due to the addition of 2GB (0x80000000) to the base address of kseg0 (also 0x80000000) which overflows & wraps to 0.
We originally had a check for this case, but it was lost in commit 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") causing problems for the affected 32 bit systems.
Signed-off-by: Paul Burton paul.burton@mips.com Fixes: 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
--- Feel free to fold this into 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") if you prefer Daniel.
board/imgtec/boston/ddr.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/board/imgtec/boston/ddr.c b/board/imgtec/boston/ddr.c index 00428496bd..892bb1754c 100644 --- a/board/imgtec/boston/ddr.c +++ b/board/imgtec/boston/ddr.c @@ -26,6 +26,15 @@ int dram_init(void) ulong board_get_usable_ram_top(ulong total_size) { DECLARE_GLOBAL_DATA_PTR; + ulong max_top;
- return min_t(ulong, gd->ram_top, (ulong)phys_to_virt(SZ_256M)); + /* Limit memory use to the top of (c)kseg0 */ + max_top = (ulong)phys_to_virt(SZ_256M); + + if (gd->ram_top < (ulong)phys_to_virt(0)) { + /* >= 2GB RAM on a 32 bit system wrapped around to 0 */ + return max_top; + } + + return min_t(ulong, gd->ram_top, max_top); }
On 18.01.2018 22:19, Paul Burton wrote:
When constraining the highest DDR address that U-Boot will use for its data & relocated self, we need to handle the common case in which a 32 bit system with 2GB DDR will have a zero gd->ram_top, due to the addition of 2GB (0x80000000) to the base address of kseg0 (also 0x80000000) which overflows & wraps to 0.
We originally had a check for this case, but it was lost in commit 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") causing problems for the affected 32 bit systems.
I think I did a wrong conflict resolution because the patch didn't apply anymore. I folded this patch into "boston: Provide physical CONFIG_SYS_SDRAM_BASE" to fix this. Actually I wanted to resend the updated patches. But if you are okay with the current state in u-boot-mips/next branch, I'll take them as they are.
BTW: could you resend your series "boston: Ethernet support for MIPS Boston board"? I still have no Acks or Reviews on the generic DM parts. Thanks.
Hi Daniel,
On Fri, Jan 19, 2018 at 12:31:25PM +0100, Daniel Schwierzeck wrote:
On 18.01.2018 22:19, Paul Burton wrote:
When constraining the highest DDR address that U-Boot will use for its data & relocated self, we need to handle the common case in which a 32 bit system with 2GB DDR will have a zero gd->ram_top, due to the addition of 2GB (0x80000000) to the base address of kseg0 (also 0x80000000) which overflows & wraps to 0.
We originally had a check for this case, but it was lost in commit 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") causing problems for the affected 32 bit systems.
I think I did a wrong conflict resolution because the patch didn't apply anymore. I folded this patch into "boston: Provide physical CONFIG_SYS_SDRAM_BASE" to fix this. Actually I wanted to resend the updated patches. But if you are okay with the current state in u-boot-mips/next branch, I'll take them as they are.
BTW: could you resend your series "boston: Ethernet support for MIPS Boston board"? I still have no Acks or Reviews on the generic DM parts. Thanks.
When I last fetched from u-boot-mips.git I saw patches up to 564cc3a11c45 ("mips: Remove virt_to_phys call on bi_memstart") in the next branch, which I have then rebased my ethernet patches atop with the result working fine on a real Boston board.
I see that next now contains only 2 patches up to d2a4e3664150 ("mips: bmips: increment SYS_MALLOC_F_LEN") and has dropped the patches switching to a physical CONFIG_SYS_SDRAM_BASE. Would you like me to rebase those plus the Boston ethernet support atop the current next branch?
Thanks, Paul
On 22.01.2018 19:01, Paul Burton wrote:
Hi Daniel,
On Fri, Jan 19, 2018 at 12:31:25PM +0100, Daniel Schwierzeck wrote:
On 18.01.2018 22:19, Paul Burton wrote:
When constraining the highest DDR address that U-Boot will use for its data & relocated self, we need to handle the common case in which a 32 bit system with 2GB DDR will have a zero gd->ram_top, due to the addition of 2GB (0x80000000) to the base address of kseg0 (also 0x80000000) which overflows & wraps to 0.
We originally had a check for this case, but it was lost in commit 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") causing problems for the affected 32 bit systems.
I think I did a wrong conflict resolution because the patch didn't apply anymore. I folded this patch into "boston: Provide physical CONFIG_SYS_SDRAM_BASE" to fix this. Actually I wanted to resend the updated patches. But if you are okay with the current state in u-boot-mips/next branch, I'll take them as they are.
BTW: could you resend your series "boston: Ethernet support for MIPS Boston board"? I still have no Acks or Reviews on the generic DM parts. Thanks.
When I last fetched from u-boot-mips.git I saw patches up to 564cc3a11c45 ("mips: Remove virt_to_phys call on bi_memstart") in the next branch, which I have then rebased my ethernet patches atop with the result working fine on a real Boston board.
I see that next now contains only 2 patches up to d2a4e3664150 ("mips: bmips: increment SYS_MALLOC_F_LEN") and has dropped the patches switching to a physical CONFIG_SYS_SDRAM_BASE. Would you like me to rebase those plus the Boston ethernet support atop the current next branch?
I had to remove the patches because there is a failing test case in qemu pytest [1] which needs to be fixed. The test case fetches the RAM base address from the "bdinfo" output which is 0x0 due to CONFIG_SYS_SDRAM_BASE = 0. I'm not sure if we need to add a phys_to_virt mapping to the "md" command or if "bdinfo" should show the virtual address. What do you think? Actually other tools like "cp" are also affected.
Also we now need a new patch for CONFIG_SYS_SDRAM_BASE in various "include/configs/bmips_*.h" files.
[1] https://travis-ci.org/danielschwierzeck/u-boot/jobs/330776664
On 22.01.2018 19:54, Daniel Schwierzeck wrote:
On 22.01.2018 19:01, Paul Burton wrote:
Hi Daniel,
On Fri, Jan 19, 2018 at 12:31:25PM +0100, Daniel Schwierzeck wrote:
On 18.01.2018 22:19, Paul Burton wrote:
When constraining the highest DDR address that U-Boot will use for its data & relocated self, we need to handle the common case in which a 32 bit system with 2GB DDR will have a zero gd->ram_top, due to the addition of 2GB (0x80000000) to the base address of kseg0 (also 0x80000000) which overflows & wraps to 0.
We originally had a check for this case, but it was lost in commit 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE") causing problems for the affected 32 bit systems.
I think I did a wrong conflict resolution because the patch didn't apply anymore. I folded this patch into "boston: Provide physical CONFIG_SYS_SDRAM_BASE" to fix this. Actually I wanted to resend the updated patches. But if you are okay with the current state in u-boot-mips/next branch, I'll take them as they are.
BTW: could you resend your series "boston: Ethernet support for MIPS Boston board"? I still have no Acks or Reviews on the generic DM parts. Thanks.
When I last fetched from u-boot-mips.git I saw patches up to 564cc3a11c45 ("mips: Remove virt_to_phys call on bi_memstart") in the next branch, which I have then rebased my ethernet patches atop with the result working fine on a real Boston board.
I see that next now contains only 2 patches up to d2a4e3664150 ("mips: bmips: increment SYS_MALLOC_F_LEN") and has dropped the patches switching to a physical CONFIG_SYS_SDRAM_BASE. Would you like me to rebase those plus the Boston ethernet support atop the current next branch?
I had to remove the patches because there is a failing test case in qemu pytest [1] which needs to be fixed. The test case fetches the RAM base address from the "bdinfo" output which is 0x0 due to CONFIG_SYS_SDRAM_BASE = 0. I'm not sure if we need to add a phys_to_virt mapping to the "md" command or if "bdinfo" should show the virtual address. What do you think? Actually other tools like "cp" are also affected.
I think we need to implement "include/mapmem.h" for MIPS. But this won't work with the current phys_to_virt() implementation because there a lot of places using map_sysmem() and therefore expecting a physical address.
One hack would be:
diff --git a/arch/mips/config.mk b/arch/mips/config.mk index cefdbe65e1..82fcd55f8c 100644 --- a/arch/mips/config.mk +++ b/arch/mips/config.mk @@ -39,6 +39,9 @@ PLATFORM_CPPFLAGS += -D__MIPS__ PLATFORM_ELFENTRY = "__start" PLATFORM_ELFFLAGS += -B mips $(OBJCOPYFLAGS)
+# Force this until converted to a Kconfig symbol +PLATFORM_CPPFLAGS += -DCONFIG_ARCH_MAP_SYSMEM + # # From Linux arch/mips/Makefile # diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h index 45d7ca0cc6..9173beda0b 100644 --- a/arch/mips/include/asm/io.h +++ b/arch/mips/include/asm/io.h @@ -559,6 +559,20 @@ BUILD_CLRSETBITS(q, le64, le64, u64) BUILD_CLRSETBITS(q, be64, be64, u64) BUILD_CLRSETBITS(q, 64, _, u64)
+static inline void *map_sysmem(phys_addr_t paddr, unsigned long len) +{ + return (void *)CKSEG0ADDR(paddr); +} + +static inline void unmap_sysmem(const void *vaddr) +{ +} + +static inline phys_addr_t map_to_sysmem(const void *ptr) +{ + return CKSEG0ADDR((uintptr_t)ptr); +} + #include <asm-generic/io.h>
#endif /* _ASM_IO_H */
Test with qemu_mips:
qemu-mips # bdinfo boot_params = 0x87F488E8 memstart = 0x00000000 memsize = 0x08000000 flashstart = 0xBFC00000 flashsize = 0x00400000 flashoffset = 0x00030508 ethaddr = 52:54:00:12:34:56 IP addr = <NULL> baudrate = 115200 bps relocaddr = 0x87F90000 reloc off = 0xC8390000 qemu-mips # md 0 00000000: 00000000 00000000 00000000 00000000 ................ 00000010: 00000000 00000000 00000000 00000000 ................ 00000020: 00000000 00000000 00000000 00000000 ................
There is another issue. If "struct bd_info" is supposed to hold physical addresses, we would need to fix bi_flashstart and CONFIG_SYS_FLASH_BASE too ;)
participants (2)
-
Daniel Schwierzeck -
Paul Burton