[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 at linaro.org>:
>
> Hi Takahiro,
>
> 2022年2月3日(木) 10:24 AKASHI Takahiro <takahiro.akashi at 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 at 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 at 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