| Message ID | 20230608065258.7769-1-hayashi.kunihiko@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | board_f: Relocate fdt if SKIP_RELOC and fdt is in bss | expand |
Hi Kunihiko, On Thu, 8 Jun 2023 at 07:53, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote: > > There are cases that the devicetree blob is placed after _end, such as > fdt_find_separate() returns _end. This is in bss area cleared before > relocation. > > When GD_FLG_SKIP_RELOC is set, the blob is still in bss, but will be > cleared. As a result, the devicetree become invalid. > > To avoid this issue, should relocate it to the new fdt area using the > latter condition in reloc_fdt(). > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > common/board_f.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/common/board_f.c b/common/board_f.c > index 51d2f3c365e9..9a245872d190 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -622,7 +622,8 @@ static int init_post(void) > static int reloc_fdt(void) > { > if (!IS_ENABLED(CONFIG_OF_EMBED)) { > - if (gd->flags & GD_FLG_SKIP_RELOC) > + if (gd->flags & GD_FLG_SKIP_RELOC && > + gd->fdt_blob != &_end) !IS_ENABLED(CONFIG_OF_EMBED) == IS_ENABLED(CONFIG_OF_SEPARATE) reloc_fdt() is only called by U-Boot (not SP) So the FDT was found by fdt_find_separate() and gd->fdt_blob == &_end So, is there any case where: gd->flags & GD_FLG_SKIP_RELO is true gd->fdt_blob != &_end is true ? I can't think of one. If that is the case, then you could add a comment to this effect and unconditionally relocate if !CONFIG_OF_EMBED. Of course the down size is that you would probably rather relocate it to just after BSS (rather than to the top of memory) but it doesn't much matter, I suspect. > return 0; > if (gd->new_fdt) { > memcpy(gd->new_fdt, gd->fdt_blob, > -- > 2.25.1 > Regards, Simon
Hi Simon, Thank you for your comment. On 2023/06/13 6:17, Simon Glass wrote: > Hi Kunihiko, > > On Thu, 8 Jun 2023 at 07:53, Kunihiko Hayashi > <hayashi.kunihiko@socionext.com> wrote: >> >> There are cases that the devicetree blob is placed after _end, such as >> fdt_find_separate() returns _end. This is in bss area cleared before >> relocation. >> >> When GD_FLG_SKIP_RELOC is set, the blob is still in bss, but will be >> cleared. As a result, the devicetree become invalid. >> >> To avoid this issue, should relocate it to the new fdt area using the >> latter condition in reloc_fdt(). >> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >> --- >> common/board_f.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/common/board_f.c b/common/board_f.c >> index 51d2f3c365e9..9a245872d190 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -622,7 +622,8 @@ static int init_post(void) >> static int reloc_fdt(void) >> { >> if (!IS_ENABLED(CONFIG_OF_EMBED)) { >> - if (gd->flags & GD_FLG_SKIP_RELOC) >> + if (gd->flags & GD_FLG_SKIP_RELOC && >> + gd->fdt_blob != &_end) > > !IS_ENABLED(CONFIG_OF_EMBED) == IS_ENABLED(CONFIG_OF_SEPARATE) > > reloc_fdt() is only called by U-Boot (not SP) > > So the FDT was found by fdt_find_separate() and gd->fdt_blob == &_end > > So, is there any case where: > > gd->flags & GD_FLG_SKIP_RELO is true > gd->fdt_blob != &_end is true > > ? Yes. But in fact, if (gd->flags & GD_FLG_SKIP_RELOC is true) and (gd->fdt_blob overlaps .bss section), the fdt should be relocated because clear_bss() will clear the fdt after that. > I can't think of one. > > If that is the case, then you could add a comment to this effect and > unconditionally relocate if !CONFIG_OF_EMBED. I'm not sure if it is possible to unconditionally relocate the fdt, I think we need to know if the fdt overlaps .bss section. > Of course the down size is that you would probably rather relocate it > to just after BSS (rather than to the top of memory) but it doesn't > much matter, I suspect. Thank you, --- Best Regards Kunihiko Hayashi
Hi Kunihiko, On Fri, 16 Jun 2023 at 00:59, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote: > > Hi Simon, > > Thank you for your comment. > > On 2023/06/13 6:17, Simon Glass wrote: > > Hi Kunihiko, > > > > On Thu, 8 Jun 2023 at 07:53, Kunihiko Hayashi > > <hayashi.kunihiko@socionext.com> wrote: > >> > >> There are cases that the devicetree blob is placed after _end, such as > >> fdt_find_separate() returns _end. This is in bss area cleared before > >> relocation. > >> > >> When GD_FLG_SKIP_RELOC is set, the blob is still in bss, but will be > >> cleared. As a result, the devicetree become invalid. > >> > >> To avoid this issue, should relocate it to the new fdt area using the > >> latter condition in reloc_fdt(). > >> > >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > >> --- > >> common/board_f.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/common/board_f.c b/common/board_f.c > >> index 51d2f3c365e9..9a245872d190 100644 > >> --- a/common/board_f.c > >> +++ b/common/board_f.c > >> @@ -622,7 +622,8 @@ static int init_post(void) > >> static int reloc_fdt(void) > >> { > >> if (!IS_ENABLED(CONFIG_OF_EMBED)) { > >> - if (gd->flags & GD_FLG_SKIP_RELOC) > >> + if (gd->flags & GD_FLG_SKIP_RELOC && > >> + gd->fdt_blob != &_end) > > > > !IS_ENABLED(CONFIG_OF_EMBED) == IS_ENABLED(CONFIG_OF_SEPARATE) > > > > reloc_fdt() is only called by U-Boot (not SP) > > > > So the FDT was found by fdt_find_separate() and gd->fdt_blob == &_end > > > > So, is there any case where: > > > > gd->flags & GD_FLG_SKIP_RELO is true > > gd->fdt_blob != &_end is true > > > > ? > > Yes. But in fact, > if (gd->flags & GD_FLG_SKIP_RELOC is true) and (gd->fdt_blob overlaps .bss section), > the fdt should be relocated because clear_bss() will clear the fdt after that. I misstated my point. I am wondering if gd(->fdt_blob != &_end) is ever false? If not, then we can always relocate. > > > I can't think of one. > > > > If that is the case, then you could add a comment to this effect and > > unconditionally relocate if !CONFIG_OF_EMBED. > > I'm not sure if it is possible to unconditionally relocate the fdt, > I think we need to know if the fdt overlaps .bss section. What is the down-side of always relocating, and when would the fdt not overlap the .bss section? Note that OF_EMBED is not to be used in production boards. That is violated by a few things at present, but IMO always relocating should be safe. > > > Of course the down size is that you would probably rather relocate it > > to just after BSS (rather than to the top of memory) but it doesn't > > much matter, I suspect. > Thank you, Regards, Simon
Hi Simon, On 2023/06/19 21:36, Simon Glass wrote: > Hi Kunihiko, > > On Fri, 16 Jun 2023 at 00:59, Kunihiko Hayashi > <hayashi.kunihiko@socionext.com> wrote: >> >> Hi Simon, >> >> Thank you for your comment. >> >> On 2023/06/13 6:17, Simon Glass wrote: >>> Hi Kunihiko, >>> >>> On Thu, 8 Jun 2023 at 07:53, Kunihiko Hayashi >>> <hayashi.kunihiko@socionext.com> wrote: >>>> >>>> There are cases that the devicetree blob is placed after _end, such as >>>> fdt_find_separate() returns _end. This is in bss area cleared before >>>> relocation. >>>> >>>> When GD_FLG_SKIP_RELOC is set, the blob is still in bss, but will be >>>> cleared. As a result, the devicetree become invalid. >>>> >>>> To avoid this issue, should relocate it to the new fdt area using the >>>> latter condition in reloc_fdt(). >>>> >>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>> --- >>>> common/board_f.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/common/board_f.c b/common/board_f.c >>>> index 51d2f3c365e9..9a245872d190 100644 >>>> --- a/common/board_f.c >>>> +++ b/common/board_f.c >>>> @@ -622,7 +622,8 @@ static int init_post(void) >>>> static int reloc_fdt(void) >>>> { >>>> if (!IS_ENABLED(CONFIG_OF_EMBED)) { >>>> - if (gd->flags & GD_FLG_SKIP_RELOC) >>>> + if (gd->flags & GD_FLG_SKIP_RELOC && >>>> + gd->fdt_blob != &_end) >>> >>> !IS_ENABLED(CONFIG_OF_EMBED) == IS_ENABLED(CONFIG_OF_SEPARATE) >>> >>> reloc_fdt() is only called by U-Boot (not SP) >>> >>> So the FDT was found by fdt_find_separate() and gd->fdt_blob == &_end >>> >>> So, is there any case where: >>> >>> gd->flags & GD_FLG_SKIP_RELO is true >>> gd->fdt_blob != &_end is true >>> >>> ? >> >> Yes. But in fact, >> if (gd->flags & GD_FLG_SKIP_RELOC is true) and (gd->fdt_blob overlaps .bss >> section), >> the fdt should be relocated because clear_bss() will clear the fdt after >> that. > > I misstated my point. I am wondering if gd(->fdt_blob != &_end) is > ever false? If not, then we can always relocate. When CONFIG_OF_SEPARATE is true, it seems that (gd->fdt_blob != &_end) is always false because the fdt file is concatinated to the end of the image file in Makefile. >> >>> I can't think of one. >>> >>> If that is the case, then you could add a comment to this effect and >>> unconditionally relocate if !CONFIG_OF_EMBED. >> >> I'm not sure if it is possible to unconditionally relocate the fdt, >> I think we need to know if the fdt overlaps .bss section. > > What is the down-side of always relocating, and when would the fdt not > overlap the .bss section? The fdt always overlaps the .bss section, so it should be relocated even if (gd->flags & GL_FLG_SKIP_RELOC) is true. > Note that OF_EMBED is not to be used in production boards. That is > violated by a few things at present, but IMO always relocating should > be safe. Yes, I undestand. I delete the condition in v2, and change to always relocate the fdt in case of !OF_EMBED. Thank you, --- Best Regards Kunihiko Hayashi
diff --git a/common/board_f.c b/common/board_f.c index 51d2f3c365e9..9a245872d190 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -622,7 +622,8 @@ static int init_post(void) static int reloc_fdt(void) { if (!IS_ENABLED(CONFIG_OF_EMBED)) { - if (gd->flags & GD_FLG_SKIP_RELOC) + if (gd->flags & GD_FLG_SKIP_RELOC && + gd->fdt_blob != &_end) return 0; if (gd->new_fdt) { memcpy(gd->new_fdt, gd->fdt_blob,
There are cases that the devicetree blob is placed after _end, such as fdt_find_separate() returns _end. This is in bss area cleared before relocation. When GD_FLG_SKIP_RELOC is set, the blob is still in bss, but will be cleared. As a result, the devicetree become invalid. To avoid this issue, should relocate it to the new fdt area using the latter condition in reloc_fdt(). Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- common/board_f.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)