[PATCH] env: Leave invalid env for nowhere location

[PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
to gd->env_valid, and sets default_environment before relocation to
gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
by the previous fix.

If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
default_environment, however, env_get_char() returns gd->env_addr before
relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
will cause a fault.

This leaves gd->env_valid as ENV_INVALID for "nowhere" location.

Cc: Marek Vasut <marex@denx.de>
Fixes: 5557eec01cbf ("env: Fix invalid env handling in env_init()")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 env/env.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index e534008..3233172 100644
--- a/env/env.c
+++ b/env/env.c
@@ -336,7 +336,8 @@ int env_init(void)
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
 
-		if (gd->env_valid == ENV_INVALID)
+		if (gd->env_valid == ENV_INVALID
+		    && drv->location != ENVL_NOWHERE)
 			ret = -ENOENT;
 	}
 
-- 
2.7.4

[PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
> to gd->env_valid, and sets default_environment before relocation to
> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
> by the previous fix.
> 
> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
> default_environment, however, env_get_char() returns gd->env_addr before
> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
> will cause a fault.
> 
> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
> 
> Cc: Marek Vasut <marex@denx.de>
> Fixes: 5557eec01cbf ("env: Fix invalid env handling in env_init()")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>   env/env.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index e534008..3233172 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -336,7 +336,8 @@ int env_init(void)
>   		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>   		      drv->name, ret);
>   
> -		if (gd->env_valid == ENV_INVALID)
> +		if (gd->env_valid == ENV_INVALID
> +		    && drv->location != ENVL_NOWHERE)
>   			ret = -ENOENT;
>   	}

I'm CCing Tim, it would be good to get a TB from him.

Re: [PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Tim,

How about this fix?

You already tested Marek's patch, and I'd like to hear your comment
about this patch, or know whether it occurs the issue with
CONFIG_ENV_IS_NOWHERE if possible.

Thank you,

On 2021/05/17 2:19, Marek Vasut wrote:
> On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
>> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
>> to gd->env_valid, and sets default_environment before relocation to
>> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
>> by the previous fix.
>>
>> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
>> default_environment, however, env_get_char() returns gd->env_addr before
>> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
>> will cause a fault.
>>
>> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
>>
>> Cc: Marek Vasut <marex@denx.de>
>> Fixes: 5557eec01cbf ("env: Fix invalid env handling in env_init()")
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>    env/env.c | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index e534008..3233172 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -336,7 +336,8 @@ int env_init(void)
>>    		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>    		      drv->name, ret);
>>    
>> -		if (gd->env_valid == ENV_INVALID)
>> +		if (gd->env_valid == ENV_INVALID
>> +		    && drv->location != ENVL_NOWHERE)
>>    			ret = -ENOENT;
>>    	}
> 
> I'm CCing Tim, it would be good to get a TB from him.
> 

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
> to gd->env_valid, and sets default_environment before relocation to
> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
> by the previous fix.
> 
> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
> default_environment, however, env_get_char() returns gd->env_addr before
> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
> will cause a fault.
> 
> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.

So do I understand this correctly that _after_ relocation, env_init() is 
called and env_init() does not update gd->env_addr to the relocated one?

I would expect that after relocation, if all you have is env_nowhere 
driver, the env_nowhere_init() is called again from the first for() loop 
of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and 
that for() loop would exit with ret = -ENOENT [2], so then the last part 
of env_init() would check for ret == -ENOENT and update gd->env_addr to 
relocated default_environment [3].

324  int env_init(void)
325  {
326    struct env_driver *drv;
327    int ret = -ENOENT;
328    int prio;
329
330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
          /* Part [1] */
331      if (!drv->init || !(ret = drv->init()))
332        env_set_inited(drv->location);
333      if (ret == -ENOENT)
334        env_set_inited(drv->location);
335
336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
337            drv->name, ret);
338
          /* Part [2] */
339      if (gd->env_valid == ENV_INVALID)
340        ret = -ENOENT;
341    }
342
343    if (!prio)
344      return -ENODEV;
345
        /* Part [3] */
346    if (ret == -ENOENT) {
          /* This should be relocated default_environment address */
347      gd->env_addr = (ulong)&default_environment[0];
348      gd->env_valid = ENV_VALID;
349
350      return 0;
351    }
352
353    return ret;
354  }

Or am I missing something obvious ?

> diff --git a/env/env.c b/env/env.c
> index e534008..3233172 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -336,7 +336,8 @@ int env_init(void)
>   		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>   		      drv->name, ret);
>   
> -		if (gd->env_valid == ENV_INVALID)
> +		if (gd->env_valid == ENV_INVALID
> +		    && drv->location != ENVL_NOWHERE)
>   			ret = -ENOENT;
>   	}

Re: [PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Marek,
Sorry for rate reply.

On 2021/05/25 16:35, Marek Vasut wrote:
> On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
>> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
>> to gd->env_valid, and sets default_environment before relocation to
>> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
>> by the previous fix.
>>
>> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
>> default_environment, however, env_get_char() returns gd->env_addr before
>> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
>> will cause a fault.
>>
>> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
> 
> So do I understand this correctly that _after_ relocation, env_init() is
> called and env_init() does not update gd->env_addr to the relocated one?

In my understandings, env_init() belongs to init_sequence_f[]
and env_init() is called before relocation.

> I would expect that after relocation, if all you have is env_nowhere
> driver, the env_nowhere_init() is called again from the first for() loop
> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
> that for() loop would exit with ret = -ENOENT [2], so then the last part
> of env_init() would check for ret == -ENOENT and update gd->env_addr to
> relocated default_environment [3].
> 
> 324  int env_init(void)
> 325  {
> 326    struct env_driver *drv;
> 327    int ret = -ENOENT;
> 328    int prio;
> 329
> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>            /* Part [1] */
> 331      if (!drv->init || !(ret = drv->init()))
> 332        env_set_inited(drv->location);
> 333      if (ret == -ENOENT)
> 334        env_set_inited(drv->location);
> 335
> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
> 337            drv->name, ret);
> 338
>            /* Part [2] */
> 339      if (gd->env_valid == ENV_INVALID)
> 340        ret = -ENOENT;
> 341    }
> 342
> 343    if (!prio)
> 344      return -ENODEV;
> 345
>          /* Part [3] */
> 346    if (ret == -ENOENT) {
>            /* This should be relocated default_environment address */
> 347      gd->env_addr = (ulong)&default_environment[0];
> 348      gd->env_valid = ENV_VALID;
> 349
> 350      return 0;
> 351    }
> 352
> 353    return ret;
> 354  }
> 
> Or am I missing something obvious ?

These are called before relocation, and update gd->env_addr to non-relocated
default_environment by [3].

After that, gd->env_addr is relocated in initr_reloc_global_data()
if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.

| #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
| 	/*
| 	 * Relocate the early env_addr pointer unless we know it is not inside
| 	 * the binary. Some systems need this and for the rest, it doesn't hurt.
| 	 */
| 	gd->env_addr += gd->reloc_off;
| #endif


However, I misunderstood my situation.
gd->reloc_off doesn't have the proper value because CONFIG_SYS_TEXT_BASE
is zero due to CONFIG_POSITION_INDENENDENT=y.

gd->reloc_off is calculated with CONFIG_SYS_TEXT_BASE in setup_reloc().

| 	gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;

gd->env_addr is added with gd->reloc_off (== gd->relocaddr - 0),
as a result, gd->env_addr has wrong address.

In this case, I think the proper solution is to undefine
CONFIG_SYS_RELOC_GD_ENV_ADDR.

My patch isn't necessary no longer and your patch also works with
"nowhere".

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 6/3/21 6:15 PM, Kunihiko Hayashi wrote:
> Hi Marek,

Hi,

> Sorry for rate reply.

No worries, same here.

> On 2021/05/25 16:35, Marek Vasut wrote:
>> On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
>>> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets 
>>> ENV_INVALID
>>> to gd->env_valid, and sets default_environment before relocation to
>>> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
>>> by the previous fix.
>>>
>>> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
>>> default_environment, however, env_get_char() returns gd->env_addr before
>>> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
>>> will cause a fault.
>>>
>>> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
>>
>> So do I understand this correctly that _after_ relocation, env_init() is
>> called and env_init() does not update gd->env_addr to the relocated one?
> 
> In my understandings, env_init() belongs to init_sequence_f[]
> and env_init() is called before relocation.

You're right.

So the env update after relocation should then be done in env_relocate().

>> I would expect that after relocation, if all you have is env_nowhere
>> driver, the env_nowhere_init() is called again from the first for() loop
>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
>> that for() loop would exit with ret = -ENOENT [2], so then the last part
>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>> relocated default_environment [3].
>>
>> 324  int env_init(void)
>> 325  {
>> 326    struct env_driver *drv;
>> 327    int ret = -ENOENT;
>> 328    int prio;
>> 329
>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); 
>> prio++) {
>>            /* Part [1] */
>> 331      if (!drv->init || !(ret = drv->init()))
>> 332        env_set_inited(drv->location);
>> 333      if (ret == -ENOENT)
>> 334        env_set_inited(drv->location);
>> 335
>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>> 337            drv->name, ret);
>> 338
>>            /* Part [2] */
>> 339      if (gd->env_valid == ENV_INVALID)
>> 340        ret = -ENOENT;
>> 341    }
>> 342
>> 343    if (!prio)
>> 344      return -ENODEV;
>> 345
>>          /* Part [3] */
>> 346    if (ret == -ENOENT) {
>>            /* This should be relocated default_environment address */
>> 347      gd->env_addr = (ulong)&default_environment[0];
>> 348      gd->env_valid = ENV_VALID;
>> 349
>> 350      return 0;
>> 351    }
>> 352
>> 353    return ret;
>> 354  }
>>
>> Or am I missing something obvious ?
> 
> These are called before relocation, and update gd->env_addr to 
> non-relocated
> default_environment by [3].
> 
> After that, gd->env_addr is relocated in initr_reloc_global_data()
> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
> 
> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
> |     /*
> |      * Relocate the early env_addr pointer unless we know it is not 
> inside
> |      * the binary. Some systems need this and for the rest, it doesn't 
> hurt.
> |      */
> |     gd->env_addr += gd->reloc_off;
> | #endif
> 

Shouldn't the post-relocation env update happen in env_relocate() ?

> However, I misunderstood my situation.
> gd->reloc_off doesn't have the proper value because CONFIG_SYS_TEXT_BASE
> is zero due to CONFIG_POSITION_INDENENDENT=y.
> 
> gd->reloc_off is calculated with CONFIG_SYS_TEXT_BASE in setup_reloc().
> 
> |     gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
> 
> gd->env_addr is added with gd->reloc_off (== gd->relocaddr - 0),
> as a result, gd->env_addr has wrong address.
> 
> In this case, I think the proper solution is to undefine
> CONFIG_SYS_RELOC_GD_ENV_ADDR.
> 
> My patch isn't necessary no longer and your patch also works with
> "nowhere".
OK

Re: [PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Marek,

On 2021/06/07 3:08, Marek Vasut wrote:
> On 6/3/21 6:15 PM, Kunihiko Hayashi wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Sorry for rate reply.
> 
> No worries, same here.
> 
>> On 2021/05/25 16:35, Marek Vasut wrote:
>>> On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
>>>> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
>>>> to gd->env_valid, and sets default_environment before relocation to
>>>> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
>>>> by the previous fix.
>>>>
>>>> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
>>>> default_environment, however, env_get_char() returns gd->env_addr before
>>>> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
>>>> will cause a fault.
>>>>
>>>> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
>>>
>>> So do I understand this correctly that _after_ relocation, env_init() is
>>> called and env_init() does not update gd->env_addr to the relocated one?
>>
>> In my understandings, env_init() belongs to init_sequence_f[]
>> and env_init() is called before relocation.
> 
> You're right.
> 
> So the env update after relocation should then be done in env_relocate().

Yes. I understand that the relocated gd->env_addr is used in env_relocate().

>>> I would expect that after relocation, if all you have is env_nowhere
>>> driver, the env_nowhere_init() is called again from the first for() loop
>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
>>> that for() loop would exit with ret = -ENOENT [2], so then the last part
>>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>>> relocated default_environment [3].
>>>
>>> 324  int env_init(void)
>>> 325  {
>>> 326    struct env_driver *drv;
>>> 327    int ret = -ENOENT;
>>> 328    int prio;
>>> 329
>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>>>            /* Part [1] */
>>> 331      if (!drv->init || !(ret = drv->init()))
>>> 332        env_set_inited(drv->location);
>>> 333      if (ret == -ENOENT)
>>> 334        env_set_inited(drv->location);
>>> 335
>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>> 337            drv->name, ret);
>>> 338
>>>            /* Part [2] */
>>> 339      if (gd->env_valid == ENV_INVALID)
>>> 340        ret = -ENOENT;
>>> 341    }
>>> 342
>>> 343    if (!prio)
>>> 344      return -ENODEV;
>>> 345
>>>          /* Part [3] */
>>> 346    if (ret == -ENOENT) {
>>>            /* This should be relocated default_environment address */
>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>> 348      gd->env_valid = ENV_VALID;
>>> 349
>>> 350      return 0;
>>> 351    }
>>> 352
>>> 353    return ret;
>>> 354  }
>>>
>>> Or am I missing something obvious ?
>>
>> These are called before relocation, and update gd->env_addr to non-relocated
>> default_environment by [3].
>>
>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>
>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>> |     /*
>> |      * Relocate the early env_addr pointer unless we know it is not inside
>> |      * the binary. Some systems need this and for the rest, it doesn't hurt.
>> |      */
>> |     gd->env_addr += gd->reloc_off;
>> | #endif
>>
> 
> Shouldn't the post-relocation env update happen in env_relocate() ?

Usually env_relocate() calls env_load() that uses relocated gd->env_addr.
It's no problem.

If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.

 >
>> However, I misunderstood my situation.
>> gd->reloc_off doesn't have the proper value because CONFIG_SYS_TEXT_BASE
>> is zero due to CONFIG_POSITION_INDENENDENT=y.
>>
>> gd->reloc_off is calculated with CONFIG_SYS_TEXT_BASE in setup_reloc().
>>
>> |     gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
>>
>> gd->env_addr is added with gd->reloc_off (== gd->relocaddr - 0),
>> as a result, gd->env_addr has wrong address.
>>
>> In this case, I think the proper solution is to undefine
>> CONFIG_SYS_RELOC_GD_ENV_ADDR.
>>
>> My patch isn't necessary no longer and your patch also works with
>> "nowhere".
> OK

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 6/7/21 9:54 AM, Kunihiko Hayashi wrote:

Hi,

[...]

>>>> I would expect that after relocation, if all you have is env_nowhere
>>>> driver, the env_nowhere_init() is called again from the first for() 
>>>> loop
>>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], 
>>>> and
>>>> that for() loop would exit with ret = -ENOENT [2], so then the last 
>>>> part
>>>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>>>> relocated default_environment [3].
>>>>
>>>> 324  int env_init(void)
>>>> 325  {
>>>> 326    struct env_driver *drv;
>>>> 327    int ret = -ENOENT;
>>>> 328    int prio;
>>>> 329
>>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); 
>>>> prio++) {
>>>>            /* Part [1] */
>>>> 331      if (!drv->init || !(ret = drv->init()))
>>>> 332        env_set_inited(drv->location);
>>>> 333      if (ret == -ENOENT)
>>>> 334        env_set_inited(drv->location);
>>>> 335
>>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>> 337            drv->name, ret);
>>>> 338
>>>>            /* Part [2] */
>>>> 339      if (gd->env_valid == ENV_INVALID)
>>>> 340        ret = -ENOENT;
>>>> 341    }
>>>> 342
>>>> 343    if (!prio)
>>>> 344      return -ENODEV;
>>>> 345
>>>>          /* Part [3] */
>>>> 346    if (ret == -ENOENT) {
>>>>            /* This should be relocated default_environment address */
>>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>>> 348      gd->env_valid = ENV_VALID;
>>>> 349
>>>> 350      return 0;
>>>> 351    }
>>>> 352
>>>> 353    return ret;
>>>> 354  }
>>>>
>>>> Or am I missing something obvious ?
>>>
>>> These are called before relocation, and update gd->env_addr to 
>>> non-relocated
>>> default_environment by [3].
>>>
>>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>>
>>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>>> |     /*
>>> |      * Relocate the early env_addr pointer unless we know it is not 
>>> inside
>>> |      * the binary. Some systems need this and for the rest, it 
>>> doesn't hurt.
>>> |      */
>>> |     gd->env_addr += gd->reloc_off;
>>> | #endif
>>>
>>
>> Shouldn't the post-relocation env update happen in env_relocate() ?
> 
> Usually env_relocate() calls env_load() that uses relocated gd->env_addr.
> It's no problem.
> 
> If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
> CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.

But then, if CONFIG_SYS_TEXT_BASE is zero, the env shouldn't be 
relocated or how should it behave ?

Re: [PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Marek,

On 2021/06/08 2:33, Marek Vasut wrote:
> On 6/7/21 9:54 AM, Kunihiko Hayashi wrote:
> 
> Hi,
> 
> [...]
> 
>>>>> I would expect that after relocation, if all you have is env_nowhere
>>>>> driver, the env_nowhere_init() is called again from the first for() loop
>>>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
>>>>> that for() loop would exit with ret = -ENOENT [2], so then the last part
>>>>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>>>>> relocated default_environment [3].
>>>>>
>>>>> 324  int env_init(void)
>>>>> 325  {
>>>>> 326    struct env_driver *drv;
>>>>> 327    int ret = -ENOENT;
>>>>> 328    int prio;
>>>>> 329
>>>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>>>>>            /* Part [1] */
>>>>> 331      if (!drv->init || !(ret = drv->init()))
>>>>> 332        env_set_inited(drv->location);
>>>>> 333      if (ret == -ENOENT)
>>>>> 334        env_set_inited(drv->location);
>>>>> 335
>>>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>> 337            drv->name, ret);
>>>>> 338
>>>>>            /* Part [2] */
>>>>> 339      if (gd->env_valid == ENV_INVALID)
>>>>> 340        ret = -ENOENT;
>>>>> 341    }
>>>>> 342
>>>>> 343    if (!prio)
>>>>> 344      return -ENODEV;
>>>>> 345
>>>>>          /* Part [3] */
>>>>> 346    if (ret == -ENOENT) {
>>>>>            /* This should be relocated default_environment address */
>>>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>>>> 348      gd->env_valid = ENV_VALID;
>>>>> 349
>>>>> 350      return 0;
>>>>> 351    }
>>>>> 352
>>>>> 353    return ret;
>>>>> 354  }
>>>>>
>>>>> Or am I missing something obvious ?
>>>>
>>>> These are called before relocation, and update gd->env_addr to non-relocated
>>>> default_environment by [3].
>>>>
>>>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>>>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>>>
>>>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>>>> |     /*
>>>> |      * Relocate the early env_addr pointer unless we know it is not inside
>>>> |      * the binary. Some systems need this and for the rest, it doesn't hurt.
>>>> |      */
>>>> |     gd->env_addr += gd->reloc_off;
>>>> | #endif
>>>>
>>>
>>> Shouldn't the post-relocation env update happen in env_relocate() ?
>>
>> Usually env_relocate() calls env_load() that uses relocated gd->env_addr.
>> It's no problem.
>>
>> If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
>> CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.

Sorry this isn't wrong.

> But then, if CONFIG_SYS_TEXT_BASE is zero, the env shouldn't be relocated or how should it behave ?

I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y
regardless of CONFIG_SYS_TEXT_BASE.

If CONFIG_POSITION_INDEPENDENT=y and CONFIG_SYS_TEXT_BASE is zero,
there is something wrong with the calculation of the relocation address about env.

gd->reloc_off is relocated address offset from zero, however,
gd->env_addr has still non-relocated address.

 >>>> |     gd->env_addr += gd->reloc_off;

I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y.
But this code sets gd->env_addr incorrectly.

In that case, there is a non-relocated <textbase> address instead of
CONFIG_SYS_TEXT_BASE.

This should be "gd->env_addr = (gd->env_addr - <textbase>) + gd->reloc_off",
However, I'm not sure how we get non-relocated <textbase> address.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 6/8/21 9:54 AM, Kunihiko Hayashi wrote:

Hi,

[...]

>>>>>> I would expect that after relocation, if all you have is env_nowhere
>>>>>> driver, the env_nowhere_init() is called again from the first 
>>>>>> for() loop
>>>>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID 
>>>>>> [1], and
>>>>>> that for() loop would exit with ret = -ENOENT [2], so then the 
>>>>>> last part
>>>>>> of env_init() would check for ret == -ENOENT and update 
>>>>>> gd->env_addr to
>>>>>> relocated default_environment [3].
>>>>>>
>>>>>> 324  int env_init(void)
>>>>>> 325  {
>>>>>> 326    struct env_driver *drv;
>>>>>> 327    int ret = -ENOENT;
>>>>>> 328    int prio;
>>>>>> 329
>>>>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); 
>>>>>> prio++) {
>>>>>>            /* Part [1] */
>>>>>> 331      if (!drv->init || !(ret = drv->init()))
>>>>>> 332        env_set_inited(drv->location);
>>>>>> 333      if (ret == -ENOENT)
>>>>>> 334        env_set_inited(drv->location);
>>>>>> 335
>>>>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>> 337            drv->name, ret);
>>>>>> 338
>>>>>>            /* Part [2] */
>>>>>> 339      if (gd->env_valid == ENV_INVALID)
>>>>>> 340        ret = -ENOENT;
>>>>>> 341    }
>>>>>> 342
>>>>>> 343    if (!prio)
>>>>>> 344      return -ENODEV;
>>>>>> 345
>>>>>>          /* Part [3] */
>>>>>> 346    if (ret == -ENOENT) {
>>>>>>            /* This should be relocated default_environment address */
>>>>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>>>>> 348      gd->env_valid = ENV_VALID;
>>>>>> 349
>>>>>> 350      return 0;
>>>>>> 351    }
>>>>>> 352
>>>>>> 353    return ret;
>>>>>> 354  }
>>>>>>
>>>>>> Or am I missing something obvious ?
>>>>>
>>>>> These are called before relocation, and update gd->env_addr to 
>>>>> non-relocated
>>>>> default_environment by [3].
>>>>>
>>>>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>>>>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>>>>
>>>>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>>>>> |     /*
>>>>> |      * Relocate the early env_addr pointer unless we know it is 
>>>>> not inside
>>>>> |      * the binary. Some systems need this and for the rest, it 
>>>>> doesn't hurt.
>>>>> |      */
>>>>> |     gd->env_addr += gd->reloc_off;
>>>>> | #endif
>>>>>
>>>>
>>>> Shouldn't the post-relocation env update happen in env_relocate() ?
>>>
>>> Usually env_relocate() calls env_load() that uses relocated 
>>> gd->env_addr.
>>> It's no problem.
>>>
>>> If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
>>> CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.
> 
> Sorry this isn't wrong.
> 
>> But then, if CONFIG_SYS_TEXT_BASE is zero, the env shouldn't be 
>> relocated or how should it behave ?
> 
> I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> regardless of CONFIG_SYS_TEXT_BASE.
> 
> If CONFIG_POSITION_INDEPENDENT=y and CONFIG_SYS_TEXT_BASE is zero,
> there is something wrong with the calculation of the relocation address 
> about env.

Ah, got it.

> gd->reloc_off is relocated address offset from zero, however,
> gd->env_addr has still non-relocated address.
> 
>  >>>> |     gd->env_addr += gd->reloc_off;
> 
> I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y.
> But this code sets gd->env_addr incorrectly.
> 
> In that case, there is a non-relocated <textbase> address instead of
> CONFIG_SYS_TEXT_BASE.
> 
> This should be "gd->env_addr = (gd->env_addr - <textbase>) + 
> gd->reloc_off",
> However, I'm not sure how we get non-relocated <textbase> address.

Maybe what you need to do is store current $pc register when you enter 
U-Boot very early on, in _start function, and then use it here ? 
Although, I am not entirely sure whether this is still possible on arm64.

Re: [PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Marek,

On 2021/06/10 10:07, Marek Vasut wrote:
> On 6/8/21 9:54 AM, Kunihiko Hayashi wrote:
> 
> Hi,
> 
> [...]
> 
>>>>>>> I would expect that after relocation, if all you have is env_nowhere
>>>>>>> driver, the env_nowhere_init() is called again from the first for() loop
>>>>>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
>>>>>>> that for() loop would exit with ret = -ENOENT [2], so then the last part
>>>>>>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>>>>>>> relocated default_environment [3].
>>>>>>>
>>>>>>> 324  int env_init(void)
>>>>>>> 325  {
>>>>>>> 326    struct env_driver *drv;
>>>>>>> 327    int ret = -ENOENT;
>>>>>>> 328    int prio;
>>>>>>> 329
>>>>>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>>>>>>>            /* Part [1] */
>>>>>>> 331      if (!drv->init || !(ret = drv->init()))
>>>>>>> 332        env_set_inited(drv->location);
>>>>>>> 333      if (ret == -ENOENT)
>>>>>>> 334        env_set_inited(drv->location);
>>>>>>> 335
>>>>>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>>> 337            drv->name, ret);
>>>>>>> 338
>>>>>>>            /* Part [2] */
>>>>>>> 339      if (gd->env_valid == ENV_INVALID)
>>>>>>> 340        ret = -ENOENT;
>>>>>>> 341    }
>>>>>>> 342
>>>>>>> 343    if (!prio)
>>>>>>> 344      return -ENODEV;
>>>>>>> 345
>>>>>>>          /* Part [3] */
>>>>>>> 346    if (ret == -ENOENT) {
>>>>>>>            /* This should be relocated default_environment address */
>>>>>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>>>>>> 348      gd->env_valid = ENV_VALID;
>>>>>>> 349
>>>>>>> 350      return 0;
>>>>>>> 351    }
>>>>>>> 352
>>>>>>> 353    return ret;
>>>>>>> 354  }
>>>>>>>
>>>>>>> Or am I missing something obvious ?
>>>>>>
>>>>>> These are called before relocation, and update gd->env_addr to non-relocated
>>>>>> default_environment by [3].
>>>>>>
>>>>>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>>>>>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>>>>>
>>>>>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>>>>>> |     /*
>>>>>> |      * Relocate the early env_addr pointer unless we know it is not inside
>>>>>> |      * the binary. Some systems need this and for the rest, it doesn't hurt.
>>>>>> |      */
>>>>>> |     gd->env_addr += gd->reloc_off;
>>>>>> | #endif
>>>>>>
>>>>>
>>>>> Shouldn't the post-relocation env update happen in env_relocate() ?
>>>>
>>>> Usually env_relocate() calls env_load() that uses relocated gd->env_addr.
>>>> It's no problem.
>>>>
>>>> If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
>>>> CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.
>>
>> Sorry this isn't wrong.
>>
>>> But then, if CONFIG_SYS_TEXT_BASE is zero, the env shouldn't be relocated or how should it 
>>> behave ?
>>
>> I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>> regardless of CONFIG_SYS_TEXT_BASE.
>>
>> If CONFIG_POSITION_INDEPENDENT=y and CONFIG_SYS_TEXT_BASE is zero,
>> there is something wrong with the calculation of the relocation address about env.
> 
> Ah, got it.
> 
>> gd->reloc_off is relocated address offset from zero, however,
>> gd->env_addr has still non-relocated address.
>>
>>  >>>> |     gd->env_addr += gd->reloc_off;
>>
>> I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y.
>> But this code sets gd->env_addr incorrectly.
>>
>> In that case, there is a non-relocated <textbase> address instead of
>> CONFIG_SYS_TEXT_BASE.
>>
>> This should be "gd->env_addr = (gd->env_addr - <textbase>) + gd->reloc_off",
>> However, I'm not sure how we get non-relocated <textbase> address.
> 
> Maybe what you need to do is store current $pc register when you enter U-Boot very early on, in 
> _start function, and then use it here ? Although, I am not entirely sure whether this is still 
> possible on arm64.

Exactly. I guess it's reasonable to fix gd->env_addr when POSITION_INDEPENDENT=y
before relocation. I'll try it.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

On 6/10/21 10:25 AM, Kunihiko Hayashi wrote:
Hi,

[...]

>>> gd->reloc_off is relocated address offset from zero, however,
>>> gd->env_addr has still non-relocated address.
>>>
>>>  >>>> |     gd->env_addr += gd->reloc_off;
>>>
>>> I think the env should be relocated if CONFIG_SYS_RELOC_GD_ENV_ADDR=y.
>>> But this code sets gd->env_addr incorrectly.
>>>
>>> In that case, there is a non-relocated <textbase> address instead of
>>> CONFIG_SYS_TEXT_BASE.
>>>
>>> This should be "gd->env_addr = (gd->env_addr - <textbase>) + 
>>> gd->reloc_off",
>>> However, I'm not sure how we get non-relocated <textbase> address.
>>
>> Maybe what you need to do is store current $pc register when you enter 
>> U-Boot very early on, in _start function, and then use it here ? 
>> Although, I am not entirely sure whether this is still possible on arm64.
> 
> Exactly. I guess it's reasonable to fix gd->env_addr when 
> POSITION_INDEPENDENT=y
> before relocation. I'll try it.

That sounds good, thank you !