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

[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