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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Feb 01, 2022 at 11:26:38AM +0900, Masami Hiramatsu wrote:
> Hi Heinrich,
> 
> 2022年1月31日(月) 21:17 Heinrich Schuchardt <xypron.glpk@gmx.de>:
> >
> > On 1/31/22 12:19, Grant Likely wrote:
> > > On 31/01/2022 09:19, 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.
> > >>
> > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >
> > > Is there known use cases for making this an option? Feels a bit like
> > > option creep that is too easy to choose the wrong setting.
> > >
> > > Otherwise, this looks good to me.
> > >
> > > g.
> > >
> > >> ---
> > >>   lib/efi_loader/Kconfig       |   10 ++++++++++
> > >>   lib/efi_loader/efi_capsule.c |    9 +++++++++
> > >>   2 files changed, 19 insertions(+)
> > >>
> > >> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > >> index 24f9a2bb75..db05c3ad90 100644
> > >> --- a/lib/efi_loader/Kconfig
> > >> +++ b/lib/efi_loader/Kconfig
> > >> @@ -146,6 +146,16 @@ config EFI_IGNORE_OSINDICATIONS
> > >>         without setting the
> > >> EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> > >>         flag in variable OsIndications.
> > >>
> > >> +config EFI_RESET_AFTER_CAPSULE_ON_DISK
> > >> +     bool "Reset right after CapsuleUpdate on-disk"
> > >> +     depends on EFI_CAPSULE_ON_DISK
> > >> +     default y
> > >> +     help
> > >> +       UEFI specification requests the system to be restarted after
> > >> capsule
> > >> +       processing is complete. This implements that, but for some
> > >> reason,
> > >> +       if you want to keep the (old) system running after the capsule
> > >> update
> > >> +       on-disk, you can say 'n' here.
> > >> +
> > >>   config EFI_CAPSULE_ON_DISK_EARLY
> > >>       bool "Initiate capsule-on-disk at U-Boot boottime"
> > >>       depends on EFI_CAPSULE_ON_DISK
> > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > >> index 98dab1c6f5..44d4fa2f82 100644
> > >> --- a/lib/efi_loader/efi_capsule.c
> > >> +++ b/lib/efi_loader/efi_capsule.c
> > >> @@ -1142,6 +1142,15 @@ 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.
> > >> +      */
> > >> +     if (IS_ENABLED(CONFIG_EFI_RESET_AFTER_CAPSULE_ON_DISK)) {
> >
> > We should not allow configuring a non-compliant behavior.
> 
> OK, let me remove this option then.
> 
> >
> > >> +             log_info("Restarting the system to boot the updated
> > >> firmware.\n");
> >
> > I am missing a success message for each installed capsule.
> 
> Indeed, but this reboot will be done unconditionally.
> let me add a message when the capsule update is successfully completed.
> Thank you,

While I don't object to adding a message, I'd rather see that a sub-command,
like "efidebug capsule show-result," be added to efidebug command.
Please note the result for each capsule will be recorded in "CapsuleNNNN"
variable which may contain additional information.
(I haven't implemented efi_variable_result_fmp though.)

-Takahiro Akashi


> 
> >
> > >> +             do_reset(NULL, 0, 0, NULL);
> >
> > do_reset() may return. How about:
> >
> >     panic("Reboot after firmware update");
> >
> > This will hang if do_reset() returns.
> >
> > >> +     }
> > >> +
> > >>       if (IS_ENABLED(CONFIG_EFI_ESRT)) {
> >
> > We don't need this code anymore if we call panic() above.
> >
> > Best regards
> >
> > Heinrich
> >
> > >>               /* Rebuild the ESRT to reflect any updated FW images. */
> > >>               ret = efi_esrt_populate();
> > >>
> > >
> 
> 
> 
> --
> Masami Hiramatsu