| Message ID | 1620828554-24013-1-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | env: Leave invalid env for nowhere location | expand |
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.
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
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; > }
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
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
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
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 ?
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
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.
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
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 !
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; }
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(-) -- 2.7.4