[PATCH v2 17/33] boot: Update fit_image_get_emb_data to use abuf
Simon Glass
sjg at chromium.org
Fri Jan 10 20:23:42 CET 2025
Hi Tom,
On Fri, 10 Jan 2025 at 09:17, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Jan 10, 2025 at 06:39:11AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 9 Jan 2025 at 11:08, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Thu, Jan 09, 2025 at 08:14:53AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 9 Jan 2025 at 08:10, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jan 09, 2025 at 05:36:03AM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 8 Jan 2025 at 11:25, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jan 06, 2025 at 07:32:13AM -0700, Simon Glass wrote:
> > > > > > >
> > > > > > > > This function uses separate arguments for data and size. Use the new
> > > > > > > > abuf instead, so that they are paired and in one place. In some cases it
> > > > > > > > also saves an argument, thus potentially reducing code size.
> > > > > > >
> > > > > > > This is one of the commits that globally increases size in both full
> > > > > > > U-Boot and SPL/etc.
> > > > > > >
> > > > > > > Is all of the "abuf" changes just a "tidy up" that increases the code a
> > > > > > > bit?
> > > > > >
> > > > > > Yes, a tidy-up which I hope will help overall. I have been thinking
> > > > > > for a while of how to avoid having addr/size and ptr/size passed
> > > > > > everywhere. For now abuf seems to provide some sort of solution.
> > > > > >
> > > > > > I see this:
> > > > > >
> > > > > > 18: boot: Update fit_image_get_emb_data to use abuf
> > > > > > aarch64: (for 1/1 boards) all +4.0 bss -24.0 spl/u-boot-spl:all
> > > > > > +16.0 spl/u-boot-spl:text +16.0 text +28.0
> > > > > >
> > > > > > so growth on firefly-rk3399 but not with rk3288. I am not sure if the
> > > > > > growth will tail off as there are more users, though. We might even be
> > > > > > able to be more clever with static inlines.
> > > > >
> > > > > Yeah, lets not do this now then and worry about some "clean up" later
> > > > > when we can show that it does, or does not, improve size.
> > > >
> > > > Oh.
> > > >
> > > > > And there's
> > > > > something wrong with your numbers:
> > > > > 01: Fix neighbor discovery ethernet address saving
> > > > > aarch64: w+ firefly-rk3399
> > > > > +(firefly-rk3399) Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
> > > > > +(firefly-rk3399)
> > > > > +(firefly-rk3399) /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
> > > > > +(firefly-rk3399) See the documentation for your board. You may need to build ARM Trusted
> > > > > +(firefly-rk3399) Firmware and build with BL31=/path/to/bl31.bin
> > > > > +(firefly-rk3399) Image 'simple-bin' is missing optional external blobs but is still functional: tee-os
> > > > > +(firefly-rk3399) /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
> > > > > +(firefly-rk3399) See the documentation for your board. You may need to build Open Portable
> > > > > +(firefly-rk3399) Trusted Execution Environment (OP-TEE) and build with TEE=/path/to/tee.bin
> > > > > +(firefly-rk3399) Some images are invalid
> > > > > 37: dm: core: Provide ofnode_find_subnode_unit()
> > > > > aarch64: (for 1/1 boards) all +324.0 bss +32.0 spl/u-boot-spl:all +16.0 spl/u-boot-spl:text +16.0 text +292.0
> > > > > firefly-rk3399 : all +324 bss +32 spl/u-boot-spl:all +16 spl/u-boot-spl:text +16 text +292
> > > > > u-boot: add: 6/-1, grow: 4/-4 bytes: 516/-224 (292)
> > > > > function old new delta
> > > > > ofnode_name_eq_unit - 160 +160
> > > > > ofnode_find_subnode_unit - 116 +116
> > > > > fit_image_get_data 80 176 +96
> > > > > fit_image_get_emb_data - 84 +84
> > > > > ofnode_write_prop 224 236 +12
> > > > > ofnode_add_subnode 232 244 +12
> > > > > abuf_init_const - 12 +12
> > > > > abuf_init - 12 +12
> > > > > abuf_addr - 8 +8
> > > > > fit_image_print 780 784 +4
> > > > > image_locate_script 696 692 -4
> > > > > fit_image_load 1584 1580 -4
> > > > > fit_image_verify 176 164 -12
> > > > > ofnode_find_subnode 140 116 -24
> > > > > fit_image_get_data_and_size 180 - -180
> > > > > spl-u-boot-spl: add: 3/-1, grow: 0/-1 bytes: 108/-92 (16)
> > > > > function old new delta
> > > > > fit_image_get_emb_data - 84 +84
> > > > > abuf_init_const - 12 +12
> > > > > abuf_init - 12 +12
> > > > > load_simple_fit 580 568 -12
> > > > > fit_image_get_data 80 - -80
> > > >
> > > > Yes, that's the whole series, so not related to this change.
> > >
> > > Yes, that's the whole series including this change, so it's related to
> > > this change.
> >
> > Right, but it is due to ofnode_find_subnode(), etc.
> >
> > >
> > > > I elected to have two versions of ofnode_find_subnode() to avoid the
> > > > size growth in the previous version. But the cost is larger size
> > > > growth when OF_LIVE is used.
> > > >
> > > > Without OF_LIVE, the size growth is tiny.
> > >
> > > And even worse in SPL, somehow. But you want more OF_LIVE users, not
> > > less, yes?
> >
> > Well, OF_LIVE is always quite a bit larger, at least at the moment. It
> > has both Linux's of_access stuff and libfdt. It's not OF_LIVE I am
> > bothered about, but I do want people using ofnode. Unfortunately
> > people still send patches which use libfdt directly.
> >
> > >
> > > > So...what to do?
> > >
> > > Well, if you drop the abuf changes for now, SPL won't change at all for
> > > most platforms and that'll be an improvement.
> >
> > Yes, I'll look at that. This is one of many examples where I have a
> > problem and realise that we need a nicer way of dealing with it, then
> > implement it in the series, but then the series loses focus. So then I
> > take it out again, then forget about it until next time, but I never
> > actually make the change.
>
> I hate to start to derail this, but refactor for "nicer code" is very
> much subjective. Especially when it also grows the code (and it's not
> clear that wider usage would result in shrinkage). So yes, this really
> needs to be put aside and also part of why I keep asking for one thing
> at a time.
Yes I very much agree with this.
>
> > > And I'm going to keep complaining about size growth here because a
> > > non-trivial subset of users just wants things to boot quickly and be
> > > small.
> >
> > Yes, you won't get any complaints from me on that. I did propose some
> > automated checking a few years back, but it never went anywhere.
>
> It be great if buildman size comparison had some way to csv the output.
> That's what's missing imo from being able to have some automation or
> even just nicer tooling.
What sort of tooling could we have? I would like something in CI which
reports code-size changes in a useful way, perhaps failing if the
delta is too large for more than x boards. WDYT?
Regards,
Simon
More information about the U-Boot
mailing list