[PATCH 0/2] EFI: Add CAPSULE_FLAG_INITIATE_RESET support
Hi,
This is the patchset for adding CAPSULE_FLAG_INITIATE_RESET support to EFU capsule support, according to Takahiro's comment.
The first patch is the core patch to add supporting the CAPSULE_FLAG_INITIATE_RESET. The system reset soon after applying the capsule file which has this flag. The rest of capsule files on disk will be applied after reboot. The second one adding "--flags reset" option to mkeficapsule tool. This might better be rebased on top of Takahiro's work, but in this version I made this series on top of the latest master branch.
Thank you,
---
Masami Hiramatsu (2): EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update mkeficapsule: Support "--flags reset" option
lib/efi_loader/efi_capsule.c | 13 +++++++++++++ tools/mkeficapsule.c | 26 ++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-)
-- Masami Hiramatsu masami.hiramatsu@linaro.org
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); + } + return ret; } #else
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
Support "--flags reset" option to set the CAPSULE_FLAGS_INITIATE_RESET flag to capsule header.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- tools/mkeficapsule.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 4995ba4e0c..ca3a1c77ad 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -44,6 +44,7 @@ static struct option options[] = { {"raw", required_argument, NULL, 'r'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'}, + {"flags", required_argument, NULL, 'F' }, {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0}, }; @@ -57,12 +58,13 @@ static void print_usage(void) "\t-r, --raw <raw image> new raw image file\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n" + "\t-F, --flags <flags> set capsule flags (support only "reset")\n" "\t-h, --help print a help message\n", tool_name); }
static int create_fwbin(char *path, char *bin, efi_guid_t *guid, - unsigned long index, unsigned long instance) + unsigned long index, unsigned long instance, u32 flags) { struct efi_capsule_header header; struct efi_firmware_management_capsule_header capsule; @@ -101,7 +103,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, header.capsule_guid = efi_guid_fm_capsule; header.header_size = sizeof(header); /* TODO: The current implementation ignores flags */ - header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET; + header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET | flags; header.capsule_image_size = sizeof(header) + sizeof(capsule) + sizeof(u64) + sizeof(image) @@ -171,6 +173,14 @@ err_1: return -1; }
+int decode_capsule_flags(const char *flagstr, u32 *flags) +{ + if (strcmp(flagstr, "reset")) + return -EINVAL; + *flags = CAPSULE_FLAGS_INITIATE_RESET; + return 0; +} + /* * Usage: * $ mkeficapsule -f <firmware binary> <output file> @@ -178,6 +188,7 @@ err_1: int main(int argc, char **argv) { char *file; + u32 flags; efi_guid_t *guid; unsigned long index, instance; int c, idx; @@ -186,8 +197,9 @@ int main(int argc, char **argv) guid = NULL; index = 0; instance = 0; + flags = 0; for (;;) { - c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx); + c = getopt_long(argc, argv, "f:r:i:I:v:F:h", options, &idx); if (c == -1) break;
@@ -214,6 +226,12 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break; + case 'F': + if (decode_capsule_flags(optarg, &flags) < 0) { + printf("Unsupported flags %s\n", optarg); + return -1; + } + break; case 'h': print_usage(); return 0; @@ -232,7 +250,7 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); }
- if (create_fwbin(argv[optind], file, guid, index, instance) + if (create_fwbin(argv[optind], file, guid, index, instance, flags) < 0) { printf("Creating firmware capsule failed\n"); exit(EXIT_FAILURE);
On 1/25/22 12:31, Masami Hiramatsu wrote:
Support "--flags reset" option to set the CAPSULE_FLAGS_INITIATE_RESET flag to capsule header.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
tools/mkeficapsule.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 4995ba4e0c..ca3a1c77ad 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -44,6 +44,7 @@ static struct option options[] = { {"raw", required_argument, NULL, 'r'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'},
- {"flags", required_argument, NULL, 'F' }, {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0}, };
@@ -57,12 +58,13 @@ static void print_usage(void) "\t-r, --raw <raw image> new raw image file\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n"
"\t-F, --flags <flags> set capsule flags (support only \"reset\")\n"
How should a user know if this "reset" means CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 or CAPSULE_FLAGS_INITIATE_RESET 0x00040000?
We need a man-page file for mkeficapsule (cf. /doc/mkimage.1 for mkimage).
The tool should support CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000 even if it is not implemented in U-Boot yet.
CAPSULE_FLAGS_PERSIST_ACROSS_RESET should not be set automatically if --flags is provided.
Isn't CAPSULE_FLAGS_PERSIST_ACROSS_RESET | CAPSULE_FLAGS_INITIATE_RESET what we need by default?
Best regards
Heinrich
"\t-h, --help print a help message\n", tool_name);}
static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
unsigned long index, unsigned long instance)
unsigned long index, unsigned long instance, u32 flags)@@ -101,7 +103,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, header.capsule_guid = efi_guid_fm_capsule; header.header_size = sizeof(header); /* TODO: The current implementation ignores flags */
- header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET;
- header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET | flags; header.capsule_image_size = sizeof(header) + sizeof(capsule) + sizeof(u64) + sizeof(image)
@@ -171,6 +173,14 @@ err_1: return -1; }
+int decode_capsule_flags(const char *flagstr, u32 *flags) +{
- if (strcmp(flagstr, "reset"))
return -EINVAL;- *flags = CAPSULE_FLAGS_INITIATE_RESET;
- return 0;
+}
- /*
- Usage:
- $ mkeficapsule -f <firmware binary> <output file>
@@ -178,6 +188,7 @@ err_1: int main(int argc, char **argv) { char *file;
- u32 flags; efi_guid_t *guid; unsigned long index, instance; int c, idx;
@@ -186,8 +197,9 @@ int main(int argc, char **argv) guid = NULL; index = 0; instance = 0;
- flags = 0; for (;;) {
c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
c = getopt_long(argc, argv, "f:r:i:I:v:F:h", options, &idx);@@ -214,6 +226,12 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break;
case 'F':if (decode_capsule_flags(optarg, &flags) < 0) {printf("Unsupported flags %s\n", optarg);return -1;}break;@@ -232,7 +250,7 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); }
- if (create_fwbin(argv[optind], file, guid, index, instance)
- if (create_fwbin(argv[optind], file, guid, index, instance, flags) < 0) { printf("Creating firmware capsule failed\n"); exit(EXIT_FAILURE);
Hi Heinrich,
Thanks for reviewing. BTW, I actually don't need this feature anymore. I and Takahiro had misunderstood what the initiate-reset flag meant. But maybe this flag support itself will be good to have.
2022年2月5日(土) 15:47 Heinrich Schuchardt xypron.glpk@gmx.de:
On 1/25/22 12:31, Masami Hiramatsu wrote:
Support "--flags reset" option to set the CAPSULE_FLAGS_INITIATE_RESET flag to capsule header.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
tools/mkeficapsule.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 4995ba4e0c..ca3a1c77ad 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -44,6 +44,7 @@ static struct option options[] = { {"raw", required_argument, NULL, 'r'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'},
{"flags", required_argument, NULL, 'F' }, {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0},@@ -57,12 +58,13 @@ static void print_usage(void) "\t-r, --raw <raw image> new raw image file\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n"
"\t-F, --flags <flags> set capsule flags (support only \"reset\")\n"How should a user know if this "reset" means CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 or CAPSULE_FLAGS_INITIATE_RESET 0x00040000?
Ah, right. "initiate-reset" "persist-across-reset" will be bettter?
We need a man-page file for mkeficapsule (cf. /doc/mkimage.1 for mkimage).
Agreed.
The tool should support CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000 even if it is not implemented in U-Boot yet.
Yes, indeed.
CAPSULE_FLAGS_PERSIST_ACROSS_RESET should not be set automatically if --flags is provided.
Isn't CAPSULE_FLAGS_PERSIST_ACROSS_RESET | CAPSULE_FLAGS_INITIATE_RESET what we need by default?
I'm not sure but I guess this depends on the platform. If the platform firmware *runtime* service can update the firmware, we don't need that flag. But if the runtime service can NOT update the firmware (e.g. no storage driver) CAPSULE_FLAGS_PERSIST_ACROSS_RESET is required to update firmware in the next boot time (and this requires OS to do a warm reset if I understand correctly). And the CAPSULE_FLAGS_INITIATE_RESET forces to reset the system when we call the runtime service, that may ensure the system does warm reset, but may not good for the OS because it suddenly reboot the system.
Anyway, for capsule-on-disk update, we don't need those flags by default.
Thank you,
Best regards
Heinrich
"\t-h, --help print a help message\n", tool_name);}
static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
unsigned long index, unsigned long instance)
unsigned long index, unsigned long instance, u32 flags)@@ -101,7 +103,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, header.capsule_guid = efi_guid_fm_capsule; header.header_size = sizeof(header); /* TODO: The current implementation ignores flags */
header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET;
header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET | flags; header.capsule_image_size = sizeof(header) + sizeof(capsule) + sizeof(u64) + sizeof(image)@@ -171,6 +173,14 @@ err_1: return -1; }
+int decode_capsule_flags(const char *flagstr, u32 *flags) +{
if (strcmp(flagstr, "reset"))return -EINVAL;*flags = CAPSULE_FLAGS_INITIATE_RESET;return 0;+}
- /*
- Usage:
- $ mkeficapsule -f <firmware binary> <output file>
@@ -178,6 +188,7 @@ err_1: int main(int argc, char **argv) { char *file;
u32 flags; efi_guid_t *guid; unsigned long index, instance; int c, idx;@@ -186,8 +197,9 @@ int main(int argc, char **argv) guid = NULL; index = 0; instance = 0;
flags = 0; for (;;) {
c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
c = getopt_long(argc, argv, "f:r:i:I:v:F:h", options, &idx); if (c == -1) break;@@ -214,6 +226,12 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break;
case 'F':if (decode_capsule_flags(optarg, &flags) < 0) {printf("Unsupported flags %s\n", optarg);return -1;}break; case 'h': print_usage(); return 0;@@ -232,7 +250,7 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); }
if (create_fwbin(argv[optind], file, guid, index, instance)
if (create_fwbin(argv[optind], file, guid, index, instance, flags) < 0) { printf("Creating firmware capsule failed\n"); exit(EXIT_FAILURE);
-- Masami Hiramatsu
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Masami Hiramatsu