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

[Heinrich Schuchardt]

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.

> +		u32 fw_version = 0;
> +		u32 lowest_supported_version = 0;

These assignments should be in efi_firmware_get_firmware_version.

> +
>   		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.

>   		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.

> +		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().

Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
will simplify the code.

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);
>   }
>