[PATCH v3 0/3] test: test UEFI boot manager

[PATCH v3 0/3] test: test UEFI boot manager

[Heinrich Schuchardt]

A unit test for the boot manager is provided.

In the test a disk images is prepared and bound as device in the sandbox.
intrddump.efi is run as boot binary. The CRC32 of the initrd image
presented via the EFI_LOAD_FILE2_PROTOCOL is checked.

The initrddump.efi application is adjusted to better fit into the Python
test framework.

The unit test patch was dropped when merging the series
"[PATCH v2 0/9] efi_loader: booting via short-form device-path"
https://lists.denx.de/pipermail/u-boot/2022-March/478410.html
due to test failures caused by initrddump.efi.

Heinrich Schuchardt (3):
  efi_loader: nocolor command line attr for initrddump.efi
  efi_loader: initrddump: drain input before prompt
  test: test UEFI boot manager

 lib/efi_loader/initrddump.c                   | 91 ++++++++++++++++---
 test/py/tests/test_efi_bootmgr/conftest.py    | 42 +++++++++
 .../test_efi_bootmgr/test_efi_bootmgr.py      | 31 +++++++
 3 files changed, 151 insertions(+), 13 deletions(-)
 create mode 100644 test/py/tests/test_efi_bootmgr/conftest.py
 create mode 100644 test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py

-- 
2.34.1

[PATCH v3 1/3] efi_loader: nocolor command line attr for initrddump.efi

[Heinrich Schuchardt]

initrddump.efi uses colored output and clear the screen. This is not
helpful for integration into Python tests. Allow specifying 'nocolor' in
the load option data to suppress color output and clearing the screen.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	new patch
---
 lib/efi_loader/initrddump.c | 77 ++++++++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/lib/efi_loader/initrddump.c b/lib/efi_loader/initrddump.c
index 7de43bcfff..25dbd635e5 100644
--- a/lib/efi_loader/initrddump.c
+++ b/lib/efi_loader/initrddump.c
@@ -4,6 +4,9 @@
  *
  * initrddump.efi saves the initial RAM disk provided via the
  * EFI_LOAD_FILE2_PROTOCOL.
+ *
+ * Specifying 'nocolor' as load option data suppresses colored output and
+ * clearing of the screen.
  */
 
 #include <common.h>
@@ -25,6 +28,7 @@ static const efi_guid_t guid_simple_file_system_protocol =
 					EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
 static const efi_guid_t load_file2_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
 static efi_handle_t handle;
+static bool nocolor;
 
 /*
  * Device path defined by Linux to identify the handle providing the
@@ -46,6 +50,17 @@ static const struct efi_initrd_dp initrd_dp = {
 	}
 };
 
+/**
+ * color() - set foreground color
+ *
+ * @color:	foreground color
+ */
+static void color(u8 color)
+{
+	if (!nocolor)
+		cout->set_attribute(cout, color | EFI_BACKGROUND_BLACK);
+}
+
 /**
  * print() - print string
  *
@@ -56,6 +71,17 @@ static void print(u16 *string)
 	cout->output_string(cout, string);
 }
 
+/**
+ * cls() - clear screen
+ */
+static void cls(void)
+{
+	if (nocolor)
+		print(u"\r\n");
+	else
+		cout->clear_screen(cout);
+}
+
 /**
  * error() - print error string
  *
@@ -63,9 +89,9 @@ static void print(u16 *string)
  */
 static void error(u16 *string)
 {
-	cout->set_attribute(cout, EFI_LIGHTRED | EFI_BACKGROUND_BLACK);
+	color(EFI_LIGHTRED);
 	print(string);
-	cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
+	color(EFI_LIGHTBLUE);
 }
 
 /*
@@ -215,10 +241,13 @@ static u16 *skip_whitespace(u16 *pos)
  *
  * @string:	string to search for keyword
  * @keyword:	keyword to be searched
- * Return:	true fi @string starts with the keyword
+ * Return:	true if @string starts with the keyword
  */
 static bool starts_with(u16 *string, u16 *keyword)
 {
+	if (!string || !keyword)
+		return false;
+
 	for (; *keyword; ++string, ++keyword) {
 		if (*string != *keyword)
 			return false;
@@ -400,6 +429,30 @@ out:
 	return ret;
 }
 
+/**
+ * get_load_options() - get load options
+ *
+ * Return:	load options or NULL
+ */
+u16 *get_load_options(void)
+{
+	efi_status_t ret;
+	struct efi_loaded_image *loaded_image;
+
+	ret = bs->open_protocol(handle, &loaded_image_guid,
+				(void **)&loaded_image, NULL, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret != EFI_SUCCESS) {
+		error(u"Loaded image protocol not found\r\n");
+		return NULL;
+	}
+
+	if (!loaded_image->load_options_size || !loaded_image->load_options)
+		return NULL;
+
+	return loaded_image->load_options;
+}
+
 /**
  * efi_main() - entry point of the EFI application.
  *
@@ -410,18 +463,23 @@ out:
 efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
 			     struct efi_system_table *systab)
 {
+	u16 *load_options;
+
 	handle = image_handle;
 	systable = systab;
 	cerr = systable->std_err;
 	cout = systable->con_out;
 	cin = systable->con_in;
 	bs = systable->boottime;
+	load_options = get_load_options();
 
-	cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
-	cout->clear_screen(cout);
-	cout->set_attribute(cout, EFI_WHITE | EFI_BACKGROUND_BLACK);
+	if (starts_with(load_options, u"nocolor"))
+		nocolor = true;
+
+	color(EFI_WHITE);
+	cls();
 	print(u"INITRD Dump\r\n===========\r\n\r\n");
-	cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
+	color(EFI_LIGHTBLUE);
 
 	for (;;) {
 		u16 command[BUFFER_SIZE];
@@ -443,7 +501,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
 			do_help();
 	}
 
-	cout->set_attribute(cout, EFI_LIGHTGRAY | EFI_BACKGROUND_BLACK);
-	cout->clear_screen(cout);
+	color(EFI_LIGHTGRAY);
+	cls();
+
 	return EFI_SUCCESS;
 }
-- 
2.34.1

[PATCH v3 2/3] efi_loader: initrddump: drain input before prompt

[Heinrich Schuchardt]

Up to now the initrddump.efi application has drained the input after
showing the prompt. This works for humans but leads to problems when
automating testing. If the input is drained, this should be done before
showing the prompt.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	new patch
---
 lib/efi_loader/initrddump.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/initrddump.c b/lib/efi_loader/initrddump.c
index 25dbd635e5..4dd40eb0d3 100644
--- a/lib/efi_loader/initrddump.c
+++ b/lib/efi_loader/initrddump.c
@@ -120,6 +120,14 @@ static void printx(u64 val, u32 prec)
 	print(buf);
 }
 
+/**
+ * efi_drain_input() - drain console input
+ */
+static void efi_drain_input(void)
+{
+	cin->reset(cin, true);
+}
+
 /**
  * efi_input_yn() - get answer to yes/no question
  *
@@ -137,8 +145,6 @@ static efi_status_t efi_input_yn(void)
 	efi_uintn_t index;
 	efi_status_t ret;
 
-	/* Drain the console input */
-	ret = cin->reset(cin, true);
 	for (;;) {
 		ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
 		if (ret != EFI_SUCCESS)
@@ -179,8 +185,6 @@ static efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
 	u16 outbuf[2] = u" ";
 	efi_status_t ret;
 
-	/* Drain the console input */
-	ret = cin->reset(cin, true);
 	*buffer = 0;
 	for (;;) {
 		ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
@@ -393,6 +397,7 @@ static efi_status_t do_save(u16 *filename)
 	ret = root->open(root, &file, filename, EFI_FILE_MODE_READ, 0);
 	if (ret == EFI_SUCCESS) {
 		file->close(file);
+		efi_drain_input();
 		print(u"Overwrite existing file (y/n)? ");
 		ret = efi_input_yn();
 		print(u"\r\n");
@@ -486,6 +491,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
 		u16 *pos;
 		efi_uintn_t ret;
 
+		efi_drain_input();
 		print(u"=> ");
 		ret = efi_input(command, sizeof(command));
 		if (ret == EFI_ABORTED)
-- 
2.34.1

[PATCH v3 3/3] test: test UEFI boot manager

[Heinrich Schuchardt]

Provide a unit test for

* efidebug boot add
* efidebug bootmgr
* initrd via EFI_LOAD_FILE2_PROTOCOL

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	pass u"nocolor" as optional data to initrddump.efi
v2:
	new patch
---
 test/py/tests/test_efi_bootmgr/conftest.py    | 42 +++++++++++++++++++
 .../test_efi_bootmgr/test_efi_bootmgr.py      | 31 ++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 test/py/tests/test_efi_bootmgr/conftest.py
 create mode 100644 test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py

diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
new file mode 100644
index 0000000000..69008fddce
--- /dev/null
+++ b/test/py/tests/test_efi_bootmgr/conftest.py
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier:      GPL-2.0+
+
+"""Fixture for UEFI bootmanager test
+"""
+
+import os
+import pytest
+import shutil
+from subprocess import call, check_call
+
+@pytest.fixture(scope='session')
+def efi_bootmgr_data(u_boot_config):
+    """Set up a file system to be used in UEFI bootmanager
+       tests
+
+    Args:
+        u_boot_config: U-boot configuration.
+
+    Return:
+        A path to disk image to be used for testing
+    """
+    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
+    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr.img'
+
+    shutil.rmtree(mnt_point, ignore_errors=True)
+    os.mkdir(mnt_point, mode = 0o755)
+
+    with open(mnt_point + '/initrd-1.img', 'w', encoding = 'ascii') as file:
+        file.write("initrd 1")
+
+    with open(mnt_point + '/initrd-2.img', 'w', encoding = 'ascii') as file:
+        file.write("initrd 2")
+
+    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/initrddump.efi',
+                    mnt_point + '/initrddump.efi')
+
+    check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat {} {}'
+               .format(mnt_point, image_path), shell=True)
+
+    print(image_path)
+
+    yield image_path
diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
new file mode 100644
index 0000000000..feba306580
--- /dev/null
+++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier:      GPL-2.0+
+
+import pytest
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('efi_loader')
+def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
+    u_boot_console.run_command(cmd = 'host bind 0 {}'.format(efi_bootmgr_data))
+
+    u_boot_console.run_command(cmd = 'efidebug boot add ' \
+        '-b 0001 label-1 host 0:1 initrddump.efi ' \
+        '-i host 0:1 initrd-1.img -s nocolor')
+    u_boot_console.run_command(cmd = 'efidebug boot dump')
+    u_boot_console.run_command(cmd = 'efidebug boot order 0001')
+    u_boot_console.run_command(cmd = 'bootefi bootmgr')
+    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
+    assert 'crc32: 0x181464af' in response
+    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
+
+    u_boot_console.run_command(cmd = 'efidebug boot add ' \
+        '-B 0002 label-2 host 0:1 initrddump.efi ' \
+        '-I host 0:1 initrd-2.img -s nocolor')
+    u_boot_console.run_command(cmd = 'efidebug boot dump')
+    u_boot_console.run_command(cmd = 'efidebug boot order 0002')
+    u_boot_console.run_command(cmd = 'bootefi bootmgr')
+    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
+    assert 'crc32: 0x811d3515' in response
+    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
+
+    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
+    u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
-- 
2.34.1

Re: [PATCH v3 3/3] test: test UEFI boot manager

[AKASHI Takahiro]

On Tue, Mar 22, 2022 at 09:58:07PM +0100, Heinrich Schuchardt wrote:
> Provide a unit test for
> 
> * efidebug boot add

and other boot sub-commands as well?
See below.

> * efidebug bootmgr

-> bootefi bootmgr

> * initrd via EFI_LOAD_FILE2_PROTOCOL
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	pass u"nocolor" as optional data to initrddump.efi
> v2:
> 	new patch
> ---
>  test/py/tests/test_efi_bootmgr/conftest.py    | 42 +++++++++++++++++++
>  .../test_efi_bootmgr/test_efi_bootmgr.py      | 31 ++++++++++++++
>  2 files changed, 73 insertions(+)
>  create mode 100644 test/py/tests/test_efi_bootmgr/conftest.py
>  create mode 100644 test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> 
> diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
> new file mode 100644
> index 0000000000..69008fddce
> --- /dev/null
> +++ b/test/py/tests/test_efi_bootmgr/conftest.py
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier:      GPL-2.0+
> +
> +"""Fixture for UEFI bootmanager test
> +"""
> +
> +import os
> +import pytest
> +import shutil
> +from subprocess import call, check_call
> +
> +@pytest.fixture(scope='session')
> +def efi_bootmgr_data(u_boot_config):
> +    """Set up a file system to be used in UEFI bootmanager
> +       tests
> +
> +    Args:
> +        u_boot_config: U-boot configuration.
> +
> +    Return:
> +        A path to disk image to be used for testing
> +    """
> +    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
> +    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr.img'
> +
> +    shutil.rmtree(mnt_point, ignore_errors=True)
> +    os.mkdir(mnt_point, mode = 0o755)
> +
> +    with open(mnt_point + '/initrd-1.img', 'w', encoding = 'ascii') as file:
> +        file.write("initrd 1")
> +
> +    with open(mnt_point + '/initrd-2.img', 'w', encoding = 'ascii') as file:
> +        file.write("initrd 2")
> +
> +    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/initrddump.efi',
> +                    mnt_point + '/initrddump.efi')
> +
> +    check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat {} {}'
> +               .format(mnt_point, image_path), shell=True)
> +
> +    print(image_path)
> +
> +    yield image_path
> diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> new file mode 100644
> index 0000000000..feba306580
> --- /dev/null
> +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier:      GPL-2.0+
> +
> +import pytest
> +
> +@pytest.mark.boardspec('sandbox')
> +@pytest.mark.buildconfigspec('efi_loader')

Obviously, this test depends on 'cmd_efidebug' and 'cmd_bootefi'.

> +def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
> +    u_boot_console.run_command(cmd = 'host bind 0 {}'.format(efi_bootmgr_data))
> +
> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
> +        '-b 0001 label-1 host 0:1 initrddump.efi ' \
> +        '-i host 0:1 initrd-1.img -s nocolor')
> +    u_boot_console.run_command(cmd = 'efidebug boot dump')

Why do you call "boot dump" here?
If you intend to test this sub-command, you should check the output.
Or do you think it's enough to simply exercise the code for testing?

> +    u_boot_console.run_command(cmd = 'efidebug boot order 0001')
> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
> +    assert 'crc32: 0x181464af' in response
> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
> +
> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
> +        '-B 0002 label-2 host 0:1 initrddump.efi ' \
> +        '-I host 0:1 initrd-2.img -s nocolor')
> +    u_boot_console.run_command(cmd = 'efidebug boot dump')
> +    u_boot_console.run_command(cmd = 'efidebug boot order 0002')
> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
> +    assert 'crc32: 0x811d3515' in response
> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)

If you like, please add a test for "boot next" (or BootNext variable)
as well.

Thanks,
-Takahiro Akashi

> +
> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
> -- 
> 2.34.1
> 

Re: [PATCH v3 1/3] efi_loader: nocolor command line attr for initrddump.efi

[Ilias Apalodimas]

Hi Heinrich,

On Tue, 22 Mar 2022 at 22:58, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> initrddump.efi uses colored output and clear the screen. This is not
> helpful for integration into Python tests. Allow specifying 'nocolor' in
> the load option data to suppress color output and clearing the screen.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
>         new patch
> ---
>  lib/efi_loader/initrddump.c | 77 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/lib/efi_loader/initrddump.c b/lib/efi_loader/initrddump.c
> index 7de43bcfff..25dbd635e5 100644
> --- a/lib/efi_loader/initrddump.c
> +++ b/lib/efi_loader/initrddump.c
> @@ -4,6 +4,9 @@
>   *
>   * initrddump.efi saves the initial RAM disk provided via the
>   * EFI_LOAD_FILE2_PROTOCOL.
> + *
> + * Specifying 'nocolor' as load option data suppresses colored output and
> + * clearing of the screen.
>   */
>
>  #include <common.h>
> @@ -25,6 +28,7 @@ static const efi_guid_t guid_simple_file_system_protocol =
>                                         EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
>  static const efi_guid_t load_file2_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>  static efi_handle_t handle;
> +static bool nocolor;
>
>  /*
>   * Device path defined by Linux to identify the handle providing the
> @@ -46,6 +50,17 @@ static const struct efi_initrd_dp initrd_dp = {
>         }
>  };
>
> +/**
> + * color() - set foreground color
> + *
> + * @color:     foreground color
> + */
> +static void color(u8 color)
> +{
> +       if (!nocolor)
> +               cout->set_attribute(cout, color | EFI_BACKGROUND_BLACK);
> +}
> +
>  /**
>   * print() - print string
>   *
> @@ -56,6 +71,17 @@ static void print(u16 *string)
>         cout->output_string(cout, string);
>  }
>
> +/**
> + * cls() - clear screen
> + */
> +static void cls(void)
> +{
> +       if (nocolor)
> +               print(u"\r\n");
> +       else
> +               cout->clear_screen(cout);
> +}
> +
>  /**
>   * error() - print error string
>   *
> @@ -63,9 +89,9 @@ static void print(u16 *string)
>   */
>  static void error(u16 *string)
>  {
> -       cout->set_attribute(cout, EFI_LIGHTRED | EFI_BACKGROUND_BLACK);
> +       color(EFI_LIGHTRED);
>         print(string);
> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
> +       color(EFI_LIGHTBLUE);
>  }
>
>  /*
> @@ -215,10 +241,13 @@ static u16 *skip_whitespace(u16 *pos)
>   *
>   * @string:    string to search for keyword
>   * @keyword:   keyword to be searched
> - * Return:     true fi @string starts with the keyword
> + * Return:     true if @string starts with the keyword
>   */
>  static bool starts_with(u16 *string, u16 *keyword)
>  {
> +       if (!string || !keyword)
> +               return false;
> +
>         for (; *keyword; ++string, ++keyword) {
>                 if (*string != *keyword)
>                         return false;
> @@ -400,6 +429,30 @@ out:
>         return ret;
>  }
>
> +/**
> + * get_load_options() - get load options
> + *
> + * Return:     load options or NULL
> + */
> +u16 *get_load_options(void)
> +{
> +       efi_status_t ret;
> +       struct efi_loaded_image *loaded_image;
> +
> +       ret = bs->open_protocol(handle, &loaded_image_guid,
> +                               (void **)&loaded_image, NULL, NULL,
> +                               EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +       if (ret != EFI_SUCCESS) {
> +               error(u"Loaded image protocol not found\r\n");
> +               return NULL;
> +       }
> +
> +       if (!loaded_image->load_options_size || !loaded_image->load_options)
> +               return NULL;
> +
> +       return loaded_image->load_options;
> +}
> +
>  /**
>   * efi_main() - entry point of the EFI application.
>   *
> @@ -410,18 +463,23 @@ out:
>  efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>                              struct efi_system_table *systab)
>  {
> +       u16 *load_options;
> +
>         handle = image_handle;
>         systable = systab;
>         cerr = systable->std_err;
>         cout = systable->con_out;
>         cin = systable->con_in;
>         bs = systable->boottime;
> +       load_options = get_load_options();

Don't we need to check for NULL here?

>
> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
> -       cout->clear_screen(cout);
> -       cout->set_attribute(cout, EFI_WHITE | EFI_BACKGROUND_BLACK);
> +       if (starts_with(load_options, u"nocolor"))
> +               nocolor = true;
> +
> +       color(EFI_WHITE);
> +       cls();
>         print(u"INITRD Dump\r\n===========\r\n\r\n");
> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
> +       color(EFI_LIGHTBLUE);
>
>         for (;;) {
>                 u16 command[BUFFER_SIZE];
> @@ -443,7 +501,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>                         do_help();
>         }
>
> -       cout->set_attribute(cout, EFI_LIGHTGRAY | EFI_BACKGROUND_BLACK);
> -       cout->clear_screen(cout);
> +       color(EFI_LIGHTGRAY);
> +       cls();
> +
>         return EFI_SUCCESS;
>  }
> --
> 2.34.1
>

Cheers
/Ilias

Re: [PATCH v3 2/3] efi_loader: initrddump: drain input before prompt

[Ilias Apalodimas]

On Tue, 22 Mar 2022 at 22:58, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Up to now the initrddump.efi application has drained the input after
> showing the prompt. This works for humans but leads to problems when
> automating testing. If the input is drained, this should be done before
> showing the prompt.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
>         new patch
> ---
>  lib/efi_loader/initrddump.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/initrddump.c b/lib/efi_loader/initrddump.c
> index 25dbd635e5..4dd40eb0d3 100644
> --- a/lib/efi_loader/initrddump.c
> +++ b/lib/efi_loader/initrddump.c
> @@ -120,6 +120,14 @@ static void printx(u64 val, u32 prec)
>         print(buf);
>  }
>
> +/**
> + * efi_drain_input() - drain console input
> + */
> +static void efi_drain_input(void)
> +{
> +       cin->reset(cin, true);
> +}
> +
>  /**
>   * efi_input_yn() - get answer to yes/no question
>   *
> @@ -137,8 +145,6 @@ static efi_status_t efi_input_yn(void)
>         efi_uintn_t index;
>         efi_status_t ret;
>
> -       /* Drain the console input */
> -       ret = cin->reset(cin, true);
>         for (;;) {
>                 ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
>                 if (ret != EFI_SUCCESS)
> @@ -179,8 +185,6 @@ static efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
>         u16 outbuf[2] = u" ";
>         efi_status_t ret;
>
> -       /* Drain the console input */
> -       ret = cin->reset(cin, true);
>         *buffer = 0;
>         for (;;) {
>                 ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
> @@ -393,6 +397,7 @@ static efi_status_t do_save(u16 *filename)
>         ret = root->open(root, &file, filename, EFI_FILE_MODE_READ, 0);
>         if (ret == EFI_SUCCESS) {
>                 file->close(file);
> +               efi_drain_input();
>                 print(u"Overwrite existing file (y/n)? ");
>                 ret = efi_input_yn();
>                 print(u"\r\n");
> @@ -486,6 +491,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>                 u16 *pos;
>                 efi_uintn_t ret;
>
> +               efi_drain_input();
>                 print(u"=> ");
>                 ret = efi_input(command, sizeof(command));
>                 if (ret == EFI_ABORTED)
> --
> 2.34.1
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v3 1/3] efi_loader: nocolor command line attr for initrddump.efi

[Heinrich Schuchardt]

On 3/25/22 15:58, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Tue, 22 Mar 2022 at 22:58, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> initrddump.efi uses colored output and clear the screen. This is not
>> helpful for integration into Python tests. Allow specifying 'nocolor' in
>> the load option data to suppress color output and clearing the screen.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v3:
>>          new patch
>> ---
>>   lib/efi_loader/initrddump.c | 77 ++++++++++++++++++++++++++++++++-----
>>   1 file changed, 68 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/efi_loader/initrddump.c b/lib/efi_loader/initrddump.c
>> index 7de43bcfff..25dbd635e5 100644
>> --- a/lib/efi_loader/initrddump.c
>> +++ b/lib/efi_loader/initrddump.c
>> @@ -4,6 +4,9 @@
>>    *
>>    * initrddump.efi saves the initial RAM disk provided via the
>>    * EFI_LOAD_FILE2_PROTOCOL.
>> + *
>> + * Specifying 'nocolor' as load option data suppresses colored output and
>> + * clearing of the screen.
>>    */
>>
>>   #include <common.h>
>> @@ -25,6 +28,7 @@ static const efi_guid_t guid_simple_file_system_protocol =
>>                                          EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
>>   static const efi_guid_t load_file2_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>>   static efi_handle_t handle;
>> +static bool nocolor;
>>
>>   /*
>>    * Device path defined by Linux to identify the handle providing the
>> @@ -46,6 +50,17 @@ static const struct efi_initrd_dp initrd_dp = {
>>          }
>>   };
>>
>> +/**
>> + * color() - set foreground color
>> + *
>> + * @color:     foreground color
>> + */
>> +static void color(u8 color)
>> +{
>> +       if (!nocolor)
>> +               cout->set_attribute(cout, color | EFI_BACKGROUND_BLACK);
>> +}
>> +
>>   /**
>>    * print() - print string
>>    *
>> @@ -56,6 +71,17 @@ static void print(u16 *string)
>>          cout->output_string(cout, string);
>>   }
>>
>> +/**
>> + * cls() - clear screen
>> + */
>> +static void cls(void)
>> +{
>> +       if (nocolor)
>> +               print(u"\r\n");
>> +       else
>> +               cout->clear_screen(cout);
>> +}
>> +
>>   /**
>>    * error() - print error string
>>    *
>> @@ -63,9 +89,9 @@ static void print(u16 *string)
>>    */
>>   static void error(u16 *string)
>>   {
>> -       cout->set_attribute(cout, EFI_LIGHTRED | EFI_BACKGROUND_BLACK);
>> +       color(EFI_LIGHTRED);
>>          print(string);
>> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
>> +       color(EFI_LIGHTBLUE);
>>   }
>>
>>   /*
>> @@ -215,10 +241,13 @@ static u16 *skip_whitespace(u16 *pos)
>>    *
>>    * @string:    string to search for keyword
>>    * @keyword:   keyword to be searched
>> - * Return:     true fi @string starts with the keyword
>> + * Return:     true if @string starts with the keyword
>>    */
>>   static bool starts_with(u16 *string, u16 *keyword)
>>   {
>> +       if (!string || !keyword)
>> +               return false;
>> +
>>          for (; *keyword; ++string, ++keyword) {
>>                  if (*string != *keyword)
>>                          return false;
>> @@ -400,6 +429,30 @@ out:
>>          return ret;
>>   }
>>
>> +/**
>> + * get_load_options() - get load options
>> + *
>> + * Return:     load options or NULL
>> + */
>> +u16 *get_load_options(void)
>> +{
>> +       efi_status_t ret;
>> +       struct efi_loaded_image *loaded_image;
>> +
>> +       ret = bs->open_protocol(handle, &loaded_image_guid,
>> +                               (void **)&loaded_image, NULL, NULL,
>> +                               EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> +       if (ret != EFI_SUCCESS) {
>> +               error(u"Loaded image protocol not found\r\n");
>> +               return NULL;
>> +       }
>> +
>> +       if (!loaded_image->load_options_size || !loaded_image->load_options)
>> +               return NULL;
>> +
>> +       return loaded_image->load_options;
>> +}
>> +
>>   /**
>>    * efi_main() - entry point of the EFI application.
>>    *
>> @@ -410,18 +463,23 @@ out:
>>   efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>>                               struct efi_system_table *systab)
>>   {
>> +       u16 *load_options;
>> +
>>          handle = image_handle;
>>          systable = systab;
>>          cerr = systable->std_err;
>>          cout = systable->con_out;
>>          cin = systable->con_in;
>>          bs = systable->boottime;
>> +       load_options = get_load_options();
> 
> Don't we need to check for NULL here?

Thanks for reviewing.

The check is in starts_with().

Best regards

Heinrich

> 
>>
>> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
>> -       cout->clear_screen(cout);
>> -       cout->set_attribute(cout, EFI_WHITE | EFI_BACKGROUND_BLACK);
>> +       if (starts_with(load_options, u"nocolor"))
>> +               nocolor = true;
>> +
>> +       color(EFI_WHITE);
>> +       cls();
>>          print(u"INITRD Dump\r\n===========\r\n\r\n");
>> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
>> +       color(EFI_LIGHTBLUE);
>>
>>          for (;;) {
>>                  u16 command[BUFFER_SIZE];
>> @@ -443,7 +501,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>>                          do_help();
>>          }
>>
>> -       cout->set_attribute(cout, EFI_LIGHTGRAY | EFI_BACKGROUND_BLACK);
>> -       cout->clear_screen(cout);
>> +       color(EFI_LIGHTGRAY);
>> +       cls();
>> +
>>          return EFI_SUCCESS;
>>   }
>> --
>> 2.34.1
>>
> 
> Cheers
> /Ilias

Re: [PATCH v3 3/3] test: test UEFI boot manager

[Heinrich Schuchardt]

On 3/23/22 07:32, AKASHI Takahiro wrote:
> On Tue, Mar 22, 2022 at 09:58:07PM +0100, Heinrich Schuchardt wrote:
>> Provide a unit test for
>>
>> * efidebug boot add
> 
> and other boot sub-commands as well?
> See below.

Thanks for reviewing

efidebug boot order - yes

Properly testing the other sub-commands will require additional work.

> 
>> * efidebug bootmgr
> 
> -> bootefi bootmgr

yes

Best regards

Heinrich

> 
>> * initrd via EFI_LOAD_FILE2_PROTOCOL
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v3:
>> 	pass u"nocolor" as optional data to initrddump.efi
>> v2:
>> 	new patch
>> ---
>>   test/py/tests/test_efi_bootmgr/conftest.py    | 42 +++++++++++++++++++
>>   .../test_efi_bootmgr/test_efi_bootmgr.py      | 31 ++++++++++++++
>>   2 files changed, 73 insertions(+)
>>   create mode 100644 test/py/tests/test_efi_bootmgr/conftest.py
>>   create mode 100644 test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>>
>> diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
>> new file mode 100644
>> index 0000000000..69008fddce
>> --- /dev/null
>> +++ b/test/py/tests/test_efi_bootmgr/conftest.py
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier:      GPL-2.0+
>> +
>> +"""Fixture for UEFI bootmanager test
>> +"""
>> +
>> +import os
>> +import pytest
>> +import shutil
>> +from subprocess import call, check_call
>> +
>> +@pytest.fixture(scope='session')
>> +def efi_bootmgr_data(u_boot_config):
>> +    """Set up a file system to be used in UEFI bootmanager
>> +       tests
>> +
>> +    Args:
>> +        u_boot_config: U-boot configuration.
>> +
>> +    Return:
>> +        A path to disk image to be used for testing
>> +    """
>> +    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
>> +    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr.img'
>> +
>> +    shutil.rmtree(mnt_point, ignore_errors=True)
>> +    os.mkdir(mnt_point, mode = 0o755)
>> +
>> +    with open(mnt_point + '/initrd-1.img', 'w', encoding = 'ascii') as file:
>> +        file.write("initrd 1")
>> +
>> +    with open(mnt_point + '/initrd-2.img', 'w', encoding = 'ascii') as file:
>> +        file.write("initrd 2")
>> +
>> +    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/initrddump.efi',
>> +                    mnt_point + '/initrddump.efi')
>> +
>> +    check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat {} {}'
>> +               .format(mnt_point, image_path), shell=True)
>> +
>> +    print(image_path)
>> +
>> +    yield image_path
>> diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>> new file mode 100644
>> index 0000000000..feba306580
>> --- /dev/null
>> +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>> @@ -0,0 +1,31 @@
>> +# SPDX-License-Identifier:      GPL-2.0+
>> +
>> +import pytest
>> +
>> +@pytest.mark.boardspec('sandbox')
>> +@pytest.mark.buildconfigspec('efi_loader')
> 
> Obviously, this test depends on 'cmd_efidebug' and 'cmd_bootefi'.
> 
>> +def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
>> +    u_boot_console.run_command(cmd = 'host bind 0 {}'.format(efi_bootmgr_data))
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
>> +        '-b 0001 label-1 host 0:1 initrddump.efi ' \
>> +        '-i host 0:1 initrd-1.img -s nocolor')
>> +    u_boot_console.run_command(cmd = 'efidebug boot dump')
> 
> Why do you call "boot dump" here?
> If you intend to test this sub-command, you should check the output.
> Or do you think it's enough to simply exercise the code for testing?
> 
>> +    u_boot_console.run_command(cmd = 'efidebug boot order 0001')
>> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
>> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
>> +    assert 'crc32: 0x181464af' in response
>> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
>> +        '-B 0002 label-2 host 0:1 initrddump.efi ' \
>> +        '-I host 0:1 initrd-2.img -s nocolor')
>> +    u_boot_console.run_command(cmd = 'efidebug boot dump')
>> +    u_boot_console.run_command(cmd = 'efidebug boot order 0002')
>> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
>> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
>> +    assert 'crc32: 0x811d3515' in response
>> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
> 
> If you like, please add a test for "boot next" (or BootNext variable)
> as well.
> 
> Thanks,
> -Takahiro Akashi
> 
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
>> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
>> -- 
>> 2.34.1
>>

Re: [PATCH v3 3/3] test: test UEFI boot manager

[Heinrich Schuchardt]

On 3/23/22 07:32, AKASHI Takahiro wrote:
> On Tue, Mar 22, 2022 at 09:58:07PM +0100, Heinrich Schuchardt wrote:
>> Provide a unit test for
>>
>> * efidebug boot add
> 
> and other boot sub-commands as well?
> See below.
> 
>> * efidebug bootmgr
> 
> -> bootefi bootmgr

ok

> 
>> * initrd via EFI_LOAD_FILE2_PROTOCOL
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v3:
>> 	pass u"nocolor" as optional data to initrddump.efi
>> v2:
>> 	new patch
>> ---
>>   test/py/tests/test_efi_bootmgr/conftest.py    | 42 +++++++++++++++++++
>>   .../test_efi_bootmgr/test_efi_bootmgr.py      | 31 ++++++++++++++
>>   2 files changed, 73 insertions(+)
>>   create mode 100644 test/py/tests/test_efi_bootmgr/conftest.py
>>   create mode 100644 test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>>
>> diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
>> new file mode 100644
>> index 0000000000..69008fddce
>> --- /dev/null
>> +++ b/test/py/tests/test_efi_bootmgr/conftest.py
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier:      GPL-2.0+
>> +
>> +"""Fixture for UEFI bootmanager test
>> +"""
>> +
>> +import os
>> +import pytest
>> +import shutil
>> +from subprocess import call, check_call
>> +
>> +@pytest.fixture(scope='session')
>> +def efi_bootmgr_data(u_boot_config):
>> +    """Set up a file system to be used in UEFI bootmanager
>> +       tests
>> +
>> +    Args:
>> +        u_boot_config: U-boot configuration.
>> +
>> +    Return:
>> +        A path to disk image to be used for testing
>> +    """
>> +    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
>> +    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr.img'
>> +
>> +    shutil.rmtree(mnt_point, ignore_errors=True)
>> +    os.mkdir(mnt_point, mode = 0o755)
>> +
>> +    with open(mnt_point + '/initrd-1.img', 'w', encoding = 'ascii') as file:
>> +        file.write("initrd 1")
>> +
>> +    with open(mnt_point + '/initrd-2.img', 'w', encoding = 'ascii') as file:
>> +        file.write("initrd 2")
>> +
>> +    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/initrddump.efi',
>> +                    mnt_point + '/initrddump.efi')
>> +
>> +    check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat {} {}'
>> +               .format(mnt_point, image_path), shell=True)
>> +
>> +    print(image_path)
>> +
>> +    yield image_path
>> diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>> new file mode 100644
>> index 0000000000..feba306580
>> --- /dev/null
>> +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
>> @@ -0,0 +1,31 @@
>> +# SPDX-License-Identifier:      GPL-2.0+
>> +
>> +import pytest
>> +
>> +@pytest.mark.boardspec('sandbox')
>> +@pytest.mark.buildconfigspec('efi_loader')
> 
> Obviously, this test depends on 'cmd_efidebug' and 'cmd_bootefi'.

yes these should be tested:

@pytest.mark.buildconfigspec('cmd_efidebug')
@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')

> 
>> +def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
>> +    u_boot_console.run_command(cmd = 'host bind 0 {}'.format(efi_bootmgr_data))
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
>> +        '-b 0001 label-1 host 0:1 initrddump.efi ' \
>> +        '-i host 0:1 initrd-1.img -s nocolor')
>> +    u_boot_console.run_command(cmd = 'efidebug boot dump')
> 
> Why do you call "boot dump" here?
> If you intend to test this sub-command, you should check the output.
> Or do you think it's enough to simply exercise the code for testing?

When the test breaks I want to see the output.

> 
>> +    u_boot_console.run_command(cmd = 'efidebug boot order 0001')
>> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
>> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
>> +    assert 'crc32: 0x181464af' in response
>> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
>> +        '-B 0002 label-2 host 0:1 initrddump.efi ' \
>> +        '-I host 0:1 initrd-2.img -s nocolor')
>> +    u_boot_console.run_command(cmd = 'efidebug boot dump')
>> +    u_boot_console.run_command(cmd = 'efidebug boot order 0002')
>> +    u_boot_console.run_command(cmd = 'bootefi bootmgr')
>> +    response = u_boot_console.run_command(cmd = 'load', wait_for_echo=False)
>> +    assert 'crc32: 0x811d3515' in response
>> +    u_boot_console.run_command(cmd = 'exit', wait_for_echo=False)
> 
> If you like, please add a test for "boot next" (or BootNext variable)
> as well.

Thanks for reviewing.

This is a patch is a starting point. We can and should expand what we 
are testing in a future patch.

efidebug boot next is tested to some degree in
test/py/tests/test_efi_secboot/test_signed.py

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> +
>> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
>> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
>> -- 
>> 2.34.1
>>