[PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[Marek Vasut]

The current implementation of boot_relocate_fdt() places DT at the
highest usable DRAM address, which is calculated as:
  env_get_bootm_low() + env_get_bootm_mapsize()
which by default becomes gd->ram_base + gd->ram_size.

Systems like i.MX53 can have multiple DRAM banks with gap between them,
e.g. have DRAM at 0x70000000-0x8fffffff and 0xb0000000-0xcfffffff , so
for them the calculated highest DRAM address is 0xafffffff, which is
exactly in the gap and thus not usable.

Fix this by iterating over all DRAM banks and tracking the remaining
amount of the total mapping size obtained from env_get_bootm_mapsize().
Limit the maximum LMB area size to each bank, to avoid using nonexistent
DRAM.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 common/image-fdt.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index 327a8c4c395..2b199ffc2b1 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -157,8 +157,11 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size)
 {
 	void	*fdt_blob = *of_flat_tree;
 	void	*of_start = NULL;
+	u64	start, size, usable;
 	char	*fdt_high;
+	ulong	mapsize, low;
 	ulong	of_len = 0;
+	int	bank;
 	int	err;
 	int	disable_relocation = 0;
 
@@ -198,10 +201,39 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size)
 			    (void *)(ulong) lmb_alloc(lmb, of_len, 0x1000);
 		}
 	} else {
-		of_start =
-		    (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
-						   env_get_bootm_mapsize()
-						   + env_get_bootm_low());
+		mapsize = env_get_bootm_mapsize();
+		low = env_get_bootm_low();
+		of_start = NULL;
+
+		for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
+			start = gd->bd->bi_dram[bank].start;
+			size = gd->bd->bi_dram[bank].size;
+
+			/* DRAM bank addresses are too low, skip it. */
+			if (start + size < low)
+				continue;
+
+			usable = min(size, (u64)mapsize);
+
+			/*
+			 * At least part of this DRAM bank is usable, try
+			 * using it for LMB allocation.
+			 */
+			of_start =
+			    (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
+							   start + usable);
+			/* Allocation succeeded, use this block. */
+			if (of_start != NULL)
+				break;
+
+			/*
+			 * Reduce the mapping size in the next bank
+			 * by the size of attempt in current bank.
+			 */
+			mapsize -= usable - max(start, (u64)low);
+			if (!mapsize)
+				break;
+		}
 	}
 
 	if (of_start == NULL) {
-- 
2.30.2

Re: [PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[Simon Glass]

Hi Marek,

On Wed, 21 Jul 2021 at 18:05, Marek Vasut <marex@denx.de> wrote:
>
> The current implementation of boot_relocate_fdt() places DT at the
> highest usable DRAM address, which is calculated as:
>   env_get_bootm_low() + env_get_bootm_mapsize()
> which by default becomes gd->ram_base + gd->ram_size.
>
> Systems like i.MX53 can have multiple DRAM banks with gap between them,
> e.g. have DRAM at 0x70000000-0x8fffffff and 0xb0000000-0xcfffffff , so
> for them the calculated highest DRAM address is 0xafffffff, which is
> exactly in the gap and thus not usable.
>
> Fix this by iterating over all DRAM banks and tracking the remaining
> amount of the total mapping size obtained from env_get_bootm_mapsize().
> Limit the maximum LMB area size to each bank, to avoid using nonexistent
> DRAM.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  common/image-fdt.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Should we put this behind a Kconfig option to reduce code size?

>
> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index 327a8c4c395..2b199ffc2b1 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -157,8 +157,11 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size)
>  {
>         void    *fdt_blob = *of_flat_tree;
>         void    *of_start = NULL;
> +       u64     start, size, usable;
>         char    *fdt_high;
> +       ulong   mapsize, low;
>         ulong   of_len = 0;
> +       int     bank;
>         int     err;
>         int     disable_relocation = 0;
>
> @@ -198,10 +201,39 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size)
>                             (void *)(ulong) lmb_alloc(lmb, of_len, 0x1000);
>                 }
>         } else {
> -               of_start =
> -                   (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
> -                                                  env_get_bootm_mapsize()
> -                                                  + env_get_bootm_low());
> +               mapsize = env_get_bootm_mapsize();
> +               low = env_get_bootm_low();
> +               of_start = NULL;
> +
> +               for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> +                       start = gd->bd->bi_dram[bank].start;
> +                       size = gd->bd->bi_dram[bank].size;
> +
> +                       /* DRAM bank addresses are too low, skip it. */
> +                       if (start + size < low)
> +                               continue;
> +
> +                       usable = min(size, (u64)mapsize);
> +
> +                       /*
> +                        * At least part of this DRAM bank is usable, try
> +                        * using it for LMB allocation.
> +                        */
> +                       of_start =
> +                           (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
> +                                                          start + usable);
> +                       /* Allocation succeeded, use this block. */
> +                       if (of_start != NULL)
> +                               break;
> +
> +                       /*
> +                        * Reduce the mapping size in the next bank
> +                        * by the size of attempt in current bank.
> +                        */
> +                       mapsize -= usable - max(start, (u64)low);
> +                       if (!mapsize)
> +                               break;
> +               }
>         }
>
>         if (of_start == NULL) {
> --
> 2.30.2
>

Regards,
Simon

Re: [PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[Marek Vasut]

On 3/12/22 03:24, Simon Glass wrote:
> Hi Marek,
> 
> On Wed, 21 Jul 2021 at 18:05, Marek Vasut <marex@denx.de> wrote:
>>
>> The current implementation of boot_relocate_fdt() places DT at the
>> highest usable DRAM address, which is calculated as:
>>    env_get_bootm_low() + env_get_bootm_mapsize()
>> which by default becomes gd->ram_base + gd->ram_size.
>>
>> Systems like i.MX53 can have multiple DRAM banks with gap between them,
>> e.g. have DRAM at 0x70000000-0x8fffffff and 0xb0000000-0xcfffffff , so
>> for them the calculated highest DRAM address is 0xafffffff, which is
>> exactly in the gap and thus not usable.
>>
>> Fix this by iterating over all DRAM banks and tracking the remaining
>> amount of the total mapping size obtained from env_get_bootm_mapsize().
>> Limit the maximum LMB area size to each bank, to avoid using nonexistent
>> DRAM.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>   common/image-fdt.c | 40 ++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 36 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Should we put this behind a Kconfig option to reduce code size?

Since this depends on DT content, we cannot predict what kind of DT will 
be passed to U-Boot, so no, we cannot put this behind a Kconfig option.

Re: [PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[Simon Glass]

Hi Marek,

On Fri, 11 Mar 2022 at 19:41, Marek Vasut <marex@denx.de> wrote:
>
> On 3/12/22 03:24, Simon Glass wrote:
> > Hi Marek,
> >
> > On Wed, 21 Jul 2021 at 18:05, Marek Vasut <marex@denx.de> wrote:
> >>
> >> The current implementation of boot_relocate_fdt() places DT at the
> >> highest usable DRAM address, which is calculated as:
> >>    env_get_bootm_low() + env_get_bootm_mapsize()
> >> which by default becomes gd->ram_base + gd->ram_size.
> >>
> >> Systems like i.MX53 can have multiple DRAM banks with gap between them,
> >> e.g. have DRAM at 0x70000000-0x8fffffff and 0xb0000000-0xcfffffff , so
> >> for them the calculated highest DRAM address is 0xafffffff, which is
> >> exactly in the gap and thus not usable.
> >>
> >> Fix this by iterating over all DRAM banks and tracking the remaining
> >> amount of the total mapping size obtained from env_get_bootm_mapsize().
> >> Limit the maximum LMB area size to each bank, to avoid using nonexistent
> >> DRAM.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >>   common/image-fdt.c | 40 ++++++++++++++++++++++++++++++++++++----
> >>   1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Should we put this behind a Kconfig option to reduce code size?
>
> Since this depends on DT content, we cannot predict what kind of DT will
> be passed to U-Boot, so no, we cannot put this behind a Kconfig option.

Doesn't it only affect boards with disjoint memory?

Regards,
Simon

Re: [PATCH][RFC] image: fdt: Fix DT relocation handling with multiple DRAM banks with gap

[Marek Vasut]

On 3/12/22 06:02, Simon Glass wrote:
> Hi Marek,
> 
> On Fri, 11 Mar 2022 at 19:41, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/12/22 03:24, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On Wed, 21 Jul 2021 at 18:05, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The current implementation of boot_relocate_fdt() places DT at the
>>>> highest usable DRAM address, which is calculated as:
>>>>     env_get_bootm_low() + env_get_bootm_mapsize()
>>>> which by default becomes gd->ram_base + gd->ram_size.
>>>>
>>>> Systems like i.MX53 can have multiple DRAM banks with gap between them,
>>>> e.g. have DRAM at 0x70000000-0x8fffffff and 0xb0000000-0xcfffffff , so
>>>> for them the calculated highest DRAM address is 0xafffffff, which is
>>>> exactly in the gap and thus not usable.
>>>>
>>>> Fix this by iterating over all DRAM banks and tracking the remaining
>>>> amount of the total mapping size obtained from env_get_bootm_mapsize().
>>>> Limit the maximum LMB area size to each bank, to avoid using nonexistent
>>>> DRAM.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>>    common/image-fdt.c | 40 ++++++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 36 insertions(+), 4 deletions(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> Should we put this behind a Kconfig option to reduce code size?
>>
>> Since this depends on DT content, we cannot predict what kind of DT will
>> be passed to U-Boot, so no, we cannot put this behind a Kconfig option.
> 
> Doesn't it only affect boards with disjoint memory?

Sure, it does trigger nasty unexpected failures on some systems. And you 
cannot really tell whether a board may or may not be populated with such 
a memory setup, because you cannot predict what will be in the DT.

Besides, there is far more code which correctly does handle multiple 
memory areas and it is also not ifdef'd out, so I don't see why we 
should special case this one. That would only lead to inconsistency and 
more people running into this kind of problem and wasting a lot of time 
trying to figure out a fix, and then arriving at some loadaddr 
workaround instead.