Re: [PATCH v7 09/13] FWU: Add support for the FWU Multi Bank Update feature

2022-07-15 Thread Ilias Apalodimas 
Hi Sughosh,

[...]

>  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>
> +static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
> +{
> +   return !guidcmp(>capsule_guid,
> +   _guid_os_request_fw_revert) ||
> +   !guidcmp(>capsule_guid,
> +_guid_os_request_fw_accept);
> +}
> +
> +static efi_status_t fwu_empty_capsule_process(
> +   struct efi_capsule_header *capsule)
> +{
> +   int status;
> +   u32 active_idx;
> +   efi_status_t ret;
> +   efi_guid_t *image_guid;
> +
> +   if (!guidcmp(>capsule_guid,
> +_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();
> +   if (status < 0) {
> +   log_err("Failed to revert the FWU boot index\n");
> +   if (status == -ENODEV ||
> +   status == -ERANGE ||
> +   status == -EIO)
> +   ret = EFI_DEVICE_ERROR;
> +   else if (status == -EINVAL)
> +   ret = EFI_INVALID_PARAMETER;
> +   else
> +   ret = EFI_OUT_OF_RESOURCES;

In all the case you carry those if statements,  define a function like

static efi_status_t fwu_to_efi_error (int err)
{
switch(err) {
case -ENODEV:
case -ERANGE:
case -EIO:
return EFI_DEVICE_ERROR;
}
.
}
and use it in the error handling below.
That should make the weird looking error handling go away and adding
more cases in the future a lot easier.

> +   } else {
> +   ret = EFI_SUCCESS;
> +   log_err("Reverted the FWU active_index. Recommend 
> rebooting the system\n");

Does it really need log_err?

[...]

Regards
/Ilias

[PATCH v7 09/13] FWU: Add support for the FWU Multi Bank Update feature

2022-07-14 Thread Sughosh Ganu 
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 
---
Changes since V6: None

 include/fwu.h|  10 ++
 lib/Kconfig  |   6 +
 lib/Makefile |   1 +
 lib/efi_loader/efi_capsule.c | 231 ++-
 lib/efi_loader/efi_setup.c   |   3 +-
 lib/fwu_updates/Kconfig  |  31 +
 lib/fwu_updates/Makefile |   3 +-
 lib/fwu_updates/fwu.c|  26 
 8 files changed, 304 insertions(+), 7 deletions(-)
 create mode 100644 lib/fwu_updates/Kconfig

diff --git a/include/fwu.h b/include/fwu.h
index b374fd1179..7ff1cd75d3 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -32,13 +32,23 @@ struct fwu_mdata_ops {
 };
 
 #define FWU_MDATA_VERSION  0x1
+#define FWU_IMAGE_ACCEPTED 0x1
 
 #define FWU_MDATA_GUID \
EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
 
+#define FWU_OS_REQUEST_FW_REVERT_GUID \
+   EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \
+0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0)
+
+#define FWU_OS_REQUEST_FW_ACCEPT_GUID \
+   EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
+0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
+
 u8 fwu_update_checks_pass(void);
 int fwu_boottime_checks(void);
+int fwu_trial_state_ctr_start(void);
 
 int fwu_get_mdata(struct fwu_mdata **mdata);
 int fwu_update_mdata(struct fwu_mdata *mdata);
diff --git a/lib/Kconfig b/lib/Kconfig
index 7dd777b56a..0e5390597c 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -967,3 +967,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..18a356ba08 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -32,6 +33,17 @@ static const efi_guid_t 
efi_guid_firmware_management_capsule_id =
EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
 const efi_guid_t efi_guid_firmware_management_protocol =
EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID;
+const efi_guid_t fwu_guid_os_request_fw_revert =
+   FWU_OS_REQUEST_FW_REVERT_GUID;
+const efi_guid_t fwu_guid_os_request_fw_accept =
+   FWU_OS_REQUEST_FW_ACCEPT_GUID;
+
+#define FW_ACCEPT_OS   (u32)0x8000
+
+__maybe_unused static u32 update_index;
+__maybe_unused static bool capsule_update;
+__maybe_unused static bool fw_accept_os;
+static bool image_index_check = true;
 
 #ifdef CONFIG_EFI_CAPSULE_ON_DISK
 /* for file system access */
@@ -205,7 +217,8 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 
instance,
log_debug("+++ desc[%d] index: %d, name: %ls\n",
  j, desc->image_index, desc->image_id_name);
if (!guidcmp(>image_type_id, image_type) &&
-   (desc->image_index == image_index) &&
+   (!image_index_check ||
+desc->image_index == image_index) &&
(!instance ||
 !desc->hardware_instance ||
  desc->hardware_instance == instance))
@@ -388,6 +401,87 @@ efi_status_t efi_capsule_authenticate(const void *capsule, 
efi_uintn_t capsule_s
 }
 #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
 
+static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
+{
+   return !guidcmp(>capsule_guid,
+   _guid_os_request_fw_revert) ||
+   !guidcmp(>capsule_guid,
+_guid_os_request_fw_accept);
+}
+
+static efi_status_t fwu_empty_capsule_process(
+   struct efi_capsule_header *capsule)
+{
+   int status;
+   u32 active_idx;
+   efi_status_t ret;