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

[Simon Glass]

Hi Tom,

On Mon, 9 Dec 2024 at 11:07, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Tom,
>
> On Fri, 6 Dec 2024 at 12:49, Tom Rini <trini at konsulko.com> wrote:
> >
> > 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?
>
> It seems that people are already aware of the problem this patch
> addresses, but since it is specific to fdt_high, which should not be
> used, it seems unnecessary to worry about it.
>
> I'm going to drop this patch.

I forgot to mention one other thing that I noticed.

If someone uses U-Boot with this series, but does not erase fdt_high
from the environment, then it will not boot. It seems like a bit of a
trap, to me.

Regards,
Simon