[U-Boot,RESEND,v2,1/6] image: Use ram_top, not bi_memsize, in getenv_bootm_size

From: Paul Burton <paul.burton@imgtec.com>

When determining the region of memory to allow for use by bootm, using
bi_memstart & adding bi_memsize can cause problems if that leads to an
integer overflow. For example on some MIPS systems bi_memstart would be
0xffffffff80000000 (ie. the start of the MIPS ckseg0 region) and if the
system has 2GB of memory then the addition would wrap around to 0.

The maximum amount of memory to be used by U-Boot is already accounted
for by the ram_top field of struct global_data, so make use of that for
the calculation instead.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

---

Changes in v2:
- also patch ARM specific code to get a consistent behaviour

 common/image.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Hi Daniel,

On 30 April 2017 at 13:22, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
> From: Paul Burton <paul.burton@imgtec.com>
>
> When determining the region of memory to allow for use by bootm, using
> bi_memstart & adding bi_memsize can cause problems if that leads to an
> integer overflow. For example on some MIPS systems bi_memstart would be
> 0xffffffff80000000 (ie. the start of the MIPS ckseg0 region) and if the
> system has 2GB of memory then the addition would wrap around to 0.
>
> The maximum amount of memory to be used by U-Boot is already accounted
> for by the ram_top field of struct global_data, so make use of that for
> the calculation instead.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>
> ---
>
> Changes in v2:
> - also patch ARM specific code to get a consistent behaviour
>
>  common/image.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/common/image.c b/common/image.c
> index 0f88984f2d..160c4c6fbc 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -494,11 +494,10 @@ phys_size_t getenv_bootm_size(void)
>
>  #if defined(CONFIG_ARM) && defined(CONFIG_NR_DRAM_BANKS)
>         start = gd->bd->bi_dram[0].start;
> -       size = gd->bd->bi_dram[0].size;
>  #else
>         start = gd->bd->bi_memstart;
> -       size = gd->bd->bi_memsize;
>  #endif
> +       size = gd->ram_top - start;
>

I'm a little nervous about this change. For ARM we might have
discontinuous DRAM segments. Can you not set bd->bi_memsize insead?

>         s = getenv("bootm_low");
>         if (s)
> --
> 2.11.0
>

Regards,
Simon

Am 04.05.2017 um 18:49 schrieb Simon Glass:
> Hi Daniel,
> 
> On 30 April 2017 at 13:22, Daniel Schwierzeck
> <daniel.schwierzeck@gmail.com> wrote:
>> From: Paul Burton <paul.burton@imgtec.com>
>>
>> When determining the region of memory to allow for use by bootm, using
>> bi_memstart & adding bi_memsize can cause problems if that leads to an
>> integer overflow. For example on some MIPS systems bi_memstart would be
>> 0xffffffff80000000 (ie. the start of the MIPS ckseg0 region) and if the
>> system has 2GB of memory then the addition would wrap around to 0.
>>
>> The maximum amount of memory to be used by U-Boot is already accounted
>> for by the ram_top field of struct global_data, so make use of that for
>> the calculation instead.
>>
>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>>
>> ---
>>
>> Changes in v2:
>> - also patch ARM specific code to get a consistent behaviour
>>
>>  common/image.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/common/image.c b/common/image.c
>> index 0f88984f2d..160c4c6fbc 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -494,11 +494,10 @@ phys_size_t getenv_bootm_size(void)
>>
>>  #if defined(CONFIG_ARM) && defined(CONFIG_NR_DRAM_BANKS)
>>         start = gd->bd->bi_dram[0].start;
>> -       size = gd->bd->bi_dram[0].size;
>>  #else
>>         start = gd->bd->bi_memstart;
>> -       size = gd->bd->bi_memsize;
>>  #endif
>> +       size = gd->ram_top - start;
>>
> 
> I'm a little nervous about this change. For ARM we might have
> discontinuous DRAM segments. Can you not set bd->bi_memsize insead?
> 
>>         s = getenv("bootm_low");
>>         if (s)

after reviewing again I think this patch is wrong. Maybe we have to fix
the MIPS specific problem instead, where CONFIG_SYS_SDRAM_BASE is used
as a virtual address instead of a physical one. Actually this causes the
wrap around. Thus I withdraw this patch.