Re: [PATCH 02/18] fwu: metadata: Migrate to version 2 of the structure

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sughosh,

On Mon, Jan 22, 2024 at 05:24:23PM +0530, Sughosh Ganu wrote:
> The latest version of the FWU specification [1] has changes to the
> metadata structure. This is version 2 of the structure.
>
> Primary changes include
>  - bank_state field in the top level structure
>  - Total metadata size in the top level structure
>  - Image description structures now optional
>  - Number of banks and images per bank values part of the structure
>
> Migrate to the version 2 of the metadata structure.
>
> [1] - https://developer.arm.com/documentation/den0118/latest/
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/fwu_mdata.h | 56 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> index 56189e2f40..050ee969e3 100644
> --- a/include/fwu_mdata.h
> +++ b/include/fwu_mdata.h
> @@ -11,7 +11,7 @@
>
>  /**
>   * struct fwu_image_bank_info - firmware image information
> - * @image_uuid: Guid value of the image in this bank
> + * @image_guid: Guid value of the image in this bank
>   * @accepted: Acceptance status of the image
>   * @reserved: Reserved
>   *
> @@ -20,15 +20,15 @@
>   * acceptance status
>   */
>  struct fwu_image_bank_info {
> -	efi_guid_t  image_uuid;
> +	efi_guid_t  image_guid;
>  	uint32_t accepted;
>  	uint32_t reserved;
>  } __packed;
>
>  /**
>   * struct fwu_image_entry - information for a particular type of image
> - * @image_type_uuid: Guid value for identifying the image type
> - * @location_uuid: Guid of the storage volume where the image is located
> + * @image_type_guid: Guid value for identifying the image type
> + * @location_guid: Guid of the storage volume where the image is located
>   * @img_bank_info: Array containing properties of images
>   *
>   * This structure contains information on various types of updatable
> @@ -36,9 +36,30 @@ struct fwu_image_bank_info {
>   * information per bank.
>   */
>  struct fwu_image_entry {
> -	efi_guid_t image_type_uuid;
> -	efi_guid_t location_uuid;
> -	struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> +	efi_guid_t image_type_guid;
> +	efi_guid_t location_guid;
> +	struct fwu_image_bank_info img_bank_info[];
> +} __packed;
> +
> +/**
> + * struct fwu_fw_desc_store - FWU updatable image information
> + * @num_banks: Number of firmware banks
> + * num_images: Number of images per bank
> + * @img_entry_size: The size of the img_entry array
> + * @bank_info_entry_size: The size of the img_bank_info array
> + * @img_entry: Array of image entries each giving information on a image
> + *
> + * This image descriptor structure contains information on the number of
> + * updatable banks and images per bank. It also gives the total sizes of
> + * the fwu_image_entry and fwu_image_bank_info arrays.
> + */
> +struct fwu_fw_store_desc {
> +	uint8_t  num_banks;
> +	uint8_t  reserved;
> +	uint16_t num_images;
> +	uint16_t img_entry_size;
> +	uint16_t bank_info_entry_size;
> +	struct fwu_image_entry img_entry[];

This is a bit problematic. When a struct is defined using flexible array
members, you are not supposed to include it on other structs or arrays [0].
We need a way of working around this

>  } __packed;
>
>  /**
> @@ -48,21 +69,28 @@ struct fwu_image_entry {
>   * @active_index: Index of the bank currently used for booting images
>   * @previous_active_inde: Index of the bank used before the current bank
>   *                        being used for booting
> - * @img_entry: Array of information on various firmware images that can
> - *             be updated
> + * @metadata_size: Size of the entire metadata structure, including the
> + *                 image descriptors
> + * @desc_offset: The offset from the start of this structure where the
> + *               image descriptor structure starts. 0 if absent
> + * @bank_state: State of each bank, valid, invalid or accepted
> + * @fw_desc: The structure describing the FWU updatable images
>   *
> - * This structure is used to store all the needed information for performing
> + * This is the top level structure used to store all information for performing
>   * multi bank updates on the platform. This contains info on the bank being
> - * used to boot along with the information needed for identification of
> - * individual images
> + * used to boot along with the information on state of individual banks.
>   */
>  struct fwu_mdata {
>  	uint32_t crc32;
>  	uint32_t version;
>  	uint32_t active_index;
>  	uint32_t previous_active_index;
> -
> -	struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +	uint32_t metadata_size;
> +	uint16_t desc_offset;
> +	uint16_t reserved1;
> +	uint8_t  bank_state[4];
> +	uint32_t reserved2;
> +	struct fwu_fw_store_desc fw_desc[];

ditto

>  } __packed;
>
>  #endif /* _FWU_MDATA_H_ */
> --
> 2.34.1
>

[0] git show b10bfd0019f601