[PATCH v2 0/2] efi_loader: indicating capsule update results

[PATCH v2 0/2] efi_loader: indicating capsule update results

[Heinrich Schuchardt]

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

[PATCH v2 1/2] efi_loader: fix set_capsule_result()

[Heinrich Schuchardt]

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

[PATCH v2 2/2] efi_loader: clear OsIndications

[Heinrich Schuchardt]

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

Re: [PATCH v2 2/2] efi_loader: clear OsIndications

[Ilias Apalodimas]

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
> 

Re: [PATCH v2 1/2] efi_loader: fix set_capsule_result()

[AKASHI Takahiro]

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
> 

Re: [PATCH v2 1/2] efi_loader: fix set_capsule_result()

[Heinrich Schuchardt]

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
>> 

Re: [PATCH v2 1/2] efi_loader: fix set_capsule_result()

[AKASHI Takahiro]

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
> >> 
>