Hi Tom,
On Sun, 13 Oct 2024 at 21:51, Tom Rini <tr...@konsulko.com> wrote:
>
> On Sun, Oct 13, 2024 at 01:33:23PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 11 Oct 2024 at 16:32, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
> > >
> > > > Add a simple test of booting with the EFI bootmeth, which runs the app
> > > > and checks that it can call 'exit boot-services' (to check that all the
> > > > device-removal code doesn't break anything) and then exit back to
> > > > U-Boot.
> > > >
> > > > This uses a disk image containing the testapp, ready for execution by
> > > > sandbox when needed.
> > > >
> > > > Signed-off-by: Simon Glass <s...@chromium.org>
> > >
> > > So lets go to the problem point. What asserts here fail due to ANSI
> > > escape codes being in the output and needing to be filtered out?
> > >
> > > [snip]
> > > > diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> > > > index e05103188b4..da2ee1ab345 100644
> > > > --- a/test/boot/bootflow.c
> > > > +++ b/test/boot/bootflow.c
> > > > @@ -13,6 +13,7 @@
> > > > #include <cli.h>
> > > > #include <dm.h>
> > > > #include <efi_default_filename.h>
> > > > +#include <efi_loader.h>
> > > > #include <expo.h>
> > > > #ifdef CONFIG_SANDBOX
> > > > #include <asm/test.h>
> > > > @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android);
> > > > extern U_BOOT_DRIVER(bootmeth_cros);
> > > > extern U_BOOT_DRIVER(bootmeth_2script);
> > > >
> > > > +/* Use this as the vendor for EFI to tell the app to exit boot
> > > > services */
> > > > +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
> > > > +
> > > > static int inject_response(struct unit_test_state *uts)
> > > > {
> > > > /*
> > > > @@ -1205,3 +1209,62 @@ static int bootflow_android(struct
> > > > unit_test_state *uts)
> > > > return 0;
> > > > }
> > > > BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
> > > > +
> > > > +/* Test EFI bootmeth */
> > > > +static int bootflow_efi(struct unit_test_state *uts)
> > > > +{
> > > > + /* disable ethernet since the hunter will run dhcp */
> > > > + test_set_eth_enable(false);
> > > > +
> > > > + /* make USB scan without delays */
> > > > + test_set_skip_delays(true);
> > > > +
> > > > + bootstd_reset_usb();
> > > > +
> > > > + /* Avoid outputting ANSI characters which mess with our asserts */
> > > > + efi_console_set_ansi(false);
> > > > +
> > > > + ut_assertok(bootstd_test_drop_bootdev_order(uts));
> > > > + ut_assertok(run_command("bootflow scan", 0));
> > > > + ut_assert_skip_to_line(
> > > > + "Bus usb@1: scanning bus usb@1 for devices... 5 USB
> > > > Device(s) found");
> > > > +
> > > > + ut_assertok(run_command("bootflow list", 0));
> > > > +
> > > > + ut_assert_nextlinen("Showing all");
> > > > + ut_assert_nextlinen("Seq");
> > > > + ut_assert_nextlinen("---");
> > > > + ut_assert_nextlinen(" 0 extlinux");
> > > > + ut_assert_nextlinen(
> > > > + " 1 efi ready usb_mass_ 1
> > > > usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
> > > > + ut_assert_nextlinen("---");
> > > > + ut_assert_skip_to_line("(2 bootflows, 2 valid)");
> > > > + ut_assert_console_end();
> > > > +
> > > > + ut_assertok(run_command("bootflow select 1", 0));
> > > > + ut_assert_console_end();
> > > > +
> > > > + systab.fw_vendor = test_vendor;
> > > > +
> > > > + ut_asserteq(1, run_command("bootflow boot", 0));
> > > > + ut_assert_nextline(
> > > > + "** Booting bootflow
> > > > 'usb_mass_storage.lun0.bootdev.part_1' with efi");
> > > > + if (IS_ENABLED(CONFIG_LOGF_FUNC))
> > > > + ut_assert_skip_to_line(" efi_run_image() Booting
> > > > /\\EFI\\BOOT\\BOOTSBOX.EFI");
> > > > + else
> > > > + ut_assert_skip_to_line("Booting
> > > > /\\EFI\\BOOT\\BOOTSBOX.EFI");
> > > > +
> > > > + /* TODO: Why the \r ? */
> > > > + ut_assert_nextline("U-Boot test app for EFI_LOADER\r");
> > > > + ut_assert_nextline("Exiting boot sevices");
> > > > + if (IS_ENABLED(CONFIG_LOGF_FUNC))
> > > > + ut_assert_nextline(" do_bootefi_exec() ## Application
> > > > failed, r = 5");
> > > > + else
> > > > + ut_assert_nextline("## Application failed, r = 5");
> > > > + ut_assert_nextline("Boot failed (err=-22)");
> > > > +
> > > > + ut_assert_console_end();
> > > > +
> > > > + return 0;
> > > > +}
> > > > +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
> >
> > Thanks for taking an interest in this.
> >
> > Nothing at present, as I added ut_assert_skip_to_line() to skip it
> > all. But it really is just wrong...I get gibberish showing up in the
> > terminal after I run tests! This is what I see with this series:
> >
> > ** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi
> > 7 [r [999;999H [6n 8No EFI system partition
> >
> > ^^^ strange output there but I'm not sure if it will come through on email
> >
> > No EFI system partition
> > Failed to persist EFI variables
> > No EFI system partition
> > Failed to persist EFI variables
> > No EFI system partition
> > Failed to persist EFI variables
> > Booting /\EFI\BOOT\BOOTSBOX.EFI
> > U-Boot test app for EFI_LOADER
> >
> > Exiting boot sevices
> > ## Application failed, r = 5
> > Boot failed (err=-22)
> > Failures: 0
>
> So we're finally making progress I think to see what the problem you're
> trying to solve is. I think the next thing to figure out is if you use
> picocom or screen or whatever on console, the codes are handled
> correctly, or no? Specifically, picocom?
This is my running tests on my normal machine in a terminal. So it is
just a sandbox connection, with no picocom, etc. See the Spawn class
in test/py
> Next, is this perhaps just
> another strange artifact of how oddly (and as you've noted before,
> slowly) pytest consumes the console input? It feels like maybe we're
> doing several layers of wrong there?
Well to be frank I would have just applied my ANSI patch some months
ago and stopped worrying about it. We just shouldn't be sending ANSI
characters unless we know there is a terminal there. See Color() in
tools/u_boot_pylib/terminal.py
But even if we did want to blindly send characters, we should have a
way to turn it off.
> Because to be clear, this is not a
> sandbox problem. Looking back at the log I had posted about the watchdog
> reset not counting banners correctly I see that same escape sequence,
> now that I know what I'm looking for/at.
I see it all over CI as well. We also have [1].
>
> All of that said, if it's not a pytest-consuming-the-console problem
> (and it's not a picocom problem, like I was just asking about since the
> board failure in question was via labgrid-client), maybe it's just a
> thing to note about making sure tests understand. And perhaps document
> somewhere why it matters (and I'm not saying it doesn't!) that we know
> what the dimensions of the console are, for EFI, and so that's why we
> init them when/where we do.
Or perhaps just have a way to turn it off? I first sent this patch
last November. It is just wrong to generate output like this which we
don't want. There isn't even a test for it, so just add a way to
disable it, and be done!
Regards,
SImon
[1]
https://patchwork.ozlabs.org/project/uboot/patch/20240329160922.127095-1-heinrich.schucha...@canonical.com/
