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

[Simon Glass]

Hi Tom,

On Tue, 14 Jan 2025 at 19:48, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 14 Jan 2025 at 18:41, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, Jan 14, 2025 at 06:18:26PM -0700, Simon Glass wrote:
> > > 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).
> >
> > Personally, I would make the script that does the world build for size
> > test turn off the runner before starting and turn it back on when done
> > otherwise build time will suffer greatly and you would have been better
> > served doing it all on a slower class of machine.
> >
> > >
> > > > >
> > > > > > 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
> >
> > Ah, that is all also true, thanks.
> >
> > > > > > > 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
> >
> > Yes, I think we both agree there's a number of useless functionality
> > enabled on these handful of platforms. PARTITIONS explains how it gets
> > optimized away and all of the useless block-related things like MMC
> > core should also be dropped.
> >
> > > > - 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?
> >
> > I know I NAK'd
> > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-2-sjg@chromium.org/
> > as that should be instead done with
> > https://patchwork.ozlabs.org/project/uboot/list/?series=440336 instead.
> >
> > With that series then yes, you should be able to have BOOTSTD not depend
> > on BLK. But in a practical sense I worry that your changes in this
> > series will break with BLK=n. My point above is to please make sure it
> > still works in that case.
> >
> > > 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).
> >
> > In a practical sense EFI_LOADER depends on BLK and I've fixed the
> > dependency with my series and talked with Ilias and Heinrich about it.
> > Making EFI_LOADER buildable without BLK is a mechanical effort, but
> > making it usefully functional still might require a bit more work and
> > they'll cross that bridge when they come to it.
> >
> > > Anyway, from what I can tell, the code-size increase is explainable
> > > and perhaps we should just accept it.
> >
> > Other than the double buffer one, yes, I suppose it's not unreasonable
> > to ask me to fixup the oddball platforms with extraneous PARTITIONS and
> > other block type things enabled. Still need to get back to the buffer
> > one as 200 bytes isn't normal for re-organizing things such that the
> > compiler can't decide to inline something.
>
> What is the double buffer one?
>
> I have a v2 of this series which splits the first patch up a bit more. But the end result is the same.

I can send that if you like, but I just want to make sure I haven't
missed anything from this thread.

Regards,
SImon