[PATCH 2/3] fdt: Allow expanding the devicetree during relocation

[Tom Rini]

On Fri, Dec 06, 2024 at 12:18:07PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 6 Dec 2024 at 07:13, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Fri, Dec 06, 2024 at 06:11:12AM -0700, Simon Glass wrote:
> >
> > [snip]
> > > A comment in that function says that we assume there is space after the
> > > existing fdt to use for padding, with the padding size set to
> > > CONFIG_SYS_FDT_PAD
> > >
> > > However, there is no guarantee that this space is available. If using
> > > the control FDT, then global_data is immediately above it, so expanding
> > > the FDT and adding FDT properties will cause U-Boot to fail.
> >
> > This sounds like a generic issue and should be fixed outside of a "Pi"
> > series.
> 
> Yes and I know you keep asking for multiple smaller series, but I've
> explained that keeping track of all that for months at a time is too
> challenging for me.

Yes, and it's also too challenging to review for the rest of us.

> > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > index 41a758e83a6..360611edd01 100644
> > > --- a/dts/Kconfig
> > > +++ b/dts/Kconfig
> > > @@ -219,6 +219,17 @@ config OF_OMIT_DTB
> > >         This is used for boards which normally provide a devicetree via a
> > >         runtime mechanism (such as OF_BOARD), to avoid confusion.
> > >
> > > +config OF_EXPAND
> > > +     hex # "Amount to allow the control FDT to expand"
> >
> > No "# ..." because the help explains what to do here, if needed.
> 
> OK
> 
> >
> > > +     default SYS_FDT_PAD if OF_LIBFDT
> > > +     default 0
> >
> > A "default 0" is almost always wrong.
> 
> In this case it is correct, since it means not to add any extra space
> for the FDT.

Which is still wrong? Because we don't allow for this particular device
tree in memory to be read-only and so the question is "Do you want to
break things on some platforms or not?".

> > > +     help
> > > +       Some boards make use of the control FDT to boot an OS, thus when
> > > +       image_setup_libfdt() adds extra things to the end of the FDT, there
> > > +       needs to be enough space.
> > > +
> > > +       Set this to the number of bytes of extra space required for the FDT.
> >
> > We should just use SYS_FDT_PAD directly and not add a new option.
> 
> I thought I read somewhere that fdt_high == -1 should not be used. So
> for most boards, this padding is not needed?

That's also true, and gets back to why putting multiple patches in a
single series is a Bad Idea. The function in question isn't doing some
check for if we've disabled device tree relocation for the OS, it's
early on, it's reserving our control fdt, yes? And then the problem ends
up being later on, we use the control fdt for the OS, and modify it, but
haven't ensured there's enough space for said modifications to not stomp
on something else? That is a much bigger generic problem, no?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241206/b7374e16/attachment.sig>