On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima <masahisa.koj...@linaro.org> > wrote: > > > > Hi Ilias, > > > > On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas > > <ilias.apalodi...@linaro.org> wrote: > > > > > > Kojima-san > > > > > > On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote: > > > > This commit stores the firmware version into the array > > > > of fmp_state structure to support the fmp versioning > > > > for multi bank update. The index of the array is identified > > > > by the bank index. > > > > > > Why do we all of them? Can't we just always store/use the version of the > > > active > > > bank? > > > > Sorry for the late reply. > > When the capsule update is reverted, the active bank is back to the > > previous active bank. > > So I think versions for all banks should be stored. > > But even in that case, the active bank should point to the correct > version when the ESRT tables are generated. Wouldn't it be simpler to > just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and FmpState variable is set when the UEFI capsule update is performed. If we only store the fw_version of the active bank, the fw_version of the previous active bank is overwritten. When the capsule is reverted, we can not get the fw_version of the previous active bank. Thanks, Masahisa Kojima > Am I missing anything? > > Thanks > /Ilias > > > > Thanks, > > Masahisa Kojima > > > > > > > > > > > > > This modification keeps the backward compatibility with > > > > the existing versioning feature. > > > > > > > > > > Thanks > > > /Ilias > > > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > > > > --- > > > > lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- > > > > 1 file changed, 54 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c > > > > b/lib/efi_loader/efi_firmware.c > > > > index 9b8630625f..5459f3d38d 100644 > > > > --- a/lib/efi_loader/efi_firmware.c > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct > > > > efi_firmware_image_descriptor *image_ > > > > { > > > > u16 varname[13]; /* u"FmpStateXXXX" */ > > > > efi_status_t ret; > > > > - efi_uintn_t size; > > > > - struct fmp_state var_state = { 0 }; > > > > - > > > > - efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > > > - fw_array->image_index); > > > > - size = sizeof(var_state); > > > > - ret = efi_get_variable_int(varname, &fw_array->image_type_id, > > > > - NULL, &size, &var_state, NULL); > > > > - if (ret == EFI_SUCCESS) > > > > - image_info->version = var_state.fw_version; > > > > - else > > > > - image_info->version = 0; > > > > + efi_uintn_t size, expected_size; > > > > + uint num_banks = 1; > > > > + uint active_index = 0; > > > > + struct fmp_state *var_state; > > > > > > > > efi_firmware_get_lsv_from_dtb(fw_array->image_index, > > > > &fw_array->image_type_id, > > > > @@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct > > > > efi_firmware_image_descriptor *image_ > > > > image_info->version_name = NULL; /* not supported */ > > > > image_info->last_attempt_version = 0; > > > > image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > > > + image_info->version = 0; > > > > + > > > > + /* get the fw_version */ > > > > + efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > > > + fw_array->image_index); > > > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > > > > + ret = fwu_get_active_index(&active_index); > > > > + if (ret) > > > > + return; > > > > + > > > > + num_banks = CONFIG_FWU_NUM_BANKS; > > > > + } > > > > + > > > > + size = num_banks * sizeof(*var_state); > > > > + expected_size = size; > > > > + var_state = calloc(1, size); > > > > + if (!var_state) > > > > + return; > > > > + > > > > + ret = efi_get_variable_int(varname, &fw_array->image_type_id, > > > > + NULL, &size, var_state, NULL); > > > > + if (ret == EFI_SUCCESS && expected_size == size) > > > > + image_info->version = var_state[active_index].fw_version; > > > > + > > > > + free(var_state); > > > > } > > > > > > > > /** > > > > @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct > > > > fmp_state *state, u8 image_in > > > > { > > > > u16 varname[13]; /* u"FmpStateXXXX" */ > > > > efi_status_t ret; > > > > + uint num_banks = 1; > > > > + uint update_bank = 0; > > > > + efi_uintn_t size; > > > > efi_guid_t *image_type_id; > > > > - struct fmp_state var_state = { 0 }; > > > > + struct fmp_state *var_state; > > > > > > > > image_type_id = efi_firmware_get_image_type_id(image_index); > > > > if (!image_type_id) > > > > @@ -372,19 +392,38 @@ efi_status_t > > > > efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in > > > > efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > > > image_index); > > > > > > > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > > > > + ret = fwu_plat_get_update_index(&update_bank); > > > > + if (ret) > > > > + return EFI_INVALID_PARAMETER; > > > > + > > > > + num_banks = CONFIG_FWU_NUM_BANKS; > > > > + } > > > > + > > > > + size = num_banks * sizeof(*var_state); > > > > + var_state = calloc(1, size); > > > > + if (!var_state) > > > > + return EFI_OUT_OF_RESOURCES; > > > > + > > > > + ret = efi_get_variable_int(varname, image_type_id, NULL, &size, > > > > + var_state, NULL); > > > > + > > > > /* > > > > * Only the fw_version is set here. > > > > * lowest_supported_version in FmpState variable is ignored since > > > > * it can be tampered if the file based EFI variable storage is > > > > used. > > > > */ > > > > - var_state.fw_version = state->fw_version; > > > > + var_state[update_bank].fw_version = state->fw_version; > > > > > > > > + size = num_banks * sizeof(*var_state); > > > > ret = efi_set_variable_int(varname, image_type_id, > > > > EFI_VARIABLE_READ_ONLY | > > > > EFI_VARIABLE_NON_VOLATILE | > > > > EFI_VARIABLE_BOOTSERVICE_ACCESS | > > > > EFI_VARIABLE_RUNTIME_ACCESS, > > > > - sizeof(var_state), &var_state, false); > > > > + size, var_state, false); > > > > + > > > > + free(var_state); > > > > > > > > return ret; > > > > } > > > > -- > > > > 2.34.1 > > > >