| Message ID | 164311028928.175897.11020560607087529330.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
| Series | EFI: Add CAPSULE_FLAG_INITIATE_RESET support | expand |
Hi Masami, Thank you for this enhancement. On Tue, Jan 25, 2022 at 08:31:29PM +0900, Masami Hiramatsu wrote: > Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after > updating firmware. Note that the machine will reboot soon after > applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET > flag. If there are multiple capsules and one has this flag, the > machine may reboot while scanning the capsule files. > You can control when the machine reboot by renaming the capsule > file because the capsule files will be applied alphabetically. > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > --- > lib/efi_loader/efi_capsule.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index 4463ae00fd..24a2a026a9 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( > struct efi_firmware_management_protocol *fmp; > u16 *abort_reason; > efi_status_t ret = EFI_SUCCESS; > + bool reset = false; > > /* sanity check */ > if (capsule_data->header_size < sizeof(*capsule) || > capsule_data->header_size >= capsule_data->capsule_image_size) > return EFI_INVALID_PARAMETER; > > + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { > + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ > + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) > + return EFI_INVALID_PARAMETER; > + reset = true; > + } > + > capsule = (void *)capsule_data + capsule_data->header_size; > capsule_size = capsule_data->capsule_image_size > - capsule_data->header_size; > @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( > out: > efi_free_pool(handles); > > + if (ret == EFI_SUCCESS && reset) { > + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); > + do_reset(NULL, 0, 0, NULL); I don't think this is the right place of calling do_reset(). Whenever you have processed any capsule file, you have to - delete the capsule file, - create/update "CapsuleXXXX" and "CapsuleLast" variables, and - modify ESRT table before initiating a reset. -Takahiro Akashi > + } > + > return ret; > } > #else >
Hi Takahiro, 2022年1月25日(火) 21:49 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > Hi Masami, > > Thank you for this enhancement. > > On Tue, Jan 25, 2022 at 08:31:29PM +0900, Masami Hiramatsu wrote: > > Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after > > updating firmware. Note that the machine will reboot soon after > > applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET > > flag. If there are multiple capsules and one has this flag, the > > machine may reboot while scanning the capsule files. > > You can control when the machine reboot by renaming the capsule > > file because the capsule files will be applied alphabetically. > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > --- > > lib/efi_loader/efi_capsule.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index 4463ae00fd..24a2a026a9 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( > > struct efi_firmware_management_protocol *fmp; > > u16 *abort_reason; > > efi_status_t ret = EFI_SUCCESS; > > + bool reset = false; > > > > /* sanity check */ > > if (capsule_data->header_size < sizeof(*capsule) || > > capsule_data->header_size >= capsule_data->capsule_image_size) > > return EFI_INVALID_PARAMETER; > > > > + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { > > + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ > > + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) > > + return EFI_INVALID_PARAMETER; > > + reset = true; > > + } > > + > > capsule = (void *)capsule_data + capsule_data->header_size; > > capsule_size = capsule_data->capsule_image_size > > - capsule_data->header_size; > > @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( > > out: > > efi_free_pool(handles); > > > > + if (ret == EFI_SUCCESS && reset) { > > + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); > > + do_reset(NULL, 0, 0, NULL); > > I don't think this is the right place of calling do_reset(). > Whenever you have processed any capsule file, you have to > - delete the capsule file, > - create/update "CapsuleXXXX" and "CapsuleLast" variables, and > - modify ESRT table > before initiating a reset. Oops, indeed. Let me update the patch. Thank you! > > -Takahiro Akashi > > > + } > > + > > return ret; > > } > > #else > >
BTW, the original code comes from EDK2 implementation. It seems that this INITIATE_RESET flag meaning in EDK2 is a bit different from what we thought here. The flag is only used for resetting system via UpdateCapsule() EFI call. The capsule update process itself will be done at the boot time and warm reset soon after that. (See HandleCapsules()@ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c) Is it OK to change the INITIATE_RESET flag means? or should we forcibly reset the system if we succeeded to update capsule on boot time? (note that capsule on disk must be done at boot time in U-Boot) Thank you, 2022年1月26日(水) 7:31 Masami Hiramatsu <masami.hiramatsu@linaro.org>: > > Hi Takahiro, > > 2022年1月25日(火) 21:49 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > Hi Masami, > > > > Thank you for this enhancement. > > > > On Tue, Jan 25, 2022 at 08:31:29PM +0900, Masami Hiramatsu wrote: > > > Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after > > > updating firmware. Note that the machine will reboot soon after > > > applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET > > > flag. If there are multiple capsules and one has this flag, the > > > machine may reboot while scanning the capsule files. > > > You can control when the machine reboot by renaming the capsule > > > file because the capsule files will be applied alphabetically. > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > --- > > > lib/efi_loader/efi_capsule.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > index 4463ae00fd..24a2a026a9 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( > > > struct efi_firmware_management_protocol *fmp; > > > u16 *abort_reason; > > > efi_status_t ret = EFI_SUCCESS; > > > + bool reset = false; > > > > > > /* sanity check */ > > > if (capsule_data->header_size < sizeof(*capsule) || > > > capsule_data->header_size >= capsule_data->capsule_image_size) > > > return EFI_INVALID_PARAMETER; > > > > > > + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { > > > + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ > > > + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) > > > + return EFI_INVALID_PARAMETER; > > > + reset = true; > > > + } > > > + > > > capsule = (void *)capsule_data + capsule_data->header_size; > > > capsule_size = capsule_data->capsule_image_size > > > - capsule_data->header_size; > > > @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( > > > out: > > > efi_free_pool(handles); > > > > > > + if (ret == EFI_SUCCESS && reset) { > > > + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); > > > + do_reset(NULL, 0, 0, NULL); > > > > I don't think this is the right place of calling do_reset(). > > Whenever you have processed any capsule file, you have to > > - delete the capsule file, > > - create/update "CapsuleXXXX" and "CapsuleLast" variables, and > > - modify ESRT table > > before initiating a reset. > > Oops, indeed. Let me update the patch. > Thank you! > > > > > -Takahiro Akashi > > > > > + } > > > + > > > return ret; > > > } > > > #else > > > > > > > -- > Masami Hiramatsu
On Wed, Jan 26, 2022 at 11:29:10AM +0900, Masami Hiramatsu wrote: > BTW, the original code comes from EDK2 implementation. What do you mean by "original code"? > It seems that this INITIATE_RESET flag meaning in EDK2 is a bit different > from what we thought here. Yeah, > The flag is only used for resetting system via > UpdateCapsule() EFI call. The capsule update process itself will be done > at the boot time and warm reset soon after that. EDK2's UpdateCapsule() seems to - immediately process *all* the capsules with !PERSIST_ACROSS_RESET - Set "CapsuleUpdateData" variable with a physical address of capsule data - If some of capsules have INITIATE_RESET, kick in warm restart After restart, - process the rest of capsules with PERSIST_ACROSS_RESET using "CapsuleUpdateData" (?) > (See HandleCapsules()@ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c) > > Is it OK to change the INITIATE_RESET flag means? or should we forcibly > reset the system if we succeeded to update capsule on boot time? > (note that capsule on disk must be done at boot time in U-Boot) Obviously, we are trying to use the flag in a different way; initiating a reset *after* processing a capsule. While I think that the text in the specification is still ambiguous, we should not give the flag a different meaning. In 8.5.5 "Delivery of Capsules via file on Mass Storage device", In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed. So a reset in case of capsule-on-disk seems mandatory. (Personally, I don't like the system to enforce a reset.) The discussion above also indicates that the current efi_launch_capsules() must be reworked so not to use efi_update_capsule() which is to honor the flags in a capsule header. -Takahiro Akashi > Thank you, > > 2022年1月26日(水) 7:31 Masami Hiramatsu <masami.hiramatsu@linaro.org>: > > > > Hi Takahiro, > > > > 2022年1月25日(火) 21:49 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > > > Hi Masami, > > > > > > Thank you for this enhancement. > > > > > > On Tue, Jan 25, 2022 at 08:31:29PM +0900, Masami Hiramatsu wrote: > > > > Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after > > > > updating firmware. Note that the machine will reboot soon after > > > > applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET > > > > flag. If there are multiple capsules and one has this flag, the > > > > machine may reboot while scanning the capsule files. > > > > You can control when the machine reboot by renaming the capsule > > > > file because the capsule files will be applied alphabetically. > > > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > --- > > > > lib/efi_loader/efi_capsule.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > index 4463ae00fd..24a2a026a9 100644 > > > > --- a/lib/efi_loader/efi_capsule.c > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( > > > > struct efi_firmware_management_protocol *fmp; > > > > u16 *abort_reason; > > > > efi_status_t ret = EFI_SUCCESS; > > > > + bool reset = false; > > > > > > > > /* sanity check */ > > > > if (capsule_data->header_size < sizeof(*capsule) || > > > > capsule_data->header_size >= capsule_data->capsule_image_size) > > > > return EFI_INVALID_PARAMETER; > > > > > > > > + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { > > > > + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ > > > > + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) > > > > + return EFI_INVALID_PARAMETER; > > > > + reset = true; > > > > + } > > > > + > > > > capsule = (void *)capsule_data + capsule_data->header_size; > > > > capsule_size = capsule_data->capsule_image_size > > > > - capsule_data->header_size; > > > > @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( > > > > out: > > > > efi_free_pool(handles); > > > > > > > > + if (ret == EFI_SUCCESS && reset) { > > > > + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); > > > > + do_reset(NULL, 0, 0, NULL); > > > > > > I don't think this is the right place of calling do_reset(). > > > Whenever you have processed any capsule file, you have to > > > - delete the capsule file, > > > - create/update "CapsuleXXXX" and "CapsuleLast" variables, and > > > - modify ESRT table > > > before initiating a reset. > > > > Oops, indeed. Let me update the patch. > > Thank you! > > > > > > > > -Takahiro Akashi > > > > > > > + } > > > > + > > > > return ret; > > > > } > > > > #else > > > > > > > > > > > > -- > > Masami Hiramatsu > > > > -- > Masami Hiramatsu
2022年1月26日(水) 16:20 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > On Wed, Jan 26, 2022 at 11:29:10AM +0900, Masami Hiramatsu wrote: > > BTW, the original code comes from EDK2 implementation. > > What do you mean by "original code"? I meant the version1, this patch. EDK2 resets the machine in CapsuleUpdate() EFI API, but as you said the flag itself has different meaning. > > > It seems that this INITIATE_RESET flag meaning in EDK2 is a bit different > > from what we thought here. > > Yeah, > > > The flag is only used for resetting system via > > UpdateCapsule() EFI call. The capsule update process itself will be done > > at the boot time and warm reset soon after that. > > EDK2's UpdateCapsule() seems to > - immediately process *all* the capsules with !PERSIST_ACROSS_RESET > - Set "CapsuleUpdateData" variable with a physical address of capsule data > - If some of capsules have INITIATE_RESET, kick in warm restart > After restart, > - process the rest of capsules with PERSIST_ACROSS_RESET > using "CapsuleUpdateData" (?) > > > (See HandleCapsules()@ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c) > > > > Is it OK to change the INITIATE_RESET flag means? or should we forcibly > > reset the system if we succeeded to update capsule on boot time? > > (note that capsule on disk must be done at boot time in U-Boot) > > Obviously, we are trying to use the flag in a different way; > initiating a reset *after* processing a capsule. > > While I think that the text in the specification is still ambiguous, > we should not give the flag a different meaning. Agreed. > > In 8.5.5 "Delivery of Capsules via file on Mass Storage device", > In all cases that a capsule is identified for processing the system is > restarted after capsule processing is completed. > > So a reset in case of capsule-on-disk seems mandatory. > (Personally, I don't like the system to enforce a reset.) Yeah, so it seems my first attempt (Kconfig to forcibly reset after applying capsules) was correct way... > > The discussion above also indicates that the current efi_launch_capsules() > must be reworked so not to use efi_update_capsule() which is to honor > the flags in a capsule header. Indeed. If the system will forcibly reset, ESRT also no need to be updated. Then we can use efi_capsule_update_firmware() directly from efi_launch_capsules(). Thank you, > -Takahiro Akashi > > > Thank you, > > > > 2022年1月26日(水) 7:31 Masami Hiramatsu <masami.hiramatsu@linaro.org>: > > > > > > Hi Takahiro, > > > > > > 2022年1月25日(火) 21:49 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > > > > > Hi Masami, > > > > > > > > Thank you for this enhancement. > > > > > > > > On Tue, Jan 25, 2022 at 08:31:29PM +0900, Masami Hiramatsu wrote: > > > > > Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after > > > > > updating firmware. Note that the machine will reboot soon after > > > > > applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET > > > > > flag. If there are multiple capsules and one has this flag, the > > > > > machine may reboot while scanning the capsule files. > > > > > You can control when the machine reboot by renaming the capsule > > > > > file because the capsule files will be applied alphabetically. > > > > > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > --- > > > > > lib/efi_loader/efi_capsule.c | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > > index 4463ae00fd..24a2a026a9 100644 > > > > > --- a/lib/efi_loader/efi_capsule.c > > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > > @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( > > > > > struct efi_firmware_management_protocol *fmp; > > > > > u16 *abort_reason; > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > + bool reset = false; > > > > > > > > > > /* sanity check */ > > > > > if (capsule_data->header_size < sizeof(*capsule) || > > > > > capsule_data->header_size >= capsule_data->capsule_image_size) > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { > > > > > + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ > > > > > + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) > > > > > + return EFI_INVALID_PARAMETER; > > > > > + reset = true; > > > > > + } > > > > > + > > > > > capsule = (void *)capsule_data + capsule_data->header_size; > > > > > capsule_size = capsule_data->capsule_image_size > > > > > - capsule_data->header_size; > > > > > @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( > > > > > out: > > > > > efi_free_pool(handles); > > > > > > > > > > + if (ret == EFI_SUCCESS && reset) { > > > > > + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); > > > > > + do_reset(NULL, 0, 0, NULL); > > > > > > > > I don't think this is the right place of calling do_reset(). > > > > Whenever you have processed any capsule file, you have to > > > > - delete the capsule file, > > > > - create/update "CapsuleXXXX" and "CapsuleLast" variables, and > > > > - modify ESRT table > > > > before initiating a reset. > > > > > > Oops, indeed. Let me update the patch. > > > Thank you! > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > + } > > > > > + > > > > > return ret; > > > > > } > > > > > #else > > > > > > > > > > > > > > > > > -- > > > Masami Hiramatsu > > > > > > > > -- > > Masami Hiramatsu
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 4463ae00fd..24a2a026a9 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -407,12 +407,20 @@ static efi_status_t efi_capsule_update_firmware( struct efi_firmware_management_protocol *fmp; u16 *abort_reason; efi_status_t ret = EFI_SUCCESS; + bool reset = false; /* sanity check */ if (capsule_data->header_size < sizeof(*capsule) || capsule_data->header_size >= capsule_data->capsule_image_size) return EFI_INVALID_PARAMETER; + if (capsule_data->flags & CAPSULE_FLAGS_INITIATE_RESET) { + /* INITIATE_RESET flag requires PERSIST_ACROSS_RESET flag */ + if (!(capsule_data->flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) + return EFI_INVALID_PARAMETER; + reset = true; + } + capsule = (void *)capsule_data + capsule_data->header_size; capsule_size = capsule_data->capsule_image_size - capsule_data->header_size; @@ -498,6 +506,11 @@ static efi_status_t efi_capsule_update_firmware( out: efi_free_pool(handles); + if (ret == EFI_SUCCESS && reset) { + log_debug("This capsule has CAPSULE_FLAGS_INITIATE_RESET. Reboot machine.\n"); + do_reset(NULL, 0, 0, NULL); + } + return ret; } #else
Support CAPSULE_FLAGS_INITIATE_RESET for rebooting uboot soon after updating firmware. Note that the machine will reboot soon after applying the capsule file which has CAPSULE_FLAGS_INITIATE_RESET flag. If there are multiple capsules and one has this flag, the machine may reboot while scanning the capsule files. You can control when the machine reboot by renaming the capsule file because the capsule files will be applied alphabetically. Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> --- lib/efi_loader/efi_capsule.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)