[PATCH v4 0/2] EFI: Reset system after capsule-on-disk
Hi,
Here is the 4th version of the reset after capsule-on-disk. This version updates the patch description and use do_reset() and halt or reset_cpu() if available, according to Takahiro and Sughosh's comment.
The reset after completing the capsule-on-disk is stated in the UEFI specification 2.9, section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as below,
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.
Thank you,
---
Masami Hiramatsu (2): efi_loader: use efi_update_capsule_firmware() for capsule on disk efi_loader: Reset system after CapsuleUpdate on disk
lib/efi_loader/efi_capsule.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
-- Masami Hiramatsu masami.hiramatsu@linaro.org
Since the efi_update_capsule() represents the UpdateCapsule() runtime service, it has to handle the capsule flags and update ESRT. However the capsule-on-disk doesn't need to care about such things.
Thus, the capsule-on-disk should use the efi_capsule_update_firmware() directly instead of calling efi_update_capsule().
This means the roles of the efi_update_capsule() and capsule-on-disk are different. We have to keep the efi_update_capsule() for providing runtime service API at boot time.
Suggested-by: AKASHI Takahiro takahiro.akashi@linaro.org Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- Changes in v4: - Update patch description. --- lib/efi_loader/efi_capsule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 4463ae00fd..1ec7ea29ff 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) index = 0; ret = efi_capsule_read_file(files[i], &capsule); if (ret == EFI_SUCCESS) { - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); + ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS) log_err("Applying capsule %ls failed\n", files[i]);
On 2/3/22 10:23, Masami Hiramatsu wrote:
Since the efi_update_capsule() represents the UpdateCapsule() runtime service, it has to handle the capsule flags and update ESRT. However the capsule-on-disk doesn't need to care about such things.
Thus, the capsule-on-disk should use the efi_capsule_update_firmware() directly instead of calling efi_update_capsule().
This means the roles of the efi_update_capsule() and capsule-on-disk are different. We have to keep the efi_update_capsule() for providing runtime service API at boot time.
Suggested-by: AKASHI Takahiro takahiro.akashi@linaro.org Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- Changes in v4: - Do not use sysreset because that is a warm reset. - Fix patch description. --- lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS) - log_err("Applying capsule %ls failed\n", + log_err("Applying capsule %ls failed.\n", files[i]); + else + log_info("Applying capsule %ls succeeded.\n", + files[i]);
/* create CapsuleXXXX */ set_capsule_result(index, capsule, ret); @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- return ret; + /* + * UEFI spec requires to reset system after complete processing capsule + * update on the storage. + */ + log_info("Reboot after firmware update"); + /* Cold reset is required for loading the new firmware. */ + do_reset(NULL, 0, 0, NULL); + hang(); + /* not reach here */ + + return 0; } #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
On Thu, Feb 03, 2022 at 06:23:27PM +0900, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.
Once this behavior is enforced on U-Boot, CONFIG_EFI_CAPSULE_ON_DISK_EARLY will make little sense. This option will have to always be set. Otherwise, a user will see a sudden system reboot when he or she types any of efi commands, like "env print -e".
-Takahiro Akashi
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- return ret;
- /*
* UEFI spec requires to reset system after complete processing capsule* update on the storage.*/- log_info("Reboot after firmware update");
- /* Cold reset is required for loading the new firmware. */
- do_reset(NULL, 0, 0, NULL);
- hang();
- /* not reach here */
- return 0;
} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Hi Takahiro,
2022年2月9日(水) 12:13 AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Feb 03, 2022 at 06:23:27PM +0900, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.Once this behavior is enforced on U-Boot, CONFIG_EFI_CAPSULE_ON_DISK_EARLY will make little sense. This option will have to always be set.
Agreed. This option is recommended. I hope U-Boot scans the devices before running this early capsule-on-disk so that it can find the appropriate storage for ESP, but anyway it works.
Otherwise, a user will see a sudden system reboot when he or she types any of efi commands, like "env print -e".
Yes if there is a capsule to be updated, and they will see the new u-boot coming back soon. I guess the reason why the capsule-on-disk runs at first, is to avoid inconsistent status for users, or we can postpone the capsule update until running "efidebug capsule disk" command. If that is correct, shouldn't we avoid showing the firmware "to be updated" too?
Thank you,
-Takahiro Akashi
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
return ret;
/** UEFI spec requires to reset system after complete processing capsule* update on the storage.*/log_info("Reboot after firmware update");/* Cold reset is required for loading the new firmware. */do_reset(NULL, 0, 0, NULL);hang();/* not reach here */return 0;} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
On 2/3/22 10:23, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
On 2/13/22 10:01, Heinrich Schuchardt wrote:
On 2/3/22 10:23, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Gitlab CI tests fail. Please, resubmit with the Python tests adjusted. Make sure that 'make tests' does not fail.
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345
FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4
Best regards
Heinrich
Best regards
Heinrich
On Sun, Feb 13, 2022 at 11:17:37AM +0100, Heinrich Schuchardt wrote:
On 2/13/22 10:01, Heinrich Schuchardt wrote:
On 2/3/22 10:23, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Gitlab CI tests fail. Please, resubmit with the Python tests adjusted. Make sure that 'make tests' does not fail.
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345
FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4
I should have mentioned this in my previous comment.
My capsule tests assume that the capsule update does *not* initiate a reboot automatically and does "reboot" by "env print -e Capsule0000".
Furthermore, since the current sandbox_defconfig does not enable any U-Boot environment storage, "dfu_alt_info," for instance, cannot retain across the reboot (and so I didn't use CAPSULE_ON_DISK_EARLY).
I will help Masami fix the issue.
-Takahiro Akashi
Best regards
Heinrich
Best regards
Heinrich
Hi Takahiro,
2022年2月14日(月) 10:06 AKASHI Takahiro takahiro.akashi@linaro.org:
On Sun, Feb 13, 2022 at 11:17:37AM +0100, Heinrich Schuchardt wrote:
On 2/13/22 10:01, Heinrich Schuchardt wrote:
On 2/3/22 10:23, Masami Hiramatsu wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing thesystem is restarted after capsule processing is completed.
This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Gitlab CI tests fail. Please, resubmit with the Python tests adjusted. Make sure that 'make tests' does not fail.
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345
FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3 FAILED test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4
I should have mentioned this in my previous comment.
My capsule tests assume that the capsule update does *not* initiate a reboot automatically and does "reboot" by "env print -e Capsule0000".
Hm, so this should be fixed by the test case.
Furthermore, since the current sandbox_defconfig does not enable any U-Boot environment storage, "dfu_alt_info," for instance, cannot retain across the reboot (and so I didn't use CAPSULE_ON_DISK_EARLY).
OK, but would this be a matter? (I guess you can define dfu_alt_info and update the firmware afterwards.)
I will help Masami fix the issue.
Thank you!
-Takahiro Akashi
Best regards
Heinrich
Best regards
Heinrich
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting. - run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True)
output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img, @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' not in ''.join(output)
output = u_boot_console.run_command_list(['efidebug capsule esrt'])
@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' not in ''.join(output)
output = u_boot_console.run_command_list(['efidebug capsule esrt'])
diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True, - wait_for_prompt=True): + wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console.
The command is always sent to U-Boot. @@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed. + wait_for_reboot: Boolean indication whether to wait for the + reboot U-Boot. If this is True, wait_for_prompt is ignored.
Returns: If wait_for_prompt == False: @@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return - m = self.p.expect([self.prompt_compiled] + self.bad_patterns) - if m != 0: - self.at_prompt = False - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) + if wait_for_reboot: + bcfg = self.config.buildconfig + config_spl = bcfg.get('config_spl', 'n') == 'y' + config_spl_serial = bcfg.get('config_spl_serial', + 'n') == 'y' + env_spl_skipped = self.config.env.get('env__spl_skipped', + False) + env_spl2_skipped = self.config.env.get('env__spl2_skipped', + True) + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) + self.u_boot_version_string = self.p.after + while True: + m = self.p.expect([self.prompt_compiled, + pattern_stop_autoboot_prompt] + self.bad_patterns) + if m == 0: + break + if m == 1: + self.p.send(' ') + continue + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 2]) + else: + m = self.p.expect([self.prompt_compiled] + self.bad_patterns) + if m != 0: + self.at_prompt = False + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing @@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self): + def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target @@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args: - None. + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default.
Returns: Nothing. @@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True) - if config_spl and config_spl_serial and not env_spl_skipped: - m = self.p.expect([pattern_u_boot_spl_signon] + - self.bad_patterns) + if expect_earlyreset: + loop_num = 2 + else: + loop_num = 1 + + while loop_num > 0: + loop_num -= 1 + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0: - raise Exception('Bad pattern found on SPL console: ' + - self.bad_pattern_ids[m - 1]) - if not env_spl2_skipped: - m = self.p.expect([pattern_u_boot_spl2_signon] + - self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on SPL2 console: ' + + raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1]) - m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled, @@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self): + def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn() - self.ensure_spawned() + self.ensure_spawned(expect_earlyreset)
def get_spawn_output(self): """Return the start-up output from U-Boot diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags): + def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a string + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default.
Returns: A u_boot_spawn.Spawn object that is attached to U-Boot. @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags - return self.restart_uboot() + return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
Hi Heinrich,
I and Takahiro found that the capsule update test case and test framework didn't expect that a command can reboot the sandbox and the sandbox can reboot while booting, which happens when my "Reset system after CapsuleUpdate on disk" patch applied. This patch fixes those issues.
Thank you,
2022年2月15日(火) 18:05 Masami Hiramatsu masami.hiramatsu@linaro.org:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistentvariables @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img,@@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistentvariables @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x1000000x50000;u-boot-env raw 0x150000 0x200000"',
'host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test02' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsuleesrt'])
@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistentvariables @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x1000000x50000;u-boot-env raw 0x150000 0x200000"',
'host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsuleesrt'])
diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True,
wait_for_prompt=True):
wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot.@@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed.
wait_for_reboot: Boolean indication whether to wait for thereboot U-Boot. If this is True, wait_for_prompt isignored.
Returns: If wait_for_prompt == False:@@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return
m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
if wait_for_reboot:bcfg = self.config.buildconfigconfig_spl = bcfg.get('config_spl', 'n') == 'y'config_spl_serial = bcfg.get('config_spl_serial','n') == 'y'env_spl_skipped = self.config.env.get('env__spl_skipped',False)env_spl2_skipped =self.config.env.get('env__spl2_skipped',
True)if config_spl and config_spl_serial and notenv_spl_skipped:
m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPLconsole: ' +
self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2console: ' +
self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] +self.bad_patterns)
if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])self.u_boot_version_string = self.p.afterwhile True:m = self.p.expect([self.prompt_compiled,pattern_stop_autoboot_prompt] + self.bad_patterns)if m == 0:breakif m == 1:self.p.send(' ')continueraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 2])else:m = self.p.expect([self.prompt_compiled] +self.bad_patterns)
if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing@@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self):
def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resettingtarget @@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and notenv_spl_skipped:
m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPLconsole: ' +
self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2console: ' +
self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] +self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: '
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] +self.bad_patterns)
if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset)def get_spawn_output(self): """Return the start-up output from U-Boot
diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
On 2/15/22 10:09, Masami Hiramatsu wrote:
Hi Heinrich,
I and Takahiro found that the capsule update test case and test framework didn't expect that a command can reboot the sandbox and the sandbox can reboot while booting, which happens when my "Reset system after CapsuleUpdate on disk" patch applied. This patch fixes those issues.
Gitlab CI testing does not pass. So this series has to be reworked.
Best regards
Heinrich
Thank you,
2022年2月15日(火) 18:05 Masami Hiramatsu <masami.hiramatsu@linaro.org mailto:masami.hiramatsu@linaro.org>:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot. Thus this update the uboot_console for those 2 cases; - restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting. - run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command. And enable those options in the test_capsule_firmware.py test cases. Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org <mailto:masami.hiramatsu@linaro.org>> --- .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-) diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output) - # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object): # need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img, @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output) - # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object): # need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt']) @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output) - # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object): # need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt']) diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close() def run_command(self, cmd, wait_for_echo=True, send_nl=True, - wait_for_prompt=True): + wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot. @@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed. + wait_for_reboot: Boolean indication whether to wait for the + reboot U-Boot. If this is True, wait_for_prompt is ignored. Returns: If wait_for_prompt == False: @@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return - m = self.p.expect([self.prompt_compiled] + self.bad_patterns) - if m != 0: - self.at_prompt = False - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) + if wait_for_reboot: + bcfg = self.config.buildconfig + config_spl = bcfg.get('config_spl', 'n') == 'y' + config_spl_serial = bcfg.get('config_spl_serial', + 'n') == 'y' + env_spl_skipped = self.config.env.get('env__spl_skipped', + False) + env_spl2_skipped = self.config.env.get('env__spl2_skipped', + True) + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) + self.u_boot_version_string = self.p.after + while True: + m = self.p.expect([self.prompt_compiled, + pattern_stop_autoboot_prompt] + self.bad_patterns) + if m == 0: + break + if m == 1: + self.p.send(' ') + continue + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 2]) + else: + m = self.p.expect([self.prompt_compiled] + self.bad_patterns) + if m != 0: + self.at_prompt = False + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing @@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout - def ensure_spawned(self): + def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance. This may require spawning a new Sandbox process or resetting target @@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly. Args: - None. + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default. Returns: Nothing. @@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True) - if config_spl and config_spl_serial and not env_spl_skipped: - m = self.p.expect([pattern_u_boot_spl_signon] + - self.bad_patterns) + if expect_earlyreset: + loop_num = 2 + else: + loop_num = 1 + + while loop_num > 0: + loop_num -= 1 + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0: - raise Exception('Bad pattern found on SPL console: ' + - self.bad_pattern_ids[m - 1]) - if not env_spl2_skipped: - m = self.p.expect([pattern_u_boot_spl2_signon] + - self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on SPL2 console: ' + + raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1]) - m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled, @@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None - def restart_uboot(self): + def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn() - self.ensure_spawned() + self.ensure_spawned(expect_earlyreset) def get_spawn_output(self): """Return the start-up output from U-Boot diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir) - def restart_uboot_with_flags(self, flags): + def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags Args: flags: List of flags to pass, each a string + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot. @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase): try: self.sandbox_flags = flags - return self.restart_uboot() + return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []-- Masami Hiramatsu
On 2/15/22 14:51, Heinrich Schuchardt wrote:
On 2/15/22 10:09, Masami Hiramatsu wrote:
Hi Heinrich,
I and Takahiro found that the capsule update test case and test framework didn't expect that a command can reboot the sandbox and the sandbox can reboot while booting, which happens when my "Reset system after CapsuleUpdate on disk" patch applied. This patch fixes those issues.
Gitlab CI testing does not pass. So this series has to be reworked.
Sorry I missed that you put a new patch into the old series. I will retest.
Best regards
Heinrich
Best regards
Heinrich
Thank you,
2022年2月15日(火) 18:05 Masami Hiramatsu <masami.hiramatsu@linaro.org mailto:masami.hiramatsu@linaro.org>:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting. - run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org mailto:masami.hiramatsu@linaro.org> --- .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True)
output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img, @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test02' not in ''.join(output)
output = u_boot_console.run_command_list(['efidebug capsule esrt'])
@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
- # reboot - u_boot_console.restart_uboot() - capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate') + + # reboot + u_boot_console.restart_uboot(expect_earlyreset = capsule_early) + with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command( - 'env print -e Capsule0000') + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test03' not in ''.join(output)
output = u_boot_console.run_command_list(['efidebug capsule esrt'])
diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True, - wait_for_prompt=True): + wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console.
The command is always sent to U-Boot. @@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed. + wait_for_reboot: Boolean indication whether to wait for the + reboot U-Boot. If this is True, wait_for_prompt is ignored.
Returns: If wait_for_prompt == False: @@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return - m = self.p.expect([self.prompt_compiled] + self.bad_patterns) - if m != 0: - self.at_prompt = False - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) + if wait_for_reboot: + bcfg = self.config.buildconfig + config_spl = bcfg.get('config_spl', 'n') == 'y' + config_spl_serial = bcfg.get('config_spl_serial', + 'n') == 'y' + env_spl_skipped = self.config.env.get('env__spl_skipped', + False) + env_spl2_skipped = self.config.env.get('env__spl2_skipped', + True) + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) + self.u_boot_version_string = self.p.after + while True: + m = self.p.expect([self.prompt_compiled, + pattern_stop_autoboot_prompt] + self.bad_patterns) + if m == 0: + break + if m == 1: + self.p.send(' ') + continue + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 2]) + else: + m = self.p.expect([self.prompt_compiled] + self.bad_patterns) + if m != 0: + self.at_prompt = False + raise Exception('Bad pattern found on console: ' + + self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing @@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self): + def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target @@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args: - None. + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default.
Returns: Nothing. @@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True) - if config_spl and config_spl_serial and not env_spl_skipped: - m = self.p.expect([pattern_u_boot_spl_signon] + - self.bad_patterns) + if expect_earlyreset: + loop_num = 2 + else: + loop_num = 1 + + while loop_num > 0: + loop_num -= 1 + if config_spl and config_spl_serial and not env_spl_skipped: + m = self.p.expect([pattern_u_boot_spl_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL console: ' + + self.bad_pattern_ids[m - 1]) + if not env_spl2_skipped: + m = self.p.expect([pattern_u_boot_spl2_signon] + + self.bad_patterns) + if m != 0: + raise Exception('Bad pattern found on SPL2 console: ' + + self.bad_pattern_ids[m - 1]) + m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0: - raise Exception('Bad pattern found on SPL console: ' + - self.bad_pattern_ids[m - 1]) - if not env_spl2_skipped: - m = self.p.expect([pattern_u_boot_spl2_signon] + - self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on SPL2 console: ' + + raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1]) - m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) - if m != 0: - raise Exception('Bad pattern found on console: ' + - self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled, @@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self): + def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn() - self.ensure_spawned() + self.ensure_spawned(expect_earlyreset)
def get_spawn_output(self): """Return the start-up output from U-Boot diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags): + def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a string + expect_earlyreset: This boot is expected to be reset in early + stage (before prompt). False by default.
Returns: A u_boot_spawn.Spawn object that is attached to U-Boot. @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags - return self.restart_uboot() + return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
-- Masami Hiramatsu
On 2/15/22 10:05, Masami Hiramatsu wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
When applying a patch series we must ensure that after each individual patch we have a valid software state.
'make tests' fails in test_capsule_firmware.py after applying only this patch to U-Boot origin/master.
This patch should be split in two:
* changes to the console base, to be applied before the capsule series * changes to the capsule test, to be merged into the "EFI: Reset system after capsule-on-disk" series.
Please, ensure that after each single patch 'make tests' succeeds.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img,@@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test02' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True,
wait_for_prompt=True):
wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot.@@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed.
wait_for_reboot: Boolean indication whether to wait for thereboot U-Boot. If this is True, wait_for_prompt is ignored. Returns: If wait_for_prompt == False:@@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return
m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
if wait_for_reboot:bcfg = self.config.buildconfigconfig_spl = bcfg.get('config_spl', 'n') == 'y'config_spl_serial = bcfg.get('config_spl_serial','n') == 'y'env_spl_skipped = self.config.env.get('env__spl_skipped',False)env_spl2_skipped = self.config.env.get('env__spl2_skipped',True)if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])self.u_boot_version_string = self.p.afterwhile True:m = self.p.expect([self.prompt_compiled,pattern_stop_autoboot_prompt] + self.bad_patterns)if m == 0:breakif m == 1:self.p.send(' ')continueraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 2])else:m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing@@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self):
def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target@@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default.
From the description of the argument it is not clear that only a reset inside main U-Boot is tolerated if expect_earlyreset==true while an early reset in SPL leads to a test failure.
Best regards
Heinrich
Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset) def get_spawn_output(self): """Return the start-up output from U-Bootdiff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
Hi
2022年2月15日(火) 23:15 Heinrich Schuchardt xypron.glpk@gmx.de:
On 2/15/22 10:05, Masami Hiramatsu wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
When applying a patch series we must ensure that after each individual patch we have a valid software state.
'make tests' fails in test_capsule_firmware.py after applying only this patch to U-Boot origin/master.
Ah, OK. So let me update and resend the reset-after-capsule series.
This patch should be split in two:
- changes to the console base, to be applied before the capsule series
- changes to the capsule test, to be merged into the "EFI: Reset system
after capsule-on-disk" series.
Please, ensure that after each single patch 'make tests' succeeds.
OK, I'll make it bisectable. BTW, I think the latter part should be merged to the "reset-after-capsule" patch because it changes the behavior that the old test expects.
[snip]
@@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default.From the description of the argument it is not clear that only a reset inside main U-Boot is tolerated if expect_earlyreset==true while an early reset in SPL leads to a test failure.
Indeed. I'll make it simply "expect_reset" and update the comment that the reset is expected after SPL before prompt.
Thank you,
Best regards
Heinrich
Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset) def get_spawn_output(self): """Return the start-up output from U-Bootdiff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
2022年2月15日(火) 23:15 Heinrich Schuchardt xypron.glpk@gmx.de:
On 2/15/22 10:05, Masami Hiramatsu wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
When applying a patch series we must ensure that after each individual patch we have a valid software state.
'make tests' fails in test_capsule_firmware.py after applying only this patch to U-Boot origin/master.
This patch should be split in two:
- changes to the console base, to be applied before the capsule series
- changes to the capsule test, to be merged into the "EFI: Reset system
after capsule-on-disk" series.
Please, ensure that after each single patch 'make tests' succeeds.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img,@@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test02' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True,
wait_for_prompt=True):
wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot.@@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed.
wait_for_reboot: Boolean indication whether to wait for thereboot U-Boot. If this is True, wait_for_prompt is ignored. Returns: If wait_for_prompt == False:@@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return
m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
if wait_for_reboot:bcfg = self.config.buildconfigconfig_spl = bcfg.get('config_spl', 'n') == 'y'config_spl_serial = bcfg.get('config_spl_serial','n') == 'y'env_spl_skipped = self.config.env.get('env__spl_skipped',False)env_spl2_skipped = self.config.env.get('env__spl2_skipped',True)if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])self.u_boot_version_string = self.p.afterwhile True:m = self.p.expect([self.prompt_compiled,pattern_stop_autoboot_prompt] + self.bad_patterns)if m == 0:breakif m == 1:self.p.send(' ')continueraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 2])else:m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing@@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self):
def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target@@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default.From the description of the argument it is not clear that only a reset inside main U-Boot is tolerated if expect_earlyreset==true while an early reset in SPL leads to a test failure.
Best regards
Heinrich
Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset) def get_spawn_output(self): """Return the start-up output from U-Bootdiff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
On Tue, Feb 15, 2022 at 06:05:50PM +0900, Masami Hiramatsu wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img,@@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
Probably most people don't understand why we need dfu_alt_info here. It would be better to merge it to "efidebug capsule esrt" and leave a comment.
'host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test02' not in ''.join(output)
Anyhow, this assertion exists below in this test case scenario, doesn't it?
output = u_boot_console.run_command_list(['efidebug capsule esrt'])@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True,
wait_for_prompt=True):
wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot.@@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed.
wait_for_reboot: Boolean indication whether to wait for thereboot U-Boot. If this is True, wait_for_prompt is ignored.
Umm, if so,
Returns: If wait_for_prompt == False:@@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return
This check must be pushed backward after "if wait_for_reboot".
m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
if wait_for_reboot:bcfg = self.config.buildconfigconfig_spl = bcfg.get('config_spl', 'n') == 'y'config_spl_serial = bcfg.get('config_spl_serial','n') == 'y'env_spl_skipped = self.config.env.get('env__spl_skipped',False)env_spl2_skipped = self.config.env.get('env__spl2_skipped',True)if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])self.u_boot_version_string = self.p.afterwhile True:m = self.p.expect([self.prompt_compiled,pattern_stop_autoboot_prompt] + self.bad_patterns)if m == 0:breakif m == 1:self.p.send(' ')continueraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 2])else:m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
This part of hunk is somehow duplicated in ensure_spawned() below except loop_num. Please rework the code if possible.
-Takahiro Akashi
self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing@@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self):
def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target@@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset)def get_spawn_output(self): """Return the start-up output from U-Boot
diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
Hi Takahiro,
2022年2月16日(水) 10:35 AKASHI Takahiro takahiro.akashi@linaro.org:
On Tue, Feb 15, 2022 at 06:05:50PM +0900, Masami Hiramatsu wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py index 6e803f699f..a539134ec2 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test01' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 2-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True) output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img,@@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test02' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 3-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',Probably most people don't understand why we need dfu_alt_info here. It would be better to merge it to "efidebug capsule esrt" and leave a comment.
Sure.
'host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test02' not in ''.join(output)Anyhow, this assertion exists below in this test case scenario, doesn't it?
Oops, I missed that. OK, let me remove it.
output = u_boot_console.run_command_list(['efidebug capsule esrt'])@@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object): 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) assert 'Test03' in ''.join(output)
# rebootu_boot_console.restart_uboot()capsule_early = u_boot_config.buildconfig.get( 'config_efi_capsule_on_disk_early') capsule_auth = u_boot_config.buildconfig.get( 'config_efi_capsule_authenticate')
# rebootu_boot_console.restart_uboot(expect_earlyreset = capsule_early)with u_boot_console.log.section('Test Case 4-b, after reboot'): if not capsule_early: # make sure that dfu_alt_info exists even persistent variables@@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
# need to run uefi command to initiate capsule handling output = u_boot_console.run_command(
'env print -e Capsule0000')
'env print -e Capsule0000', wait_for_reboot = True)output = u_boot_console.run_command_list(['env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"','host bind 0 %s' % disk_img,'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])assert 'Test03' not in ''.join(output) output = u_boot_console.run_command_list(['efidebug capsule esrt'])diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py index 384fd53c65..e84f4d74ef 100644 --- a/test/py/u_boot_console_base.py +++ b/test/py/u_boot_console_base.py @@ -140,7 +140,7 @@ class ConsoleBase(object): self.logstream.close()
def run_command(self, cmd, wait_for_echo=True, send_nl=True,
wait_for_prompt=True):
wait_for_prompt=True, wait_for_reboot=False): """Execute a command via the U-Boot console. The command is always sent to U-Boot.@@ -168,6 +168,8 @@ class ConsoleBase(object): wait_for_prompt: Boolean indicating whether to wait for the command prompt to be sent by U-Boot. This typically occurs immediately after the command has been executed.
wait_for_reboot: Boolean indication whether to wait for thereboot U-Boot. If this is True, wait_for_prompt is ignored.Umm, if so,
Returns: If wait_for_prompt == False:@@ -202,11 +204,48 @@ class ConsoleBase(object): self.bad_pattern_ids[m - 1]) if not wait_for_prompt: return
This check must be pushed backward after "if wait_for_reboot".
Oops, sorry, the description is wrong. Actually wait_for_prompt must be True for wait_for_reboot because wait_for_reboot will wait for the prompt after reboot.
m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])
if wait_for_reboot:bcfg = self.config.buildconfigconfig_spl = bcfg.get('config_spl', 'n') == 'y'config_spl_serial = bcfg.get('config_spl_serial','n') == 'y'env_spl_skipped = self.config.env.get('env__spl_skipped',False)env_spl2_skipped = self.config.env.get('env__spl2_skipped',True)if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])self.u_boot_version_string = self.p.afterwhile True:m = self.p.expect([self.prompt_compiled,pattern_stop_autoboot_prompt] + self.bad_patterns)if m == 0:breakif m == 1:self.p.send(' ')continueraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 2])else:m = self.p.expect([self.prompt_compiled] + self.bad_patterns)if m != 0:self.at_prompt = Falseraise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1])This part of hunk is somehow duplicated in ensure_spawned() below except loop_num. Please rework the code if possible.
Yes, OK, I'll refactor this part out and reuse it from both methods.
Thank you!
-Takahiro Akashi
self.at_prompt = True self.at_prompt_logevt = self.logstream.logfile.cur_evt # Only strip \r\n; space/TAB might be significant if testing@@ -321,7 +360,7 @@ class ConsoleBase(object): finally: self.p.timeout = orig_timeout
- def ensure_spawned(self):
def ensure_spawned(self, expect_earlyreset=False): """Ensure a connection to a correctly running U-Boot instance.
This may require spawning a new Sandbox process or resetting target@@ -330,7 +369,8 @@ class ConsoleBase(object): This is an internal function and should not be called directly.
Args:
None.
expect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: Nothing.@@ -357,22 +397,29 @@ class ConsoleBase(object): False) env_spl2_skipped = self.config.env.get('env__spl2_skipped', True)
if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)
if expect_earlyreset:loop_num = 2else:loop_num = 1while loop_num > 0:loop_num -= 1if config_spl and config_spl_serial and not env_spl_skipped:m = self.p.expect([pattern_u_boot_spl_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +self.bad_pattern_ids[m - 1])m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns) if m != 0:
raise Exception('Bad pattern found on SPL console: ' +self.bad_pattern_ids[m - 1])if not env_spl2_skipped:m = self.p.expect([pattern_u_boot_spl2_signon] +self.bad_patterns)if m != 0:raise Exception('Bad pattern found on SPL2 console: ' +
raise Exception('Bad pattern found on console: ' + self.bad_pattern_ids[m - 1])
m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)if m != 0:raise Exception('Bad pattern found on console: ' +self.bad_pattern_ids[m - 1]) self.u_boot_version_string = self.p.after while True: m = self.p.expect([self.prompt_compiled,@@ -416,10 +463,10 @@ class ConsoleBase(object): pass self.p = None
- def restart_uboot(self):
- def restart_uboot(self, expect_earlyreset=False): """Shut down and restart U-Boot.""" self.cleanup_spawn()
self.ensure_spawned()
self.ensure_spawned(expect_earlyreset)def get_spawn_output(self): """Return the start-up output from U-Boot
diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py index 7e1eb0e0b4..9cd9ccad30 100644 --- a/test/py/u_boot_console_sandbox.py +++ b/test/py/u_boot_console_sandbox.py @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase): cmd += self.sandbox_flags return Spawn(cmd, cwd=self.config.source_dir)
- def restart_uboot_with_flags(self, flags):
def restart_uboot_with_flags(self, flags, expect_earlyreset=False): """Run U-Boot with the given command-line flags
Args: flags: List of flags to pass, each a stringexpect_earlyreset: This boot is expected to be reset in earlystage (before prompt). False by default. Returns: A u_boot_spawn.Spawn object that is attached to U-Boot.@@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
try: self.sandbox_flags = flags
return self.restart_uboot()
return self.restart_uboot(expect_earlyreset) finally: self.sandbox_flags = []
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
PLEASE use that instead of adding all this code. Also make sure that test works with 'make qcheck' too.
Regards, Simon
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Best regards
Heinrich
PLEASE use that instead of adding all this code. Also make sure that test works with 'make qcheck' too.
Regards, Simon
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
You will not want to update the firmware on real hardware especially if there is a rollback protection. But testing on QEMU would make sense.
Best regards
Heinrich
On Wed, Feb 16, 2022 at 06:45:32PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
OK, I'll leave sandbox commentary to Simon.
You will not want to update the firmware on real hardware especially if there is a rollback protection. But testing on QEMU would make sense.
On QEMU we should be able to since we can turn boards off and on and would be writing to our emulated flash. Then on real hardware, we should be able to do the same tests, or at least the ones involving "update was successful" ? We have DFU tests today too, so I'm not immediately sure why we couldn't (aside from possibly wearing down flash life, and gating HW tests on emulator tests passing is always a good idea).
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself), but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
If so, I think there are 2 issues to be solved. 1. change the capsule update testcase runable on Qemu 2. change my patch to handle the real reset correctly (not only waiting for the next boot, but also respawn it again)
Do I understand correctly?
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote: > > Since now the capsule_on_disk will restart the u-boot sandbox right > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the > boot with a new capsule file will repeat reboot sequence. On the > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' > command will execute the capsule update on disk and reboot. > > Thus this update the uboot_console for those 2 cases; > > - restart_uboot(): Add expect_earlyreset optional parameter so that > it can handle the reboot while booting. > - run_command(): Add wait_for_reboot optional parameter so that it > can handle the reboot after executing a command. > > And enable those options in the test_capsule_firmware.py test cases. > > Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org > --- > .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- > test/py/u_boot_console_base.py | 95 +++++++++++++++----- > test/py/u_boot_console_sandbox.py | 6 + > 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Hmm, would you mean adding a runtime flag to sandbox so that it will not does real reset but just showing some token on console like "sandbox fake reset done." ?
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
OK.
Thank you,
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote: > Hi Masami, > > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu > masami.hiramatsu@linaro.org wrote: >> >> Since now the capsule_on_disk will restart the u-boot sandbox right >> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the >> boot with a new capsule file will repeat reboot sequence. On the >> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' >> command will execute the capsule update on disk and reboot. >> >> Thus this update the uboot_console for those 2 cases; >> >> - restart_uboot(): Add expect_earlyreset optional parameter so that >> it can handle the reboot while booting. >> - run_command(): Add wait_for_reboot optional parameter so that it >> can handle the reboot after executing a command. >> >> And enable those options in the test_capsule_firmware.py test cases. >> >> Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org >> --- >> .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- >> test/py/u_boot_console_base.py | 95 +++++++++++++++----- >> test/py/u_boot_console_sandbox.py | 6 + >> 3 files changed, 102 insertions(+), 38 deletions(-) > > We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
-- Masami Hiramatsu
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
Best regards
Heinrich
Hmm, would you mean adding a runtime flag to sandbox so that it will not does real reset but just showing some token on console like "sandbox fake reset done." ?
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
OK.
Thank you,
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote: > On 2/16/22 16:26, Simon Glass wrote: >> Hi Masami, >> >> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu >> masami.hiramatsu@linaro.org wrote: >>> >>> Since now the capsule_on_disk will restart the u-boot sandbox right >>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the >>> boot with a new capsule file will repeat reboot sequence. On the >>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' >>> command will execute the capsule update on disk and reboot. >>> >>> Thus this update the uboot_console for those 2 cases; >>> >>> - restart_uboot(): Add expect_earlyreset optional parameter so that >>> it can handle the reboot while booting. >>> - run_command(): Add wait_for_reboot optional parameter so that it >>> can handle the reboot after executing a command. >>> >>> And enable those options in the test_capsule_firmware.py test cases. >>> >>> Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org >>> --- >>> .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- >>> test/py/u_boot_console_base.py | 95 +++++++++++++++----- >>> test/py/u_boot_console_sandbox.py | 6 + >>> 3 files changed, 102 insertions(+), 38 deletions(-) >> >> We have a means to avoid actually doing the reset, see the reset driver. > > The UEFI specification requires a cold reset after a capsule is updated > and before the console is reached. How could the reset driver help to > fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
-- Masami Hiramatsu
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
-Takahiro Akashi
Best regards
Heinrich
Hmm, would you mean adding a runtime flag to sandbox so that it will not does real reset but just showing some token on console like "sandbox fake reset done." ?
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
OK.
Thank you,
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote: > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote: > > On 2/16/22 16:26, Simon Glass wrote: > > > Hi Masami, > > > > > > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu > > > masami.hiramatsu@linaro.org wrote: > > > > > > > > Since now the capsule_on_disk will restart the u-boot sandbox right > > > > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the > > > > boot with a new capsule file will repeat reboot sequence. On the > > > > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' > > > > command will execute the capsule update on disk and reboot. > > > > > > > > Thus this update the uboot_console for those 2 cases; > > > > > > > > - restart_uboot(): Add expect_earlyreset optional parameter so that > > > > it can handle the reboot while booting. > > > > - run_command(): Add wait_for_reboot optional parameter so that it > > > > can handle the reboot after executing a command. > > > > > > > > And enable those options in the test_capsule_firmware.py test cases. > > > > > > > > Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org > > > > --- > > > > .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- > > > > test/py/u_boot_console_base.py | 95 +++++++++++++++----- > > > > test/py/u_boot_console_sandbox.py | 6 + > > > > 3 files changed, 102 insertions(+), 38 deletions(-) > > > > > > We have a means to avoid actually doing the reset, see the reset driver. > > > > The UEFI specification requires a cold reset after a capsule is updated > > and before the console is reached. How could the reset driver help to > > fix the Python tests? > > Is this test going to be able to run on qemu, sandbox, real hardware, or > all 3? The tests may well end up having to know a bit more, sadly, > about the type of system they're testing. > Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
-- Masami Hiramatsu
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code. You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
[..]
Regards, Simon
Hi Simon and Takahiro,
I also would like to know how the UEFI is tested in the U-Boot. It seems that we have lib/efi_selftest/ but it seems not unified to 'ut' command. Should I expand efi_selftest for testing capsule update with reset?
Thank you,
2022年3月12日(土) 11:24 Simon Glass sjg@chromium.org:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code. You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
[..]
Regards, Simon
Hi Masami,
On Sun, 13 Mar 2022 at 08:00, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon and Takahiro,
I also would like to know how the UEFI is tested in the U-Boot. It seems that we have lib/efi_selftest/ but it seems not unified to 'ut' command.
I think it would be good to add it there. There is some init that must be done, but we do try to use 'ut' for all tests.
Should I expand efi_selftest for testing capsule update with reset?
Well I think the update could be triggered by a command, so you can test it without actually doing the reset, at least as a starting point. We need to write the code with testing in mind.
Regards, Simon
Thank you,
2022年3月12日(土) 11:24 Simon Glass sjg@chromium.org:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote: > > Hi Simon, > > Let me confirm your point. > So are you concerning the 'real' reset for the capsule update test > case itself or this patch? > > I'm actually learning how the test is working, so please help me to > understand how I can solve it. > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
> but Qemu and real board will cause a real reset and it will terminate > the qemu or stop the board (depends on how it is implemented). Thus, > if a command or boot process will cause a reset, it will need a > special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code. You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
[..]
Regards, Simon
-- Masami Hiramatsu
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
-Takahiro Akashi
[..]
Regards, Simon
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote: > > Hi Simon, > > Let me confirm your point. > So are you concerning the 'real' reset for the capsule update test > case itself or this patch? > > I'm actually learning how the test is working, so please help me to > understand how I can solve it. > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
> but Qemu and real board will cause a real reset and it will terminate > the qemu or stop the board (depends on how it is implemented). Thus, > if a command or boot process will cause a reset, it will need a > special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
Regards, SImon
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote:
Hi Simon,
Thank you for your reply.
2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org:
> > Hi Masami, > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > masami.hiramatsu@linaro.org wrote: > > > > Hi Simon, > > > > Let me confirm your point. > > So are you concerning the 'real' reset for the capsule update test > > case itself or this patch? > > > > I'm actually learning how the test is working, so please help me to > > understand how I can solve it. > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > > If we reset a sandbox, it will continue to run (just restart itself), > > Here you should be able to avoid doing a reset. See > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
Would you mean that reset-after-capsule-on-disk itself should not make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
In dm_test_sysreset_base(), I can see the below code, this means sysreset_request() will not execute real reset, but just mimic the reset, right?
state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); state->sysreset_allowed[SYSRESET_WARM] = false;
> > but Qemu and real board will cause a real reset and it will terminate > > the qemu or stop the board (depends on how it is implemented). Thus, > > if a command or boot process will cause a reset, it will need a > > special care (maybe respawn?). > > Here you need to worry about the surrounding automation logic which > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and > handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
There are two ways: a. CapsuleUpdate runtime service b. capsule delivery on disk
My original patch provides only (b), partly, because runtime service is a bit hard to implement under the current framework.
UEFI specification requires that (b) can/should be initiated by a *reset by a user* and another reset be automatically triggered by UEFI when the update has been completed either successfully or in vain. The latter behavior has been enforced on U-BOOT UEFI implementation by Masami's patch (not this series).
Masami's patch (this series) fixes issues around those two resets in pytest.
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
Hi Takahiro,
On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
On 2/18/22 03:16, Masami Hiramatsu wrote: > Hi Simon, > > Thank you for your reply. > > 2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org: > > > > > Hi Masami, > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > > masami.hiramatsu@linaro.org wrote: > > > > > > Hi Simon, > > > > > > Let me confirm your point. > > > So are you concerning the 'real' reset for the capsule update test > > > case itself or this patch? > > > > > > I'm actually learning how the test is working, so please help me to > > > understand how I can solve it. > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > > > If we reset a sandbox, it will continue to run (just restart itself), > > > > Here you should be able to avoid doing a reset. See > > dm_test_sysreset_base() which tests sysreset drivers on sandbox. > > Would you mean that reset-after-capsule-on-disk itself should not > make a reset on sandbox?
We have several tests that do resets by calling do_reset(), e.g. the UEFI unit tests. There is nothing wrong about it.
We want the sandbox to behave like any other board where capsule updates lead to resets.
> > In dm_test_sysreset_base(), I can see the below code, this means > sysreset_request() > will not execute real reset, but just mimic the reset, right? > > state->sysreset_allowed[SYSRESET_WARM] = true; > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); > state->sysreset_allowed[SYSRESET_WARM] = false; > > > > but Qemu and real board will cause a real reset and it will terminate > > > the qemu or stop the board (depends on how it is implemented). Thus, > > > if a command or boot process will cause a reset, it will need a > > > special care (maybe respawn?). > > > > Here you need to worry about the surrounding automation logic which > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and > > handle it some other way, without reset.
The sandbox should run through exactly the same code path as all other boards to get a meaningful test results. Therefore don't put in any quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
There are two ways: a. CapsuleUpdate runtime service b. capsule delivery on disk
My original patch provides only (b), partly, because runtime service is a bit hard to implement under the current framework.
UEFI specification requires that (b) can/should be initiated by a *reset by a user* and another reset be automatically triggered by UEFI when the update has been completed either successfully or in vain. The latter behavior has been enforced on U-BOOT UEFI implementation by Masami's patch (not this series).
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
Regards, Simon
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
Hi Simon,
2022年3月14日(月) 15:45 Simon Glass sjg@chromium.org:
Hi Takahiro,
On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote: > On 2/18/22 03:16, Masami Hiramatsu wrote: > > Hi Simon, > > > > Thank you for your reply. > > > > 2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org: > > > > > > > > Hi Masami, > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > > > masami.hiramatsu@linaro.org wrote: > > > > > > > > Hi Simon, > > > > > > > > Let me confirm your point. > > > > So are you concerning the 'real' reset for the capsule update test > > > > case itself or this patch? > > > > > > > > I'm actually learning how the test is working, so please help me to > > > > understand how I can solve it. > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > > > > If we reset a sandbox, it will continue to run (just restart itself), > > > > > > Here you should be able to avoid doing a reset. See > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox. > > > > Would you mean that reset-after-capsule-on-disk itself should not > > make a reset on sandbox? > > We have several tests that do resets by calling do_reset(), e.g. the > UEFI unit tests. There is nothing wrong about it. > > We want the sandbox to behave like any other board where capsule updates > lead to resets. > > > > > In dm_test_sysreset_base(), I can see the below code, this means > > sysreset_request() > > will not execute real reset, but just mimic the reset, right? > > > > state->sysreset_allowed[SYSRESET_WARM] = true; > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); > > state->sysreset_allowed[SYSRESET_WARM] = false; > > > > > > but Qemu and real board will cause a real reset and it will terminate > > > > the qemu or stop the board (depends on how it is implemented). Thus, > > > > if a command or boot process will cause a reset, it will need a > > > > special care (maybe respawn?). > > > > > > Here you need to worry about the surrounding automation logic which > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and > > > handle it some other way, without reset. > > The sandbox should run through exactly the same code path as all other > boards to get a meaningful test results. Therefore don't put in any > quirks on C level. Your Python test changes are all that is needed.
+1, I have the same opinion here. To exercise capsule-on-disk code, we need a "real" reset because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
There are two ways: a. CapsuleUpdate runtime service b. capsule delivery on disk
My original patch provides only (b), partly, because runtime service is a bit hard to implement under the current framework.
UEFI specification requires that (b) can/should be initiated by a *reset by a user* and another reset be automatically triggered by UEFI when the update has been completed either successfully or in vain. The latter behavior has been enforced on U-BOOT UEFI implementation by Masami's patch (not this series).
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Thank you,
Regards, Simon
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
On Mon, Mar 14, 2022 at 04:35:45PM +0900, Masami Hiramatsu wrote:
Hi Simon,
2022年3月14日(月) 15:45 Simon Glass sjg@chromium.org:
Hi Takahiro,
On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote: > > On 2/18/22 03:16, Masami Hiramatsu wrote: > > > Hi Simon, > > > > > > Thank you for your reply. > > > > > > 2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org: > > > > > > > > > > > Hi Masami, > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > > > > masami.hiramatsu@linaro.org wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > Let me confirm your point. > > > > > So are you concerning the 'real' reset for the capsule update test > > > > > case itself or this patch? > > > > > > > > > > I'm actually learning how the test is working, so please help me to > > > > > understand how I can solve it. > > > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > > > > > If we reset a sandbox, it will continue to run (just restart itself), > > > > > > > > Here you should be able to avoid doing a reset. See > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox. > > > > > > Would you mean that reset-after-capsule-on-disk itself should not > > > make a reset on sandbox? > > > > We have several tests that do resets by calling do_reset(), e.g. the > > UEFI unit tests. There is nothing wrong about it. > > > > We want the sandbox to behave like any other board where capsule updates > > lead to resets. > > > > > > > > In dm_test_sysreset_base(), I can see the below code, this means > > > sysreset_request() > > > will not execute real reset, but just mimic the reset, right? > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true; > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); > > > state->sysreset_allowed[SYSRESET_WARM] = false; > > > > > > > > but Qemu and real board will cause a real reset and it will terminate > > > > > the qemu or stop the board (depends on how it is implemented). Thus, > > > > > if a command or boot process will cause a reset, it will need a > > > > > special care (maybe respawn?). > > > > > > > > Here you need to worry about the surrounding automation logic which > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and > > > > handle it some other way, without reset. > > > > The sandbox should run through exactly the same code path as all other > > boards to get a meaningful test results. Therefore don't put in any > > quirks on C level. Your Python test changes are all that is needed. > > +1, I have the same opinion here. > To exercise capsule-on-disk code, we need a "real" reset > because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
There are two ways: a. CapsuleUpdate runtime service b. capsule delivery on disk
My original patch provides only (b), partly, because runtime service is a bit hard to implement under the current framework.
UEFI specification requires that (b) can/should be initiated by a *reset by a user* and another reset be automatically triggered by UEFI when the update has been completed either successfully or in vain. The latter behavior has been enforced on U-BOOT UEFI implementation by Masami's patch (not this series).
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
I oppose it simply
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
because of this.
System test should not use an internal function, it should only exercise public interfaces from user's viewpoint.
-Takahiro Akashi
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Thank you,
Regards, Simon
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
-- Masami Hiramatsu
Hi Masami,
On Mon, 14 Mar 2022 at 01:35, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月14日(月) 15:45 Simon Glass sjg@chromium.org:
Hi Takahiro,
On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
Hi Takahiro,
On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
Hi Takahiro,
On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote: > > On 2/18/22 03:16, Masami Hiramatsu wrote: > > > Hi Simon, > > > > > > Thank you for your reply. > > > > > > 2022年2月18日(金) 2:56 Simon Glass sjg@chromium.org: > > > > > > > > > > > Hi Masami, > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > > > > masami.hiramatsu@linaro.org wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > Let me confirm your point. > > > > > So are you concerning the 'real' reset for the capsule update test > > > > > case itself or this patch? > > > > > > > > > > I'm actually learning how the test is working, so please help me to > > > > > understand how I can solve it. > > > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board. > > > > > If we reset a sandbox, it will continue to run (just restart itself), > > > > > > > > Here you should be able to avoid doing a reset. See > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox. > > > > > > Would you mean that reset-after-capsule-on-disk itself should not > > > make a reset on sandbox? > > > > We have several tests that do resets by calling do_reset(), e.g. the > > UEFI unit tests. There is nothing wrong about it. > > > > We want the sandbox to behave like any other board where capsule updates > > lead to resets. > > > > > > > > In dm_test_sysreset_base(), I can see the below code, this means > > > sysreset_request() > > > will not execute real reset, but just mimic the reset, right? > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true; > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM)); > > > state->sysreset_allowed[SYSRESET_WARM] = false; > > > > > > > > but Qemu and real board will cause a real reset and it will terminate > > > > > the qemu or stop the board (depends on how it is implemented). Thus, > > > > > if a command or boot process will cause a reset, it will need a > > > > > special care (maybe respawn?). > > > > > > > > Here you need to worry about the surrounding automation logic which > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and > > > > handle it some other way, without reset. > > > > The sandbox should run through exactly the same code path as all other > > boards to get a meaningful test results. Therefore don't put in any > > quirks on C level. Your Python test changes are all that is needed. > > +1, I have the same opinion here. > To exercise capsule-on-disk code, we need a "real" reset > because pytest/CI loop is basically a black-box test.
I don't see why you need the reset at all to test the code.
As I repeatedly said, I believe that this is a black-box test and a system test. The purpose of the test is to make sure the firmware update be performed in real operations as expected, that is, a *reset* triggers the action *at the beginning of* system reboot.
I understand you are frustrated with this, but I just don't agree, or perhaps don't understand.
What specific mechanism is used to initiate the firmware update? Is this happening in U-Boot or somewhere else?
There are two ways: a. CapsuleUpdate runtime service b. capsule delivery on disk
My original patch provides only (b), partly, because runtime service is a bit hard to implement under the current framework.
UEFI specification requires that (b) can/should be initiated by a *reset by a user* and another reset be automatically triggered by UEFI when the update has been completed either successfully or in vain. The latter behavior has been enforced on U-BOOT UEFI implementation by Masami's patch (not this series).
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Anyway we should design subsystems so they are easy to test.
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
Regards, Simon [..]
You should be able to run a command to make the update happen. How does the updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios). - confirm that the capsule update on disk can find the capsule file from ESP specified by the BOOTXXXX EFI variable. - confirm that the capsule update on disk writes the firmware correctly to the storage which specified by DFU. - confirm that the capsule update on disk success if the capsule image type is supported. - confirm that the capsule update on disk fails if the capsule image type is not supported. - confirm that the capsule update on disk will reboot after update even if the update is failed.
The only spec we can not test is - confirm that the capsule update on disk is kicked when the UEFI is initialized.
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
Thank you,
Regards, Simon [..]
> You should > be able to run a command to make the update happen. How does the > updata actually get triggered when you reset?
It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
On Tue, Mar 15, 2022 at 09:40:34AM +0900, Masami Hiramatsu wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
It is important to verify this feature in system test like pytest.
Anyway we should design subsystems so they are easy to test.
Anyway we should design systems test so that they emulate operations in real world.
-Takahiro Akashi
Here I guess you mean the unit test, not system test, am I correct?
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
Thank you,
Regards, Simon [..]
> > > You should > > be able to run a command to make the update happen. How does the > > updata actually get triggered when you reset? > > It's not the purpose of this test.
Then drop the reset and design the feature with testing in mind.
So it's just beyond of my scope.
-Takahiro Akashi
Regards, SImon
-- Masami Hiramatsu
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
OK thank you.
Regards, Simon
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Thank you,
Masami's patch (this series) fixes issues around those two resets in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
OK thank you.
Regards, Simon
-- Masami Hiramatsu
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
OK, well 'reset by a user' presumably starts the board up and then runs some code to do the update in U-Boot? Is that right? If so, we just need to trigger that update from the test. We don't need to test the actual reset, at least not with sandbox. As I said, we need to write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
> Masami's patch (this series) fixes issues around those two resets > in pytest.
Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
OK thank you.
Regards, SImon
On Tue, Mar 15, 2022 at 09:13:15PM -0600, Simon Glass wrote:
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
> OK, well 'reset by a user' presumably starts the board up and then > runs some code to do the update in U-Boot? Is that right? If so, we > just need to trigger that update from the test. We don't need to test > the actual reset, at least not with sandbox. As I said, we need to > write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
I don't really understand why you want and need to avoid a real reset in, what you call, integration test.
-Takahiro Akashi
> > Masami's patch (this series) fixes issues around those two resets > > in pytest. > > Yes and that is the problem I have, at least on sandbox.
So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update, it could help?
Yes that can help, because sandbox can detect that and turn it into a nop.
OK, let me submit a patch to update it.
OK thank you.
Regards, SImon
Hi Simon,
2022年3月16日(水) 12:13 Simon Glass sjg@chromium.org:
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
> OK, well 'reset by a user' presumably starts the board up and then > runs some code to do the update in U-Boot? Is that right? If so, we > just need to trigger that update from the test. We don't need to test > the actual reset, at least not with sandbox. As I said, we need to > write the code so that it is easy to test.
Actually, we already have that command, "efidebug capsule disk-update" which kicks the capsule update code even without the 'reset by a user'. So we can just kick this command for checking whether the U-Boot UEFI code correctly find the capsule file from ESP which specified by UEFI vars.
However, the 'capsule update on-disk' feature is also expected (and defined in the spec?) to run when the UEFI subsystem is initialized. This behavior will not be tested if we skip the 'reset by a user'. I guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
EFI has the ExitBootServices call, but I'm not sure it is actually clear all resources. Maybe we need to check what resources are released by the ExitBootServices.
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
OK.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
So the fake USB stick data is generated in the build process? If so, we also can build a fake ESP master image which already includes a capsule file.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
OK. BTW, as you said above, when we run such integration test for EFI which includes to run sysreset, before that sandbox will switch the sysreset driver for resetting EFI to avoid restarting it?
Thank you,
Hi Masami,
On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月16日(水) 12:13 Simon Glass sjg@chromium.org:
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org:
> > OK, well 'reset by a user' presumably starts the board up and then > > runs some code to do the update in U-Boot? Is that right? If so, we > > just need to trigger that update from the test. We don't need to test > > the actual reset, at least not with sandbox. As I said, we need to > > write the code so that it is easy to test. > > Actually, we already have that command, "efidebug capsule disk-update" > which kicks the capsule update code even without the 'reset by a > user'. So we can just kick this command for checking whether the > U-Boot UEFI code correctly find the capsule file from ESP which > specified by UEFI vars. > > However, the 'capsule update on-disk' feature is also expected (and > defined in the spec?) to run when the UEFI subsystem is initialized. > This behavior will not be tested if we skip the 'reset by a user'. I > guess Takahiro's current test case tries to check it.
The 'UEFI subsystem is intialised' is a problem, actually, since if it were better integrated into driver model, it would not have separate structures or they would be present and enabled when driver model is. I hope that it can be fixed and Takahiro's series is a start in that direction.
OK.
But as to a test that an update is called when UEFI starts, that seems like a single line of code. Sure it is nice to test it, but it is much more important to test the installation of the update and the execution of the update. I suppose another way to test that is to shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
usb start fatload usb 0 $kernel_addr_r test.cap fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize efidebug capsule disk-update
(run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
EFI has the ExitBootServices call, but I'm not sure it is actually clear all resources. Maybe we need to check what resources are released by the ExitBootServices.
Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
OK.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
So the fake USB stick data is generated in the build process? If so, we also can build a fake ESP master image which already includes a capsule file.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
OK. BTW, as you said above, when we run such integration test for EFI which includes to run sysreset, before that sandbox will switch the sysreset driver for resetting EFI to avoid restarting it?
Yes. The UEFI update seems quite monolithic from what I am hearing. Could it be split into a few separate U-Boot commands, liike:
- efi update prepare (set up the update somewhere) - efi update exec (do the update)
Regards, SImon
Am 16. März 2022 20:23:37 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月16日(水) 12:13 Simon Glass sjg@chromium.org:
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org: > > > > OK, well 'reset by a user' presumably starts the board up and then > > > runs some code to do the update in U-Boot? Is that right? If so, we > > > just need to trigger that update from the test. We don't need to test > > > the actual reset, at least not with sandbox. As I said, we need to > > > write the code so that it is easy to test. > > > > Actually, we already have that command, "efidebug capsule disk-update" > > which kicks the capsule update code even without the 'reset by a > > user'. So we can just kick this command for checking whether the > > U-Boot UEFI code correctly find the capsule file from ESP which > > specified by UEFI vars. > > > > However, the 'capsule update on-disk' feature is also expected (and > > defined in the spec?) to run when the UEFI subsystem is initialized. > > This behavior will not be tested if we skip the 'reset by a user'. I > > guess Takahiro's current test case tries to check it. > > The 'UEFI subsystem is intialised' is a problem, actually, since if it > were better integrated into driver model, it would not have separate > structures or they would be present and enabled when driver model is. > I hope that it can be fixed and Takahiro's series is a start in that > direction.
OK.
> But as to a test that an update is called when UEFI starts, that seems > like a single line of code. Sure it is nice to test it, but it is much > more important to test the installation of the update and the > execution of the update. I suppose another way to test that is to > shut down the UEFI subsystem and start it up?
Yes, currently we call do_reset() after install the capsule file. (This reset can be avoided if we replace it with sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
Here is how I tested it on my machine;
> usb start > fatload usb 0 $kernel_addr_r test.cap > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize > efidebug capsule disk-update (run install process and reboot the machine)
So, if we can avoid the last reset, we can test the below without reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.
The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
EFI has the ExitBootServices call, but I'm not sure it is actually clear all resources. Maybe we need to check what resources are released by the ExitBootServices.
> > Anyway we should design subsystems so they are easy to test.
Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
OK.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
So the fake USB stick data is generated in the build process? If so, we also can build a fake ESP master image which already includes a capsule file.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
OK. BTW, as you said above, when we run such integration test for EFI which includes to run sysreset, before that sandbox will switch the sysreset driver for resetting EFI to avoid restarting it?
Yes. The UEFI update seems quite monolithic from what I am hearing. Could it be split into a few separate U-Boot commands, liike:
- efi update prepare (set up the update somewhere)
- efi update exec (do the update)
Capsule updates are not done via the command line.
Either you call the UpdateCapsule() service or you set a flag in the OsIndications variable and provide a file in a predefined path.
Best regards
Heinrich
Regards, SImon
Hi Heinrich,
On Wed, 16 Mar 2022 at 14:47, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 16. März 2022 20:23:37 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Masami,
On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月16日(水) 12:13 Simon Glass sjg@chromium.org:
Hi Masami,
On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
2022年3月15日(火) 14:04 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu masami.hiramatsu@linaro.org wrote: > > Hi Simon, > > 2022年3月15日(火) 3:24 Simon Glass sjg@chromium.org: > > > > > > OK, well 'reset by a user' presumably starts the board up and then > > > > runs some code to do the update in U-Boot? Is that right? If so, we > > > > just need to trigger that update from the test. We don't need to test > > > > the actual reset, at least not with sandbox. As I said, we need to > > > > write the code so that it is easy to test. > > > > > > Actually, we already have that command, "efidebug capsule disk-update" > > > which kicks the capsule update code even without the 'reset by a > > > user'. So we can just kick this command for checking whether the > > > U-Boot UEFI code correctly find the capsule file from ESP which > > > specified by UEFI vars. > > > > > > However, the 'capsule update on-disk' feature is also expected (and > > > defined in the spec?) to run when the UEFI subsystem is initialized. > > > This behavior will not be tested if we skip the 'reset by a user'. I > > > guess Takahiro's current test case tries to check it. > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it > > were better integrated into driver model, it would not have separate > > structures or they would be present and enabled when driver model is. > > I hope that it can be fixed and Takahiro's series is a start in that > > direction. > > OK. > > > But as to a test that an update is called when UEFI starts, that seems > > like a single line of code. Sure it is nice to test it, but it is much > > more important to test the installation of the update and the > > execution of the update. I suppose another way to test that is to > > shut down the UEFI subsystem and start it up? > > Yes, currently we call do_reset() after install the capsule file. > (This reset can be avoided if we replace it with > sysreset_walk_halt(SYSRESET_COLD) as you said, right?) > > Here is how I tested it on my machine; > > > usb start > > fatload usb 0 $kernel_addr_r test.cap > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize > > efidebug capsule disk-update > (run install process and reboot the machine) > > So, if we can avoid the last reset, we can test the below without > reset on sandbox (depends on scenarios). > - confirm that the capsule update on disk can find the capsule file > from ESP specified by the BOOTXXXX EFI variable. > - confirm that the capsule update on disk writes the firmware > correctly to the storage which specified by DFU. > - confirm that the capsule update on disk success if the capsule image > type is supported. > - confirm that the capsule update on disk fails if the capsule image > type is not supported. > - confirm that the capsule update on disk will reboot after update > even if the update is failed. > > The only spec we can not test is > - confirm that the capsule update on disk is kicked when the UEFI is > initialized.
Even that could be tested, by installing an update and then initing UEFI?
yeah, if the UEFI is not initialized yet, we can run some UEFI related command (e.g. printenv -e) instead of efidebug capsule... to execute the capsule update on disk. But anyway, this is only available at the first time. We need a way to reset UEFI subsystem without system reset.
Yes. It is certainly possible, but I'm not sure how easy it is. Perhaps just drop all the EFI data structures and run the EFI init again? We have something similar with driver model. See dm_test_pre_run()
EFI has the ExitBootServices call, but I'm not sure it is actually clear all resources. Maybe we need to check what resources are released by the ExitBootServices.
> > > > Anyway we should design subsystems so they are easy to test. > > Here I guess you mean the unit test, not system test, am I correct?
Yes. Easy testing is so important for developer productivity and happiness. It is fine to have large system/functional tests as a fall back or catch-all, but they tend to test the happy path only. When they fail, they are hard to debug because they cover such as large area of the code and they often have complex setup requirements so are hard to run manually.
My hope is that all the functionality should be covered by unit tests or integration tests, so that system/functional almost never fail.
My another question is how small is the granularity of the unit test. As I showed, the UEFI capsule update needs to prepare a capsule file installed in the storage. That seems to be very system-level. But you think that is still be a unit test? (I expected that the 'Unit test' is something like KUnit in Linux)
Well I am using your terminology here. Technically many of the U-Boot tests (executed by 'ut' command) are not really unit tests. They bring in a lot of code and run one test case using it.
OK.
For example, one of the tests brings up the USB subsystem, including a fake USB stick, then checks it can read data from the stick, using the USB stack.
So the fake USB stick data is generated in the build process? If so, we also can build a fake ESP master image which already includes a capsule file.
Another one writes some things to the emulated display and then checks that the correct pixels are there.
Perhaps a better name would be integration test. But the point is that we can run these tests very, very quickly and (setup aside) without outside influence, or without restarting the executable, etc.
OK. BTW, as you said above, when we run such integration test for EFI which includes to run sysreset, before that sandbox will switch the sysreset driver for resetting EFI to avoid restarting it?
Yes. The UEFI update seems quite monolithic from what I am hearing. Could it be split into a few separate U-Boot commands, liike:
- efi update prepare (set up the update somewhere)
- efi update exec (do the update)
Capsule updates are not done via the command line.
Either you call the UpdateCapsule() service or you set a flag in the OsIndications variable and provide a file in a predefined path.
OK, but I think it would be helpful to provide this feature via the cmdline too. That is how things normally work in U-Boot, right?
Regards, Simon
On 2/17/22 18:55, Simon Glass wrote:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Respawning of QEMU instances after a reset is handled by the scripts in https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my local tests I have used: https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test
If you want to set up an environment for a real board, you have to write your own python scripts and make them available over PYTHONPATH. For an example see https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
Why fake a reset? Just call the normal reset code. Avoid sandbox quirks. We want the sandbox to just run through the same code as any other board to get meaningful test results.
Best regards
Heinrich
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
On 2/16/22 16:26, Simon Glass wrote: > Hi Masami, > > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu > masami.hiramatsu@linaro.org wrote: >> >> Since now the capsule_on_disk will restart the u-boot sandbox right >> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the >> boot with a new capsule file will repeat reboot sequence. On the >> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' >> command will execute the capsule update on disk and reboot. >> >> Thus this update the uboot_console for those 2 cases; >> >> - restart_uboot(): Add expect_earlyreset optional parameter so that >> it can handle the reboot while booting. >> - run_command(): Add wait_for_reboot optional parameter so that it >> can handle the reboot after executing a command. >> >> And enable those options in the test_capsule_firmware.py test cases. >> >> Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org >> --- >> .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- >> test/py/u_boot_console_base.py | 95 +++++++++++++++----- >> test/py/u_boot_console_sandbox.py | 6 + >> 3 files changed, 102 insertions(+), 38 deletions(-) > > We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
On Fri, Feb 18, 2022 at 02:59:05PM +0100, Heinrich Schuchardt wrote:
On 2/17/22 18:55, Simon Glass wrote:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Respawning of QEMU instances after a reset is handled by the scripts in https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my local tests I have used: https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test
If you want to set up an environment for a real board, you have to write your own python scripts and make them available over PYTHONPATH. For an example see https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test
To be clear, Simon also has a number of physical boards in a CI lab. Perhaps part of the confusion here is that we're thinking of these tests as something more special than what we have already, but I don't think that's the case. It's just shifting the logic around a little bit in that today we have "build u-boot, flash u-boot, ensure that version is now running" and this is "build u-boot, update u-boot, ensure that version is now running".
Hi Heinrich,
2022年2月18日(金) 22:59 Heinrich Schuchardt xypron.glpk@gmx.de:
On 2/17/22 18:55, Simon Glass wrote:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Respawning of QEMU instances after a reset is handled by the scripts in https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my local tests I have used: https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test
If you want to set up an environment for a real board, you have to write your own python scripts and make them available over PYTHONPATH. For an example see https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test
Great! thank you for the information. I actually would like to confirm that my change on ConsoleBase class will work on non-sandbox, or it should be ConsoleSandbox class method.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
Why fake a reset? Just call the normal reset code. Avoid sandbox quirks. We want the sandbox to just run through the same code as any other board to get meaningful test results.
OK. I got it.
Thank you,
Best regards
Heinrich
Regards, Simon
Thank you,
2022年2月17日(木) 2:53 Simon Glass sjg@chromium.org:
Hi Heinrich,
On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:46, Tom Rini wrote:
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote: > On 2/16/22 16:26, Simon Glass wrote: >> Hi Masami, >> >> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu >> masami.hiramatsu@linaro.org wrote: >>> >>> Since now the capsule_on_disk will restart the u-boot sandbox right >>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the >>> boot with a new capsule file will repeat reboot sequence. On the >>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' >>> command will execute the capsule update on disk and reboot. >>> >>> Thus this update the uboot_console for those 2 cases; >>> >>> - restart_uboot(): Add expect_earlyreset optional parameter so that >>> it can handle the reboot while booting. >>> - run_command(): Add wait_for_reboot optional parameter so that it >>> can handle the reboot after executing a command. >>> >>> And enable those options in the test_capsule_firmware.py test cases. >>> >>> Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org >>> --- >>> .../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- >>> test/py/u_boot_console_base.py | 95 +++++++++++++++----- >>> test/py/u_boot_console_sandbox.py | 6 + >>> 3 files changed, 102 insertions(+), 38 deletions(-) >> >> We have a means to avoid actually doing the reset, see the reset driver. > > The UEFI specification requires a cold reset after a capsule is updated > and before the console is reached. How could the reset driver help to > fix the Python tests?
Is this test going to be able to run on qemu, sandbox, real hardware, or all 3? The tests may well end up having to know a bit more, sadly, about the type of system they're testing.
Currently the test will only run on the sandbox in Gitlab (see usage of @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
Let me know if you need help reworking this patch to operate on sandbox without a 'real' reset.
Regards, Simon
-- Masami Hiramatsu
On Thu, Feb 17, 2022 at 10:55:58AM -0700, Simon Glass wrote:
Hi Masami,
On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
Let me confirm your point. So are you concerning the 'real' reset for the capsule update test case itself or this patch?
I'm actually learning how the test is working, so please help me to understand how I can solve it.
There are 3 environments to run the test, sandbox, Qemu, and a real board. If we reset a sandbox, it will continue to run (just restart itself),
Here you should be able to avoid doing a reset. See dm_test_sysreset_base() which tests sysreset drivers on sandbox.
but Qemu and real board will cause a real reset and it will terminate the qemu or stop the board (depends on how it is implemented). Thus, if a command or boot process will cause a reset, it will need a special care (maybe respawn?).
Here you need to worry about the surrounding automation logic which could be tbot of the U-Boot pytest hooks. I suggest you avoid this and handle it some other way, without reset.
Well, tbot and uboot-test-hooks implement the abstractions we have for things like "reset the target".
Since the capsule update testcase only runs on sandbox, it will not cause real reset. But maybe it is possible to support running on Qemu.
Maybe, but I don't think you should worry about that, at least for now. The sandbox test is enough.
Current my test patch (and capsule update testcase itself) doesn't handle the real reset case correctly even on Qemu. The Qemu needs spawn a new instance and re-connect the console when the reset happens.
Indeed.
If so, I think there are 2 issues to be solved.
- change the capsule update testcase runable on Qemu
- change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)
Do I understand correctly?
I think the best approach is to get your test running on sandbox, with the faked reset. Don't worry about the other cases as we don't support them.
I don't think we need to test qemu/real hardware to start with. I think we should be testing real hardware in the near future however. Flash this U-Boot and confirm it's running is the point of the "version" test.
Hi Heinrich,
On Wed, 16 Feb 2022 at 08:37, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/16/22 16:26, Simon Glass wrote:
Hi Masami,
On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Since now the capsule_on_disk will restart the u-boot sandbox right after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the boot with a new capsule file will repeat reboot sequence. On the other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e' command will execute the capsule update on disk and reboot.
Thus this update the uboot_console for those 2 cases;
- restart_uboot(): Add expect_earlyreset optional parameter so that it can handle the reboot while booting.
- run_command(): Add wait_for_reboot optional parameter so that it can handle the reboot after executing a command.
And enable those options in the test_capsule_firmware.py test cases.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
.../test_efi_capsule/test_capsule_firmware.py | 39 ++++++-- test/py/u_boot_console_base.py | 95 +++++++++++++++----- test/py/u_boot_console_sandbox.py | 6 + 3 files changed, 102 insertions(+), 38 deletions(-)
We have a means to avoid actually doing the reset, see the reset driver.
The UEFI specification requires a cold reset after a capsule is updated and before the console is reached. How could the reset driver help to fix the Python tests?
The sandbox reset driver can handle this, so at least for sandbox (where CI runs) this should be used.
For real boards, you are getting into very strange territory, with test harnesses and whatever.
Make it run on sandbox for now, and that is good enough for CI at present.
NAK on this patch.
PLEASE use that instead of adding all this code. Also make sure that test works with 'make qcheck' too.
Regards, Simon
Regards, Simon
Hi Masami,
On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
I don't get that.
You should use sysreset and the SYSRESET_COLD type.
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
return ret;
/** UEFI spec requires to reset system after complete processing capsule* update on the storage.*/log_info("Reboot after firmware update");/* Cold reset is required for loading the new firmware. */do_reset(NULL, 0, 0, NULL);
No! This should use sysreset.
hang();/* not reach here */return 0;} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Regards, Simon
Hi Simon,
2022年2月27日(日) 3:37 Simon Glass sjg@chromium.org:
Hi Masami,
On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
I don't get that.
You should use sysreset and the SYSRESET_COLD type.
Thanks, yeah, I found that parameter. Let's fix that.
Thank you,
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
return ret;
/** UEFI spec requires to reset system after complete processing capsule* update on the storage.*/log_info("Reboot after firmware update");/* Cold reset is required for loading the new firmware. */do_reset(NULL, 0, 0, NULL);No! This should use sysreset.
hang();/* not reach here */return 0;} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Regards, Simon
Hi Simon,
BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass 0 to argc, it seems to do SYSRESET_COLD, isn't it?
int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { enum sysreset_t reset_type = SYSRESET_COLD;
if (argc > 2) return CMD_RET_USAGE;
if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') { reset_type = SYSRESET_WARM; }
printf("resetting ...\n"); mdelay(100);
sysreset_walk_halt(reset_type);
return 0; }
2022年2月28日(月) 13:19 Masami Hiramatsu masami.hiramatsu@linaro.org:
Hi Simon,
2022年2月27日(日) 3:37 Simon Glass sjg@chromium.org:
Hi Masami,
On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
I don't get that.
You should use sysreset and the SYSRESET_COLD type.
Thanks, yeah, I found that parameter. Let's fix that.
Thank you,
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
return ret;
/** UEFI spec requires to reset system after complete processing capsule* update on the storage.*/log_info("Reboot after firmware update");/* Cold reset is required for loading the new firmware. */do_reset(NULL, 0, 0, NULL);No! This should use sysreset.
hang();/* not reach here */return 0;} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Regards, Simon
-- Masami Hiramatsu
Hi Masami,
On Mon, 28 Feb 2022 at 00:53, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass 0 to argc, it seems to do SYSRESET_COLD, isn't it?
Yes, but we should use the driver interface to do things, not the command-line interface :-)
- Simon
int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { enum sysreset_t reset_type = SYSRESET_COLD;
if (argc > 2) return CMD_RET_USAGE; if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') { reset_type = SYSRESET_WARM; } printf("resetting ...\n"); mdelay(100); sysreset_walk_halt(reset_type); return 0;}
2022年2月28日(月) 13:19 Masami Hiramatsu masami.hiramatsu@linaro.org:
Hi Simon,
2022年2月27日(日) 3:37 Simon Glass sjg@chromium.org:
Hi Masami,
On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Add a cold reset soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as;
In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed.This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Changes in v4:
- Do not use sysreset because that is a warm reset.
I don't get that.
You should use sysreset and the SYSRESET_COLD type.
Thanks, yeah, I found that parameter. Let's fix that.
Thank you,
- Fix patch description.
lib/efi_loader/efi_capsule.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..20d9490dbd 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -14,6 +14,7 @@ #include <env.h> #include <fdtdec.h> #include <fs.h> +#include <hang.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void) if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS)
log_err("Applying capsule %ls failed\n",
log_err("Applying capsule %ls failed.\n", files[i]);elselog_info("Applying capsule %ls succeeded.\n",files[i]); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret);@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
return ret;
/** UEFI spec requires to reset system after complete processing capsule* update on the storage.*/log_info("Reboot after firmware update");/* Cold reset is required for loading the new firmware. */do_reset(NULL, 0, 0, NULL);No! This should use sysreset.
hang();/* not reach here */return 0;} #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Regards, Simon
-- Masami Hiramatsu
-- Masami Hiramatsu
Hi Simon,
2022年3月1日(火) 23:58 Simon Glass sjg@chromium.org:
Hi Masami,
On Mon, 28 Feb 2022 at 00:53, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Hi Simon,
BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass 0 to argc, it seems to do SYSRESET_COLD, isn't it?
Yes, but we should use the driver interface to do things, not the command-line interface :-)
OK, I got it. So something like this attached patch? (This is for the next branch)
Thank you,
participants (5)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Masami Hiramatsu -
Simon Glass -
Tom Rini