[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