| Message ID | 20220531071535.219661-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | EFI: Miscellaneous capsule update fixes | expand |
On 5/31/22 09:15, Sughosh Ganu wrote: > The EFI_IGNORE_OSINDICATIONS config symbol was introduced as a > mechanism to have capsule updates work even on platforms where the > SetVariable runtime service was not supported. The current logic > requires the OsIndications variable to have been set to a 64 bit value > even when the EFI_IGNORE_OSINDICATIONS config is enabled. Move the > check to see if the config symbol is enabled at the beginning of the > function. If the config is enabled, return a success code without > checking for the existence of the variable. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > lib/efi_loader/efi_capsule.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index c76a5f3570..0ea21dc168 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void) > efi_uintn_t size; > efi_status_t r; > > + if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) > + return EFI_SUCCESS; > + If CONFIG_EFI_IGNORE_OSINDICATIONS=y it is correct to ignore a missing OsIndications variable. But I think we should stay as close as possible to the UEFI specification: If the variable exists and the flag EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED is set, we should remove the flag even if CONFIG_EFI_IGNORE_OSINDICATIONS=y. Please, have a look at the configuration symbols EFI_CAPSULE_ON_DISK_EARLY and EFI_SETUP_EARLY. As block devices now can be added to the UEFI sub-system after initialization of the same shouldn't we always initialize the sub-system early. And shouldn't we always consider capsules before distro-boot, U-Boot menu, or entering the U-Boot shell? Best regards Heinrich > size = sizeof(os_indications); > r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid, > NULL, &size, &os_indications, NULL); > @@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void) > if (r != EFI_SUCCESS) > log_err("Setting %ls failed\n", L"OsIndications"); > return EFI_SUCCESS; > - } else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) { > - return EFI_SUCCESS; > } else { > return EFI_NOT_FOUND; > }
hi Heinrich, On Wed, 1 Jun 2022 at 11:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 5/31/22 09:15, Sughosh Ganu wrote: > > The EFI_IGNORE_OSINDICATIONS config symbol was introduced as a > > mechanism to have capsule updates work even on platforms where the > > SetVariable runtime service was not supported. The current logic > > requires the OsIndications variable to have been set to a 64 bit value > > even when the EFI_IGNORE_OSINDICATIONS config is enabled. Move the > > check to see if the config symbol is enabled at the beginning of the > > function. If the config is enabled, return a success code without > > checking for the existence of the variable. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > lib/efi_loader/efi_capsule.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index c76a5f3570..0ea21dc168 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void) > > efi_uintn_t size; > > efi_status_t r; > > > > + if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) > > + return EFI_SUCCESS; > > + > > If CONFIG_EFI_IGNORE_OSINDICATIONS=y it is correct to ignore a missing > OsIndications variable. > > But I think we should stay as close as possible to the UEFI specification: > > If the variable exists and the flag > EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED is set, we should > remove the flag even if CONFIG_EFI_IGNORE_OSINDICATIONS=y. Okay. I get your point. Will make the change to not return EFI_NOT_FOUND when the OsIndications variable is not set with CONFIG_EFI_IGNORE_OSINDICATIONS=y. The subsequent code will clear the EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED bit if the variable is found irrespective of the value of the above config. > > Please, have a look at the configuration symbols > EFI_CAPSULE_ON_DISK_EARLY and EFI_SETUP_EARLY. > > As block devices now can be added to the UEFI sub-system after > initialization of the same shouldn't we always initialize the sub-system > early. And shouldn't we always consider capsules before distro-boot, > U-Boot menu, or entering the U-Boot shell? I believe that can be done by enabling CONFIG_EFI_CAPSULE_ON_DISK_EARLY. -sughosh > > Best regards > > Heinrich > > > size = sizeof(os_indications); > > r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid, > > NULL, &size, &os_indications, NULL); > > @@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void) > > if (r != EFI_SUCCESS) > > log_err("Setting %ls failed\n", L"OsIndications"); > > return EFI_SUCCESS; > > - } else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) { > > - return EFI_SUCCESS; > > } else { > > return EFI_NOT_FOUND; > > }
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index c76a5f3570..0ea21dc168 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void) efi_uintn_t size; efi_status_t r; + if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) + return EFI_SUCCESS; + size = sizeof(os_indications); r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid, NULL, &size, &os_indications, NULL); @@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void) if (r != EFI_SUCCESS) log_err("Setting %ls failed\n", L"OsIndications"); return EFI_SUCCESS; - } else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) { - return EFI_SUCCESS; } else { return EFI_NOT_FOUND; }
The EFI_IGNORE_OSINDICATIONS config symbol was introduced as a mechanism to have capsule updates work even on platforms where the SetVariable runtime service was not supported. The current logic requires the OsIndications variable to have been set to a 64 bit value even when the EFI_IGNORE_OSINDICATIONS config is enabled. Move the check to see if the config symbol is enabled at the beginning of the function. If the config is enabled, return a success code without checking for the existence of the variable. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- lib/efi_loader/efi_capsule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)