[PATCH v11 00/24] Modernize U-Boot shell

[Tom Rini]

On Wed, Nov 08, 2023 at 12:14:28PM +0000, Peter Robinson wrote:
> On Wed, Nov 8, 2023 at 12:49 AM Francis Laniel
> <francis.laniel at amarulasolutions.com> wrote:
> >
> > Hi.
> >
> >
> > During 2021 summer, Sean Anderson wrote a contribution to add a new shell, based
> > on LIL, to U-Boot [1, 2].
> > While one of the goals of this contribution was to address the fact actual
> > U-Boot shell, which is based on Busybox hush, is old there was a discussion
> > about adding a new shell versus updating the actual one [3, 4].
> >
> > So, in this series, with Harald Seiler, we updated the actual U-Boot shell to
> > reflect what is currently in Busybox source code.
> > Basically, this contribution is about taking a snapshot of Busybox shell/hush.c
> > file (as it exists in commit 37460f5da) and adapt it to suit U-Boot needs.
> >
> > This contribution was written to be as backward-compatible as possible to avoid
> > breaking the existing.
> > So, the 2021 hush flavor offers the same as the actual, that is to say:
> > 1. Variable expansion.
> > 2. Instruction lists (;, && and ||).
> > 3. If, then and else.
> > 4. Loops (for, while and until).
> > No new features offered by Busybox hush were implemented (e.g. functions).
> >
> > It is possible to change the parser at runtime using the "cli" command:
> > => cli print
> > old
> > => cli set 2021
> > => cli print
> > 2021
> > => cli set old
> > The default parser is the old one.
> > Note that to use both parser, you would need to set both CONFIG_HUSH_2021_PARSER
> > and CONFIG_HUSH_OLD_PARSER.
> >
> > In terms of testing, new unit tests were added to ut to ensure the new behavior
> > is the same as the old one and it does not add regression.
> > Nonetheless, if old behavior was buggy and fixed upstream, the fix is then added
> > to U-Boot [5].
> > In sandbox, all of these tests pass smoothly:
> > => printenv board
> > board=sandbox
> > => ut hush
> > Running 20 hush tests
> > ...
> > Failures: 0
> > => parser set 2021
> > => ut hush
> > Running 20 hush tests
> > ...
> > Failures: 0
> >
> > Thanks to the effort of Harald Seiler, I was successful booting a board:
> > => printenv fdtfile
> > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb
> > => cli get
> > old
> > => boot
> > ...
> > root at lepotato:~#
> > root at lepotato:~# reboot
> > ...
> > => cli set 2021
> > => cli get
> > 2021
> > => printenv fdtfile
> > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb
> > => boot
> > ...
> > root at lepotato:~#
> >
> > I had to not use CONFIG_HUSH_2021_PARSER for the keymile board.
> > Indeed, the keymile board family is the only set of boards to call
> > get_local_var(), set_local_var() and unset_local_var().
> > Sadly, these functions are static in this contribution.
> > I could have change all of them to introduce code like this:
> > *_local_var(/*...*/)
> > {
> >         if (gd->flags & GD_FLG_HUSH_OLD_PARSER)
> >                 return *_local_var_old(/*...*/);
> >         if (gd->flags & GD_FLG_HUSH_2021_PARSER)
> >                 return *_local_var_2021(/*...*/);
> > }
> > But this would have mean renaming all old hush functions calls and I did not
> > want to change the old hush particularly to avoid breaking things.
> > Instead, I change the keymile board to use environment variable instead of local
> > ones.
> > I think this particularities can be addressed in future works.
> >
> > I also had to enable CONFIG_LTO for kirkwoord sheevaplug and phytec bk4r1, so
> > they do not hit their limits.

Note that v11 only enables LTO on sheevaplug, out of this list. And a
quick test right now shows that it's not needed either. And I'm putting
out some general feedback now as well so that v12 can go in to -next.

And I did throw this at my hardware lab and eveything boots and tests
run as normal.

> With nearly 15K lines of code added and the above limits being reached
> how much does this increase the size of a binary if it's selected?
> Those sorts of details are useful in these cover letters. Also of the
> 13k lines in cli_hush_upstream.c how much of that functionality is
> actually used in U-Boot? You mention functions are not, while I
> understand adding it straight from busybox has it's advantages for
> easier rebasing, if it's also pulling in a lot of code that is never
> used there's also detractions to adding 13k lines of code.

To repeat myself from last night,
https://gist.github.com/trini/53d9a3d59c797ecdbb3aec8edbbb9a12 is my
world build, before/after on current next. The gains are between 4172
bytes on the high side (r8a77980_v3hsk and family, which enable LTO and
seems like a size loss in this case) with 3500-3800 bytes being average
for aarch64. On arc it's only 2300-2500 bytes. 32bit ARM is similar to
aarch64 with the outliers being the platforms which enable unit tests so
have new tests too. On 32bit ARM, LTO is a clear win as those platforms
tend to grow by only 2200-2400 bytes. I'll note MIPS grows by almost
5000 bytes in some cases and PowerPC is in the mid 4000s. Everything
else is within those smaller bounds. All in all, since we're talking
about 13+ years I think of code progress all at once, these are quite
acceptable numbers and we wouldn't have noticed if it had been a yearly
resync instead, over time.

In terms of functionality out of 13k lines of code, it's trickier to
say. One could use the tool whose name evades me that just removes
unreachable due to defines code to see how much of the busybox.c file we
use. It wouldn't be something to commit as that would make resync a
nightmare (and we don't do it today). But I can say from looking at the
map files, we aren't compiling anything to then discard, so there's no
extra code being built that we don't need. Perhaps an audit of some of
the structs is in order, but upstream has done a good job already it
looks like in terms of keeping functionality isolated like that.

I do suspect we might have a few places where there's !__U_BOOT__ tests
being added that we can avoid by disabling features, or maybe don't need
since the feature flag is already off, and I might play with that in a
bit.

But overall, I think the argument for why lwip is good also applies
here. We're better off tracking a well known and updated upstream than
staying off in the weeds on our own.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231109/b93429f1/attachment.sig>