[PATCH 01/15] vbe: Split out some VBE code into a common file

[Simon Glass]

Hi Tom,

On Tue, 14 Jan 2025 at 10:33, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Jan 14, 2025 at 10:58:01AM -0600, Tom Rini wrote:
> > On Tue, Jan 14, 2025 at 05:58:04AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 13 Jan 2025 at 18:22, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Thu, Jan 09, 2025 at 05:29:56AM -0700, Simon Glass wrote:
> > > >
> > > > > Loading a FIT is useful for other VBE methods, such as ABrec, so start
> > > > > a new common file. Add functions for reading the version and nvdata.
> > > > > Also add a function to get the block device.
> > > >
> > > > This is not a good commit message when you're also introducing
> > > > functional changes (blk_dread -> blk_read).
> > >
> > > Yes, I did not actually notice this size change at all, as mentioned,
> > > as I only built for a few boards (firefly rk3399/rk3288 and a few
> > > sandbox ones). The blk_dread() function is the old API for reading
> >
> > You have much faster build machines available than I do, it would be
> > good to run a world build before/after (it might take an hour on
> > alexandra) before posting these big series.

OK. I used to do this, but with CI sometimes grabbing the machines it
ends up with a huge load and sometimes runs out of memory. I suppose I
can stop the runner before doing it and remember to restart it
afterwards. Or perhaps Alex has enough memory per thread? (96GB).

> >
> > > from a block. It didn't occur to me that blk_dread() would bypass the
> > > cache.
> >
> > Yes, how is that happening? That's what doesn't make sense.

OK, I see. It is actually one level deeper than I thought.

PARTITIONS is not enabled, for example on phycore_am62x_r5_usbdfu
which means that blk_get_dev() returns NULL

So long as all the code is in one module, blk_get_dev() returning NULL
causes the calls to vbe_simple_read_state() and simple_read_nvdata()
to be dropped, so their code is dropped too, so there is no
blk_read()/blk_dread() call. It is optimised away. It just happens to
be the only call in U-Boot

> >
> > >
> > > > Further, this functional
> > > > change seems to introduce some other code being pulled in very
> > > > unexpectedly, and that needs to be explained. For example,
> > > > phycore_am62x_r5_usbdfu is another case and that has
> > > > CONFIG_BLOCK_CACHE=y but CONFIG_SPL_BLOCK_CACHE=n, and yet the size
> > > > growth is in full U-Boot.
> > >
> > > This board currently uses blk_dread(), but not blk_read(), so the
> > > addition of a blk_read() call in non-SPL causes the BLK cache to be
> > > pulled in.
> >
> > But, how? I mean, this sounds like some underlying bug that needs to be
> > fixed. The platform sets CONFIG_BLK=y and CONFIG_BLOCK_CACHE=y and
> > drivers/block/blk-uclass.c has:
> > ulong blk_dread(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt,
> >                 void *buffer)
> > {
> >         return blk_read(desc->bdev, start, blkcnt, buffer);
> > }
> >
> > Or perhaps the answer / problem here is that I need to track down what's
> > going wrong here instead of asking you to track this down.
>
> So, digging in to this, what's going on is that the platforms I've noted
> here (and a few others) don't actually enable any block devices. That's
> why, today, with VBE on still, they discard all of the code still. Your
> rework means that while there's still no block devices, we're now
> including the code. What to do? Both of:
> - These platforms *should* disable the assorted block device related
>   library options they have enabled if / when possible (it might not be
>   a neat and easy un-tangle).

As above, the issue seems to be PARTITIONS

> - VBE needs to handle the case where there's not a block device enabled
>   and not fail the build. Part of this requires the Kconfig rework I
>   linked to earlier (so that we can have BLK off) to be complete, but
>   having the VBE code check for BLK being enabled either in Makefile or
>   C code should be the same regardless.

I thought you NAK'd my patch to drop the 'depends on BLK' for bootstd?

>From what I can tell EFI_LOADER doesn't do any block access unless
PARTITIONS is enabled. I don't want that for VBE because the VBE data
is not actually in a partition (although perhaps it could be in
future).

Anyway, from what I can tell, the code-size increase is explainable
and perhaps we should just accept it.

Regards,
Simon