[v10,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 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()
  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

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
> 
> 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 d5f77ce83c..1d15ac98da 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
> @@ -269,4 +288,15 @@ void fwu_plat_get_bootidx(uint *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..7f431ab477 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,12 @@ 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
>  
>  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
>  /* for file system access */
> @@ -133,6 +140,7 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
>   * @instance:		Instance number
>   * @handles:		Handles of FMP drivers
>   * @no_handles:		Number of handles
> + * @image_index_check:	Image Index check flag
>   *
>   * Search for Firmware Management Protocol drivers, matching the image
>   * type, @image_type and the machine instance, @instance, from the list,
> @@ -144,7 +152,8 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
>   */
>  static struct efi_firmware_management_protocol *
>  efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
> -	     efi_handle_t *handles, efi_uintn_t no_handles)
> +	     efi_handle_t *handles, efi_uintn_t no_handles,
> +	     bool image_index_check)
>  {
>  	efi_handle_t *handle;
>  	struct efi_firmware_management_protocol *fmp;
> @@ -205,7 +214,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 +398,130 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>  }
>  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>  
> +static __maybe_unused 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 __maybe_unused 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 __maybe_unused 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;
> +}
> +
> +static __maybe_unused void fwu_post_update_checks(
> +	struct efi_capsule_header *capsule,
> +	bool *fw_accept_os, bool *capsule_update)
> +{
> +	if (fwu_empty_capsule(capsule))
> +		*capsule_update = false;
> +	else
> +		if (!*fw_accept_os)
> +			*fw_accept_os =
> +				capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> +}
> +
> +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> +{
> +	int status;
> +	u32 update_index;
> +	efi_status_t ret;
> +
> +	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;
> +	}
> +
> +	/*
> +	 * 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;
> +		}
> +	}
> +
> +	return ret;
> +}
>  
>  /**
>   * 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().

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.

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
>

() 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
> >
> > 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 d5f77ce83c..1d15ac98da 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
> > @@ -269,4 +288,15 @@ void fwu_plat_get_bootidx(uint *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..7f431ab477 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,12 @@ 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
> >
> >  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
> >  /* for file system access */
> > @@ -133,6 +140,7 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
> >   * @instance:                Instance number
> >   * @handles:         Handles of FMP drivers
> >   * @no_handles:              Number of handles
> > + * @image_index_check:       Image Index check flag
> >   *
> >   * Search for Firmware Management Protocol drivers, matching the image
> >   * type, @image_type and the machine instance, @instance, from the list,
> > @@ -144,7 +152,8 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
> >   */
> >  static struct efi_firmware_management_protocol *
> >  efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
> > -          efi_handle_t *handles, efi_uintn_t no_handles)
> > +          efi_handle_t *handles, efi_uintn_t no_handles,
> > +          bool image_index_check)
> >  {
> >       efi_handle_t *handle;
> >       struct efi_firmware_management_protocol *fmp;
> > @@ -205,7 +214,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 +398,130 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
> >
> > +static __maybe_unused 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 __maybe_unused 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 __maybe_unused 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;
> > +}
> > +
> > +static __maybe_unused void fwu_post_update_checks(
> > +     struct efi_capsule_header *capsule,
> > +     bool *fw_accept_os, bool *capsule_update)
> > +{
> > +     if (fwu_empty_capsule(capsule))
> > +             *capsule_update = false;
> > +     else
> > +             if (!*fw_accept_os)
> > +                     *fw_accept_os =
> > +                             capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> > +}
> > +
> > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > +{
> > +     int status;
> > +     u32 update_index;
> > +     efi_status_t ret;
> > +
> > +     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;
> > +     }
> > +
> > +     /*
> > +      * 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;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +}
> >
> >  /**
> >   * 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.

>
> 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
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
> >

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
> > >
> > > 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 d5f77ce83c..1d15ac98da 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
> > > @@ -269,4 +288,15 @@ void fwu_plat_get_bootidx(uint *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..7f431ab477 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,12 @@ 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
> > >
> > >  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
> > >  /* for file system access */
> > > @@ -133,6 +140,7 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
> > >   * @instance:                Instance number
> > >   * @handles:         Handles of FMP drivers
> > >   * @no_handles:              Number of handles
> > > + * @image_index_check:       Image Index check flag
> > >   *
> > >   * Search for Firmware Management Protocol drivers, matching the image
> > >   * type, @image_type and the machine instance, @instance, from the list,
> > > @@ -144,7 +152,8 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
> > >   */
> > >  static struct efi_firmware_management_protocol *
> > >  efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
> > > -          efi_handle_t *handles, efi_uintn_t no_handles)
> > > +          efi_handle_t *handles, efi_uintn_t no_handles,
> > > +          bool image_index_check)
> > >  {
> > >       efi_handle_t *handle;
> > >       struct efi_firmware_management_protocol *fmp;
> > > @@ -205,7 +214,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 +398,130 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> > >  }
> > >  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
> > >
> > > +static __maybe_unused 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 __maybe_unused 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 __maybe_unused 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;
> > > +}
> > > +
> > > +static __maybe_unused void fwu_post_update_checks(
> > > +     struct efi_capsule_header *capsule,
> > > +     bool *fw_accept_os, bool *capsule_update)
> > > +{
> > > +     if (fwu_empty_capsule(capsule))
> > > +             *capsule_update = false;
> > > +     else
> > > +             if (!*fw_accept_os)
> > > +                     *fw_accept_os =
> > > +                             capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> > > +}
> > > +
> > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > +{
> > > +     int status;
> > > +     u32 update_index;
> > > +     efi_status_t ret;
> > > +
> > > +     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;
> > > +     }
> > > +
> > > +     /*
> > > +      * 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;
> > > +             }
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > >
> > >  /**
> > >   * 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?

> >
> > 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.

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
> > >

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
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
> > > >

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 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?

-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
> > > > >

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.

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
> > > > > >

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
> > > > > > >

hi Takahiro,

On Wed, 21 Sept 2022 at 10:58, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> 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.

Yes, this is true for the normal capsule update scenario. However, in
the case of A/B updates, the above condition is no longer true. We
will have a single ImageTypeId with multiple ImageIndex values, since
we have multiple banks. And if we have to extrapolate the ImageIndex
of BankB based on the value of ImageIndex in BankA, we will have to
put that restriction on the order of placement of the images on the
storage media -- this would mean that the order of images in all banks
will have to be the same. While in case of FWU A/B updates,we do not
need that restriction, since the FWU metadata does not put any
restriction on placement of images. Which is why the image_index is
being derived at run-time based on the values of ImageTypeId and
update bank.

Actually, this is something pretty similar to the case of updating FIT
images. Even in case of FIT updates, we use a single image_index(0x1)
for the entire FIT image, and then the actual FIT update code is
figuring out the location of the constituent images at run-time based
on parsing of the FIT header. In the case of FWU(A/B) updates, we
figure out the image_index at run-time based on the values of the
ImageTypeId and the update bank. The way I see it, this is not
affecting the native capsule update code behaviour. Just that in the
case of A/B updates, the way of deriving the ImageIndex is different.

-sughosh

>
> 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
> > > > > > > >

On Wed, Sep 21, 2022 at 04:56:20PM +0530, Sughosh Ganu wrote:
> hi Takahiro,
> 
> On Wed, 21 Sept 2022 at 10:58, Takahiro Akashi
> <takahiro.akashi@linaro.org> wrote:
> >
> > 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.
> 
> Yes, this is true for the normal capsule update scenario. However, in
> the case of A/B updates, the above condition is no longer true. We
> will have a single ImageTypeId with multiple ImageIndex values, since
> we have multiple banks. And if we have to extrapolate the ImageIndex
> of BankB based on the value of ImageIndex in BankA, we will have to
> put that restriction on the order of placement of the images on the
> storage media -- this would mean that the order of images in all banks
> will have to be the same.

I don't think you fully understand what I meant to say.

As I said in my previous reply, ImageIndex is quite unique enough
to distinguish all the firmware images, which is also items in a list of
ImageInfo returned by GetImageInfo(), that a given FMP driver can handle.

This means that SetImage() doesn't have to take a ImageTypeId as one
of arguments, and so there is no reason that might prevent us from calling
fwu_get_image_index() in FMP's SetImage().
(i.e. we can drop ImageTypeId from arguments of fwu_get_image_index().)

I think that the issue is the only one that you mentioned as your concern.

> While in case of FWU A/B updates,we do not
> need that restriction, since the FWU metadata does not put any
> restriction on placement of images. Which is why the image_index is
> being derived at run-time based on the values of ImageTypeId and
> update bank.
> 
> Actually, this is something pretty similar to the case of updating FIT
> images. Even in case of FIT updates, we use a single image_index(0x1)
> for the entire FIT image, and then the actual FIT update code is
> figuring out the location of the constituent images at run-time based
> on parsing of the FIT header.

This is not a fair comparison.
As I repeatedly said, FIT FMP driver recognizes a FIT file as a single
image in respect of UEFI APIs. How it will handle a capsule is totally
up to the driver itself.
We don't have to care how it will manage multiple banks internally.

-Takahiro Akashi

> In the case of FWU(A/B) updates, we
> figure out the image_index at run-time based on the values of the
> ImageTypeId and the update bank. The way I see it, this is not
> affecting the native capsule update code behaviour. Just that in the
> case of A/B updates, the way of deriving the ImageIndex is different.
> 
> -sughosh
> 
> >
> > 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
> > > > > > > > >

On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
....
> +
> +static __maybe_unused void fwu_post_update_checks(
> +       struct efi_capsule_header *capsule,
> +       bool *fw_accept_os, bool *capsule_update)
> +{
> +       if (fwu_empty_capsule(capsule))
> +               *capsule_update = false;
> +       else
> +               if (!*fw_accept_os)
> +                       *fw_accept_os =
> +                               capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
>
True and False, instead of 1 and 0 for consistency.

.....
>
> +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> +{
> +       int status;
> +       u32 update_index;
> +       efi_status_t ret;
> +
> +       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;
> +       }
> +
> +       /*
> +        * 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);
>
Do we want to check if all images in the bank are updated via capsules
before switching the bank?

A developer will make sure all images are provided in one go, so that
the switch is successful.
But a malicious user may force some old vulnerable image back into use
by updating all but that image.

....
> @@ -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);
> +
This could be simplified as
               if (fwu_empty_capsule(capsule_data))
                       return fwu_empty_capsule_process(capsule_data);

                if (!fwu_update_checks_pass()) {
                       log_err("FWU checks failed. Cannot start update\n");
                       return EFI_INVALID_PARAMETER;
               }

....
> @@ -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)) {
>
missing newline before the if


> +               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");
> +               }
nit:  maybe keep the order of capsule_update and update_status same in
both clauses.

cheers.

On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> ....
> > +
> > +static __maybe_unused void fwu_post_update_checks(
> > +       struct efi_capsule_header *capsule,
> > +       bool *fw_accept_os, bool *capsule_update)
> > +{
> > +       if (fwu_empty_capsule(capsule))
> > +               *capsule_update = false;
> > +       else
> > +               if (!*fw_accept_os)
> > +                       *fw_accept_os =
> > +                               capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> >
> True and False, instead of 1 and 0 for consistency.

Okay

>
> .....
> >
> > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > +{
> > +       int status;
> > +       u32 update_index;
> > +       efi_status_t ret;
> > +
> > +       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;
> > +       }
> > +
> > +       /*
> > +        * 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);
> >
> Do we want to check if all images in the bank are updated via capsules
> before switching the bank?

This function does get called only when the update status for every
capsule is a success. Even if one of the capsules does not get
updated, the active index will not get updated.

>
> A developer will make sure all images are provided in one go, so that
> the switch is successful.
> But a malicious user may force some old vulnerable image back into use
> by updating all but that image.

That I believe is to be handled through a combination of implementing
a rollback protection mechanism, along with capsule authentication.
These are separate to the implementation of the multi bank updates
that these patches are aiming for.

>
> ....
> > @@ -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);
> > +
> This could be simplified as
>                if (fwu_empty_capsule(capsule_data))
>                        return fwu_empty_capsule_process(capsule_data);
>
>                 if (!fwu_update_checks_pass()) {
>                        log_err("FWU checks failed. Cannot start update\n");
>                        return EFI_INVALID_PARAMETER;
>                }

Yes, will change.

>
> ....
> > @@ -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)) {
> >
> missing newline before the if

Will add.

-sughosh

>
>
> > +               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");
> > +               }
> nit:  maybe keep the order of capsule_update and update_status same in
> both clauses.
>
> cheers.

On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:

> > .....
> > >
> > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > +{
> > > +       int status;
> > > +       u32 update_index;
> > > +       efi_status_t ret;
> > > +
> > > +       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;
> > > +       }
> > > +
> > > +       /*
> > > +        * 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);
> > >
> > Do we want to check if all images in the bank are updated via capsules
> > before switching the bank?
>
> This function does get called only when the update status for every
> capsule is a success. Even if one of the capsules does not get
> updated, the active index will not get updated.
>
.... but we don't check if the capsule for each image in the bank is
provided for update.

> >
> > A developer will make sure all images are provided in one go, so that
> > the switch is successful.
> > But a malicious user may force some old vulnerable image back into use
> > by updating all but that image.
>
> That I believe is to be handled through a combination of implementing
> a rollback protection mechanism, along with capsule authentication.
> These are separate to the implementation of the multi bank updates
> that these patches are aiming for.
>
This sounds like : we don't worry about buffer-overflow
vulnerabilities because the system will be secured and hardened by
other mechanisms.

A/B update does not _require_ rollback-protection or
capsure-authentication. A platform may rely on some other technology
for tamper-proofing.

-j

On Mon, 26 Sept 2022 at 20:24, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> > > .....
> > > >
> > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > > +{
> > > > +       int status;
> > > > +       u32 update_index;
> > > > +       efi_status_t ret;
> > > > +
> > > > +       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;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * 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);
> > > >
> > > Do we want to check if all images in the bank are updated via capsules
> > > before switching the bank?
> >
> > This function does get called only when the update status for every
> > capsule is a success. Even if one of the capsules does not get
> > updated, the active index will not get updated.
> >
> .... but we don't check if the capsule for each image in the bank is
> provided for update.

Yes, we have had this discussion earlier. Neither the FWU spec, nor
the capsule update spec in UEFI puts that restriction that all images
on the platform need to be updated. If a user wants to ensure such a
behaviour, they may choose some kind of image packaging like FIP or
FIT which would mean that all the images on the platform are being
updated. But this is not something to be ensured by the FWU update
code.

>
> > >
> > > A developer will make sure all images are provided in one go, so that
> > > the switch is successful.
> > > But a malicious user may force some old vulnerable image back into use
> > > by updating all but that image.
> >
> > That I believe is to be handled through a combination of implementing
> > a rollback protection mechanism, along with capsule authentication.
> > These are separate to the implementation of the multi bank updates
> > that these patches are aiming for.
> >
> This sounds like : we don't worry about buffer-overflow
> vulnerabilities because the system will be secured and hardened by
> other mechanisms.

Not sure how this is related. The aim of the FWU spec is for providing
a means for a platform to maintain multiple partitions of images and
to specify the metadata structure for the bookkeeping of the different
partitions and images on those partitions. If we need a more secure
and hardened system, we do have the Dependable Boot spec, which is
talking precisely about these things. Those are indeed separate
features or aspects from what the FWU spec is talking about. And this
patchset is implementing the FWU spec.

-sughosh

>
> A/B update does not _require_ rollback-protection or
> capsure-authentication. A platform may rely on some other technology
> for tamper-proofing.
>
> -j

On Tue, Sep 27, 2022 at 2:22 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 20:24, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > > > .....
> > > > >
> > > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > > > +{
> > > > > +       int status;
> > > > > +       u32 update_index;
> > > > > +       efi_status_t ret;
> > > > > +
> > > > > +       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;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * 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);
> > > > >
> > > > Do we want to check if all images in the bank are updated via capsules
> > > > before switching the bank?
> > >
> > > This function does get called only when the update status for every
> > > capsule is a success. Even if one of the capsules does not get
> > > updated, the active index will not get updated.
> > >
> > .... but we don't check if the capsule for each image in the bank is
> > provided for update.
>
> Yes, we have had this discussion earlier. Neither the FWU spec, nor
> the capsule update spec in UEFI puts that restriction that all images
> on the platform need to be updated. If a user wants to ensure such a
> behaviour, they may choose some kind of image packaging like FIP or
> FIT which would mean that all the images on the platform are being
> updated. But this is not something to be ensured by the FWU update
> code.
>
> >
> > > >
> > > > A developer will make sure all images are provided in one go, so that
> > > > the switch is successful.
> > > > But a malicious user may force some old vulnerable image back into use
> > > > by updating all but that image.
> > >
> > > That I believe is to be handled through a combination of implementing
> > > a rollback protection mechanism, along with capsule authentication.
> > > These are separate to the implementation of the multi bank updates
> > > that these patches are aiming for.
> > >
> > This sounds like : we don't worry about buffer-overflow
> > vulnerabilities because the system will be secured and hardened by
> > other mechanisms.
>
> Not sure how this is related. The aim of the FWU spec is for providing
> a means for a platform to maintain multiple partitions of images and
> to specify the metadata structure for the bookkeeping of the different
> partitions and images on those partitions. If we need a more secure
> and hardened system, we do have the Dependable Boot spec, which is
> talking precisely about these things. Those are indeed separate
> features or aspects from what the FWU spec is talking about. And this
> patchset is implementing the FWU spec.
>

I am out of ways to explain. Best of luck.

On Tue, 27 Sept 2022 at 22:19, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Tue, Sep 27, 2022 at 2:22 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Mon, 26 Sept 2022 at 20:24, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > > > .....
> > > > > >
> > > > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > > > > +{
> > > > > > +       int status;
> > > > > > +       u32 update_index;
> > > > > > +       efi_status_t ret;
> > > > > > +
> > > > > > +       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;
> > > > > > +       }
> > > > > > +
> > > > > > +       /*
> > > > > > +        * 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);
> > > > > >
> > > > > Do we want to check if all images in the bank are updated via capsules
> > > > > before switching the bank?
> > > >
> > > > This function does get called only when the update status for every
> > > > capsule is a success. Even if one of the capsules does not get
> > > > updated, the active index will not get updated.
> > > >
> > > .... but we don't check if the capsule for each image in the bank is
> > > provided for update.
> >
> > Yes, we have had this discussion earlier. Neither the FWU spec, nor
> > the capsule update spec in UEFI puts that restriction that all images
> > on the platform need to be updated. If a user wants to ensure such a
> > behaviour, they may choose some kind of image packaging like FIP or
> > FIT which would mean that all the images on the platform are being
> > updated. But this is not something to be ensured by the FWU update
> > code.
> >
> > >
> > > > >
> > > > > A developer will make sure all images are provided in one go, so that
> > > > > the switch is successful.
> > > > > But a malicious user may force some old vulnerable image back into use
> > > > > by updating all but that image.
> > > >
> > > > That I believe is to be handled through a combination of implementing
> > > > a rollback protection mechanism, along with capsule authentication.
> > > > These are separate to the implementation of the multi bank updates
> > > > that these patches are aiming for.
> > > >
> > > This sounds like : we don't worry about buffer-overflow
> > > vulnerabilities because the system will be secured and hardened by
> > > other mechanisms.
> >
> > Not sure how this is related. The aim of the FWU spec is for providing
> > a means for a platform to maintain multiple partitions of images and
> > to specify the metadata structure for the bookkeeping of the different
> > partitions and images on those partitions. If we need a more secure
> > and hardened system, we do have the Dependable Boot spec, which is
> > talking precisely about these things. Those are indeed separate
> > features or aspects from what the FWU spec is talking about. And this
> > patchset is implementing the FWU spec.
> >
>
> I am out of ways to explain. Best of luck.

Let me try one last time. If you still do not agree with what I say,
let us agree to disagree :)

You mentioned earlier that a malicious user may force some old
vulnerable image back into use by updating all but that image. Now a
few points to consider here. The FWU spec recommends that platforms
follow the Platform Security Boot Guide [1] which specifies the
trusted boot flow for platforms where the firmware images being booted
on the platform, including the NWd bootloader(BL33 in tf-a jargon) are
verified before getting booted. So, if the platform is booting with
the trusted boot flow, the above scenario should not occur. Secondly,
at the time of the update, the capsule can be authenticated before
getting written to the storage. This again should be able to identify
a malicious image getting written to the device. So this hypothesis of
a malicious image being present should not occur with authenticated
capsules and a trusted boot flow. And lastly, if a supposedly good
image is found to have some vulnerability because of which an image
with a fix should be installed and the vulnerable image not allowed to
boot, this is precisely what we have rollback protection for.

You also mentioned that a platform might not support these
functionalities. Well, if these are not being supported, we don't have
much of a safety net here. Moreover, one more point to think here is
that if it is possible for the user to install and boot a vulnerable
image on the platform in the first place, it will not be protected by
updating all the images -- the malicious user can still put a corrupt
image as part of the update image bundle. The protection against
booting a corrupt image or a buggy image has to come through
mechanisms like trusted boot and rollback protection.

-sughosh

[1] - https://developer.arm.com/documentation/den0072/0101/?lang=en

Hello Jassi, Sughosh and all,

On Wed, 28 Sept 2022 at 08:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 22:19, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Tue, Sep 27, 2022 at 2:22 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Mon, 26 Sept 2022 at 20:24, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > > > .....
> > > > > > >
> > > > > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > > > > > > +{
> > > > > > > +       int status;
> > > > > > > +       u32 update_index;
> > > > > > > +       efi_status_t ret;
> > > > > > > +
> > > > > > > +       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;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * 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);
> > > > > > >
> > > > > > Do we want to check if all images in the bank are updated via capsules
> > > > > > before switching the bank?
> > > > >
> > > > > This function does get called only when the update status for every
> > > > > capsule is a success. Even if one of the capsules does not get
> > > > > updated, the active index will not get updated.
> > > > >
> > > > .... but we don't check if the capsule for each image in the bank is
> > > > provided for update.
> > >
> > > Yes, we have had this discussion earlier. Neither the FWU spec, nor
> > > the capsule update spec in UEFI puts that restriction that all images
> > > on the platform need to be updated. If a user wants to ensure such a
> > > behaviour, they may choose some kind of image packaging like FIP or
> > > FIT which would mean that all the images on the platform are being
> > > updated. But this is not something to be ensured by the FWU update
> > > code.
> > >
> > > >
> > > > > >
> > > > > > A developer will make sure all images are provided in one go, so that
> > > > > > the switch is successful.
> > > > > > But a malicious user may force some old vulnerable image back into use
> > > > > > by updating all but that image.
> > > > >
> > > > > That I believe is to be handled through a combination of implementing
> > > > > a rollback protection mechanism, along with capsule authentication.
> > > > > These are separate to the implementation of the multi bank updates
> > > > > that these patches are aiming for.
> > > > >
> > > > This sounds like : we don't worry about buffer-overflow
> > > > vulnerabilities because the system will be secured and hardened by
> > > > other mechanisms.
> > >
> > > Not sure how this is related. The aim of the FWU spec is for providing
> > > a means for a platform to maintain multiple partitions of images and
> > > to specify the metadata structure for the bookkeeping of the different
> > > partitions and images on those partitions. If we need a more secure
> > > and hardened system, we do have the Dependable Boot spec, which is
> > > talking precisely about these things. Those are indeed separate
> > > features or aspects from what the FWU spec is talking about. And this
> > > patchset is implementing the FWU spec.
> > >
> >
> > I am out of ways to explain. Best of luck.
>
> Let me try one last time. If you still do not agree with what I say,
> let us agree to disagree :)
>
> You mentioned earlier that a malicious user may force some old
> vulnerable image back into use by updating all but that image. Now a
> few points to consider here. The FWU spec recommends that platforms
> follow the Platform Security Boot Guide [1] which specifies the
> trusted boot flow for platforms where the firmware images being booted
> on the platform, including the NWd bootloader(BL33 in tf-a jargon) are
> verified before getting booted.

Let me add that in tf-a we can bind booted images (actually booted
certificates) version id to a platform non-volatile monotonic counter.
This adds rollback protection on booted images upgrades.

Enabling firmware update, we need this rollback protection to prevent
cases like you mentioned above:
 >>> But a malicious user may force some old vulnerable image back into use
 >>> by updating all but that image.

When the system boots with accepted images (referring to fwu-mdata
regular/trial state), the platform monotonic counter is updated
against booted image version number if needed, preventing older images
to be booted when an accepted image has been deployed.
@Jassi, does this answer your question?


> So, if the platform is booting with
> the trusted boot flow, the above scenario should not occur. Secondly,
> at the time of the update, the capsule can be authenticated before
> getting written to the storage. This again should be able to identify
> a malicious image getting written to the device. So this hypothesis of
> a malicious image being present should not occur with authenticated
> capsules and a trusted boot flow.

Authenticating capsules rather relates to reducing the attack surface
IMHO. I don't mean it is useless, far from it.
In our system, authenticated boot is the boot guard in the chain of
trust. With firmware update, the guard must prevent firmware version
rollback.


> And lastly, if a supposedly good
> image is found to have some vulnerability because of which an image
> with a fix should be installed and the vulnerable image not allowed to
> boot, this is precisely what we have rollback protection for.
>
> You also mentioned that a platform might not support these
> functionalities. Well, if these are not being supported, we don't have
> much of a safety net here. Moreover, one more point to think here is
> that if it is possible for the user to install and boot a vulnerable
> image on the platform in the first place, it will not be protected by
> updating all the images -- the malicious user can still put a corrupt
> image as part of the update image bundle. The protection against
> booting a corrupt image or a buggy image has to come through
> mechanisms like trusted boot and rollback protection.

"rollback protection" here should also cover tracking of updatable
images version ID.

Best regards,
Etienne

>
> -sughosh
>
> [1] - https://developer.arm.com/documentation/den0072/0101/?lang=en

Hi Etienne,

On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
> Hello Jassi, Sughosh and all,
>
>  >>> But a malicious user may force some old vulnerable image back into use
>  >>> by updating all but that image.
>
> When the system boots with accepted images (referring to fwu-mdata
> regular/trial state), the platform monotonic counter is updated
> against booted image version number if needed, preventing older images
> to be booted when an accepted image has been deployed.
> @Jassi, does this answer your question?
>
As I said in my earlier post, I know we can employ security+integrity
techniques to prevent such misuse.
My point is FWU should still be implemented assuming no such technique
might be available due to any reason, and we do the best we can. Just
as we don't say lets not care about buffer-overflow vulnerabilities
because the system can implement secure boot and other such
techniques.

For example, the spec warns : "The metadata can be maliciously
crafted, it should be treated as an insecure information source." So
clearly the spec doesn't count on rollback and authentication
mechanisms to be always available - and that is how it should be.

cheers.

Hello Jassi,

On Wed, 28 Sept 2022 at 17:17, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> Hi Etienne,
>
> On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> > Hello Jassi, Sughosh and all,
> >
> >  >>> But a malicious user may force some old vulnerable image back into use
> >  >>> by updating all but that image.
> >
> > When the system boots with accepted images (referring to fwu-mdata
> > regular/trial state), the platform monotonic counter is updated
> > against booted image version number if needed, preventing older images
> > to be booted when an accepted image has been deployed.
> > @Jassi, does this answer your question?
> >
> As I said in my earlier post, I know we can employ security+integrity
> techniques to prevent such misuse.
> My point is FWU should still be implemented assuming no such technique
> might be available due to any reason, and we do the best we can. Just
> as we don't say lets not care about buffer-overflow vulnerabilities
> because the system can implement secure boot and other such
> techniques.
>
> For example, the spec warns : "The metadata can be maliciously
> crafted, it should be treated as an insecure information source." So
> clearly the spec doesn't count on rollback and authentication
> mechanisms to be always available - and that is how it should be.

It is true fwu metadata content are not secure, as the GPT content itself.
We cannot prevent OS from corrupting fwu-mdata partitions or the
device GPT despite the boot sequence heavily relies on their
information.
When fwu mdata and GPT are not secure, FWU only allows updating boot
image, it cannot secure them.
Early boot stage (tf-a) is in charge of verifying booted images
(authent. and rollback counter), whatever the booted images are.

Best regards,
etienne


>
> cheers.

Hi Jassi, 

On Wed, Sep 28, 2022 at 10:16:53AM -0500, Jassi Brar wrote:
> Hi Etienne,
> 
> On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> > Hello Jassi, Sughosh and all,
> >
> >  >>> But a malicious user may force some old vulnerable image back into use
> >  >>> by updating all but that image.
> >
> > When the system boots with accepted images (referring to fwu-mdata
> > regular/trial state), the platform monotonic counter is updated
> > against booted image version number if needed, preventing older images
> > to be booted when an accepted image has been deployed.
> > @Jassi, does this answer your question?
> >
> As I said in my earlier post, I know we can employ security+integrity
> techniques to prevent such misuse.
> My point is FWU should still be implemented assuming no such technique
> might be available due to any reason, and we do the best we can. Just
> as we don't say lets not care about buffer-overflow vulnerabilities
> because the system can implement secure boot and other such
> techniques.
> 
> For example, the spec warns : "The metadata can be maliciously
> crafted, it should be treated as an insecure information source." So
> clearly the spec doesn't count on rollback and authentication
> mechanisms to be always available - and that is how it should be.

We've discussed this extensively during drafting the spec.  You are right
that we would be better off trying to protect the fwu metadata somehow.  In
fact Heinrich had similar concerns when the original RFC was posted.  i

But can you think of such a reliable mechanism?  The only thing
we could come up without overcomplicating the entire spec was a device that
boots from the secure world and stores the metadata either in a flash there
or a device with such protection mechanisms (e.g an RPMB).

Cheers
/Ilias

> 
> cheers.

Hi Ilias,

On Mon, Oct 3, 2022 at 7:21 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jassi,
>
> On Wed, Sep 28, 2022 at 10:16:53AM -0500, Jassi Brar wrote:
> > Hi Etienne,
> >
> > On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > > Hello Jassi, Sughosh and all,
> > >
> > >  >>> But a malicious user may force some old vulnerable image back into use
> > >  >>> by updating all but that image.
> > >
> > > When the system boots with accepted images (referring to fwu-mdata
> > > regular/trial state), the platform monotonic counter is updated
> > > against booted image version number if needed, preventing older images
> > > to be booted when an accepted image has been deployed.
> > > @Jassi, does this answer your question?
> > >
> > As I said in my earlier post, I know we can employ security+integrity
> > techniques to prevent such misuse.
> > My point is FWU should still be implemented assuming no such technique
> > might be available due to any reason, and we do the best we can. Just
> > as we don't say lets not care about buffer-overflow vulnerabilities
> > because the system can implement secure boot and other such
> > techniques.
> >
> > For example, the spec warns : "The metadata can be maliciously
> > crafted, it should be treated as an insecure information source." So
> > clearly the spec doesn't count on rollback and authentication
> > mechanisms to be always available - and that is how it should be.
>
> We've discussed this extensively during drafting the spec.  You are right
> that we would be better off trying to protect the fwu metadata somehow.  In
> fact Heinrich had similar concerns when the original RFC was posted.  i
>
Actually I never said we should protect the metadata.
If you read the whole thread, the point was that we should try to
protect against partial bank updates - accidental or malicious. We can
not assume a user updating only partially, knows what they are doing.

cheers.