Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32872
to review the following change.
Change subject: fit: Add device tree compression ......................................................................
fit: Add device tree compression
This patch adds support for compressing individual device trees in the FIT image. In order to make this efficient, we'll have to pull the compatible property out of the FDT and store it directly in the config node of the FIT image, so that we don't have to scan (and therefore decompress) every single FDT on boot. Device tree compression is only supported for FIT images that have this external compatible property. For older images with no compression, we still support fallback to scanning the FDT for the property.
This patch was adapted from depthcharge's http://crosreview.com/1553458
Change-Id: Ifcb6997782c480c8ef6692df17b66ad96264e623 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/lib/fit.c M src/lib/fit_payload.c 2 files changed, 55 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/32872/1
diff --git a/src/lib/fit.c b/src/lib/fit.c index aa3fee7..464be3a 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -136,6 +136,8 @@ config->fdt = find_image(prop->prop.data); else if (!strcmp("ramdisk", prop->prop.name)) config->ramdisk = find_image(prop->prop.data); + else if (!strcmp("compatible", prop->prop.name)) + config->compat = prop->prop; }
list_insert_after(&config->list_node, &config_nodes); @@ -391,35 +393,45 @@ */ static int fit_update_compat(struct fit_config_node *config) { - if (config->fdt->compression != CBFS_COMPRESS_NONE) { - printk(BIOS_ERR, - "FDT compression not yet supported, skipping %s.\n", - config->name); - return -1; - } + // If there was no "compatible" property in config node, this is a + // legacy FIT image. Must extract compat prop from FDT itself. + if (!config->compat.name) { + void *fdt_blob = config->fdt->data; + const struct fdt_header *fdt_header = fdt_blob; + uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset);
- void *fdt_blob = config->fdt->data; - struct compat_string_entry *compat_node; - const struct fdt_header *fdt_header = - (const struct fdt_header *)fdt_blob; - uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset); - size_t i = 0; + if (config->fdt->compression != CBFS_COMPRESS_NONE) { + printk(BIOS_ERR, + "ERROR: config %s has a compressed FDT without " + "external compatible property, skipping.\n", + config->name); + return -1; + } + + if (fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { + printk(BIOS_ERR, + "ERROR: Can't find compat string in FDT %s " + "for config %s, skipping.\n", + config->fdt->name, config->name); + return -1; + } + }
config->compat_pos = -1; config->compat_rank = -1; - if (!fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { - list_for_each(compat_node, compat_strings, list_node) { - int pos = fit_check_compat(&config->compat, - compat_node->compat_string); - if (pos >= 0) { - config->compat_pos = pos; - config->compat_rank = i; - config->compat_string = - compat_node->compat_string; - break; - } - i++; + size_t i = 0; + struct compat_string_entry *compat_node; + list_for_each(compat_node, compat_strings, list_node) { + int pos = fit_check_compat(&config->compat, + compat_node->compat_string); + if (pos >= 0) { + config->compat_pos = pos; + config->compat_rank = i; + config->compat_string = + compat_node->compat_string; + break; } + i++; }
return 0; diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index c977903..b049188 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -96,6 +96,24 @@ return false; }
+static struct device_tree *unpack_fdt(struct fit_image_node *image_node) +{ + void *data = image_node->data; + + if (image_node->compression != CBFS_COMPRESS_NONE) { + /* TODO: This is an ugly heuristic for how much the size will + expand on decompression, fix once FIT images support storing + the real uncompressed size. */ + struct region r = { .offset = 0, .size = image_node->size * 5 }; + data = malloc(r.size); + r.offset = (uintptr_t)data; + if (!data || extract(&r, image_node)) + return NULL; + } + + return fdt_unflatten(data); +} + /** * Add coreboot tables, CBMEM information and optional board specific strapping * IDs to the device tree loaded via FIT. @@ -181,7 +199,7 @@ return; }
- dt = fdt_unflatten(config->fdt->data); + dt = unpack_fdt(config->fdt); if (!dt) { printk(BIOS_ERR, "ERROR: Failed to unflatten the FDT.\n"); rdev_munmap(prog_rdev(payload), data);
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32872 )
Change subject: fit: Add device tree compression ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32872 )
Change subject: fit: Add device tree compression ......................................................................
fit: Add device tree compression
This patch adds support for compressing individual device trees in the FIT image. In order to make this efficient, we'll have to pull the compatible property out of the FDT and store it directly in the config node of the FIT image, so that we don't have to scan (and therefore decompress) every single FDT on boot. Device tree compression is only supported for FIT images that have this external compatible property. For older images with no compression, we still support fallback to scanning the FDT for the property.
This patch was adapted from depthcharge's http://crosreview.com/1553458
Change-Id: Ifcb6997782c480c8ef6692df17b66ad96264e623 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32872 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/fit.c M src/lib/fit_payload.c 2 files changed, 55 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/lib/fit.c b/src/lib/fit.c index 5fbcd77..befedde 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -136,6 +136,8 @@ config->fdt = find_image(prop->prop.data); else if (!strcmp("ramdisk", prop->prop.name)) config->ramdisk = find_image(prop->prop.data); + else if (!strcmp("compatible", prop->prop.name)) + config->compat = prop->prop; }
list_insert_after(&config->list_node, &config_nodes); @@ -391,35 +393,45 @@ */ static int fit_update_compat(struct fit_config_node *config) { - if (config->fdt->compression != CBFS_COMPRESS_NONE) { - printk(BIOS_ERR, - "FDT compression not yet supported, skipping %s.\n", - config->name); - return -1; - } + // If there was no "compatible" property in config node, this is a + // legacy FIT image. Must extract compat prop from FDT itself. + if (!config->compat.name) { + void *fdt_blob = config->fdt->data; + const struct fdt_header *fdt_header = fdt_blob; + uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset);
- void *fdt_blob = config->fdt->data; - struct compat_string_entry *compat_node; - const struct fdt_header *fdt_header = - (const struct fdt_header *)fdt_blob; - uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset); - size_t i = 0; + if (config->fdt->compression != CBFS_COMPRESS_NONE) { + printk(BIOS_ERR, + "ERROR: config %s has a compressed FDT without " + "external compatible property, skipping.\n", + config->name); + return -1; + } + + if (fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { + printk(BIOS_ERR, + "ERROR: Can't find compat string in FDT %s " + "for config %s, skipping.\n", + config->fdt->name, config->name); + return -1; + } + }
config->compat_pos = -1; config->compat_rank = -1; - if (!fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { - list_for_each(compat_node, compat_strings, list_node) { - int pos = fit_check_compat(&config->compat, - compat_node->compat_string); - if (pos >= 0) { - config->compat_pos = pos; - config->compat_rank = i; - config->compat_string = - compat_node->compat_string; - break; - } - i++; + size_t i = 0; + struct compat_string_entry *compat_node; + list_for_each(compat_node, compat_strings, list_node) { + int pos = fit_check_compat(&config->compat, + compat_node->compat_string); + if (pos >= 0) { + config->compat_pos = pos; + config->compat_rank = i; + config->compat_string = + compat_node->compat_string; + break; } + i++; }
return 0; diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index c977903..b049188 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -96,6 +96,24 @@ return false; }
+static struct device_tree *unpack_fdt(struct fit_image_node *image_node) +{ + void *data = image_node->data; + + if (image_node->compression != CBFS_COMPRESS_NONE) { + /* TODO: This is an ugly heuristic for how much the size will + expand on decompression, fix once FIT images support storing + the real uncompressed size. */ + struct region r = { .offset = 0, .size = image_node->size * 5 }; + data = malloc(r.size); + r.offset = (uintptr_t)data; + if (!data || extract(&r, image_node)) + return NULL; + } + + return fdt_unflatten(data); +} + /** * Add coreboot tables, CBMEM information and optional board specific strapping * IDs to the device tree loaded via FIT. @@ -181,7 +199,7 @@ return; }
- dt = fdt_unflatten(config->fdt->data); + dt = unpack_fdt(config->fdt); if (!dt) { printk(BIOS_ERR, "ERROR: Failed to unflatten the FDT.\n"); rdev_munmap(prog_rdev(payload), data);
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)