Re: [RFC PATCH 10/14] FWU: Reboot soon after successfully install the new firmware

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

On Fri, Jan 21, 2022 at 03:54:12PM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> 2022年1月21日(金) 13:35 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >
> > Hi Takahiro,
> >
> > 2022年1月21日(金) 10:46 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > >
> > > On Fri, Jan 21, 2022 at 12:31:00AM +0900, Masami Hiramatsu wrote:
> > > > Reboot to the trial state soon after successfully installing
> > > > the new firmware to the next bank and updating the active_index.
> > > > This is enabled by CONFIG_FWU_REBOOT_AFTER_UPDATE and is a
> > > > recommended option.
> > >
> > > EFI_CAPSULE_HEADER.Flags may have a flag, CAPSULE_FLAGS_INITIATE_RESET.
> > > See Section "8.5.3 Update Capsule" in the UEFI specification.
> > >
> > > I think that we'd better implement the feature rather than adding
> > > CONFIG_FWU_REBOOT_AFTER_UPDATE.
> >
> > Thanks for pointing it! I agree with you, the flag is more useful.
> 
> According to the UEFI spec 2.9, we need to consider implementing some
> related things.
> 
> In 8.5.3 Update Capsule
>  ----
> A capsule which has the CAPSULE_FLAGS_INITIATE_RESET Flag must have
> CAPSULE_FLAGS_PERSIST_ACROSS_RESET set in its header as well.
>  [...]
> If a capsule has the CAPSULE_FLAGS_PERSIST_ACROSS_RESET Flag set in its
> header, the firmware will process the capsules after system reset. The
> caller must
> ensure to reset the system using the required reset value obtained from
> QueryCapsuleCapabilities.
>  ----
> In Table 8-8 Flag Firmware Behavior
> ----
> CAPSULE_FLAGS_PERSIST_ACROSS_RESET +
> CAPSULE_FLAGS_INITIATE_RESET
> 
> Firmware will attempt to process or launch the capsule
> across a reset. The firmware will initiate a reset which is
> compatible with the passed-in capsule request and will
> not return back to the caller. If the capsule is not
> recognized, can expect an error. If the processing requires
> a reset which is unsupported by the platform, expect an
> error.
> ----
> 
> So, I have 2 questions;
> 
> 1) Should we implement CAPSULE_FLAGS_PERSIST_ACROSS_RESET too?
>  Since U-Boot only supports capsule update on disk, it seems the capsule already
>  applied "across a reset". :-)

Yeah, I suppose that PERSIST_ACROSS_RESET is most for capsules
via UpdateCapsule API.

> 2) If there are multiple capsule files and only one file (e.g. aaaa.cap) has
>   CAPSULE_FLAGS_INITIATE_RESET flag, should U-Boot resets after
>    applying that capsule, or wait after all capsule files are applied?
>    (current implementation is the latter one)

The order of capsules applied can be controlled by their file names.
So IIUC, a reset should be initiated immediately after processing
a capsule with INITIATE_RESET. The rest of capsules can be processed
even after the reboot.
I think that this behavior gives us more flexibility.

-Takahiro Akashi


> Thank you,
> 
> >
> > Regards,
> >
> > >
> > > -Takahiro Akashi
> > >
> > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > ---
> > > >  lib/efi_loader/efi_capsule.c |   10 ++++++++--
> > > >  lib/fwu_updates/Kconfig      |    9 +++++++++
> > > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > index 83c89a0cbb..0928425b5f 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -1355,10 +1355,16 @@ efi_status_t efi_launch_capsules(void)
> > > >                       } else {
> > > >                               log_debug("Successfully updated the active_index\n");
> > > >                               status = fwu_trial_state_ctr_start();
> > > > -                             if (status < 0)
> > > > +                             if (status < 0) {
> > > >                                       ret = EFI_DEVICE_ERROR;
> > > > -                             else
> > > > +                             } else {
> > > >                                       ret = EFI_SUCCESS;
> > > > +                                     if (IS_ENABLED(CONFIG_FWU_REBOOT_AFTER_UPDATE)) {
> > > > +                                             log_info("New firmware is installed in bank#%d. Reboot from that bank.\n",
> > > > +                                                      update_index);
> > > > +                                             do_reset(NULL, 0, 0, NULL);
> > > > +                                     }
> > > > +                             }
> > > >                       }
> > > >               } else if (capsule_update == true && update_status == false) {
> > > >                       log_err("All capsules were not updated. Not updating FWU metadata\n");
> > > > diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig
> > > > index 6de28e0c9c..0940a90747 100644
> > > > --- a/lib/fwu_updates/Kconfig
> > > > +++ b/lib/fwu_updates/Kconfig
> > > > @@ -29,3 +29,12 @@ config FWU_TRIAL_STATE_CNT
> > > >         With FWU Multi Bank Update feature enabled, number of times
> > > >         the platform is allowed to boot in Trial State after an
> > > >         update.
> > > > +
> > > > +config FWU_REBOOT_AFTER_UPDATE
> > > > +     bool "Reboot soon after installing new firmware"
> > > > +     depends on FWU_MULTI_BANK_UPDATE
> > > > +     default y
> > > > +     help
> > > > +       Reboot the machine soon after installing a new firmware
> > > > +       and start trial boot. You can disable this option for
> > > > +       debugging or FWU development, but recommended to enable it.
> > > >
> >
> >
> >
> > --
> > Masami Hiramatsu
> 
> 
> 
> -- 
> Masami Hiramatsu