[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[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.

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);
 }
-- 
2.15.1

[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[Daniel Schwierzeck]

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.


-- 
- 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>

[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[Paul Burton]

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

[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[Daniel Schwierzeck]

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

-- 
- 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>

[U-Boot] [PATCH] boston: Ensure DDR address calcuations don't overflow

[Daniel Schwierzeck]

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 ;)

-- 
- 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>