[PATCH 6/6] fdtdec: apply DT overlays from bloblist

[Michal Simek]

Hi,

On 6/19/25 18:22, Raymond Mao wrote:
> Hi Michal,
> 
> On Thu, 19 Jun 2025 at 07:39, Michal Simek <michal.simek at amd.com> wrote:
>>
>>
>>
>> On 6/18/25 16:59, Raymond Mao wrote:
>>> During FDT setup, apply all existing DT overlays from the bloblist
>>> to the base FDT if bloblist is being used for handoff from previous
>>> boot stage.
>>
>> More technical details would be good to have here.
>> Especially that the way what you are using is to expand DT
>>
> Agree. I can add more details in the next post.
> 
>>>
>>> Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
>>> ---
>>>    lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 56 insertions(+)
>>>
>>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>>> index c38738b48c7..3a218d8c94a 100644
>>> --- a/lib/fdtdec.c
>>> +++ b/lib/fdtdec.c
>>> @@ -1687,6 +1687,60 @@ void fdtdec_setup_embed(void)
>>>        gd->fdt_src = FDTSRC_EMBED;
>>>    }
>>>
>>> +static int fdtdec_apply_dto_from_blob(void **blob)
>>> +{
>>> +     int ret;
>>> +     struct fdt_header *live_fdt;
>>> +     size_t new_fdt_size, blob_size;
>>> +     int expand_by;
>>> +
>>> +     if (!CONFIG_IS_ENABLED(OF_LIBFDT_OVERLAY) ||
>>> +         !CONFIG_IS_ENABLED(BLOBLIST))
>>> +             return -EPERM;
>>> +
>>> +     live_fdt = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
>>> +     ret = fdt_check_header(live_fdt);
>>> +     if (ret)
>>> +             return ret;
>>
>>
>> Why is this needed? You already have gd->fdt_blob which has been checked.
>>
> You are right, these can be skipped.
> 
>>
>>> +
>>> +     ret = fdt_check_header(*blob);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     blob_size = fdt_totalsize(*blob);
>>> +
>>> +     /* Expand the FDT for spare spaces to apply the overlay */
>>> +     new_fdt_size = fdt_totalsize(live_fdt) + blob_size + 0x400;
>>
>> Some notes about this 0x400 would be good. Do you see issue if you don't use
>> this value? Is file DTB after expansion actually bigger then origin+overlay that
>> you need to add 0x400 magic value here?
>>
> The value 0x400 is arbitrary and it really depends on the DTOs.
> When applying a DTO, it may create new entries in:
> The structure block (nodes, properties)
> The strings block (all unique strings, including property names and values)
> These new entries require extra space, not just equal to the overlay
> size - because:
> Some parts will bring in new strings, which get appended to the string table.
> Also alignment padding might require more bytes.
> What I am concerned about is that 0x400 might be not sufficient for
> those complex DTOs.
> Maybe more bytes are required.

Valid concern but it should be described in commit message to also say that it 
is just a number based on your experiments but maybe it is not enough.


> 
>>
>>> +     ret = bloblist_resize(BLOBLISTT_CONTROL_FDT, new_fdt_size, &expand_by);
>>> +     if (ret)
>>> +             return ret;
>>
>> You do resize inside tl list.. That should be described in commit message
>> because my exception was that this location can be read only from NS world.
>>
> I can update the commit message with more details but why should it be
> read-only? At least I don't recall if the spec says the TL is not
> expected to be updated in BL33.

It is not about if should be read only but about it can be because it is 
exported from secure world. I understand why you are doing it but please 
describe it to make it clear.

Spec is saying
"TL relocation typically happens in later phases of the boot when there is
more memory available, which is needed for adding larger entries."

> 
>> I didn't run this on HW and likely I should do it to understand this better
>> but isn't this moving CONTROL_FDT entry to different location which will have
>> bigger space for applying overlay?
>>
> Do you mean to move it to a predefined address like existing
> CONFIG_BLOBLIST_ADDR with more space reserved? I am open to it.

I don't want this because I don't want to define address where it should be.
I want to have one universal bootloader which can run from different memory 
locations and relocate based on information in DT/TL.

I just want to make sure that description declare that properly how it is 
supposed to be used.

> But what I have to say is that the entry memory can only be marked
> with void, no reclaim memory mechanisms. This leads to duplicated
> memory ranges.
> Other than that, I think dynamically increasing/shrinking the size is
> more flexible.

I have to take a look how exactly this is working. I though that you can reclaim 
that memory back. We plan to have limited space (6MB) for TL. There are going to 
be multiple DT overlays inside. It means I don't want to be any close to 6MB limit.


>> Isn't it also worth to check if existing CONTROL_FDT has enough space inside
>> that new overlay can fit there before resizing it?
> By default each entry only contains the actual size since bloblist
> (aka transfer list) is designed as a compact data structure and there
> should not be a waiver for CONTROL_FDT to carry additional spare
> spaces.

Right but you can build dtb via dtc -S/-p to add additional padding. It means 
dtb will be bigger which goes to TL and should be possible to fit overlay there.


>>
>>
>>> +
>>> +     /* The blob is shifted if it was following the FDT */
>>> +     if (*blob > (void *)live_fdt)
>>> +             *blob += expand_by;
>>> +
>>> +     ret = fdt_open_into(live_fdt, live_fdt, new_fdt_size);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = fdt_overlay_apply_verbose(live_fdt, *blob);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* Shrink the excessive spaces */
>>> +     fdt_pack(live_fdt);
>>> +     new_fdt_size = fdt_totalsize(live_fdt);
>>> +     ret = bloblist_resize(BLOBLISTT_CONTROL_FDT, new_fdt_size, &expand_by);
>>> +     if (ret)
>>> +             return ret;
>>
>> I didn't try to run on HW but don't you need to also adjust gd->fdt_blob to
>> point to current location where fdt is?
>>
> gd->fdt_blob is already pointed to the one inside TL, it remains the
> same, only the size is changed.

I though that you are moving base DTB inside TL list. Isn't this the case?
I have to do closer look and debug your code to understand it better.

Thanks,
Michal