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

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

[Mike Rapoport]

From: Mike Rapoport <rppt@linux.ibm.com>

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.

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@linux.ibm.com>
---
 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);
 }
-- 
2.26.2

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@linux.ibm.com>
>
> 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@linux.ibm.com>

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

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@linux.ibm.com>
> >
> > 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@linux.ibm.com>
> 
> 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.