[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 3rd version of the reset after capsule-on-disk. This version
updates the patch description and use do_reset() and halt or reset_cpu()
if available, according to Takahiro and Sughosh's comment.

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: use efi_update_capsule_firmware() for capsule on disk
      efi_loader: Reset system after CapsuleUpdate on disk


 lib/efi_loader/efi_capsule.c |   24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

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

[PATCH v2 1/2] efi_loader: use efi_update_capsule_firmware() for capsule on disk

[Masami Hiramatsu]

Since the efi_update_capsule() represents the UpdateCapsule() runtime
service, it has to handle the capsule flags and update ESRT.
However, the capsule-on-disk doesn't need to care about such things.
Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
directly instead of calling efi_update_capsule().
This means the roles of the efi_update_capsule() and capsule-on-disk
are different. We have to keep the efi_update_capsule() for providing
runtime service at boot time.

Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 Changes in v3:
  - Update patch description.
 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 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 */

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

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

[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

[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

[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

[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 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 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 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 */
> 

[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 */