Re: [PATCH 30/32] fdt: Allow the devicetree to come from a bloblist

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 8173 bytes --]

On Fri, Oct 20, 2023 at 11:21:29AM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> On Wed, 18 Oct 2023 at 18:26, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 25 Sept 2023 at 04:19, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > > >>>>>>>>>> +config OF_BLOBLIST
> > > > > > >>>>>>>>>> +     bool "DTB is provided by a bloblist"
> > > > > > >>>>>>>>>> +     help
> > > > > > >>>>>>>>>> +       Select this to read the devicetree from the bloblist. This allows
> > > > > > >>>>>>>>>> +       using a bloblist to transfer the devicetree between  U-Boot phases.
> > > > > > >>>>>>>>>> +       The devicetree is stored in the bloblist by an early phase so that
> > > > > > >>>>>>>>>> +       U-Boot can read it.
> > > > > > >>>>>>>>>> +
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>> I dont think this is a good idea.  We used to have 4-5 different options
> > > > > > >>>>>>>>> here, which we tried to clean up and ended up with two very discrete
> > > > > > >>>>>>>>> options.  Why do we have to reintroduce a new one?  Doesn't that bloblist
> > > > > > >>>>>>>>> have a header of some sort?  The bloblist literally comes from a previous
> > > > > > >>>>>>>>> stage bootloader which is what OF_BOARD is here for. So why can't we just
> > > > > > >>>>>>>>> read the header and figure out if the magic of the bloblist matches?
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> No, OF_BOARD is a hack to allow boards to do what they like with the FDT.
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> This patch is a standard mechanism to pass the DT from one firmware
> > > > > > >>>>>>>> phase to the next. We have spent quite a bit of time creating a spec
> > > > > > >>>>>>>> for it, and we should use it.
> > > > > > >>>>>>>
> > > > > > >>>>>>> Where exactly am I objecting using the spec?   Can you please re-read my email?
> > > > > > >>>>>>> I am actually pointing out we should use the spec *properly*.  So
> > > > > > >>>>>>> instead of having a Kconfig option for the DT, which is pretty
> > > > > > >>>>>>> pointless,  we should parse the bloblist.  If the header defined by
> > > > > > >>>>>>> the *spec* is found, we should just search for the DT in there.
> > > > > > >>>>>>> What you are doing here, is take the spec, pick a very specific item
> > > > > > >>>>>>> that the list contains, and create a Kconfig option out of it.  Which
> > > > > > >>>>>>> basically ignores the discoverable options of the bloblist.  For
> > > > > > >>>>>>> example, that bloblist can also contain an entry to a TPM eventlog.
> > > > > > >>>>>>> Should we start creating Kconfig options for all the firmware handoff
> > > > > > >>>>>>> entries that are defined on that spec?
> > > > > > >>>>>>
> > > > > > >>>>>> OK so that is a different thing. What should it do if it expects to find a bloblist but cannot? I want it to throw an error, because I am trying to make the boot deterministic. What do you think?
> > > > > > >>>>>
> > > > > > >>>>> That's fine by me.  You can even put that under IS_ENABLED for the
> > > > > > >>>>> bloblist inside the existing OF_BOARD check.  So I was thinking
> > > > > > >>>>> - If no bloblist is required in Kconfig options we do the hacks we used to
> > > > > > >>>>> - if bloblist is selected and the config option is OF_BOARD, throw an
> > > > > > >>>>> error and mention that the previous stage loader should hand over a DT
> > > > > > >>>>>
> > > > > > >>>>> Is that what you had in mind?
> > > > > > >>>>
> > > > > > >>>> Yes, that sounds good to me.
> > > > > > >>>>
> > > > > > >>>> But I still think we need an OF_BLOBLIST option to control whether the
> > > > > > >>>> devicetree comes from there.
> > > > > > >>>>    Otherwise we will end up with people
> > > > > > >>>> using OF_BOARD to work around it. We also have the SPL case which does
> > > > > > >>>> not pass the DT in a bloblist...in fact SPL typically doesn't even
> > > > > > >>>> have the full DT.
> > > > > > >>>
> > > > > > >>> Wouldn't IS_ENABLED(BLOBLIST) || IS_ENABLED(SPL_BLOBLIST) be enough?
> > > > > > >>> Inside the OF_BOARD portion of the function, search for a bloblist if
> > > > > > >>> the option is enabled.  If you can't find a bloblist and the DT within
> > > > > > >>> that bloblist, then error out
> > > > > > >>
> > > > > > >> Just my 2c here.
> > > > > > >> I think all options should be possible to disable. It means I can imagine to
> > > > > > >> disable u-boot not to take care about DT provided from previous stage. The same
> > > > > > >> is for TPM event log. IMHO every stage should have an option to simply ignore
> > > > > > >> data pass from previous stage. I don't really mind how it is done but there
> > > > > > >> should be an option to by purpose say to ignore the part of pass data.
> > > > > > >
> > > > > > > That would be done by disabling BLOBLIST.  I don't think having a Kconfig to say
> > > > > > > "I want a bloblist, but I only want the DT from there" is reasonable
> > > > > > > (or for any other item the bloblist can carry).   OF_BOARD means the
> > > > > > > DT will come from a previous stage loader. If bloblist is enabled then
> > > > > > > the DT must come from there.
> > > > > >
> > > > > > I don't agree on this. If bloblist is enabled and DT is passed SW should have a
> > > > > > freedom to ignore it.
> > > > > >
> > > > > > At start everything is likely in sync but later stages before U-Boot can stay at
> > > > > > certain version but there could be a need to update u-boot where DT from
> > > > > > previous stage could be broken.
> > > > >
> > > > > But you *can* ignore it and load a different one later.  The only
> > > > > restriction is that if you compile u-boot with the assumption an early
> > > > > stage bootloader provides a DT you *must* find it.  But we will still
> > > > > just keep 2 options in U-Boot of how you get a DT.
> > > > > A previous loader provided it or U-Boot provided it.  Whether that
> > > > > comes from a bloblist or a register is irrelevant no ?
> > > >
> > > > I'm not sure what is being requested here, so I'll leave this as is for v2.
> > >
> > > Please don't.   A few mails above there's a discussion of how I'd
> > > prefer things to look like, please have a look and let me know if
> > > something isn't clear.
> > > tl;dr
> > >  "That's fine by me.  You can even put that under IS_ENABLED for the
> > >  bloblist inside the existing OF_BOARD check.  So I was thinking
> > >  - If no bloblist is required in Kconfig options we do the hacks we used to
> > >  - if bloblist is selected and the config option is OF_BOARD, throw an
> > > error and mention that the previous stage loader should hand over a DT"
> > >
> > > >
> > > > The main struggle I have is how to tell whether you expect to
> > > > *receive* the DT in the bloblist, or expect it to be attached to the
> > > > image and be *sent* to the next phase.
> > >
> > > bloblist
> > >
> > > >
> > > > SPL - attached to image, send in bloblist
> > > > U-Boot proper - not attached to image, receive it from bloblist
> > > >
> > > > This is exactly the problem that is solved by the 'standard passage'
> > > > stuff [1] but that depends on Firmware Handoff and [2] which are not
> > > > ready yet...
> > > >
> > > > So I think what I have is the best we can do for now.
> > >
> > > We can avoid the extra complication in Kconfigs.  The DT either comes
> > > from u-boot itself or from a previous stage loader. we don't need the
> > > extra "it comes from a bloblist".
> > > If they come from a previous stage loader and BLOBLIST *is* enabled,
> > > then just scan for the DT, if you don't find it error out.
> >
> > Are you planning to send a patch for this? Otherwise, what do you
> > think about going with this one and dealing with OF_BOARD board by
> > board?
> 
> Yes, I can do that.  Since this patch is part of your series, I'll
> send it over to you and carry it there

The rest of this series, minus this patch has been merged.  We should
just start a new series I think.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]