Hi Geert,
On 04.01.2021 14:01, Geert Uytterhoeven wrote:
> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000. This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
>
> Fix this limitation by validating the masked address against the memory
> information in the passed DTB. Only use the start address
> from DTB when masking would yield an out-of-range address, prefer the
> traditional method in all other cases. Note that this applies only to the
> explicitly passed DTB on modern systems, and not to a DTB appended to
> the kernel, or to ATAGS. The appended DTB may need to be augmented by
> information from ATAGS, which may need to rely on knowledge of the start
> address of physical memory itself.
>
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
>
> Suggested-by: Nicolas Pitre <nico@xxxxxxxxxxx>
> Suggested-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Acked-by: Nicolas Pitre <nico@xxxxxxxxxxx>
I've checked all of my arm 32bit test systems and they still boot fine
with this patch. Feel free to add my:
Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
although I didn't test exactly the new features added by it.
> ---
> Submitted to RMK's patch tracker.
>
> v12:
> - Drop unneeded initialization of r in get_val(),
>
> v11:
> - Add Reviewed-by, Acked-by,
> - Document GOT fixup caveat,
>
> v10:
> - Update for commit 9443076e4330a14a ("ARM: p2v: reduce p2v alignment
> requirement to 2 MiB"),
> - Use OF_DT_MAGIC macro,
> - Rename fdt_get_mem_start() to fdt_check_mem_start(),
> - Skip validation if there is an appended DTB,
> - Pass mem_start to fdt_check_mem_start() to streamline code,
> - Optimize register allocation,
> - Update CONFIG_AUTO_ZRELADDR help text,
> - Check all memory nodes and ranges (not just the first one), and
> "linux,usable-memory", similar to early_init_dt_scan_memory(),
> - Drop Reviewed-by, Tested-by tags,
>
> v9:
> - Add minlen parameter to getprop(), for better validation of
> memory properties,
> - Only use start of memory from the DTB if masking would yield an
> out-of-range address, to fix kdump, as suggested by Ard.
>
> v8:
> - Rebase on top of commit 893ab00439a45513 ("kbuild: remove cc-option
> test of -fno-stack-protector"),
>
> v7:
> - Rebase on top of commit 161e04a5bae58a65 ("ARM: decompressor: split
> off _edata and stack base into separate object"),
>
> v6:
> - Rebase on top of commit 7ae4a78daacf240a ("ARM: 8969/1:
> decompressor: simplify libfdt builds"),
> - Include <linux/libfdt.h> instead of <libfdt.h>,
>
> v5:
> - Add Tested-by, Reviewed-by,
> - Round up start of memory to satisfy 16 MiB alignment rule,
>
> v4:
> - Fix stack location after commit 184bf653a7a452c1 ("ARM:
> decompressor: factor out routine to obtain the inflated image
> size"),
>
> v3:
> - Add Reviewed-by,
> - Fix ATAGs with appended DTB,
> - Add Tested-by,
>
> v2:
> - Use "cmp r0, #-1", instead of "cmn r0, #1",
> - Add missing stack setup,
> - Support appended DTB.
> ---
> arch/arm/Kconfig | 7 +-
> arch/arm/boot/compressed/Makefile | 5 +-
> .../arm/boot/compressed/fdt_check_mem_start.c | 131 ++++++++++++++++++
> arch/arm/boot/compressed/head.S | 36 ++++-
> 4 files changed, 172 insertions(+), 7 deletions(-)
> create mode 100644 arch/arm/boot/compressed/fdt_check_mem_start.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 138248999df7421e..9860057775ee72a9 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1875,9 +1875,10 @@ config AUTO_ZRELADDR
> help
> ZRELADDR is the physical address where the decompressed kernel
> image will be placed. If AUTO_ZRELADDR is selected, the address
> - will be determined at run-time by masking the current IP with
> - 0xf8000000. This assumes the zImage being placed in the first 128MB
> - from start of memory.
> + will be determined at run-time, either by masking the current IP
> + with 0xf8000000, or, if invalid, from the DTB passed in r2.
> + This assumes the zImage being placed in the first 128MB from
> + start of memory.
>
> config EFI_STUB
> bool
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index fb521efcc6c20a4f..fd94e27ba4fa3d12 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -87,10 +87,13 @@ libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
> ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
> OBJS += $(libfdt_objs) atags_to_fdt.o
> endif
> +ifeq ($(CONFIG_USE_OF),y)
> +OBJS += $(libfdt_objs) fdt_check_mem_start.o
> +endif
>
> # -fstack-protector-strong triggers protection checks in this code,
> # but it is being used too early to link to meaningful stack_chk logic.
> -$(foreach o, $(libfdt_objs) atags_to_fdt.o, \
> +$(foreach o, $(libfdt_objs) atags_to_fdt.o fdt_check_mem_start.o, \
> $(eval CFLAGS_$(o) := -I $(srctree)/scripts/dtc/libfdt -fno-stack-protector))
>
> # These were previously generated C files. When you are building the kernel
> diff --git a/arch/arm/boot/compressed/fdt_check_mem_start.c b/arch/arm/boot/compressed/fdt_check_mem_start.c
> new file mode 100644
> index 0000000000000000..62450d824c3ca180
> --- /dev/null
> +++ b/arch/arm/boot/compressed/fdt_check_mem_start.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kernel.h>
> +#include <linux/libfdt.h>
> +#include <linux/sizes.h>
> +
> +static const void *get_prop(const void *fdt, const char *node_path,
> + const char *property, int minlen)
> +{
> + const void *prop;
> + int offset, len;
> +
> + offset = fdt_path_offset(fdt, node_path);
> + if (offset < 0)
> + return NULL;
> +
> + prop = fdt_getprop(fdt, offset, property, &len);
> + if (!prop || len < minlen)
> + return NULL;
> +
> + return prop;
> +}
> +
> +static uint32_t get_cells(const void *fdt, const char *name)
> +{
> + const fdt32_t *prop = get_prop(fdt, "/", name, sizeof(fdt32_t));
> +
> + if (!prop) {
> + /* default */
> + return 1;
> + }
> +
> + return fdt32_ld(prop);
> +}
> +
> +static uint64_t get_val(const fdt32_t *cells, uint32_t ncells)
> +{
> + uint64_t r;
> +
> + r = fdt32_ld(cells);
> + if (ncells > 1)
> + r = (r << 32) | fdt32_ld(cells + 1);
> +
> + return r;
> +}
> +
> +/*
> + * Check the start of physical memory
> + *
> + * Traditionally, the start address of physical memory is obtained by masking
> + * the program counter. However, this does require that this address is a
> + * multiple of 128 MiB, precluding booting Linux on platforms where this
> + * requirement is not fulfilled.
> + * Hence validate the calculated address against the memory information in the
> + * DTB, and, if out-of-range, replace it by the real start address.
> + * To preserve backwards compatibility (systems reserving a block of memory
> + * at the start of physical memory, kdump, ...), the traditional method is
> + * always used if it yields a valid address.
> + *
> + * Return value: start address of physical memory to use
> + */
> +uint32_t fdt_check_mem_start(uint32_t mem_start, const void *fdt)
> +{
> + uint32_t addr_cells, size_cells, base;
> + uint32_t fdt_mem_start = 0xffffffff;
> + const fdt32_t *reg, *endp;
> + uint64_t size, end;
> + const char *type;
> + int offset, len;
> +
> + if (!fdt)
> + return mem_start;
> +
> + if (fdt_magic(fdt) != FDT_MAGIC)
> + return mem_start;
> +
> + /* There may be multiple cells on LPAE platforms */
> + addr_cells = get_cells(fdt, "#address-cells");
> + size_cells = get_cells(fdt, "#size-cells");
> + if (addr_cells > 2 || size_cells > 2)
> + return mem_start;
> +
> + /* Walk all memory nodes and regions */
> + for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
> + offset = fdt_next_node(fdt, offset, NULL)) {
> + type = fdt_getprop(fdt, offset, "device_type", NULL);
> + if (!type || strcmp(type, "memory"))
> + continue;
> +
> + reg = fdt_getprop(fdt, offset, "linux,usable-memory", &len);
> + if (!reg)
> + reg = fdt_getprop(fdt, offset, "reg", &len);
> + if (!reg)
> + continue;
> +
> + for (endp = reg + (len / sizeof(fdt32_t));
> + endp - reg >= addr_cells + size_cells;
> + reg += addr_cells + size_cells) {
> + size = get_val(reg + addr_cells, size_cells);
> + if (!size)
> + continue;
> +
> + if (addr_cells > 1 && fdt32_ld(reg)) {
> + /* Outside 32-bit address space, skipping */
> + continue;
> + }
> +
> + base = fdt32_ld(reg + addr_cells - 1);
> + end = base + size;
> + if (mem_start >= base && mem_start < end) {
> + /* Calculated address is valid, use it */
> + return mem_start;
> + }
> +
> + if (base < fdt_mem_start)
> + fdt_mem_start = base;
> + }
> + }
> +
> + if (fdt_mem_start == 0xffffffff) {
> + /* No usable memory found, falling back to default */
> + return mem_start;
> + }
> +
> + /*
> + * The calculated address is not usable.
> + * Use the lowest usable physical memory address from the DTB instead,
> + * and make sure this is a multiple of 2 MiB for phys/virt patching.
> + */
> + return round_up(fdt_mem_start, SZ_2M);
> +}
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index d9cce7238a365081..04f6b7bb7c43b66c 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -282,10 +282,40 @@ not_angel:
> * are already placing their zImage in (eg) the top 64MB
> * of this range.
> */
> - mov r4, pc
> - and r4, r4, #0xf8000000
> + mov r0, pc
> + and r0, r0, #0xf8000000
> +#ifdef CONFIG_USE_OF
> + adr r1, LC1
> +#ifdef CONFIG_ARM_APPENDED_DTB
> + /*
> + * Look for an appended DTB. If found, we cannot use it to
> + * validate the calculated start of physical memory, as its
> + * memory nodes may need to be augmented by ATAGS stored at
> + * an offset from the same start of physical memory.
> + */
> + ldr r2, [r1, #4] @ get &_edata
> + add r2, r2, r1 @ relocate it
> + ldr r2, [r2] @ get DTB signature
> + ldr r3, =OF_DT_MAGIC
> + cmp r2, r3 @ do we have a DTB there?
> + beq 1f @ if yes, skip validation
> +#endif /* CONFIG_ARM_APPENDED_DTB */
> +
> + /*
> + * Make sure we have some stack before calling C code.
> + * No GOT fixup has occurred yet, but none of the code we're
> + * about to call uses any global variables.
> + */
> + ldr sp, [r1] @ get stack location
> + add sp, sp, r1 @ apply relocation
> +
> + /* Validate calculated start against passed DTB */
> + mov r1, r8
> + bl fdt_check_mem_start
> +1:
> +#endif /* CONFIG_USE_OF */
> /* Determine final kernel image address. */
> - add r4, r4, #TEXT_OFFSET
> + add r4, r0, #TEXT_OFFSET
> #else
> ldr r4, =zreladdr
> #endif
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
