Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini <trini@konsulko.com>]

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

On Mon, Oct 14, 2024 at 09:00:40AM +0200, Heinrich Schuchardt wrote:
> 
> 
> Am 14. Oktober 2024 05:51:22 MESZ schrieb Tom Rini <trini@konsulko.com>:
> >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 <trini@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 <sjg@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
> 
> It seems you are not providing a video console. So the EFI sub-system is asking the serial console for its size in query_console_serial().
> 
> Set CONFIG_VIDEO=y and let vidconsole be your first output device to avoid query_console_serial() being called.

That's not an answer to the problem. Please note that
rpi_arm64_defconfig shows this same issue, and it's about being able to
run tests on hardware and parse the output reasonably. And so:

> 
> Best regards
> 
> Heinrich
> 
> >> 
> >> 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? 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? 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.
> >
> >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.

My last question here was to you or Ilias, thanks.

-- 
Tom

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