Re: [PATCH 10/13] arch, mm: set high_memory in free_area_init()

On Tue, Mar 11, 2025 at 09:59:32PM +0000, Russell King (Oracle) wrote:
> On Tue, Mar 11, 2025 at 05:51:06PM +0000, Mark Brown wrote:
> > On Thu, Mar 06, 2025 at 08:51:20PM +0200, Mike Rapoport wrote:
> > > From: "Mike Rapoport (Microsoft)" <[email protected]>
> > > 
> > > high_memory defines upper bound on the directly mapped memory.
> > > This bound is defined by the beginning of ZONE_HIGHMEM when a system has
> > > high memory and by the end of memory otherwise.
> > > 
> > > All this is known to generic memory management initialization code that
> > > can set high_memory while initializing core mm structures.
> > > 
> > > Remove per-architecture calculation of high_memory and add a generic
> > > version to free_area_init().
> > 
> > This patch appears to be causing breakage on a number of 32 bit arm
> > platforms, including qemu's virt-2.11,gic-version=3.  Affected platforms
> > die on boot with no output, a bisect with qemu points at this commit and
> > those for physical platforms appear to be converging on the same place.
> 
> I'm not convinced that the old and the new code is doing the same
> thing.
> 
> The new code:
> 
> +       phys_addr_t highmem = memblock_end_of_DRAM();
> +
> +#ifdef CONFIG_HIGHMEM
> +       unsigned long pfn = arch_zone_lowest_possible_pfn[ZONE_HIGHMEM];
> +
> +       if (arch_has_descending_max_zone_pfns() || highmem > PFN_PHYS(pfn))
> +               highmem = PFN_PHYS(pfn);
> +#endif
> +
> +       high_memory = phys_to_virt(highmem - 1) + 1;
> 
> First, when CONFIG_HIGHMEM is disabled, this code assumes that the last
> byte of DRAM declared to memblock is the highmem limit. This _could_
> overflow phys_to_virt() and lead to an invalid value for high_memory.
> 
> Second, arch_zone_lowest_possible_pfn[ZONE_HIGHMEM] is the _start_ of
> highmem. This is not what arch code sets high_memory to - because
> the start of highmem may not contiguously follow on from lowmem.
> 
> In arch/arm/mm/mmu.c, lowmem_limit is computed to be the highest + 1
> physical address that lowmem can possibly be, taking into account the
> amount of vmalloc memory that is required. This is used to set
> high_memory.
> 
> We also limit the amount of usable RAM via memblock_set_current_limit()
> which memblock_end_of_DRAM() doesn't respect.
> 
> I don't think the proposed generic version is suitable for 32-bit arm.

Unless I'm missing something, both memblock.current_limit and start of
ZONE_HIGHMEM are set to arm_lowmem_limit which will be different from
memblock_end_of_DRAM() only for machines with more than nearly 4GiB of RAM
and those will supposedly use HIGHMEM anyway.

But this does not matter anyway because failures Mark reported happen
because 32-bit arm uses high_memory before mem_init() and that what causes
the hangs. 

Here's the fix I have, I'll send v2 shortly.

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e492d58a0386..f02f872ea8a9 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1250,6 +1250,8 @@ void __init adjust_lowmem_bounds(void)
 
 	arm_lowmem_limit = lowmem_limit;
 
+	high_memory = __va(arm_lowmem_limit - 1) + 1;
+
 	if (!memblock_limit)
 		memblock_limit = arm_lowmem_limit;
 
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index 65903ed5e80d..1a8f6914ee59 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -146,6 +146,7 @@ void __init adjust_lowmem_bounds(void)
 	phys_addr_t end;
 	adjust_lowmem_bounds_mpu();
 	end = memblock_end_of_DRAM();
+	high_memory = __va(end - 1) + 1;
 	memblock_set_current_limit(end);
 }
 
diff --git a/mm/mm_init.c b/mm/mm_init.c
index 545e11f1a3ba..0aef4bef93c4 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -1765,14 +1765,20 @@ static bool arch_has_descending_max_zone_pfns(void)
 
 static void set_high_memory(void)
 {
+	unsigned long pfn = arch_zone_lowest_possible_pfn[ZONE_HIGHMEM];
 	phys_addr_t highmem = memblock_end_of_DRAM();
 
-#ifdef CONFIG_HIGHMEM
-	unsigned long pfn = arch_zone_lowest_possible_pfn[ZONE_HIGHMEM];
+	/*
+	 * Some architectures (e.g. ARM) set high_memory very early and
+	 * use it in arch setup code.
+	 * If an architecture already set high_memory don't overwrite it
+	 */
+	if (high_memory)
+		return;
 
-	if (arch_has_descending_max_zone_pfns() || highmem > PFN_PHYS(pfn))
+	if (IS_ENABLED(CONFIG_HIGHMEM) &&
+	    (arch_has_descending_max_zone_pfns() || highmem > PFN_PHYS(pfn)))
 		highmem = PFN_PHYS(pfn);
-#endif
 
 	high_memory = phys_to_virt(highmem - 1) + 1;
 }

-- 
Sincerely yours,
Mike.