[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

Tue Dec 17 21:15:46 CET 2024

On Tue, Dec 17, 2024 at 12:45:46PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 16 Dec 2024 at 21:00, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sun, Dec 15, 2024 at 05:25:24PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 13 Dec 2024 at 10:07, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, Dec 13, 2024 at 07:32:24AM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 12 Dec 2024 at 08:11, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > need to have this be configurable because we do not (and cannot) know
> > > if
> > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > address this early.
> > > > > >
> > > > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > > > ---
> > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > ---
> > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > >
> > > > >
> > > > > Since this is the essentially the same as my OF_BLOBLIST patch, which
> > > > > was rejected:
> > > > >
> > > > > NAK
> > > > >
> > > > > The issue is not whether some 'previous stage' set up a bloblist. For
> > > > > example, if VPL_BLOBLIST_PRIOR_STAGE is enabled, that presumably means
> > > > > that TPL set it up. But it doesn't mean that TPL put the FDT in there.
> > > >
> > > > This is wrong. The root problem is saying that the bloblist is in
> > > > possibly uninitialized memory. The code is quite happy to NOT find the
> > > > device tree in the bloblist and continue searching other paths.
> > >
> > > But until the bloblist is set up, it doesn't exist.
> >
> > An almost philosophical statement, yes. How should the generic code know
> > if it exists, or does not exist? That is the question.
> 
> U-Boot code 'knows', since if this is the first phase which enables
> CONFIG_IS_ENABLED(BLOBLOST), then the bloblist is available
> before/after relocation as well as after RAM setup in SPL.

And when U-Boot isn't the first stage?

> > > We should not be
> > > looking for a bloblist that we know is not there yet. The bloblist is set
> > > up by bloblist_init(). You seem to be imputing your own semantics for
> > > bloblist.
> >
> > We don't know if the bloblist exists or not. That's the problem. It
> > could already exist. That's the whole reason we're in this particular
> > argument.
> 
> See above.

Yes, you seem to be missing the case where U-Boot isn't the first stage.

> > > In SPL, init happens in board_init_r(), i.e. after RAM is set up. So don't
> > > look in the bloblist until it is set up! It doesn't exist.
> >
> > Unless of course it was setup before. We don't know if it was setup
> > before or not until we look. A big part of the problem is that for prior
> > to U-Boot bloblist we don't need to use main DRAM, the bloblist is tiny
> > and we have some other persistent memory available. Unfortunately, x86
> > does not.
> 
> The only situation where this matters is for the devicetree, which I
> why i created OF_BLOBLIST, a way to indicate that the bloblist should
> be checked for a devicetree.

Yes, you keep looking at this from the wrong direction and seem to have
backed yourself in to a corner. Why are you insisting that for the
normal use case of memory infos and maybe display parameters we need to
carve out a hunk of main memory? A bloblist is perfectly capable of NOT
containing things. We can avoid having to care about what phase of
things we're in too.

> > > In U-Boot proper, we look for it before relocation, so we can relocate and
> > > expand it for use with ACPI tables, etc.
> >
> > I'm not sure that's relevant. This means we've taken what we were passed
> > at a fixed address and allocated more space for it and are using it
> > somewhere else.
> 
> Yes
> 
> >
> > > Take a look at this patch, too:
> > >
> > > 3d6531803e1 bloblist: Support initing from multiple places
> >
> > I'm not sure it's relevant. It does show we have a problem of not
> > knowing if the bloblist exists, or not. And I don't think we're solving
> > that right today (nor does v1 of my patch).
> 
> OK
> 
> >
> > > > An alternative here would be to better document the requirements of
> > > > BLOBLIST_ADDR, and then just disable it on chromebook_coral until
> > > > someone is inclined to move DDR init in to TPL, or someone finds a
> > > > non-main-memory address to use for BLOBLIST_ADDR or switches to using
> > > > BLOBLIST_ALLOC.
> > >
> > > Coral's TPL runs in cache-as-ram, with a 30KB limit on code size. It cannot
> > > do DDR init in TPL because the DDR blob is large (160KB?) and there is a
> > > ton of setup to do first, in any case. When CAR goes away, its memory
> > > vanishes, so you need to have copied the bloblist somewhere else (the
> > > bloblist holds the MRC data which needs to be written to SPI flash later
> > > on, when possible). This all works perfectly and there is really no need to
> > > change anything.
> >
> > Ah, so there's some of the details finally. And oh goodness, we're
> > writing to the SPI flash on each boot? That's not good for longevity...
> 
> It writes a sector each time, across an area which can hold quite a
> few writes. It only writes when the details change, so it is fine.
> This algorithm has worked in the field for years across tens of
> millions of devices and is common on x86.

To be clear, I wasn't saying that its your design.

> > > Again, you seem to be imputing semantics to bloblist which don't exist.
> >
> > Unfortunately some things were missed along the way, and are still
> > missing.
> 
> Well we should have started from OF_BLOBLIST and then dealt with
> problems as they came up. In fact, so far there are no problems which
> it can't solve.
> 
> As it is, we have Raymond sending a patch to push the prior-stage mess
> into TPM, for reasons which make no sense to me. It is just adding
> confusion.
> 
> One other thing we would have likely done by now if my patch[1] had
> gone in a year ago, is moving away from BLOBLIST_FIXED to using a
> register protocol. That is more deterministic.
> 
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/

I think once again your obsession with OF_BLOBLIST and device tree is
causing you to miss everything else. Today we can take a *bloblist* via
register. Linaro wrote that, even. The next problem really is that we
can't pass that bloblist along to the next stage because all of the
U-Boot side of things assumes fixed address.

-- 
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/20241217/75f3d7d3/attachment.sig>