[PATCH 28/29] Revert "bloblist: Load the bloblist from the previous loader"

[Tom Rini]

On Fri, Feb 07, 2025 at 01:26:46PM -0500, Raymond Mao wrote:
> Hi Simon,
> 
> On Fri, 7 Feb 2025 at 12:51, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Fri, 7 Feb 2025 at 08:10, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Thu, 6 Feb 2025 at 19:09, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Raymond,
> > > >
> > > > On Thu, 6 Feb 2025 at 15:35, Raymond Mao <raymond.mao at linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Thu, 6 Feb 2025 at 10:41, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Raymond,
> > > > > >
> > > > > > On Wed, 5 Feb 2025 at 08:25, Raymond Mao <raymond.mao at linaro.org> wrote:
> > > > > > >
> > > > > > > +CC Ilias,
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Tue, 4 Feb 2025 at 20:57, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > The logic of this has become too confusing.
> > > > > > > >
> > > > > > > > The primary issue with the patch is that U-Boot needs to set up a
> > > > > > > > bloblist in the first phase where BLOBLIST is enabled. Subsequent
> > > > > > > > phases can then use that bloblist.
> > > > > > > >
> > > > > > > > But the first phase of U-Boot cannot assume that one exists.
> > > > > > > >
> > > > > > > > Reverting this commit seems like a better starting point for getting
> > > > > > > > things working for all use-cases.
> > > > > > > >
> > > > > > > > This reverts commit 66131310d8ff1ba228f989b41bd8812f43be41c3.
> > > > > > > >
> > > > > > > > https://lore.kernel.org/u-boot/CAPnjgZ3hMHtiH=f5ZKXNniOfV_-vFryq1Gn7QZ5hKU8Wjo8igw@mail.gmail.com/
> > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > >
> > > > > > > If my understanding is correct, you want to add some logic to control
> > > > > > > when the U-Boot should or should not get the bloblist from the
> > > > > > > existing register argument.
> > > > > > > But xferlist_from_boot_arg() should be called when a valid register
> > > > > > > argument is there, I didn't see this in your patch.
> > > > > > > Maybe you plan to do this with other patch series, but simply
> > > > > > > reverting this results in a breaking of handoff policy and the
> > > > > > > firmware handoff won't work.
> > > > > >
> > > > > > Yes, I certainly did not want to revert it, but the current code is
> > > > > > too hard to understand and I did not look at it at the time it went
> > > > > > in. I've had three tries at working with what you have here, but each
> > > > > > turns to spaghetti.
> > > > > >
> > > > >
> > > > > Still not very clear on what concerns you have and what is the way you
> > > > > want to go.
> > > > > The logic is straight forward, when U-Boot has a previous loader and
> > > > > the registers pass in valid arguments - It indicates handoff should be
> > > > > done using the transfer list.
> > > > > Other kconfig options decide whether to use the passed in address
> > > > > directly or copy to a predefined address.
> > > > > But in either way, xferlist_from_boot_arg() is doing the right thing
> > > > > to get the transfer list from the register if it exists and is valid.
> > > > > I don't see a reason for removing it.
> > > >
> > > > Here is the initial code:
> > > > >>>>
> > > > bool from_addr = fixed && !xpl_is_first_phase();
> > > > /*
> > > > * If U-Boot is in the first phase that an arch custom routine should
> > > > * install the bloblist passed from previous loader to this fixed
> > > > * address.
> > > > */
> > > > bool from_boot_arg = fixed && xpl_is_first_phase();
> > > >
> > > > if (xpl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST))
> > > >    from_addr = false;
> > > > <<<<
> > > >
> > > > and by the way, that is my tree. In -next it is even worse:
> > > > >>>>
> > > > /*
> > > > * If U-Boot is not in the first phase, an existing bloblist must be
> > > > * at a fixed address.
> > > > */
> > > > bool from_addr = fixed && !xpl_is_first_phase();
> > > > /*
> > > > * If U-Boot is in the first phase that an arch custom routine should
> > > > * install the bloblist passed from previous loader to this fixed
> > > > * address.
> > > > */
> > > > bool from_boot_arg = fixed && xpl_is_first_phase();
> > > >
> > > > if (xpl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST))
> > > > from_addr = false;
> > > > if (fixed)
> > > > addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED,
> > > >       CONFIG_BLOBLIST_ADDR);
> > > > size = CONFIG_BLOBLIST_SIZE;
> > > >
> > > > if (from_boot_arg)
> > > > ret = xferlist_from_boot_arg(addr, size);
> > > > else if (from_addr)
> > > > ret = bloblist_check(addr, size);
> > > > <<<<
> > > >
> > > > I want to update it so that TPL creates a bloblist and passes it
> > > > through the following phases, ending up at U-Boot.
> > > >
> > > > To my mind, if CONFIG_IS_ENABLED(BLOBLIST), then we should check the
> > > > registers and always accept standard passage. If not, we should
> > > > either:
> > > > - create a bloblist (if this *is* the first phase with
> > > > CONFIG_IS_ENABLED(BLOBLIST))
> > > > - use an existing bloblistl, which must exist (if not)
> > > >
> > >
> > > To my mind, first we should check whether there is a valid transfer
> > > list passed in, which is done by xferlist_from_boot_arg().
> > > If it does, and BLOBLIST is selected, then use the transfer list as bloblist.
> > > And we do need a new kconfig (e.g. FIRMWARE_HANDOFF_MANDATORY) to
> > > determine whether fw handoff is mandatory.
> > >
> > > See below pseudo code:
> > > ```
> > > if (!xferlist_from_boot_arg()) {
> > >     /* a valid transfer list exists */
> > >     if (!CONFIG_IS_ENABLED(BLOBLIST))
> > >         return ERROR;
> > >
> > >     /* use the transfer list as a bloblist */
> > >     [...]
> > > } else if (CONFIG_IS_ENABLED(FIRMWARE_HANDOFF_MANDATORY)) {
> > >     /* no valid transfer list passed in, but the arm spec is mandatory */
> > >     return ERROR;
> > > } else {
> > >     /* arm spec is not in use at all, create or use a static bloblist */
> > >     [...]
> > > }
> > > ```
> > >
> >
> > Yes, that sounds good to me. Please go ahead.
> 
> @Tom Rini @Ilias Apalodimas What's your idea? If all of you feel good
> I will post a patch for it.

I think it's a step in the right direction, thanks.

-- 
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/20250207/eec5865d/attachment.sig>