[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

Wed Dec 18 02:46:58 CET 2024

Hi Tom,

On Tue, 17 Dec 2024 at 17:18, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Dec 17, 2024 at 04:42:26PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 17 Dec 2024 at 13:15, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > 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?
> >
> > The only case where that matters is for the devicetree, so we have
> > OF_BLOBLIST to control whether U-Boot looks for it. Eventually we can
> > drop BLOBLIST_FIXED and use registers, perhaps even on x86, which will
> > make things easier.
>
> The only reason it ever matters is if you're picking memory that's not
> always accessible. Which is a bad choice and should be avoided whenever
> possible and practical. Which it is here.
>
> > > > > > 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.
> >
> > I really can't see how you came to that conclusion. Just enable
> > OF_BLOBLIST - in fact we can enable it for almost all boards today and
> > it won't harm anything.
>
> I mean, there's very few places using BLOBLIST today. But we can't
> enable OF_BLOBLIST for any of them unless we say that the BLOBLIST
> exists in always accessible memory. And that wouldn't be most of them.
>
> > > > > > 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.
> >
> > Devicetree is the only 'strange' thing here because we need to look at
> > it early.
>
> Device tree is also "strange" in that it's the only thing we read (not
> write) today prior to U-Boot too from bloblist.
>
> > For Raymond's patch, it shouldn't need any conditions.
>
> Yes, and I replied to him that way 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.
> >
> > No, it isn't my design, but I would be happy if it were. You were
> > suggesting it had a longevity problem, so I wanted to explain it.
> >
> > >
> > > > > > 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.
> >
> > Perhaps if we had applied OF_BLOBLIST a year ago, so my special 3
> > ancient chromebooks that nobody cares about or uses apart from me,
> > worked, we could have got bloblist to where it should be today. You
> > are missing the big picture, but stopping me from pursuing it.
> >
> > I am disappointed that it is only at this stage that you have shown
> > any interest in getting my special 3 ancient chromebooks running.
>
> And I am disappointed that it's only at this stage there's an
> understanding of what the actual problem is with your special 3 ancient
> chromebooks. Everything last year was read by me as you arguing that you
> need a way to control passing the device tree because that's where the
> device tree is:
> https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/#3174895
> And if not every at least most of your other replies in that thread are
> about how device tree is passed in. This is why I assumed that your
> boards were working after having a way to enforce that the device tree
> comes from bloblist.

That thread is crazy. I should have given up at v1. I wish you hadn't
assumed that and I am still a bit shocked that you did, particularly
after 4-5 revisions.

>
> So if there's some "big picture" about bloblist being delayed, it is
> because once again your vision isn't communicated to everyone else
> you're trying to work with and that is the big problem.

Yes, agreed, that is exactly the problem. I hope my tree will solve
that problem and everyone will be happier.

Regards,
Simon