Hi Michal,
On Thu, 19 Jun 2025 at 07:39, Michal Simek <michal.si...@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....@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.
>
> > + 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.
> 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.
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.
> 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.
>
>
> > +
> > + /* 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.
Regards,
Raymond
> Thanks,
> Michal
>
>
