Re: [PATCH] arc: fix memory initialization for systems with two memory banks

[Vineet Gupta]

Hi Mike,

On 8/28/20 9:39 AM, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
>
> Rework if memory map initialization broke initialization of ARC systems
> with two memory banks. Before these changes, memblock was not aware of
> nodes configuration and the memory map was always allocated from the
> "lowmem" bank. After the addition of node information to memblock, the core
> mm attempts to allocate the memory map for the "highmem" bank from its
> node. The access to this memory using __va() fails because it can be only
> accessed using kmap.
>
> Anther problem that was uncovered is that {min,max}_high_pfn are calculated
> from u64 high_mem_start variable which prevents truncation to 32-bit
> physical address and the PFN values are above the node and zone boundaries.

Not sure if I quite follow this part. We should not be relying on truncation: the
pfn should be derived off of zone addresses ?

> Use phys_addr_t type for high_mem_start and high_mem_size to ensure
> correspondence between PFNs and highmem zone boundaries and reserve the
> entire highmem bank until mem_init() to avoid accesses to it before highmem
> is enabled.
>
> Fixes: 51930df5801e ("mm: free_area_init: allow defining max_zone_pfn in descend ing order")
> Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>

Thx for the fix. I verified that a 2 mem bank system with HIGHMEM enabled now
works again.
And I've also added a couple of lines to changelog to describe how to test such a
config.

|    To test this:
|    1. Enable HIGHMEM in ARC config
|    2. Enable 2 memory banks in haps_hs.dts (uncomment the 2nd bank)


> ---
>  arch/arc/mm/init.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> index f886ac69d8ad..3a35b82a718e 100644
> --- a/arch/arc/mm/init.c
> +++ b/arch/arc/mm/init.c
> @@ -26,8 +26,8 @@ static unsigned long low_mem_sz;
>  
>  #ifdef CONFIG_HIGHMEM
>  static unsigned long min_high_pfn, max_high_pfn;
> -static u64 high_mem_start;
> -static u64 high_mem_sz;
> +static phys_addr_t high_mem_start;
> +static phys_addr_t high_mem_sz;
>  #endif
>  
>  #ifdef CONFIG_DISCONTIGMEM
> @@ -69,6 +69,7 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>  		high_mem_sz = size;
>  		in_use = 1;
>  		memblock_add_node(base, size, 1);
> +		memblock_reserve(base, size);
>  #endif
>  	}
>  
> @@ -157,7 +158,7 @@ void __init setup_arch_memory(void)
>  	min_high_pfn = PFN_DOWN(high_mem_start);
>  	max_high_pfn = PFN_DOWN(high_mem_start + high_mem_sz);
>  
> -	max_zone_pfn[ZONE_HIGHMEM] = max_high_pfn;
> +	max_zone_pfn[ZONE_HIGHMEM] = min_low_pfn;
>  
>  	high_memory = (void *)(min_high_pfn << PAGE_SHIFT);
>  	kmap_init();
> @@ -166,22 +167,26 @@ void __init setup_arch_memory(void)
>  	free_area_init(max_zone_pfn);
>  }
>  
> -/*
> - * mem_init - initializes memory
> - *
> - * Frees up bootmem
> - * Calculates and displays memory available/used
> - */
> -void __init mem_init(void)
> +static void __init highmem_init(void)
>  {
>  #ifdef CONFIG_HIGHMEM
>  	unsigned long tmp;
>  
> -	reset_all_zones_managed_pages();
> +	memblock_free(high_mem_start, high_mem_sz);
>  	for (tmp = min_high_pfn; tmp < max_high_pfn; tmp++)
>  		free_highmem_page(pfn_to_page(tmp));
>  #endif
> +}
>  
> +/*
> + * mem_init - initializes memory
> + *
> + * Frees up bootmem
> + * Calculates and displays memory available/used
> + */
> +void __init mem_init(void)
> +{
>  	memblock_free_all();
> +	highmem_init();
>  	mem_init_print_info(NULL);
>  }