Re: [PATCH v3 3/3] ARM: dts: broadcom: Use CFE CPU bring-up on the XG6846B

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ARM: dts: broadcom: xg6846b partial no-map removal breaks lowmem setup
--

commit 83c5177a7db3728e4dd180472b439b349fdb5f65
Author: Linus Walleij <linusw@kernel.org>

ARM: dts: broadcom: Use CFE CPU bring-up on the XG6846B

This commit disables the inherited PSCI node on the Genexis XG6846B and
configures the CPUs to use the Broadcom BCA CFE SMP bring-up method. It
also removes the no-map property from the secondary-boot@0 reserved-memory
node so the reservation does not fail when overlapping the initial page
tables.

> diff --git a/arch/arm/boot/dts/broadcom/bcm6846-genexis-xg6846b.dts b/arch/arm/boot/dts/broadcom/bcm6846-genexis-xg6846b.dts
> index a3616fb7b3a8d..45d4df5231109 100644
> --- a/arch/arm/boot/dts/broadcom/bcm6846-genexis-xg6846b.dts
> +++ b/arch/arm/boot/dts/broadcom/bcm6846-genexis-xg6846b.dts
> @@ -34,7 +34,7 @@ reserved-memory {
>  		ranges;
>  
>  		secondary-boot@0 {
> -			no-map;
> +			/* Includes the kernel's initial page tables. */
>  			reg = <0x00000000 0x00008000>;
>  		};
>  		pmc3-firmware@8000 {

[Severity: High]
Could removing no-map from secondary-boot@0 while leaving no-map on the
immediately adjacent pmc3-firmware@8000 node cause a boot regression in
adjust_lowmem_bounds(), prepare_page_table(), and map_lowmem()?

In arch/arm/boot/dts/broadcom/bcm6846-genexis-xg6846b.dts, reserved-memory
now has a mapped 32 KiB region followed by a 1 MiB no-map region:

		secondary-boot@0 {
			/* Includes the kernel's initial page tables. */
			reg = <0x00000000 0x00008000>;
		};
		pmc3-firmware@8000 {
			no-map;
			reg = <0x00008000 0x00100000>;
		};

On multi_v7_defconfig (which enables CONFIG_ARCH_BCMBCA=y alongside
CONFIG_ARCH_MESON=y), arch/arm/Makefile sets TEXT_OFFSET to 0x00208000,
placing swapper_pg_dir at physical [0x00204000, 0x00208000) and
KERNEL_START (_stext) at 0x00208000.

During setup_arch() -> arm_memblock_init() ->
early_init_fdt_scan_reserved_mem(), secondary-boot@0 is reserved via
memblock_reserve(0x0, 0x8000) and remains mapped in memblock.memory, while
pmc3-firmware@8000 does not overlap the kernel at 0x00208000 and is marked
MEMBLOCK_NOMAP via memblock_mark_nomap(0x8000, 0x100000). This splits
memblock.memory into:

  regions[0] = [0x00000000, 0x00008000) (MEMBLOCK_NONE)
  regions[1] = [0x00008000, 0x00108000) (MEMBLOCK_NOMAP)
  regions[2] = [0x00108000, 0x10000000) (MEMBLOCK_NONE)

In arch/arm/mm/mmu.c:adjust_lowmem_bounds(), the first loop checks whether
block_start of the first usable range [0x0, 0x8000) is aligned to PMD_SIZE
(which succeeds because 0x0 is aligned) and breaks without aligning
regions[2] (0x00108000). The second loop then checks block_end on
[0x0, 0x8000):

	for_each_mem_range(i, &block_start, &block_end) {
		if (block_start < vmalloc_limit) {
			if (block_end > lowmem_limit)
				lowmem_limit = min_t(u64,
							 vmalloc_limit,
							 block_end);

			if (!memblock_limit) {
				if (!IS_ALIGNED(block_start, PMD_SIZE))
					memblock_limit = block_start;
				else if (!IS_ALIGNED(block_end, PMD_SIZE))
					memblock_limit = lowmem_limit;
			}

		}
	}

Because block_end is 0x8000 (!IS_ALIGNED(block_end, PMD_SIZE)),
memblock_limit is set to 0x8000, which round_down(memblock_limit, PMD_SIZE)
rounds down to 0, calling memblock_set_current_limit(0).

Next, in paging_init() -> prepare_page_table(), end is computed from
memblock.memory.regions[0] as 0x8000:

	end = memblock.memory.regions[0].base + memblock.memory.regions[0].size;
	if (end >= arm_lowmem_limit)
		end = arm_lowmem_limit;

	for (addr = __phys_to_virt(end);
	     addr < VMALLOC_START; addr += PMD_SIZE)
		pmd_clear(pmd_off_k(addr));

This clears all kernel PMDs from __phys_to_virt(0x8000) up to
VMALLOC_START, unmapping the running kernel at 0xc0208000. Immediately
afterward in paging_init() -> map_lowmem(), processing regions[0]
([0x0, 0x8000)) with kernel_sec_start == 0 == start and end (0x8000) <
kernel_sec_end hits Case 1:

			if (kernel_sec_start == start)
				start = kernel_sec_end;
			if (kernel_sec_end == end)
				end = kernel_sec_start;
		...
		map.pfn = __phys_to_pfn(start);
		map.virtual = __phys_to_virt(start);
		map.length = end - start;
		map.type = MT_MEMORY_RW;
		create_mapping(&map);

which sets start = kernel_sec_end while end remains 0x8000, underflowing
map.length = end - start while memblock.current_limit is 0.

Conversely, on configs where TEXT_OFFSET == 0x00008000 (such as
bcm6846_defconfig), KERNEL_START is at physical 0x00008000 and is already
reserved by arm_memblock_init() before early_init_fdt_scan_reserved_mem()
runs, so pmc3-firmware@8000 with no-map overlaps the kernel image and fails
in early_init_dt_reserve_memory() with -EBUSY (which is the only reason
memblock.memory is not split at 0x8000 on bcm6846_defconfig).

Should no-map also be removed from pmc3-firmware@8000, or how is
pmc3-firmware@8000 intended to coexist with the kernel image at physical
0x00008000 and 32-bit ARM's PMD_SIZE lowmem alignment requirements?

[ ... ]

>
>base-commit: 6356fd3dd29d8b5620507a0ce037c644e052b5dd
>-- 
>2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-genexis-xg6846-smp-v7-3-rc1-v3-0-7c871f1755db@kernel.org?part=3