* [PATCH] efi_loader: FMP cleanups
@ 2021-06-14 15:10 Ilias Apalodimas
2021-06-14 15:13 ` Ilias Apalodimas
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-14 15:10 UTC (permalink / raw)
To: xypron.glpk
Cc: masami.hiramatsu, takahiro.akashi, Ilias Apalodimas, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
Right now we allow both of the FMPs (RAW and FIT based) to be installed at
the same time. Moreover we only install those if a CapsuleUpdate is
requested. Since we now have an ESRT table, it makes more sense to
unconditionally install the FMP, so any userspace applications (e.g fwupd)
can make use of them and trigger an update.
While at it clean up the FMP installation as well. Chapter 23 of the EFI
spec (rev 2.9) says:
"A specific updatable hardware firmware store must be represented by
exactly one FMP instance".
This is not the case for us, since both of our FMP protocols can be
installed at the same time and are controlled by a single 'dfu_alt_info'
env variable.
So make the config option a choice and allow the user to install one
of them at any given time.
The overall changes show up in fwupd
pre-patch:
fwupdmgr get-devices
No detected devices
post-patch (with FIT FMP installed):
fwupdmgr get-devices
WARNING: Required efivarfs filesystem was not found
See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
Unknown Product
│
└─Unknown Firmware:
Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
Current version: 0
Vendor: DMI:U-Boot
Update Error: Not updatable as efivarfs was not found
GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
Device Flags: • Internal device
• System requires external power source
• Needs a reboot after installation
• Device is usable for the duration of the update
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
configs/sandbox64_defconfig | 1 -
configs/sandbox_defconfig | 1 -
configs/xilinx_zynqmp_virt_defconfig | 1 -
include/efi_loader.h | 1 +
lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
lib/efi_loader/efi_capsule.c | 22 ++++---------
lib/efi_loader/efi_setup.c | 4 +++
7 files changed, 37 insertions(+), 41 deletions(-)
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 9a373bab6fe3..af18b6c7826e 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -233,7 +233,6 @@ CONFIG_LZ4=y
CONFIG_ERRNO_STR=y
CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
CONFIG_EFI_CAPSULE_ON_DISK=y
-CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
CONFIG_EFI_SECURE_BOOT=y
CONFIG_TEST_FDTDEC=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index bdbf714e2bd9..24313fdfa53d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -280,7 +280,6 @@ CONFIG_LZ4=y
CONFIG_ERRNO_STR=y
CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
CONFIG_EFI_CAPSULE_ON_DISK=y
-CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
CONFIG_EFI_SECURE_BOOT=y
CONFIG_TEST_FDTDEC=y
diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
index e939b04ef6a5..0c2d1a70a5a1 100644
--- a/configs/xilinx_zynqmp_virt_defconfig
+++ b/configs/xilinx_zynqmp_virt_defconfig
@@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
CONFIG_EFI_CAPSULE_ON_DISK=y
CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
-CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0a9c82a257e1..b81180cfda8b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
* - error code otherwise.
*/
efi_status_t efi_esrt_populate(void);
+efi_status_t efi_load_capsule_drivers(void);
#endif /* _EFI_LOADER_H */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 6242caceb7f9..da6f5faf5adb 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
Select this option if you want to enable capsule-based
firmware update using Firmware Management Protocol.
+choice EFI_CAPSULE_TYPE
+ prompt "Capsule type (RAW/FIT)"
+ depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
+
+config EFI_CAPSULE_FIRMWARE_FIT
+ bool "FMP driver for FIT images"
+ depends on FIT
+ select UPDATE_FIT
+ select DFU
+ select EFI_CAPSULE_FIRMWARE
+ help
+ Select this option if you want to enable firmware management protocol
+ driver for FIT image
+
+config EFI_CAPSULE_FIRMWARE_RAW
+ bool "FMP driver for raw images"
+ select DFU_WRITE_ALT
+ select DFU
+ select EFI_CAPSULE_FIRMWARE
+ help
+ Select this option if you want to enable firmware management protocol
+ driver for raw image
+
+endchoice
+
config EFI_CAPSULE_AUTHENTICATE
bool "Update Capsule authentication"
depends on EFI_CAPSULE_FIRMWARE
@@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
Select this option if you want to enable capsule
authentication
-config EFI_CAPSULE_FIRMWARE_FIT
- bool "FMP driver for FIT image"
- depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
- depends on FIT
- select UPDATE_FIT
- select DFU
- select EFI_CAPSULE_FIRMWARE
- default n
- help
- Select this option if you want to enable firmware management protocol
- driver for FIT image
-
-config EFI_CAPSULE_FIRMWARE_RAW
- bool "FMP driver for raw image"
- depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
- select DFU
- select DFU_WRITE_ALT
- select EFI_CAPSULE_FIRMWARE
- default n
- help
- Select this option if you want to enable firmware management protocol
- driver for raw image
-
config EFI_DEVICE_PATH_TO_TEXT
bool "Device path to text protocol"
default y
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 9ead0d2c7816..3b4214a8d4cc 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
}
/**
- * arch_efi_load_capsule_drivers - initialize capsule drivers
+ * efi_load_capsule_drivers - initialize capsule drivers
*
- * Architecture or board specific initialization routine
+ * Generic FMP drivers backed by DFU
*
* Return: status code
*/
-efi_status_t __weak arch_efi_load_capsule_drivers(void)
+efi_status_t efi_load_capsule_drivers(void)
{
- __maybe_unused efi_handle_t handle;
+ __maybe_unused efi_handle_t handle = NULL;
efi_status_t ret = EFI_SUCCESS;
- if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
- handle = NULL;
+ if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
&handle, &efi_guid_firmware_management_protocol,
&efi_fmp_fit, NULL));
- }
- if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
- handle = NULL;
+ if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
- &efi_root,
+ &handle,
&efi_guid_firmware_management_protocol,
&efi_fmp_raw, NULL));
- }
return ret;
}
@@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
index = get_last_capsule();
- /* Load capsule drivers */
- ret = arch_efi_load_capsule_drivers();
- if (ret != EFI_SUCCESS)
- return ret;
/*
* Find capsules on disk.
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 3c5cf9a4357e..0106efdc161b 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
if (ret != EFI_SUCCESS)
goto out;
+ ret = efi_load_capsule_drivers();
+ if (ret != EFI_SUCCESS)
+ goto out;
+
#if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
ret = efi_gop_register();
if (ret != EFI_SUCCESS)
--
2.32.0.rc0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-14 15:10 [PATCH] efi_loader: FMP cleanups Ilias Apalodimas
@ 2021-06-14 15:13 ` Ilias Apalodimas
2021-06-15 0:49 ` Masami Hiramatsu
2021-06-15 1:51 ` AKASHI Takahiro
2 siblings, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-14 15:13 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Masami Hiramatsu, Takahiro Akashi, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, U-Boot Mailing List
Too fast on the trigger.
The efi_load_capsule_drivers() must go into an IS_ENABLED. I'll wait
for any other comments and send a V2
On Mon, 14 Jun 2021 at 18:10, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> the same time. Moreover we only install those if a CapsuleUpdate is
> requested. Since we now have an ESRT table, it makes more sense to
> unconditionally install the FMP, so any userspace applications (e.g fwupd)
> can make use of them and trigger an update.
>
> While at it clean up the FMP installation as well. Chapter 23 of the EFI
> spec (rev 2.9) says:
> "A specific updatable hardware firmware store must be represented by
> exactly one FMP instance".
> This is not the case for us, since both of our FMP protocols can be
> installed at the same time and are controlled by a single 'dfu_alt_info'
> env variable.
> So make the config option a choice and allow the user to install one
> of them at any given time.
>
> The overall changes show up in fwupd
>
> pre-patch:
> fwupdmgr get-devices
> No detected devices
>
> post-patch (with FIT FMP installed):
> fwupdmgr get-devices
> WARNING: Required efivarfs filesystem was not found
> See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> Unknown Product
> │
> └─Unknown Firmware:
> Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> Current version: 0
> Vendor: DMI:U-Boot
> Update Error: Not updatable as efivarfs was not found
> GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> Device Flags: • Internal device
> • System requires external power source
> • Needs a reboot after installation
> • Device is usable for the duration of the update
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> configs/sandbox64_defconfig | 1 -
> configs/sandbox_defconfig | 1 -
> configs/xilinx_zynqmp_virt_defconfig | 1 -
> include/efi_loader.h | 1 +
> lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> lib/efi_loader/efi_capsule.c | 22 ++++---------
> lib/efi_loader/efi_setup.c | 4 +++
> 7 files changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 9a373bab6fe3..af18b6c7826e 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index bdbf714e2bd9..24313fdfa53d 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> index e939b04ef6a5..0c2d1a70a5a1 100644
> --- a/configs/xilinx_zynqmp_virt_defconfig
> +++ b/configs/xilinx_zynqmp_virt_defconfig
> @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0a9c82a257e1..b81180cfda8b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> * - error code otherwise.
> */
> efi_status_t efi_esrt_populate(void);
> +efi_status_t efi_load_capsule_drivers(void);
> #endif /* _EFI_LOADER_H */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 6242caceb7f9..da6f5faf5adb 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> Select this option if you want to enable capsule-based
> firmware update using Firmware Management Protocol.
>
> +choice EFI_CAPSULE_TYPE
> + prompt "Capsule type (RAW/FIT)"
> + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> +
> +config EFI_CAPSULE_FIRMWARE_FIT
> + bool "FMP driver for FIT images"
> + depends on FIT
> + select UPDATE_FIT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for FIT image
> +
> +config EFI_CAPSULE_FIRMWARE_RAW
> + bool "FMP driver for raw images"
> + select DFU_WRITE_ALT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for raw image
> +
> +endchoice
> +
> config EFI_CAPSULE_AUTHENTICATE
> bool "Update Capsule authentication"
> depends on EFI_CAPSULE_FIRMWARE
> @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> Select this option if you want to enable capsule
> authentication
>
> -config EFI_CAPSULE_FIRMWARE_FIT
> - bool "FMP driver for FIT image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - depends on FIT
> - select UPDATE_FIT
> - select DFU
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for FIT image
> -
> -config EFI_CAPSULE_FIRMWARE_RAW
> - bool "FMP driver for raw image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - select DFU
> - select DFU_WRITE_ALT
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for raw image
> -
> config EFI_DEVICE_PATH_TO_TEXT
> bool "Device path to text protocol"
> default y
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 9ead0d2c7816..3b4214a8d4cc 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> }
>
> /**
> - * arch_efi_load_capsule_drivers - initialize capsule drivers
> + * efi_load_capsule_drivers - initialize capsule drivers
> *
> - * Architecture or board specific initialization routine
> + * Generic FMP drivers backed by DFU
> *
> * Return: status code
> */
> -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> +efi_status_t efi_load_capsule_drivers(void)
> {
> - __maybe_unused efi_handle_t handle;
> + __maybe_unused efi_handle_t handle = NULL;
> efi_status_t ret = EFI_SUCCESS;
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> &handle, &efi_guid_firmware_management_protocol,
> &efi_fmp_fit, NULL));
> - }
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &efi_root,
> + &handle,
> &efi_guid_firmware_management_protocol,
> &efi_fmp_raw, NULL));
> - }
>
> return ret;
> }
> @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
>
> index = get_last_capsule();
>
> - /* Load capsule drivers */
> - ret = arch_efi_load_capsule_drivers();
> - if (ret != EFI_SUCCESS)
> - return ret;
>
> /*
> * Find capsules on disk.
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 3c5cf9a4357e..0106efdc161b 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> if (ret != EFI_SUCCESS)
> goto out;
>
> + ret = efi_load_capsule_drivers();
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> ret = efi_gop_register();
> if (ret != EFI_SUCCESS)
> --
> 2.32.0.rc0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-14 15:10 [PATCH] efi_loader: FMP cleanups Ilias Apalodimas
2021-06-14 15:13 ` Ilias Apalodimas
@ 2021-06-15 0:49 ` Masami Hiramatsu
2021-06-15 3:56 ` Ilias Apalodimas
2021-06-15 1:51 ` AKASHI Takahiro
2 siblings, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2021-06-15 0:49 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Heinrich Schuchardt, Takahiro Akashi, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, U-Boot Mailing List
2021年6月15日(火) 0:10 Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>
> Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> the same time. Moreover we only install those if a CapsuleUpdate is
> requested. Since we now have an ESRT table, it makes more sense to
> unconditionally install the FMP, so any userspace applications (e.g fwupd)
> can make use of them and trigger an update.
>
> While at it clean up the FMP installation as well. Chapter 23 of the EFI
> spec (rev 2.9) says:
> "A specific updatable hardware firmware store must be represented by
> exactly one FMP instance".
> This is not the case for us, since both of our FMP protocols can be
> installed at the same time and are controlled by a single 'dfu_alt_info'
> env variable.
> So make the config option a choice and allow the user to install one
> of them at any given time.
>
> The overall changes show up in fwupd
>
> pre-patch:
> fwupdmgr get-devices
> No detected devices
>
> post-patch (with FIT FMP installed):
> fwupdmgr get-devices
> WARNING: Required efivarfs filesystem was not found
> See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> Unknown Product
> │
> └─Unknown Firmware:
> Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> Current version: 0
> Vendor: DMI:U-Boot
> Update Error: Not updatable as efivarfs was not found
> GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> Device Flags: • Internal device
> • System requires external power source
> • Needs a reboot after installation
> • Device is usable for the duration of the update
>
This looks good to me, and this covers one patch which I sent before.
https://lists.denx.de/pipermail/u-boot/2021-June/451401.html
Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Thank you!
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> configs/sandbox64_defconfig | 1 -
> configs/sandbox_defconfig | 1 -
> configs/xilinx_zynqmp_virt_defconfig | 1 -
> include/efi_loader.h | 1 +
> lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> lib/efi_loader/efi_capsule.c | 22 ++++---------
> lib/efi_loader/efi_setup.c | 4 +++
> 7 files changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 9a373bab6fe3..af18b6c7826e 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index bdbf714e2bd9..24313fdfa53d 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> index e939b04ef6a5..0c2d1a70a5a1 100644
> --- a/configs/xilinx_zynqmp_virt_defconfig
> +++ b/configs/xilinx_zynqmp_virt_defconfig
> @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0a9c82a257e1..b81180cfda8b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> * - error code otherwise.
> */
> efi_status_t efi_esrt_populate(void);
> +efi_status_t efi_load_capsule_drivers(void);
> #endif /* _EFI_LOADER_H */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 6242caceb7f9..da6f5faf5adb 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> Select this option if you want to enable capsule-based
> firmware update using Firmware Management Protocol.
>
> +choice EFI_CAPSULE_TYPE
> + prompt "Capsule type (RAW/FIT)"
> + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> +
> +config EFI_CAPSULE_FIRMWARE_FIT
> + bool "FMP driver for FIT images"
> + depends on FIT
> + select UPDATE_FIT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for FIT image
> +
> +config EFI_CAPSULE_FIRMWARE_RAW
> + bool "FMP driver for raw images"
> + select DFU_WRITE_ALT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for raw image
> +
> +endchoice
> +
> config EFI_CAPSULE_AUTHENTICATE
> bool "Update Capsule authentication"
> depends on EFI_CAPSULE_FIRMWARE
> @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> Select this option if you want to enable capsule
> authentication
>
> -config EFI_CAPSULE_FIRMWARE_FIT
> - bool "FMP driver for FIT image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - depends on FIT
> - select UPDATE_FIT
> - select DFU
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for FIT image
> -
> -config EFI_CAPSULE_FIRMWARE_RAW
> - bool "FMP driver for raw image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - select DFU
> - select DFU_WRITE_ALT
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for raw image
> -
> config EFI_DEVICE_PATH_TO_TEXT
> bool "Device path to text protocol"
> default y
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 9ead0d2c7816..3b4214a8d4cc 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> }
>
> /**
> - * arch_efi_load_capsule_drivers - initialize capsule drivers
> + * efi_load_capsule_drivers - initialize capsule drivers
> *
> - * Architecture or board specific initialization routine
> + * Generic FMP drivers backed by DFU
> *
> * Return: status code
> */
> -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> +efi_status_t efi_load_capsule_drivers(void)
> {
> - __maybe_unused efi_handle_t handle;
> + __maybe_unused efi_handle_t handle = NULL;
> efi_status_t ret = EFI_SUCCESS;
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> &handle, &efi_guid_firmware_management_protocol,
> &efi_fmp_fit, NULL));
> - }
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &efi_root,
> + &handle,
> &efi_guid_firmware_management_protocol,
> &efi_fmp_raw, NULL));
> - }
>
> return ret;
> }
> @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
>
> index = get_last_capsule();
>
> - /* Load capsule drivers */
> - ret = arch_efi_load_capsule_drivers();
> - if (ret != EFI_SUCCESS)
> - return ret;
>
> /*
> * Find capsules on disk.
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 3c5cf9a4357e..0106efdc161b 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> if (ret != EFI_SUCCESS)
> goto out;
>
> + ret = efi_load_capsule_drivers();
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> ret = efi_gop_register();
> if (ret != EFI_SUCCESS)
> --
> 2.32.0.rc0
>
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-14 15:10 [PATCH] efi_loader: FMP cleanups Ilias Apalodimas
2021-06-14 15:13 ` Ilias Apalodimas
2021-06-15 0:49 ` Masami Hiramatsu
@ 2021-06-15 1:51 ` AKASHI Takahiro
2021-06-15 3:55 ` Ilias Apalodimas
2 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2021-06-15 1:51 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, masami.hiramatsu, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, u-boot
Ilias,
In this patch, you are trying to address a couple of independent
issues in a single commit.
Please split.
(Heinrich doesn't like that.)
On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> the same time. Moreover we only install those if a CapsuleUpdate is
> requested. Since we now have an ESRT table, it makes more sense to
> unconditionally install the FMP, so any userspace applications (e.g fwupd)
> can make use of them and trigger an update.
>
> While at it clean up the FMP installation as well. Chapter 23 of the EFI
> spec (rev 2.9) says:
> "A specific updatable hardware firmware store must be represented by
> exactly one FMP instance".
> This is not the case for us, since both of our FMP protocols can be
> installed at the same time and are controlled by a single 'dfu_alt_info'
> env variable.
> So make the config option a choice and allow the user to install one
> of them at any given time.
I'd like to say nak in some respects:
- Although I do understand the UEFI requirement that you mentioned above,
FIT and RAW FMP drivers can handle *different* firmware even though
they share the same dfu_alt_info.
We should not impose unnecessary restriction if we manage to have some
workaround to meet the requirement.
(I still believe that the firmware definition for ESRT should exist
elsewhere other than in FMP themselves.)
- We should allow users to add their own FMP drivers and so not call
[arch_]efi_load_capsule_drivers() unconditionally
even if you don't like "__weak" attribute.
- Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
leave some test cases in pytest skipped.
-Takahiro Akashi
> The overall changes show up in fwupd
>
> pre-patch:
> fwupdmgr get-devices
> No detected devices
>
> post-patch (with FIT FMP installed):
> fwupdmgr get-devices
> WARNING: Required efivarfs filesystem was not found
> See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> Unknown Product
> │
> └─Unknown Firmware:
> Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> Current version: 0
> Vendor: DMI:U-Boot
> Update Error: Not updatable as efivarfs was not found
> GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> Device Flags: • Internal device
> • System requires external power source
> • Needs a reboot after installation
> • Device is usable for the duration of the update
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> configs/sandbox64_defconfig | 1 -
> configs/sandbox_defconfig | 1 -
> configs/xilinx_zynqmp_virt_defconfig | 1 -
> include/efi_loader.h | 1 +
> lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> lib/efi_loader/efi_capsule.c | 22 ++++---------
> lib/efi_loader/efi_setup.c | 4 +++
> 7 files changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 9a373bab6fe3..af18b6c7826e 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index bdbf714e2bd9..24313fdfa53d 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> CONFIG_ERRNO_STR=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> CONFIG_EFI_SECURE_BOOT=y
> CONFIG_TEST_FDTDEC=y
> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> index e939b04ef6a5..0c2d1a70a5a1 100644
> --- a/configs/xilinx_zynqmp_virt_defconfig
> +++ b/configs/xilinx_zynqmp_virt_defconfig
> @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> CONFIG_EFI_CAPSULE_ON_DISK=y
> CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0a9c82a257e1..b81180cfda8b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> * - error code otherwise.
> */
> efi_status_t efi_esrt_populate(void);
> +efi_status_t efi_load_capsule_drivers(void);
> #endif /* _EFI_LOADER_H */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 6242caceb7f9..da6f5faf5adb 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> Select this option if you want to enable capsule-based
> firmware update using Firmware Management Protocol.
>
> +choice EFI_CAPSULE_TYPE
> + prompt "Capsule type (RAW/FIT)"
> + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> +
> +config EFI_CAPSULE_FIRMWARE_FIT
> + bool "FMP driver for FIT images"
> + depends on FIT
> + select UPDATE_FIT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for FIT image
> +
> +config EFI_CAPSULE_FIRMWARE_RAW
> + bool "FMP driver for raw images"
> + select DFU_WRITE_ALT
> + select DFU
> + select EFI_CAPSULE_FIRMWARE
> + help
> + Select this option if you want to enable firmware management protocol
> + driver for raw image
> +
> +endchoice
> +
> config EFI_CAPSULE_AUTHENTICATE
> bool "Update Capsule authentication"
> depends on EFI_CAPSULE_FIRMWARE
> @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> Select this option if you want to enable capsule
> authentication
>
> -config EFI_CAPSULE_FIRMWARE_FIT
> - bool "FMP driver for FIT image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - depends on FIT
> - select UPDATE_FIT
> - select DFU
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for FIT image
> -
> -config EFI_CAPSULE_FIRMWARE_RAW
> - bool "FMP driver for raw image"
> - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> - select DFU
> - select DFU_WRITE_ALT
> - select EFI_CAPSULE_FIRMWARE
> - default n
> - help
> - Select this option if you want to enable firmware management protocol
> - driver for raw image
> -
> config EFI_DEVICE_PATH_TO_TEXT
> bool "Device path to text protocol"
> default y
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 9ead0d2c7816..3b4214a8d4cc 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> }
>
> /**
> - * arch_efi_load_capsule_drivers - initialize capsule drivers
> + * efi_load_capsule_drivers - initialize capsule drivers
> *
> - * Architecture or board specific initialization routine
> + * Generic FMP drivers backed by DFU
> *
> * Return: status code
> */
> -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> +efi_status_t efi_load_capsule_drivers(void)
> {
> - __maybe_unused efi_handle_t handle;
> + __maybe_unused efi_handle_t handle = NULL;
> efi_status_t ret = EFI_SUCCESS;
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> &handle, &efi_guid_firmware_management_protocol,
> &efi_fmp_fit, NULL));
> - }
>
> - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> - handle = NULL;
> + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &efi_root,
> + &handle,
> &efi_guid_firmware_management_protocol,
> &efi_fmp_raw, NULL));
> - }
>
> return ret;
> }
> @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
>
> index = get_last_capsule();
>
> - /* Load capsule drivers */
> - ret = arch_efi_load_capsule_drivers();
> - if (ret != EFI_SUCCESS)
> - return ret;
>
> /*
> * Find capsules on disk.
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 3c5cf9a4357e..0106efdc161b 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> if (ret != EFI_SUCCESS)
> goto out;
>
> + ret = efi_load_capsule_drivers();
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> ret = efi_gop_register();
> if (ret != EFI_SUCCESS)
> --
> 2.32.0.rc0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 1:51 ` AKASHI Takahiro
@ 2021-06-15 3:55 ` Ilias Apalodimas
2021-06-15 4:44 ` AKASHI Takahiro
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 3:55 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, masami.hiramatsu, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
Akashi-san,
On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> Ilias,
>
> In this patch, you are trying to address a couple of independent
> issues in a single commit.
> Please split.
> (Heinrich doesn't like that.)
>
> On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > the same time. Moreover we only install those if a CapsuleUpdate is
> > requested. Since we now have an ESRT table, it makes more sense to
> > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > can make use of them and trigger an update.
> >
> > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > spec (rev 2.9) says:
> > "A specific updatable hardware firmware store must be represented by
> > exactly one FMP instance".
> > This is not the case for us, since both of our FMP protocols can be
> > installed at the same time and are controlled by a single 'dfu_alt_info'
> > env variable.
> > So make the config option a choice and allow the user to install one
> > of them at any given time.
>
> I'd like to say nak in some respects:
> - Although I do understand the UEFI requirement that you mentioned above,
> FIT and RAW FMP drivers can handle *different* firmware even though
> they share the same dfu_alt_info.
How ?
> We should not impose unnecessary restriction if we manage to have some
> workaround to meet the requirement.
It's not the updating part only. It's that the .get_image_info also relies on
the same env variable. Specifically in the fwupd case on an RPI4 with the
dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
populated only one was reported. Probably because this really does give you
2 ways of updating the same flash.
> (I still believe that the firmware definition for ESRT should exist
> elsewhere other than in FMP themselves.)
That's a whole different story, and if we have that, then .get_image_info
should change as well instead of using the DFU information. Because right
now we enabled security (or think we have), while allowing users to set an env
variable which is not authenticated, and completely change what the
firmware reports (or updates). We can always add a huge warning saying
something along the lines of "If you really care this should come with a
CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
The spec is pretty clear and we allow users to *break* it with a config
option. Arguably this is fine, since the code continues to work fine and
you can perform the updates, but in essence RAW and FITs are used to update
the same medium right now. You can't have a capsule with half it's contents
describing something RAW and the other half being a FIT. You have a FIT based
capsule or a RAW based capsule.
> - We should allow users to add their own FMP drivers and so not call
> [arch_]efi_load_capsule_drivers() unconditionally
> even if you don't like "__weak" attribute.
I am fine with the __weak attribute. On the other hand I consider the
current code the defacto way users would use to update their firmware. That's
why I removed the __weak attribute. If a hardware vendor was to update
their special PCI option ROM or a flash that lives on the secure world they
should install their FMPs on a different handle and leave the current code
as is.
> - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> leave some test cases in pytest skipped.
Yea that's unfortunate, but maybe we can just add an extra config on the
sandbox?
Thanks
/Ilias
>
> -Takahiro Akashi
>
> > The overall changes show up in fwupd
> >
> > pre-patch:
> > fwupdmgr get-devices
> > No detected devices
> >
> > post-patch (with FIT FMP installed):
> > fwupdmgr get-devices
> > WARNING: Required efivarfs filesystem was not found
> > See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> > Unknown Product
> > │
> > └─Unknown Firmware:
> > Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> > Current version: 0
> > Vendor: DMI:U-Boot
> > Update Error: Not updatable as efivarfs was not found
> > GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> > 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> > 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> > Device Flags: • Internal device
> > • System requires external power source
> > • Needs a reboot after installation
> > • Device is usable for the duration of the update
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > configs/sandbox64_defconfig | 1 -
> > configs/sandbox_defconfig | 1 -
> > configs/xilinx_zynqmp_virt_defconfig | 1 -
> > include/efi_loader.h | 1 +
> > lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> > lib/efi_loader/efi_capsule.c | 22 ++++---------
> > lib/efi_loader/efi_setup.c | 4 +++
> > 7 files changed, 37 insertions(+), 41 deletions(-)
> >
> > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > index 9a373bab6fe3..af18b6c7826e 100644
> > --- a/configs/sandbox64_defconfig
> > +++ b/configs/sandbox64_defconfig
> > @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> > CONFIG_ERRNO_STR=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > CONFIG_EFI_SECURE_BOOT=y
> > CONFIG_TEST_FDTDEC=y
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index bdbf714e2bd9..24313fdfa53d 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> > CONFIG_ERRNO_STR=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > CONFIG_EFI_SECURE_BOOT=y
> > CONFIG_TEST_FDTDEC=y
> > diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> > index e939b04ef6a5..0c2d1a70a5a1 100644
> > --- a/configs/xilinx_zynqmp_virt_defconfig
> > +++ b/configs/xilinx_zynqmp_virt_defconfig
> > @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 0a9c82a257e1..b81180cfda8b 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> > * - error code otherwise.
> > */
> > efi_status_t efi_esrt_populate(void);
> > +efi_status_t efi_load_capsule_drivers(void);
> > #endif /* _EFI_LOADER_H */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 6242caceb7f9..da6f5faf5adb 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > Select this option if you want to enable capsule-based
> > firmware update using Firmware Management Protocol.
> >
> > +choice EFI_CAPSULE_TYPE
> > + prompt "Capsule type (RAW/FIT)"
> > + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > +
> > +config EFI_CAPSULE_FIRMWARE_FIT
> > + bool "FMP driver for FIT images"
> > + depends on FIT
> > + select UPDATE_FIT
> > + select DFU
> > + select EFI_CAPSULE_FIRMWARE
> > + help
> > + Select this option if you want to enable firmware management protocol
> > + driver for FIT image
> > +
> > +config EFI_CAPSULE_FIRMWARE_RAW
> > + bool "FMP driver for raw images"
> > + select DFU_WRITE_ALT
> > + select DFU
> > + select EFI_CAPSULE_FIRMWARE
> > + help
> > + Select this option if you want to enable firmware management protocol
> > + driver for raw image
> > +
> > +endchoice
> > +
> > config EFI_CAPSULE_AUTHENTICATE
> > bool "Update Capsule authentication"
> > depends on EFI_CAPSULE_FIRMWARE
> > @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> > Select this option if you want to enable capsule
> > authentication
> >
> > -config EFI_CAPSULE_FIRMWARE_FIT
> > - bool "FMP driver for FIT image"
> > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > - depends on FIT
> > - select UPDATE_FIT
> > - select DFU
> > - select EFI_CAPSULE_FIRMWARE
> > - default n
> > - help
> > - Select this option if you want to enable firmware management protocol
> > - driver for FIT image
> > -
> > -config EFI_CAPSULE_FIRMWARE_RAW
> > - bool "FMP driver for raw image"
> > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > - select DFU
> > - select DFU_WRITE_ALT
> > - select EFI_CAPSULE_FIRMWARE
> > - default n
> > - help
> > - Select this option if you want to enable firmware management protocol
> > - driver for raw image
> > -
> > config EFI_DEVICE_PATH_TO_TEXT
> > bool "Device path to text protocol"
> > default y
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 9ead0d2c7816..3b4214a8d4cc 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> > }
> >
> > /**
> > - * arch_efi_load_capsule_drivers - initialize capsule drivers
> > + * efi_load_capsule_drivers - initialize capsule drivers
> > *
> > - * Architecture or board specific initialization routine
> > + * Generic FMP drivers backed by DFU
> > *
> > * Return: status code
> > */
> > -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> > +efi_status_t efi_load_capsule_drivers(void)
> > {
> > - __maybe_unused efi_handle_t handle;
> > + __maybe_unused efi_handle_t handle = NULL;
> > efi_status_t ret = EFI_SUCCESS;
> >
> > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> > - handle = NULL;
> > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > &handle, &efi_guid_firmware_management_protocol,
> > &efi_fmp_fit, NULL));
> > - }
> >
> > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> > - handle = NULL;
> > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > - &efi_root,
> > + &handle,
> > &efi_guid_firmware_management_protocol,
> > &efi_fmp_raw, NULL));
> > - }
> >
> > return ret;
> > }
> > @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
> >
> > index = get_last_capsule();
> >
> > - /* Load capsule drivers */
> > - ret = arch_efi_load_capsule_drivers();
> > - if (ret != EFI_SUCCESS)
> > - return ret;
> >
> > /*
> > * Find capsules on disk.
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 3c5cf9a4357e..0106efdc161b 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> > if (ret != EFI_SUCCESS)
> > goto out;
> >
> > + ret = efi_load_capsule_drivers();
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> > ret = efi_gop_register();
> > if (ret != EFI_SUCCESS)
> > --
> > 2.32.0.rc0
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 0:49 ` Masami Hiramatsu
@ 2021-06-15 3:56 ` Ilias Apalodimas
0 siblings, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 3:56 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Heinrich Schuchardt, Takahiro Akashi, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, U-Boot Mailing List
On Tue, Jun 15, 2021 at 09:49:58AM +0900, Masami Hiramatsu wrote:
> 2021年6月15日(火) 0:10 Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>
> >
> > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > the same time. Moreover we only install those if a CapsuleUpdate is
> > requested. Since we now have an ESRT table, it makes more sense to
> > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > can make use of them and trigger an update.
> >
> > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > spec (rev 2.9) says:
> > "A specific updatable hardware firmware store must be represented by
> > exactly one FMP instance".
> > This is not the case for us, since both of our FMP protocols can be
> > installed at the same time and are controlled by a single 'dfu_alt_info'
> > env variable.
> > So make the config option a choice and allow the user to install one
> > of them at any given time.
> >
> > The overall changes show up in fwupd
> >
> > pre-patch:
> > fwupdmgr get-devices
> > No detected devices
> >
> > post-patch (with FIT FMP installed):
> > fwupdmgr get-devices
> > WARNING: Required efivarfs filesystem was not found
> > See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> > Unknown Product
> > │
> > └─Unknown Firmware:
> > Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> > Current version: 0
> > Vendor: DMI:U-Boot
> > Update Error: Not updatable as efivarfs was not found
> > GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> > 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> > 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> > Device Flags: • Internal device
> > • System requires external power source
> > • Needs a reboot after installation
> > • Device is usable for the duration of the update
> >
>
> This looks good to me, and this covers one patch which I sent before.
> https://lists.denx.de/pipermail/u-boot/2021-June/451401.html
>
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Thanks Masami. I'll send a v2 regardless since I am missing an IS_ENABLED
option when trying to install the FMPs
Cheers
/Ilias
>
> Thank you!
>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > configs/sandbox64_defconfig | 1 -
> > configs/sandbox_defconfig | 1 -
> > configs/xilinx_zynqmp_virt_defconfig | 1 -
> > include/efi_loader.h | 1 +
> > lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> > lib/efi_loader/efi_capsule.c | 22 ++++---------
> > lib/efi_loader/efi_setup.c | 4 +++
> > 7 files changed, 37 insertions(+), 41 deletions(-)
> >
> > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > index 9a373bab6fe3..af18b6c7826e 100644
> > --- a/configs/sandbox64_defconfig
> > +++ b/configs/sandbox64_defconfig
> > @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> > CONFIG_ERRNO_STR=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > CONFIG_EFI_SECURE_BOOT=y
> > CONFIG_TEST_FDTDEC=y
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index bdbf714e2bd9..24313fdfa53d 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> > CONFIG_ERRNO_STR=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > CONFIG_EFI_SECURE_BOOT=y
> > CONFIG_TEST_FDTDEC=y
> > diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> > index e939b04ef6a5..0c2d1a70a5a1 100644
> > --- a/configs/xilinx_zynqmp_virt_defconfig
> > +++ b/configs/xilinx_zynqmp_virt_defconfig
> > @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 0a9c82a257e1..b81180cfda8b 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> > * - error code otherwise.
> > */
> > efi_status_t efi_esrt_populate(void);
> > +efi_status_t efi_load_capsule_drivers(void);
> > #endif /* _EFI_LOADER_H */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 6242caceb7f9..da6f5faf5adb 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > Select this option if you want to enable capsule-based
> > firmware update using Firmware Management Protocol.
> >
> > +choice EFI_CAPSULE_TYPE
> > + prompt "Capsule type (RAW/FIT)"
> > + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > +
> > +config EFI_CAPSULE_FIRMWARE_FIT
> > + bool "FMP driver for FIT images"
> > + depends on FIT
> > + select UPDATE_FIT
> > + select DFU
> > + select EFI_CAPSULE_FIRMWARE
> > + help
> > + Select this option if you want to enable firmware management protocol
> > + driver for FIT image
> > +
> > +config EFI_CAPSULE_FIRMWARE_RAW
> > + bool "FMP driver for raw images"
> > + select DFU_WRITE_ALT
> > + select DFU
> > + select EFI_CAPSULE_FIRMWARE
> > + help
> > + Select this option if you want to enable firmware management protocol
> > + driver for raw image
> > +
> > +endchoice
> > +
> > config EFI_CAPSULE_AUTHENTICATE
> > bool "Update Capsule authentication"
> > depends on EFI_CAPSULE_FIRMWARE
> > @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> > Select this option if you want to enable capsule
> > authentication
> >
> > -config EFI_CAPSULE_FIRMWARE_FIT
> > - bool "FMP driver for FIT image"
> > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > - depends on FIT
> > - select UPDATE_FIT
> > - select DFU
> > - select EFI_CAPSULE_FIRMWARE
> > - default n
> > - help
> > - Select this option if you want to enable firmware management protocol
> > - driver for FIT image
> > -
> > -config EFI_CAPSULE_FIRMWARE_RAW
> > - bool "FMP driver for raw image"
> > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > - select DFU
> > - select DFU_WRITE_ALT
> > - select EFI_CAPSULE_FIRMWARE
> > - default n
> > - help
> > - Select this option if you want to enable firmware management protocol
> > - driver for raw image
> > -
> > config EFI_DEVICE_PATH_TO_TEXT
> > bool "Device path to text protocol"
> > default y
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 9ead0d2c7816..3b4214a8d4cc 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> > }
> >
> > /**
> > - * arch_efi_load_capsule_drivers - initialize capsule drivers
> > + * efi_load_capsule_drivers - initialize capsule drivers
> > *
> > - * Architecture or board specific initialization routine
> > + * Generic FMP drivers backed by DFU
> > *
> > * Return: status code
> > */
> > -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> > +efi_status_t efi_load_capsule_drivers(void)
> > {
> > - __maybe_unused efi_handle_t handle;
> > + __maybe_unused efi_handle_t handle = NULL;
> > efi_status_t ret = EFI_SUCCESS;
> >
> > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> > - handle = NULL;
> > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > &handle, &efi_guid_firmware_management_protocol,
> > &efi_fmp_fit, NULL));
> > - }
> >
> > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> > - handle = NULL;
> > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > - &efi_root,
> > + &handle,
> > &efi_guid_firmware_management_protocol,
> > &efi_fmp_raw, NULL));
> > - }
> >
> > return ret;
> > }
> > @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
> >
> > index = get_last_capsule();
> >
> > - /* Load capsule drivers */
> > - ret = arch_efi_load_capsule_drivers();
> > - if (ret != EFI_SUCCESS)
> > - return ret;
> >
> > /*
> > * Find capsules on disk.
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 3c5cf9a4357e..0106efdc161b 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> > if (ret != EFI_SUCCESS)
> > goto out;
> >
> > + ret = efi_load_capsule_drivers();
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> > ret = efi_gop_register();
> > if (ret != EFI_SUCCESS)
> > --
> > 2.32.0.rc0
> >
>
>
> --
> Masami Hiramatsu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 3:55 ` Ilias Apalodimas
@ 2021-06-15 4:44 ` AKASHI Takahiro
2021-06-15 5:23 ` Ilias Apalodimas
0 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2021-06-15 4:44 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, masami.hiramatsu, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> Akashi-san,
>
> On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > Ilias,
> >
> > In this patch, you are trying to address a couple of independent
> > issues in a single commit.
> > Please split.
> > (Heinrich doesn't like that.)
Any comment?
> > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > requested. Since we now have an ESRT table, it makes more sense to
> > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > can make use of them and trigger an update.
> > >
> > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > spec (rev 2.9) says:
> > > "A specific updatable hardware firmware store must be represented by
> > > exactly one FMP instance".
> > > This is not the case for us, since both of our FMP protocols can be
> > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > env variable.
> > > So make the config option a choice and allow the user to install one
> > > of them at any given time.
> >
> > I'd like to say nak in some respects:
> > - Although I do understand the UEFI requirement that you mentioned above,
> > FIT and RAW FMP drivers can handle *different* firmware even though
> > they share the same dfu_alt_info.
>
> How ?
One idea that I can imagine is that we may be able to utilize
"update_image_index", which is currently not used effectively,
in order to specify which firmware in dfu_alt_info be handled
by either FIT FMP or RAW FMP.
> > We should not impose unnecessary restriction if we manage to have some
> > workaround to meet the requirement.
>
> It's not the updating part only. It's that the .get_image_info also relies on
> the same env variable.
The above idea can and should be applied to GetImageInfo implementation
at the same time.
> Specifically in the fwupd case on an RPI4 with the
> dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> populated only one was reported. Probably because this really does give you
> 2 ways of updating the same flash.
>
> > (I still believe that the firmware definition for ESRT should exist
> > elsewhere other than in FMP themselves.)
>
> That's a whole different story, and if we have that, then .get_image_info
> should change as well instead of using the DFU information.
I don't think so as I mentioned above.
> Because right
> now we enabled security (or think we have), while allowing users to set an env
> variable which is not authenticated, and completely change what the
> firmware reports (or updates).
This is the point that I mentioned earlier in our private chat,
and it's a "whole different" story in this context.
> We can always add a huge warning saying
> something along the lines of "If you really care this should come with a
> CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
>
> The spec is pretty clear and we allow users to *break* it with a config
> option. Arguably this is fine, since the code continues to work fine and
> you can perform the updates, but in essence RAW and FITs are used to update
> the same medium right now. You can't have a capsule with half it's contents
> describing something RAW and the other half being a FIT. You have a FIT based
> capsule or a RAW based capsule.
See above.
> > - We should allow users to add their own FMP drivers and so not call
> > [arch_]efi_load_capsule_drivers() unconditionally
> > even if you don't like "__weak" attribute.
>
> I am fine with the __weak attribute. On the other hand I consider the
> current code the defacto way users would use to update their firmware. That's
> why I removed the __weak attribute. If a hardware vendor was to update
> their special PCI option ROM or a flash that lives on the secure world they
> should install their FMPs on a different handle and leave the current code
> as is.
And we should provide an option that disables these existing handle.
> > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > leave some test cases in pytest skipped.
>
> Yea that's unfortunate, but maybe we can just add an extra config on the
> sandbox?
Please add another patch that is missing.
-Takahiro Akashi
> Thanks
> /Ilias
>
> >
> > -Takahiro Akashi
> >
> > > The overall changes show up in fwupd
> > >
> > > pre-patch:
> > > fwupdmgr get-devices
> > > No detected devices
> > >
> > > post-patch (with FIT FMP installed):
> > > fwupdmgr get-devices
> > > WARNING: Required efivarfs filesystem was not found
> > > See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> > > Unknown Product
> > > │
> > > └─Unknown Firmware:
> > > Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> > > Current version: 0
> > > Vendor: DMI:U-Boot
> > > Update Error: Not updatable as efivarfs was not found
> > > GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> > > 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> > > 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> > > Device Flags: • Internal device
> > > • System requires external power source
> > > • Needs a reboot after installation
> > > • Device is usable for the duration of the update
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > configs/sandbox64_defconfig | 1 -
> > > configs/sandbox_defconfig | 1 -
> > > configs/xilinx_zynqmp_virt_defconfig | 1 -
> > > include/efi_loader.h | 1 +
> > > lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> > > lib/efi_loader/efi_capsule.c | 22 ++++---------
> > > lib/efi_loader/efi_setup.c | 4 +++
> > > 7 files changed, 37 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > > index 9a373bab6fe3..af18b6c7826e 100644
> > > --- a/configs/sandbox64_defconfig
> > > +++ b/configs/sandbox64_defconfig
> > > @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> > > CONFIG_ERRNO_STR=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > CONFIG_EFI_SECURE_BOOT=y
> > > CONFIG_TEST_FDTDEC=y
> > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > index bdbf714e2bd9..24313fdfa53d 100644
> > > --- a/configs/sandbox_defconfig
> > > +++ b/configs/sandbox_defconfig
> > > @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> > > CONFIG_ERRNO_STR=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > CONFIG_EFI_SECURE_BOOT=y
> > > CONFIG_TEST_FDTDEC=y
> > > diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> > > index e939b04ef6a5..0c2d1a70a5a1 100644
> > > --- a/configs/xilinx_zynqmp_virt_defconfig
> > > +++ b/configs/xilinx_zynqmp_virt_defconfig
> > > @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 0a9c82a257e1..b81180cfda8b 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> > > * - error code otherwise.
> > > */
> > > efi_status_t efi_esrt_populate(void);
> > > +efi_status_t efi_load_capsule_drivers(void);
> > > #endif /* _EFI_LOADER_H */
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 6242caceb7f9..da6f5faf5adb 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > Select this option if you want to enable capsule-based
> > > firmware update using Firmware Management Protocol.
> > >
> > > +choice EFI_CAPSULE_TYPE
> > > + prompt "Capsule type (RAW/FIT)"
> > > + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > +
> > > +config EFI_CAPSULE_FIRMWARE_FIT
> > > + bool "FMP driver for FIT images"
> > > + depends on FIT
> > > + select UPDATE_FIT
> > > + select DFU
> > > + select EFI_CAPSULE_FIRMWARE
> > > + help
> > > + Select this option if you want to enable firmware management protocol
> > > + driver for FIT image
> > > +
> > > +config EFI_CAPSULE_FIRMWARE_RAW
> > > + bool "FMP driver for raw images"
> > > + select DFU_WRITE_ALT
> > > + select DFU
> > > + select EFI_CAPSULE_FIRMWARE
> > > + help
> > > + Select this option if you want to enable firmware management protocol
> > > + driver for raw image
> > > +
> > > +endchoice
> > > +
> > > config EFI_CAPSULE_AUTHENTICATE
> > > bool "Update Capsule authentication"
> > > depends on EFI_CAPSULE_FIRMWARE
> > > @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> > > Select this option if you want to enable capsule
> > > authentication
> > >
> > > -config EFI_CAPSULE_FIRMWARE_FIT
> > > - bool "FMP driver for FIT image"
> > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > - depends on FIT
> > > - select UPDATE_FIT
> > > - select DFU
> > > - select EFI_CAPSULE_FIRMWARE
> > > - default n
> > > - help
> > > - Select this option if you want to enable firmware management protocol
> > > - driver for FIT image
> > > -
> > > -config EFI_CAPSULE_FIRMWARE_RAW
> > > - bool "FMP driver for raw image"
> > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > - select DFU
> > > - select DFU_WRITE_ALT
> > > - select EFI_CAPSULE_FIRMWARE
> > > - default n
> > > - help
> > > - Select this option if you want to enable firmware management protocol
> > > - driver for raw image
> > > -
> > > config EFI_DEVICE_PATH_TO_TEXT
> > > bool "Device path to text protocol"
> > > default y
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 9ead0d2c7816..3b4214a8d4cc 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> > > }
> > >
> > > /**
> > > - * arch_efi_load_capsule_drivers - initialize capsule drivers
> > > + * efi_load_capsule_drivers - initialize capsule drivers
> > > *
> > > - * Architecture or board specific initialization routine
> > > + * Generic FMP drivers backed by DFU
> > > *
> > > * Return: status code
> > > */
> > > -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> > > +efi_status_t efi_load_capsule_drivers(void)
> > > {
> > > - __maybe_unused efi_handle_t handle;
> > > + __maybe_unused efi_handle_t handle = NULL;
> > > efi_status_t ret = EFI_SUCCESS;
> > >
> > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> > > - handle = NULL;
> > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > > &handle, &efi_guid_firmware_management_protocol,
> > > &efi_fmp_fit, NULL));
> > > - }
> > >
> > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> > > - handle = NULL;
> > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > > - &efi_root,
> > > + &handle,
> > > &efi_guid_firmware_management_protocol,
> > > &efi_fmp_raw, NULL));
> > > - }
> > >
> > > return ret;
> > > }
> > > @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
> > >
> > > index = get_last_capsule();
> > >
> > > - /* Load capsule drivers */
> > > - ret = arch_efi_load_capsule_drivers();
> > > - if (ret != EFI_SUCCESS)
> > > - return ret;
> > >
> > > /*
> > > * Find capsules on disk.
> > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > index 3c5cf9a4357e..0106efdc161b 100644
> > > --- a/lib/efi_loader/efi_setup.c
> > > +++ b/lib/efi_loader/efi_setup.c
> > > @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> > > if (ret != EFI_SUCCESS)
> > > goto out;
> > >
> > > + ret = efi_load_capsule_drivers();
> > > + if (ret != EFI_SUCCESS)
> > > + goto out;
> > > +
> > > #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> > > ret = efi_gop_register();
> > > if (ret != EFI_SUCCESS)
> > > --
> > > 2.32.0.rc0
> > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 4:44 ` AKASHI Takahiro
@ 2021-06-15 5:23 ` Ilias Apalodimas
2021-06-15 5:55 ` AKASHI Takahiro
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 5:23 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, masami.hiramatsu, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 01:44:58PM +0900, AKASHI Takahiro wrote:
> On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> > Akashi-san,
> >
> > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > > Ilias,
> > >
> > > In this patch, you are trying to address a couple of independent
> > > issues in a single commit.
> > > Please split.
> > > (Heinrich doesn't like that.)
>
> Any comment?
They are fixing the ESRT table generation, while cleaning up what's already in
there. Besides Heinrich can comment himself if he wants them split or not.
>
> > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > > requested. Since we now have an ESRT table, it makes more sense to
> > > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > > can make use of them and trigger an update.
> > > >
> > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > > spec (rev 2.9) says:
> > > > "A specific updatable hardware firmware store must be represented by
> > > > exactly one FMP instance".
> > > > This is not the case for us, since both of our FMP protocols can be
> > > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > > env variable.
> > > > So make the config option a choice and allow the user to install one
> > > > of them at any given time.
> > >
> > > I'd like to say nak in some respects:
> > > - Although I do understand the UEFI requirement that you mentioned above,
> > > FIT and RAW FMP drivers can handle *different* firmware even though
> > > they share the same dfu_alt_info.
> >
> > How ?
>
> One idea that I can imagine is that we may be able to utilize
> "update_image_index", which is currently not used effectively,
> in order to specify which firmware in dfu_alt_info be handled
> by either FIT FMP or RAW FMP.
So it's not being used right now, and the fact is they are at the moment doing
the same thing. And even if it does, no one in his right mind will create a
platform and say "Hey let me create half of the capsules as raw and the rest
of them as FIT, it would be fun to watch users struggle".
Is there anything very specific that you can achieve with FIT capsules that
you can't achieve with RAW ones (or vice versa), that would justify having
them both present at the same time?
>
> > > We should not impose unnecessary restriction if we manage to have some
> > > workaround to meet the requirement.
> >
> > It's not the updating part only. It's that the .get_image_info also relies on
> > the same env variable.
>
> The above idea can and should be applied to GetImageInfo implementation
> at the same time.
Yes but can you do it with just changing the env variable now? Or you need to
add more code into the DFU logic?
>
> > Specifically in the fwupd case on an RPI4 with the
> > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> > populated only one was reported. Probably because this really does give you
> > 2 ways of updating the same flash.
> >
> > > (I still believe that the firmware definition for ESRT should exist
> > > elsewhere other than in FMP themselves.)
> >
> > That's a whole different story, and if we have that, then .get_image_info
> > should change as well instead of using the DFU information.
>
> I don't think so as I mentioned above.
And I don't see any benefit from storing the same information in 2 completely
disjoint entities.
>
> > Because right
> > now we enabled security (or think we have), while allowing users to set an env
> > variable which is not authenticated, and completely change what the
> > firmware reports (or updates).
>
> This is the point that I mentioned earlier in our private chat,
> and it's a "whole different" story in this context.
You mentioned that in the context of "can we install the FMPs during the EFI
init". Since the variable is interpreted at runtime, we definitely can. I
looked back at that chat and saw nothing related to the security problems
we'll create.
In any case the problem here is real, but there are sane ways to avoid it.
>
> > We can always add a huge warning saying
> > something along the lines of "If you really care this should come with a
> > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
> >
> > The spec is pretty clear and we allow users to *break* it with a config
> > option. Arguably this is fine, since the code continues to work fine and
> > you can perform the updates, but in essence RAW and FITs are used to update
> > the same medium right now. You can't have a capsule with half it's contents
> > describing something RAW and the other half being a FIT. You have a FIT based
> > capsule or a RAW based capsule.
>
> See above.
I still don't get it.
The fact is we have a config option, that if the user decides to set in a
specific way (and that specific way is 99% of the use cases) we'll break the
EFI spec. So unless we add code into the dfu logic, parsing dfu_alt_info and
figuring out if the user is allowed to do that or not, I really think those two
must be treated as mutually exclusive.
>
> > > - We should allow users to add their own FMP drivers and so not call
> > > [arch_]efi_load_capsule_drivers() unconditionally
> > > even if you don't like "__weak" attribute.
> >
> > I am fine with the __weak attribute. On the other hand I consider the
> > current code the defacto way users would use to update their firmware. That's
> > why I removed the __weak attribute. If a hardware vendor was to update
> > their special PCI option ROM or a flash that lives on the secure world they
> > should install their FMPs on a different handle and leave the current code
> > as is.
>
> And we should provide an option that disables these existing handle.
The existing one is not enough?
>
> > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > > leave some test cases in pytest skipped.
> >
> > Yea that's unfortunate, but maybe we can just add an extra config on the
> > sandbox?
>
> Please add another patch that is missing.
>
> -Takahiro Akashi
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 5:23 ` Ilias Apalodimas
@ 2021-06-15 5:55 ` AKASHI Takahiro
2021-06-15 6:22 ` Ilias Apalodimas
0 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2021-06-15 5:55 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, masami.hiramatsu, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 08:23:35AM +0300, Ilias Apalodimas wrote:
> On Tue, Jun 15, 2021 at 01:44:58PM +0900, AKASHI Takahiro wrote:
> > On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> > > Akashi-san,
> > >
> > > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > > > Ilias,
> > > >
> > > > In this patch, you are trying to address a couple of independent
> > > > issues in a single commit.
> > > > Please split.
> > > > (Heinrich doesn't like that.)
> >
> > Any comment?
>
> They are fixing the ESRT table generation, while cleaning up what's already in
> there. Besides Heinrich can comment himself if he wants them split or not.
They are fixing "different" problems relating ESRT generation.
That is my point.
>
> >
> > > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > > > requested. Since we now have an ESRT table, it makes more sense to
> > > > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > > > can make use of them and trigger an update.
> > > > >
> > > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > > > spec (rev 2.9) says:
> > > > > "A specific updatable hardware firmware store must be represented by
> > > > > exactly one FMP instance".
> > > > > This is not the case for us, since both of our FMP protocols can be
> > > > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > > > env variable.
> > > > > So make the config option a choice and allow the user to install one
> > > > > of them at any given time.
> > > >
> > > > I'd like to say nak in some respects:
> > > > - Although I do understand the UEFI requirement that you mentioned above,
> > > > FIT and RAW FMP drivers can handle *different* firmware even though
> > > > they share the same dfu_alt_info.
> > >
> > > How ?
> >
> > One idea that I can imagine is that we may be able to utilize
> > "update_image_index", which is currently not used effectively,
> > in order to specify which firmware in dfu_alt_info be handled
> > by either FIT FMP or RAW FMP.
>
> So it's not being used right now, and the fact is they are at the moment doing
> the same thing. And even if it does, no one in his right mind will create a
> platform and say "Hey let me create half of the capsules as raw and the rest
> of them as FIT, it would be fun to watch users struggle".
You misunderstand me.
Because you asked me about an idea about how to fix the issue,
I answered to it. I have never said that the current code does not
have a problem that you mentioned.
So I said:
> > > > We should not impose unnecessary restriction if we manage to have some
> > > > workaround to meet the requirement.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Is there anything very specific that you can achieve with FIT capsules that
> you can't achieve with RAW ones (or vice versa), that would justify having
> them both present at the same time?
Yes.
We may have different *firmware* for different software components
and different devices. For example,
You have firmare like U-Boot binary and default variable storage
in different partitions.
On the other hand, you have an extra firmware for a particular
peripheral, like PCI device or anything else, which comes
from a 3rd party vendor of the device.
The former may and can be packed into a single binary in FIT format.
The latter can be used in a separate RAW format as the timing of
updating those firmware is likely to be different.
> >
> > > > We should not impose unnecessary restriction if we manage to have some
> > > > workaround to meet the requirement.
> > >
> > > It's not the updating part only. It's that the .get_image_info also relies on
> > > the same env variable.
> >
> > The above idea can and should be applied to GetImageInfo implementation
> > at the same time.
>
> Yes but can you do it with just changing the env variable now? Or you need to
> add more code into the DFU logic?
Those *meta* data for firmware can be declared/specified outside of FMP,
and be referred to by FMP (and/or ESRT). That is what I meant by:
> > > > (I still believe that the firmware definition for ESRT should exist
> > > > elsewhere other than in FMP themselves.)
> >
> > > Specifically in the fwupd case on an RPI4 with the
> > > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> > > populated only one was reported. Probably because this really does give you
> > > 2 ways of updating the same flash.
> > >
> > > > (I still believe that the firmware definition for ESRT should exist
> > > > elsewhere other than in FMP themselves.)
> > >
> > > That's a whole different story, and if we have that, then .get_image_info
> > > should change as well instead of using the DFU information.
> >
> > I don't think so as I mentioned above.
>
> And I don't see any benefit from storing the same information in 2 completely
> disjoint entities.
?
> >
> > > Because right
> > > now we enabled security (or think we have), while allowing users to set an env
> > > variable which is not authenticated, and completely change what the
> > > firmware reports (or updates).
> >
> > This is the point that I mentioned earlier in our private chat,
> > and it's a "whole different" story in this context.
>
> You mentioned that in the context of "can we install the FMPs during the EFI
> init". Since the variable is interpreted at runtime, we definitely can. I
> looked back at that chat and saw nothing related to the security problems
> we'll create.
You have referred to this issue in the context of security.
So I said that it was a different story.
The issue that you're trying to address in this patch is *NOT* security.
> In any case the problem here is real, but there are sane ways to avoid it.
>
> >
> > > We can always add a huge warning saying
> > > something along the lines of "If you really care this should come with a
> > > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
> > >
> > > The spec is pretty clear and we allow users to *break* it with a config
> > > option. Arguably this is fine, since the code continues to work fine and
> > > you can perform the updates, but in essence RAW and FITs are used to update
> > > the same medium right now. You can't have a capsule with half it's contents
> > > describing something RAW and the other half being a FIT. You have a FIT based
> > > capsule or a RAW based capsule.
> >
> > See above.
>
> I still don't get it.
> The fact is we have a config option, that if the user decides to set in a
> specific way (and that specific way is 99% of the use cases) we'll break the
> EFI spec.
As I said above, I have never said that the current implementation does
not break EFI spec if not properly used.
So I suggested a possible solution in the previous email as you asked me.
> So unless we add code into the dfu logic, parsing dfu_alt_info and
> figuring out if the user is allowed to do that or not, I really think those two
> must be treated as mutually exclusive.
I don't think that we need to modify DFU code.
-Takahiro Akashi
> >
> > > > - We should allow users to add their own FMP drivers and so not call
> > > > [arch_]efi_load_capsule_drivers() unconditionally
> > > > even if you don't like "__weak" attribute.
> > >
> > > I am fine with the __weak attribute. On the other hand I consider the
> > > current code the defacto way users would use to update their firmware. That's
> > > why I removed the __weak attribute. If a hardware vendor was to update
> > > their special PCI option ROM or a flash that lives on the secure world they
> > > should install their FMPs on a different handle and leave the current code
> > > as is.
> >
> > And we should provide an option that disables these existing handle.
>
> The existing one is not enough?
>
> >
> > > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > > > leave some test cases in pytest skipped.
> > >
> > > Yea that's unfortunate, but maybe we can just add an extra config on the
> > > sandbox?
> >
> > Please add another patch that is missing.
>
>
> >
> > -Takahiro Akashi
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 5:55 ` AKASHI Takahiro
@ 2021-06-15 6:22 ` Ilias Apalodimas
2021-06-15 6:40 ` AKASHI Takahiro
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 6:22 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, masami.hiramatsu, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 02:55:38PM +0900, AKASHI Takahiro wrote:
> On Tue, Jun 15, 2021 at 08:23:35AM +0300, Ilias Apalodimas wrote:
> > On Tue, Jun 15, 2021 at 01:44:58PM +0900, AKASHI Takahiro wrote:
> > > On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> > > > Akashi-san,
> > > >
> > > > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > > > > Ilias,
> > > > >
> > > > > In this patch, you are trying to address a couple of independent
> > > > > issues in a single commit.
> > > > > Please split.
> > > > > (Heinrich doesn't like that.)
> > >
> > > Any comment?
> >
> > They are fixing the ESRT table generation, while cleaning up what's already in
> > there. Besides Heinrich can comment himself if he wants them split or not.
>
> They are fixing "different" problems relating ESRT generation.
> That is my point.
>
Sure, but it's a minor clean up really. As I said the current code works
fine. So I dont really mind the fact that it breaks a sentence of the spec.
Hence I considered the cleanup and the mutual exclusive part to be really
minor.
> >
> > >
> > > > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > > > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > > > > requested. Since we now have an ESRT table, it makes more sense to
> > > > > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > > > > can make use of them and trigger an update.
> > > > > >
> > > > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > > > > spec (rev 2.9) says:
> > > > > > "A specific updatable hardware firmware store must be represented by
> > > > > > exactly one FMP instance".
> > > > > > This is not the case for us, since both of our FMP protocols can be
> > > > > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > > > > env variable.
> > > > > > So make the config option a choice and allow the user to install one
> > > > > > of them at any given time.
> > > > >
> > > > > I'd like to say nak in some respects:
> > > > > - Although I do understand the UEFI requirement that you mentioned above,
> > > > > FIT and RAW FMP drivers can handle *different* firmware even though
> > > > > they share the same dfu_alt_info.
> > > >
> > > > How ?
> > >
> > > One idea that I can imagine is that we may be able to utilize
> > > "update_image_index", which is currently not used effectively,
> > > in order to specify which firmware in dfu_alt_info be handled
> > > by either FIT FMP or RAW FMP.
> >
> > So it's not being used right now, and the fact is they are at the moment doing
> > the same thing. And even if it does, no one in his right mind will create a
> > platform and say "Hey let me create half of the capsules as raw and the rest
> > of them as FIT, it would be fun to watch users struggle".
>
> You misunderstand me.
> Because you asked me about an idea about how to fix the issue,
> I answered to it. I have never said that the current code does not
> have a problem that you mentioned.
> So I said:
> > > > > We should not impose unnecessary restriction if we manage to have some
> > > > > workaround to meet the requirement.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I was mostly asking for existing code that I might have missed in my greps,
but I get the idea.
>
> > Is there anything very specific that you can achieve with FIT capsules that
> > you can't achieve with RAW ones (or vice versa), that would justify having
> > them both present at the same time?
>
> Yes.
> We may have different *firmware* for different software components
> and different devices. For example,
> You have firmare like U-Boot binary and default variable storage
> in different partitions.
> On the other hand, you have an extra firmware for a particular
> peripheral, like PCI device or anything else, which comes
> from a 3rd party vendor of the device.
> The former may and can be packed into a single binary in FIT format.
> The latter can be used in a separate RAW format as the timing of
> updating those firmware is likely to be different.
>
Sure that's a use case. But that's not a specific one, nor something you cant
do without both of them being installed. You can arguably just create a RAW
image for the second firmware and put the info into dfu_alt_info. So unless we
have an example of a device that says "This firmware file can only be updated
by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
see any reason why we need to support that. The changes are not set in stone
anyway. The code was fine before the ESRT got involved. So all my patch
really does is make the current code useful when an ESRT is installed. We can
then break the spec on purpose (yes break it :>) ignore the OsIndications
bit and have fwupd working with U-Boot. This will have an actual impact on
devices and the code usability, since people will start using it. I prefer
this over adding a very cumbersome corner case, that's arguably no one will
ever need.
We can always go back and make them a config option in the future. But unless
we get a use case for it, I'd still prefer having them mutually exclusive,
rather than adding code for an imaginary device (which I really doubt anyone
will ever design).
> > >
> > > > > We should not impose unnecessary restriction if we manage to have some
> > > > > workaround to meet the requirement.
> > > >
> > > > It's not the updating part only. It's that the .get_image_info also relies on
> > > > the same env variable.
> > >
> > > The above idea can and should be applied to GetImageInfo implementation
> > > at the same time.
> >
> > Yes but can you do it with just changing the env variable now? Or you need to
> > add more code into the DFU logic?
>
> Those *meta* data for firmware can be declared/specified outside of FMP,
> and be referred to by FMP (and/or ESRT). That is what I meant by:
> > > > > (I still believe that the firmware definition for ESRT should exist
> > > > > elsewhere other than in FMP themselves.)
>
>
> > >
> > > > Specifically in the fwupd case on an RPI4 with the
> > > > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> > > > populated only one was reported. Probably because this really does give you
> > > > 2 ways of updating the same flash.
> > > >
> > > > > (I still believe that the firmware definition for ESRT should exist
> > > > > elsewhere other than in FMP themselves.)
> > > >
> > > > That's a whole different story, and if we have that, then .get_image_info
> > > > should change as well instead of using the DFU information.
> > >
> > > I don't think so as I mentioned above.
> >
> > And I don't see any benefit from storing the same information in 2 completely
> > disjoint entities.
>
> ?
The ESRT code right now uses get_image_info from the FMP code and the FMP code
uses the dfu_alt_info to derive whatever information it needs. Both of these
concepts are trying to provide information about the running firmware. So if
we change that imho both of them should get that info from an abstracted
object (file/c struct in u-boot/whatever). But really I think using FMP to
fill ESRT entries is fine (at least for me).
>
> > >
> > > > Because right
> > > > now we enabled security (or think we have), while allowing users to set an env
> > > > variable which is not authenticated, and completely change what the
> > > > firmware reports (or updates).
> > >
> > > This is the point that I mentioned earlier in our private chat,
> > > and it's a "whole different" story in this context.
> >
> > You mentioned that in the context of "can we install the FMPs during the EFI
> > init". Since the variable is interpreted at runtime, we definitely can. I
> > looked back at that chat and saw nothing related to the security problems
> > we'll create.
>
> You have referred to this issue in the context of security.
> So I said that it was a different story.
> The issue that you're trying to address in this patch is *NOT* security.
>
No it's not, I am just pointing out the obvious.
> > In any case the problem here is real, but there are sane ways to avoid it.
> >
> > >
> > > > We can always add a huge warning saying
> > > > something along the lines of "If you really care this should come with a
> > > > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
> > > >
> > > > The spec is pretty clear and we allow users to *break* it with a config
> > > > option. Arguably this is fine, since the code continues to work fine and
> > > > you can perform the updates, but in essence RAW and FITs are used to update
> > > > the same medium right now. You can't have a capsule with half it's contents
> > > > describing something RAW and the other half being a FIT. You have a FIT based
> > > > capsule or a RAW based capsule.
> > >
> > > See above.
> >
> > I still don't get it.
> > The fact is we have a config option, that if the user decides to set in a
> > specific way (and that specific way is 99% of the use cases) we'll break the
> > EFI spec.
>
> As I said above, I have never said that the current implementation does
> not break EFI spec if not properly used.
> So I suggested a possible solution in the previous email as you asked me.
>
> > So unless we add code into the dfu logic, parsing dfu_alt_info and
> > figuring out if the user is allowed to do that or not, I really think those two
> > must be treated as mutually exclusive.
>
> I don't think that we need to modify DFU code.
Yea me neither, but since the firmware runtime information are derived from
that, we don't have that many options.
Thanks
/Ilias
>
> -Takahiro Akashi
>
> > >
> > > > > - We should allow users to add their own FMP drivers and so not call
> > > > > [arch_]efi_load_capsule_drivers() unconditionally
> > > > > even if you don't like "__weak" attribute.
> > > >
> > > > I am fine with the __weak attribute. On the other hand I consider the
> > > > current code the defacto way users would use to update their firmware. That's
> > > > why I removed the __weak attribute. If a hardware vendor was to update
> > > > their special PCI option ROM or a flash that lives on the secure world they
> > > > should install their FMPs on a different handle and leave the current code
> > > > as is.
> > >
> > > And we should provide an option that disables these existing handle.
> >
> > The existing one is not enough?
> >
> > >
> > > > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > > > > leave some test cases in pytest skipped.
> > > >
> > > > Yea that's unfortunate, but maybe we can just add an extra config on the
> > > > sandbox?
> > >
> > > Please add another patch that is missing.
> >
> >
> > >
> > > -Takahiro Akashi
> > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 6:22 ` Ilias Apalodimas
@ 2021-06-15 6:40 ` AKASHI Takahiro
2021-06-15 6:56 ` Ilias Apalodimas
0 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2021-06-15 6:40 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, masami.hiramatsu, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 09:22:31AM +0300, Ilias Apalodimas wrote:
> On Tue, Jun 15, 2021 at 02:55:38PM +0900, AKASHI Takahiro wrote:
> > On Tue, Jun 15, 2021 at 08:23:35AM +0300, Ilias Apalodimas wrote:
> > > On Tue, Jun 15, 2021 at 01:44:58PM +0900, AKASHI Takahiro wrote:
> > > > On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> > > > > Akashi-san,
> > > > >
> > > > > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > > > > > Ilias,
> > > > > >
> > > > > > In this patch, you are trying to address a couple of independent
> > > > > > issues in a single commit.
> > > > > > Please split.
> > > > > > (Heinrich doesn't like that.)
> > > >
> > > > Any comment?
> > >
> > > They are fixing the ESRT table generation, while cleaning up what's already in
> > > there. Besides Heinrich can comment himself if he wants them split or not.
> >
> > They are fixing "different" problems relating ESRT generation.
> > That is my point.
> >
>
> Sure, but it's a minor clean up really. As I said the current code works
> fine. So I dont really mind the fact that it breaks a sentence of the spec.
> Hence I considered the cleanup and the mutual exclusive part to be really
> minor.
Yes, it's minor but still a different problem.
Let me give you an example.
If I correct a misspelling in a given code
very close to the change, Heinrich would ask
me to add a separate patch as it is simply not
related.
Moreover, from the viewpoint of maintenance (i.e. bisect ability),
they should be separated from each other.
> > >
> > > >
> > > > > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > > > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > > > > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > > > > > requested. Since we now have an ESRT table, it makes more sense to
> > > > > > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > > > > > can make use of them and trigger an update.
> > > > > > >
> > > > > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > > > > > spec (rev 2.9) says:
> > > > > > > "A specific updatable hardware firmware store must be represented by
> > > > > > > exactly one FMP instance".
> > > > > > > This is not the case for us, since both of our FMP protocols can be
> > > > > > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > > > > > env variable.
> > > > > > > So make the config option a choice and allow the user to install one
> > > > > > > of them at any given time.
> > > > > >
> > > > > > I'd like to say nak in some respects:
> > > > > > - Although I do understand the UEFI requirement that you mentioned above,
> > > > > > FIT and RAW FMP drivers can handle *different* firmware even though
> > > > > > they share the same dfu_alt_info.
> > > > >
> > > > > How ?
> > > >
> > > > One idea that I can imagine is that we may be able to utilize
> > > > "update_image_index", which is currently not used effectively,
> > > > in order to specify which firmware in dfu_alt_info be handled
> > > > by either FIT FMP or RAW FMP.
> > >
> > > So it's not being used right now, and the fact is they are at the moment doing
> > > the same thing. And even if it does, no one in his right mind will create a
> > > platform and say "Hey let me create half of the capsules as raw and the rest
> > > of them as FIT, it would be fun to watch users struggle".
> >
> > You misunderstand me.
> > Because you asked me about an idea about how to fix the issue,
> > I answered to it. I have never said that the current code does not
> > have a problem that you mentioned.
> > So I said:
> > > > > > We should not impose unnecessary restriction if we manage to have some
> > > > > > workaround to meet the requirement.
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I was mostly asking for existing code that I might have missed in my greps,
> but I get the idea.
>
> >
> > > Is there anything very specific that you can achieve with FIT capsules that
> > > you can't achieve with RAW ones (or vice versa), that would justify having
> > > them both present at the same time?
> >
> > Yes.
> > We may have different *firmware* for different software components
> > and different devices. For example,
> > You have firmare like U-Boot binary and default variable storage
> > in different partitions.
> > On the other hand, you have an extra firmware for a particular
> > peripheral, like PCI device or anything else, which comes
> > from a 3rd party vendor of the device.
> > The former may and can be packed into a single binary in FIT format.
> > The latter can be used in a separate RAW format as the timing of
> > updating those firmware is likely to be different.
> >
>
> Sure that's a use case. But that's not a specific one, nor something you cant
> do without both of them being installed. You can arguably just create a RAW
> image for the second firmware and put the info into dfu_alt_info.
Why do you stick to a single format?
We can reasonably assume that each FMP may
have a different format.
I think it's a very natural thing.
> So unless we
> have an example of a device that says "This firmware file can only be updated
> by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
> see any reason why we need to support that. The changes are not set in stone
> anyway. The code was fine before the ESRT got involved. So all my patch
> really does is make the current code useful when an ESRT is installed. We can
> then break the spec on purpose (yes break it :>) ignore the OsIndications
> bit and have fwupd working with U-Boot. This will have an actual impact on
> devices and the code usability, since people will start using it. I prefer
> this over adding a very cumbersome corner case, that's arguably no one will
> ever need.
> We can always go back and make them a config option in the future. But unless
> we get a use case for it, I'd still prefer having them mutually exclusive,
> rather than adding code for an imaginary device (which I really doubt anyone
> will ever design).
I don't think that the example I gave is a imaginary device.
> > > >
> > > > > > We should not impose unnecessary restriction if we manage to have some
> > > > > > workaround to meet the requirement.
> > > > >
> > > > > It's not the updating part only. It's that the .get_image_info also relies on
> > > > > the same env variable.
> > > >
> > > > The above idea can and should be applied to GetImageInfo implementation
> > > > at the same time.
> > >
> > > Yes but can you do it with just changing the env variable now? Or you need to
> > > add more code into the DFU logic?
> >
> > Those *meta* data for firmware can be declared/specified outside of FMP,
> > and be referred to by FMP (and/or ESRT). That is what I meant by:
> > > > > > (I still believe that the firmware definition for ESRT should exist
> > > > > > elsewhere other than in FMP themselves.)
> >
> >
> > > >
> > > > > Specifically in the fwupd case on an RPI4 with the
> > > > > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> > > > > populated only one was reported. Probably because this really does give you
> > > > > 2 ways of updating the same flash.
> > > > >
> > > > > > (I still believe that the firmware definition for ESRT should exist
> > > > > > elsewhere other than in FMP themselves.)
> > > > >
> > > > > That's a whole different story, and if we have that, then .get_image_info
> > > > > should change as well instead of using the DFU information.
> > > >
> > > > I don't think so as I mentioned above.
> > >
> > > And I don't see any benefit from storing the same information in 2 completely
> > > disjoint entities.
> >
> > ?
>
> The ESRT code right now uses get_image_info from the FMP code and the FMP code
> uses the dfu_alt_info to derive whatever information it needs. Both of these
> concepts are trying to provide information about the running firmware. So if
> we change that imho both of them should get that info from an abstracted
> object (file/c struct in u-boot/whatever). But really I think using FMP to
> fill ESRT entries is fine (at least for me).
Well, dfu_alt_info can already be seen as abstracted object
in terms of FMP.
> >
> > > >
> > > > > Because right
> > > > > now we enabled security (or think we have), while allowing users to set an env
> > > > > variable which is not authenticated, and completely change what the
> > > > > firmware reports (or updates).
> > > >
> > > > This is the point that I mentioned earlier in our private chat,
> > > > and it's a "whole different" story in this context.
> > >
> > > You mentioned that in the context of "can we install the FMPs during the EFI
> > > init". Since the variable is interpreted at runtime, we definitely can. I
> > > looked back at that chat and saw nothing related to the security problems
> > > we'll create.
> >
> > You have referred to this issue in the context of security.
> > So I said that it was a different story.
> > The issue that you're trying to address in this patch is *NOT* security.
> >
>
> No it's not, I am just pointing out the obvious.
>
> > > In any case the problem here is real, but there are sane ways to avoid it.
> > >
> > > >
> > > > > We can always add a huge warning saying
> > > > > something along the lines of "If you really care this should come with a
> > > > > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
> > > > >
> > > > > The spec is pretty clear and we allow users to *break* it with a config
> > > > > option. Arguably this is fine, since the code continues to work fine and
> > > > > you can perform the updates, but in essence RAW and FITs are used to update
> > > > > the same medium right now. You can't have a capsule with half it's contents
> > > > > describing something RAW and the other half being a FIT. You have a FIT based
> > > > > capsule or a RAW based capsule.
> > > >
> > > > See above.
> > >
> > > I still don't get it.
> > > The fact is we have a config option, that if the user decides to set in a
> > > specific way (and that specific way is 99% of the use cases) we'll break the
> > > EFI spec.
> >
> > As I said above, I have never said that the current implementation does
> > not break EFI spec if not properly used.
> > So I suggested a possible solution in the previous email as you asked me.
> >
> > > So unless we add code into the dfu logic, parsing dfu_alt_info and
> > > figuring out if the user is allowed to do that or not, I really think those two
> > > must be treated as mutually exclusive.
> >
> > I don't think that we need to modify DFU code.
>
> Yea me neither, but since the firmware runtime information are derived from
> that, we don't have that many options.
What do you mean by "options"?
-Takahiro Akashi
> Thanks
> /Ilias
> >
> > -Takahiro Akashi
> >
> > > >
> > > > > > - We should allow users to add their own FMP drivers and so not call
> > > > > > [arch_]efi_load_capsule_drivers() unconditionally
> > > > > > even if you don't like "__weak" attribute.
> > > > >
> > > > > I am fine with the __weak attribute. On the other hand I consider the
> > > > > current code the defacto way users would use to update their firmware. That's
> > > > > why I removed the __weak attribute. If a hardware vendor was to update
> > > > > their special PCI option ROM or a flash that lives on the secure world they
> > > > > should install their FMPs on a different handle and leave the current code
> > > > > as is.
> > > >
> > > > And we should provide an option that disables these existing handle.
> > >
> > > The existing one is not enough?
> > >
> > > >
> > > > > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > > > > > leave some test cases in pytest skipped.
> > > > >
> > > > > Yea that's unfortunate, but maybe we can just add an extra config on the
> > > > > sandbox?
> > > >
> > > > Please add another patch that is missing.
> > >
> > >
> > > >
> > > > -Takahiro Akashi
> > > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 6:40 ` AKASHI Takahiro
@ 2021-06-15 6:56 ` Ilias Apalodimas
2021-06-15 7:13 ` AKASHI Takahiro
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 6:56 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, masami.hiramatsu, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
> > >
[...]
> > > They are fixing "different" problems relating ESRT generation.
> > > That is my point.
> > >
> >
> > Sure, but it's a minor clean up really. As I said the current code works
> > fine. So I dont really mind the fact that it breaks a sentence of the spec.
> > Hence I considered the cleanup and the mutual exclusive part to be really
> > minor.
>
> Yes, it's minor but still a different problem.
> Let me give you an example.
> If I correct a misspelling in a given code
> very close to the change, Heinrich would ask
> me to add a separate patch as it is simply not
> related.
>
> Moreover, from the viewpoint of maintenance (i.e. bisect ability),
> they should be separated from each other.
>
> > > >
> > > > >
> > > > Is there anything very specific that you can achieve with FIT capsules that
[...]
> > > > you can't achieve with RAW ones (or vice versa), that would justify having
> > > > them both present at the same time?
> > >
> > > Yes.
> > > We may have different *firmware* for different software components
> > > and different devices. For example,
> > > You have firmare like U-Boot binary and default variable storage
> > > in different partitions.
> > > On the other hand, you have an extra firmware for a particular
> > > peripheral, like PCI device or anything else, which comes
> > > from a 3rd party vendor of the device.
> > > The former may and can be packed into a single binary in FIT format.
> > > The latter can be used in a separate RAW format as the timing of
> > > updating those firmware is likely to be different.
> > >
> >
> > Sure that's a use case. But that's not a specific one, nor something you cant
> > do without both of them being installed. You can arguably just create a RAW
> > image for the second firmware and put the info into dfu_alt_info.
>
> Why do you stick to a single format?
I think it's the other way around. Why wouldn't you? It's the easiest and
sanest thing to do when generating capsules.
> We can reasonably assume that each FMP may
> have a different format.
> I think it's a very natural thing.
The FMPs logic in the EFI spec is not tied to 'format', it's tied to 'device'
and currently both FMPs target the same device. So my understanding is, that
in order to use it you need to:
1. Create 2 capsules, 1 raw, 1 fmp.
2. Set dfu_alt_info -> process RAW capsule.
3. Set dfu_alt_info to something different -> process FIT capsule.
and by doing so the ESRTs will use one of the information found in
dfu_alt_info.
>
> > So unless we
> > have an example of a device that says "This firmware file can only be updated
> > by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
> > see any reason why we need to support that. The changes are not set in stone
> > anyway. The code was fine before the ESRT got involved. So all my patch
> > really does is make the current code useful when an ESRT is installed. We can
> > then break the spec on purpose (yes break it :>) ignore the OsIndications
> > bit and have fwupd working with U-Boot. This will have an actual impact on
> > devices and the code usability, since people will start using it. I prefer
> > this over adding a very cumbersome corner case, that's arguably no one will
> > ever need.
> > We can always go back and make them a config option in the future. But unless
> > we get a use case for it, I'd still prefer having them mutually exclusive,
> > rather than adding code for an imaginary device (which I really doubt anyone
> > will ever design).
>
> I don't think that the example I gave is a imaginary device.
>
All of the devices I've tested and seen up to now are working fine with just
RAW capsules installed and I can't understand why a specific *format* should
play a role in the capsule creation.
FITs are a nice way to get authentication and bundle things without having the
EFI capsule authentication code, but really apart from that those 2 are doing
the same thing.
[...]
> > The ESRT code right now uses get_image_info from the FMP code and the FMP code
> > uses the dfu_alt_info to derive whatever information it needs. Both of these
> > concepts are trying to provide information about the running firmware. So if
> > we change that imho both of them should get that info from an abstracted
> > object (file/c struct in u-boot/whatever). But really I think using FMP to
> > fill ESRT entries is fine (at least for me).
>
> Well, dfu_alt_info can already be seen as abstracted object
> in terms of FMP.
Yes but it can't handle the ESRT generation properly. So if you change that,
why leave the FMPs get_image_info, read the information differently?
> >
[...]
> > Yea me neither, but since the firmware runtime information are derived from
> > that, we don't have that many options.
>
> What do you mean by "options"?
I don't see any point of trating .get_image_info and the information that get
emitted into an ERST differently.
[...]
Thanks
/Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 6:56 ` Ilias Apalodimas
@ 2021-06-15 7:13 ` AKASHI Takahiro
2021-06-15 8:19 ` Ilias Apalodimas
0 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2021-06-15 7:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, masami.hiramatsu, Simon Glass, Mario Six,
Michal Simek, Alexander Graf, u-boot
On Tue, Jun 15, 2021 at 09:56:10AM +0300, Ilias Apalodimas wrote:
> > > >
>
> [...]
>
> > > > They are fixing "different" problems relating ESRT generation.
> > > > That is my point.
> > > >
> > >
> > > Sure, but it's a minor clean up really. As I said the current code works
> > > fine. So I dont really mind the fact that it breaks a sentence of the spec.
> > > Hence I considered the cleanup and the mutual exclusive part to be really
> > > minor.
> >
> > Yes, it's minor but still a different problem.
> > Let me give you an example.
> > If I correct a misspelling in a given code
> > very close to the change, Heinrich would ask
> > me to add a separate patch as it is simply not
> > related.
> >
> > Moreover, from the viewpoint of maintenance (i.e. bisect ability),
> > they should be separated from each other.
> >
> > > > >
> > > > > >
> > > > > Is there anything very specific that you can achieve with FIT capsules that
>
> [...]
>
> > > > > you can't achieve with RAW ones (or vice versa), that would justify having
> > > > > them both present at the same time?
> > > >
> > > > Yes.
> > > > We may have different *firmware* for different software components
> > > > and different devices. For example,
> > > > You have firmare like U-Boot binary and default variable storage
> > > > in different partitions.
> > > > On the other hand, you have an extra firmware for a particular
> > > > peripheral, like PCI device or anything else, which comes
> > > > from a 3rd party vendor of the device.
> > > > The former may and can be packed into a single binary in FIT format.
> > > > The latter can be used in a separate RAW format as the timing of
> > > > updating those firmware is likely to be different.
> > > >
> > >
> > > Sure that's a use case. But that's not a specific one, nor something you cant
> > > do without both of them being installed. You can arguably just create a RAW
> > > image for the second firmware and put the info into dfu_alt_info.
> >
> > Why do you stick to a single format?
>
> I think it's the other way around. Why wouldn't you? It's the easiest and
> sanest thing to do when generating capsules.
I don't know why you think it's the easiest.
"mkeficapsule" can create any format of capsule file.
(This is not true in the current form. but it's quite easy
to enhance this command for this purpose.)
> > We can reasonably assume that each FMP may
> > have a different format.
> > I think it's a very natural thing.
>
> The FMPs logic in the EFI spec is not tied to 'format', it's tied to 'device'
> and currently both FMPs target the same device.
the same device? What do you mean by 'same device'?
I probably won't agree.
> So my understanding is, that
> in order to use it you need to:
> 1. Create 2 capsules, 1 raw, 1 fmp.
> 2. Set dfu_alt_info -> process RAW capsule.
What do you mean by "process"?
> 3. Set dfu_alt_info to something different -> process FIT capsule.
No. What you need to do is to *add* definition for firmware
in FIT. We can have a single definition of dfu_alt_info
even if we use both FIT and RAW FMPs.
> and by doing so the ESRTs will use one of the information found in
> dfu_alt_info.
So what?
>
> >
> > > So unless we
> > > have an example of a device that says "This firmware file can only be updated
> > > by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
> > > see any reason why we need to support that. The changes are not set in stone
> > > anyway. The code was fine before the ESRT got involved. So all my patch
> > > really does is make the current code useful when an ESRT is installed. We can
> > > then break the spec on purpose (yes break it :>) ignore the OsIndications
> > > bit and have fwupd working with U-Boot. This will have an actual impact on
> > > devices and the code usability, since people will start using it. I prefer
> > > this over adding a very cumbersome corner case, that's arguably no one will
> > > ever need.
> > > We can always go back and make them a config option in the future. But unless
> > > we get a use case for it, I'd still prefer having them mutually exclusive,
> > > rather than adding code for an imaginary device (which I really doubt anyone
> > > will ever design).
> >
> > I don't think that the example I gave is a imaginary device.
> >
>
> All of the devices I've tested and seen up to now are working fine with just
> RAW capsules installed and I can't understand why a specific *format* should
> play a role in the capsule creation.
I don't get your point.
I never said, "a specific format should play a role."
> FITs are a nice way to get authentication and bundle things without having the
> EFI capsule authentication code, but really apart from that those 2 are doing
> the same thing.
Yes, but why do we have to have the restriction?
Different capsule files for different firmware/device may
come from different owners of the firmware.
> [...]
> > > The ESRT code right now uses get_image_info from the FMP code and the FMP code
> > > uses the dfu_alt_info to derive whatever information it needs. Both of these
> > > concepts are trying to provide information about the running firmware. So if
> > > we change that imho both of them should get that info from an abstracted
> > > object (file/c struct in u-boot/whatever). But really I think using FMP to
> > > fill ESRT entries is fine (at least for me).
> >
> > Well, dfu_alt_info can already be seen as abstracted object
> > in terms of FMP.
>
> Yes but it can't handle the ESRT generation properly. So if you change that,
> why leave the FMPs get_image_info, read the information differently?
Again, I don't get your point.
-Takahiro Akashi
> > >
> [...]
> > > Yea me neither, but since the firmware runtime information are derived from
> > > that, we don't have that many options.
> >
> > What do you mean by "options"?
>
> I don't see any point of trating .get_image_info and the information that get
> emitted into an ERST differently.
>
> [...]
>
> Thanks
> /Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] efi_loader: FMP cleanups
2021-06-15 7:13 ` AKASHI Takahiro
@ 2021-06-15 8:19 ` Ilias Apalodimas
0 siblings, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2021-06-15 8:19 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, masami.hiramatsu, Simon Glass,
Mario Six, Michal Simek, Alexander Graf, u-boot
[...]
> > > > > Yes.
> > > > > We may have different *firmware* for different software components
> > > > > and different devices. For example,
> > > > > You have firmare like U-Boot binary and default variable storage
> > > > > in different partitions.
> > > > > On the other hand, you have an extra firmware for a particular
> > > > > peripheral, like PCI device or anything else, which comes
> > > > > from a 3rd party vendor of the device.
> > > > > The former may and can be packed into a single binary in FIT format.
> > > > > The latter can be used in a separate RAW format as the timing of
> > > > > updating those firmware is likely to be different.
> > > > >
> > > >
> > > > Sure that's a use case. But that's not a specific one, nor something you cant
> > > > do without both of them being installed. You can arguably just create a RAW
> > > > image for the second firmware and put the info into dfu_alt_info.
> > >
> > > Why do you stick to a single format?
> >
> > I think it's the other way around. Why wouldn't you? It's the easiest and
> > sanest thing to do when generating capsules.
>
> I don't know why you think it's the easiest.
> "mkeficapsule" can create any format of capsule file.
> (This is not true in the current form. but it's quite easy
> to enhance this command for this purpose.)
Because it's easier from a system maintenance perspective to have to deal with
a single capsule format instead of two.
>
> > > We can reasonably assume that each FMP may
> > > have a different format.
> > > I think it's a very natural thing.
> >
> > The FMPs logic in the EFI spec is not tied to 'format', it's tied to 'device'
> > and currently both FMPs target the same device.
>
> the same device? What do you mean by 'same device'?
> I probably won't agree.
>
This depends on the dfu_alt_info. See below
> > So my understanding is, that
> > in order to use it you need to:
> > 1. Create 2 capsules, 1 raw, 1 fmp.
> > 2. Set dfu_alt_info -> process RAW capsule.
>
> What do you mean by "process"?
>
> > 3. Set dfu_alt_info to something different -> process FIT capsule.
>
> No. What you need to do is to *add* definition for firmware
> in FIT. We can have a single definition of dfu_alt_info
> even if we use both FIT and RAW FMPs.
Ok that's what I've been asking all along.
Do you have an *working* example for that?
Set the dfu_alt_info once and be able to update 2 different flash devices on
the same system, using 1 capsule as RAW and 1 capsule as FIT. While at the
same time populate those 2 different flash devices correctly in the ESRT.
>
> > and by doing so the ESRTs will use one of the information found in
> > dfu_alt_info.
>
> So what?
If they both refer to the same flash, the spec says you shouldn't do that and
you have no way of knowing this right now.
But if you can define a dfu_alt_info, that exposes 1 flash device handled by
the RAW capsule and 1 flash device handled by the FIT capsule, I guess that
would be fine.
But looking at the dfu entities code, we can add mmc, mtd, nand, ram, sf or
virt and the it handles things like 'raw' of a filesystem. How do you point
out "this dfu entry is a fit"?
>
> >
> > >
> > > > So unless we
> > > > have an example of a device that says "This firmware file can only be updated
> > > > by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't
> > > > see any reason why we need to support that. The changes are not set in stone
> > > > anyway. The code was fine before the ESRT got involved. So all my patch
> > > > really does is make the current code useful when an ESRT is installed. We can
> > > > then break the spec on purpose (yes break it :>) ignore the OsIndications
> > > > bit and have fwupd working with U-Boot. This will have an actual impact on
> > > > devices and the code usability, since people will start using it. I prefer
> > > > this over adding a very cumbersome corner case, that's arguably no one will
> > > > ever need.
> > > > We can always go back and make them a config option in the future. But unless
> > > > we get a use case for it, I'd still prefer having them mutually exclusive,
> > > > rather than adding code for an imaginary device (which I really doubt anyone
> > > > will ever design).
> > >
> > > I don't think that the example I gave is a imaginary device.
> > >
> >
> > All of the devices I've tested and seen up to now are working fine with just
> > RAW capsules installed and I can't understand why a specific *format* should
> > play a role in the capsule creation.
>
> I don't get your point.
> I never said, "a specific format should play a role."
>
> > FITs are a nice way to get authentication and bundle things without having the
> > EFI capsule authentication code, but really apart from that those 2 are doing
> > the same thing.
>
> Yes, but why do we have to have the restriction?
> Different capsule files for different firmware/device may
> come from different owners of the firmware.
No that's a completely moot point imho. If any vendor has a very special
firmware he needs to treat in a very special way, he needs to install his own
FMP. If we are talking about different firmwares on *different* flash
devices, then the final board re-seller should say "all of you individual
people send me RAW capsules, or FIT capsules". There still hasn't been a
single argument on what you can achieve with a FIT specific image, which you
can't with a RAW. So I'll ask again. The fact that you *can* do it doesn't
mean it the sane thing to do.
>
> > [...]
> > > > The ESRT code right now uses get_image_info from the FMP code and the FMP code
> > > > uses the dfu_alt_info to derive whatever information it needs. Both of these
> > > > concepts are trying to provide information about the running firmware. So if
> > > > we change that imho both of them should get that info from an abstracted
> > > > object (file/c struct in u-boot/whatever). But really I think using FMP to
> > > > fill ESRT entries is fine (at least for me).
> > >
> > > Well, dfu_alt_info can already be seen as abstracted object
> > > in terms of FMP.
> >
> > Yes but it can't handle the ESRT generation properly. So if you change that,
> > why leave the FMPs get_image_info, read the information differently?
>
> Again, I don't get your point.
>
I've tried to explain this a couple of times. If we manage to answer the
dfu_alt_info question above sufficiently, then maybe you will
Thanks
/Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-15 8:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-14 15:10 [PATCH] efi_loader: FMP cleanups Ilias Apalodimas
2021-06-14 15:13 ` Ilias Apalodimas
2021-06-15 0:49 ` Masami Hiramatsu
2021-06-15 3:56 ` Ilias Apalodimas
2021-06-15 1:51 ` AKASHI Takahiro
2021-06-15 3:55 ` Ilias Apalodimas
2021-06-15 4:44 ` AKASHI Takahiro
2021-06-15 5:23 ` Ilias Apalodimas
2021-06-15 5:55 ` AKASHI Takahiro
2021-06-15 6:22 ` Ilias Apalodimas
2021-06-15 6:40 ` AKASHI Takahiro
2021-06-15 6:56 ` Ilias Apalodimas
2021-06-15 7:13 ` AKASHI Takahiro
2021-06-15 8:19 ` Ilias Apalodimas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).