[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 at 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 at 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