[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