* [PATCH v5 0/4] FMP versioning support
@ 2023-04-10 9:07 Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-10 9:07 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi, Masahisa Kojima
Firmware version management is not implemented in the current
FMP implementation. This series aims to add the versioning support
in FMP.
There is a major design change in v5.
Until v4, the fw_version and lowest_supported_version are stored
as a EFI variable. If the backing storage is a file we can't trust
any of that information since anyone can tamper with the file,
although the variables are defined as RO.
With that, we store the version information in the device tree
in v5. We can trust the information from dtb as long as the former
stage boot loader verifies the image containing the dtb.
The disadvantage of this design change is that we need to maintain
the fw_version in both device tree and FMP Payload Header.
It is inevitable since not all the capsule files contain the dtb.
EDK II reference implementation utilizes the FMP Payload Header
inserted right before the capsule payload. With this series,
U-Boot also follows the EDK II implementation.
Currently, there is no way to know the current running firmware
version through the EFI interface. FMP->GetImageInfo() returns
always 0 for the version number. So a user can not know that
expected firmware is running after the capsule update.
With this series applied, version number can be specified
in the capsule file generation with mkeficapsule tool, then
user can know the running firmware version through
FMP->GetImageInfo() and ESRT.
Note that this series does not mandate the FMP Payload Header,
compatible with boards that are already using the existing
U-Boot FMP implementation.
If no FMP Payload Header is found in the capsule file, fw_version,
lowest supported version, last attempt version and last attempt
status is set to 0 and this is the same behavior as existing FMP
implementation.
Major Changes in v5:
- major design changes, versioning is implemented with
device tree instead of EFI variable
Major Changes in v4:
- add python-based test
Major Changes in v3:
- exclude CONFIG_FWU_MULTI
Masahisa Kojima (4):
efi_loader: get version information from device tree
efi_loader: check lowest supported version
mkeficapsule: add FMP Payload Header
test/py: efi_capsule: test for FMP versioning
.../firmware/firmware-version.txt | 25 +++
doc/mkeficapsule.1 | 10 +
lib/efi_loader/efi_firmware.c | 187 +++++++++++++---
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
.../test_capsule_firmware_fit.py | 187 ++++++++++++++++
.../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
.../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
.../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
test/py/tests/test_efi_capsule/version.dts | 27 +++
tools/eficapsule.h | 30 +++
tools/mkeficapsule.c | 37 +++-
11 files changed, 1082 insertions(+), 29 deletions(-)
create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
create mode 100644 test/py/tests/test_efi_capsule/version.dts
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
@ 2023-04-10 9:07 ` Masahisa Kojima
2023-04-27 6:09 ` Heinrich Schuchardt
2023-04-27 23:35 ` Simon Glass
2023-04-10 9:07 ` [PATCH v5 2/4] efi_loader: check lowest supported version Masahisa Kojima
` (3 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-10 9:07 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi, Masahisa Kojima
Current FMP->GetImageInfo() always return 0 for the firmware
version, user can not identify which firmware version is currently
running through the EFI interface.
This commit gets the version information from device tree,
then fills the firmware version, lowest supported version
in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v5:
- newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++
lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
2 files changed, 84 insertions(+), 4 deletions(-)
create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
new file mode 100644
index 0000000000..6112de4a1d
--- /dev/null
+++ b/doc/device-tree-bindings/firmware/firmware-version.txt
@@ -0,0 +1,25 @@
+firmware-version bindings
+-------------------------------
+
+Required properties:
+- image-type-id : guid for image blob type
+- image-index : image index
+- fw-version : firmware version
+- lowest-supported-version : lowest supported version
+
+Example:
+
+ firmware-version {
+ image1 {
+ image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
+ image-index = <1>;
+ fw-version = <5>;
+ lowest-supported-version = <3>;
+ };
+ image2 {
+ image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
+ image-index = <2>;
+ fw-version = <10>;
+ lowest-supported-version = <7>;
+ };
+ };
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 93e2b01c07..1c6ef468bf 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
return EFI_EXIT(EFI_UNSUPPORTED);
}
+/**
+ * efi_firmware_get_firmware_version - get firmware version from dtb
+ * @image_index: Image index
+ * @image_type_id: Image type id
+ * @fw_version: Pointer to store the version number
+ * @lsv: Pointer to store the lowest supported version
+ *
+ * Authenticate the capsule if authentication is enabled.
+ * The image pointer and the image size are updated in case of success.
+ */
+void efi_firmware_get_firmware_version(u8 image_index,
+ efi_guid_t *image_type_id,
+ u32 *fw_version, u32 *lsv)
+{
+ const void *fdt = gd->fdt_blob;
+ const fdt32_t *val;
+ const char *guid_str;
+ int len, offset, index;
+ int parent;
+
+ parent = fdt_subnode_offset(fdt, 0, "firmware-version");
+ if (parent < 0)
+ return;
+
+ fdt_for_each_subnode(offset, fdt, parent) {
+ efi_guid_t guid;
+
+ guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
+ if (!guid_str)
+ continue;
+ uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
+
+ val = fdt_getprop(fdt, offset, "image-index", &len);
+ if (!val)
+ continue;
+ index = fdt32_to_cpu(*val);
+
+ if (!guidcmp(&guid, image_type_id) && index == image_index) {
+ val = fdt_getprop(fdt, offset, "fw-version", &len);
+ if (val)
+ *fw_version = fdt32_to_cpu(*val);
+
+ val = fdt_getprop(fdt, offset,
+ "lowest-supported-version", &len);
+ if (val)
+ *lsv = fdt32_to_cpu(*val);
+ }
+ }
+}
+
/**
* efi_fill_image_desc_array - populate image descriptor array
* @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
*package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {
+ u32 fw_version = 0;
+ u32 lowest_supported_version = 0;
+
image_info[i].image_index = fw_array[i].image_index;
image_info[i].image_type_id = fw_array[i].image_type_id;
image_info[i].image_id = fw_array[i].image_index;
image_info[i].image_id_name = fw_array[i].fw_name;
-
- image_info[i].version = 0; /* not supported */
+ efi_firmware_get_firmware_version(fw_array[i].image_index,
+ &fw_array[i].image_type_id,
+ &fw_version,
+ &lowest_supported_version);
+ image_info[i].version = fw_version;
image_info[i].version_name = NULL; /* not supported */
image_info[i].size = 0;
image_info[i].attributes_supported =
@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
image_info[0].attributes_setting |=
IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
- image_info[i].lowest_supported_image_version = 0;
+ image_info[i].lowest_supported_image_version = lowest_supported_version;
image_info[i].last_attempt_version = 0;
image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
image_info[i].hardware_instance = 1;
@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
descriptor_version, descriptor_count,
descriptor_size, package_version,
package_version_name);
-
return EFI_EXIT(ret);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 2/4] efi_loader: check lowest supported version
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
@ 2023-04-10 9:07 ` Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 3/4] mkeficapsule: add FMP Payload Header Masahisa Kojima
` (2 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-10 9:07 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi, Masahisa Kojima
The FMP Payload Header which EDK II capsule generation scripts
insert has a firmware version.
This commit reads the lowest supported version stored in the
device tree, then check if the firmware version in FMP payload header
of the ongoing capsule is equal or greater than the
lowest supported version. If the firmware version is lower than
lowest supported version, capsule update fails.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v5:
- newly implement the device tree based versioning
Changes in v4:
- use log_err() instead of printf()
Changes in v2:
- add error message when the firmware version is lower than
lowest supported version
lib/efi_loader/efi_firmware.c | 124 ++++++++++++++++++++++++++++------
1 file changed, 103 insertions(+), 21 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 1c6ef468bf..c88c1bb364 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -33,7 +33,7 @@ struct fmp_payload_header {
u32 signature;
u32 header_size;
u32 fw_version;
- u32 lowest_supported_version;
+ u32 lowest_supported_version; /* not used */
};
__weak void set_dfu_alt_info(char *interface, char *devstr)
@@ -250,8 +250,6 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
{
const void *image = *p_image;
efi_uintn_t image_size = *p_image_size;
- u32 fmp_hdr_signature;
- struct fmp_payload_header *header;
void *capsule_payload;
efi_status_t status;
efi_uintn_t capsule_payload_size;
@@ -264,7 +262,7 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
&capsule_payload_size);
if (status == EFI_SECURITY_VIOLATION) {
- printf("Capsule authentication check failed. Aborting update\n");
+ log_err("Capsule authentication check failed. Aborting update\n");
return status;
} else if (status != EFI_SUCCESS) {
return status;
@@ -278,21 +276,6 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
debug("Updating capsule without authenticating.\n");
}
- fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
- header = (void *)image;
-
- if (!memcmp(&header->signature, &fmp_hdr_signature,
- sizeof(fmp_hdr_signature))) {
- /*
- * When building the capsule with the scripts in
- * edk2, a FMP header is inserted above the capsule
- * payload. Compensate for this header to get the
- * actual payload that is to be updated.
- */
- image += header->header_size;
- image_size -= header->header_size;
- }
-
*p_image = image;
*p_image_size = image_size;
return EFI_SUCCESS;
@@ -349,6 +332,105 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
return EFI_EXIT(ret);
}
+/**
+ * efi_firmware_get_image_type_id - get image_type_id
+ * @image_index: image index
+ *
+ * Return the image_type_id identified by the image index.
+ *
+ * Return: pointer to the image_type_id, NULL if image_index is invalid
+ */
+static
+efi_guid_t *efi_firmware_get_image_type_id(u8 image_index)
+{
+ int i;
+ struct efi_fw_image *fw_array;
+
+ fw_array = update_info.images;
+ for (i = 0; i < num_image_type_guids; i++) {
+ if (fw_array[i].image_index == image_index)
+ return &fw_array[i].image_type_id;
+ }
+
+ return NULL;
+}
+
+/**
+ * efi_firmware_check_lowest_supported_version - check the lowest supported version
+ * @p_image: Pointer to new image
+ * @p_image_size: Pointer to size of new image
+ * @image_index: image_index
+ *
+ * Check the fw_version in FMP payload header is equal to or greather than the
+ * lowest_supported_version stored in the device tree
+ *
+ * Return: status code
+ */
+static
+efi_status_t efi_firmware_check_lowest_supported_version(const void **p_image,
+ efi_uintn_t *p_image_size,
+ u8 image_index)
+{
+ const void *image = *p_image;
+ efi_uintn_t image_size = *p_image_size;
+ const struct fmp_payload_header *header;
+ u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
+
+ /* FMP header is inserted above the capsule payload */
+ header = image;
+ if (header->signature == fmp_hdr_signature) {
+ efi_guid_t *image_type_id;
+ u32 version = 0, lsv = 0;
+
+ image_type_id = efi_firmware_get_image_type_id(image_index);
+ if (!image_type_id)
+ return EFI_INVALID_PARAMETER;
+
+ efi_firmware_get_firmware_version(image_index, image_type_id,
+ &version, &lsv);
+
+ if (header->fw_version >= lsv) {
+ image += header->header_size;
+ image_size -= header->header_size;
+ } else {
+ log_err("Firmware version %u too low. Expecting >= %u. Aborting update\n",
+ header->fw_version, lsv);
+ return EFI_INCOMPATIBLE_VERSION;
+ }
+ }
+
+ *p_image = image;
+ *p_image_size = image_size;
+
+ return EFI_SUCCESS;
+}
+
+/**
+ * efi_firmware_verify_image - verify image
+ * @p_image: Pointer to new image
+ * @p_image_size: Pointer to size of new image
+ * @image_index Image index
+ *
+ * Verify the capsule file
+ *
+ * Return: status code
+ */
+static
+efi_status_t efi_firmware_verify_image(const void **p_image,
+ efi_uintn_t *p_image_size,
+ u8 image_index)
+{
+ efi_status_t ret;
+
+ ret = efi_firmware_capsule_authenticate(p_image, p_image_size);
+ if (ret != EFI_SUCCESS)
+ return ret;
+
+ ret = efi_firmware_check_lowest_supported_version(p_image, p_image_size, image_index);
+
+ return ret;
+}
+
#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_FIT
/*
* This FIRMWARE_MANAGEMENT_PROTOCOL driver provides a firmware update
@@ -393,7 +475,7 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
if (!image || image_index != 1)
return EFI_EXIT(EFI_INVALID_PARAMETER);
- status = efi_firmware_capsule_authenticate(&image, &image_size);
+ status = efi_firmware_verify_image(&image, &image_size, image_index);
if (status != EFI_SUCCESS)
return EFI_EXIT(status);
@@ -454,7 +536,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
if (!image)
return EFI_EXIT(EFI_INVALID_PARAMETER);
- status = efi_firmware_capsule_authenticate(&image, &image_size);
+ status = efi_firmware_verify_image(&image, &image_size, image_index);
if (status != EFI_SUCCESS)
return EFI_EXIT(status);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 3/4] mkeficapsule: add FMP Payload Header
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 2/4] efi_loader: check lowest supported version Masahisa Kojima
@ 2023-04-10 9:07 ` Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning Masahisa Kojima
2023-04-11 1:48 ` [PATCH v5 0/4] FMP versioning support Takahiro Akashi
4 siblings, 0 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-10 9:07 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi,
Masahisa Kojima, Sughosh Ganu, Etienne Carriere
Current mkeficapsule tool does not provide firmware
version management. EDK II reference implementation inserts
the FMP Payload Header right before the payload.
It coutains the fw_version and lowest supported version.
This commit adds a new parameters required to generate
the FMP Payload Header for mkeficapsule tool.
'-v' indicates the firmware version.
When mkeficapsule tool is invoked without '-v' option,
FMP Payload Header is not inserted, the behavior is same as
current implementation.
The lowest supported version included in the FMP Payload Header
is not used in the current versioning support, the value stored
in the device tree is used.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v5:
- remove --lsv since we use the lowest_supported_version in the dtb
Changes in v3:
- remove '-f' option
- move some definitions into tools/eficapsule.h
- add dependency check of fw_version and lowest_supported_version
- remove unexpected modification of existing fprintf() call
- add documentation
Newly created in v2
doc/mkeficapsule.1 | 10 ++++++++++
tools/eficapsule.h | 30 ++++++++++++++++++++++++++++++
tools/mkeficapsule.c | 37 +++++++++++++++++++++++++++++++++----
3 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
index 1ca245a10f..c4c2057d5c 100644
--- a/doc/mkeficapsule.1
+++ b/doc/mkeficapsule.1
@@ -61,6 +61,16 @@ Specify an image index
.BI "-I\fR,\fB --instance " instance
Specify a hardware instance
+.PP
+FMP Payload Header is inserted right before the payload if
+.BR --fw-version
+is specified
+
+
+.TP
+.BI "-v\fR,\fB --fw-version " firmware-version
+Specify a firmware version, 0 if omitted
+
.PP
For generation of firmware accept empty capsule
.BR --guid
diff --git a/tools/eficapsule.h b/tools/eficapsule.h
index 072a4b5598..753fb73313 100644
--- a/tools/eficapsule.h
+++ b/tools/eficapsule.h
@@ -113,4 +113,34 @@ struct efi_firmware_image_authentication {
struct win_certificate_uefi_guid auth_info;
} __packed;
+/* fmp payload header */
+#define SIGNATURE_16(A, B) ((A) | ((B) << 8))
+#define SIGNATURE_32(A, B, C, D) \
+ (SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16))
+
+#define FMP_PAYLOAD_HDR_SIGNATURE SIGNATURE_32('M', 'S', 'S', '1')
+
+/**
+ * struct fmp_payload_header - EDK2 header for the FMP payload
+ *
+ * This structure describes the header which is preprended to the
+ * FMP payload by the edk2 capsule generation scripts.
+ *
+ * @signature: Header signature used to identify the header
+ * @header_size: Size of the structure
+ * @fw_version: Firmware versions used
+ * @lowest_supported_version: Lowest supported version (not used)
+ */
+struct fmp_payload_header {
+ uint32_t signature;
+ uint32_t header_size;
+ uint32_t fw_version;
+ uint32_t lowest_supported_version;
+};
+
+struct fmp_payload_header_params {
+ bool have_header;
+ uint32_t fw_version;
+};
+
#endif /* _EFI_CAPSULE_H */
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index b71537beee..52be1f122e 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -41,6 +41,7 @@ static struct option options[] = {
{"guid", required_argument, NULL, 'g'},
{"index", required_argument, NULL, 'i'},
{"instance", required_argument, NULL, 'I'},
+ {"fw-version", required_argument, NULL, 'v'},
{"private-key", required_argument, NULL, 'p'},
{"certificate", required_argument, NULL, 'c'},
{"monotonic-count", required_argument, NULL, 'm'},
@@ -60,6 +61,7 @@ static void print_usage(void)
"\t-g, --guid <guid string> guid for image blob type\n"
"\t-i, --index <index> update image index\n"
"\t-I, --instance <instance> update hardware instance\n"
+ "\t-v, --fw-version <version> firmware version\n"
"\t-p, --private-key <privkey file> private key file\n"
"\t-c, --certificate <cert file> signer's certificate file\n"
"\t-m, --monotonic-count <count> monotonic count\n"
@@ -402,6 +404,7 @@ static void free_sig_data(struct auth_context *ctx)
*/
static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
unsigned long index, unsigned long instance,
+ struct fmp_payload_header_params *fmp_ph_params,
uint64_t mcount, char *privkey_file, char *cert_file,
uint16_t oemflags)
{
@@ -410,10 +413,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
struct efi_firmware_management_capsule_image_header image;
struct auth_context auth_context;
FILE *f;
- uint8_t *data;
+ uint8_t *data, *new_data, *buf;
off_t bin_size;
uint64_t offset;
int ret;
+ struct fmp_payload_header payload_header;
#ifdef DEBUG
fprintf(stderr, "For output: %s\n", path);
@@ -423,6 +427,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
auth_context.sig_size = 0;
f = NULL;
data = NULL;
+ new_data = NULL;
ret = -1;
/*
@@ -431,12 +436,30 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
if (read_bin_file(bin, &data, &bin_size))
goto err;
+ buf = data;
+
+ /* insert fmp payload header right before the payload */
+ if (fmp_ph_params->have_header) {
+ new_data = malloc(bin_size + sizeof(payload_header));
+ if (!new_data)
+ goto err;
+
+ payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE;
+ payload_header.header_size = sizeof(payload_header);
+ payload_header.fw_version = fmp_ph_params->fw_version;
+ payload_header.lowest_supported_version = 0; /* not used */
+ memcpy(new_data, &payload_header, sizeof(payload_header));
+ memcpy(new_data + sizeof(payload_header), data, bin_size);
+ buf = new_data;
+ bin_size += sizeof(payload_header);
+ }
+
/* first, calculate signature to determine its size */
if (privkey_file && cert_file) {
auth_context.key_file = privkey_file;
auth_context.cert_file = cert_file;
auth_context.auth.monotonic_count = mcount;
- auth_context.image_data = data;
+ auth_context.image_data = buf;
auth_context.image_size = bin_size;
if (create_auth_data(&auth_context)) {
@@ -536,7 +559,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
/*
* firmware binary
*/
- if (write_capsule_file(f, data, bin_size, "Firmware binary"))
+ if (write_capsule_file(f, buf, bin_size, "Firmware binary"))
goto err;
ret = 0;
@@ -545,6 +568,7 @@ err:
fclose(f);
free_sig_data(&auth_context);
free(data);
+ free(new_data);
return ret;
}
@@ -644,6 +668,7 @@ int main(int argc, char **argv)
unsigned long oemflags;
char *privkey_file, *cert_file;
int c, idx;
+ struct fmp_payload_header_params fmp_ph_params = { 0 };
guid = NULL;
index = 0;
@@ -679,6 +704,10 @@ int main(int argc, char **argv)
case 'I':
instance = strtoul(optarg, NULL, 0);
break;
+ case 'v':
+ fmp_ph_params.fw_version = strtoul(optarg, NULL, 0);
+ fmp_ph_params.have_header = true;
+ break;
case 'p':
if (privkey_file) {
fprintf(stderr,
@@ -751,7 +780,7 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
} else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
- index, instance, mcount, privkey_file,
+ index, instance, &fmp_ph_params, mcount, privkey_file,
cert_file, (uint16_t)oemflags) < 0) {
fprintf(stderr, "Creating firmware capsule failed\n");
exit(EXIT_FAILURE);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
` (2 preceding siblings ...)
2023-04-10 9:07 ` [PATCH v5 3/4] mkeficapsule: add FMP Payload Header Masahisa Kojima
@ 2023-04-10 9:07 ` Masahisa Kojima
2023-04-19 1:46 ` Simon Glass
2023-04-11 1:48 ` [PATCH v5 0/4] FMP versioning support Takahiro Akashi
4 siblings, 1 reply; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-10 9:07 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi, Masahisa Kojima
This test covers FMP versioning for both raw and FIT image,
and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v5:
- get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
.../test_capsule_firmware_fit.py | 187 ++++++++++++++++
.../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
.../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
.../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
test/py/tests/test_efi_capsule/version.dts | 27 +++
6 files changed, 822 insertions(+)
create mode 100644 test/py/tests/test_efi_capsule/version.dts
diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
index 4879f2b5c2..9655e5ec1f 100644
--- a/test/py/tests/test_efi_capsule/conftest.py
+++ b/test/py/tests/test_efi_capsule/conftest.py
@@ -70,6 +70,23 @@ def efi_capsule_data(request, u_boot_config):
'-out SIGNER2.crt -nodes -days 365'
% data_dir, shell=True)
+ # Update dtb adding version information
+ check_call('cd %s; '
+ 'cp %s/test/py/tests/test_efi_capsule/version.dts .'
+ % (data_dir, u_boot_config.source_dir), shell=True)
+ if capsule_auth_enabled:
+ check_call('cd %s; '
+ 'dtc -@ -I dts -O dtb -o version.dtbo version.dts; '
+ 'fdtoverlay -i test_sig.dtb '
+ '-o test_ver.dtb version.dtbo'
+ % (data_dir), shell=True)
+ else:
+ check_call('cd %s; '
+ 'dtc -@ -I dts -O dtb -o version.dtbo version.dts; '
+ 'fdtoverlay -i %s/arch/sandbox/dts/test.dtb '
+ '-o test_ver.dtb version.dtbo'
+ % (data_dir, u_boot_config.build_dir), shell=True)
+
# Create capsule files
# two regions: one for u-boot.bin and the other for u-boot.env
check_call('cd %s; echo -n u-boot:Old > u-boot.bin.old; echo -n u-boot:New > u-boot.bin.new; echo -n u-boot-env:Old > u-boot.env.old; echo -n u-boot-env:New > u-boot.env.new' % data_dir,
@@ -95,6 +112,26 @@ def efi_capsule_data(request, u_boot_config):
check_call('cd %s; %s/tools/mkeficapsule --index 1 --guid 058B7D83-50D5-4C47-A195-60D86AD341C4 uboot_bin_env.itb Test05' %
(data_dir, u_boot_config.build_dir),
shell=True)
+ check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 5 '
+ '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 u-boot.bin.new Test101' %
+ (data_dir, u_boot_config.build_dir),
+ shell=True)
+ check_call('cd %s; %s/tools/mkeficapsule --index 2 --fw-version 10 '
+ '--guid 5A7021F5-FEF2-48B4-AABA-832E777418C0 u-boot.env.new Test102' %
+ (data_dir, u_boot_config.build_dir),
+ shell=True)
+ check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 2 '
+ '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 u-boot.bin.new Test103' %
+ (data_dir, u_boot_config.build_dir),
+ shell=True)
+ check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 5 '
+ '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 uboot_bin_env.itb Test104' %
+ (data_dir, u_boot_config.build_dir),
+ shell=True)
+ check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 2 '
+ '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 uboot_bin_env.itb Test105' %
+ (data_dir, u_boot_config.build_dir),
+ shell=True)
if capsule_auth_enabled:
# raw firmware signed with proper key
@@ -131,6 +168,42 @@ def efi_capsule_data(request, u_boot_config):
'uboot_bin_env.itb Test14'
% (data_dir, u_boot_config.build_dir),
shell=True)
+ # raw firmware signed with proper key with version information
+ check_call('cd %s; '
+ '%s/tools/mkeficapsule --index 1 --monotonic-count 1 '
+ '--fw-version 5 '
+ '--private-key SIGNER.key --certificate SIGNER.crt '
+ '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 '
+ 'u-boot.bin.new Test111'
+ % (data_dir, u_boot_config.build_dir),
+ shell=True)
+ # raw firmware signed with *mal* key with version information
+ check_call('cd %s; '
+ '%s/tools/mkeficapsule --index 1 --monotonic-count 1 '
+ '--fw-version 2 '
+ '--private-key SIGNER.key --certificate SIGNER.crt '
+ '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 '
+ 'u-boot.bin.new Test112'
+ % (data_dir, u_boot_config.build_dir),
+ shell=True)
+ # FIT firmware signed with proper key with version information
+ check_call('cd %s; '
+ '%s/tools/mkeficapsule --index 1 --monotonic-count 1 '
+ '--fw-version 5 '
+ '--private-key SIGNER.key --certificate SIGNER.crt '
+ '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 '
+ 'uboot_bin_env.itb Test113'
+ % (data_dir, u_boot_config.build_dir),
+ shell=True)
+ # FIT firmware signed with *mal* key with version information
+ check_call('cd %s; '
+ '%s/tools/mkeficapsule --index 1 --monotonic-count 1 '
+ '--fw-version 2 '
+ '--private-key SIGNER.key --certificate SIGNER.crt '
+ '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 '
+ 'uboot_bin_env.itb Test114'
+ % (data_dir, u_boot_config.build_dir),
+ shell=True)
# Create a disk image with EFI system partition
check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat %s %s' %
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
index d28b53a1a1..e774a06d10 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
@@ -189,3 +189,190 @@ class TestEfiCapsuleFirmwareFit(object):
assert 'u-boot-env:Old' in ''.join(output)
else:
assert 'u-boot-env:New' in ''.join(output)
+
+ def test_efi_capsule_fw3(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 3 - Update U-Boot and U-Boot environment on SPI Flash with version information
+ 0x100000-0x150000: U-Boot binary (but dummy)
+ 0x150000-0x200000: U-Boot environment (but dummy)
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 3-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize contents
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 150000 10',
+ 'sf read 5000000 150000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test104' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test104 $filesize' % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test104' in ''.join(output)
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ capsule_auth = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_authenticate')
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot(expect_reset = capsule_early)
+
+ with u_boot_console.log.section('Test Case 3-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test104' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test104' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ if capsule_auth:
+ assert 'u-boot:Old' in ''.join(output)
+ else:
+ assert 'u-boot:New' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf read 4000000 150000 10',
+ 'md.b 4000000 10'])
+ if capsule_auth:
+ assert 'u-boot-env:Old' in ''.join(output)
+ else:
+ assert 'u-boot-env:New' in ''.join(output)
+
+ def test_efi_capsule_fw4(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 4 - Update U-Boot and U-Boot environment on SPI Flash with version information
+ but fw_version is lower than lowest_supported_version
+ No update should happen
+ 0x100000-0x150000: U-Boot binary (but dummy)
+ 0x150000-0x200000: U-Boot environment (but dummy)
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 4-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize contents
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 150000 10',
+ 'sf read 5000000 150000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test105' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test105 $filesize' % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test105' in ''.join(output)
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ capsule_auth = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_authenticate')
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot(expect_reset = capsule_early)
+
+ with u_boot_console.log.section('Test Case 4-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test105' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test105' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ # make sure capsule update failed
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:Old' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf read 4000000 150000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot-env:Old' in ''.join(output)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
index 92bfb14932..bd6419bcc3 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
@@ -292,3 +292,204 @@ class TestEfiCapsuleFirmwareRaw:
assert 'u-boot-env:Old' in ''.join(output)
else:
assert 'u-boot-env:New' in ''.join(output)
+
+ def test_efi_capsule_fw4(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """ Test Case 4
+ Update U-Boot on SPI Flash, raw image format with fw_version and lowest_supported_version
+ 0x100000-0x150000: U-Boot binary (but dummy)
+ 0x150000-0x200000: U-Boot environment (but dummy)
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 4-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize contents
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 150000 10',
+ 'sf read 5000000 150000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place the capsule files
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test101' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test101 $filesize' % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test101' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test102' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test102 $filesize' % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test102' in ''.join(output)
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ capsule_auth = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_authenticate')
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot(expect_reset = capsule_early)
+
+ with u_boot_console.log.section('Test Case 4-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test101' in ''.join(output)
+ assert 'Test102' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test101' not in ''.join(output)
+ assert 'Test102' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+
+ # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
+ assert '09D7CF52-0720-4710-91D1-08469B7FE9C8' in ''.join(output)
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT.
+ assert '5A7021F5-FEF2-48B4-AABA-832E777418C0' in ''.join(output)
+ assert 'ESRT: fw_version=10' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ if capsule_auth:
+ assert 'u-boot:Old' in ''.join(output)
+ else:
+ assert 'u-boot:New' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 150000 10',
+ 'md.b 4000000 10'])
+
+ if capsule_auth:
+ assert 'u-boot-env:Old' in ''.join(output)
+ else:
+ assert 'u-boot-env:New' in ''.join(output)
+
+ def test_efi_capsule_fw5(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """ Test Case 5
+ Update U-Boot on SPI Flash, raw image format with fw_version and lowest_supported_version
+ but fw_version is lower than lowest_supported_version
+ No update should happen
+ 0x100000-0x150000: U-Boot binary (but dummy)
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 5-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize contents
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place the capsule files
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test103' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test103 $filesize' % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test103' in ''.join(output)
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ capsule_auth = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_authenticate')
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot(expect_reset = capsule_early)
+
+ with u_boot_console.log.section('Test Case 5-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test103' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test103' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+
+ # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
+ assert '09D7CF52-0720-4710-91D1-08469B7FE9C8' in ''.join(output)
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT.
+ assert '5A7021F5-FEF2-48B4-AABA-832E777418C0' in ''.join(output)
+ assert 'ESRT: fw_version=10' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output)
+
+ # make sure capsule update failed
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:Old' in ''.join(output)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
index 8c2d616fd0..4696ad962a 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
@@ -257,3 +257,168 @@ class TestEfiCapsuleFirmwareSignedFit(object):
'sf read 4000000 100000 10',
'md.b 4000000 10'])
assert 'u-boot:Old' in ''.join(output)
+
+ def test_efi_capsule_auth4(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 4 - Update U-Boot on SPI Flash, FIT image format
+ 0x100000-0x150000: U-Boot binary (but dummy)
+
+ If the capsule is properly signed with version information,
+ the authentication should pass and the firmware be updated.
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 4-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize content
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old'
+ % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test113' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test113 $filesize'
+ % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test113' in ''.join(output)
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot()
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ with u_boot_console.log.section('Test Case 4-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test113' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test113' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:New' in ''.join(output)
+
+ def test_efi_capsule_auth5(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 5 - Update U-Boot on SPI Flash, FIT image format
+ 0x100000-0x150000: U-Boot binary (but dummy)
+
+ If the capsule is signed but fw_version is lower than lowest
+ supported version, the authentication should fail and the firmware
+ not be updated.
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 5-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize content
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old'
+ % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test114' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test114 $filesize'
+ % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test114' in ''.join(output)
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot()
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ with u_boot_console.log.section('Test Case 5-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test114' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test114' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:Old' in ''.join(output)
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
index 2bbaa9cc55..98ce9c9afd 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
@@ -254,3 +254,172 @@ class TestEfiCapsuleFirmwareSignedRaw(object):
'sf read 4000000 100000 10',
'md.b 4000000 10'])
assert 'u-boot:Old' in ''.join(output)
+
+ def test_efi_capsule_auth4(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 4 - Update U-Boot on SPI Flash, raw image format with version information
+ 0x100000-0x150000: U-Boot binary (but dummy)
+
+ If the capsule is properly signed, the authentication
+ should pass and the firmware be updated.
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 4-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+
+ output = u_boot_console.run_command('efidebug capsule esrt')
+
+
+
+ # initialize content
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old'
+ % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test111' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test111 $filesize'
+ % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test111' in ''.join(output)
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot()
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ with u_boot_console.log.section('Test Case 4-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test111' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test111' not in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:New' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ def test_efi_capsule_auth5(
+ self, u_boot_config, u_boot_console, efi_capsule_data):
+ """
+ Test Case 5 - Update U-Boot on SPI Flash, raw image format with version information
+ 0x100000-0x150000: U-Boot binary (but dummy)
+
+ If the capsule is signed but fw_version is lower than lowest
+ supported version, the authentication should fail and the firmware
+ not be updated.
+ """
+ disk_img = efi_capsule_data
+ with u_boot_console.log.section('Test Case 5-a, before reboot'):
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'printenv -e PlatformLangCodes', # workaround for terminal size determination
+ 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi',
+ 'efidebug boot order 1',
+ 'env set -e -nv -bs -rt OsIndications =0x0000000000000004',
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'env save'])
+
+ # initialize content
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'fatload host 0:1 4000000 %s/u-boot.bin.old'
+ % CAPSULE_DATA_DIR,
+ 'sf write 4000000 100000 10',
+ 'sf read 5000000 100000 10',
+ 'md.b 5000000 10'])
+ assert 'Old' in ''.join(output)
+
+ # place a capsule file
+ output = u_boot_console.run_command_list([
+ 'fatload host 0:1 4000000 %s/Test112' % CAPSULE_DATA_DIR,
+ 'fatwrite host 0:1 4000000 %s/Test112 $filesize'
+ % CAPSULE_INSTALL_DIR,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test112' in ''.join(output)
+
+ # reboot
+ mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
+ u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \
+ + '/test_ver.dtb'
+ u_boot_console.restart_uboot()
+
+ capsule_early = u_boot_config.buildconfig.get(
+ 'config_efi_capsule_on_disk_early')
+ with u_boot_console.log.section('Test Case 5-b, after reboot'):
+ if not capsule_early:
+ # make sure that dfu_alt_info exists even persistent variables
+ # are not available.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info '
+ '"sf 0:0=u-boot-bin raw 0x100000 '
+ '0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test112' in ''.join(output)
+
+ # need to run uefi command to initiate capsule handling
+ output = u_boot_console.run_command(
+ 'env print -e Capsule0000', wait_for_reboot = True)
+
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+ assert 'Test112' not in ''.join(output)
+
+ # make sure the dfu_alt_info exists because it is required for making ESRT.
+ output = u_boot_console.run_command_list([
+ 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+ 'efidebug capsule esrt'])
+ assert 'ESRT: fw_version=5' in ''.join(output)
+ assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
+
+ output = u_boot_console.run_command_list([
+ 'sf probe 0:0',
+ 'sf read 4000000 100000 10',
+ 'md.b 4000000 10'])
+ assert 'u-boot:Old' in ''.join(output)
diff --git a/test/py/tests/test_efi_capsule/version.dts b/test/py/tests/test_efi_capsule/version.dts
new file mode 100644
index 0000000000..0e544524e3
--- /dev/null
+++ b/test/py/tests/test_efi_capsule/version.dts
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+ firmware-version {
+ image1 {
+ lowest-supported-version = <3>;
+ fw-version = <5>;
+ image-index = <1>;
+ image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
+ };
+ image2 {
+ lowest-supported-version = <7>;
+ fw-version = <10>;
+ image-index = <2>;
+ image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
+ };
+ image3 {
+ lowest-supported-version = <3>;
+ fw-version = <5>;
+ image-index = <1>;
+ image-type-id = "3673B45D-6A7C-46F3-9E60-ADABB03F7937";
+ };
+ };
+};
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v5 0/4] FMP versioning support
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
` (3 preceding siblings ...)
2023-04-10 9:07 ` [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning Masahisa Kojima
@ 2023-04-11 1:48 ` Takahiro Akashi
2023-04-11 2:58 ` Masahisa Kojima
4 siblings, 1 reply; 20+ messages in thread
From: Takahiro Akashi @ 2023-04-11 1:48 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas
Hi Kojima-san,
On Mon, Apr 10, 2023 at 06:07:28PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP implementation. This series aims to add the versioning support
> in FMP.
>
> There is a major design change in v5.
> Until v4, the fw_version and lowest_supported_version are stored
> as a EFI variable. If the backing storage is a file we can't trust
> any of that information since anyone can tamper with the file,
> although the variables are defined as RO.
> With that, we store the version information in the device tree
> in v5. We can trust the information from dtb as long as the former
> stage boot loader verifies the image containing the dtb.
I have a basic question here.
You said that the former-stage boot loader is responsible for maintaining
the dtb with the correct firmware version to be passed to U-Boot.
But how can it obtain the new version number of firmware which is only
contained in a capsule file (and its header)?
Looking into you pytest, you simply always *reboot* the sandbox with
an already-modified dtb (test_ver.dtb).
(Please note that, at the reboot time, a capsule has not been
applied yet.)
I believe that your current approach is rather incomplete
as a workable solution.
-Takahiro Akashi
> The disadvantage of this design change is that we need to maintain
> the fw_version in both device tree and FMP Payload Header.
> It is inevitable since not all the capsule files contain the dtb.
>
> EDK II reference implementation utilizes the FMP Payload Header
> inserted right before the capsule payload. With this series,
> U-Boot also follows the EDK II implementation.
>
> Currently, there is no way to know the current running firmware
> version through the EFI interface. FMP->GetImageInfo() returns
> always 0 for the version number. So a user can not know that
> expected firmware is running after the capsule update.
>
> With this series applied, version number can be specified
> in the capsule file generation with mkeficapsule tool, then
> user can know the running firmware version through
> FMP->GetImageInfo() and ESRT.
>
> Note that this series does not mandate the FMP Payload Header,
> compatible with boards that are already using the existing
> U-Boot FMP implementation.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is set to 0 and this is the same behavior as existing FMP
> implementation.
>
> Major Changes in v5:
> - major design changes, versioning is implemented with
> device tree instead of EFI variable
>
> Major Changes in v4:
> - add python-based test
>
> Major Changes in v3:
> - exclude CONFIG_FWU_MULTI
>
> Masahisa Kojima (4):
> efi_loader: get version information from device tree
> efi_loader: check lowest supported version
> mkeficapsule: add FMP Payload Header
> test/py: efi_capsule: test for FMP versioning
>
> .../firmware/firmware-version.txt | 25 +++
> doc/mkeficapsule.1 | 10 +
> lib/efi_loader/efi_firmware.c | 187 +++++++++++++---
> test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
> .../test_capsule_firmware_fit.py | 187 ++++++++++++++++
> .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
> .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
> .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
> test/py/tests/test_efi_capsule/version.dts | 27 +++
> tools/eficapsule.h | 30 +++
> tools/mkeficapsule.c | 37 +++-
> 11 files changed, 1082 insertions(+), 29 deletions(-)
> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> create mode 100644 test/py/tests/test_efi_capsule/version.dts
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 0/4] FMP versioning support
2023-04-11 1:48 ` [PATCH v5 0/4] FMP versioning support Takahiro Akashi
@ 2023-04-11 2:58 ` Masahisa Kojima
0 siblings, 0 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-11 2:58 UTC (permalink / raw)
To: Takahiro Akashi, Masahisa Kojima, u-boot, Heinrich Schuchardt,
Ilias Apalodimas
Hi Akashi-san,
On Tue, 11 Apr 2023 at 10:48, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> Hi Kojima-san,
>
> On Mon, Apr 10, 2023 at 06:07:28PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP implementation. This series aims to add the versioning support
> > in FMP.
> >
> > There is a major design change in v5.
> > Until v4, the fw_version and lowest_supported_version are stored
> > as a EFI variable. If the backing storage is a file we can't trust
> > any of that information since anyone can tamper with the file,
> > although the variables are defined as RO.
> > With that, we store the version information in the device tree
> > in v5. We can trust the information from dtb as long as the former
> > stage boot loader verifies the image containing the dtb.
>
> I have a basic question here.
> You said that the former-stage boot loader is responsible for maintaining
> the dtb with the correct firmware version to be passed to U-Boot.
> But how can it obtain the new version number of firmware which is only
> contained in a capsule file (and its header)?
Yes, there is a problem that we need to maintain the fw_version
in both FMP Payload Header and dtb, it is not an ideal situation and
prone to errors.
On second thought, I need to change my approach again.
fw_version: specified in FMP Payload Header and stored in EFI variable
lowest_supported_version: stored in dtb
When the capsule update is performed, U-Boot gets the fw_version from
FMP Payload Header
of the ongoing capsule, then checks if the version is equal to or
greater than lowest_supported_version
read from dtb.
One disadvantage is that fw_version in the EFI variable can be tampered
if the backing storage is file, but it is not the critical issue.
Thanks,
Masahisa Kojima
>
> Looking into you pytest, you simply always *reboot* the sandbox with
> an already-modified dtb (test_ver.dtb).
> (Please note that, at the reboot time, a capsule has not been
> applied yet.)
>
> I believe that your current approach is rather incomplete
> as a workable solution.
>
> -Takahiro Akashi
>
> > The disadvantage of this design change is that we need to maintain
> > the fw_version in both device tree and FMP Payload Header.
> > It is inevitable since not all the capsule files contain the dtb.
> >
> > EDK II reference implementation utilizes the FMP Payload Header
> > inserted right before the capsule payload. With this series,
> > U-Boot also follows the EDK II implementation.
> >
> > Currently, there is no way to know the current running firmware
> > version through the EFI interface. FMP->GetImageInfo() returns
> > always 0 for the version number. So a user can not know that
> > expected firmware is running after the capsule update.
> >
> > With this series applied, version number can be specified
> > in the capsule file generation with mkeficapsule tool, then
> > user can know the running firmware version through
> > FMP->GetImageInfo() and ESRT.
> >
> > Note that this series does not mandate the FMP Payload Header,
> > compatible with boards that are already using the existing
> > U-Boot FMP implementation.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is set to 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Major Changes in v5:
> > - major design changes, versioning is implemented with
> > device tree instead of EFI variable
> >
> > Major Changes in v4:
> > - add python-based test
> >
> > Major Changes in v3:
> > - exclude CONFIG_FWU_MULTI
> >
> > Masahisa Kojima (4):
> > efi_loader: get version information from device tree
> > efi_loader: check lowest supported version
> > mkeficapsule: add FMP Payload Header
> > test/py: efi_capsule: test for FMP versioning
> >
> > .../firmware/firmware-version.txt | 25 +++
> > doc/mkeficapsule.1 | 10 +
> > lib/efi_loader/efi_firmware.c | 187 +++++++++++++---
> > test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
> > .../test_capsule_firmware_fit.py | 187 ++++++++++++++++
> > .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
> > .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
> > .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
> > test/py/tests/test_efi_capsule/version.dts | 27 +++
> > tools/eficapsule.h | 30 +++
> > tools/mkeficapsule.c | 37 +++-
> > 11 files changed, 1082 insertions(+), 29 deletions(-)
> > create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> > create mode 100644 test/py/tests/test_efi_capsule/version.dts
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning
2023-04-10 9:07 ` [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning Masahisa Kojima
@ 2023-04-19 1:46 ` Simon Glass
2023-04-20 5:17 ` Masahisa Kojima
0 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2023-04-19 1:46 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This test covers FMP versioning for both raw and FIT image,
> and both signed and non-signed capsule update.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v5:
> - get aligned to the device tree based versioning
>
> Newly created in v4
>
> test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
> .../test_capsule_firmware_fit.py | 187 ++++++++++++++++
> .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
> .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
> .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
> test/py/tests/test_efi_capsule/version.dts | 27 +++
> 6 files changed, 822 insertions(+)
> create mode 100644 test/py/tests/test_efi_capsule/version.dts
I hate to say this, but we must fix the reboot stuff here before
adding more code.
The test needs to tell U-Boot to do the update (e.g. via a command),
then continue. It should not reboot sandbox.I have said this before
but the feedback was not understood or taken. If you need help
designing this, please let me know. In fact I am happy to create a
patch for one of the tests if that is what it takes to convey this.
Also, while it is normal to have a fair bit of duplication in tests,
just to make them easier to read, it does make them hard to maintain,
and the duplication here seems very large.
We have the same code repeated but with different code style or
indentation. Sorry, but this is not setting us up for the future, in
terms of maintaining this code. Just search for 'env set -e -nv -bs
-rt OsIndications =0x0000000000000004' for example.
The common code needs to go in functions in a separate Python file, as
we have done for other tests. The goal should be to use the same code
for the same functions. Sure there will be some duplication, but it is
too much.
These fixes should happen before we accept any more non=trivial
patches to this code.
BTW I do wonder whether the test would be better written mostly in C,
since from what I can see, it mostly just runs commands. You can still
have a Python wrapper - see for example how test_vbe works. It uses
'ut xxx -f' to execute the C part of a Python test. You can do setup
in Python, then run the C test. What do you think?
Regards,
Simon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning
2023-04-19 1:46 ` Simon Glass
@ 2023-04-20 5:17 ` Masahisa Kojima
2023-04-24 19:42 ` Simon Glass
0 siblings, 1 reply; 20+ messages in thread
From: Masahisa Kojima @ 2023-04-20 5:17 UTC (permalink / raw)
To: Simon Glass
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi
Hi Simon,
On Wed, 19 Apr 2023 at 10:47, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This test covers FMP versioning for both raw and FIT image,
> > and both signed and non-signed capsule update.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v5:
> > - get aligned to the device tree based versioning
> >
> > Newly created in v4
> >
> > test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
> > .../test_capsule_firmware_fit.py | 187 ++++++++++++++++
> > .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
> > .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
> > .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
> > test/py/tests/test_efi_capsule/version.dts | 27 +++
> > 6 files changed, 822 insertions(+)
> > create mode 100644 test/py/tests/test_efi_capsule/version.dts
>
> I hate to say this, but we must fix the reboot stuff here before
> adding more code.
I read the previous discussion.
https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgit@localhost/
>
> The test needs to tell U-Boot to do the update (e.g. via a command),
> then continue. It should not reboot sandbox.I have said this before
> but the feedback was not understood or taken. If you need help
> designing this, please let me know. In fact I am happy to create a
> patch for one of the tests if that is what it takes to convey this.
Current efi capsule update test tries to verify the following behavior
stated in the UEFI spec v2.10, so the reboot is required to trigger
the capsule update.
=== quote from UEFI v2.10 P.243
8.5.5 Delivery of Capsules via file on Mass Storage Device
As an alternative to the UpdateCapsule() runtime API, capsules of any
type supported by platform may also be delivered to firmware via a
file within the EFI system partition on the mass storage device
targeted for boot. Capsules staged using this method are processed on
the next system restart. This method is only available when booting
from mass storage devices which are formatted with GPT and contain an
EFI System Partition in the device image. System firmware will search
for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
bit in OsIndications is set as described in Exchanging information
between the OS and Firmware.
===
>
> Also, while it is normal to have a fair bit of duplication in tests,
> just to make them easier to read, it does make them hard to m
aintain,
> and the duplication here seems very large.
>
> We have the same code repeated but with different code style or
> indentation. Sorry, but this is not setting us up for the future, in
> terms of maintaining this code. Just search for 'env set -e -nv -bs
> -rt OsIndications =0x0000000000000004' for example.
>
> The common code needs to go in functions in a separate Python file, as
> we have done for other tests. The goal should be to use the same code
> for the same functions. Sure there will be some duplication, but it is
> too much.
>
> These fixes should happen before we accept any more non=trivial
> patches to this code.
I agree, I will try to create common functions to reduce code duplication.
>
> BTW I do wonder whether the test would be better written mostly in C,
> since from what I can see, it mostly just runs commands. You can still
> have a Python wrapper - see for example how test_vbe works. It uses
> 'ut xxx -f' to execute the C part of a Python test. You can do setup
> in Python, then run the C test. What do you think?
It is just my opinion, if the test requires some setup in Python, I like Python
to write the test cases as far as tests can be implemented in Python,
because we can maintain the test code in one place.
Thanks,
Masahisa Kojima
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning
2023-04-20 5:17 ` Masahisa Kojima
@ 2023-04-24 19:42 ` Simon Glass
0 siblings, 0 replies; 20+ messages in thread
From: Simon Glass @ 2023-04-24 19:42 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi
Hi Masahisa,
On Wed, 19 Apr 2023 at 23:17, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 19 Apr 2023 at 10:47, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > This test covers FMP versioning for both raw and FIT image,
> > > and both signed and non-signed capsule update.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > Changes in v5:
> > > - get aligned to the device tree based versioning
> > >
> > > Newly created in v4
> > >
> > > test/py/tests/test_efi_capsule/conftest.py | 73 +++++++
> > > .../test_capsule_firmware_fit.py | 187 ++++++++++++++++
> > > .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++
> > > .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++
> > > .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++
> > > test/py/tests/test_efi_capsule/version.dts | 27 +++
> > > 6 files changed, 822 insertions(+)
> > > create mode 100644 test/py/tests/test_efi_capsule/version.dts
> >
> > I hate to say this, but we must fix the reboot stuff here before
> > adding more code.
>
> I read the previous discussion.
> https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgit@localhost/
>
> >
> > The test needs to tell U-Boot to do the update (e.g. via a command),
> > then continue. It should not reboot sandbox.I have said this before
> > but the feedback was not understood or taken. If you need help
> > designing this, please let me know. In fact I am happy to create a
> > patch for one of the tests if that is what it takes to convey this.
>
> Current efi capsule update test tries to verify the following behavior
> stated in the UEFI spec v2.10, so the reboot is required to trigger
> the capsule update.
>
> === quote from UEFI v2.10 P.243
> 8.5.5 Delivery of Capsules via file on Mass Storage Device
>
> As an alternative to the UpdateCapsule() runtime API, capsules of any
> type supported by platform may also be delivered to firmware via a
> file within the EFI system partition on the mass storage device
> targeted for boot. Capsules staged using this method are processed on
> the next system restart. This method is only available when booting
> from mass storage devices which are formatted with GPT and contain an
> EFI System Partition in the device image. System firmware will search
> for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> bit in OsIndications is set as described in Exchanging information
> between the OS and Firmware.
> ===
Let's agree for the moment that the spec actually requires a reboot to
initial the update. For a test, we can split it into three parts:
- setting up the update
- triggering the update
- processing the update
We can write separate tests for each of these:
- test that the update is set up correctly
- test that the update is triggered on a reboot, if one is present
- test that the update is processed correctly
There is no need to bring them all together.
>
> >
> > Also, while it is normal to have a fair bit of duplication in tests,
> > just to make them easier to read, it does make them hard to m
> aintain,
> > and the duplication here seems very large.
> >
> > We have the same code repeated but with different code style or
> > indentation. Sorry, but this is not setting us up for the future, in
> > terms of maintaining this code. Just search for 'env set -e -nv -bs
> > -rt OsIndications =0x0000000000000004' for example.
> >
> > The common code needs to go in functions in a separate Python file, as
> > we have done for other tests. The goal should be to use the same code
> > for the same functions. Sure there will be some duplication, but it is
> > too much.
> >
> > These fixes should happen before we accept any more non=trivial
> > patches to this code.
>
> I agree, I will try to create common functions to reduce code duplication.
OK great.
>
> >
> > BTW I do wonder whether the test would be better written mostly in C,
> > since from what I can see, it mostly just runs commands. You can still
> > have a Python wrapper - see for example how test_vbe works. It uses
> > 'ut xxx -f' to execute the C part of a Python test. You can do setup
> > in Python, then run the C test. What do you think?
>
> It is just my opinion, if the test requires some setup in Python, I like Python
> to write the test cases as far as tests can be implemented in Python,
> because we can maintain the test code in one place.
Actually almost all the test code can go in C. See here for the trade-offs [1].
Regards,
Simon
[1] https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
@ 2023-04-27 6:09 ` Heinrich Schuchardt
2023-05-08 8:15 ` Masahisa Kojima
2023-04-27 23:35 ` Simon Glass
1 sibling, 1 reply; 20+ messages in thread
From: Heinrich Schuchardt @ 2023-04-27 6:09 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: Ilias Apalodimas, Takahiro Akashi, u-boot
On 4/10/23 11:07, Masahisa Kojima wrote:
> Current FMP->GetImageInfo() always return 0 for the firmware
> version, user can not identify which firmware version is currently
> running through the EFI interface.
>
> This commit gets the version information from device tree,
> then fills the firmware version, lowest supported version
> in FMP->GetImageInfo().
>
> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v5:
> - newly implement a device tree based versioning
>
> .../firmware/firmware-version.txt | 25 ++++++++
> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> 2 files changed, 84 insertions(+), 4 deletions(-)
> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
>
> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> new file mode 100644
> index 0000000000..6112de4a1d
> --- /dev/null
> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> @@ -0,0 +1,25 @@
> +firmware-version bindings
> +-------------------------------
> +
> +Required properties:
> +- image-type-id : guid for image blob type
> +- image-index : image index
> +- fw-version : firmware version
> +- lowest-supported-version : lowest supported version
> +
> +Example:
> +
> + firmware-version {
> + image1 {
> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> + image-index = <1>;
> + fw-version = <5>;
> + lowest-supported-version = <3>;
> + };
> + image2 {
> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> + image-index = <2>;
> + fw-version = <10>;
> + lowest-supported-version = <7>;
> + };
> + };
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 93e2b01c07..1c6ef468bf 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> return EFI_EXIT(EFI_UNSUPPORTED);
> }
>
> +/**
> + * efi_firmware_get_firmware_version - get firmware version from dtb
> + * @image_index: Image index
> + * @image_type_id: Image type id
> + * @fw_version: Pointer to store the version number
> + * @lsv: Pointer to store the lowest supported version
> + *
> + * Authenticate the capsule if authentication is enabled.
> + * The image pointer and the image size are updated in case of success.
> + */
> +void efi_firmware_get_firmware_version(u8 image_index,
> + efi_guid_t *image_type_id,
> + u32 *fw_version, u32 *lsv)
> +{
> + const void *fdt = gd->fdt_blob;
> + const fdt32_t *val;
> + const char *guid_str;
> + int len, offset, index;
> + int parent;
> +
> + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> + if (parent < 0)
> + return;
> +
> + fdt_for_each_subnode(offset, fdt, parent) {
> + efi_guid_t guid;
> +
> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> + if (!guid_str)
> + continue;
> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> +
> + val = fdt_getprop(fdt, offset, "image-index", &len);
> + if (!val)
> + continue;
> + index = fdt32_to_cpu(*val);
> +
> + if (!guidcmp(&guid, image_type_id) && index == image_index) {
> + val = fdt_getprop(fdt, offset, "fw-version", &len);
> + if (val)
> + *fw_version = fdt32_to_cpu(*val);
> +
> + val = fdt_getprop(fdt, offset,
> + "lowest-supported-version", &len);
> + if (val)
> + *lsv = fdt32_to_cpu(*val);
> + }
> + }
> +}
> +
> /**
> * efi_fill_image_desc_array - populate image descriptor array
> * @image_info_size: Size of @image_info
> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> *package_version_name = NULL; /* not supported */
>
> for (i = 0; i < num_image_type_guids; i++) {
Currently we define num_image_type_guids per board in a C file. Using
the same line of code once per board makes no sense to me. Please, move
the definition of that variable to lib/efi_loader/efi_firmware.c.
> + u32 fw_version = 0;
> + u32 lowest_supported_version = 0;
These assignments should be in efi_firmware_get_firmware_version.
> +
> image_info[i].image_index = fw_array[i].image_index;
Why did we ever introduce the field image_index? Please, eliminate it it
as the GUID is always sufficient to identify an image.
> image_info[i].image_type_id = fw_array[i].image_type_id;
> image_info[i].image_id = fw_array[i].image_index;
>
> image_info[i].image_id_name = fw_array[i].fw_name;
> -
> - image_info[i].version = 0; /* not supported */
> + efi_firmware_get_firmware_version(fw_array[i].image_index,
> + &fw_array[i].image_type_id,
> + &fw_version,
> + &lowest_supported_version);
This interface makes no sense to me. We expect images with specific
GUIDs and should not care about images with other GUIDs that may
additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
> + image_info[i].version = fw_version;
> image_info[i].version_name = NULL; /* not supported */
Please, add the missing functionality to
efi_firmware_get_firmware_version().
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
will simplify the code.
Best regards
Heinrich
> image_info[i].size = 0;
> image_info[i].attributes_supported =
> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
> image_info[0].attributes_setting |=
> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
>
> - image_info[i].lowest_supported_image_version = 0;
> + image_info[i].lowest_supported_image_version = lowest_supported_version;
> image_info[i].last_attempt_version = 0;
> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> image_info[i].hardware_instance = 1;
> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> descriptor_version, descriptor_count,
> descriptor_size, package_version,
> package_version_name);
> -
> return EFI_EXIT(ret);
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
2023-04-27 6:09 ` Heinrich Schuchardt
@ 2023-04-27 23:35 ` Simon Glass
2023-04-28 4:07 ` Heinrich Schuchardt
1 sibling, 1 reply; 20+ messages in thread
From: Simon Glass @ 2023-04-27 23:35 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Takahiro Akashi
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Current FMP->GetImageInfo() always return 0 for the firmware
> version, user can not identify which firmware version is currently
> running through the EFI interface.
>
> This commit gets the version information from device tree,
> then fills the firmware version, lowest supported version
> in FMP->GetImageInfo().
>
> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v5:
> - newly implement a device tree based versioning
>
> .../firmware/firmware-version.txt | 25 ++++++++
> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> 2 files changed, 84 insertions(+), 4 deletions(-)
> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
>
> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> new file mode 100644
> index 0000000000..6112de4a1d
> --- /dev/null
> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> @@ -0,0 +1,25 @@
> +firmware-version bindings
> +-------------------------------
> +
> +Required properties:
> +- image-type-id : guid for image blob type
> +- image-index : image index
> +- fw-version : firmware version
> +- lowest-supported-version : lowest supported version
> +
> +Example:
> +
> + firmware-version {
> + image1 {
> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
Nit: please use lower-case hex and add a decoder to uuid.c so we can
look it up when debugging.
> + image-index = <1>;
> + fw-version = <5>;
> + lowest-supported-version = <3>;
> + };
> + image2 {
> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> + image-index = <2>;
> + fw-version = <10>;
> + lowest-supported-version = <7>;
> + };
> + };
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-27 23:35 ` Simon Glass
@ 2023-04-28 4:07 ` Heinrich Schuchardt
2023-05-10 20:46 ` Simon Glass
0 siblings, 1 reply; 20+ messages in thread
From: Heinrich Schuchardt @ 2023-04-28 4:07 UTC (permalink / raw)
To: Simon Glass, Masahisa Kojima; +Cc: u-boot, Ilias Apalodimas, Takahiro Akashi
Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Masahisa,
>
>On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
><masahisa.kojima@linaro.org> wrote:
>>
>> Current FMP->GetImageInfo() always return 0 for the firmware
>> version, user can not identify which firmware version is currently
>> running through the EFI interface.
>>
>> This commit gets the version information from device tree,
>> then fills the firmware version, lowest supported version
>> in FMP->GetImageInfo().
>>
>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
>>
>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>> ---
>> Changes in v5:
>> - newly implement a device tree based versioning
>>
>> .../firmware/firmware-version.txt | 25 ++++++++
>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
>> 2 files changed, 84 insertions(+), 4 deletions(-)
>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
>>
>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
>> new file mode 100644
>> index 0000000000..6112de4a1d
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
>> @@ -0,0 +1,25 @@
>> +firmware-version bindings
>> +-------------------------------
>> +
>> +Required properties:
>> +- image-type-id : guid for image blob type
>> +- image-index : image index
>> +- fw-version : firmware version
>> +- lowest-supported-version : lowest supported version
>> +
>> +Example:
>> +
>> + firmware-version {
>> + image1 {
>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
>
>Nit: please use lower-case hex and add a decoder to uuid.c so we can
>look it up when debugging.
The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
Instead we should define a string per firmware image. We already have a field for it:
fw_array[i].fw_name
Best regards
Heinrich
>
>> + image-index = <1>;
>> + fw-version = <5>;
>> + lowest-supported-version = <3>;
>> + };
>> + image2 {
>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
>> + image-index = <2>;
>> + fw-version = <10>;
>> + lowest-supported-version = <7>;
>> + };
>> + };
>[..]
>
>Regards,
>Simon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-27 6:09 ` Heinrich Schuchardt
@ 2023-05-08 8:15 ` Masahisa Kojima
2023-05-08 9:44 ` Heinrich Schuchardt
0 siblings, 1 reply; 20+ messages in thread
From: Masahisa Kojima @ 2023-05-08 8:15 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, Takahiro Akashi, u-boot
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/10/23 11:07, Masahisa Kojima wrote:
> > Current FMP->GetImageInfo() always return 0 for the firmware
> > version, user can not identify which firmware version is currently
> > running through the EFI interface.
> >
> > This commit gets the version information from device tree,
> > then fills the firmware version, lowest supported version
> > in FMP->GetImageInfo().
> >
> > Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v5:
> > - newly implement a device tree based versioning
> >
> > .../firmware/firmware-version.txt | 25 ++++++++
> > lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> > 2 files changed, 84 insertions(+), 4 deletions(-)
> > create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> >
> > diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> > new file mode 100644
> > index 0000000000..6112de4a1d
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> > @@ -0,0 +1,25 @@
> > +firmware-version bindings
> > +-------------------------------
> > +
> > +Required properties:
> > +- image-type-id : guid for image blob type
> > +- image-index : image index
> > +- fw-version : firmware version
> > +- lowest-supported-version : lowest supported version
> > +
> > +Example:
> > +
> > + firmware-version {
> > + image1 {
> > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> > + image-index = <1>;
> > + fw-version = <5>;
> > + lowest-supported-version = <3>;
> > + };
> > + image2 {
> > + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> > + image-index = <2>;
> > + fw-version = <10>;
> > + lowest-supported-version = <7>;
> > + };
> > + };
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 93e2b01c07..1c6ef468bf 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > return EFI_EXIT(EFI_UNSUPPORTED);
> > }
> >
> > +/**
> > + * efi_firmware_get_firmware_version - get firmware version from dtb
> > + * @image_index: Image index
> > + * @image_type_id: Image type id
> > + * @fw_version: Pointer to store the version number
> > + * @lsv: Pointer to store the lowest supported version
> > + *
> > + * Authenticate the capsule if authentication is enabled.
> > + * The image pointer and the image size are updated in case of success.
> > + */
> > +void efi_firmware_get_firmware_version(u8 image_index,
> > + efi_guid_t *image_type_id,
> > + u32 *fw_version, u32 *lsv)
> > +{
> > + const void *fdt = gd->fdt_blob;
> > + const fdt32_t *val;
> > + const char *guid_str;
> > + int len, offset, index;
> > + int parent;
> > +
> > + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> > + if (parent < 0)
> > + return;
> > +
> > + fdt_for_each_subnode(offset, fdt, parent) {
> > + efi_guid_t guid;
> > +
> > + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> > + if (!guid_str)
> > + continue;
> > + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> > +
> > + val = fdt_getprop(fdt, offset, "image-index", &len);
> > + if (!val)
> > + continue;
> > + index = fdt32_to_cpu(*val);
> > +
> > + if (!guidcmp(&guid, image_type_id) && index == image_index) {
> > + val = fdt_getprop(fdt, offset, "fw-version", &len);
> > + if (val)
> > + *fw_version = fdt32_to_cpu(*val);
> > +
> > + val = fdt_getprop(fdt, offset,
> > + "lowest-supported-version", &len);
> > + if (val)
> > + *lsv = fdt32_to_cpu(*val);
> > + }
> > + }
> > +}
> > +
> > /**
> > * efi_fill_image_desc_array - populate image descriptor array
> > * @image_info_size: Size of @image_info
> > @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> > *package_version_name = NULL; /* not supported */
> >
> > for (i = 0; i < num_image_type_guids; i++) {
>
> Currently we define num_image_type_guids per board in a C file. Using
> the same line of code once per board makes no sense to me. Please, move
> the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
fw_images[] array is also defined in each board file,
so we can not simply move num_image_type_guids into
lib/efi_loader/efi_firmware.c.
And fw_images[] array is abstracted by struct efi_capsule_update_info,
so I think
we should not extract the fw_images[] array.
>
> > + u32 fw_version = 0;
> > + u32 lowest_supported_version = 0;
>
> These assignments should be in efi_firmware_get_firmware_version.
OK.
>
> > +
> > image_info[i].image_index = fw_array[i].image_index;
>
> Why did we ever introduce the field image_index? Please, eliminate it it
> as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification.
UEFI specification "23.1.2
EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the
device. The number is between 1 and
DescriptorCount.
>
> > image_info[i].image_type_id = fw_array[i].image_type_id;
> > image_info[i].image_id = fw_array[i].image_index;
> >
> > image_info[i].image_id_name = fw_array[i].fw_name;
> > -
> > - image_info[i].version = 0; /* not supported */
> > + efi_firmware_get_firmware_version(fw_array[i].image_index,
> > + &fw_array[i].image_type_id,
> > + &fw_version,
> > + &lowest_supported_version);
>
> This interface makes no sense to me. We expect images with specific
> GUIDs and should not care about images with other GUIDs that may
> additionally exist in the capsule.
>
> So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is
fw_array[i].image_type_id.
>
> > + image_info[i].version = fw_version;
> > image_info[i].version_name = NULL; /* not supported */
>
> Please, add the missing functionality to
> efi_firmware_get_firmware_version().
Does it mean we need to support version_name?
I can add a version_name in dtb.
>
> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
> will simplify the code.
OK.
Thank you for your review.
Regards,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> > image_info[i].size = 0;
> > image_info[i].attributes_supported =
> > @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
> > image_info[0].attributes_setting |=
> > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> >
> > - image_info[i].lowest_supported_image_version = 0;
> > + image_info[i].lowest_supported_image_version = lowest_supported_version;
> > image_info[i].last_attempt_version = 0;
> > image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > image_info[i].hardware_instance = 1;
> > @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> > descriptor_version, descriptor_count,
> > descriptor_size, package_version,
> > package_version_name);
> > -
> > return EFI_EXIT(ret);
> > }
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-05-08 8:15 ` Masahisa Kojima
@ 2023-05-08 9:44 ` Heinrich Schuchardt
2023-05-09 9:57 ` Masahisa Kojima
0 siblings, 1 reply; 20+ messages in thread
From: Heinrich Schuchardt @ 2023-05-08 9:44 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: Ilias Apalodimas, Takahiro Akashi, u-boot
On 5/8/23 10:15, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/10/23 11:07, Masahisa Kojima wrote:
>>> Current FMP->GetImageInfo() always return 0 for the firmware
>>> version, user can not identify which firmware version is currently
>>> running through the EFI interface.
>>>
>>> This commit gets the version information from device tree,
>>> then fills the firmware version, lowest supported version
>>> in FMP->GetImageInfo().
>>>
>>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>> Changes in v5:
>>> - newly implement a device tree based versioning
>>>
>>> .../firmware/firmware-version.txt | 25 ++++++++
>>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
>>> 2 files changed, 84 insertions(+), 4 deletions(-)
>>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
>>>
>>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
>>> new file mode 100644
>>> index 0000000000..6112de4a1d
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
>>> @@ -0,0 +1,25 @@
>>> +firmware-version bindings
>>> +-------------------------------
>>> +
>>> +Required properties:
>>> +- image-type-id : guid for image blob type
>>> +- image-index : image index
>>> +- fw-version : firmware version
>>> +- lowest-supported-version : lowest supported version
>>> +
>>> +Example:
>>> +
>>> + firmware-version {
>>> + image1 {
>>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
>>> + image-index = <1>;
>>> + fw-version = <5>;
>>> + lowest-supported-version = <3>;
>>> + };
>>> + image2 {
>>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
>>> + image-index = <2>;
>>> + fw-version = <10>;
>>> + lowest-supported-version = <7>;
>>> + };
>>> + };
>>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
>>> index 93e2b01c07..1c6ef468bf 100644
>>> --- a/lib/efi_loader/efi_firmware.c
>>> +++ b/lib/efi_loader/efi_firmware.c
>>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
>>> return EFI_EXIT(EFI_UNSUPPORTED);
>>> }
>>>
>>> +/**
>>> + * efi_firmware_get_firmware_version - get firmware version from dtb
>>> + * @image_index: Image index
>>> + * @image_type_id: Image type id
>>> + * @fw_version: Pointer to store the version number
>>> + * @lsv: Pointer to store the lowest supported version
>>> + *
>>> + * Authenticate the capsule if authentication is enabled.
>>> + * The image pointer and the image size are updated in case of success.
>>> + */
>>> +void efi_firmware_get_firmware_version(u8 image_index,
>>> + efi_guid_t *image_type_id,
>>> + u32 *fw_version, u32 *lsv)
>>> +{
>>> + const void *fdt = gd->fdt_blob;
>>> + const fdt32_t *val;
>>> + const char *guid_str;
>>> + int len, offset, index;
>>> + int parent;
>>> +
>>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
>>> + if (parent < 0)
>>> + return;
>>> +
>>> + fdt_for_each_subnode(offset, fdt, parent) {
>>> + efi_guid_t guid;
>>> +
>>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
>>> + if (!guid_str)
>>> + continue;
>>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
>>> +
>>> + val = fdt_getprop(fdt, offset, "image-index", &len);
>>> + if (!val)
>>> + continue;
>>> + index = fdt32_to_cpu(*val);
>>> +
>>> + if (!guidcmp(&guid, image_type_id) && index == image_index) {
>>> + val = fdt_getprop(fdt, offset, "fw-version", &len);
>>> + if (val)
>>> + *fw_version = fdt32_to_cpu(*val);
>>> +
>>> + val = fdt_getprop(fdt, offset,
>>> + "lowest-supported-version", &len);
>>> + if (val)
>>> + *lsv = fdt32_to_cpu(*val);
>>> + }
>>> + }
>>> +}
>>> +
>>> /**
>>> * efi_fill_image_desc_array - populate image descriptor array
>>> * @image_info_size: Size of @image_info
>>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
>>> *package_version_name = NULL; /* not supported */
>>>
>>> for (i = 0; i < num_image_type_guids; i++) {
>>
>> Currently we define num_image_type_guids per board in a C file. Using
>> the same line of code once per board makes no sense to me. Please, move
>> the definition of that variable to lib/efi_loader/efi_firmware.c.
>
> Sorry for the late reply.
>
> num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
> fw_images[] array is also defined in each board file,
> so we can not simply move num_image_type_guids into
> lib/efi_loader/efi_firmware.c.
Why can't we have
int num_image_type_guids = ARRAY_SIZE(fw_images);
in lib/efi_loader/efi_firmware.c?
Best regards
Heinrich
>
> And fw_images[] array is abstracted by struct efi_capsule_update_info,
> so I think
> we should not extract the fw_images[] array.
>
>>
>>> + u32 fw_version = 0;
>>> + u32 lowest_supported_version = 0;
>>
>> These assignments should be in efi_firmware_get_firmware_version.
>
> OK.
>
>>
>>> +
>>> image_info[i].image_index = fw_array[i].image_index;
>>
>> Why did we ever introduce the field image_index? Please, eliminate it it
>> as the GUID is always sufficient to identify an image.
>
> This is derived from the UEFI specification.
> UEFI specification "23.1.2
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
> and ImageTypeId(guid).
>
> ImageIndex: A unique number identifying the firmware image within the
> device. The number is between 1 and
> DescriptorCount.
>
>>
>>> image_info[i].image_type_id = fw_array[i].image_type_id;
>>> image_info[i].image_id = fw_array[i].image_index;
>>>
>>> image_info[i].image_id_name = fw_array[i].fw_name;
>>> -
>>> - image_info[i].version = 0; /* not supported */
>>> + efi_firmware_get_firmware_version(fw_array[i].image_index,
>>> + &fw_array[i].image_type_id,
>>> + &fw_version,
>>> + &lowest_supported_version);
>>
>> This interface makes no sense to me. We expect images with specific
>> GUIDs and should not care about images with other GUIDs that may
>> additionally exist in the capsule.
>>
>> So you must pass the expected GUID as input variable here.
>
> I don't clearly understand this comment, but the expected GUID is
> fw_array[i].image_type_id.
>
>>
>>> + image_info[i].version = fw_version;
>>> image_info[i].version_name = NULL; /* not supported */
>>
>> Please, add the missing functionality to
>> efi_firmware_get_firmware_version().
>
> Does it mean we need to support version_name?
> I can add a version_name in dtb.
>
>>
>> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
>> will simplify the code.
>
> OK.
>
> Thank you for your review.
>
> Regards,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> image_info[i].size = 0;
>>> image_info[i].attributes_supported =
>>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
>>> image_info[0].attributes_setting |=
>>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
>>>
>>> - image_info[i].lowest_supported_image_version = 0;
>>> + image_info[i].lowest_supported_image_version = lowest_supported_version;
>>> image_info[i].last_attempt_version = 0;
>>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
>>> image_info[i].hardware_instance = 1;
>>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
>>> descriptor_version, descriptor_count,
>>> descriptor_size, package_version,
>>> package_version_name);
>>> -
>>> return EFI_EXIT(ret);
>>> }
>>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-05-08 9:44 ` Heinrich Schuchardt
@ 2023-05-09 9:57 ` Masahisa Kojima
2023-05-10 0:28 ` Takahiro Akashi
0 siblings, 1 reply; 20+ messages in thread
From: Masahisa Kojima @ 2023-05-09 9:57 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, Takahiro Akashi, u-boot
On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/8/23 10:15, Masahisa Kojima wrote:
> > Hi Heinrich,
> >
> > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/10/23 11:07, Masahisa Kojima wrote:
> >>> Current FMP->GetImageInfo() always return 0 for the firmware
> >>> version, user can not identify which firmware version is currently
> >>> running through the EFI interface.
> >>>
> >>> This commit gets the version information from device tree,
> >>> then fills the firmware version, lowest supported version
> >>> in FMP->GetImageInfo().
> >>>
> >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> ---
> >>> Changes in v5:
> >>> - newly implement a device tree based versioning
> >>>
> >>> .../firmware/firmware-version.txt | 25 ++++++++
> >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> >>> 2 files changed, 84 insertions(+), 4 deletions(-)
> >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> >>>
> >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> >>> new file mode 100644
> >>> index 0000000000..6112de4a1d
> >>> --- /dev/null
> >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> >>> @@ -0,0 +1,25 @@
> >>> +firmware-version bindings
> >>> +-------------------------------
> >>> +
> >>> +Required properties:
> >>> +- image-type-id : guid for image blob type
> >>> +- image-index : image index
> >>> +- fw-version : firmware version
> >>> +- lowest-supported-version : lowest supported version
> >>> +
> >>> +Example:
> >>> +
> >>> + firmware-version {
> >>> + image1 {
> >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> >>> + image-index = <1>;
> >>> + fw-version = <5>;
> >>> + lowest-supported-version = <3>;
> >>> + };
> >>> + image2 {
> >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> >>> + image-index = <2>;
> >>> + fw-version = <10>;
> >>> + lowest-supported-version = <7>;
> >>> + };
> >>> + };
> >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> >>> index 93e2b01c07..1c6ef468bf 100644
> >>> --- a/lib/efi_loader/efi_firmware.c
> >>> +++ b/lib/efi_loader/efi_firmware.c
> >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> >>> return EFI_EXIT(EFI_UNSUPPORTED);
> >>> }
> >>>
> >>> +/**
> >>> + * efi_firmware_get_firmware_version - get firmware version from dtb
> >>> + * @image_index: Image index
> >>> + * @image_type_id: Image type id
> >>> + * @fw_version: Pointer to store the version number
> >>> + * @lsv: Pointer to store the lowest supported version
> >>> + *
> >>> + * Authenticate the capsule if authentication is enabled.
> >>> + * The image pointer and the image size are updated in case of success.
> >>> + */
> >>> +void efi_firmware_get_firmware_version(u8 image_index,
> >>> + efi_guid_t *image_type_id,
> >>> + u32 *fw_version, u32 *lsv)
> >>> +{
> >>> + const void *fdt = gd->fdt_blob;
> >>> + const fdt32_t *val;
> >>> + const char *guid_str;
> >>> + int len, offset, index;
> >>> + int parent;
> >>> +
> >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> >>> + if (parent < 0)
> >>> + return;
> >>> +
> >>> + fdt_for_each_subnode(offset, fdt, parent) {
> >>> + efi_guid_t guid;
> >>> +
> >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> >>> + if (!guid_str)
> >>> + continue;
> >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> >>> +
> >>> + val = fdt_getprop(fdt, offset, "image-index", &len);
> >>> + if (!val)
> >>> + continue;
> >>> + index = fdt32_to_cpu(*val);
> >>> +
> >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) {
> >>> + val = fdt_getprop(fdt, offset, "fw-version", &len);
> >>> + if (val)
> >>> + *fw_version = fdt32_to_cpu(*val);
> >>> +
> >>> + val = fdt_getprop(fdt, offset,
> >>> + "lowest-supported-version", &len);
> >>> + if (val)
> >>> + *lsv = fdt32_to_cpu(*val);
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> /**
> >>> * efi_fill_image_desc_array - populate image descriptor array
> >>> * @image_info_size: Size of @image_info
> >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> >>> *package_version_name = NULL; /* not supported */
> >>>
> >>> for (i = 0; i < num_image_type_guids; i++) {
> >>
> >> Currently we define num_image_type_guids per board in a C file. Using
> >> the same line of code once per board makes no sense to me. Please, move
> >> the definition of that variable to lib/efi_loader/efi_firmware.c.
> >
> > Sorry for the late reply.
> >
> > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
> > fw_images[] array is also defined in each board file,
> > so we can not simply move num_image_type_guids into
> > lib/efi_loader/efi_firmware.c.
>
> Why can't we have
>
> int num_image_type_guids = ARRAY_SIZE(fw_images);
>
> in lib/efi_loader/efi_firmware.c?
At first thought, I thought it was a matter of abstraction.
But there is a compilation error when we expose fw_images[].
fw_images[] array is initialized in each board file,
and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c
will cause compilation failure.
We need to specify the array size when fw_images is exposed,
for example:
extern struct efi_fw_image fw_images[2];
but currently there is no method to pre-define the fw_images[] array size,
it is board specific.
We can define the macro to indicate the array size or having
sentinel in the fw_images[] array, but I think the current
implementation is simpler,
I would like to keep the current implementation.
Correct me if I'm wrong.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > And fw_images[] array is abstracted by struct efi_capsule_update_info,
> > so I think
> > we should not extract the fw_images[] array.
> >
> >>
> >>> + u32 fw_version = 0;
> >>> + u32 lowest_supported_version = 0;
> >>
> >> These assignments should be in efi_firmware_get_firmware_version.
> >
> > OK.
> >
> >>
> >>> +
> >>> image_info[i].image_index = fw_array[i].image_index;
> >>
> >> Why did we ever introduce the field image_index? Please, eliminate it it
> >> as the GUID is always sufficient to identify an image.
> >
> > This is derived from the UEFI specification.
> > UEFI specification "23.1.2
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
> > and ImageTypeId(guid).
> >
> > ImageIndex: A unique number identifying the firmware image within the
> > device. The number is between 1 and
> > DescriptorCount.
> >
> >>
> >>> image_info[i].image_type_id = fw_array[i].image_type_id;
> >>> image_info[i].image_id = fw_array[i].image_index;
> >>>
> >>> image_info[i].image_id_name = fw_array[i].fw_name;
> >>> -
> >>> - image_info[i].version = 0; /* not supported */
> >>> + efi_firmware_get_firmware_version(fw_array[i].image_index,
> >>> + &fw_array[i].image_type_id,
> >>> + &fw_version,
> >>> + &lowest_supported_version);
> >>
> >> This interface makes no sense to me. We expect images with specific
> >> GUIDs and should not care about images with other GUIDs that may
> >> additionally exist in the capsule.
> >>
> >> So you must pass the expected GUID as input variable here.
> >
> > I don't clearly understand this comment, but the expected GUID is
> > fw_array[i].image_type_id.
> >
> >>
> >>> + image_info[i].version = fw_version;
> >>> image_info[i].version_name = NULL; /* not supported */
> >>
> >> Please, add the missing functionality to
> >> efi_firmware_get_firmware_version().
> >
> > Does it mean we need to support version_name?
> > I can add a version_name in dtb.
> >
> >>
> >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
> >> will simplify the code.
> >
> > OK.
> >
> > Thank you for your review.
> >
> > Regards,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> image_info[i].size = 0;
> >>> image_info[i].attributes_supported =
> >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
> >>> image_info[0].attributes_setting |=
> >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> >>>
> >>> - image_info[i].lowest_supported_image_version = 0;
> >>> + image_info[i].lowest_supported_image_version = lowest_supported_version;
> >>> image_info[i].last_attempt_version = 0;
> >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> >>> image_info[i].hardware_instance = 1;
> >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> >>> descriptor_version, descriptor_count,
> >>> descriptor_size, package_version,
> >>> package_version_name);
> >>> -
> >>> return EFI_EXIT(ret);
> >>> }
> >>>
> >>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-05-09 9:57 ` Masahisa Kojima
@ 2023-05-10 0:28 ` Takahiro Akashi
2023-05-10 4:48 ` Masahisa Kojima
0 siblings, 1 reply; 20+ messages in thread
From: Takahiro Akashi @ 2023-05-10 0:28 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: Heinrich Schuchardt, Ilias Apalodimas, u-boot
On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote:
> On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 5/8/23 10:15, Masahisa Kojima wrote:
> > > Hi Heinrich,
> > >
> > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 4/10/23 11:07, Masahisa Kojima wrote:
> > >>> Current FMP->GetImageInfo() always return 0 for the firmware
> > >>> version, user can not identify which firmware version is currently
> > >>> running through the EFI interface.
> > >>>
> > >>> This commit gets the version information from device tree,
> > >>> then fills the firmware version, lowest supported version
> > >>> in FMP->GetImageInfo().
> > >>>
> > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > >>>
> > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > >>> ---
> > >>> Changes in v5:
> > >>> - newly implement a device tree based versioning
> > >>>
> > >>> .../firmware/firmware-version.txt | 25 ++++++++
> > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> > >>> 2 files changed, 84 insertions(+), 4 deletions(-)
> > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> > >>>
> > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> > >>> new file mode 100644
> > >>> index 0000000000..6112de4a1d
> > >>> --- /dev/null
> > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> > >>> @@ -0,0 +1,25 @@
> > >>> +firmware-version bindings
> > >>> +-------------------------------
> > >>> +
> > >>> +Required properties:
> > >>> +- image-type-id : guid for image blob type
> > >>> +- image-index : image index
> > >>> +- fw-version : firmware version
> > >>> +- lowest-supported-version : lowest supported version
> > >>> +
> > >>> +Example:
> > >>> +
> > >>> + firmware-version {
> > >>> + image1 {
> > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> > >>> + image-index = <1>;
> > >>> + fw-version = <5>;
> > >>> + lowest-supported-version = <3>;
> > >>> + };
> > >>> + image2 {
> > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> > >>> + image-index = <2>;
> > >>> + fw-version = <10>;
> > >>> + lowest-supported-version = <7>;
> > >>> + };
> > >>> + };
> > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > >>> index 93e2b01c07..1c6ef468bf 100644
> > >>> --- a/lib/efi_loader/efi_firmware.c
> > >>> +++ b/lib/efi_loader/efi_firmware.c
> > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > >>> return EFI_EXIT(EFI_UNSUPPORTED);
> > >>> }
> > >>>
> > >>> +/**
> > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb
> > >>> + * @image_index: Image index
> > >>> + * @image_type_id: Image type id
> > >>> + * @fw_version: Pointer to store the version number
> > >>> + * @lsv: Pointer to store the lowest supported version
> > >>> + *
> > >>> + * Authenticate the capsule if authentication is enabled.
> > >>> + * The image pointer and the image size are updated in case of success.
> > >>> + */
> > >>> +void efi_firmware_get_firmware_version(u8 image_index,
> > >>> + efi_guid_t *image_type_id,
> > >>> + u32 *fw_version, u32 *lsv)
> > >>> +{
> > >>> + const void *fdt = gd->fdt_blob;
> > >>> + const fdt32_t *val;
> > >>> + const char *guid_str;
> > >>> + int len, offset, index;
> > >>> + int parent;
> > >>> +
> > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> > >>> + if (parent < 0)
> > >>> + return;
> > >>> +
> > >>> + fdt_for_each_subnode(offset, fdt, parent) {
> > >>> + efi_guid_t guid;
> > >>> +
> > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> > >>> + if (!guid_str)
> > >>> + continue;
> > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> > >>> +
> > >>> + val = fdt_getprop(fdt, offset, "image-index", &len);
> > >>> + if (!val)
> > >>> + continue;
> > >>> + index = fdt32_to_cpu(*val);
> > >>> +
> > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) {
> > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len);
> > >>> + if (val)
> > >>> + *fw_version = fdt32_to_cpu(*val);
> > >>> +
> > >>> + val = fdt_getprop(fdt, offset,
> > >>> + "lowest-supported-version", &len);
> > >>> + if (val)
> > >>> + *lsv = fdt32_to_cpu(*val);
> > >>> + }
> > >>> + }
> > >>> +}
> > >>> +
> > >>> /**
> > >>> * efi_fill_image_desc_array - populate image descriptor array
> > >>> * @image_info_size: Size of @image_info
> > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> > >>> *package_version_name = NULL; /* not supported */
> > >>>
> > >>> for (i = 0; i < num_image_type_guids; i++) {
> > >>
> > >> Currently we define num_image_type_guids per board in a C file. Using
> > >> the same line of code once per board makes no sense to me. Please, move
> > >> the definition of that variable to lib/efi_loader/efi_firmware.c.
> > >
> > > Sorry for the late reply.
> > >
> > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
> > > fw_images[] array is also defined in each board file,
> > > so we can not simply move num_image_type_guids into
> > > lib/efi_loader/efi_firmware.c.
> >
> > Why can't we have
> >
> > int num_image_type_guids = ARRAY_SIZE(fw_images);
> >
> > in lib/efi_loader/efi_firmware.c?
>
> At first thought, I thought it was a matter of abstraction.
>
> But there is a compilation error when we expose fw_images[].
> fw_images[] array is initialized in each board file,
> and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c
> will cause compilation failure.
> We need to specify the array size when fw_images is exposed,
> for example:
> extern struct efi_fw_image fw_images[2];
>
> but currently there is no method to pre-define the fw_images[] array size,
> it is board specific.
>
> We can define the macro to indicate the array size or having
> sentinel in the fw_images[] array, but I think the current
I simply wonder if the value should be embedded in "struct efi_capsule_update_info".
struct efi_capsule_update_info {
const char *dfu_string;
int num_images; <- added
struct efi_fw_image *images;
};
This is the best place because the value must match not only "images"
but also (entries in) "dfu_string".
Even now, efi_fill_image_desc_array() tries to access "fw_images[]"
via the exposed update_info variable. Beautiful, isn't it?
One more comment:
uefi.rst doesn't mention anything about num_image_type_guids.
-Takahiro Akashi
> implementation is simpler,
> I would like to keep the current implementation.
> Correct me if I'm wrong.
>
> Thanks,
> Masahisa Kojima
>
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > And fw_images[] array is abstracted by struct efi_capsule_update_info,
> > > so I think
> > > we should not extract the fw_images[] array.
> > >
> > >>
> > >>> + u32 fw_version = 0;
> > >>> + u32 lowest_supported_version = 0;
> > >>
> > >> These assignments should be in efi_firmware_get_firmware_version.
> > >
> > > OK.
> > >
> > >>
> > >>> +
> > >>> image_info[i].image_index = fw_array[i].image_index;
> > >>
> > >> Why did we ever introduce the field image_index? Please, eliminate it it
> > >> as the GUID is always sufficient to identify an image.
> > >
> > > This is derived from the UEFI specification.
> > > UEFI specification "23.1.2
> > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
> > > and ImageTypeId(guid).
> > >
> > > ImageIndex: A unique number identifying the firmware image within the
> > > device. The number is between 1 and
> > > DescriptorCount.
> > >
> > >>
> > >>> image_info[i].image_type_id = fw_array[i].image_type_id;
> > >>> image_info[i].image_id = fw_array[i].image_index;
> > >>>
> > >>> image_info[i].image_id_name = fw_array[i].fw_name;
> > >>> -
> > >>> - image_info[i].version = 0; /* not supported */
> > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index,
> > >>> + &fw_array[i].image_type_id,
> > >>> + &fw_version,
> > >>> + &lowest_supported_version);
> > >>
> > >> This interface makes no sense to me. We expect images with specific
> > >> GUIDs and should not care about images with other GUIDs that may
> > >> additionally exist in the capsule.
> > >>
> > >> So you must pass the expected GUID as input variable here.
> > >
> > > I don't clearly understand this comment, but the expected GUID is
> > > fw_array[i].image_type_id.
> > >
> > >>
> > >>> + image_info[i].version = fw_version;
> > >>> image_info[i].version_name = NULL; /* not supported */
> > >>
> > >> Please, add the missing functionality to
> > >> efi_firmware_get_firmware_version().
> > >
> > > Does it mean we need to support version_name?
> > > I can add a version_name in dtb.
> > >
> > >>
> > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
> > >> will simplify the code.
> > >
> > > OK.
> > >
> > > Thank you for your review.
> > >
> > > Regards,
> > > Masahisa Kojima
> > >
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>> image_info[i].size = 0;
> > >>> image_info[i].attributes_supported =
> > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
> > >>> image_info[0].attributes_setting |=
> > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> > >>>
> > >>> - image_info[i].lowest_supported_image_version = 0;
> > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version;
> > >>> image_info[i].last_attempt_version = 0;
> > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > >>> image_info[i].hardware_instance = 1;
> > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> > >>> descriptor_version, descriptor_count,
> > >>> descriptor_size, package_version,
> > >>> package_version_name);
> > >>> -
> > >>> return EFI_EXIT(ret);
> > >>> }
> > >>>
> > >>
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-05-10 0:28 ` Takahiro Akashi
@ 2023-05-10 4:48 ` Masahisa Kojima
0 siblings, 0 replies; 20+ messages in thread
From: Masahisa Kojima @ 2023-05-10 4:48 UTC (permalink / raw)
To: Takahiro Akashi, Masahisa Kojima, Heinrich Schuchardt,
Ilias Apalodimas, u-boot
Hi Akashi-san,
On Wed, 10 May 2023 at 09:28, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote:
> > On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 5/8/23 10:15, Masahisa Kojima wrote:
> > > > Hi Heinrich,
> > > >
> > > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >> On 4/10/23 11:07, Masahisa Kojima wrote:
> > > >>> Current FMP->GetImageInfo() always return 0 for the firmware
> > > >>> version, user can not identify which firmware version is currently
> > > >>> running through the EFI interface.
> > > >>>
> > > >>> This commit gets the version information from device tree,
> > > >>> then fills the firmware version, lowest supported version
> > > >>> in FMP->GetImageInfo().
> > > >>>
> > > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > > >>>
> > > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > >>> ---
> > > >>> Changes in v5:
> > > >>> - newly implement a device tree based versioning
> > > >>>
> > > >>> .../firmware/firmware-version.txt | 25 ++++++++
> > > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> > > >>> 2 files changed, 84 insertions(+), 4 deletions(-)
> > > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> > > >>>
> > > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> > > >>> new file mode 100644
> > > >>> index 0000000000..6112de4a1d
> > > >>> --- /dev/null
> > > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> > > >>> @@ -0,0 +1,25 @@
> > > >>> +firmware-version bindings
> > > >>> +-------------------------------
> > > >>> +
> > > >>> +Required properties:
> > > >>> +- image-type-id : guid for image blob type
> > > >>> +- image-index : image index
> > > >>> +- fw-version : firmware version
> > > >>> +- lowest-supported-version : lowest supported version
> > > >>> +
> > > >>> +Example:
> > > >>> +
> > > >>> + firmware-version {
> > > >>> + image1 {
> > > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> > > >>> + image-index = <1>;
> > > >>> + fw-version = <5>;
> > > >>> + lowest-supported-version = <3>;
> > > >>> + };
> > > >>> + image2 {
> > > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";
> > > >>> + image-index = <2>;
> > > >>> + fw-version = <10>;
> > > >>> + lowest-supported-version = <7>;
> > > >>> + };
> > > >>> + };
> > > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > >>> index 93e2b01c07..1c6ef468bf 100644
> > > >>> --- a/lib/efi_loader/efi_firmware.c
> > > >>> +++ b/lib/efi_loader/efi_firmware.c
> > > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > > >>> return EFI_EXIT(EFI_UNSUPPORTED);
> > > >>> }
> > > >>>
> > > >>> +/**
> > > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb
> > > >>> + * @image_index: Image index
> > > >>> + * @image_type_id: Image type id
> > > >>> + * @fw_version: Pointer to store the version number
> > > >>> + * @lsv: Pointer to store the lowest supported version
> > > >>> + *
> > > >>> + * Authenticate the capsule if authentication is enabled.
> > > >>> + * The image pointer and the image size are updated in case of success.
> > > >>> + */
> > > >>> +void efi_firmware_get_firmware_version(u8 image_index,
> > > >>> + efi_guid_t *image_type_id,
> > > >>> + u32 *fw_version, u32 *lsv)
> > > >>> +{
> > > >>> + const void *fdt = gd->fdt_blob;
> > > >>> + const fdt32_t *val;
> > > >>> + const char *guid_str;
> > > >>> + int len, offset, index;
> > > >>> + int parent;
> > > >>> +
> > > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version");
> > > >>> + if (parent < 0)
> > > >>> + return;
> > > >>> +
> > > >>> + fdt_for_each_subnode(offset, fdt, parent) {
> > > >>> + efi_guid_t guid;
> > > >>> +
> > > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);
> > > >>> + if (!guid_str)
> > > >>> + continue;
> > > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
> > > >>> +
> > > >>> + val = fdt_getprop(fdt, offset, "image-index", &len);
> > > >>> + if (!val)
> > > >>> + continue;
> > > >>> + index = fdt32_to_cpu(*val);
> > > >>> +
> > > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) {
> > > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len);
> > > >>> + if (val)
> > > >>> + *fw_version = fdt32_to_cpu(*val);
> > > >>> +
> > > >>> + val = fdt_getprop(fdt, offset,
> > > >>> + "lowest-supported-version", &len);
> > > >>> + if (val)
> > > >>> + *lsv = fdt32_to_cpu(*val);
> > > >>> + }
> > > >>> + }
> > > >>> +}
> > > >>> +
> > > >>> /**
> > > >>> * efi_fill_image_desc_array - populate image descriptor array
> > > >>> * @image_info_size: Size of @image_info
> > > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array(
> > > >>> *package_version_name = NULL; /* not supported */
> > > >>>
> > > >>> for (i = 0; i < num_image_type_guids; i++) {
> > > >>
> > > >> Currently we define num_image_type_guids per board in a C file. Using
> > > >> the same line of code once per board makes no sense to me. Please, move
> > > >> the definition of that variable to lib/efi_loader/efi_firmware.c.
> > > >
> > > > Sorry for the late reply.
> > > >
> > > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)",
> > > > fw_images[] array is also defined in each board file,
> > > > so we can not simply move num_image_type_guids into
> > > > lib/efi_loader/efi_firmware.c.
> > >
> > > Why can't we have
> > >
> > > int num_image_type_guids = ARRAY_SIZE(fw_images);
> > >
> > > in lib/efi_loader/efi_firmware.c?
> >
> > At first thought, I thought it was a matter of abstraction.
> >
> > But there is a compilation error when we expose fw_images[].
> > fw_images[] array is initialized in each board file,
> > and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c
> > will cause compilation failure.
> > We need to specify the array size when fw_images is exposed,
> > for example:
> > extern struct efi_fw_image fw_images[2];
> >
> > but currently there is no method to pre-define the fw_images[] array size,
> > it is board specific.
> >
> > We can define the macro to indicate the array size or having
> > sentinel in the fw_images[] array, but I think the current
>
> I simply wonder if the value should be embedded in "struct efi_capsule_update_info".
> struct efi_capsule_update_info {
> const char *dfu_string;
> int num_images; <- added
> struct efi_fw_image *images;
> };
Thank you, I agree with this.
> This is the best place because the value must match not only "images"
> but also (entries in) "dfu_string".
>
> Even now, efi_fill_image_desc_array() tries to access "fw_images[]"
> via the exposed update_info variable. Beautiful, isn't it?
>
> One more comment:
> uefi.rst doesn't mention anything about num_image_type_guids.
I will also update uefi.rst.
Regards,
Masahisa Kojima
>
> -Takahiro Akashi
>
> > implementation is simpler,
> > I would like to keep the current implementation.
> > Correct me if I'm wrong.
> >
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >
> > > > And fw_images[] array is abstracted by struct efi_capsule_update_info,
> > > > so I think
> > > > we should not extract the fw_images[] array.
> > > >
> > > >>
> > > >>> + u32 fw_version = 0;
> > > >>> + u32 lowest_supported_version = 0;
> > > >>
> > > >> These assignments should be in efi_firmware_get_firmware_version.
> > > >
> > > > OK.
> > > >
> > > >>
> > > >>> +
> > > >>> image_info[i].image_index = fw_array[i].image_index;
> > > >>
> > > >> Why did we ever introduce the field image_index? Please, eliminate it it
> > > >> as the GUID is always sufficient to identify an image.
> > > >
> > > > This is derived from the UEFI specification.
> > > > UEFI specification "23.1.2
> > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex
> > > > and ImageTypeId(guid).
> > > >
> > > > ImageIndex: A unique number identifying the firmware image within the
> > > > device. The number is between 1 and
> > > > DescriptorCount.
> > > >
> > > >>
> > > >>> image_info[i].image_type_id = fw_array[i].image_type_id;
> > > >>> image_info[i].image_id = fw_array[i].image_index;
> > > >>>
> > > >>> image_info[i].image_id_name = fw_array[i].fw_name;
> > > >>> -
> > > >>> - image_info[i].version = 0; /* not supported */
> > > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index,
> > > >>> + &fw_array[i].image_type_id,
> > > >>> + &fw_version,
> > > >>> + &lowest_supported_version);
> > > >>
> > > >> This interface makes no sense to me. We expect images with specific
> > > >> GUIDs and should not care about images with other GUIDs that may
> > > >> additionally exist in the capsule.
> > > >>
> > > >> So you must pass the expected GUID as input variable here.
> > > >
> > > > I don't clearly understand this comment, but the expected GUID is
> > > > fw_array[i].image_type_id.
> > > >
> > > >>
> > > >>> + image_info[i].version = fw_version;
> > > >>> image_info[i].version_name = NULL; /* not supported */
> > > >>
> > > >> Please, add the missing functionality to
> > > >> efi_firmware_get_firmware_version().
> > > >
> > > > Does it mean we need to support version_name?
> > > > I can add a version_name in dtb.
> > > >
> > > >>
> > > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That
> > > >> will simplify the code.
> > > >
> > > > OK.
> > > >
> > > > Thank you for your review.
> > > >
> > > > Regards,
> > > > Masahisa Kojima
> > > >
> > > >>
> > > >> Best regards
> > > >>
> > > >> Heinrich
> > > >>
> > > >>> image_info[i].size = 0;
> > > >>> image_info[i].attributes_supported =
> > > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array(
> > > >>> image_info[0].attributes_setting |=
> > > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> > > >>>
> > > >>> - image_info[i].lowest_supported_image_version = 0;
> > > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version;
> > > >>> image_info[i].last_attempt_version = 0;
> > > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > > >>> image_info[i].hardware_instance = 1;
> > > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info(
> > > >>> descriptor_version, descriptor_count,
> > > >>> descriptor_size, package_version,
> > > >>> package_version_name);
> > > >>> -
> > > >>> return EFI_EXIT(ret);
> > > >>> }
> > > >>>
> > > >>
> > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-04-28 4:07 ` Heinrich Schuchardt
@ 2023-05-10 20:46 ` Simon Glass
2023-05-10 21:12 ` Heinrich Schuchardt
0 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2023-05-10 20:46 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Masahisa Kojima, u-boot, Ilias Apalodimas, Takahiro Akashi
Hi Heinrich,
On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Masahisa,
> >
> >On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
> ><masahisa.kojima@linaro.org> wrote:
> >>
> >> Current FMP->GetImageInfo() always return 0 for the firmware
> >> version, user can not identify which firmware version is currently
> >> running through the EFI interface.
> >>
> >> This commit gets the version information from device tree,
> >> then fills the firmware version, lowest supported version
> >> in FMP->GetImageInfo().
> >>
> >> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> >>
> >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >> ---
> >> Changes in v5:
> >> - newly implement a device tree based versioning
> >>
> >> .../firmware/firmware-version.txt | 25 ++++++++
> >> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
> >> 2 files changed, 84 insertions(+), 4 deletions(-)
> >> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
> >>
> >> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
> >> new file mode 100644
> >> index 0000000000..6112de4a1d
> >> --- /dev/null
> >> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
> >> @@ -0,0 +1,25 @@
> >> +firmware-version bindings
> >> +-------------------------------
> >> +
> >> +Required properties:
> >> +- image-type-id : guid for image blob type
> >> +- image-index : image index
> >> +- fw-version : firmware version
> >> +- lowest-supported-version : lowest supported version
> >> +
> >> +Example:
> >> +
> >> + firmware-version {
> >> + image1 {
> >> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
> >
> >Nit: please use lower-case hex and add a decoder to uuid.c so we can
> >look it up when debugging.
>
> The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
>
> Instead we should define a string per firmware image. We already have a field for it:
>
> fw_array[i].fw_name
This is of course madness. NAK to any UUIDs in U-Boot that don't have
a decoder ring.
Let's use a compatible string (vendor,board) and drop these silly UUIDs.
Regards,
Simon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] efi_loader: get version information from device tree
2023-05-10 20:46 ` Simon Glass
@ 2023-05-10 21:12 ` Heinrich Schuchardt
0 siblings, 0 replies; 20+ messages in thread
From: Heinrich Schuchardt @ 2023-05-10 21:12 UTC (permalink / raw)
To: Simon Glass; +Cc: Masahisa Kojima, u-boot, Ilias Apalodimas, Takahiro Akashi
On 5/10/23 22:46, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Masahisa,
>>>
>>> On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
>>> <masahisa.kojima@linaro.org> wrote:
>>>>
>>>> Current FMP->GetImageInfo() always return 0 for the firmware
>>>> version, user can not identify which firmware version is currently
>>>> running through the EFI interface.
>>>>
>>>> This commit gets the version information from device tree,
>>>> then fills the firmware version, lowest supported version
>>>> in FMP->GetImageInfo().
>>>>
>>>> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
>>>>
>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>> ---
>>>> Changes in v5:
>>>> - newly implement a device tree based versioning
>>>>
>>>> .../firmware/firmware-version.txt | 25 ++++++++
>>>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++--
>>>> 2 files changed, 84 insertions(+), 4 deletions(-)
>>>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
>>>>
>>>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt
>>>> new file mode 100644
>>>> index 0000000000..6112de4a1d
>>>> --- /dev/null
>>>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt
>>>> @@ -0,0 +1,25 @@
>>>> +firmware-version bindings
>>>> +-------------------------------
>>>> +
>>>> +Required properties:
>>>> +- image-type-id : guid for image blob type
>>>> +- image-index : image index
>>>> +- fw-version : firmware version
>>>> +- lowest-supported-version : lowest supported version
>>>> +
>>>> +Example:
>>>> +
>>>> + firmware-version {
>>>> + image1 {
>>>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
>>>
>>> Nit: please use lower-case hex and add a decoder to uuid.c so we can
>>> look it up when debugging.
>>
>> The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
>>
>> Instead we should define a string per firmware image. We already have a field for it:
>>
>> fw_array[i].fw_name
>
> This is of course madness. NAK to any UUIDs in U-Boot that don't have
> a decoder ring.
I never have heard of a "decoder ring" for UUIDs in U-Boot.
git grep -A2 -ni decoder | grep -i ring
does not find such a term.
Our development target is to keep the code size of U-Boot small. Hence,
we should not clutter uuid.c with board specific strings. These should
be kept in board specific files.
>
> Let's use a compatible string (vendor,board) and drop these silly UUIDs.
Please, avoid derogatory language like "silly".
The UEFI specification defines EFI_CAPSULE_HEADER. This structure
contains field CapusuleGuid (A GUID that defines the contents of a capsule).
Best regards
Heinrich
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-05-10 21:13 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-10 9:07 [PATCH v5 0/4] FMP versioning support Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 1/4] efi_loader: get version information from device tree Masahisa Kojima
2023-04-27 6:09 ` Heinrich Schuchardt
2023-05-08 8:15 ` Masahisa Kojima
2023-05-08 9:44 ` Heinrich Schuchardt
2023-05-09 9:57 ` Masahisa Kojima
2023-05-10 0:28 ` Takahiro Akashi
2023-05-10 4:48 ` Masahisa Kojima
2023-04-27 23:35 ` Simon Glass
2023-04-28 4:07 ` Heinrich Schuchardt
2023-05-10 20:46 ` Simon Glass
2023-05-10 21:12 ` Heinrich Schuchardt
2023-04-10 9:07 ` [PATCH v5 2/4] efi_loader: check lowest supported version Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 3/4] mkeficapsule: add FMP Payload Header Masahisa Kojima
2023-04-10 9:07 ` [PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning Masahisa Kojima
2023-04-19 1:46 ` Simon Glass
2023-04-20 5:17 ` Masahisa Kojima
2023-04-24 19:42 ` Simon Glass
2023-04-11 1:48 ` [PATCH v5 0/4] FMP versioning support Takahiro Akashi
2023-04-11 2:58 ` Masahisa Kojima
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).