[PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Masahisa Kojima]

Hi Akashi-san,

On Fri, 3 Mar 2023 at 09:10, Takahiro Akashi <takahiro.akashi at linaro.org> wrote:
>
> On Thu, Mar 02, 2023 at 07:05:50PM +0900, Masahisa Kojima wrote:
> > On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi at linaro.org> wrote:
> > >
> > > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote:
> > > > Current FMP->GetImageInfo() always return 0 for the firmware
> > > > version, user can not identify which firmware version is currently
> > > > running through the EFI interface.
> > > >
> > > > This commit reads the "FmpStateXXXX" EFI variable, then fills the
> > > > firmware version, lowest supported version, last attempt version
> > > > and last attempt status in FMP->GetImageInfo().
> > > >
> > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > > > ---
> > > > No update since v1
> > > >
> > > >  lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
> > > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > > index d1afafb052..ead20fa914 100644
> > > > --- a/lib/efi_loader/efi_firmware.c
> > > > +++ b/lib/efi_loader/efi_firmware.c
> > > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
> > > >       *package_version_name = NULL; /* not supported */
> > > >
> > > >       for (i = 0; i < num_image_type_guids; i++) {
> > > > +             u16 varname[13]; /* u"FmpStateXXXX" */
> > > > +             efi_status_t ret;
> > > > +             efi_uintn_t size;
> > > > +             struct fmp_state var_state = { 0 };
> > > > +
> > > >               image_info[i].image_index = fw_array[i].image_index;
> > > >               image_info[i].image_type_id = fw_array[i].image_type_id;
> > > >               image_info[i].image_id = fw_array[i].image_index;
> > > >
> > > >               image_info[i].image_id_name = fw_array[i].fw_name;
> > > >
> > > > -             image_info[i].version = 0; /* not supported */
> > > > +             efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > > +                                     fw_array[i].image_index);
> > >
> > > Don't we have to think of the systems where multiple FMP drivers are
> > > used? In those cases, 'image_index' doesn't work as an unique ID.
> > > It is unlikely under the current code, but we should consider
> > > any future extension.
> >
> > I assume that other FMP drivers implement their own version management.
>
> I don't have a strong opinion, but my idea about a variable name is
> to use 'image_type_id' as a 'vendor' field which allows for making the variable
> globally unique.

Thank you for the suggestion, I agree to use image_type_id as vendor field.

Regards,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > +             size = sizeof(var_state);
> > > > +             ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
> > > > +                                        &size, &var_state, NULL);
> > > > +             if (ret == EFI_SUCCESS) {
> > > > +                     image_info[i].version = var_state.fw_version;
> > > > +                     image_info[i].lowest_supported_image_version =
> > > > +                             var_state.lowest_supported_version;
> > > > +                     image_info[i].last_attempt_version =
> > > > +                             var_state.last_attempt_version;
> > > > +                     image_info[i].last_attempt_status =
> > > > +                             var_state.last_attempt_status;
> > > > +             } else {
> > > > +                     image_info[i].version = 0;
> > > > +                     image_info[i].lowest_supported_image_version = 0;
> > > > +                     image_info[i].last_attempt_version = 0;
> > > > +                     image_info[i].last_attempt_status =
> > > > +                             LAST_ATTEMPT_STATUS_SUCCESS;
> > > > +             }
> > > > +
> > > >               image_info[i].version_name = NULL; /* not supported */
> > > >               image_info[i].size = 0;
> > > >               image_info[i].attributes_supported =
> > > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
> > > >                       image_info[0].attributes_setting |=
> > > >                               IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> > > >
> > > > -             image_info[i].lowest_supported_image_version = 0;
> > > > -             image_info[i].last_attempt_version = 0;
> > > > -             image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > > >               image_info[i].hardware_instance = 1;
> > > >               image_info[i].dependencies = NULL;
> > > >       }
> > > > --
> > > > 2.17.1
> > > >