On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.g...@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.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. > > 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 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); > >>> } > >>> > >> >