[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[Keerthy]

With introduction of commit:

a156c47e39ad: tftp: prevent overwriting reserved memory

tftp with loadaddr is failing for Images of the size bigger than SPL.
The issue is that SPL image is located at bootaddr
and dtb is allocated at bootaddr + spl_size.

The region where dtb is present is marked reserved.

So when the tftp tries to load the Image it tries to find
a location from loadaddr to the first reserved memory region
which is the dtb base address. The max_size obtained is pretty
less and hence tftp fails with the below error:

TFTP error: trying to overwrite reserved memory...

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

I am sure there are better solutions so just wanted to know
if there is one.

 common/fdt_support.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 42583e3ed8..2bb101e56e 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -633,11 +633,6 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
 	/* Change the fdt header to reflect the correct size */
 	fdt_set_totalsize(blob, actualsize);
 
-	/* Add the new reservation */
-	ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
-	if (ret < 0)
-		return ret;
-
 	return actualsize;
 }
 
-- 
2.17.1

[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[Simon Glass]

Hi Keerthy,

On Wed, 30 Jan 2019 at 06:41, Keerthy <j-keerthy@ti.com> wrote:
>
> With introduction of commit:
>
> a156c47e39ad: tftp: prevent overwriting reserved memory
>
> tftp with loadaddr is failing for Images of the size bigger than SPL.
> The issue is that SPL image is located at bootaddr
> and dtb is allocated at bootaddr + spl_size.
>
> The region where dtb is present is marked reserved.
>
> So when the tftp tries to load the Image it tries to find
> a location from loadaddr to the first reserved memory region
> which is the dtb base address. The max_size obtained is pretty
> less and hence tftp fails with the below error:
>
> TFTP error: trying to overwrite reserved memory...

I don't quite get this. Are you saying that we load U-Boot to
loadaddr, overwriting the SPL DT?

Simon Goldschmidt, any thoughts?

Regards,
Simon


>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>
> I am sure there are better solutions so just wanted to know
> if there is one.
>
>  common/fdt_support.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 42583e3ed8..2bb101e56e 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -633,11 +633,6 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>         /* Change the fdt header to reflect the correct size */
>         fdt_set_totalsize(blob, actualsize);
>
> -       /* Add the new reservation */
> -       ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> -       if (ret < 0)
> -               return ret;
> -
>         return actualsize;
>  }
>
> --
> 2.17.1
>

[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[keerthy]

On 4/12/2019 7:39 AM, Simon Glass wrote:
> Hi Keerthy,
> 
> On Wed, 30 Jan 2019 at 06:41, Keerthy <j-keerthy@ti.com> wrote:
>>
>> With introduction of commit:
>>
>> a156c47e39ad: tftp: prevent overwriting reserved memory
>>
>> tftp with loadaddr is failing for Images of the size bigger than SPL.
>> The issue is that SPL image is located at bootaddr
>> and dtb is allocated at bootaddr + spl_size.
>>
>> The region where dtb is present is marked reserved.
>>
>> So when the tftp tries to load the Image it tries to find
>> a location from loadaddr to the first reserved memory region
>> which is the dtb base address. The max_size obtained is pretty
>> less and hence tftp fails with the below error:
>>
>> TFTP error: trying to overwrite reserved memory...
> 
> I don't quite get this. Are you saying that we load U-Boot to
> loadaddr, overwriting the SPL DT?

Simon,

No. Basically when we try to load kernel to bootaddr using tftp there is 
dtb present at bootaddr+spl_size and its marked as reserved. So TFTP 
fails. IMHO post relocation it should not be marked as reserved. 
Basically we will never be able to find enough space between bootaddr 
and dtb address for us to fit kernel Image and tftp will always fail for 
bootaddr.

Hope i am clear now.

Regards,
Keerthy
> 
> Simon Goldschmidt, any thoughts?
> 
> Regards,
> Simon
> 
> 
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>
>> I am sure there are better solutions so just wanted to know
>> if there is one.
>>
>>   common/fdt_support.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index 42583e3ed8..2bb101e56e 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -633,11 +633,6 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>>          /* Change the fdt header to reflect the correct size */
>>          fdt_set_totalsize(blob, actualsize);
>>
>> -       /* Add the new reservation */
>> -       ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
>> -       if (ret < 0)
>> -               return ret;
>> -
>>          return actualsize;
>>   }
>>
>> --
>> 2.17.1
>>

[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[Simon Glass]

Hi Keerthy, Kumar,

On Thu, 11 Apr 2019 at 21:41, keerthy <j-keerthy@ti.com> wrote:
>
>
>
> On 4/12/2019 7:39 AM, Simon Glass wrote:
> > Hi Keerthy,
> >
> > On Wed, 30 Jan 2019 at 06:41, Keerthy <j-keerthy@ti.com> wrote:
> >>
> >> With introduction of commit:
> >>
> >> a156c47e39ad: tftp: prevent overwriting reserved memory
> >>
> >> tftp with loadaddr is failing for Images of the size bigger than SPL.
> >> The issue is that SPL image is located at bootaddr
> >> and dtb is allocated at bootaddr + spl_size.
> >>
> >> The region where dtb is present is marked reserved.
> >>
> >> So when the tftp tries to load the Image it tries to find
> >> a location from loadaddr to the first reserved memory region
> >> which is the dtb base address. The max_size obtained is pretty
> >> less and hence tftp fails with the below error:
> >>
> >> TFTP error: trying to overwrite reserved memory...
> >
> > I don't quite get this. Are you saying that we load U-Boot to
> > loadaddr, overwriting the SPL DT?
>
> Simon,
>
> No. Basically when we try to load kernel to bootaddr using tftp there is
> dtb present at bootaddr+spl_size and its marked as reserved. So TFTP
> fails. IMHO post relocation it should not be marked as reserved.
> Basically we will never be able to find enough space between bootaddr
> and dtb address for us to fit kernel Image and tftp will always fail for
> bootaddr.
>
> Hope i am clear now.
>

It looks like this code was added from PowerPC by Kumar Gala
<galak@kernel.crashing.org>, now copied.

3082d2348c fdt: refactor fdt resize code

I don't really understand why the tftp command is looking at reserved
memory in the DT. Is this due to the recent improvements to enforce
reserved memory that Simon Goldschmidt did?

I'm sorry but I cannot really help with reviewing this. Perhaps Tom knows more.

Regards,
Simon

> Regards,
> Keerthy
> >
> > Simon Goldschmidt, any thoughts?
> >
> > Regards,
> > Simon
> >
> >
> >>
> >> Signed-off-by: Keerthy <j-keerthy@ti.com>
> >> ---
> >>
> >> I am sure there are better solutions so just wanted to know
> >> if there is one.
> >>
> >>   common/fdt_support.c | 5 -----
> >>   1 file changed, 5 deletions(-)
> >>
> >> diff --git a/common/fdt_support.c b/common/fdt_support.c
> >> index 42583e3ed8..2bb101e56e 100644
> >> --- a/common/fdt_support.c
> >> +++ b/common/fdt_support.c
> >> @@ -633,11 +633,6 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> >>          /* Change the fdt header to reflect the correct size */
> >>          fdt_set_totalsize(blob, actualsize);
> >>
> >> -       /* Add the new reservation */
> >> -       ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> >> -       if (ret < 0)
> >> -               return ret;
> >> -
> >>          return actualsize;
> >>   }
> >>
> >> --
> >> 2.17.1
> >>

[U-Boot] [RFC PATCH] common: fdt_support: Remove fdt_add_mem_rsv in fdt_shrink_to_minimum

[Simon Goldschmidt]

On Fri, Apr 12, 2019 at 2:25 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Keerthy, Kumar,
>
> On Thu, 11 Apr 2019 at 21:41, keerthy <j-keerthy@ti.com> wrote:
> >
> >
> >
> > On 4/12/2019 7:39 AM, Simon Glass wrote:
> > > Hi Keerthy,
> > >
> > > On Wed, 30 Jan 2019 at 06:41, Keerthy <j-keerthy@ti.com> wrote:
> > >>
> > >> With introduction of commit:
> > >>
> > >> a156c47e39ad: tftp: prevent overwriting reserved memory
> > >>
> > >> tftp with loadaddr is failing for Images of the size bigger than SPL.
> > >> The issue is that SPL image is located at bootaddr
> > >> and dtb is allocated at bootaddr + spl_size.
> > >>
> > >> The region where dtb is present is marked reserved.
> > >>
> > >> So when the tftp tries to load the Image it tries to find
> > >> a location from loadaddr to the first reserved memory region
> > >> which is the dtb base address. The max_size obtained is pretty
> > >> less and hence tftp fails with the below error:
> > >>
> > >> TFTP error: trying to overwrite reserved memory...
> > >
> > > I don't quite get this. Are you saying that we load U-Boot to
> > > loadaddr, overwriting the SPL DT?
> >
> > Simon,
> >
> > No. Basically when we try to load kernel to bootaddr using tftp there is
> > dtb present at bootaddr+spl_size and its marked as reserved. So TFTP
> > fails. IMHO post relocation it should not be marked as reserved.
> > Basically we will never be able to find enough space between bootaddr
> > and dtb address for us to fit kernel Image and tftp will always fail for
> > bootaddr.
> >
> > Hope i am clear now.
> >
>
> It looks like this code was added from PowerPC by Kumar Gala
> <galak@kernel.crashing.org>, now copied.
>
> 3082d2348c fdt: refactor fdt resize code
>
> I don't really understand why the tftp command is looking at reserved
> memory in the DT. Is this due to the recent improvements to enforce
> reserved memory that Simon Goldschmidt did?

The tftp command looks at reserved memory in the DT because in
many situations, we have to prevent writing to that memory to prevent
security issues where reserved memory gets overwritten before we have
checked the signature of a loaded binary.

That being said, I don't really understand the boot flow and what's going
wrong, could someone please explain this to me further?

Why is the region of the SPL dtb marked as reserved?

Regards,
Simon

>
> I'm sorry but I cannot really help with reviewing this. Perhaps Tom knows more.
>
> Regards,
> Simon
>
> > Regards,
> > Keerthy
> > >
> > > Simon Goldschmidt, any thoughts?
> > >
> > > Regards,
> > > Simon
> > >
> > >
> > >>
> > >> Signed-off-by: Keerthy <j-keerthy@ti.com>
> > >> ---
> > >>
> > >> I am sure there are better solutions so just wanted to know
> > >> if there is one.
> > >>
> > >>   common/fdt_support.c | 5 -----
> > >>   1 file changed, 5 deletions(-)
> > >>
> > >> diff --git a/common/fdt_support.c b/common/fdt_support.c
> > >> index 42583e3ed8..2bb101e56e 100644
> > >> --- a/common/fdt_support.c
> > >> +++ b/common/fdt_support.c
> > >> @@ -633,11 +633,6 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
> > >>          /* Change the fdt header to reflect the correct size */
> > >>          fdt_set_totalsize(blob, actualsize);
> > >>
> > >> -       /* Add the new reservation */
> > >> -       ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> > >> -       if (ret < 0)
> > >> -               return ret;
> > >> -
> > >>          return actualsize;
> > >>   }
> > >>
> > >> --
> > >> 2.17.1
> > >>