From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 17EB3C77B7D for ; Sat, 13 May 2023 21:42:30 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 159C38471F; Sat, 13 May 2023 23:42:28 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=amarulasolutions.com header.i=@amarulasolutions.com header.b="BDlNkqvx"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E3F6C85E81; Sat, 13 May 2023 23:42:25 +0200 (CEST) Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BF69D8471F for ; Sat, 13 May 2023 23:42:22 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=francis.laniel@amarulasolutions.com Received: by mail-wm1-x333.google.com with SMTP id 5b1f17b1804b1-3f427118644so69961395e9.0 for ; Sat, 13 May 2023 14:42:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; t=1684014142; x=1686606142; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=WEspmlioJ/rsQ5IdyzfkfPYTPYd07KKRmjNYoWX74DQ=; b=BDlNkqvxacd9j+D/gGipbw/bw9aXfuw0rL5e4OW4HlZgkzkw38JWVhMDAoLRDaWb6r DGKH6iZAgibLK3Alq8JHvfKPsB67N0fYrUC5p2bbJaxg4Q869zUFYIJoCiDYGdZSY5SF zi7TXVi6fOH0mePB0qxk2nNemHvu7pQ+IXK6Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684014142; x=1686606142; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WEspmlioJ/rsQ5IdyzfkfPYTPYd07KKRmjNYoWX74DQ=; b=K+C+Pg9MUckYul8xRCMMOX4k6tlE3tM1m5uqT4z4idmeT6Hw9IQi9TEknXB/PIIq26 K8ckjHmja/ZoedbEtqQf6oczc8pL22Z2P0Fh4oUix0BjtyZMpsIfdxt8Dk0DzAPK5b/q H/ALTOP0AfEl0NV+4GR+AEGQDQuHKTOYRHhe/ttNNTqqzCTTuC0z0tB3tGnGHhnaONg6 yLdaNP3O4H91uIJUlPB9q44hGYD8ppP+aIZ/oQufuGm7IKfKWRiCyx3hqqgsH2gOFfCY vaLYzZa/WlV+waJ+Na3O849q2BqaPhuaVODYj2hX8DxtB4ZRTosdAGD7vsTozTSYCnwE ly7g== X-Gm-Message-State: AC+VfDxA9SxmazPLUtHfyu0BKgMIRowE/Cri75FL6s0ldyJMOGqKqcLb 2weLaQojlvsp7ahQ8276y/Iivg== X-Google-Smtp-Source: ACHHUZ5FEnJxkbZX/lMYO/LdYyD6f19pJmAb0kD33LBcS4LBlECtP11GjqI4i0qjiTkE7MQ8ZCuiUw== X-Received: by 2002:a1c:ed0d:0:b0:3f4:f012:5cae with SMTP id l13-20020a1ced0d000000b003f4f0125caemr7048432wmh.20.1684014142002; Sat, 13 May 2023 14:42:22 -0700 (PDT) Received: from pwmachine.localnet ([213.94.16.214]) by smtp.gmail.com with ESMTPSA id j10-20020adff54a000000b00304b5b2f5ffsm27509627wrp.53.2023.05.13.14.42.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 13 May 2023 14:42:21 -0700 (PDT) From: Francis Laniel To: Peter Robinson Cc: u-boot@lists.denx.de, Michael Nazzareno Trimarchi , Tom Rini , Simon Glass , Harald Seiler Subject: Re: [RFC PATCH v8 00/23] Modernize U-Boot shell Date: Sat, 13 May 2023 22:42:20 +0100 Message-ID: <3738095.kQq0lBPeGt@pwmachine> In-Reply-To: References: <20230512200331.51457-1-francis.laniel@amarulasolutions.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Le samedi 13 mai 2023, 10:54:10 WEST Peter Robinson a =C3=A9crit : > On Fri, May 12, 2023 at 9:04=E2=80=AFPM Francis Laniel >=20 > wrote: > > Hi. > >=20 > >=20 > > During 2021 summer, Sean Anderson wrote a contribution to add a new she= ll, > > based on LIL, to U-Boot [1, 2]. > > While one of the goals of this contribution was to address the fact act= ual > > 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]. > >=20 > > So, in this series, with Harald Seiler, we updated the actual U-Boot sh= ell > > 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 su= it > > U-Boot needs. > >=20 > > 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. function= s). >=20 > What sort of impact does this have on firmware sizes? Without any numbers, I would have said this increase it as I had to use=20 CONFIG_LTO for some boards to be inside the board limit. So, I compiled all amlogic boards with and without the new parser: odroid-c4;796;800;4;0.503 libretech-s905d-pc;764;768;4;0.524 beelink-gsking-x;796;796;0;0 bananapi-m2s;816;816;0;0 beelink-gtking;796;800;4;0.503 odroid-n2l;784;788;4;0.51 libretech-cc_v2;740;744;4;0.541 beelink-gt1-ultimate;736;740;4;0.543 odroid-hc4;852;856;4;0.469 p200;592;596;4;0.676 sei510;880;884;4;0.455 khadas-vim3l_android_ab;972;976;4;0.412 khadas-vim3;876;880;4;0.457 libretech-s912-pc;764;768;4;0.524 s400;692;696;4;0.578 libretech-cc;728;732;4;0.549 jethub_j80;712;716;4;0.562 wetek-play2;696;700;4;0.575 sei610;884;884;0;0 wetek-core2;748;752;4;0.535 khadas-vim3l;872;876;4;0.459 khadas-vim3l_android;968;972;4;0.413 khadas-vim3_android;968;972;4;0.413 jethub_j100;728;732;4;0.549 khadas-vim2;712;716;4;0.562 p201;592;592;0;0 radxa-zero2;768;772;4;0.521 bananapi-m5;796;800;4;0.503 p212;644;648;4;0.621 khadas-vim3_android_ab;972;976;4;0.412 u200;732;736;4;0.546 nanopi-k2;596;600;4;0.671 khadas-vim;684;688;4;0.585 libretech-ac;760;764;4;0.526 wetek-hub;696;700;4;0.575 odroid-go-ultra;752;756;4;0.532 odroid-c2;696;700;4;0.575 bananapi-m2-pro;788;792;4;0.508 bananapi-cm4-cm4io;820;824;4;0.488 odroid-n2;808;812;4;0.495 radxa-zero;772;772;0;0 beelink-gtkingpro;796;800;4;0.503 I tried to pretty everything with "column" but when I paste the output in m= y=20 mailer this is not pretty, so I prefer to paste the CSV. Here are the explanations for the columns: 1. The first indicates the board name. 2. The second indicates u-boot.bin size in K (given by "du -sh") compiled f= or=20 old parser. 3. The third indicates u-boot.bin size in K (given by "du -sh") compiled fo= r=20 new parser. 4. The fourth indicates the absolute difference, so third column minus seco= nd=20 one. 5. The fifth indicates the relative difference in percent, rounded with 10^= =2D3=20 precision. The sample is indeed not representative of all u-boot boards as I compiled= =20 only one family. But we can see that the increase is in the worst case of 4K, which seems=20 acceptable with regard to the original size. > > It is possible to change the parser at runtime using the "cli" command: > > =3D> cli print > > old > > =3D> cli set 2021 > > =3D> cli print > > 2021 > > =3D> 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. > >=20 > > 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 t= hen > > added to U-Boot [5]. > > In sandbox, all of these tests pass smoothly: > > =3D> printenv board > > board=3Dsandbox > > =3D> ut hush > > Running 20 hush tests > > ... > > Failures: 0 > > =3D> parser set 2021 > > =3D> ut hush > > Running 20 hush tests > > ... > > Failures: 0 > >=20 > > Thanks to the effort of Harald Seiler, I was successful booting a board: > > =3D> printenv fdtfile > > fdtfile=3Damlogic/meson-gxl-s905x-libretech-cc.dtb > > =3D> cli get > > old > > =3D> boot > > ... > > root@lepotato:~# > > root@lepotato:~# reboot > > ... > > =3D> cli set 2021 > > =3D> cli get > > 2021 > > =3D> printenv fdtfile > > fdtfile=3Damlogic/meson-gxl-s905x-libretech-cc.dtb > > =3D> boot > > ... > > root@lepotato:~# > >=20 > > I was able to have the CI passes but 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(/*...*/) > > { > >=20 > > if (gd->flags & GD_FLG_HUSH_OLD_PARSER) > > =20 > > return *_local_var_old(/*...*/); > > =20 > > if (gd->flags & GD_FLG_HUSH_2021_PARSER) > > =20 > > return *_local_var_2021(/*...*/); > >=20 > > } > > 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. > >=20 > > I also had to enable CONFIG_LTO for kirkwoord sheevaplug and phytec bk4= r1, > > so they do not hit their limits. > >=20 > > For all these reasons, I marked this contribution as RFC to indeed coll= ect > > your opinions. > > My goal is not to change suddenly actual shell to this one, we clearly > > need a transition period to think about it. > > I think it is better to see this contribution as a proof of concept whi= ch > > shows it is possible to update the actual shell. > >=20 > > If you want to review it - your review will really be appreciated - here > > are some information regarding the commits: > > * commits marked as "test:" deal with unit tests. > > * commit "cli: Add Busybox upstream hush.c file." copies Busybox > > shell/hush.c into U-Boot tree, this explain why this commit contains > > around 12000 additions. * commit "cli: Port Busybox 2021 hush to U-Boot= =2E" > > modifies previously added file to permit us to use this as new shell. > > The really good idea of #include'ing Busybox code into a wrapper file to > > define some particular functions while minimizing modifications to > > upstream code comes from Harald Seiler. > > * commit "cmd: Add new parser command" adds a new command which permits > > selecting parser at runtime. > > I am not really satisfied with the fact it calls cli_init() and cli_loo= p() > > each time the parser is set, so your reviews would be welcomed. > > * Other commits focus on enabling features we need (e.g. if). > >=20 > > Changes since: > > v2: > > * Added a small fix to compile sandbox with NO_SDL=3D1. > > * Added a command to change parser at runtime. > > * Added 2021 parser function to all run_command*(). > > =20 > > v3: > > * Various bug fixes pointed by the CI. > > * Added upstream busybox hush commits until 6th February 2022. > > =20 > > v4: > > * Various cleaning. > > * Modified python test to accept failure output when the test are > > designed to fail. > > * Bumped upstream busybox hush commits until 24h March 2022. > > =20 > > v5: > > * Bumped upstream busybox hush commits until 30th January 2023. > > * Fix how hush interprets '<' and '>', indeed we needed to escape them > > but I removed this behavior as tests are handled by test command and > > not hush itself. This permitted to have the ut fdt to pass. > > * Fix a problem with how exit was handled. This was reported by the ut > > exit > > test. > > =20 > > v6: > > * There was no v6 and I got mixed up with version. > > =20 > > v7: > > * Bumped upstream busybox hush commits until 9th May 2023. > > * Renamed parser command to change parser at runtime to cli and added > > documentation. > > * Added better separation of patches. > > * Removed code about __gnu_thumb1_case_si as it was merged in another > > series. * Various cleaning. > >=20 > > Francis Laniel (23): > > test: Add framework to test hush behavior > > test: hush: Test hush if/else > > test/py: hush_if_test: Remove the test file > > test: hush: Test hush variable expansion > > test: hush: Test hush commands list > > test: hush: Test hush loops > > cli: Add Busybox upstream hush.c file > > cli: Port Busybox 2021 hush to U-Boot > > cli: Add menu for hush parser > > global_data.h: add GD_FLG_HUSH_OLD_PARSER flag > > cmd: Add new cli command > > cli: Enables using hush 2021 parser as command line parser > > cli: hush_2021: Enable variables expansion for hush 2021 > > cli: hush_2021: Add functions to be called from run_command() > > cli: add hush 2021 as parser for run_command*() > > test: hush: Fix instructions list tests for hush 2021 > > test: hush: Fix variable expansion tests for hush 2021 > > cli: hush_2021: Enable using < and > as string compare operators > > cli: hush_2021: Enable if keyword > > cli: hush_2021: Enable loops > > test: hush: Fix loop tests for hush 2021 > > cli: hush_2021: Add upstream commits up to 9th May 2023. > > DO NOT MERGE: only to make CI happy > > =20 > > cmd/Kconfig | 22 + > > cmd/Makefile | 2 + > > cmd/cli.c | 125 + > > common/Makefile | 3 +- > > common/cli.c | 82 +- > > common/cli_hush_2021.c | 306 + > > common/cli_hush_upstream.c | 13021 +++++++++++++++++++++++++++ > > configs/sheevaplug_defconfig | 1 + > > doc/usage/cmd/cli.rst | 74 + > > doc/usage/index.rst | 1 + > > include/asm-generic/global_data.h | 8 + > > include/cli_hush.h | 51 +- > > include/test/hush.h | 15 + > > include/test/suites.h | 1 + > > test/Makefile | 3 + > > test/cmd_ut.c | 6 + > > test/hush/Makefile | 10 + > > test/hush/cmd_ut_hush.c | 20 + > > test/hush/dollar.c | 226 + > > test/hush/if.c | 316 + > > test/hush/list.c | 140 + > > test/hush/loop.c | 91 + > > test/py/tests/test_hush_if_test.py | 197 - > > test/py/tests/test_ut.py | 8 +- > > tools/patman/series.py | 4 + > > 25 files changed, 14521 insertions(+), 212 deletions(-) > > create mode 100644 cmd/cli.c > > create mode 100644 common/cli_hush_2021.c > > create mode 100644 common/cli_hush_upstream.c > > create mode 100644 doc/usage/cmd/cli.rst > > create mode 100644 include/test/hush.h > > create mode 100644 test/hush/Makefile > > create mode 100644 test/hush/cmd_ut_hush.c > > create mode 100644 test/hush/dollar.c > > create mode 100644 test/hush/if.c > > create mode 100644 test/hush/list.c > > create mode 100644 test/hush/loop.c > > delete mode 100644 test/py/tests/test_hush_if_test.py > >=20 > > Best regards and thank you in advance. > > --- > > [1] https://lists.denx.de/pipermail/u-boot/2021-July/453347.html > > [2] https://runtimeterror.com/tech/lil/ > > [3] https://lists.denx.de/pipermail/u-boot/2021-July/453790.html > > [4] https://lists.denx.de/pipermail/u-boot/2021-July/453848.html > > [5] https://lists.denx.de/pipermail/u-boot/2021-August/458569.html > > -- > > 2.34.1