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.lan...@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@lepotato:~# > > root@lepotato:~# reboot > > ... > > => cli set 2021 > > => cli get > > 2021 > > => printenv fdtfile > > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb > > => boot > > ... > > root@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
signature.asc
Description: PGP signature