[PATCH v2 0/2] EFI: Reset system after capsule-on-disk

[PATCH v2 0/2] EFI: Reset system after capsule-on-disk

[Masami Hiramatsu]

Hi,

Here is the patch to reset after capsule-on-disk. This version fixes some
bugs and remove kconfig for the reset (which uses panic).

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: Avoid using efi_update_capsule() from update capsule on disk
      efi_loader: Reset system after CapsuleUpdate on disk


 lib/efi_loader/efi_capsule.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

--
Masami Hiramatsu <masami.hiramatsu@linaro.org>

[PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Masami Hiramatsu]

The efi_update_capsule() may have to handle the capsule flags as an UEFI
runtime and boottime service, but the capsule-on-disk process doesn't.
Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
directly instead of efi_update_capsule().

Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 Changes in v2:
  - Fix to pass correct pointer to efi_capsule_update_firmware
  - Remove ESRT generation, because this part anyway will be removed
    next patch.
---
 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]);

[PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Add a config option to reset system 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 v2:
  - Remove kconfig option to disable this feature.
  - Use panic() instead of do_reset() so that if the reset fails,
    the machine halt.
  - Log the result of each capsule update always.
---
 lib/efi_loader/efi_capsule.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 1ec7ea29ff..39bce714f7 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
 		ret = efi_capsule_read_file(files[i], &capsule);
 		if (ret == EFI_SUCCESS) {
 			ret = efi_capsule_update_firmware(capsule);
-			if (ret != EFI_SUCCESS)
-				log_err("Applying capsule %ls failed\n",
-					files[i]);
+			log_err("Applying capsule %ls %s\n",
+				files[i],
+				ret == EFI_SUCCESS ? "succeeded" : "failed");
 
 			/* create CapsuleXXXX */
 			set_capsule_result(index, capsule, ret);
@@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
 		free(files[i]);
 	free(files);
 
+	/*
+	 * UEFI spec requires to reset system after complete processing capsule
+	 * update on the storage.
+	 */
+	panic("Reboot after firmware update");
+
 	return ret;
 }
 #endif /* CONFIG_EFI_CAPSULE_ON_DISK */

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[AKASHI Takahiro]

On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
> Add a config option to reset system 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 v2:
>   - Remove kconfig option to disable this feature.
>   - Use panic() instead of do_reset() so that if the reset fails,
>     the machine halt.
>   - Log the result of each capsule update always.
> ---
>  lib/efi_loader/efi_capsule.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 1ec7ea29ff..39bce714f7 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
>  		ret = efi_capsule_read_file(files[i], &capsule);
>  		if (ret == EFI_SUCCESS) {
>  			ret = efi_capsule_update_firmware(capsule);
> -			if (ret != EFI_SUCCESS)
> -				log_err("Applying capsule %ls failed\n",
> -					files[i]);
> +			log_err("Applying capsule %ls %s\n",
> +				files[i],
> +				ret == EFI_SUCCESS ? "succeeded" : "failed");

log_err()? log_info() is better, I think.

>  
>  			/* create CapsuleXXXX */
>  			set_capsule_result(index, capsule, ret);
> @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
>  		free(files[i]);
>  	free(files);
>  
> +	/*
> +	 * UEFI spec requires to reset system after complete processing capsule
> +	 * update on the storage.
> +	 */
> +	panic("Reboot after firmware update");

If CONFIG_PANIC_HANG is enabled, the system won't restart.
It's not what we want here.

-Takahiro Akashi

> +
>  	return ret;
>  }
>  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> 

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

hi Masami,

On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> runtime and boottime service, but the capsule-on-disk process doesn't.
> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> directly instead of efi_update_capsule().
>
> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  Changes in v2:
>   - Fix to pass correct pointer to efi_capsule_update_firmware
>   - Remove ESRT generation, because this part anyway will be removed
>     next patch.
> ---
>  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);

I believe this is not fixing any issue as such. If so, I would vote
for keeping the call to efi_update_capsule. With the FWU Multi Bank
feature enabled, the checks for capsule acceptance and revert are
being done in this function. The reason I have put this code in the
function is that it caters to both scenarios of capsule-on-disk and
the runtime functionality. In addition, the FWU bootup checks are also
done in this function through a call to fwu_update_checks_pass. So if
this is not a fix, which I don't think it is, I would prefer this call
to remain.

-sughosh

>                         if (ret != EFI_SUCCESS)
>                                 log_err("Applying capsule %ls failed\n",
>                                         files[i]);
>

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Heinrich Schuchardt]

Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
>hi Masami,
>
>On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
><masami.hiramatsu@linaro.org> wrote:
>>
>> The efi_update_capsule() may have to handle the capsule flags as an UEFI
>> runtime and boottime service, but the capsule-on-disk process doesn't.
>> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
>> directly instead of efi_update_capsule().
>>
>> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> ---
>>  Changes in v2:
>>   - Fix to pass correct pointer to efi_capsule_update_firmware
>>   - Remove ESRT generation, because this part anyway will be removed
>>     next patch.
>> ---
>>  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);
>
>I believe this is not fixing any issue as such. If so, I would vote
>for keeping the call to efi_update_capsule.

No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.

Best regards

Heinrich 

 With the FWU Multi Bank
>feature enabled, the checks for capsule acceptance and revert are
>being done in this function. The reason I have put this code in the
>function is that it caters to both scenarios of capsule-on-disk and
>the runtime functionality. In addition, the FWU bootup checks are also
>done in this function through a call to fwu_update_checks_pass. So if
>this is not a fix, which I don't think it is, I would prefer this call
>to remain.
>
>-sughosh
>
>>                         if (ret != EFI_SUCCESS)
>>                                 log_err("Applying capsule %ls failed\n",
>>                                         files[i]);
>>

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> >hi Masami,
> >
> >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> ><masami.hiramatsu@linaro.org> wrote:
> >>
> >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> >> runtime and boottime service, but the capsule-on-disk process doesn't.
> >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> >> directly instead of efi_update_capsule().
> >>
> >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >> ---
> >>  Changes in v2:
> >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> >>   - Remove ESRT generation, because this part anyway will be removed
> >>     next patch.
> >> ---
> >>  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);
> >
> >I believe this is not fixing any issue as such. If so, I would vote
> >for keeping the call to efi_update_capsule.
>
> No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.

Okay, in that case, I will put a check for the FWU Multi Banks feature
being enabled -- with the feature enabled, the call will be to
efi_update_capsule, and with the feature disabled, the call will be
made to efi_capsule_update_firmware. The compiler should compile out
the code whenever the FWU feature is disabled and that will not impact
the code size.

-sughosh

>
> Best regards
>
> Heinrich
>
>  With the FWU Multi Bank
> >feature enabled, the checks for capsule acceptance and revert are
> >being done in this function. The reason I have put this code in the
> >function is that it caters to both scenarios of capsule-on-disk and
> >the runtime functionality. In addition, the FWU bootup checks are also
> >done in this function through a call to fwu_update_checks_pass. So if
> >this is not a fix, which I don't think it is, I would prefer this call
> >to remain.
> >
> >-sughosh
> >
> >>                         if (ret != EFI_SUCCESS)
> >>                                 log_err("Applying capsule %ls failed\n",
> >>                                         files[i]);
> >>

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[AKASHI Takahiro]

On Tue, Feb 01, 2022 at 10:33:20PM +0530, Sughosh Ganu wrote:
> On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > >hi Masami,
> > >
> > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > ><masami.hiramatsu@linaro.org> wrote:
> > >>
> > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > >> directly instead of efi_update_capsule().
> > >>
> > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >> ---
> > >>  Changes in v2:
> > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > >>   - Remove ESRT generation, because this part anyway will be removed
> > >>     next patch.
> > >> ---
> > >>  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);
> > >
> > >I believe this is not fixing any issue as such. If so, I would vote
> > >for keeping the call to efi_update_capsule.
> >
> > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
> 
> Okay, in that case, I will put a check for the FWU Multi Banks feature
> being enabled -- with the feature enabled, the call will be to
> efi_update_capsule, and with the feature disabled, the call will be
> made to efi_capsule_update_firmware.

Please don't do that.
Instead, you should carve out a *common* function for UpdateCapsule api
and capsule-on-disk.
Please note, as I repeatedly said, that I didn't intend to implement
the API with my initial commits. I think I should not have added
efi_update_capsule() function to avoid any confusion.

-Takahiro Akashi

> The compiler should compile out
> the code whenever the FWU feature is disabled and that will not impact
> the code size.
> 
> -sughosh
> 
> >
> > Best regards
> >
> > Heinrich
> >
> >  With the FWU Multi Bank
> > >feature enabled, the checks for capsule acceptance and revert are
> > >being done in this function. The reason I have put this code in the
> > >function is that it caters to both scenarios of capsule-on-disk and
> > >the runtime functionality. In addition, the FWU bootup checks are also
> > >done in this function through a call to fwu_update_checks_pass. So if
> > >this is not a fix, which I don't think it is, I would prefer this call
> > >to remain.
> > >
> > >-sughosh
> > >
> > >>                         if (ret != EFI_SUCCESS)
> > >>                                 log_err("Applying capsule %ls failed\n",
> > >>                                         files[i]);
> > >>

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Masami Hiramatsu]

Hi Sughosh,

Could you tell me why do you need to do the FWU code in the efi_update_capsule?
If you need to add some logic to both of the efi_update_capsule API
and capsule-on-disk,
it is better to be implemented in the efi_capsule_update_firmware() as
a common part.
Or, make an independent additional function and call it from both path.
This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from
the capsule update logic itself.

Thank you,


2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>:
>
> On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > >hi Masami,
> > >
> > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > ><masami.hiramatsu@linaro.org> wrote:
> > >>
> > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > >> directly instead of efi_update_capsule().
> > >>
> > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >> ---
> > >>  Changes in v2:
> > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > >>   - Remove ESRT generation, because this part anyway will be removed
> > >>     next patch.
> > >> ---
> > >>  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);
> > >
> > >I believe this is not fixing any issue as such. If so, I would vote
> > >for keeping the call to efi_update_capsule.
> >
> > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
>
> Okay, in that case, I will put a check for the FWU Multi Banks feature
> being enabled -- with the feature enabled, the call will be to
> efi_update_capsule, and with the feature disabled, the call will be
> made to efi_capsule_update_firmware. The compiler should compile out
> the code whenever the FWU feature is disabled and that will not impact
> the code size.
>
> -sughosh
>
> >
> > Best regards
> >
> > Heinrich
> >
> >  With the FWU Multi Bank
> > >feature enabled, the checks for capsule acceptance and revert are
> > >being done in this function. The reason I have put this code in the
> > >function is that it caters to both scenarios of capsule-on-disk and
> > >the runtime functionality. In addition, the FWU bootup checks are also
> > >done in this function through a call to fwu_update_checks_pass. So if
> > >this is not a fix, which I don't think it is, I would prefer this call
> > >to remain.
> > >
> > >-sughosh
> > >
> > >>                         if (ret != EFI_SUCCESS)
> > >>                                 log_err("Applying capsule %ls failed\n",
> > >>                                         files[i]);
> > >>



-- 
Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Hi Takahiro,

2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>:

>
> On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
> > Add a config option to reset system 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 v2:
> >   - Remove kconfig option to disable this feature.
> >   - Use panic() instead of do_reset() so that if the reset fails,
> >     the machine halt.
> >   - Log the result of each capsule update always.
> > ---
> >  lib/efi_loader/efi_capsule.c |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 1ec7ea29ff..39bce714f7 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
> >               ret = efi_capsule_read_file(files[i], &capsule);
> >               if (ret == EFI_SUCCESS) {
> >                       ret = efi_capsule_update_firmware(capsule);
> > -                     if (ret != EFI_SUCCESS)
> > -                             log_err("Applying capsule %ls failed\n",
> > -                                     files[i]);
> > +                     log_err("Applying capsule %ls %s\n",
> > +                             files[i],
> > +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
>
> log_err()? log_info() is better, I think.

Hmm, would you think to use log_info() even if it is failed? Or should
we have log_err(failure) and log_info(success)?

>
> >
> >                       /* create CapsuleXXXX */
> >                       set_capsule_result(index, capsule, ret);
> > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
> >               free(files[i]);
> >       free(files);
> >
> > +     /*
> > +      * UEFI spec requires to reset system after complete processing capsule
> > +      * update on the storage.
> > +      */
> > +     panic("Reboot after firmware update");
>
> If CONFIG_PANIC_HANG is enabled, the system won't restart.
> It's not what we want here.

Indeed.
Heinrich, what would you think if do_reset() doesn't work?
(I think it is OK to get it back here, but needs a warning)

Thank you,

>
> -Takahiro Akashi
>
> > +
> >       return ret;
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> >



--
Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[AKASHI Takahiro]

On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote:
> Hi Takahiro,
> 
> 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> 
> >
> > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
> > > Add a config option to reset system 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 v2:
> > >   - Remove kconfig option to disable this feature.
> > >   - Use panic() instead of do_reset() so that if the reset fails,
> > >     the machine halt.
> > >   - Log the result of each capsule update always.
> > > ---
> > >  lib/efi_loader/efi_capsule.c |   12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 1ec7ea29ff..39bce714f7 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
> > >               ret = efi_capsule_read_file(files[i], &capsule);
> > >               if (ret == EFI_SUCCESS) {
> > >                       ret = efi_capsule_update_firmware(capsule);
> > > -                     if (ret != EFI_SUCCESS)
> > > -                             log_err("Applying capsule %ls failed\n",
> > > -                                     files[i]);
> > > +                     log_err("Applying capsule %ls %s\n",
> > > +                             files[i],
> > > +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
> >
> > log_err()? log_info() is better, I think.
> 
> Hmm, would you think to use log_info() even if it is failed? Or should
> we have log_err(failure) and log_info(success)?

It is what I meant :)

> >
> > >
> > >                       /* create CapsuleXXXX */
> > >                       set_capsule_result(index, capsule, ret);
> > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
> > >               free(files[i]);
> > >       free(files);
> > >
> > > +     /*
> > > +      * UEFI spec requires to reset system after complete processing capsule
> > > +      * update on the storage.
> > > +      */
> > > +     panic("Reboot after firmware update");
> >
> > If CONFIG_PANIC_HANG is enabled, the system won't restart.
> > It's not what we want here.
> 
> Indeed.
> Heinrich, what would you think if do_reset() doesn't work?
> (I think it is OK to get it back here, but needs a warning)

If (CONFIG_IS_ENABLED(SYSRESET)) {
    puts ("resetting ...\n");
    sysreset_reset_walk(SYSRESET_WARM);
} else {
    do_reset(...)
    halt();
}
/* not reach here */

-Takahiro Akashi


> Thank you,
> 
> >
> > -Takahiro Akashi
> >
> > > +
> > >       return ret;
> > >  }
> > >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> > >
> 
> 
> 
> --
> Masami Hiramatsu

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

On Wed, 2 Feb 2022 at 05:17, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Tue, Feb 01, 2022 at 10:33:20PM +0530, Sughosh Ganu wrote:
> > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > > >hi Masami,
> > > >
> > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > > ><masami.hiramatsu@linaro.org> wrote:
> > > >>
> > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > > >> directly instead of efi_update_capsule().
> > > >>
> > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >> ---
> > > >>  Changes in v2:
> > > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > > >>   - Remove ESRT generation, because this part anyway will be removed
> > > >>     next patch.
> > > >> ---
> > > >>  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);
> > > >
> > > >I believe this is not fixing any issue as such. If so, I would vote
> > > >for keeping the call to efi_update_capsule.
> > >
> > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
> >
> > Okay, in that case, I will put a check for the FWU Multi Banks feature
> > being enabled -- with the feature enabled, the call will be to
> > efi_update_capsule, and with the feature disabled, the call will be
> > made to efi_capsule_update_firmware.
>
> Please don't do that.
> Instead, you should carve out a *common* function for UpdateCapsule api
> and capsule-on-disk.

Can you also point out the issue you see with having the FWU checks in
the efi_update_capsule.  As I have said, having the checks here caters
to both the scenarios -- capsule-on-disk update as well as secure
world update. I think with the FWU feature enabled for secure world,
the efi_update_capsule function will get called, before branching off
to a different FMP.

> Please note, as I repeatedly said, that I didn't intend to implement
> the API with my initial commits. I think I should not have added
> efi_update_capsule() function to avoid any confusion.

Maybe I missed this, but I don't know why you think the
efi_update_capsule is superfluous. Also, if it really is superfluous,
this commit from Masami should also be removing the function
definition rather than just not calling the function.

-sughosh

>
> -Takahiro Akashi
>
> > The compiler should compile out
> > the code whenever the FWU feature is disabled and that will not impact
> > the code size.
> >
> > -sughosh
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > >  With the FWU Multi Bank
> > > >feature enabled, the checks for capsule acceptance and revert are
> > > >being done in this function. The reason I have put this code in the
> > > >function is that it caters to both scenarios of capsule-on-disk and
> > > >the runtime functionality. In addition, the FWU bootup checks are also
> > > >done in this function through a call to fwu_update_checks_pass. So if
> > > >this is not a fix, which I don't think it is, I would prefer this call
> > > >to remain.
> > > >
> > > >-sughosh
> > > >
> > > >>                         if (ret != EFI_SUCCESS)
> > > >>                                 log_err("Applying capsule %ls failed\n",
> > > >>                                         files[i]);
> > > >>

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

hi Masami,

On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Sughosh,
>
> Could you tell me why do you need to do the FWU code in the efi_update_capsule?

I thought I explained this in my previous email. Putting the FWU
checks in efi_update_capsule caters to the scenario where FWU updates
are being done in secure world. Even for such scenario, the
efi_update_capsule function will get called. So having the checks in
one single place is better.

> If you need to add some logic to both of the efi_update_capsule API
> and capsule-on-disk,
> it is better to be implemented in the efi_capsule_update_firmware() as
> a common part.
> Or, make an independent additional function and call it from both path.
> This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from
> the capsule update logic itself.

Like I asked Takahiro, I don't understand why you find the
efi_update_capsule function superfluous. I do see it being called for
secure world FWU updates. Also, if the function is indeed superfluous,
you should also be removing the function definition as well as part of
this patch.

-sughosh

>
> Thank you,
>
>
> 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>:
> >
> > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > > >hi Masami,
> > > >
> > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > > ><masami.hiramatsu@linaro.org> wrote:
> > > >>
> > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > > >> directly instead of efi_update_capsule().
> > > >>
> > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >> ---
> > > >>  Changes in v2:
> > > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > > >>   - Remove ESRT generation, because this part anyway will be removed
> > > >>     next patch.
> > > >> ---
> > > >>  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);
> > > >
> > > >I believe this is not fixing any issue as such. If so, I would vote
> > > >for keeping the call to efi_update_capsule.
> > >
> > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
> >
> > Okay, in that case, I will put a check for the FWU Multi Banks feature
> > being enabled -- with the feature enabled, the call will be to
> > efi_update_capsule, and with the feature disabled, the call will be
> > made to efi_capsule_update_firmware. The compiler should compile out
> > the code whenever the FWU feature is disabled and that will not impact
> > the code size.
> >
> > -sughosh
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > >  With the FWU Multi Bank
> > > >feature enabled, the checks for capsule acceptance and revert are
> > > >being done in this function. The reason I have put this code in the
> > > >function is that it caters to both scenarios of capsule-on-disk and
> > > >the runtime functionality. In addition, the FWU bootup checks are also
> > > >done in this function through a call to fwu_update_checks_pass. So if
> > > >this is not a fix, which I don't think it is, I would prefer this call
> > > >to remain.
> > > >
> > > >-sughosh
> > > >
> > > >>                         if (ret != EFI_SUCCESS)
> > > >>                                 log_err("Applying capsule %ls failed\n",
> > > >>                                         files[i]);
> > > >>
>
>
>
> --
> Masami Hiramatsu

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Masami Hiramatsu]

Hi Sughosh,

2022年2月2日(水) 14:35 Sughosh Ganu <sughosh.ganu@linaro.org>:
>
> hi Masami,
>
> On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > Could you tell me why do you need to do the FWU code in the efi_update_capsule?
>
> I thought I explained this in my previous email. Putting the FWU
> checks in efi_update_capsule caters to the scenario where FWU updates
> are being done in secure world. Even for such scenario, the
> efi_update_capsule function will get called. So having the checks in
> one single place is better.

Hmm, I'm not so sure the process flow of when the FWU update are
being done in secure world. What will happen?

[OS] -> [UEFI UpdateCapsule()] -(SMC)> [secure FWU] -> [update firmware] ?

Or,

[OS]  -(SMC)> [secure FWU] -> [UEFI UpdateCapsule()] -> [update firmware] ?

And anyway, if the FWU is done in secure world, will the FWU metadata
be processed in the secure world too? (in this case, U-boot may not do
anything about firmware update but just an interface, right?)

>
> > If you need to add some logic to both of the efi_update_capsule API
> > and capsule-on-disk,
> > it is better to be implemented in the efi_capsule_update_firmware() as
> > a common part.
> > Or, make an independent additional function and call it from both path.
> > This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from
> > the capsule update logic itself.
>
> Like I asked Takahiro, I don't understand why you find the
> efi_update_capsule function superfluous. I do see it being called for
> secure world FWU updates. Also, if the function is indeed superfluous,
> you should also be removing the function definition as well as part of
> this patch.

We don't said that the efi_update_capsule() is superfluous, but it has
a different role (e.g. processing multiple capsules and handle the
capsule flags) as UpdateCapsule() UEFI service API, which is defined
in UEFI spec. This means we will allow user to run CapsuleApp.efi on
U-Boot.

If it has to call secure world for FWU, I think that should be done in the
efi_update_capsule_firmware(), so that that is called from *both* of
UpdateCapsule() API and Capsule-on-disk.

Thank you,

>
> -sughosh
>
> >
> > Thank you,
> >
> >
> > 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>:
> > >
> > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > > > >hi Masami,
> > > > >
> > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > > > ><masami.hiramatsu@linaro.org> wrote:
> > > > >>
> > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > > > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > > > >> directly instead of efi_update_capsule().
> > > > >>
> > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > >> ---
> > > > >>  Changes in v2:
> > > > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > > > >>   - Remove ESRT generation, because this part anyway will be removed
> > > > >>     next patch.
> > > > >> ---
> > > > >>  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);
> > > > >
> > > > >I believe this is not fixing any issue as such. If so, I would vote
> > > > >for keeping the call to efi_update_capsule.
> > > >
> > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
> > >
> > > Okay, in that case, I will put a check for the FWU Multi Banks feature
> > > being enabled -- with the feature enabled, the call will be to
> > > efi_update_capsule, and with the feature disabled, the call will be
> > > made to efi_capsule_update_firmware. The compiler should compile out
> > > the code whenever the FWU feature is disabled and that will not impact
> > > the code size.
> > >
> > > -sughosh
> > >
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > >  With the FWU Multi Bank
> > > > >feature enabled, the checks for capsule acceptance and revert are
> > > > >being done in this function. The reason I have put this code in the
> > > > >function is that it caters to both scenarios of capsule-on-disk and
> > > > >the runtime functionality. In addition, the FWU bootup checks are also
> > > > >done in this function through a call to fwu_update_checks_pass. So if
> > > > >this is not a fix, which I don't think it is, I would prefer this call
> > > > >to remain.
> > > > >
> > > > >-sughosh
> > > > >
> > > > >>                         if (ret != EFI_SUCCESS)
> > > > >>                                 log_err("Applying capsule %ls failed\n",
> > > > >>                                         files[i]);
> > > > >>
> >
> >
> >
> > --
> > Masami Hiramatsu



-- 
Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Hi Takahiro,

2022年2月2日(水) 13:15 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote:
> > Hi Takahiro,
> >
> > 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >
> > >
> > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
> > > > Add a config option to reset system 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 v2:
> > > >   - Remove kconfig option to disable this feature.
> > > >   - Use panic() instead of do_reset() so that if the reset fails,
> > > >     the machine halt.
> > > >   - Log the result of each capsule update always.
> > > > ---
> > > >  lib/efi_loader/efi_capsule.c |   12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > index 1ec7ea29ff..39bce714f7 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
> > > >               ret = efi_capsule_read_file(files[i], &capsule);
> > > >               if (ret == EFI_SUCCESS) {
> > > >                       ret = efi_capsule_update_firmware(capsule);
> > > > -                     if (ret != EFI_SUCCESS)
> > > > -                             log_err("Applying capsule %ls failed\n",
> > > > -                                     files[i]);
> > > > +                     log_err("Applying capsule %ls %s\n",
> > > > +                             files[i],
> > > > +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
> > >
> > > log_err()? log_info() is better, I think.
> >
> > Hmm, would you think to use log_info() even if it is failed? Or should
> > we have log_err(failure) and log_info(success)?
>
> It is what I meant :)

OK.


> > > >
> > > >                       /* create CapsuleXXXX */
> > > >                       set_capsule_result(index, capsule, ret);
> > > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
> > > >               free(files[i]);
> > > >       free(files);
> > > >
> > > > +     /*
> > > > +      * UEFI spec requires to reset system after complete processing capsule
> > > > +      * update on the storage.
> > > > +      */
> > > > +     panic("Reboot after firmware update");
> > >
> > > If CONFIG_PANIC_HANG is enabled, the system won't restart.
> > > It's not what we want here.
> >
> > Indeed.
> > Heinrich, what would you think if do_reset() doesn't work?
> > (I think it is OK to get it back here, but needs a warning)
>
> If (CONFIG_IS_ENABLED(SYSRESET)) {
>     puts ("resetting ...\n");
>     sysreset_reset_walk(SYSRESET_WARM);
> } else {
>     do_reset(...)
>     halt();
> }
> /* not reach here */

OK, and in both case we should we puts() some messages before reboot, right?

Thank you,

>
> -Takahiro Akashi
>
>
> > Thank you,
> >
> > >
> > > -Takahiro Akashi
> > >
> > > > +
> > > >       return ret;
> > > >  }
> > > >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> > > >
> >
> >
> >
> > --
> > Masami Hiramatsu



--
Masami Hiramatsu

Re: [PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

hi Masami,

On Wed, 2 Feb 2022 at 12:33, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Sughosh,
>
> 2022年2月2日(水) 14:35 Sughosh Ganu <sughosh.ganu@linaro.org>:
> >
> > hi Masami,
> >
> > On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > Could you tell me why do you need to do the FWU code in the efi_update_capsule?
> >
> > I thought I explained this in my previous email. Putting the FWU
> > checks in efi_update_capsule caters to the scenario where FWU updates
> > are being done in secure world. Even for such scenario, the
> > efi_update_capsule function will get called. So having the checks in
> > one single place is better.
>
> Hmm, I'm not so sure the process flow of when the FWU update are
> being done in secure world. What will happen?
>
> [OS] -> [UEFI UpdateCapsule()] -(SMC)> [secure FWU] -> [update firmware] ?

Yes, this would be the flow.

>
> Or,
>
> [OS]  -(SMC)> [secure FWU] -> [UEFI UpdateCapsule()] -> [update firmware] ?
>
> And anyway, if the FWU is done in secure world, will the FWU metadata
> be processed in the secure world too? (in this case, U-boot may not do
> anything about firmware update but just an interface, right?)

I think certain api's can be re-used, but I will re-check Jose's
secure FWU implementation.

>
> >
> > > If you need to add some logic to both of the efi_update_capsule API
> > > and capsule-on-disk,
> > > it is better to be implemented in the efi_capsule_update_firmware() as
> > > a common part.
> > > Or, make an independent additional function and call it from both path.
> > > This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from
> > > the capsule update logic itself.
> >
> > Like I asked Takahiro, I don't understand why you find the
> > efi_update_capsule function superfluous. I do see it being called for
> > secure world FWU updates. Also, if the function is indeed superfluous,
> > you should also be removing the function definition as well as part of
> > this patch.
>
> We don't said that the efi_update_capsule() is superfluous, but it has
> a different role (e.g. processing multiple capsules and handle the
> capsule flags) as UpdateCapsule() UEFI service API, which is defined
> in UEFI spec. This means we will allow user to run CapsuleApp.efi on
> U-Boot.
>
> If it has to call secure world for FWU, I think that should be done in the
> efi_update_capsule_firmware(), so that that is called from *both* of
> UpdateCapsule() API and Capsule-on-disk.

Okay. In that case, I will put the checks in
efi_update_capsule_firmware. Can you please expand your commit message
a bit to explain why is the call to efi_update_capsule being bypassed.
I believe there was a discussion between you and Takahiro where there
is a more detailed explanation. It would help to have that in the
commit message. Thanks.

-sughosh

>
> Thank you,
>
> >
> > -sughosh
> >
> > >
> > > Thank you,
> > >
> > >
> > > 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>:
> > > >
> > > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>:
> > > > > >hi Masami,
> > > > > >
> > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > > > > ><masami.hiramatsu@linaro.org> wrote:
> > > > > >>
> > > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI
> > > > > >> runtime and boottime service, but the capsule-on-disk process doesn't.
> > > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
> > > > > >> directly instead of efi_update_capsule().
> > > > > >>
> > > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > >> ---
> > > > > >>  Changes in v2:
> > > > > >>   - Fix to pass correct pointer to efi_capsule_update_firmware
> > > > > >>   - Remove ESRT generation, because this part anyway will be removed
> > > > > >>     next patch.
> > > > > >> ---
> > > > > >>  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);
> > > > > >
> > > > > >I believe this is not fixing any issue as such. If so, I would vote
> > > > > >for keeping the call to efi_update_capsule.
> > > > >
> > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour.
> > > >
> > > > Okay, in that case, I will put a check for the FWU Multi Banks feature
> > > > being enabled -- with the feature enabled, the call will be to
> > > > efi_update_capsule, and with the feature disabled, the call will be
> > > > made to efi_capsule_update_firmware. The compiler should compile out
> > > > the code whenever the FWU feature is disabled and that will not impact
> > > > the code size.
> > > >
> > > > -sughosh
> > > >
> > > > >
> > > > > Best regards
> > > > >
> > > > > Heinrich
> > > > >
> > > > >  With the FWU Multi Bank
> > > > > >feature enabled, the checks for capsule acceptance and revert are
> > > > > >being done in this function. The reason I have put this code in the
> > > > > >function is that it caters to both scenarios of capsule-on-disk and
> > > > > >the runtime functionality. In addition, the FWU bootup checks are also
> > > > > >done in this function through a call to fwu_update_checks_pass. So if
> > > > > >this is not a fix, which I don't think it is, I would prefer this call
> > > > > >to remain.
> > > > > >
> > > > > >-sughosh
> > > > > >
> > > > > >>                         if (ret != EFI_SUCCESS)
> > > > > >>                                 log_err("Applying capsule %ls failed\n",
> > > > > >>                                         files[i]);
> > > > > >>
> > >
> > >
> > >
> > > --
> > > Masami Hiramatsu
>
>
>
> --
> Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Heinrich Schuchardt]

On 2/2/22 05:15, AKASHI Takahiro wrote:
> On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote:
>> Hi Takahiro,
>>
>> 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>>
>>>
>>> On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
>>>> Add a config option to reset system 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 v2:
>>>>    - Remove kconfig option to disable this feature.
>>>>    - Use panic() instead of do_reset() so that if the reset fails,
>>>>      the machine halt.
>>>>    - Log the result of each capsule update always.
>>>> ---
>>>>   lib/efi_loader/efi_capsule.c |   12 +++++++++---
>>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
>>>> index 1ec7ea29ff..39bce714f7 100644
>>>> --- a/lib/efi_loader/efi_capsule.c
>>>> +++ b/lib/efi_loader/efi_capsule.c
>>>> @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
>>>>                ret = efi_capsule_read_file(files[i], &capsule);
>>>>                if (ret == EFI_SUCCESS) {
>>>>                        ret = efi_capsule_update_firmware(capsule);
>>>> -                     if (ret != EFI_SUCCESS)
>>>> -                             log_err("Applying capsule %ls failed\n",
>>>> -                                     files[i]);
>>>> +                     log_err("Applying capsule %ls %s\n",
>>>> +                             files[i],
>>>> +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
>>>
>>> log_err()? log_info() is better, I think.
>>
>> Hmm, would you think to use log_info() even if it is failed? Or should
>> we have log_err(failure) and log_info(success)?
>
> It is what I meant :)
>
>>>
>>>>
>>>>                        /* create CapsuleXXXX */
>>>>                        set_capsule_result(index, capsule, ret);
>>>> @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
>>>>                free(files[i]);
>>>>        free(files);
>>>>
>>>> +     /*
>>>> +      * UEFI spec requires to reset system after complete processing capsule
>>>> +      * update on the storage.
>>>> +      */
>>>> +     panic("Reboot after firmware update");
>>>
>>> If CONFIG_PANIC_HANG is enabled, the system won't restart.
>>> It's not what we want here.
>>
>> Indeed.
>> Heinrich, what would you think if do_reset() doesn't work?
>> (I think it is OK to get it back here, but needs a warning)
>
> If (CONFIG_IS_ENABLED(SYSRESET)) {
>      puts ("resetting ...\n");
>      sysreset_reset_walk(SYSRESET_WARM);

do_reset() is implemented in many 25 places.
drivers/sysreset/sysreset-uclass.c is just one of them.

@Tom, @Simon:
Is there a migration timeline to replace all other do_reset()
implementations?

A dummy implementation like in arch/riscv/lib/reset.c should not exist.
The sysreset uclass handles the case of no sysreset driver already.

Best regards

Heinrich

> } else {
>      do_reset(...)
>      halt();
> }
> /* not reach here */
>
> -Takahiro Akashi
>
>
>> Thank you,
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> +
>>>>        return ret;
>>>>   }
>>>>   #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
>>>>
>>
>>
>>
>> --
>> Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

On Thu, Feb 03, 2022 at 06:32:50PM +0100, Heinrich Schuchardt wrote:
> On 2/2/22 05:15, AKASHI Takahiro wrote:
> > On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote:
> > > Hi Takahiro,
> > > 
> > > 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > > 
> > > > 
> > > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote:
> > > > > Add a config option to reset system 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 v2:
> > > > >    - Remove kconfig option to disable this feature.
> > > > >    - Use panic() instead of do_reset() so that if the reset fails,
> > > > >      the machine halt.
> > > > >    - Log the result of each capsule update always.
> > > > > ---
> > > > >   lib/efi_loader/efi_capsule.c |   12 +++++++++---
> > > > >   1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > > index 1ec7ea29ff..39bce714f7 100644
> > > > > --- a/lib/efi_loader/efi_capsule.c
> > > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
> > > > >                ret = efi_capsule_read_file(files[i], &capsule);
> > > > >                if (ret == EFI_SUCCESS) {
> > > > >                        ret = efi_capsule_update_firmware(capsule);
> > > > > -                     if (ret != EFI_SUCCESS)
> > > > > -                             log_err("Applying capsule %ls failed\n",
> > > > > -                                     files[i]);
> > > > > +                     log_err("Applying capsule %ls %s\n",
> > > > > +                             files[i],
> > > > > +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
> > > > 
> > > > log_err()? log_info() is better, I think.
> > > 
> > > Hmm, would you think to use log_info() even if it is failed? Or should
> > > we have log_err(failure) and log_info(success)?
> > 
> > It is what I meant :)
> > 
> > > > 
> > > > > 
> > > > >                        /* create CapsuleXXXX */
> > > > >                        set_capsule_result(index, capsule, ret);
> > > > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void)
> > > > >                free(files[i]);
> > > > >        free(files);
> > > > > 
> > > > > +     /*
> > > > > +      * UEFI spec requires to reset system after complete processing capsule
> > > > > +      * update on the storage.
> > > > > +      */
> > > > > +     panic("Reboot after firmware update");
> > > > 
> > > > If CONFIG_PANIC_HANG is enabled, the system won't restart.
> > > > It's not what we want here.
> > > 
> > > Indeed.
> > > Heinrich, what would you think if do_reset() doesn't work?
> > > (I think it is OK to get it back here, but needs a warning)
> > 
> > If (CONFIG_IS_ENABLED(SYSRESET)) {
> >      puts ("resetting ...\n");
> >      sysreset_reset_walk(SYSRESET_WARM);
> 
> do_reset() is implemented in many 25 places.
> drivers/sysreset/sysreset-uclass.c is just one of them.
> 
> @Tom, @Simon:
> Is there a migration timeline to replace all other do_reset()
> implementations?
> 
> A dummy implementation like in arch/riscv/lib/reset.c should not exist.
> The sysreset uclass handles the case of no sysreset driver already.

Not yet, please feel free to propose something, if it can't just be done
outright, right now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Hi Takahiro,

We forgot a simple fact. The warm reset doesn't load the new firmware
from media.

If this reset is for reloading the new firmware, we anyway need the
cold reset :-)
(and I can't think of any reason other than this)

Thank you,

2022年2月3日(木) 13:34 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hi Takahiro,
>
> 2022年2月3日(木) 10:24 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >
> > On Wed, Feb 02, 2022 at 10:54:43PM +0900, Masami Hiramatsu wrote:
> > > Add a config option to reset system soon after processing capsule update
> > > on disk.
> >
> > We no longer have a new config option :)
>
> Oops, that's my fault.
>
> >
> > > 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 v3:
> > >   - Log succeeded capsule update in info level.
> > >   - Use sysreset if possible.
> > >   - Use do_reset() and hang() instead of panic().
> > >  Changes in v2:
> > >   - Remove kconfig option to disable this feature.
> > >   - Use panic() instead of do_reset() so that if the reset fails,
> > >     the machine halt.
> > >   - Log the result of each capsule update always.
> > > ---
> > >  lib/efi_loader/efi_capsule.c |   22 ++++++++++++++++++++--
> > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 1ec7ea29ff..ade9155042 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -14,9 +14,11 @@
> > >  #include <env.h>
> > >  #include <fdtdec.h>
> > >  #include <fs.h>
> > > +#include <hang.h>
> > >  #include <malloc.h>
> > >  #include <mapmem.h>
> > >  #include <sort.h>
> > > +#include <sysreset.h>
> > >  #include <asm/global_data.h>
> > >
> > >  #include <crypto/pkcs7.h>
> > > @@ -1120,8 +1122,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 +1147,19 @@ 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.
> > > +      */
> > > +     puts("Reboot after firmware update");
> > > +     if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > +             reset_cpu();
> > > +     } else {
> > > +             do_reset(NULL, 0, 0, NULL);
> > > +             hang();
> > > +     }
> > > +     /* not reach here */
> >
> > Despite the code that I proposed, I have a few concerns:
> > 1) warm or cold reset
> > Now that we are updating firmware, we may have to initiate
> > a cold reset in some cases.
> > (That's why I used 'sysreset(WARM)' to raise a question.)
>
> Indeed. Hm, as far as I can see the EDK2, it also uses cold reset.
> (HandleCapsules@ArmPkg/Library/PlatformBootManagerLib/PlatformBM.c)
> Since do_reset() calls sysreset_walk_halt(), I think do_reset() is enough.
>
> >
> > From the viewpoint of UEFI specification,
> >   * A type of reset can be determined per capsule by calling
> >     QueryCapsuleCapabilities API.
> >     (The spec said, "Returns if the capsule can be supported via
> >      UpdateCapsule()" and Capsule-on-disk might be out of scope?)
>
> I think that is only for UpdateCapsule(), as far as I can read the EDK2 code.
>
> >   * There exists ResetSystem API and it takes a *reset type*
> >     as a parameter.
>
> This API is independent from UpdateCapsule(). But while executing
> the UpdateCapsule() this API is prohibited. (See Table 8-1)
>
> >
> > 2) ResetSystem at boot time
> > So we may want to internally make use of efi_reset_system() following
> > capsule-on-disk processing.
> > The current implementation, however, does not utilize SYSRESET drivers,
> > but call do_reset(). This should be changed (as I suggested above?).
>
> As I said above, I think it should always be a cold reset and not need to use
> efi_reset_system(). For the UpdateCapsule(), there is a reason to use
> warm reset, because the capsule images which will be applied after reset,
> will be on the memory. In this case the system must be reboot without
> resetting the memory.
> But after capsule-on-disk process, all capsule images are applied and the
> firmware image on the storage is updated. So it is better to reset the
> system with cold reset so that the new firmware image can start with
> cleaned memory and devices.
>
> Thank you,
>
> >
> > -Takahiro Akashi
> >
> >
> > > +
> > > +     return 0;
> > >  }
> > >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> > >
>
>
>
> --
> Masami Hiramatsu



-- 
Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Hi Takahiro,

2022年2月3日(木) 10:24 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Wed, Feb 02, 2022 at 10:54:43PM +0900, Masami Hiramatsu wrote:
> > Add a config option to reset system soon after processing capsule update
> > on disk.
>
> We no longer have a new config option :)

Oops, that's my fault.

>
> > 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 v3:
> >   - Log succeeded capsule update in info level.
> >   - Use sysreset if possible.
> >   - Use do_reset() and hang() instead of panic().
> >  Changes in v2:
> >   - Remove kconfig option to disable this feature.
> >   - Use panic() instead of do_reset() so that if the reset fails,
> >     the machine halt.
> >   - Log the result of each capsule update always.
> > ---
> >  lib/efi_loader/efi_capsule.c |   22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 1ec7ea29ff..ade9155042 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -14,9 +14,11 @@
> >  #include <env.h>
> >  #include <fdtdec.h>
> >  #include <fs.h>
> > +#include <hang.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <sort.h>
> > +#include <sysreset.h>
> >  #include <asm/global_data.h>
> >
> >  #include <crypto/pkcs7.h>
> > @@ -1120,8 +1122,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 +1147,19 @@ 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.
> > +      */
> > +     puts("Reboot after firmware update");
> > +     if (CONFIG_IS_ENABLED(SYSRESET)) {
> > +             reset_cpu();
> > +     } else {
> > +             do_reset(NULL, 0, 0, NULL);
> > +             hang();
> > +     }
> > +     /* not reach here */
>
> Despite the code that I proposed, I have a few concerns:
> 1) warm or cold reset
> Now that we are updating firmware, we may have to initiate
> a cold reset in some cases.
> (That's why I used 'sysreset(WARM)' to raise a question.)

Indeed. Hm, as far as I can see the EDK2, it also uses cold reset.
(HandleCapsules@ArmPkg/Library/PlatformBootManagerLib/PlatformBM.c)
Since do_reset() calls sysreset_walk_halt(), I think do_reset() is enough.

>
> From the viewpoint of UEFI specification,
>   * A type of reset can be determined per capsule by calling
>     QueryCapsuleCapabilities API.
>     (The spec said, "Returns if the capsule can be supported via
>      UpdateCapsule()" and Capsule-on-disk might be out of scope?)

I think that is only for UpdateCapsule(), as far as I can read the EDK2 code.

>   * There exists ResetSystem API and it takes a *reset type*
>     as a parameter.

This API is independent from UpdateCapsule(). But while executing
the UpdateCapsule() this API is prohibited. (See Table 8-1)

>
> 2) ResetSystem at boot time
> So we may want to internally make use of efi_reset_system() following
> capsule-on-disk processing.
> The current implementation, however, does not utilize SYSRESET drivers,
> but call do_reset(). This should be changed (as I suggested above?).

As I said above, I think it should always be a cold reset and not need to use
efi_reset_system(). For the UpdateCapsule(), there is a reason to use
warm reset, because the capsule images which will be applied after reset,
will be on the memory. In this case the system must be reboot without
resetting the memory.
But after capsule-on-disk process, all capsule images are applied and the
firmware image on the storage is updated. So it is better to reset the
system with cold reset so that the new firmware image can start with
cleaned memory and devices.

Thank you,

>
> -Takahiro Akashi
>
>
> > +
> > +     return 0;
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> >



-- 
Masami Hiramatsu

Re: [PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[AKASHI Takahiro]

On Wed, Feb 02, 2022 at 10:54:43PM +0900, Masami Hiramatsu wrote:
> Add a config option to reset system soon after processing capsule update
> on disk.

We no longer have a new config option :)

> 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 v3:
>   - Log succeeded capsule update in info level.
>   - Use sysreset if possible.
>   - Use do_reset() and hang() instead of panic().
>  Changes in v2:
>   - Remove kconfig option to disable this feature.
>   - Use panic() instead of do_reset() so that if the reset fails,
>     the machine halt.
>   - Log the result of each capsule update always.
> ---
>  lib/efi_loader/efi_capsule.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 1ec7ea29ff..ade9155042 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -14,9 +14,11 @@
>  #include <env.h>
>  #include <fdtdec.h>
>  #include <fs.h>
> +#include <hang.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <sort.h>
> +#include <sysreset.h>
>  #include <asm/global_data.h>
>  
>  #include <crypto/pkcs7.h>
> @@ -1120,8 +1122,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 +1147,19 @@ 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.
> +	 */
> +	puts("Reboot after firmware update");
> +	if (CONFIG_IS_ENABLED(SYSRESET)) {
> +		reset_cpu();
> +	} else {
> +		do_reset(NULL, 0, 0, NULL);
> +		hang();
> +	}
> +	/* not reach here */

Despite the code that I proposed, I have a few concerns:
1) warm or cold reset
Now that we are updating firmware, we may have to initiate
a cold reset in some cases.
(That's why I used 'sysreset(WARM)' to raise a question.)

From the viewpoint of UEFI specification,
  * A type of reset can be determined per capsule by calling
    QueryCapsuleCapabilities API.
    (The spec said, "Returns if the capsule can be supported via
     UpdateCapsule()" and Capsule-on-disk might be out of scope?)
  * There exists ResetSystem API and it takes a *reset type*
    as a parameter.

2) ResetSystem at boot time
So we may want to internally make use of efi_reset_system() following
capsule-on-disk processing.
The current implementation, however, does not utilize SYSRESET drivers,
but call do_reset(). This should be changed (as I suggested above?).

-Takahiro Akashi


> +
> +	return 0;
>  }
>  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> 

[PATCH v2 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Add a config option to reset system 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 v3:
  - Log succeeded capsule update in info level.
  - Use sysreset if possible.
  - Use do_reset() and hang() instead of panic().
 Changes in v2:
  - Remove kconfig option to disable this feature.
  - Use panic() instead of do_reset() so that if the reset fails,
    the machine halt.
  - Log the result of each capsule update always.
---
 lib/efi_loader/efi_capsule.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 1ec7ea29ff..ade9155042 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -14,9 +14,11 @@
 #include <env.h>
 #include <fdtdec.h>
 #include <fs.h>
+#include <hang.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <sort.h>
+#include <sysreset.h>
 #include <asm/global_data.h>
 
 #include <crypto/pkcs7.h>
@@ -1120,8 +1122,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 +1147,19 @@ 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.
+	 */
+	puts("Reboot after firmware update");
+	if (CONFIG_IS_ENABLED(SYSRESET)) {
+		reset_cpu();
+	} else {
+		do_reset(NULL, 0, 0, NULL);
+		hang();
+	}
+	/* not reach here */
+
+	return 0;
 }
 #endif /* CONFIG_EFI_CAPSULE_ON_DISK */