Hi Heinrich,
On Sun, 10 Dec 2023 at 00:09, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> 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.
Thanks. I will implement the next version with the array of versions
of each bank.
Regards,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.koj...@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
> >
>
