Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
[WIP]arch/arm: Implement FIT support
Untested
Change-Id: Ifae3cbae85bcd1c35f31278f76f4d97700979da3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M payloads/Kconfig M src/arch/arm/Makefile.inc A src/arch/arm/fit_payload.c 3 files changed, 152 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/35410/1
diff --git a/payloads/Kconfig b/payloads/Kconfig index 46cfaf5..04566cc 100644 --- a/payloads/Kconfig +++ b/payloads/Kconfig @@ -30,7 +30,7 @@
config PAYLOAD_FIT bool "A FIT payload" - depends on ARCH_ARM64 || ARCH_RISCV + depends on ARCH_ARM || ARCH_ARM64 || ARCH_RISCV select PAYLOAD_FIT_SUPPORT help Select this option if you have a payload image (a FIT file) which @@ -100,7 +100,7 @@ bool "FIT support" default n default y if PAYLOAD_LINUX && (ARCH_ARM || ARCH_ARM64 || ARCH_RISCV) - depends on ARCH_ARM64 || ARCH_RISCV + depends on ARCH_ARM || ARCH_ARM64 || ARCH_RISCV select FLATTENED_DEVICE_TREE help Select this option if your payload is of type FIT. diff --git a/src/arch/arm/Makefile.inc b/src/arch/arm/Makefile.inc index 508b0a8..83c698a 100644 --- a/src/arch/arm/Makefile.inc +++ b/src/arch/arm/Makefile.inc @@ -131,6 +131,7 @@ ramstage-y += memcpy.S ramstage-y += memmove.S ramstage-y += clock.c +ramstage-$(CONFIG_PAYLOAD_FIT_SUPPORT) += fit_payload.c
rmodules_arm-y += memset.S rmodules_arm-y += memcpy.S diff --git a/src/arch/arm/fit_payload.c b/src/arch/arm/fit_payload.c new file mode 100644 index 0000000..5792dfb --- /dev/null +++ b/src/arch/arm/fit_payload.c @@ -0,0 +1,149 @@ +/* + * Copyright 2013 Google Inc. + * Copyright 2018 Facebook, Inc. + * Copyright 2019 9elements Agency GmbH patrick.rudolph@9elements.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <bootmem.h> +#include <stdlib.h> +#include <program_loading.h> +#include <string.h> +#include <commonlib/compression.h> +#include <commonlib/cbfs_serialized.h> +#include <lib.h> +#include <fit.h> +#include <endian.h> + +#define MAX_KERNEL_SIZE (64*MiB) +#define SECTION_ALIGN 64 + +static size_t get_kernel_size(const struct fit_image_node *node) +{ + /* + * Since we don't have a way to determine the uncompressed size of the + * kernel, we have to keep as much memory as possible free for use by + * the kernel immediately after the end of the kernel image. The amount + * of space required will vary depending on selected features, and is + * effectively unbound. + */ + + printk(BIOS_INFO, + "FIT: Leaving additional %u MiB of free space after kernel.\n", + MAX_KERNEL_SIZE >> 20); + + return node->size + MAX_KERNEL_SIZE; +} + +/** + * Place the region in free memory range. + * + * The caller has to set region->offset to the minimum allowed address. + */ +static bool fit_place_mem(const struct range_entry *r, void *arg) +{ + struct region *region = arg; + resource_t start; + + if (range_entry_tag(r) != BM_MEM_RAM) + return true; + + /* Section must be aligned at page boundary */ + start = ALIGN_UP(MAX(region->offset, range_entry_base(r)), SECTION_ALIGN); + + if (start + region->size < range_entry_end(r)) { + region->offset = (size_t)start; + return false; + } + + return true; +} + +static uint32_t arm_kernel_args[3]; + +bool fit_payload_arch(struct prog *payload, struct fit_config_node *config, + struct region *kernel, + struct region *fdt, + struct region *initrd) +{ + size_t lower_mem; + + if (!config->fdt || !fdt) { + printk(BIOS_CRIT, "CRIT: Providing a valid FDT is mandatory to " + "boot a RISC-V kernel!\n"); + return false; + /* TODO: Fall back to the ROM FDT? */ + } + + /* Update kernel size from image header, if possible */ + kernel->size = get_kernel_size(config->kernel); + printk(BIOS_DEBUG, "FIT: Using kernel size of 0x%zx bytes\n", + kernel->size); + + /* + * The code assumes that bootmem_walk provides a sorted list of memory + * regions, starting from the lowest address. + * The order of the calls here doesn't matter, as the placement is + * enforced in the called functions. + * For details check code on top. + */ + kernel->offset = 0; + if (!bootmem_walk(fit_place_mem, kernel)) + return false; + + /* + * The kernel documentation recommends place the image 32M above to avoid + * the need to relocate prior to compression. + */ + lower_mem = kernel->offset; + kernel->offset += 32 * MiB; + + /* Mark as reserved for future allocations. */ + bootmem_add_range(kernel->offset, kernel->size, BM_MEM_PAYLOAD); + + /* Place FDT and INITRD after kernel. */ + + /* Place INITRD and FDT: + * The Kernel documentation recommends the safe location of 128M above + * the start of ram to avoid being overwritten by the kernel + * decompressor. + */ + if (config->ramdisk) { + initrd->offset = lower_mem + 128 * MiB; + + if (!bootmem_walk(fit_place_mem, initrd)) + return false; + /* Mark as reserved for future allocations. */ + bootmem_add_range(initrd->offset, initrd->size, BM_MEM_PAYLOAD); + } + + /* Place FDT */ + fdt->offset = lower_mem + 128 * MiB; + + if (!bootmem_walk(fit_place_mem, fdt)) + return false; + /* Mark as reserved for future allocations. */ + bootmem_add_range(fdt->offset, fdt->size, BM_MEM_PAYLOAD); + + /* Kernel expects FDT as argument */ + arm_kernel_args[0] = 0; + /* All ones for FDT */ + arm_kernel_args[1] = ~0; + arm_kernel_args[2] = fdt->offset; + + prog_set_entry(payload, (void *)kernel->offset, (void *)arm_kernel_args); + + bootmem_dump_ranges(); + + return true; +}
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 1:
For some reason enabling FIT support needs to dramatically increase the ramstage size in the linker script. (Not buildtested here) Any clue why?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 1:
Patch Set 1:
For some reason enabling FIT support needs to dramatically increase the ramstage size in the linker script. (Not buildtested here) Any clue why?
This should help:
nm -n build/cbfs/fallback/ramstage.debug
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
For some reason enabling FIT support needs to dramatically increase the ramstage size in the linker script. (Not buildtested here) Any clue why?
This should help:
nm -n build/cbfs/fallback/ramstage.debug
There is a 1M heap usage suddenly...
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
For some reason enabling FIT support needs to dramatically increase the ramstage size in the linker script. (Not buildtested here) Any clue why?
This should help:
nm -n build/cbfs/fallback/ramstage.debug
There is a 1M heap usage suddenly...
CONFIG_HEAP_SIZE is default 0x100000 if FLATTENED_DEVICE_TREE, so there's that...
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35410/1/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/1/src/arch/arm/fit_payload.c@... PS1, Line 29: #define SECTION_ALIGN 64 4096 or 2MiB?
https://review.coreboot.org/c/coreboot/+/35410/1/src/arch/arm/fit_payload.c@... PS1, Line 83: "boot a RISC-V kernel!\n"); armv7
https://review.coreboot.org/c/coreboot/+/35410/1/src/arch/arm/fit_payload.c@... PS1, Line 85: /* TODO: Fall back to the ROM FDT? */ there's no ROM FDT on armv7
https://review.coreboot.org/c/coreboot/+/35410/1/src/arch/arm/fit_payload.c@... PS1, Line 106: * the need to relocate prior to compression. doesn't that apply to zImage only? Usually FIT uses Image.
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35410
to look at the new patch set (#2).
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
[WIP]arch/arm: Implement FIT support
Untested
Change-Id: Ifae3cbae85bcd1c35f31278f76f4d97700979da3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M payloads/Kconfig M src/arch/arm/Makefile.inc A src/arch/arm/fit_payload.c 3 files changed, 151 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/35410/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 29: #define SECTION_ALIGN 64 /* TODO not mentioned in kernel DOCs */ This is not a page boundary, like your comment below says. Why not just use 4*KiB? I don't think you'd ever want to place it non-page-aligned anyway.
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 43: MAX_KERNEL_SIZE >> 20); nit: is there really a point in printing out the same constant size on every boot?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 83: "boot a armv7 kernel!\n"); nit: don't break strings (and we can use 96 chars now anyway). (Also, "an armv7"?)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 108: kernel->offset += 32 * MiB; Doesn't this mean the size you checked in fit_place_mem() isn't necessarily enough anymore (because now you subtracted 32MB)?
Why don't you just do this above
kernel->offset = (uintptr_t)_dram + 32*MiB; ...bootmem_walk...
to achieve the same thing easier?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem Again, just use _dram. All Arm platforms know where their memory starts at compile time.
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 135: fdt->size Actually, I just noticed a bug in lib/fit_payload.c with this (applies to arm64 as well): we're first determining the total FDT size with dt_flat_size(), then calling fit_payload_arch(), and then calling fit_add_ramdisk() to add the ramdisk information. This will increase the final flattened FDT size. There are no further size checks on flattening so it may overrun the struct region 'fdt' there.
We should add some constant amount of slack to the dt_flag_size() when setting fdt->size, and make pack_fdt() check for overruns after packing.
Patrick Rudolph has uploaded a new patch set (#3) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
arch/arm: Implement FIT support
Tested on qemu-system-arm -m vepxress-a9 and uImage FIT: Boots to initramfs.
Change-Id: Ifae3cbae85bcd1c35f31278f76f4d97700979da3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Patrick Rudolph siro@das-labor.org --- M payloads/Kconfig M src/arch/arm/Makefile.inc A src/arch/arm/fit_payload.c 3 files changed, 168 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/35410/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c@... PS3, Line 41: u32 endianess; 'endianess' may be misspelled - perhaps 'endianness'?
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c@... PS3, Line 53: if (hdr->magic == ZIMAGE_MAGIC) { braces {} are not necessary for any arm of this statement
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c@... PS3, Line 106: "boot a armv7 kernel!\n"); One line?
https://review.coreboot.org/c/coreboot/+/35410/3/src/arch/arm/fit_payload.c@... PS3, Line 138: /* Place INITRD and FDT: Please use:
/* * …
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35410/4/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/4/src/arch/arm/fit_payload.c@... PS4, Line 41: u32 endianess; 'endianess' may be misspelled - perhaps 'endianness'?
https://review.coreboot.org/c/coreboot/+/35410/4/src/arch/arm/fit_payload.c@... PS4, Line 53: if (hdr->magic == ZIMAGE_MAGIC) { braces {} are not necessary for any arm of this statement
Patrick Rudolph has uploaded a new patch set (#5) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
arch/arm: Implement FIT support
Tested on qemu-system-arm -m vepxress-a9 and uImage FIT: Boots to initramfs.
Change-Id: Ifae3cbae85bcd1c35f31278f76f4d97700979da3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Patrick Rudolph siro@das-labor.org --- M payloads/Kconfig M src/arch/arm/Makefile.inc A src/arch/arm/fit_payload.c 3 files changed, 166 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/35410/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 29: #define SECTION_ALIGN 64 /* TODO not mentioned in kernel DOCs */
This is not a page boundary, like your comment below says. […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 43: MAX_KERNEL_SIZE >> 20);
nit: is there really a point in printing out the same constant size on every boot?
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 83: "boot a armv7 kernel!\n");
nit: don't break strings (and we can use 96 chars now anyway). […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 108: kernel->offset += 32 * MiB;
Doesn't this mean the size you checked in fit_place_mem() isn't necessarily enough anymore (because […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem
Again, just use _dram. All Arm platforms know where their memory starts at compile time.
I guess that assumes that the payload will be loaded at _dram, which might not be the case on qemu as the stages and stage cache resides near _dram. Shouldn't we use the offset from kernel start as it is likely placed aboved the stages and stage cache?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 135: fdt->size
Actually, I just noticed a bug in lib/fit_payload. […]
done in CB:41201
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem
I guess that assumes that the payload will be loaded at _dram, which might not be the case on qemu a […]
Isn't that the point of the bootmem_walk()? The goal is to find a memory area (might as well start from the lowest possible) that isn't needed by coreboot at this point. Anything that is needed by coreboot should be reserved with a bootmem tag... for the standard stuff (e.g. ramstage image, stack) that should already be in place, but there might be some platform or arch-specific things you still need to add.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 50: /* Both of these cases (an arm32 kernel using FIT compression or being smaller than a handful of bytes) are really hard errors that should never happen (FIT compression is only meant for kernels that cannot compress themselves, like arm64). I'd maybe just error out here, or at least print an angry error message.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 107: kernel->size += DECOMP_SIZE; I still think kernel->offset = _dram + DECOMP_SIZE is better (and also makes the code intention easier to follow). The difference is that if any coreboot-reserved memory regions (e.g. the ramstage program image) are placed in that 32MB window, this version would unnecessarily push the kernel further back than it needs to be.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 121: bootmem_add_range(kernel->offset, kernel->size, BM_MEM_PAYLOAD); You're doing this twice now?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 5: Code-Review+1
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Abandoned
Implemented and merged in cb:44377
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)