[PATCH v13 10/15] FWU: Add support for the FWU Multi Bank Update feature

[Sughosh Ganu]

On Fri, 14 Oct 2022 at 13:29, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> [...]
>
> > > > > > +}
> > > > > > +
> > > > > > +static __maybe_unused efi_status_t fwu_empty_capsule_process(
> > > > > > +     struct efi_capsule_header *capsule)
> > > > > > +{
> > > > > > +     int status;
> > > > > > +     u32 active_idx;
> > > > > > +     efi_status_t ret;
> > > > > > +     efi_guid_t *image_guid;
> > > > > > +
> > > > > > +     if (!guidcmp(&capsule->capsule_guid,
> > > > > > +                  &fwu_guid_os_request_fw_revert)) {
> > > > > > +             /*
> > > > > > +              * One of the previously updated image has
> > > > > > +              * failed the OS acceptance test. OS has
> > > > > > +              * requested to revert back to the earlier
> > > > > > +              * boot index
> > > > > > +              */
> > > > > > +             status = fwu_revert_boot_index();
> > > > > > +             ret = fwu_to_efi_error(status);
> > > > > > +             if (ret == EFI_SUCCESS)
> > > > > > +                     log_info("Reverted the FWU active_index. Recommend rebooting the system\n");
> > > > > > +             else
> > > > > > +                     log_err("Failed to revert the FWU boot index\n");
> > > > > > +     } else {
> > > > >
> > > > > We should be explicit on the GUID checking here.  IOW if someone hands you
> > > > > over and empty capsule with a guid !fwu_guid_os_request_fw_revert you'll
> > > > > accept the capsule
> > > >
> > > > This won't happen since this function is called only for empty
> > > > capsules. Will handle the other two review comments.
> > >
> > > I am not sure I am following.  Why can't it happen?  If someone create
> > > an empty capsule with an invalid (by invalid I mean none of the 2
> > > GUIDs the spec expects), you'll end up trying to accept the firmware
> > > no?
> >
> > That will be checked in the fwu_empty_capsule(). Only if this function
> > returns true, will the fwu_empty_capsule_process() get called. So the
> > capsule_guid should either be one for accept or a revert capsule.
> >
> > -sughosh
>
> Right in this case then why don't we pass an extra argument to
> fwu_empty_capsule_process() instead and skip the extra guidcmp?

Yes, I can do that, but in any case I need to pass the capsule header
for getting the image_guid for the accept capsule. Hence the check in
the function. Moreover, if this is to be passed as an argument, it
will have to be determined first, whether it is an accept or a revert
capsule, and that will have to be done in fwu_empty_capsule(), which
is basically being used only for identification of the empty capsule,
since it is also called in fwu_post_update_checks(). If you have a
strong opinion on this, I will change the fwu_empty_capsule()
implementation.

-sughosh

>
> Thanks
> /Ilias
> >
> > >
> > > Thanks
> > > /Ilias
> > > >
> > > > -sughosh
> > > >
> > > > >
> > > > > > +             /*
> > > > > > +              * Image accepted by the OS. Set the acceptance
> > > > > > +              * status for the image.
> > > > > > +              */
> > > > > > +             image_guid = (void *)(char *)capsule +
> > > > > > +                     capsule->header_size;
> > > > > > +
> > > > > > +             status = fwu_get_active_index(&active_idx);
> > > > > > +             ret = fwu_to_efi_error(status);
> > > > > > +             if (ret != EFI_SUCCESS) {
> > > > > > +                     log_err("Unable to get the active_index from the FWU metadata\n");
> > > > > > +                     return ret;
> > > > > > +             }
> > > > > > +
> > > > > > +             status = fwu_accept_image(image_guid, active_idx);
> > > > > > +             ret = fwu_to_efi_error(status);
> > > > > > +             if (ret != EFI_SUCCESS)
> > > > > > +                     log_err("Unable to set the Accept bit for the image %pUs\n",
> > > > > > +                             image_guid);
> > > > > > +     }
> > > > > > +
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static __maybe_unused void fwu_post_update_checks(
> > > > > > +     struct efi_capsule_header *capsule,
> > > > > > +     bool *fw_accept_os, bool *capsule_update)
> > > > > > +{
> > > > > > +     if (fwu_empty_capsule(capsule))
> > > > > > +             *capsule_update = false;
> > > > > > +     else
> > > > > > +             if (!*fw_accept_os)
> > > > >
> > > > > This line should fold to the upper one
> > > > >
> > > > > > +                     *fw_accept_os =
> > > > > > +                             capsule->flags & FW_ACCEPT_OS ? true : false;
> > > > > > +}
> > > > > > +
> > > > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > > > > +{
> > > > > > +     int status;
> > > > > > +     u32 update_index;
> > > > > > +     efi_status_t ret;
> > > > > > +
> > > > > > +     status = fwu_plat_get_update_index(&update_index);
> > > > > > +     if (status < 0) {
> > > > > > +             log_err("Failed to get the FWU update_index value\n");
> > > > > > +             return EFI_DEVICE_ERROR;
> > > > > > +     }
> > > > > > +
> > > > > > +     /*
> > > > > > +      * All the capsules have been updated successfully,
> > > > > > +      * update the FWU metadata.
> > > > > > +      */
> > > > > > +     log_debug("Update Complete. Now updating active_index to %u\n",
> > > > > > +               update_index);
> > > > > > +     status = fwu_set_active_index(update_index);
> > > > > > +     ret = fwu_to_efi_error(status);
> > > > > > +     if (ret != EFI_SUCCESS) {
> > > > > > +             log_err("Failed to update FWU metadata index values\n");
> > > > > > +     } else {
> > > > > > +             log_debug("Successfully updated the active_index\n");
> > > > >
> > > > > [...]
> > > > >
> > > > > Thanks
> > > > > /Ilias