[PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Sep 16 12:54:35 CEST 2022
hi Takahiro,
On Fri, 16 Sept 2022 at 12:20, Takahiro Akashi
<takahiro.akashi at 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 at 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 at 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
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
> > > >
More information about the U-Boot
mailing list