Skip to contentSkip to conversation

boston: Ensure DDR address calcuations don't overflow

PATCH New Paul Burton 2 participants 5 messages Jan 18, 2018 – Jan 22, 2018 MIPS

lore
Share & tools
lore

Paul Burton

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
Fixes 78edb25729ce ("boston: Provide physical CONFIG_SYS_SDRAM_BASE")
Cc Daniel Schwierzeck <[email protected]>

---
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(-)
board/imgtec/boston/ddr.c +10 -1
index 00428496bd..892bb1754c 100644
@@ -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);
 }
Signature · 1 line
2.15.1
#1 lore raw

PATCH 1/1 boston: Ensure DDR address calcuations don't overflow

Quoted 9 lines · Paul Burton

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.

Signature · 9 lines
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180119/47acfe42/attachment.sig>
author
#2 lore raw

PATCH 1/1 boston: Ensure DDR address calcuations don't overflow

Hi Daniel,

Quoted 17 lines · Daniel Schwierzeck
Quoted 9 lines · Paul Burton

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.

Quoted 3 lines

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

#3 lore raw

PATCH 1/1 boston: Ensure DDR address calcuations don't overflow

Quoted 29 lines · Paul Burton

Hi Daniel,

Quoted 17 lines · Daniel Schwierzeck
Quoted 9 lines · Paul Burton

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.

Quoted 3 lines

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.

Quoted 6 lines

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

Signature · 9 lines
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180122/c7b14d96/attachment.sig>
#4 lore raw

PATCH 1/1 boston: Ensure DDR address calcuations don't overflow

Quoted 39 lines · Daniel Schwierzeck
Quoted 29 lines · Paul Burton

Hi Daniel,

Quoted 17 lines · Daniel Schwierzeck
Quoted 9 lines · Paul Burton

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.

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 8 lines

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:

arch/mips/config.mk +3 -0
index cefdbe65e1..82fcd55f8c 100644
@@ -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
 #
arch/mips/include/asm/io.h +14 -0
index 45d7ca0cc6..9173beda0b 100644
@@ -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 ;)

Signature · 9 lines
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180122/6d7e719c/attachment.sig>