Re: [PATCH v5 1/4] efi_loader: get version information from device tree

[Takahiro Akashi <takahiro.akashi@linaro.org>]

On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote:
> On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 5/8/23 10:15, Masahisa Kojima wrote:
> > > Hi Heinrich,
> > >
> > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 4/10/23 11:07, 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 gets the version information from device tree,
> > >>> then fills the firmware version, lowest supported version
> > >>> in FMP->GetImageInfo().
> > >>>
> > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > >>>
> > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > >>> ---
> > >>> Changes in v5:
> > >>> - newly implement a device tree based versioning
> > >>>
> > >>>    .../firmware/firmware-version.txt             | 25 ++++++++
> > >>>    lib/efi_loader/efi_firmware.c                 | 63 +++++++++++++++++--
> > >>>    2 files changed, 84 insertions(+), 4 deletions(-)
> > >>>    create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> > >>>
> > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> > >>> new file mode 100644
> > >>> index 0000000000..6112de4a1d
> > >>> --- /dev/null
> > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> > >>> @@ -0,0 +1,25 @@
> > >>> +firmware-version bindings
> > >>> +-------------------------------
> > >>> +
> > >>> +Required properties:
> > >>> +- image-type-id                      : guid for image blob type
> > >>> +- image-index                        : image index
> > >>> +- fw-version                 : firmware version
> > >>> +- lowest-supported-version   : lowest supported version
> > >>> +
> > >>> +Example:
> > >>> +
> > >>> +     firmware-version {
> > >>> +             image1 {
> > >>> +                     image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> > >>> +                     image-index = <1>;
> > >>> +                     fw-version = <5>;
> > >>> +                     lowest-supported-version = <3>;
> > >>> +             };
> > >>> +             image2 {
> > >>> +                     image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> > >>> +                     image-index = <2>;
> > >>> +                     fw-version = <10>;
> > >>> +                     lowest-supported-version = <7>;
> > >>> +             };
> > >>> +     };
> > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > >>> index 93e2b01c07..1c6ef468bf 100644
> > >>> --- a/lib/efi_loader/efi_firmware.c
> > >>> +++ b/lib/efi_loader/efi_firmware.c
> > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > >>>        return EFI_EXIT(EFI_UNSUPPORTED);
> > >>>    }
> > >>>
> > >>> +/**
> > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb
> > >>> + * @image_index:     Image index
> > >>> + * @image_type_id:   Image type id
> > >>> + * @fw_version:              Pointer to store the version number
> > >>> + * @lsv:             Pointer to store the lowest supported version
> > >>> + *
> > >>> + * Authenticate the capsule if authentication is enabled.
> > >>> + * The image pointer and the image size are updated in case of success.
> > >>> + */
> > >>> +void efi_firmware_get_firmware_version(u8 image_index,
> > >>> +                                    efi_guid_t *image_type_id,
> > >>> +                                    u32 *fw_version, u32 *lsv)
> > >>> +{
> > >>> +     const void *fdt = gd->fdt_blob;
> > >>> +     const fdt32_t *val;
> > >>> +     const char *guid_str;
> > >>> +     int len, offset, index;
> > >>> +     int parent;
> > >>> +
> > >>> +     parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> > >>> +     if (parent < 0)
> > >>> +             return;
> > >>> +
> > >>> +     fdt_for_each_subnode(offset, fdt, parent) {
> > >>> +             efi_guid_t guid;
> > >>> +
> > >>> +             guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> > >>> +             if (!guid_str)
> > >>> +                     continue;
> > >>> +             uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> > >>> +
> > >>> +             val = fdt_getprop(fdt, offset, "image-index", &len);
> > >>> +             if (!val)
> > >>> +                     continue;
> > >>> +             index = fdt32_to_cpu(*val);
> > >>> +
> > >>> +             if (!guidcmp(&guid, image_type_id) && index == image_index) {
> > >>> +                     val = fdt_getprop(fdt, offset, "fw-version", &len);
> > >>> +                     if (val)
> > >>> +                             *fw_version = fdt32_to_cpu(*val);
> > >>> +
> > >>> +                     val = fdt_getprop(fdt, offset,
> > >>> +                                       "lowest-supported-version", &len);
> > >>> +                     if (val)
> > >>> +                             *lsv = fdt32_to_cpu(*val);
> > >>> +             }
> > >>> +     }
> > >>> +}
> > >>> +
> > >>>    /**
> > >>>     * efi_fill_image_desc_array - populate image descriptor array
> > >>>     * @image_info_size:                Size of @image_info
> > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> > >>>        *package_version_name = NULL; /* not supported */
> > >>>
> > >>>        for (i = 0; i < num_image_type_guids; i++) {
> > >>
> > >> Currently we define num_image_type_guids per board in a C file. Using
> > >> the same line of code once per board makes no sense to me. Please, move
> > >> the definition of that variable to lib/efi_loader/efi_firmware.c.
> > >
> > > Sorry for the late reply.
> > >
> > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
> > > fw_images[] array is also defined in each board file,
> > > so we can not simply move num_image_type_guids into
> > > lib/efi_loader/efi_firmware.c.
> >
> > Why can't we have
> >
> >    int num_image_type_guids = ARRAY_SIZE(fw_images);
> >
> > in lib/efi_loader/efi_firmware.c?
> 
> At first thought, I thought it was a matter of abstraction.
> 
> But there is a compilation error when we expose fw_images[].
> fw_images[] array is initialized in each board file,
> and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c
> will cause compilation failure.
> We need to specify the array size when fw_images is exposed,
> for example:
>   extern struct efi_fw_image fw_images[2];
> 
> but currently there is no method to pre-define the fw_images[] array size,
> it is board specific.
> 
> We can define the macro to indicate the array size or having
> sentinel in the fw_images[] array, but I think the current

I simply wonder if the value should be embedded in "struct efi_capsule_update_info".
   struct efi_capsule_update_info {
        const char *dfu_string;
        int num_images;                <- added
        struct efi_fw_image *images;
   };
This is the best place because the value must match not only "images"
but also (entries in) "dfu_string".

Even now, efi_fill_image_desc_array() tries to access "fw_images[]"
via the exposed update_info variable. Beautiful, isn't it?

One more comment:
uefi.rst doesn't mention anything about num_image_type_guids.

-Takahiro Akashi

> implementation is simpler,
> I would like to keep the current implementation.
> Correct me if I'm wrong.
> 
> Thanks,
> Masahisa Kojima
> 
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > And fw_images[] array is abstracted by struct efi_capsule_update_info,
> > > so I think
> > > we should not extract the fw_images[] array.
> > >
> > >>
> > >>> +             u32 fw_version = 0;
> > >>> +             u32 lowest_supported_version = 0;
> > >>
> > >> These assignments should be in efi_firmware_get_firmware_version.
> > >
> > > OK.
> > >
> > >>
> > >>> +
> > >>>                image_info[i].image_index = fw_array[i].image_index;
> > >>
> > >> Why did we ever introduce the field image_index? Please, eliminate it it
> > >> as the GUID is always sufficient to identify an image.
> > >
> > > This is derived from the UEFI specification.
> > > UEFI specification "23.1.2
> > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
> > > and ImageTypeId(guid).
> > >
> > > ImageIndex: A unique number identifying the firmware image within the
> > > device. The number is between 1 and
> > > DescriptorCount.
> > >
> > >>
> > >>>                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_firmware_get_firmware_version(fw_array[i].image_index,
> > >>> +                                               &fw_array[i].image_type_id,
> > >>> +                                               &fw_version,
> > >>> +                                               &lowest_supported_version);
> > >>
> > >> This interface makes no sense to me. We expect images with specific
> > >> GUIDs and should not care about images with other GUIDs that may
> > >> additionally exist in the capsule.
> > >>
> > >> So you must pass the expected GUID as input variable here.
> > >
> > > I don't clearly understand this comment, but the expected GUID is
> > > fw_array[i].image_type_id.
> > >
> > >>
> > >>> +             image_info[i].version = fw_version;
> > >>>                image_info[i].version_name = NULL; /* not supported */
> > >>
> > >> Please, add the missing functionality to
> > >> efi_firmware_get_firmware_version().
> > >
> > > Does it mean we need to support version_name?
> > > I can add a version_name in dtb.
> > >
> > >>
> > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
> > >> will simplify the code.
> > >
> > > OK.
> > >
> > > Thank you for your review.
> > >
> > > Regards,
> > > Masahisa Kojima
> > >
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>>                image_info[i].size = 0;
> > >>>                image_info[i].attributes_supported =
> > >>> @@ -168,7 +224,7 @@ 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].lowest_supported_image_version = lowest_supported_version;
> > >>>                image_info[i].last_attempt_version = 0;
> > >>>                image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > >>>                image_info[i].hardware_instance = 1;
> > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> > >>>                                        descriptor_version, descriptor_count,
> > >>>                                        descriptor_size, package_version,
> > >>>                                        package_version_name);
> > >>> -
> > >>>        return EFI_EXIT(ret);
> > >>>    }
> > >>>
> > >>
> >