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

[Mike Rapoport]

On Fri, Aug 28, 2020 at 10:17:28PM +0000, Vineet Gupta wrote:
> 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 ?

Before the refactoring of the memmap initialization, we used

	free_area_init_node(1, zones_size, min_high_pfn, zones_holes);

With min_high_pfn being u64, min_node_pfn would be 0x80000 (presuming 8k
page and haps_hs.dts). But since we explicitly passed the min_node_pfn,
it will be eventually used at zone->zone_start_pfn, so HIGHMEM zone
would span from 0x80000.

After the refactoring, we use memblock information and architectural
limits for zone extents to detect actual zone span. Memblock uses
phys_addr_t to represent memory banks and if we still calculate
min_high_pfn using u64 there is a mismatch between pfn (0x80000) and
physicall address (0x0).

> > 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)

The second bank already enabled in the dts ;-)

I think its worthwhile adding this to the wiki [1] since it's not likely
people could dig this from the kernel log.

[1] https://github.com/foss-for-synopsys-dwc-arc-processors/linux/wiki/How-to-run-ARC-Linux-kernel-and-debug-(with-MetaWare-Debugger)

> > ---
> >  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);
> >  }
> 

-- 
Sincerely yours,
Mike.