Re: [PATCH v2 17/33] boot: Update fit_image_get_emb_data to use abuf

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7536 bytes --]

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@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@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@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.

> > 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]