[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.