[v9,10/15] FWU: Add support for the FWU Multi Bank Update feature

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 V8:
* Add comments for API functions
* Move addition of the FWU Kconfig symbols and Makefile to this patch

 drivers/Kconfig              |   2 +
 drivers/Makefile             |   1 +
 include/fwu.h                |  30 +++++
 lib/Kconfig                  |   6 +
 lib/Makefile                 |   1 +
 lib/efi_loader/efi_capsule.c | 213 ++++++++++++++++++++++++++++++++++-
 lib/efi_loader/efi_setup.c   |   3 +-
 lib/fwu_updates/Kconfig      |  32 ++++++
 lib/fwu_updates/Makefile     |   7 ++
 lib/fwu_updates/fwu.c        |  23 ++++
 10 files changed, 312 insertions(+), 6 deletions(-)
 create mode 100644 lib/fwu_updates/Kconfig
 create mode 100644 lib/fwu_updates/Makefile

Hi Sughosh,


Overall, I think the code is covering all the cases for A/B updates.
Irrelevant to this patchset, but in the future I think we shopuld look into
having efi_launch_capsules() call efi_update_capsule() would be cleaner.

[...]

I think you can get rid all of these 
> +
> +__maybe_unused static u32 update_index;

This can be calculated at runtime

> +__maybe_unused static bool capsule_update;

Dittio, you just need a call to figure out if it's an empty capsule or not

> +__maybe_unused static bool fw_accept_os;

Same for this one

> +static bool image_index_check = true;

Can't this just be an ifdef in the if statement that uses it?

>  
>  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
>  /* for file system access */
> @@ -205,7 +217,8 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>  			log_debug("+++ desc[%d] index: %d, name: %ls\n",
>  				  j, desc->image_index, desc->image_id_name);
>  			if (!guidcmp(&desc->image_type_id, image_type) &&
> -			    (desc->image_index == image_index) &&
> +			    (!image_index_check ||
> +			     desc->image_index == image_index) &&
>  			    (!instance ||
>  			     !desc->hardware_instance ||
>  			      desc->hardware_instance == instance))
> @@ -388,6 +401,83 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>  }
>  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>  
> +static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
> +{
> +	return !guidcmp(&capsule->capsule_guid,
> +			&fwu_guid_os_request_fw_revert) ||
> +		!guidcmp(&capsule->capsule_guid,
> +			 &fwu_guid_os_request_fw_accept);
> +}
> +
> +static efi_status_t fwu_to_efi_error(int err)
> +{
> +	efi_status_t ret;
> +
> +	switch(err) {
> +	case 0:
> +		ret = EFI_SUCCESS;
> +		break;
> +	case -ENODEV:
> +	case -ERANGE:
> +	case -EIO:
> +		ret = EFI_DEVICE_ERROR;
> +		break;
> +	case -EINVAL:
> +		ret = EFI_INVALID_PARAMETER;
> +		break;
> +	default:
> +		ret = EFI_OUT_OF_RESOURCES;
> +	}
> +
> +	return ret;
> +}
> +
> +static efi_status_t fwu_empty_capsule_process(
> +	struct efi_capsule_header *capsule)
> +{
> +	int status;
> +	u32 active_idx;
> +	efi_status_t ret;
> +	efi_guid_t *image_guid;
> +
> +	if (!guidcmp(&capsule->capsule_guid,
> +		     &fwu_guid_os_request_fw_revert)) {
> +		/*
> +		 * One of the previously updated image has
> +		 * failed the OS acceptance test. OS has
> +		 * requested to revert back to the earlier
> +		 * boot index
> +		 */
> +		status = fwu_revert_boot_index();
> +		ret = fwu_to_efi_error(status);
> +		if (ret == EFI_SUCCESS)
> +			log_info("Reverted the FWU active_index. Recommend rebooting the system\n");
> +		else
> +			log_err("Failed to revert the FWU boot index\n");
> +	} else {
> +		/*
> +		 * Image accepted by the OS. Set the acceptance
> +		 * status for the image.
> +		 */
> +		image_guid = (void *)(char *)capsule +
> +			capsule->header_size;
> +
> +		status = fwu_get_active_index(&active_idx);
> +		ret = fwu_to_efi_error(status);
> +		if (ret != EFI_SUCCESS) {
> +			log_err("Unable to get the active_index from the FWU metadata\n");
> +			return ret;
> +		}
> +
> +		status = fwu_accept_image(image_guid, active_idx);
> +		ret = fwu_to_efi_error(status);
> +		if (ret != EFI_SUCCESS)
> +			log_err("Unable to set the Accept bit for the image %pUs\n",
> +				image_guid);
> +	}
> +
> +	return ret;
> +}
>  
>  /**
>   * efi_capsule_update_firmware - update firmware from capsule
> @@ -407,10 +497,42 @@ static efi_status_t efi_capsule_update_firmware(
>  	void *image_binary, *vendor_code;
>  	efi_handle_t *handles;
>  	efi_uintn_t no_handles;
> -	int item;
> +	int item, alt_no;
>  	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;
> +
> +	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)) {
> +			capsule_update = false;
> +			return fwu_empty_capsule_process(capsule_data);
> +		} else {
> +			capsule_update = true;
> +		}
> +
> +		/* 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;
> +		}
> +
> +		fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> +		/*
> +		 * For Multi Bank updates, the image index is determined at
> +		 * runtime based on the value of the update bank.
> +		 */
> +		image_index_check = false;
> +	}
>  
>  	/* sanity check */
>  	if (capsule_data->header_size < sizeof(*capsule) ||
> @@ -485,8 +607,31 @@ 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 alt number 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_alt_num(&image_type_id,
> +						       update_index,
> +						       &alt_no);
> +			ret = fwu_to_efi_error(status);
> +			if (ret != EFI_SUCCESS) {
> +				log_err("Unable to get the alt no for the image type %pUs\n",
> +					&image_type_id);
> +				goto out;
> +			}
> +			log_debug("alt_no %u for Image Type Id %pUs\n",
> +				  alt_no, &image_type_id);
> +			image_index = alt_no + 1;
> +		} 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 +642,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:
> @@ -1102,8 +1274,10 @@ efi_status_t efi_launch_capsules(void)
>  {
>  	struct efi_capsule_header *capsule = NULL;
>  	u16 **files;
> +	int status;
>  	unsigned int nfiles, index, i;
>  	efi_status_t ret;
> +	bool update_status = true;
>  
>  	if (check_run_capsules() != EFI_SUCCESS)
>  		return EFI_SUCCESS;
> @@ -1131,12 +1305,14 @@ 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]);
> +			}
>  
>  			/* create CapsuleXXXX */
>  			set_capsule_result(index, capsule, ret);
> @@ -1144,6 +1320,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 +1328,33 @@ 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) {
> +			/*
> +			 * All the capsules have been updated successfully,
> +			 * update the FWU metadata.
> +			 */
> +			log_debug("Update Complete. Now updating active_index to %u\n",
> +				  update_index);
> +			status = fwu_update_active_index(update_index);
> +			ret = fwu_to_efi_error(status);
> +			if (ret != EFI_SUCCESS) {
> +				log_err("Failed to update FWU metadata index values\n");
> +			} else {
> +				log_debug("Successfully updated the active_index\n");
> +				ret = EFI_SUCCESS;
> +				if (fw_accept_os) {
> +					status = fwu_trial_state_ctr_start();
> +					if (status < 0)
> +						ret = EFI_DEVICE_ERROR;
> +				}
> +			}
> +		} 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/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 23dc7a0710..60012e873e 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -351,7 +351,8 @@ efi_status_t efi_init_obj_list(void)
>  		goto out;
>  
>  	/* Execute capsules after reboot */
> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
> +	if (!IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE) &&
> +	    IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
>  	    !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY))
>  		ret = efi_launch_capsules();
>  out:
> diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig
> new file mode 100644
> index 0000000000..e33085f1d3
> --- /dev/null
> +++ b/lib/fwu_updates/Kconfig
> @@ -0,0 +1,32 @@
> +config FWU_MULTI_BANK_UPDATE
> +	bool "Enable FWU Multi Bank Update Feature"
> +	depends on EFI_HAVE_CAPSULE_SUPPORT
> +	select PARTITION_TYPE_GUID
> +	select EFI_SETUP_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 27f48855af..de2412785d 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -513,7 +513,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
> 

Thanks
/Ilias

Hi Sughosh,

On Fri, Aug 26, 2022 at 03:27:11PM +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 V8:
> * Add comments for API functions
> * Move addition of the FWU Kconfig symbols and Makefile to this patch
> 
>  drivers/Kconfig              |   2 +
>  drivers/Makefile             |   1 +
>  include/fwu.h                |  30 +++++
>  lib/Kconfig                  |   6 +
>  lib/Makefile                 |   1 +
>  lib/efi_loader/efi_capsule.c | 213 ++++++++++++++++++++++++++++++++++-
>  lib/efi_loader/efi_setup.c   |   3 +-
>  lib/fwu_updates/Kconfig      |  32 ++++++
>  lib/fwu_updates/Makefile     |   7 ++
>  lib/fwu_updates/fwu.c        |  23 ++++
>  10 files changed, 312 insertions(+), 6 deletions(-)
>  create mode 100644 lib/fwu_updates/Kconfig
>  create mode 100644 lib/fwu_updates/Makefile
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 8b6fead351..75ac149d31 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -44,6 +44,8 @@ source "drivers/fuzz/Kconfig"
>  
>  source "drivers/fpga/Kconfig"
>  
> +source "drivers/fwu-mdata/Kconfig"
> +
>  source "drivers/gpio/Kconfig"
>  
>  source "drivers/hwspinlock/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index eba9940231..af7ed7bdf3 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -84,6 +84,7 @@ obj-y += cache/
>  obj-$(CONFIG_CPU) += cpu/
>  obj-y += crypto/
>  obj-$(CONFIG_FASTBOOT) += fastboot/
> +obj-$(CONFIG_FWU_MDATA) += fwu-mdata/
>  obj-y += misc/
>  obj-$(CONFIG_MMC) += mmc/
>  obj-$(CONFIG_NVME) += nvme/
> diff --git a/include/fwu.h b/include/fwu.h
> index 31c9923ff5..e05e1d1a00 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -60,6 +60,7 @@ struct fwu_mdata_ops {
>  };
>  
>  #define FWU_MDATA_VERSION	0x1
> +#define FWU_IMAGE_ACCEPTED	0x1
>  
>  /*
>  * GUID value defined in the FWU specification for identification
> @@ -69,6 +70,24 @@ struct fwu_mdata_ops {
>  	EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
>  		 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
>  
> +/*
> +* GUID value defined in the Dependable Boot specification for
> +* identification of the revert capsule, used for reverting
> +* any image in the updated bank.
> +*/
> +#define FWU_OS_REQUEST_FW_REVERT_GUID \
> +	EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \
> +		 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0)
> +
> +/*
> +* GUID value defined in the Dependable Boot specification for
> +* identification of the accept capsule, used for accepting
> +* an image in the updated bank.
> +*/
> +#define FWU_OS_REQUEST_FW_ACCEPT_GUID \
> +	EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> +		 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> +
>  /**
>   * fwu_get_mdata() - Get a FWU metadata copy
>   * @dev: FWU metadata device
> @@ -265,4 +284,15 @@ void fwu_plat_get_bootidx(void *boot_idx);
>   */
>  u8 fwu_update_checks_pass(void);
>  
> +/**
> + * 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);
> +
>  #endif /* _FWU_H_ */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 6121c80dc8..6abe1d0a86 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -978,3 +978,9 @@ config LMB_RESERVED_REGIONS
>  	  memory blocks.
>  
>  endmenu
> +
> +menu "FWU Multi Bank Updates"
> +
> +source lib/fwu_updates/Kconfig
> +
> +endmenu
> diff --git a/lib/Makefile b/lib/Makefile
> index e3deb15287..f2cfd1e428 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_EFI) += efi/
>  obj-$(CONFIG_EFI_LOADER) += efi_driver/
>  obj-$(CONFIG_EFI_LOADER) += efi_loader/
>  obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += efi_selftest/
> +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_updates/
>  obj-$(CONFIG_LZMA) += lzma/
>  obj-$(CONFIG_BZIP2) += bzip2/
>  obj-$(CONFIG_FIT) += libfdt/
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index a6b98f066a..44c56443dc 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -14,6 +14,7 @@
>  #include <env.h>
>  #include <fdtdec.h>
>  #include <fs.h>
> +#include <fwu.h>
>  #include <hang.h>
>  #include <malloc.h>
>  #include <mapmem.h>
> @@ -32,6 +33,17 @@ static const efi_guid_t efi_guid_firmware_management_capsule_id =
>  		EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>  const efi_guid_t efi_guid_firmware_management_protocol =
>  		EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID;
> +const efi_guid_t fwu_guid_os_request_fw_revert =
> +		FWU_OS_REQUEST_FW_REVERT_GUID;
> +const efi_guid_t fwu_guid_os_request_fw_accept =
> +		FWU_OS_REQUEST_FW_ACCEPT_GUID;
> +
> +#define FW_ACCEPT_OS	(u32)0x8000
> +
> +__maybe_unused static u32 update_index;
> +__maybe_unused static bool capsule_update;
> +__maybe_unused static bool fw_accept_os;
> +static bool image_index_check = true;
>  
>  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
>  /* for file system access */
> @@ -205,7 +217,8 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>  			log_debug("+++ desc[%d] index: %d, name: %ls\n",
>  				  j, desc->image_index, desc->image_id_name);
>  			if (!guidcmp(&desc->image_type_id, image_type) &&
> -			    (desc->image_index == image_index) &&
> +			    (!image_index_check ||
> +			     desc->image_index == image_index) &&
>  			    (!instance ||
>  			     !desc->hardware_instance ||
>  			      desc->hardware_instance == instance))
> @@ -388,6 +401,83 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>  }
>  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>  
> +static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
> +{
> +	return !guidcmp(&capsule->capsule_guid,
> +			&fwu_guid_os_request_fw_revert) ||
> +		!guidcmp(&capsule->capsule_guid,
> +			 &fwu_guid_os_request_fw_accept);
> +}
> +
> +static efi_status_t fwu_to_efi_error(int err)
> +{
> +	efi_status_t ret;
> +
> +	switch(err) {
> +	case 0:
> +		ret = EFI_SUCCESS;
> +		break;
> +	case -ENODEV:
> +	case -ERANGE:
> +	case -EIO:
> +		ret = EFI_DEVICE_ERROR;
> +		break;
> +	case -EINVAL:
> +		ret = EFI_INVALID_PARAMETER;
> +		break;
> +	default:
> +		ret = EFI_OUT_OF_RESOURCES;
> +	}
> +
> +	return ret;
> +}
> +
> +static efi_status_t fwu_empty_capsule_process(
> +	struct efi_capsule_header *capsule)
> +{
> +	int status;
> +	u32 active_idx;
> +	efi_status_t ret;
> +	efi_guid_t *image_guid;
> +
> +	if (!guidcmp(&capsule->capsule_guid,
> +		     &fwu_guid_os_request_fw_revert)) {
> +		/*
> +		 * One of the previously updated image has
> +		 * failed the OS acceptance test. OS has
> +		 * requested to revert back to the earlier
> +		 * boot index
> +		 */
> +		status = fwu_revert_boot_index();
> +		ret = fwu_to_efi_error(status);
> +		if (ret == EFI_SUCCESS)
> +			log_info("Reverted the FWU active_index. Recommend rebooting the system\n");
> +		else
> +			log_err("Failed to revert the FWU boot index\n");
> +	} else {
> +		/*
> +		 * Image accepted by the OS. Set the acceptance
> +		 * status for the image.
> +		 */
> +		image_guid = (void *)(char *)capsule +
> +			capsule->header_size;
> +
> +		status = fwu_get_active_index(&active_idx);
> +		ret = fwu_to_efi_error(status);
> +		if (ret != EFI_SUCCESS) {
> +			log_err("Unable to get the active_index from the FWU metadata\n");
> +			return ret;
> +		}
> +
> +		status = fwu_accept_image(image_guid, active_idx);
> +		ret = fwu_to_efi_error(status);
> +		if (ret != EFI_SUCCESS)
> +			log_err("Unable to set the Accept bit for the image %pUs\n",
> +				image_guid);
> +	}
> +
> +	return ret;
> +}
>  
>  /**
>   * efi_capsule_update_firmware - update firmware from capsule
> @@ -407,10 +497,42 @@ static efi_status_t efi_capsule_update_firmware(
>  	void *image_binary, *vendor_code;
>  	efi_handle_t *handles;
>  	efi_uintn_t no_handles;
> -	int item;
> +	int item, alt_no;
>  	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;
> +
> +	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)) {
> +			capsule_update = false;
> +			return fwu_empty_capsule_process(capsule_data);
> +		} else {
> +			capsule_update = true;
> +		}
> +
> +		/* 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;
> +		}
> +
> +		fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> +		/*
> +		 * For Multi Bank updates, the image index is determined at
> +		 * runtime based on the value of the update bank.
> +		 */
> +		image_index_check = false;
> +	}
>  
>  	/* sanity check */
>  	if (capsule_data->header_size < sizeof(*capsule) ||
> @@ -485,8 +607,31 @@ 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 alt number 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_alt_num(&image_type_id,
> +						       update_index,
> +						       &alt_no);

I have a concern here.
The function's name, fwd_get_image_alt_num(), suggests that it is somehow
related to dfu (alto number) feature. Actually, however, I hope not;
the function's purpose is simply to convert "update_index" to a proper
value under some condition (i.e. A/B update).

From design's point of view, the entire code in efi_capsule.c should
stay generic enough to support various type of underling firmware frameworks.
Please note that efi_firmware.c is just one of such implementations of
firmware update protocols.

So please think of changing the function's name if I am correct.

Furthermore, it would be better to call fwu_get_image_alt_num() in
FMP drivers (in this case, efi_firmware.c). (Not sure though)

> +			ret = fwu_to_efi_error(status);
> +			if (ret != EFI_SUCCESS) {
> +				log_err("Unable to get the alt no for the image type %pUs\n",
> +					&image_type_id);
> +				goto out;
> +			}
> +			log_debug("alt_no %u for Image Type Id %pUs\n",
> +				  alt_no, &image_type_id);
> +			image_index = alt_no + 1;
> +		} 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 +642,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:
> @@ -1102,8 +1274,10 @@ efi_status_t efi_launch_capsules(void)
>  {
>  	struct efi_capsule_header *capsule = NULL;
>  	u16 **files;
> +	int status;
>  	unsigned int nfiles, index, i;
>  	efi_status_t ret;
> +	bool update_status = true;
>  
>  	if (check_run_capsules() != EFI_SUCCESS)
>  		return EFI_SUCCESS;
> @@ -1131,12 +1305,14 @@ 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]);
> +			}
>  
>  			/* create CapsuleXXXX */
>  			set_capsule_result(index, capsule, ret);
> @@ -1144,6 +1320,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 +1328,33 @@ 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) {
> +			/*
> +			 * All the capsules have been updated successfully,
> +			 * update the FWU metadata.
> +			 */
> +			log_debug("Update Complete. Now updating active_index to %u\n",
> +				  update_index);
> +			status = fwu_update_active_index(update_index);
> +			ret = fwu_to_efi_error(status);
> +			if (ret != EFI_SUCCESS) {
> +				log_err("Failed to update FWU metadata index values\n");
> +			} else {
> +				log_debug("Successfully updated the active_index\n");
> +				ret = EFI_SUCCESS;
> +				if (fw_accept_os) {
> +					status = fwu_trial_state_ctr_start();
> +					if (status < 0)
> +						ret = EFI_DEVICE_ERROR;
> +				}
> +			}
> +		} 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/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 23dc7a0710..60012e873e 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -351,7 +351,8 @@ efi_status_t efi_init_obj_list(void)
>  		goto out;
>  
>  	/* Execute capsules after reboot */
> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
> +	if (!IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE) &&

Why do you need this?

> +	    IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
>  	    !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY))
>  		ret = efi_launch_capsules();
>  out:
> diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig
> new file mode 100644
> index 0000000000..e33085f1d3
> --- /dev/null
> +++ b/lib/fwu_updates/Kconfig
> @@ -0,0 +1,32 @@
> +config FWU_MULTI_BANK_UPDATE
> +	bool "Enable FWU Multi Bank Update Feature"
> +	depends on EFI_HAVE_CAPSULE_SUPPORT

As a matter of fact, your commits rely on CAPSULE_ON_DISK.

> +	select PARTITION_TYPE_GUID
> +	select EFI_SETUP_EARLY

This kconfig is always on since the commit a9bf024b2933
(efi_loader: disk: a helper function to create efi_disk objects from udevice).

Do you want to enable CAPSULE_ON_DISK_EARLY?

-Takahiro Akashi

> +	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 27f48855af..de2412785d 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -513,7 +513,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
>

hi Takahiro,

On Thu, 8 Sept 2022 at 07:45, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> Hi Sughosh,
>
> On Fri, Aug 26, 2022 at 03:27:11PM +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 V8:
> > * Add comments for API functions
> > * Move addition of the FWU Kconfig symbols and Makefile to this patch
> >
> >  drivers/Kconfig              |   2 +
> >  drivers/Makefile             |   1 +
> >  include/fwu.h                |  30 +++++
> >  lib/Kconfig                  |   6 +
> >  lib/Makefile                 |   1 +
> >  lib/efi_loader/efi_capsule.c | 213 ++++++++++++++++++++++++++++++++++-
> >  lib/efi_loader/efi_setup.c   |   3 +-
> >  lib/fwu_updates/Kconfig      |  32 ++++++
> >  lib/fwu_updates/Makefile     |   7 ++
> >  lib/fwu_updates/fwu.c        |  23 ++++
> >  10 files changed, 312 insertions(+), 6 deletions(-)
> >  create mode 100644 lib/fwu_updates/Kconfig
> >  create mode 100644 lib/fwu_updates/Makefile
> >
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index 8b6fead351..75ac149d31 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -44,6 +44,8 @@ source "drivers/fuzz/Kconfig"
> >
> >  source "drivers/fpga/Kconfig"
> >
> > +source "drivers/fwu-mdata/Kconfig"
> > +
> >  source "drivers/gpio/Kconfig"
> >
> >  source "drivers/hwspinlock/Kconfig"
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index eba9940231..af7ed7bdf3 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -84,6 +84,7 @@ obj-y += cache/
> >  obj-$(CONFIG_CPU) += cpu/
> >  obj-y += crypto/
> >  obj-$(CONFIG_FASTBOOT) += fastboot/
> > +obj-$(CONFIG_FWU_MDATA) += fwu-mdata/
> >  obj-y += misc/
> >  obj-$(CONFIG_MMC) += mmc/
> >  obj-$(CONFIG_NVME) += nvme/
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 31c9923ff5..e05e1d1a00 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -60,6 +60,7 @@ struct fwu_mdata_ops {
> >  };
> >
> >  #define FWU_MDATA_VERSION    0x1
> > +#define FWU_IMAGE_ACCEPTED   0x1
> >
> >  /*
> >  * GUID value defined in the FWU specification for identification
> > @@ -69,6 +70,24 @@ struct fwu_mdata_ops {
> >       EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> >                0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> >
> > +/*
> > +* GUID value defined in the Dependable Boot specification for
> > +* identification of the revert capsule, used for reverting
> > +* any image in the updated bank.
> > +*/
> > +#define FWU_OS_REQUEST_FW_REVERT_GUID \
> > +     EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \
> > +              0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0)
> > +
> > +/*
> > +* GUID value defined in the Dependable Boot specification for
> > +* identification of the accept capsule, used for accepting
> > +* an image in the updated bank.
> > +*/
> > +#define FWU_OS_REQUEST_FW_ACCEPT_GUID \
> > +     EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> > +              0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> > +
> >  /**
> >   * fwu_get_mdata() - Get a FWU metadata copy
> >   * @dev: FWU metadata device
> > @@ -265,4 +284,15 @@ void fwu_plat_get_bootidx(void *boot_idx);
> >   */
> >  u8 fwu_update_checks_pass(void);
> >
> > +/**
> > + * 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);
> > +
> >  #endif /* _FWU_H_ */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 6121c80dc8..6abe1d0a86 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -978,3 +978,9 @@ config LMB_RESERVED_REGIONS
> >         memory blocks.
> >
> >  endmenu
> > +
> > +menu "FWU Multi Bank Updates"
> > +
> > +source lib/fwu_updates/Kconfig
> > +
> > +endmenu
> > diff --git a/lib/Makefile b/lib/Makefile
> > index e3deb15287..f2cfd1e428 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_EFI) += efi/
> >  obj-$(CONFIG_EFI_LOADER) += efi_driver/
> >  obj-$(CONFIG_EFI_LOADER) += efi_loader/
> >  obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += efi_selftest/
> > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_updates/
> >  obj-$(CONFIG_LZMA) += lzma/
> >  obj-$(CONFIG_BZIP2) += bzip2/
> >  obj-$(CONFIG_FIT) += libfdt/
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index a6b98f066a..44c56443dc 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -14,6 +14,7 @@
> >  #include <env.h>
> >  #include <fdtdec.h>
> >  #include <fs.h>
> > +#include <fwu.h>
> >  #include <hang.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> > @@ -32,6 +33,17 @@ static const efi_guid_t efi_guid_firmware_management_capsule_id =
> >               EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> >  const efi_guid_t efi_guid_firmware_management_protocol =
> >               EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID;
> > +const efi_guid_t fwu_guid_os_request_fw_revert =
> > +             FWU_OS_REQUEST_FW_REVERT_GUID;
> > +const efi_guid_t fwu_guid_os_request_fw_accept =
> > +             FWU_OS_REQUEST_FW_ACCEPT_GUID;
> > +
> > +#define FW_ACCEPT_OS (u32)0x8000
> > +
> > +__maybe_unused static u32 update_index;
> > +__maybe_unused static bool capsule_update;
> > +__maybe_unused static bool fw_accept_os;
> > +static bool image_index_check = true;
> >
> >  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
> >  /* for file system access */
> > @@ -205,7 +217,8 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
> >                       log_debug("+++ desc[%d] index: %d, name: %ls\n",
> >                                 j, desc->image_index, desc->image_id_name);
> >                       if (!guidcmp(&desc->image_type_id, image_type) &&
> > -                         (desc->image_index == image_index) &&
> > +                         (!image_index_check ||
> > +                          desc->image_index == image_index) &&
> >                           (!instance ||
> >                            !desc->hardware_instance ||
> >                             desc->hardware_instance == instance))
> > @@ -388,6 +401,83 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
> >
> > +static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
> > +{
> > +     return !guidcmp(&capsule->capsule_guid,
> > +                     &fwu_guid_os_request_fw_revert) ||
> > +             !guidcmp(&capsule->capsule_guid,
> > +                      &fwu_guid_os_request_fw_accept);
> > +}
> > +
> > +static efi_status_t fwu_to_efi_error(int err)
> > +{
> > +     efi_status_t ret;
> > +
> > +     switch(err) {
> > +     case 0:
> > +             ret = EFI_SUCCESS;
> > +             break;
> > +     case -ENODEV:
> > +     case -ERANGE:
> > +     case -EIO:
> > +             ret = EFI_DEVICE_ERROR;
> > +             break;
> > +     case -EINVAL:
> > +             ret = EFI_INVALID_PARAMETER;
> > +             break;
> > +     default:
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static efi_status_t fwu_empty_capsule_process(
> > +     struct efi_capsule_header *capsule)
> > +{
> > +     int status;
> > +     u32 active_idx;
> > +     efi_status_t ret;
> > +     efi_guid_t *image_guid;
> > +
> > +     if (!guidcmp(&capsule->capsule_guid,
> > +                  &fwu_guid_os_request_fw_revert)) {
> > +             /*
> > +              * One of the previously updated image has
> > +              * failed the OS acceptance test. OS has
> > +              * requested to revert back to the earlier
> > +              * boot index
> > +              */
> > +             status = fwu_revert_boot_index();
> > +             ret = fwu_to_efi_error(status);
> > +             if (ret == EFI_SUCCESS)
> > +                     log_info("Reverted the FWU active_index. Recommend rebooting the system\n");
> > +             else
> > +                     log_err("Failed to revert the FWU boot index\n");
> > +     } else {
> > +             /*
> > +              * Image accepted by the OS. Set the acceptance
> > +              * status for the image.
> > +              */
> > +             image_guid = (void *)(char *)capsule +
> > +                     capsule->header_size;
> > +
> > +             status = fwu_get_active_index(&active_idx);
> > +             ret = fwu_to_efi_error(status);
> > +             if (ret != EFI_SUCCESS) {
> > +                     log_err("Unable to get the active_index from the FWU metadata\n");
> > +                     return ret;
> > +             }
> > +
> > +             status = fwu_accept_image(image_guid, active_idx);
> > +             ret = fwu_to_efi_error(status);
> > +             if (ret != EFI_SUCCESS)
> > +                     log_err("Unable to set the Accept bit for the image %pUs\n",
> > +                             image_guid);
> > +     }
> > +
> > +     return ret;
> > +}
> >
> >  /**
> >   * efi_capsule_update_firmware - update firmware from capsule
> > @@ -407,10 +497,42 @@ static efi_status_t efi_capsule_update_firmware(
> >       void *image_binary, *vendor_code;
> >       efi_handle_t *handles;
> >       efi_uintn_t no_handles;
> > -     int item;
> > +     int item, alt_no;
> >       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;
> > +
> > +     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)) {
> > +                     capsule_update = false;
> > +                     return fwu_empty_capsule_process(capsule_data);
> > +             } else {
> > +                     capsule_update = true;
> > +             }
> > +
> > +             /* 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;
> > +             }
> > +
> > +             fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> > +             /*
> > +              * For Multi Bank updates, the image index is determined at
> > +              * runtime based on the value of the update bank.
> > +              */
> > +             image_index_check = false;
> > +     }
> >
> >       /* sanity check */
> >       if (capsule_data->header_size < sizeof(*capsule) ||
> > @@ -485,8 +607,31 @@ 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 alt number 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_alt_num(&image_type_id,
> > +                                                    update_index,
> > +                                                    &alt_no);
>
> I have a concern here.
> The function's name, fwd_get_image_alt_num(), suggests that it is somehow
> related to dfu (alto number) feature. Actually, however, I hope not;
> the function's purpose is simply to convert "update_index" to a proper
> value under some condition (i.e. A/B update).
>
> From design's point of view, the entire code in efi_capsule.c should
> stay generic enough to support various type of underling firmware frameworks.
> Please note that efi_firmware.c is just one of such implementations of
> firmware update protocols.
>
> So please think of changing the function's name if I am correct.
>
> Furthermore, it would be better to call fwu_get_image_alt_num() in
> FMP drivers (in this case, efi_firmware.c). (Not sure though)

Okay. I see your point. I will change the name of the API to make it
generic so as to not tie it with the DFU alt number. Regarding your
thoughts on putting this in the FMP drivers, I feel that rather than
having the function get called from every FMP driver, it would be
better to keep it where it currently is.


>
> > +                     ret = fwu_to_efi_error(status);
> > +                     if (ret != EFI_SUCCESS) {
> > +                             log_err("Unable to get the alt no for the image type %pUs\n",
> > +                                     &image_type_id);
> > +                             goto out;
> > +                     }
> > +                     log_debug("alt_no %u for Image Type Id %pUs\n",
> > +                               alt_no, &image_type_id);
> > +                     image_index = alt_no + 1;
> > +             } 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 +642,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:
> > @@ -1102,8 +1274,10 @@ efi_status_t efi_launch_capsules(void)
> >  {
> >       struct efi_capsule_header *capsule = NULL;
> >       u16 **files;
> > +     int status;
> >       unsigned int nfiles, index, i;
> >       efi_status_t ret;
> > +     bool update_status = true;
> >
> >       if (check_run_capsules() != EFI_SUCCESS)
> >               return EFI_SUCCESS;
> > @@ -1131,12 +1305,14 @@ 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]);
> > +                     }
> >
> >                       /* create CapsuleXXXX */
> >                       set_capsule_result(index, capsule, ret);
> > @@ -1144,6 +1320,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 +1328,33 @@ 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) {
> > +                     /*
> > +                      * All the capsules have been updated successfully,
> > +                      * update the FWU metadata.
> > +                      */
> > +                     log_debug("Update Complete. Now updating active_index to %u\n",
> > +                               update_index);
> > +                     status = fwu_update_active_index(update_index);
> > +                     ret = fwu_to_efi_error(status);
> > +                     if (ret != EFI_SUCCESS) {
> > +                             log_err("Failed to update FWU metadata index values\n");
> > +                     } else {
> > +                             log_debug("Successfully updated the active_index\n");
> > +                             ret = EFI_SUCCESS;
> > +                             if (fw_accept_os) {
> > +                                     status = fwu_trial_state_ctr_start();
> > +                                     if (status < 0)
> > +                                             ret = EFI_DEVICE_ERROR;
> > +                             }
> > +                     }
> > +             } 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/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 23dc7a0710..60012e873e 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -351,7 +351,8 @@ efi_status_t efi_init_obj_list(void)
> >               goto out;
> >
> >       /* Execute capsules after reboot */
> > -     if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
> > +     if (!IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE) &&
>
> Why do you need this?

Earlier, before a57ad20d07e, efi_init_obj_list() was getting called
with CONFIG_EFI_SETUP_EARLY being set. With this,
efi_launch_capsules() would be called before a call to
fwu_boottime_checks(), which was the reason for the check above. With
a57ad20d07e, this is no longer needed. I will remove this in the next
version.

>
> > +         IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
> >           !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY))
> >               ret = efi_launch_capsules();
> >  out:
> > diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig
> > new file mode 100644
> > index 0000000000..e33085f1d3
> > --- /dev/null
> > +++ b/lib/fwu_updates/Kconfig
> > @@ -0,0 +1,32 @@
> > +config FWU_MULTI_BANK_UPDATE
> > +     bool "Enable FWU Multi Bank Update Feature"
> > +     depends on EFI_HAVE_CAPSULE_SUPPORT
>
> As a matter of fact, your commits rely on CAPSULE_ON_DISK.

Yes, will change.

>
> > +     select PARTITION_TYPE_GUID
> > +     select EFI_SETUP_EARLY
>
> This kconfig is always on since the commit a9bf024b2933
> (efi_loader: disk: a helper function to create efi_disk objects from udevice).
>
> Do you want to enable CAPSULE_ON_DISK_EARLY?

Yes, I can enable that, maybe as an imply.

-sughosh

>
> -Takahiro Akashi
>
> > +     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 27f48855af..de2412785d 100644
> > --- a/lib/fwu_updates/fwu.c
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -513,7 +513,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
> >