| Message ID | 20230120123120.1014589-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/1] efi_loader: get rid of ad-hoc EFI subsystem init | expand |
Just a small correction on the commit log On Fri, 20 Jan 2023 at 14:31, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Up to now the EFI subsystem was left out of the main U-Boot init > process. This has led to various hacks over the years, with the most > notable one being sprinkling around the efi init call to various places > such as U-Boot commands, the early boot code etc. > > Since EFI has it's own Kconfig option and people can remove it, let's > wire up the EFI init call on an event for EVT_MAIN_LOOP. Please ignore this sentence, it's a leftover from previous experiments. > > This will also get rid of ad-hoc code in the main event loop, which was > trying to initialize the subsystem early and perform capsule updates. > > TODO: > - The efi_tcg protocol implicitly initializes the TPM, as a result > some of the tpm selftests will fail with the RFC. If everyone > agrees that this is a good idea, I'll clean up the TPM hacks as well > - We still need to run capsule updates on the main_loop() code since > in some cases (e.g sandbox) we need preboot commands. > - wider tests, I've only run QEMU for now [...]
Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: >Up to now the EFI subsystem was left out of the main U-Boot init >process. This has led to various hacks over the years, with the most >notable one being sprinkling around the efi init call to various places >such as U-Boot commands, the early boot code etc. > >Since EFI has it's own Kconfig option and people can remove it, let's >wire up the EFI init call on an event for EVT_MAIN_LOOP. > >This will also get rid of ad-hoc code in the main event loop, which was >trying to initialize the subsystem early and perform capsule updates. > >TODO: >- The efi_tcg protocol implicitly initializes the TPM, as a result > some of the tpm selftests will fail with the RFC. If everyone > agrees that this is a good idea, I'll clean up the TPM hacks as well >- We still need to run capsule updates on the main_loop() code since > in some cases (e.g sandbox) we need preboot commands. >- wider tests, I've only run QEMU for now > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. Please, put the Kconfig related capsule change into a separate patch. Otherwise looks good to me. Best regards Heinrich >--- > boot/bootm_os.c | 8 -------- > cmd/bootefi.c | 8 -------- > cmd/bootmenu.c | 9 --------- > cmd/eficonfig.c | 8 -------- > cmd/efidebug.c | 9 --------- > cmd/nvedit_efi.c | 17 ----------------- > common/main.c | 8 +++----- > include/efi_loader.h | 2 -- > lib/efi_loader/Kconfig | 10 ---------- > lib/efi_loader/efi_setup.c | 25 ++++++++++++++++++------- > lib/fwu_updates/Kconfig | 1 - > lib/fwu_updates/fwu.c | 3 --- > 12 files changed, 21 insertions(+), 87 deletions(-) > >diff --git a/boot/bootm_os.c b/boot/bootm_os.c >index 99ff0e6c02d2..4775cecb4e64 100644 >--- a/boot/bootm_os.c >+++ b/boot/bootm_os.c >@@ -505,14 +505,6 @@ static int do_bootm_efi(int flag, int argc, char *const argv[], > if (ret) > return ret; > >- /* Initialize EFI drivers */ >- efi_ret = efi_init_obj_list(); >- if (efi_ret != EFI_SUCCESS) { >- printf("## Failed to initialize UEFI sub-system: r = %lu\n", >- efi_ret & ~EFI_ERROR_MASK); >- return 1; >- } >- > /* Install device tree */ > efi_ret = efi_install_fdt(images->ft_len > ? images->ft_addr : EFI_FDT_USE_INTERNAL); >diff --git a/cmd/bootefi.c b/cmd/bootefi.c >index 2a7d42925d65..5d56448ee6ce 100644 >--- a/cmd/bootefi.c >+++ b/cmd/bootefi.c >@@ -668,14 +668,6 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc, > if (argc < 2) > return CMD_RET_USAGE; > >- /* Initialize EFI drivers */ >- ret = efi_init_obj_list(); >- if (ret != EFI_SUCCESS) { >- log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- ret & ~EFI_ERROR_MASK); >- return CMD_RET_FAILURE; >- } >- > if (argc > 2) { > uintptr_t fdt_addr; > >diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c >index 3236ca5d7994..1c604c79e01d 100644 >--- a/cmd/bootmenu.c >+++ b/cmd/bootmenu.c >@@ -449,15 +449,6 @@ static void handle_uefi_bootnext(void) > efi_status_t ret; > efi_uintn_t size; > >- /* Initialize EFI drivers */ >- ret = efi_init_obj_list(); >- if (ret != EFI_SUCCESS) { >- log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- ret & ~EFI_ERROR_MASK); >- >- return; >- } >- > /* If UEFI BootNext variable is set, boot the BootNext load option */ > size = sizeof(u16); > ret = efi_get_variable_int(u"BootNext", >diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c >index d830e4af53b8..16025d451901 100644 >--- a/cmd/eficonfig.c >+++ b/cmd/eficonfig.c >@@ -2545,14 +2545,6 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a > if (argc > 1) > return CMD_RET_USAGE; > >- ret = efi_init_obj_list(); >- if (ret != EFI_SUCCESS) { >- log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- ret & ~EFI_ERROR_MASK); >- >- return CMD_RET_FAILURE; >- } >- > ret = eficonfig_init(); > if (ret != EFI_SUCCESS) > return CMD_RET_FAILURE; >diff --git a/cmd/efidebug.c b/cmd/efidebug.c >index e6959ede930f..9ad253da91b2 100644 >--- a/cmd/efidebug.c >+++ b/cmd/efidebug.c >@@ -1464,21 +1464,12 @@ static int do_efidebug(struct cmd_tbl *cmdtp, int flag, > int argc, char *const argv[]) > { > struct cmd_tbl *cp; >- efi_status_t r; > > if (argc < 2) > return CMD_RET_USAGE; > > argc--; argv++; > >- /* Initialize UEFI drivers */ >- r = efi_init_obj_list(); >- if (r != EFI_SUCCESS) { >- printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- r & ~EFI_ERROR_MASK); >- return CMD_RET_FAILURE; >- } >- > cp = find_cmd_tbl(argv[0], cmd_efidebug_sub, > ARRAY_SIZE(cmd_efidebug_sub)); > if (!cp) >diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c >index 24944ab81e23..1ac1114d4c89 100644 >--- a/cmd/nvedit_efi.c >+++ b/cmd/nvedit_efi.c >@@ -210,15 +210,6 @@ int do_env_print_efi(struct cmd_tbl *cmdtp, int flag, int argc, > const efi_guid_t *guid_p = NULL; > efi_guid_t guid; > bool verbose = true; >- efi_status_t ret; >- >- /* Initialize EFI drivers */ >- ret = efi_init_obj_list(); >- if (ret != EFI_SUCCESS) { >- printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- ret & ~EFI_ERROR_MASK); >- return CMD_RET_FAILURE; >- } > > for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { > if (!strcmp(argv[0], "-guid")) { >@@ -388,14 +379,6 @@ int do_env_set_efi(struct cmd_tbl *cmdtp, int flag, int argc, > if (argc == 1) > return CMD_RET_USAGE; > >- /* Initialize EFI drivers */ >- ret = efi_init_obj_list(); >- if (ret != EFI_SUCCESS) { >- printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", >- ret & ~EFI_ERROR_MASK); >- return CMD_RET_FAILURE; >- } >- > /* > * attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > * EFI_VARIABLE_RUNTIME_ACCESS; >diff --git a/common/main.c b/common/main.c >index 682f3359ea38..edb5f4c20682 100644 >--- a/common/main.c >+++ b/common/main.c >@@ -54,11 +54,9 @@ void main_loop(void) > if (IS_ENABLED(CONFIG_UPDATE_TFTP)) > update_tftp(0UL, NULL, NULL); > >- if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) { >- /* efi_init_early() already called */ >- if (efi_init_obj_list() == EFI_SUCCESS) >- efi_launch_capsules(); >- } >+ /* EFI subsystem will be initialized from an event */ >+ if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK)) >+ efi_launch_capsules(); > > s = bootdelay_process(); > if (cli_process_fdt(&s)) >diff --git a/include/efi_loader.h b/include/efi_loader.h >index f9e427f09059..3a801cbbc4c1 100644 >--- a/include/efi_loader.h >+++ b/include/efi_loader.h >@@ -513,8 +513,6 @@ extern struct list_head efi_register_notify_events; > > /* called at pre-initialization */ > int efi_init_early(void); >-/* Initialize efi execution environment */ >-efi_status_t efi_init_obj_list(void); > /* Set up console modes */ > void efi_setup_console_size(void); > /* Install device tree */ >diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig >index b630d88ef9e2..f3aaa51bb36a 100644 >--- a/lib/efi_loader/Kconfig >+++ b/lib/efi_loader/Kconfig >@@ -153,16 +153,6 @@ config EFI_IGNORE_OSINDICATIONS > without setting the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED > flag in variable OsIndications. > >-config EFI_CAPSULE_ON_DISK_EARLY >- bool "Initiate capsule-on-disk at U-Boot boottime" >- depends on EFI_CAPSULE_ON_DISK >- help >- Normally, without this option enabled, capsules will be >- executed only at the first time of invoking one of efi command. >- If this option is enabled, capsules will be enforced to be >- executed as part of U-Boot initialisation so that they will >- surely take place whatever is set to distro_bootcmd. >- > config EFI_CAPSULE_FIRMWARE > bool > >diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c >index f0f01d3b1d5a..e2317aed1ec2 100644 >--- a/lib/efi_loader/efi_setup.c >+++ b/lib/efi_loader/efi_setup.c >@@ -215,7 +215,7 @@ out: > * > * Return: status code > */ >-efi_status_t efi_init_obj_list(void) >+static efi_status_t efi_init_obj_list(void) > { > efi_status_t ret = EFI_SUCCESS; > >@@ -335,14 +335,25 @@ efi_status_t efi_init_obj_list(void) > > /* Initialize EFI runtime services */ > ret = efi_reset_system_init(); >- if (ret != EFI_SUCCESS) >- goto out; > >- /* Execute capsules after reboot */ >- if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) && >- !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) >- ret = efi_launch_capsules(); > out: > efi_obj_list_initialized = ret; > return ret; > } >+ >+static int efi_evt_int(void *ctx, struct event *event) >+{ >+ efi_status_t ret; >+ int r = 0; >+ >+ ret = efi_init_obj_list(); >+ if (ret != EFI_SUCCESS) { >+ log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", >+ ret & ~EFI_ERROR_MASK); >+ r = -1; >+ } >+ >+ return r; >+} >+ >+EVENT_SPY(EVT_MAIN_LOOP, efi_evt_int); >diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig >index 78759e6618df..6ff577cb53b3 100644 >--- a/lib/fwu_updates/Kconfig >+++ b/lib/fwu_updates/Kconfig >@@ -3,7 +3,6 @@ config FWU_MULTI_BANK_UPDATE > depends on EFI_CAPSULE_ON_DISK > select PARTITION_TYPE_GUID > select EFI_SETUP_EARLY >- imply EFI_CAPSULE_ON_DISK_EARLY > select EVENT > help > Feature for updating firmware images on platforms having >diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c >index 5313d0730202..e4256bea7a2c 100644 >--- a/lib/fwu_updates/fwu.c >+++ b/lib/fwu_updates/fwu.c >@@ -700,9 +700,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event) > return 0; > } > >- if (efi_init_obj_list() != EFI_SUCCESS) >- return 0; >- > ret = fwu_get_dev_mdata(&dev, &mdata); > if (ret) > return ret; >-- >2.38.1 >
Hi, On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >Up to now the EFI subsystem was left out of the main U-Boot init > >process. This has led to various hacks over the years, with the most > >notable one being sprinkling around the efi init call to various places > >such as U-Boot commands, the early boot code etc. > > > >Since EFI has it's own Kconfig option and people can remove it, let's > >wire up the EFI init call on an event for EVT_MAIN_LOOP. > > > >This will also get rid of ad-hoc code in the main event loop, which was > >trying to initialize the subsystem early and perform capsule updates. > > > >TODO: > >- The efi_tcg protocol implicitly initializes the TPM, as a result > > some of the tpm selftests will fail with the RFC. If everyone > > agrees that this is a good idea, I'll clean up the TPM hacks as well > >- We still need to run capsule updates on the main_loop() code since > > in some cases (e.g sandbox) we need preboot commands. > >- wider tests, I've only run QEMU for now > > > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. > > Please, put the Kconfig related capsule change into a separate patch. > > Otherwise looks good to me. > I am OK with this change too. Two points from my side: 1. The main loop capsule update is (still) a mistake, unfortunately. It should be a command which is run on boot. For sandbox testing, that command should be run *without* rebooting. I am sure I asked for that at the time but for various reasons it didn't happen. Please can you make that change also? 2. EFI should not be maintaining its own separate data structures, but should keep them attached to driver model. They should be created as needed, dynamically, not all at the start. Is anyone looking at this? I am happy to help suggest initial target for this refactoring. Regards, Simon
On 1/20/23 20:19, Simon Glass wrote: > Hi, > > On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: >>> Up to now the EFI subsystem was left out of the main U-Boot init >>> process. This has led to various hacks over the years, with the most >>> notable one being sprinkling around the efi init call to various places >>> such as U-Boot commands, the early boot code etc. >>> >>> Since EFI has it's own Kconfig option and people can remove it, let's >>> wire up the EFI init call on an event for EVT_MAIN_LOOP. >>> >>> This will also get rid of ad-hoc code in the main event loop, which was >>> trying to initialize the subsystem early and perform capsule updates. >>> >>> TODO: >>> - The efi_tcg protocol implicitly initializes the TPM, as a result >>> some of the tpm selftests will fail with the RFC. If everyone >>> agrees that this is a good idea, I'll clean up the TPM hacks as well >>> - We still need to run capsule updates on the main_loop() code since >>> in some cases (e.g sandbox) we need preboot commands. >>> - wider tests, I've only run QEMU for now >>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >> >> >> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. >> >> Please, put the Kconfig related capsule change into a separate patch. >> >> Otherwise looks good to me. >> > > I am OK with this change too. > > Two points from my side: > > 1. The main loop capsule update is (still) a mistake, unfortunately. > It should be a command which is run on boot. For sandbox testing, that > command should be run *without* rebooting. I am sure I asked for that Capsule updates must run outside of any command as this is required by the UEFI specification. Capsule updates require a reboot and the sandbox must behave as a regular system so that we can run the same tests on all systems. > at the time but for various reasons it didn't happen. Please can you > make that change also? > > 2. EFI should not be maintaining its own separate data structures, but > should keep them attached to driver model. They should be created as > needed, dynamically, not all at the start. Is anyone looking at this? > I am happy to help suggest initial target for this refactoring. We have started with such a link for block devices. Once those block devices are really UEFI compliant we should refactor the other devices currently supported by EFI sub-system: UART, network, RNG, video, TPM. Some initialization like setting up UEFI variables will not have any equivalence in the driver model. Best regards Heinrich
Hi Heinrich, On Fri, 20 Jan 2023 at 13:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/20/23 20:19, Simon Glass wrote: > > Hi, > > > > On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >>> Up to now the EFI subsystem was left out of the main U-Boot init > >>> process. This has led to various hacks over the years, with the most > >>> notable one being sprinkling around the efi init call to various places > >>> such as U-Boot commands, the early boot code etc. > >>> > >>> Since EFI has it's own Kconfig option and people can remove it, let's > >>> wire up the EFI init call on an event for EVT_MAIN_LOOP. > >>> > >>> This will also get rid of ad-hoc code in the main event loop, which was > >>> trying to initialize the subsystem early and perform capsule updates. > >>> > >>> TODO: > >>> - The efi_tcg protocol implicitly initializes the TPM, as a result > >>> some of the tpm selftests will fail with the RFC. If everyone > >>> agrees that this is a good idea, I'll clean up the TPM hacks as well > >>> - We still need to run capsule updates on the main_loop() code since > >>> in some cases (e.g sandbox) we need preboot commands. > >>> - wider tests, I've only run QEMU for now > >>> > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >> > >> > >> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. > >> > >> Please, put the Kconfig related capsule change into a separate patch. > >> > >> Otherwise looks good to me. > >> > > > > I am OK with this change too. > > > > Two points from my side: > > > > 1. The main loop capsule update is (still) a mistake, unfortunately. > > It should be a command which is run on boot. For sandbox testing, that > > command should be run *without* rebooting. I am sure I asked for that > > Capsule updates must run outside of any command as this is required by > the UEFI specification. What do you mean? Does the specification mention U-Boot commands? > > Capsule updates require a reboot and the sandbox must behave as a > regular system so that we can run the same tests on all systems. How can I get you past this thinking? We need to 'design for test'. The current test is a mess, sorry. Perhaps we could have a call to go through it? > > > at the time but for various reasons it didn't happen. Please can you > > make that change also? > > > > 2. EFI should not be maintaining its own separate data structures, but > > should keep them attached to driver model. They should be created as > > needed, dynamically, not all at the start. Is anyone looking at this? > > I am happy to help suggest initial target for this refactoring. > > We have started with such a link for block devices. Once those block > devices are really UEFI compliant we should refactor the other devices > currently supported by EFI sub-system: > > UART, network, RNG, video, TPM. > > Some initialization like setting up UEFI variables will not have any > equivalence in the driver model. One way to handle that is to: - set up 'global' EFI structures attached to an EFI uclass - set up 'per device' EFI structures when the device is bound or probed Regards, Simon
Hi Simon, Heinrich On Fri, Jan 20, 2023 at 03:11:13PM -0700, Simon Glass wrote: > Hi Heinrich, > > On Fri, 20 Jan 2023 at 13:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 1/20/23 20:19, Simon Glass wrote: > > > Hi, > > > > > > On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > >>> Up to now the EFI subsystem was left out of the main U-Boot init > > >>> process. This has led to various hacks over the years, with the most > > >>> notable one being sprinkling around the efi init call to various places > > >>> such as U-Boot commands, the early boot code etc. > > >>> > > >>> Since EFI has it's own Kconfig option and people can remove it, let's > > >>> wire up the EFI init call on an event for EVT_MAIN_LOOP. > > >>> > > >>> This will also get rid of ad-hoc code in the main event loop, which was > > >>> trying to initialize the subsystem early and perform capsule updates. > > >>> > > >>> TODO: > > >>> - The efi_tcg protocol implicitly initializes the TPM, as a result > > >>> some of the tpm selftests will fail with the RFC. If everyone > > >>> agrees that this is a good idea, I'll clean up the TPM hacks as well > > >>> - We still need to run capsule updates on the main_loop() code since > > >>> in some cases (e.g sandbox) we need preboot commands. > > >>> - wider tests, I've only run QEMU for now > > >>> > > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > >> > > >> > > >> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. > > >> > > >> Please, put the Kconfig related capsule change into a separate patch. > > >> > > >> Otherwise looks good to me. > > >> > > > > > > I am OK with this change too. > > > > > > Two points from my side: > > > > > > 1. The main loop capsule update is (still) a mistake, unfortunately. > > > It should be a command which is run on boot. For sandbox testing, that > > > command should be run *without* rebooting. I am sure I asked for that > > > > Capsule updates must run outside of any command as this is required by > > the UEFI specification. Yes it's still a mistake but we can't get rid of it easily. What I was going to try is add another event notifier which would run post boot. That would work and allow us to define events, after the preboot commands have executed. Simon there *is* a command do that. It's documented in [0]. The tl;dr is run: => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name> => efidebug boot next 0 => setenv -e -nv -bs -rt -v OsIndications =0x0000000000000004 => efidebug capsule disk-update The command exists for testing, but as Heinrich explains, we also need to test the automatic upgrade after a reboot since that's what the EFI specification expects. > > What do you mean? Does the specification mention U-Boot commands? > > > > > Capsule updates require a reboot and the sandbox must behave as a > > regular system so that we can run the same tests on all systems. > > How can I get you past this thinking? We need to 'design for test'. > The current test is a mess, sorry. Perhaps we could have a call to go > through it? > > > > > > at the time but for various reasons it didn't happen. Please can you > > > make that change also? > > > > > > 2. EFI should not be maintaining its own separate data structures, but > > > should keep them attached to driver model. They should be created as > > > needed, dynamically, not all at the start. Is anyone looking at this? > > > I am happy to help suggest initial target for this refactoring. This patch doesn't change anything wrt to structures or how it interacts with DM. That's a different topic, but since there has been no progress apart from block devices for a while, I'll start looking into it myself. FWIW I agree we should refactor the protocol registration and match what U-Boot does with the rest of the DM. > > > > We have started with such a link for block devices. Once those block > > devices are really UEFI compliant we should refactor the other devices > > currently supported by EFI sub-system: > > > > UART, network, RNG, video, TPM. > > > > Some initialization like setting up UEFI variables will not have any > > equivalence in the driver model. > > One way to handle that is to: > > - set up 'global' EFI structures attached to an EFI uclass > - set up 'per device' EFI structures when the device is bound or probed > > Regards, > Simon Can we do this is small steps since I prefer testing everything thoroughly and making sure the EFI init doesn't break? So my plan is to resend this, fixing any TPM selftest regressions and then start cleaning up the protocol registration. [0] https://u-boot.readthedocs.io/en/v2021.04/board/emulation/qemu_capsule_update.html Regards /Ilias
On Mon, Jan 23, 2023 at 10:15:40AM +0200, Ilias Apalodimas wrote: > Hi Simon, Heinrich > > On Fri, Jan 20, 2023 at 03:11:13PM -0700, Simon Glass wrote: > > Hi Heinrich, > > > > On Fri, 20 Jan 2023 at 13:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 1/20/23 20:19, Simon Glass wrote: > > > > Hi, > > > > > > > > On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > >> > > > >> Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > >>> Up to now the EFI subsystem was left out of the main U-Boot init > > > >>> process. This has led to various hacks over the years, with the most > > > >>> notable one being sprinkling around the efi init call to various places > > > >>> such as U-Boot commands, the early boot code etc. > > > >>> > > > >>> Since EFI has it's own Kconfig option and people can remove it, let's > > > >>> wire up the EFI init call on an event for EVT_MAIN_LOOP. > > > >>> > > > >>> This will also get rid of ad-hoc code in the main event loop, which was > > > >>> trying to initialize the subsystem early and perform capsule updates. > > > >>> > > > >>> TODO: > > > >>> - The efi_tcg protocol implicitly initializes the TPM, as a result > > > >>> some of the tpm selftests will fail with the RFC. If everyone > > > >>> agrees that this is a good idea, I'll clean up the TPM hacks as well > > > >>> - We still need to run capsule updates on the main_loop() code since > > > >>> in some cases (e.g sandbox) we need preboot commands. > > > >>> - wider tests, I've only run QEMU for now > > > >>> > > > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > >> > > > >> > > > >> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early. > > > >> > > > >> Please, put the Kconfig related capsule change into a separate patch. > > > >> > > > >> Otherwise looks good to me. > > > >> > > > > > > > > I am OK with this change too. > > > > > > > > Two points from my side: > > > > > > > > 1. The main loop capsule update is (still) a mistake, unfortunately. > > > > It should be a command which is run on boot. For sandbox testing, that > > > > command should be run *without* rebooting. I am sure I asked for that > > > > > > Capsule updates must run outside of any command as this is required by > > > the UEFI specification. > > Yes it's still a mistake but we can't get rid of it easily. What I was I don't think it's a mistake. When I implemented the feature, update_tftp() was already placed in the main loop. I followed this tradition. > going to try is add another event notifier which would run post boot. That > would work and allow us to define events, after the preboot commands have > executed. It's an idea, but it's a matter of machanism, but not a matter of timing. -Takahiro Akashi > Simon there *is* a command do that. It's documented in [0]. The tl;dr is > run: > => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name> > => efidebug boot next 0 > => setenv -e -nv -bs -rt -v OsIndications =0x0000000000000004 > => efidebug capsule disk-update > > The command exists for testing, but as Heinrich explains, we also need to > test the automatic upgrade after a reboot since that's what the EFI > specification expects. > > > > > What do you mean? Does the specification mention U-Boot commands? > > > > > > > > Capsule updates require a reboot and the sandbox must behave as a > > > regular system so that we can run the same tests on all systems. > > > > How can I get you past this thinking? We need to 'design for test'. > > The current test is a mess, sorry. Perhaps we could have a call to go > > through it? > > > > > > > > > at the time but for various reasons it didn't happen. Please can you > > > > make that change also? > > > > > > > > 2. EFI should not be maintaining its own separate data structures, but > > > > should keep them attached to driver model. They should be created as > > > > needed, dynamically, not all at the start. Is anyone looking at this? > > > > I am happy to help suggest initial target for this refactoring. > > This patch doesn't change anything wrt to structures or how it interacts > with DM. That's a different topic, but since there has been no progress > apart from block devices for a while, I'll start looking into it myself. > FWIW I agree we should refactor the protocol registration and match what > U-Boot does with the rest of the DM. > > > > > > > We have started with such a link for block devices. Once those block > > > devices are really UEFI compliant we should refactor the other devices > > > currently supported by EFI sub-system: > > > > > > UART, network, RNG, video, TPM. > > > > > > Some initialization like setting up UEFI variables will not have any > > > equivalence in the driver model. > > > > One way to handle that is to: > > > > - set up 'global' EFI structures attached to an EFI uclass > > - set up 'per device' EFI structures when the device is bound or probed > > > > Regards, > > Simon > > Can we do this is small steps since I prefer testing everything > thoroughly and making sure the EFI init doesn't break? > So my plan is to resend this, fixing any TPM selftest regressions and then > start cleaning up the protocol registration. > > [0] https://u-boot.readthedocs.io/en/v2021.04/board/emulation/qemu_capsule_update.html > > Regards > /Ilias
diff --git a/boot/bootm_os.c b/boot/bootm_os.c index 99ff0e6c02d2..4775cecb4e64 100644 --- a/boot/bootm_os.c +++ b/boot/bootm_os.c @@ -505,14 +505,6 @@ static int do_bootm_efi(int flag, int argc, char *const argv[], if (ret) return ret; - /* Initialize EFI drivers */ - efi_ret = efi_init_obj_list(); - if (efi_ret != EFI_SUCCESS) { - printf("## Failed to initialize UEFI sub-system: r = %lu\n", - efi_ret & ~EFI_ERROR_MASK); - return 1; - } - /* Install device tree */ efi_ret = efi_install_fdt(images->ft_len ? images->ft_addr : EFI_FDT_USE_INTERNAL); diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 2a7d42925d65..5d56448ee6ce 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -668,14 +668,6 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc, if (argc < 2) return CMD_RET_USAGE; - /* Initialize EFI drivers */ - ret = efi_init_obj_list(); - if (ret != EFI_SUCCESS) { - log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", - ret & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - if (argc > 2) { uintptr_t fdt_addr; diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index 3236ca5d7994..1c604c79e01d 100644 --- a/cmd/bootmenu.c +++ b/cmd/bootmenu.c @@ -449,15 +449,6 @@ static void handle_uefi_bootnext(void) efi_status_t ret; efi_uintn_t size; - /* Initialize EFI drivers */ - ret = efi_init_obj_list(); - if (ret != EFI_SUCCESS) { - log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", - ret & ~EFI_ERROR_MASK); - - return; - } - /* If UEFI BootNext variable is set, boot the BootNext load option */ size = sizeof(u16); ret = efi_get_variable_int(u"BootNext", diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index d830e4af53b8..16025d451901 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -2545,14 +2545,6 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a if (argc > 1) return CMD_RET_USAGE; - ret = efi_init_obj_list(); - if (ret != EFI_SUCCESS) { - log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", - ret & ~EFI_ERROR_MASK); - - return CMD_RET_FAILURE; - } - ret = eficonfig_init(); if (ret != EFI_SUCCESS) return CMD_RET_FAILURE; diff --git a/cmd/efidebug.c b/cmd/efidebug.c index e6959ede930f..9ad253da91b2 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -1464,21 +1464,12 @@ static int do_efidebug(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct cmd_tbl *cp; - efi_status_t r; if (argc < 2) return CMD_RET_USAGE; argc--; argv++; - /* Initialize UEFI drivers */ - r = efi_init_obj_list(); - if (r != EFI_SUCCESS) { - printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", - r & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - cp = find_cmd_tbl(argv[0], cmd_efidebug_sub, ARRAY_SIZE(cmd_efidebug_sub)); if (!cp) diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c index 24944ab81e23..1ac1114d4c89 100644 --- a/cmd/nvedit_efi.c +++ b/cmd/nvedit_efi.c @@ -210,15 +210,6 @@ int do_env_print_efi(struct cmd_tbl *cmdtp, int flag, int argc, const efi_guid_t *guid_p = NULL; efi_guid_t guid; bool verbose = true; - efi_status_t ret; - - /* Initialize EFI drivers */ - ret = efi_init_obj_list(); - if (ret != EFI_SUCCESS) { - printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", - ret & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { if (!strcmp(argv[0], "-guid")) { @@ -388,14 +379,6 @@ int do_env_set_efi(struct cmd_tbl *cmdtp, int flag, int argc, if (argc == 1) return CMD_RET_USAGE; - /* Initialize EFI drivers */ - ret = efi_init_obj_list(); - if (ret != EFI_SUCCESS) { - printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", - ret & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - /* * attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | * EFI_VARIABLE_RUNTIME_ACCESS; diff --git a/common/main.c b/common/main.c index 682f3359ea38..edb5f4c20682 100644 --- a/common/main.c +++ b/common/main.c @@ -54,11 +54,9 @@ void main_loop(void) if (IS_ENABLED(CONFIG_UPDATE_TFTP)) update_tftp(0UL, NULL, NULL); - if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) { - /* efi_init_early() already called */ - if (efi_init_obj_list() == EFI_SUCCESS) - efi_launch_capsules(); - } + /* EFI subsystem will be initialized from an event */ + if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK)) + efi_launch_capsules(); s = bootdelay_process(); if (cli_process_fdt(&s)) diff --git a/include/efi_loader.h b/include/efi_loader.h index f9e427f09059..3a801cbbc4c1 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -513,8 +513,6 @@ extern struct list_head efi_register_notify_events; /* called at pre-initialization */ int efi_init_early(void); -/* Initialize efi execution environment */ -efi_status_t efi_init_obj_list(void); /* Set up console modes */ void efi_setup_console_size(void); /* Install device tree */ diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index b630d88ef9e2..f3aaa51bb36a 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -153,16 +153,6 @@ config EFI_IGNORE_OSINDICATIONS without setting the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED flag in variable OsIndications. -config EFI_CAPSULE_ON_DISK_EARLY - bool "Initiate capsule-on-disk at U-Boot boottime" - depends on EFI_CAPSULE_ON_DISK - help - Normally, without this option enabled, capsules will be - executed only at the first time of invoking one of efi command. - If this option is enabled, capsules will be enforced to be - executed as part of U-Boot initialisation so that they will - surely take place whatever is set to distro_bootcmd. - config EFI_CAPSULE_FIRMWARE bool diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index f0f01d3b1d5a..e2317aed1ec2 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -215,7 +215,7 @@ out: * * Return: status code */ -efi_status_t efi_init_obj_list(void) +static efi_status_t efi_init_obj_list(void) { efi_status_t ret = EFI_SUCCESS; @@ -335,14 +335,25 @@ efi_status_t efi_init_obj_list(void) /* Initialize EFI runtime services */ ret = efi_reset_system_init(); - if (ret != EFI_SUCCESS) - goto out; - /* Execute capsules after reboot */ - if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) && - !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) - ret = efi_launch_capsules(); out: efi_obj_list_initialized = ret; return ret; } + +static int efi_evt_int(void *ctx, struct event *event) +{ + efi_status_t ret; + int r = 0; + + ret = efi_init_obj_list(); + if (ret != EFI_SUCCESS) { + log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", + ret & ~EFI_ERROR_MASK); + r = -1; + } + + return r; +} + +EVENT_SPY(EVT_MAIN_LOOP, efi_evt_int); diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig index 78759e6618df..6ff577cb53b3 100644 --- a/lib/fwu_updates/Kconfig +++ b/lib/fwu_updates/Kconfig @@ -3,7 +3,6 @@ config FWU_MULTI_BANK_UPDATE depends on EFI_CAPSULE_ON_DISK select PARTITION_TYPE_GUID select EFI_SETUP_EARLY - imply EFI_CAPSULE_ON_DISK_EARLY select EVENT help Feature for updating firmware images on platforms having diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index 5313d0730202..e4256bea7a2c 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -700,9 +700,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event) return 0; } - if (efi_init_obj_list() != EFI_SUCCESS) - return 0; - ret = fwu_get_dev_mdata(&dev, &mdata); if (ret) return ret;
Up to now the EFI subsystem was left out of the main U-Boot init process. This has led to various hacks over the years, with the most notable one being sprinkling around the efi init call to various places such as U-Boot commands, the early boot code etc. Since EFI has it's own Kconfig option and people can remove it, let's wire up the EFI init call on an event for EVT_MAIN_LOOP. This will also get rid of ad-hoc code in the main event loop, which was trying to initialize the subsystem early and perform capsule updates. TODO: - The efi_tcg protocol implicitly initializes the TPM, as a result some of the tpm selftests will fail with the RFC. If everyone agrees that this is a good idea, I'll clean up the TPM hacks as well - We still need to run capsule updates on the main_loop() code since in some cases (e.g sandbox) we need preboot commands. - wider tests, I've only run QEMU for now Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- boot/bootm_os.c | 8 -------- cmd/bootefi.c | 8 -------- cmd/bootmenu.c | 9 --------- cmd/eficonfig.c | 8 -------- cmd/efidebug.c | 9 --------- cmd/nvedit_efi.c | 17 ----------------- common/main.c | 8 +++----- include/efi_loader.h | 2 -- lib/efi_loader/Kconfig | 10 ---------- lib/efi_loader/efi_setup.c | 25 ++++++++++++++++++------- lib/fwu_updates/Kconfig | 1 - lib/fwu_updates/fwu.c | 3 --- 12 files changed, 21 insertions(+), 87 deletions(-) -- 2.38.1