[PATCH 2/2] efi_loader: support FMP versioning for FWU multi bank update

[Heinrich Schuchardt]

On 11/14/23 10:25, Masahisa Kojima wrote:
> Current FMP versioning uses the FMPStateXXXX UEFI variables

Do you mean FmpState####?

> that XXXX indicates image_index. When CONFIG_FWU_MULTI_BANK_UPDATE
> is enabled, U-Boot instances installed in different banks
> read the same FMPStateXXXX variable and it ends up with reading
> wrong fw_version.
>
> This commit includes the bank_index in the FMPStateXXYY variable.
> XX indicates a bank_index, YY indicates an image_index.
> With this, it keeps backward compatibility for the case that
> CONFIG_FWU_MULTI_BANK_UPDATE is not enabled, bank_index is
> always 0 for this case.
>
> The bank index size is defined 4 bytes in FWU-PSA-A_DEN0118_1.0ALP3.pdf,
> but 1-255 range is practically enough big. This commit adds the range
> for FWU_NUM_BANKS since above XX(bank_index) has 1 byte.

Who would need more than 16 banks (i.e. more than one digit)?

What happens when the image number reaches 256 (or 4096 for 3 digits)?
What happens with devices out in the field that already have reached
image number 256?

Why don't we store the fmp_state of all banks in the same FmpState####
variable as an array? This should have a better chance of backwards
compatibility.

Best regards

Heinrich

>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> ---
>   lib/efi_loader/efi_firmware.c | 63 ++++++++++++++++++++++++++---------
>   lib/fwu_updates/Kconfig       |  1 +
>   2 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 9c1a273926..6d3620a649 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -205,13 +205,33 @@ static
>   void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_info,
>   				    struct efi_fw_image *fw_array)
>   {
> -	u16 varname[13]; /* u"FmpStateXXXX" */
> +	int err;
> +	u16 varname[13]; /* u"FmpStateXXYY" */
>   	efi_status_t ret;
>   	efi_uintn_t size;
> +	uint active_bank = 0, var_index;
>   	struct fmp_state var_state = { 0 };
>
> +	efi_firmware_get_lsv_from_dtb(fw_array->image_index,
> +				      &fw_array->image_type_id,
> +				      &image_info->lowest_supported_image_version);
> +
> +	image_info->version_name = NULL; /* not supported */
> +	image_info->last_attempt_version = 0;
> +	image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +
> +	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> +		err = fwu_get_active_index(&active_bank);
> +		if (err) {
> +			log_debug("Unable to get active index\n");
> +			image_info->version = 0;
> +			return;
> +		}
> +	}
> +
> +	var_index = ((active_bank & 0xFF) << 8 | (fw_array->image_index & 0xFF));
>   	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> -				fw_array->image_index);
> +				var_index);
>   	size = sizeof(var_state);
>   	ret = efi_get_variable_int(varname, &fw_array->image_type_id,
>   				   NULL, &size, &var_state, NULL);
> @@ -219,14 +239,6 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_
>   		image_info->version = var_state.fw_version;
>   	else
>   		image_info->version = 0;
> -
> -	efi_firmware_get_lsv_from_dtb(fw_array->image_index,
> -				      &fw_array->image_type_id,
> -				      &image_info->lowest_supported_image_version);
> -
> -	image_info->version_name = NULL; /* not supported */
> -	image_info->last_attempt_version = 0;
> -	image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
>   }
>
>   /**
> @@ -353,15 +365,20 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>    * @state:		Pointer to fmp state
>    * @image_index:	image index
>    *
> - * Update the FmpStateXXXX variable with the firmware update state.
> + * Update the FmpStateXXYY variable with the firmware update state.
> + * XX is a bank index, YY is an image_index
> + * When CONFIG_FWU_MULTI_BANK_UPDATE is not enabled, update_bank is always 0.
>    *
>    * Return:		status code
>    */
>   static
>   efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index)
>   {
> -	u16 varname[13]; /* u"FmpStateXXXX" */
> +	int err;
> +	u16 varname[13]; /* u"FmpStateXXYY" */
>   	efi_status_t ret;
> +	uint update_bank = 0;
> +	unsigned int var_index;
>   	efi_guid_t *image_type_id;
>   	struct fmp_state var_state = { 0 };
>
> @@ -369,9 +386,15 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
>   	if (!image_type_id)
>   		return EFI_INVALID_PARAMETER;
>
> -	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> -				image_index);
> +	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> +		err = fwu_plat_get_update_index(&update_bank);
> +		if (err)
> +			return EFI_INVALID_PARAMETER;
> +	}
>
> +	var_index = ((update_bank & 0xFF) << 8 | (image_index & 0xFF));
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				var_index);
>   	/*
>   	 * Only the fw_version is set here.
>   	 * lowest_supported_version in FmpState variable is ignored since
> @@ -563,7 +586,11 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>   	if (fit_update(image))
>   		return EFI_EXIT(EFI_DEVICE_ERROR);
>
> -	efi_firmware_set_fmp_state_var(&state, image_index);
> +	status = efi_firmware_set_fmp_state_var(&state, image_index);
> +	if (status != EFI_SUCCESS) {
> +		log_err("Set FmpState variable failed\n");
> +		return EFI_EXIT(status);
> +	}
>
>   	return EFI_EXIT(EFI_SUCCESS);
>   }
> @@ -643,7 +670,11 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>   			     NULL, NULL))
>   		return EFI_EXIT(EFI_DEVICE_ERROR);
>
> -	efi_firmware_set_fmp_state_var(&state, original_image_index);
> +	status = efi_firmware_set_fmp_state_var(&state, original_image_index);
> +	if (status != EFI_SUCCESS) {
> +		log_err("Set FmpState variable failed\n");
> +		return EFI_EXIT(status);
> +	}
>
>   	return EFI_EXIT(EFI_SUCCESS);
>   }
> diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig
> index d35247d0e5..eb42f25fc0 100644
> --- a/lib/fwu_updates/Kconfig
> +++ b/lib/fwu_updates/Kconfig
> @@ -14,6 +14,7 @@ if FWU_MULTI_BANK_UPDATE
>
>   config FWU_NUM_BANKS
>   	int "Number of Banks defined by the platform"
> +	range 1 255
>   	help
>   	  Define the number of banks of firmware images on a platform
>