[PATCH v5 1/4] efi_loader: get version information from device tree
Heinrich Schuchardt
xypron.glpk at gmx.de
Mon May 8 11:44:20 CEST 2023
On 5/8/23 10:15, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk at 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 at 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?
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);
>>> }
>>>
>>
More information about the U-Boot
mailing list