[PATCH] board_f: Relocate fdt if SKIP_RELOC and fdt is in bss

[Kunihiko Hayashi]

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 at 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 at 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 at 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