[PATCH 0/2] EFI: Add CAPSULE_FLAG_INITIATE_RESET support

[PATCH 0/2] EFI: Add CAPSULE_FLAG_INITIATE_RESET support

[Masami Hiramatsu]

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>

[PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[Masami Hiramatsu]

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

[PATCH 2/2] mkeficapsule: Support "--flags reset" option

[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);

Re: [PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[AKASHI Takahiro]

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
> 

Re: [PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[Masami Hiramatsu]

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

Re: [PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[Masami Hiramatsu]

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



-- 
Masami Hiramatsu

Re: [PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[AKASHI Takahiro]

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

Re: [PATCH 1/2] EFI: Support CAPSULE_FLAGS_INITIATE_RESET for capsule update

[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



-- 
Masami Hiramatsu

Re: [PATCH 2/2] mkeficapsule: Support "--flags reset" option

[Heinrich Schuchardt]

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)
>   {
>   	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);
>

Re: [PATCH 2/2] mkeficapsule: Support "--flags reset" option

[Masami Hiramatsu]

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)
> >   {
> >       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);
> >
>


--
Masami Hiramatsu