Hi Heinrich, On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.g...@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.koj...@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. 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); > > } > > >