| Message ID | 20231114092508.3955838-3-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fix FMP versioning for FWU multi bank update | expand |
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@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 >
Hi Heinrich, On Sun, 10 Dec 2023 at 00:09, Heinrich Schuchardt <xypron.glpk@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.kojima@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 > > >
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
Current FMP versioning uses the FMPStateXXXX UEFI variables 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. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- lib/efi_loader/efi_firmware.c | 63 ++++++++++++++++++++++++++--------- lib/fwu_updates/Kconfig | 1 + 2 files changed, 48 insertions(+), 16 deletions(-)