[PATCH v2 0/2] efi_loader: indicating capsule update results
When creating the Capsule#### variable we should immediately update CapsuleLast.
After each reboot we must clear flag EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED in variable OsIndications.
Heinrich Schuchardt (2): efi_loader: fix set_capsule_result() efi_loader: clear OsIndications
lib/efi_loader/efi_capsule.c | 39 +++++++++++++++++++----------------- lib/efi_loader/efi_setup.c | 33 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 18 deletions(-)
-- 2.30.2
The log category must be LOG_CATEGORY LOGC_EFI.
efi_set_variable() should be called with EFI_CALL(). Use efi_set_variable_int() instead.
A log text "Updating ..." if SetVariable() fails does not make sense for a variable that is not required to be preexisting.
CapsuleLast should always be immediately updated.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2: don't update OsIndications in set_capsule_result() update CapsuleLast immediately --- lib/efi_loader/efi_capsule.c | 39 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 2c37a0d97b..f87ef2a514 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -6,6 +6,8 @@ * Author: AKASHI Takahiro */
+#define LOG_CATEGORY LOGC_EFI + #include <common.h> #include <efi_loader.h> #include <efi_variable.h> @@ -95,13 +97,25 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule, else memset(&result.capsule_processed, 0, sizeof(time)); result.capsule_status = return_status; - ret = efi_set_variable(variable_name16, &efi_guid_capsule_report, - EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, - sizeof(result), &result); - if (ret) - log_err("EFI: creating %ls failed\n", variable_name16); + ret = efi_set_variable_int(variable_name16, &efi_guid_capsule_report, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, + sizeof(result), &result, false); + if (ret != EFI_SUCCESS) { + log_err("Setting %ls failed\n", variable_name16); + return; + } + + /* Variable CapsuleLast must not include terminating 0x0000 */ + ret = efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report, + EFI_VARIABLE_READ_ONLY | + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, + 22, variable_name16, false); + if (ret != EFI_SUCCESS) + log_err("Setting %ls failed\n", L"CapsuleLast"); }
#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT @@ -988,7 +1002,6 @@ efi_status_t efi_launch_capsules(void) struct efi_capsule_header *capsule = NULL; u16 **files; unsigned int nfiles, index, i; - u16 variable_name16[12]; efi_status_t ret;
if (!check_run_capsules()) @@ -1045,16 +1058,6 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- /* CapsuleLast */ - efi_create_indexed_name(variable_name16, sizeof(variable_name16), - "Capsule", index - 1); - efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report, - EFI_VARIABLE_READ_ONLY | - EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, - 22, variable_name16, false); - return ret; } #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ -- 2.30.2
NAK.
On Wed, Jun 30, 2021 at 05:31:15PM +0200, Heinrich Schuchardt wrote:
The log category must be LOG_CATEGORY LOGC_EFI.
efi_set_variable() should be called with EFI_CALL(). Use efi_set_variable_int() instead.
A log text "Updating ..." if SetVariable() fails does not make sense for a variable that is not required to be preexisting.
CapsuleLast should always be immediately updated.
As I said to your v1 in [1],
You are trying to fix several irrelevant issues here. Please split them into separate patches as you have always asked me before.
[1] https://lists.denx.de/pipermail/u-boot/2021-June/453148.html
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2: don't update OsIndications in set_capsule_result() update CapsuleLast immediately
lib/efi_loader/efi_capsule.c | 39 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 2c37a0d97b..f87ef2a514 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -6,6 +6,8 @@
Author: AKASHI Takahiro*/
+#define LOG_CATEGORY LOGC_EFI
#include <common.h> #include <efi_loader.h> #include <efi_variable.h> @@ -95,13 +97,25 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule, else memset(&result.capsule_processed, 0, sizeof(time)); result.capsule_status = return_status;
- ret = efi_set_variable(variable_name16, &efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result);- if (ret)
log_err("EFI: creating %ls failed\n", variable_name16);
- ret = efi_set_variable_int(variable_name16, &efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result, false);- if (ret != EFI_SUCCESS) {
log_err("Setting %ls failed\n", variable_name16);return;- }
- /* Variable CapsuleLast must not include terminating 0x0000 */
- ret = efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- if (ret != EFI_SUCCESS)
log_err("Setting %ls failed\n", L"CapsuleLast");}
#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT @@ -988,7 +1002,6 @@ efi_status_t efi_launch_capsules(void) struct efi_capsule_header *capsule = NULL; u16 **files; unsigned int nfiles, index, i;
u16 variable_name16[12]; efi_status_t ret;
if (!check_run_capsules())
@@ -1045,16 +1058,6 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- /* CapsuleLast */
- efi_create_indexed_name(variable_name16, sizeof(variable_name16),
"Capsule", index - 1);- efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- return ret;
}
#endif /* CONFIG_EFI_CAPSULE_ON_DISK */
2.30.2
Am 1. Juli 2021 02:49:09 MESZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
NAK.
On Wed, Jun 30, 2021 at 05:31:15PM +0200, Heinrich Schuchardt wrote:
The log category must be LOG_CATEGORY LOGC_EFI.
efi_set_variable() should be called with EFI_CALL(). Use efi_set_variable_int() instead.
A log text "Updating ..." if SetVariable() fails does not make sense
for a
variable that is not required to be preexisting.
CapsuleLast should always be immediately updated.
As I said to your v1 in [1],
You are trying to fix several irrelevant issues here.
Why do you think the changes are irrelevant?
Please split them into separate patches as you have always asked me before.
Splitting does not change relevance.
Do you want to play tit for tat?
Best regards
Heinrich
[1] https://lists.denx.de/pipermail/u-boot/2021-June/453148.html
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2: don't update OsIndications in set_capsule_result() update CapsuleLast immediately
lib/efi_loader/efi_capsule.c | 39
+++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c
b/lib/efi_loader/efi_capsule.c
index 2c37a0d97b..f87ef2a514 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -6,6 +6,8 @@
Author: AKASHI Takahiro*/
+#define LOG_CATEGORY LOGC_EFI
#include <common.h> #include <efi_loader.h> #include <efi_variable.h> @@ -95,13 +97,25 @@ void set_capsule_result(int index, struct
efi_capsule_header *capsule,
else memset(&result.capsule_processed, 0, sizeof(time)); result.capsule_status = return_status;
- ret = efi_set_variable(variable_name16, &efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result);- if (ret)
log_err("EFI: creating %ls failed\n", variable_name16);
- ret = efi_set_variable_int(variable_name16,
&efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result, false);- if (ret != EFI_SUCCESS) {
log_err("Setting %ls failed\n", variable_name16);return;- }
- /* Variable CapsuleLast must not include terminating 0x0000 */
- ret = efi_set_variable_int(L"CapsuleLast",
&efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- if (ret != EFI_SUCCESS)
log_err("Setting %ls failed\n", L"CapsuleLast");}
#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT @@ -988,7 +1002,6 @@ efi_status_t efi_launch_capsules(void) struct efi_capsule_header *capsule = NULL; u16 **files; unsigned int nfiles, index, i;
u16 variable_name16[12]; efi_status_t ret;
if (!check_run_capsules())
@@ -1045,16 +1058,6 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- /* CapsuleLast */
- efi_create_indexed_name(variable_name16, sizeof(variable_name16),
"Capsule", index - 1);- efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- return ret;
}
#endif /* CONFIG_EFI_CAPSULE_ON_DISK */
2.30.2
On Thu, Jul 01, 2021 at 03:20:48AM +0200, Heinrich Schuchardt wrote:
Am 1. Juli 2021 02:49:09 MESZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
NAK.
For example,
On Wed, Jun 30, 2021 at 05:31:15PM +0200, Heinrich Schuchardt wrote:
The log category must be LOG_CATEGORY LOGC_EFI.
This one above and
efi_set_variable() should be called with EFI_CALL(). Use efi_set_variable_int() instead.
and this are mutually irrelevant.
A log text "Updating ..." if SetVariable() fails does not make sense
for a
variable that is not required to be preexisting.
This change is also irrelevant.
CapsuleLast should always be immediately updated.
As I said to your v1 in [1],
You are trying to fix several irrelevant issues here.
Why do you think the changes are irrelevant?
No question.
-Takahiro Akashi
Please split them into separate patches as you have always asked me before.
Splitting does not change relevance.
Do you want to play tit for tat?
Best regards
Heinrich
[1] https://lists.denx.de/pipermail/u-boot/2021-June/453148.html
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2: don't update OsIndications in set_capsule_result() update CapsuleLast immediately
lib/efi_loader/efi_capsule.c | 39
+++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c
b/lib/efi_loader/efi_capsule.c
index 2c37a0d97b..f87ef2a514 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -6,6 +6,8 @@
Author: AKASHI Takahiro*/
+#define LOG_CATEGORY LOGC_EFI
#include <common.h> #include <efi_loader.h> #include <efi_variable.h> @@ -95,13 +97,25 @@ void set_capsule_result(int index, struct
efi_capsule_header *capsule,
else memset(&result.capsule_processed, 0, sizeof(time)); result.capsule_status = return_status;
- ret = efi_set_variable(variable_name16, &efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result);- if (ret)
log_err("EFI: creating %ls failed\n", variable_name16);
- ret = efi_set_variable_int(variable_name16,
&efi_guid_capsule_report,
EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,sizeof(result), &result, false);- if (ret != EFI_SUCCESS) {
log_err("Setting %ls failed\n", variable_name16);return;- }
- /* Variable CapsuleLast must not include terminating 0x0000 */
- ret = efi_set_variable_int(L"CapsuleLast",
&efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- if (ret != EFI_SUCCESS)
log_err("Setting %ls failed\n", L"CapsuleLast");}
#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT @@ -988,7 +1002,6 @@ efi_status_t efi_launch_capsules(void) struct efi_capsule_header *capsule = NULL; u16 **files; unsigned int nfiles, index, i;
u16 variable_name16[12]; efi_status_t ret;
if (!check_run_capsules())
@@ -1045,16 +1058,6 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files);
- /* CapsuleLast */
- efi_create_indexed_name(variable_name16, sizeof(variable_name16),
"Capsule", index - 1);- efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
EFI_VARIABLE_READ_ONLY |EFI_VARIABLE_NON_VOLATILE |EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS,22, variable_name16, false);- return ret;
}
#endif /* CONFIG_EFI_CAPSULE_ON_DISK */
2.30.2
After each reboot we must clear flag EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED in variable OsIndications.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2: update OsIndications after handling all capsules --- lib/efi_loader/efi_setup.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 3c5cf9a435..ca865c5a99 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -5,9 +5,12 @@ * Copyright (c) 2016-2018 Alexander Graf et al. */
+#define LOG_CATEGORY LOGC_EFI + #include <common.h> #include <efi_loader.h> #include <efi_variable.h> +#include <log.h>
#define OBJ_LIST_NOT_INITIALIZED 1
@@ -171,6 +174,32 @@ static efi_status_t efi_init_os_indications(void) &os_indications_supported, false); }
+static efi_status_t efi_clear_os_indications(void) +{ + efi_uintn_t size; + u64 os_indications; + efi_status_t ret; + + /* Clear EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED */ + size = sizeof(os_indications); + ret = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid, + NULL, &size, &os_indications, NULL); + if (ret != EFI_SUCCESS) + os_indications = 0; + else + os_indications &= + ~EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED; + ret = efi_set_variable_int(L"OsIndications", &efi_global_variable_guid, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, + sizeof(os_indications), &os_indications, + false); + if (ret != EFI_SUCCESS) + log_err("Setting %ls failed\n", L"OsIndications"); + return EFI_SUCCESS; +} + /** * efi_init_obj_list() - Initialize and populate EFI object list * @@ -291,6 +320,10 @@ efi_status_t efi_init_obj_list(void) if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) && !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) ret = efi_launch_capsules(); + if (ret != EFI_SUCCESS) + goto out; + + ret = efi_clear_os_indications(); out: efi_obj_list_initialized = ret; return ret; -- 2.30.2
Hi Heinrich,
if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
[...]
!IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) ret = efi_launch_capsules();
- if (ret != EFI_SUCCESS)
goto out;
I think OsIndications should be cleared reagrdless of the capsuleupdate result. There's a detailed explanation on your v1
Cheers Ilias
- ret = efi_clear_os_indications();
out: efi_obj_list_initialized = ret; return ret; -- 2.30.2
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Ilias Apalodimas