Sughosh,
On Tue, Sep 20, 2022 at 06:34:12PM +0530, Sughosh Ganu wrote:
On Tue, 20 Sept 2022 at 13:46, Takahiro Akashi takahiro.akashi@linaro.org wrote:
On Fri, Sep 16, 2022 at 04:24:35PM +0530, Sughosh Ganu wrote:
hi Takahiro,
On Fri, 16 Sept 2022 at 12:20, Takahiro Akashi takahiro.akashi@linaro.org wrote:
On Fri, Sep 16, 2022 at 10:52:11AM +0530, Sughosh Ganu wrote:
() hi Takahiro,
On Fri, 16 Sept 2022 at 07:17, Takahiro Akashi takahiro.akashi@linaro.org wrote:
Hi Sughosh,
On Thu, Sep 15, 2022 at 01:44:46PM +0530, Sughosh Ganu wrote: > The FWU Multi Bank Update feature supports updation of firmware images > to one of multiple sets(also called banks) of images. The firmware > images are clubbed together in banks, with the system booting images > from the active bank. Information on the images such as which bank > they belong to is stored as part of the metadata structure, which is > stored on the same storage media as the firmware images on a dedicated > partition. > > At the time of update, the metadata is read to identify the bank to > which the images need to be flashed(update bank). On a successful > update, the metadata is modified to set the updated bank as active > bank to subsequently boot from. > > Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org > --- > Changes since V9: > * Move the global variables into local variables as suggested by > Ilias. > * Change fwu_get_image_alt_num() name to fwu_get_image_image_index()
-> typo? fwu_get_image_index()?
> as suggested by Takahiro. > * Allow capsule updates to be called from efi_init_obj_list() with the > FWU feature enabled, as suggested by Takahiro. > * Enable EFI_CAPSULE_ON_DISK_EARLY as an imply with the FWU feature > enabled. > * Define the FWU feature related functions as __maybe_unused to allow > for compilation with the FWU feature disabled. > > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > include/fwu.h | 30 +++++ > lib/Kconfig | 6 + > lib/Makefile | 1 + > lib/efi_loader/efi_capsule.c | 243 ++++++++++++++++++++++++++++++++++- > lib/fwu_updates/Kconfig | 33 +++++ > lib/fwu_updates/Makefile | 7 + > lib/fwu_updates/fwu.c | 23 ++++ > 9 files changed, 340 insertions(+), 6 deletions(-) > create mode 100644 lib/fwu_updates/Kconfig > create mode 100644 lib/fwu_updates/Makefile >
<snip>
> > /** > * efi_capsule_update_firmware - update firmware from capsule > @@ -410,7 +544,35 @@ static efi_status_t efi_capsule_update_firmware( > int item; > struct efi_firmware_management_protocol *fmp; > u16 *abort_reason; > + efi_guid_t image_type_id; > efi_status_t ret = EFI_SUCCESS; > + int status; > + u8 image_index; > + u32 update_index; > + bool fw_accept_os, image_index_check; > + > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (!fwu_empty_capsule(capsule_data) && > + !fwu_update_checks_pass()) { > + log_err("FWU checks failed. Cannot start update\n"); > + return EFI_INVALID_PARAMETER; > + } > + > + if (fwu_empty_capsule(capsule_data)) > + return fwu_empty_capsule_process(capsule_data); > + > + /* Obtain the update_index from the platform */ > + status = fwu_plat_get_update_index(&update_index); > + if (status < 0) { > + log_err("Failed to get the FWU update_index value\n"); > + return EFI_DEVICE_ERROR; > + } > + > + image_index_check = false; > + fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0; > + } else { > + image_index_check = true; > + } > > /* sanity check */ > if (capsule_data->header_size < sizeof(*capsule) || > @@ -455,7 +617,8 @@ static efi_status_t efi_capsule_update_firmware( > fmp = efi_fmp_find(&image->update_image_type_id, > image->update_image_index, > image->update_hardware_instance, > - handles, no_handles); > + handles, no_handles, > + image_index_check); > if (!fmp) { > log_err("FMP driver not found for firmware type %pUs, hardware instance %lld\n", > &image->update_image_type_id, > @@ -485,8 +648,30 @@ static efi_status_t efi_capsule_update_firmware( > goto out; > } > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + /* > + * Based on the value of update_image_type_id, > + * derive the image index value. This will be > + * passed as update_image_index to the > + * set_image function. > + */ > + image_type_id = image->update_image_type_id; > + status = fwu_get_image_index(&image_type_id, > + update_index, > + &image_index);
AS I said in my comment to v9, this function should be moved in FMP driver, that is, efi_firmware.c and contained in set_image().
Okay. I had replied to your review comment and for this specific comment, I had mentioned that I would prefer keeping this in the capsule driver. Since you did not object to that, I was under the assumption that you are fine with what I had said.
I looked at moving this to the FMP's set_image function. However, there is an issue in that the fwu_get_image_index() function needs to be passed the ImageTypeId GUID value for getting the image index. However, the set_image function has not been passed this GUID. Unless we use some global variable, it would not be possible to move this function to the set_image function.
I doubt it. Because FMP driver is looked for with image type id at efi_fmp_find(), it should know who it is. After you change in the past, current FMP drivers, either FIT or RAW, are bound only to a single GUID. Right?
With the recent change that I had made, we do need different GUIDs for different images in the capsule, but the FMP instance will be the same for all raw images, and similarly for all FIT images. But the set_image function does not know for which image the function has been called. Multiple images of a given type(raw/FIT) can use the same set_image function.
You try to use different image_index's to distinguish A and B banks, but this kind of usage is quite implementation-dependent since other firmware framework may use a different approach to support multiple banks.
True, but even with this implementation, that underlying framework can be abstracted. If, in the future, we have an option for multiple frameworks for performing the update, the fwu_get_image_index() can be extended to support those multiple framework implementations. The API
I can't image how. My point is that a caller of set_image() can and should pass an unique (and the same) index id whether the working firmware is on A or B bank.
We have discussed this earlier as well. What you say is true for the normal capsule update. However, for the FWU(A/B) updates, the image index is going to be calculated at run-time, based on the partition(bank) to which the image needs to be written to. Which is
It sound weird to me. If we assume what you said here, FMP driver is expected to handle a capsule image solely based on "index" but without knowing which type (id) the image belongs to.
I am referring to the set_image(SetImage) FMP function. That is indeed solely based on the image index -- there is no ImageTypeId being passed to the SetImage FMP function.
Before further discussion, let me ask in another way.
What contents do you expect GetImageInfo() returns, in particular, for "ImageIndex" for a specific firmware image?
UEFI specification says, in 23.1 Firmware Management Protocol, ===(a)=== The definition of GET_IMAGE_INFO(): DescriptorCount A pointer to the location in which firmware returns the number of descriptors or firmware images within this device.
...
The definition of EFI_FIRMWARE_IMAGE_DESCRIPTOR: ImageIndex A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ImageTypeId A unique GUID identifying the firmware image type. === ===
So whatever "ImageTypeId" is, ImageIndex will and can uniquely identify a specific firmware image in terms of a table of EFI_FIRMWARE_IMAGE_DESCRIPTOR's which is returned by GetImageInfo() of a FMP driver.
Furthermore,
in 23.3 Delivering Capsules Containing Updates to Firmware Management Protocol, ===(b)=== The definition of EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER: UpdateImageTypeId Used to identify device firmware targeted by this update. This guid is matched by system firmware against ImageTypeId field within a EFI_FIRMWARE_IMAGE_DESCRIPTOR returned by an instance of EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo() in the system. UpdateImageIndex Passed as ImageIndex in call to EFI_FIRMWARE_MANAGEMENT_PROTOCOL.SetImage() ^^^^^^^^^^^^^ === ===
So I believe that "UpdateImageIndex" in a header of capsule file is expected to be passed *directly* to SetImage() as one of arguments, assuming that a content of capsule image header is irrelevant to which bank the firmware is to be applied at any time.
-Takahiro Akashi
efi_status_t (EFIAPI *set_image)( struct efi_firmware_management_protocol *this, u8 image_index, const void *image, efi_uintn_t image_size, const void *vendor_code, efi_status_t (*progress)(efi_uintn_t completion), u16 **abort_reason);
I don' think it can be universal assumption for all kind of FMP's. Why must we have different semantics of set_image() for normal (non-A/B-update) case and A/B update case?
The semantics are not different for set_image() for the two cases. It's just that the image_index value being passed would be different for the A/B update case, since that value is being calculated at run-time.
-sughosh
-Takahiro Akashi
the sole purpose of having the fwu_get_image_index() API. I could have moved the function out of the efi_capsule.c to the FMP's set_image functions, but like I mentioned earlier, the set_image function does not know the ImageTypeId of the image for which it has been called -- since the image_index is a parameter being passed to the set_image function, we need to compute it earlier, before calling the function.
-sughosh
I think that all the visible part of A/B update in efi_capsule.c is a handling of accept/revert capsules.
-Takahiro Akashi
is just getting the image index for the image payload, and the image index will remain irrespective of the underlying framework for doing the updates.
-sughosh
Please remember that, from the viewpoint of API, image_index must be unique whether it is on A bank or B bank as it is used to identify a specific firmware image within a device, not a "physical" location.
Please re-think.
-Takahiro Akashi
> + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Unable to get the Image Index for the image type %pUs\n", > + &image_type_id); > + goto out; > + } > + log_debug("Image Index %u for Image Type Id %pUs\n", > + image_index, &image_type_id); > + } else { > + image_index = image->update_image_index; > + } > abort_reason = NULL; > - ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index, > + ret = EFI_CALL(fmp->set_image(fmp, image_index, > image_binary, > image_binary_size, > vendor_code, NULL, > @@ -497,6 +682,33 @@ static efi_status_t efi_capsule_update_firmware( > efi_free_pool(abort_reason); > goto out; > } > + > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (!fw_accept_os) { > + /* > + * The OS will not be accepting the firmware > + * images. Set the accept bit of all the > + * images contained in this capsule. > + */ > + status = fwu_accept_image(&image_type_id, > + update_index); > + } else { > + status = fwu_clear_accept_image(&image_type_id, > + update_index); > + } > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Unable to %s the accept bit for the image %pUs\n", > + fw_accept_os ? "clear" : "set", > + &image_type_id); > + goto out; > + } > + > + log_debug("%s the accepted bit for Image %pUs\n", > + fw_accept_os ? "Cleared" : "Set", > + &image_type_id); > + } > + > } > > out: > @@ -1104,6 +1316,9 @@ efi_status_t efi_launch_capsules(void) > u16 **files; > unsigned int nfiles, index, i; > efi_status_t ret; > + bool capsule_update = true; > + bool update_status = true; > + bool fw_accept_os = false; > > if (check_run_capsules() != EFI_SUCCESS) > return EFI_SUCCESS; > @@ -1131,12 +1346,19 @@ efi_status_t efi_launch_capsules(void) > ret = efi_capsule_read_file(files[i], &capsule); > if (ret == EFI_SUCCESS) { > ret = efi_capsule_update_firmware(capsule); > - if (ret != EFI_SUCCESS) > + if (ret != EFI_SUCCESS) { > log_err("Applying capsule %ls failed.\n", > files[i]); > - else > + update_status = false; > + } else { > log_info("Applying capsule %ls succeeded.\n", > files[i]); > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + fwu_post_update_checks(capsule, > + &fw_accept_os, > + &capsule_update); > + } > + } > > /* create CapsuleXXXX */ > set_capsule_result(index, capsule, ret); > @@ -1144,6 +1366,7 @@ efi_status_t efi_launch_capsules(void) > free(capsule); > } else { > log_err("Reading capsule %ls failed\n", files[i]); > + update_status = false; > } > /* delete a capsule either in case of success or failure */ > ret = efi_capsule_delete_file(files[i]); > @@ -1151,7 +1374,15 @@ efi_status_t efi_launch_capsules(void) > log_err("Deleting capsule %ls failed\n", > files[i]); > } > + > efi_capsule_scan_done(); > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (update_status == true && capsule_update == true) { > + ret = fwu_post_update_process(fw_accept_os); > + } else if (capsule_update == true && update_status == false) { > + log_err("All capsules were not updated. Not updating FWU metadata\n"); > + } > + } > > for (i = 0; i < nfiles; i++) > free(files[i]); > diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig > new file mode 100644 > index 0000000000..78759e6618 > --- /dev/null > +++ b/lib/fwu_updates/Kconfig > @@ -0,0 +1,33 @@ > +config FWU_MULTI_BANK_UPDATE > + bool "Enable FWU Multi Bank Update Feature" > + depends on EFI_CAPSULE_ON_DISK > + select PARTITION_TYPE_GUID > + select EFI_SETUP_EARLY > + imply EFI_CAPSULE_ON_DISK_EARLY > + select EVENT > + help > + Feature for updating firmware images on platforms having > + multiple banks(copies) of the firmware images. One of the > + bank is selected for updating all the firmware components > + > +config FWU_NUM_BANKS > + int "Number of Banks defined by the platform" > + depends on FWU_MULTI_BANK_UPDATE > + help > + Define the number of banks of firmware images on a platform > + > +config FWU_NUM_IMAGES_PER_BANK > + int "Number of firmware images per bank" > + depends on FWU_MULTI_BANK_UPDATE > + help > + Define the number of firmware images per bank. This value > + should be the same for all the banks. > + > +config FWU_TRIAL_STATE_CNT > + int "Number of times system boots in Trial State" > + depends on FWU_MULTI_BANK_UPDATE > + default 3 > + help > + With FWU Multi Bank Update feature enabled, number of times > + the platform is allowed to boot in Trial State after an > + update. > diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile > new file mode 100644 > index 0000000000..1993088e5b > --- /dev/null > +++ b/lib/fwu_updates/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# Copyright (c) 2022, Linaro Limited > +# > + > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > index 32518d6f86..7209000b56 100644 > --- a/lib/fwu_updates/fwu.c > +++ b/lib/fwu_updates/fwu.c > @@ -490,7 +490,30 @@ u8 fwu_update_checks_pass(void) > return !trial_state && boottime_check; > } > > +/** > + * fwu_trial_state_ctr_start() - Start the Trial State counter > + * > + * Start the counter to identify the platform booting in the > + * Trial State. The counter is implemented as an EFI variable. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_trial_state_ctr_start(void) > +{ > + int ret; > + u16 trial_state_ctr; > + > + trial_state_ctr = 0; > + ret = trial_counter_update(&trial_state_ctr); > + if (ret) > + log_err("Unable to initialise TrialStateCtr\n"); > + > + return ret; > +} > + > static int fwu_boottime_checks(void *ctx, struct event *event) > + > { > int ret; > struct udevice *dev; > -- > 2.34.1 >