[PATCH 0/6] DISCONTIGMEM support for PPC32

[PATCH 0/6] DISCONTIGMEM support for PPC32

[Jonathan Neuschäfer]

This patchset adds support for DISCONTIGMEM on 32-bit PowerPC. This is
required to properly support the Nintendo Wii's memory layout, in which
there are two blocks of RAM and MMIO in the middle.

Previously, this memory layout was handled by code that joins the two
RAM blocks into one, reserves the MMIO hole, and permits allocations of
reserved memory in ioremap. This hack didn't work with resource-based
allocation (as used for example in the GPIO driver for Wii[1]), however.

After this patchset, users of the Wii can either select CONFIG_FLATMEM
to get the old behaviour, or CONFIG_DISCONTIGMEM to get the new
behaviour.

Some parts of this patchset are probably not ideal (I'm thinking of my
implementation of pfn_to_nid here), and will require some discussion/
changes.

[1]: https://www.spinics.net/lists/devicetree/msg213956.html

Jonathan Neuschäfer (6):
  powerpc/mm/32: Use pfn_valid to check if pointer is in RAM
  powerpc: numa: Fix overshift on PPC32
  powerpc: numa: Use the right #ifdef guards around functions
  powerpc: numa: Restrict fake NUMA enulation to CONFIG_NUMA systems
  powerpc: Implement DISCONTIGMEM and allow selection on PPC32
  powerpc: wii: Don't rely on reserved memory hack if DISCONTIGMEM is
    set

 arch/powerpc/Kconfig                     |  5 ++++-
 arch/powerpc/include/asm/mmzone.h        | 21 +++++++++++++++++++++
 arch/powerpc/mm/numa.c                   | 18 +++++++++++++++---
 arch/powerpc/mm/pgtable_32.c             |  2 +-
 arch/powerpc/platforms/embedded6xx/wii.c | 10 +++++++---
 5 files changed, 48 insertions(+), 8 deletions(-)

-- 
2.16.1

[PATCH 1/6] powerpc/mm/32: Use pfn_valid to check if pointer is in RAM

[Jonathan Neuschäfer]

The Nintendo Wii has a memory layout that places two chunks of RAM at
non-adjacent addresses, and MMIO between them. Currently, the allocation
of these MMIO areas is made possible by declaring the MMIO hole as
reserved memory and allowing reserved memory to be allocated (cf.
wii_memory_fixups).

This patch is the first step towards proper support for discontiguous
memory on PPC32 by using pfn_valid to check if a pointer points into
RAM, rather than open-coding the check. It should result in no
functional difference.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/powerpc/mm/pgtable_32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index d35d9ad3c1cd..b5c009893a44 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -147,7 +147,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags,
 	 * Don't allow anybody to remap normal RAM that we're using.
 	 * mem_init() sets high_memory so only do the check after that.
 	 */
-	if (slab_is_available() && (p < virt_to_phys(high_memory)) &&
+	if (slab_is_available() && pfn_valid(__phys_to_pfn(p)) &&
 	    !(__allow_ioremap_reserved && memblock_is_region_reserved(p, size))) {
 		printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n",
 		       (unsigned long long)p, __builtin_return_address(0));
-- 
2.16.1

[PATCH 2/6] powerpc: numa: Fix overshift on PPC32

[Jonathan Neuschäfer]

When read_n_cells is compiled for PPC32, "result << 32" causes an
overshift, which GCC doesn't like. Fix this by using u64 instead of
unsigned long.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/powerpc/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index edd8d0bc9364..0570bc2a0b13 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -357,9 +357,9 @@ static void __init get_n_mem_cells(int *n_addr_cells, int *n_size_cells)
 	of_node_put(memory);
 }
 
-static unsigned long read_n_cells(int n, const __be32 **buf)
+static u64 read_n_cells(int n, const __be32 **buf)
 {
-	unsigned long result = 0;
+	u64 result = 0;
 
 	while (n--) {
 		result = (result << 32) | of_read_number(*buf, 1);
-- 
2.16.1

[PATCH 3/6] powerpc: numa: Use the right #ifdef guards around functions

[Jonathan Neuschäfer]

of_node_to_nid and dump_numa_cpu_topology are declared inline in their
respective header files, if CONFIG_NUMA is not set. Thus it is only
valid to define these functions in numa.c if CONFIG_NUMA is set.
(numa.c, despite the name, isn't conditionalized on CONFIG_NUMA, but
CONFIG_NEED_MULTIPLE_NODES.)

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/powerpc/mm/numa.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 0570bc2a0b13..df03a65b658f 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -254,6 +254,7 @@ static int of_node_to_nid_single(struct device_node *device)
 	return nid;
 }
 
+#ifdef CONFIG_NUMA
 /* Walk the device tree upwards, looking for an associativity id */
 int of_node_to_nid(struct device_node *device)
 {
@@ -272,6 +273,7 @@ int of_node_to_nid(struct device_node *device)
 	return nid;
 }
 EXPORT_SYMBOL(of_node_to_nid);
+#endif
 
 static int __init find_min_common_depth(void)
 {
@@ -744,6 +746,7 @@ static void __init setup_nonnuma(void)
 	}
 }
 
+#ifdef CONFIG_NUMA
 void __init dump_numa_cpu_topology(void)
 {
 	unsigned int node;
@@ -778,6 +781,7 @@ void __init dump_numa_cpu_topology(void)
 		pr_cont("\n");
 	}
 }
+#endif
 
 /* Initialize NODE_DATA for a node on the local memory */
 static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
-- 
2.16.1

[PATCH 4/6] powerpc: numa: Restrict fake NUMA enulation to CONFIG_NUMA systems

[Jonathan Neuschäfer]

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/powerpc/mm/numa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index df03a65b658f..dfe279529463 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -738,7 +738,8 @@ static void __init setup_nonnuma(void)
 		start_pfn = memblock_region_memory_base_pfn(reg);
 		end_pfn = memblock_region_memory_end_pfn(reg);
 
-		fake_numa_create_new_node(end_pfn, &nid);
+		if (IS_ENABLED(CONFIG_NUMA))
+			fake_numa_create_new_node(end_pfn, &nid);
 		memblock_set_node(PFN_PHYS(start_pfn),
 				  PFN_PHYS(end_pfn - start_pfn),
 				  &memblock.memory, nid);
-- 
2.16.1

[PATCH 5/6] powerpc: Implement DISCONTIGMEM and allow selection on PPC32

[Jonathan Neuschäfer]

The implementation of pfn_to_nid and pfn_valid in mmzone.h is based on
arch/metag's implementation.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

NOTE: Checking NODE_DATA(nid) in pfn_to_nid appears to be uncommon.
Running up to MAX_NUMNODES instead of checking NODE_DATA(nid) would
require the node_data array to be filled with valid pointers.
---
 arch/powerpc/Kconfig              |  5 ++++-
 arch/powerpc/include/asm/mmzone.h | 21 +++++++++++++++++++++
 arch/powerpc/mm/numa.c            |  7 +++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 73ce5dd07642..c2633b7b8ed9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -639,7 +639,6 @@ config HAVE_MEMORYLESS_NODES
 
 config ARCH_SELECT_MEMORY_MODEL
 	def_bool y
-	depends on PPC64
 
 config ARCH_FLATMEM_ENABLE
 	def_bool y
@@ -654,6 +653,10 @@ config ARCH_SPARSEMEM_DEFAULT
 	def_bool y
 	depends on PPC_BOOK3S_64
 
+config ARCH_DISCONTIGMEM_ENABLE
+	def_bool y
+	depends on PPC32
+
 config SYS_SUPPORTS_HUGETLBFS
 	bool
 
diff --git a/arch/powerpc/include/asm/mmzone.h b/arch/powerpc/include/asm/mmzone.h
index 91c69ff53a8a..1684a5c98d23 100644
--- a/arch/powerpc/include/asm/mmzone.h
+++ b/arch/powerpc/include/asm/mmzone.h
@@ -26,6 +26,27 @@ extern struct pglist_data *node_data[];
  */
 #define NODE_DATA(nid)		(node_data[nid])
 
+static inline int pfn_to_nid(unsigned long pfn)
+{
+	int nid;
+
+	for (nid = 0; nid < MAX_NUMNODES && NODE_DATA(nid); nid++)
+		if (pfn >= node_start_pfn(nid) && pfn <= node_end_pfn(nid))
+			return nid;
+
+	return -1;
+}
+
+static inline int pfn_valid(int pfn)
+{
+	int nid = pfn_to_nid(pfn);
+
+	if (nid >= 0)
+		return pfn < node_end_pfn(nid);
+
+	return 0;
+}
+
 /*
  * Following are specific to this numa platform.
  */
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index dfe279529463..ec47f1081509 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -744,6 +744,13 @@ static void __init setup_nonnuma(void)
 				  PFN_PHYS(end_pfn - start_pfn),
 				  &memblock.memory, nid);
 		node_set_online(nid);
+
+		/*
+		 * On DISCONTIGMEM systems, place different memory blocks into
+		 * different nodes.
+		 */
+		if (IS_ENABLED(CONFIG_DISCONTIGMEM) && nid < MAX_NUMNODES - 1)
+			nid++;
 	}
 }
 
-- 
2.16.1

[PATCH 6/6] powerpc: wii: Don't rely on reserved memory hack if DISCONTIGMEM is set

[Jonathan Neuschäfer]

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/powerpc/platforms/embedded6xx/wii.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index 4682327f76a9..589ac87a3ce0 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -81,13 +81,16 @@ void __init wii_memory_fixups(void)
 	BUG_ON(memblock.memory.cnt != 2);
 	BUG_ON(!page_aligned(p[0].base) || !page_aligned(p[1].base));
 
+	/* determine hole */
+	wii_hole_start = ALIGN(p[0].base + p[0].size, PAGE_SIZE);
+	wii_hole_size = p[1].base - wii_hole_start;
+
+#ifndef CONFIG_DISCONTIGMEM
 	/* trim unaligned tail */
 	memblock_remove(ALIGN(p[1].base + p[1].size, PAGE_SIZE),
 			(phys_addr_t)ULLONG_MAX);
 
-	/* determine hole, add & reserve them */
-	wii_hole_start = ALIGN(p[0].base + p[0].size, PAGE_SIZE);
-	wii_hole_size = p[1].base - wii_hole_start;
+	/* add & reserve hole */
 	memblock_add(wii_hole_start, wii_hole_size);
 	memblock_reserve(wii_hole_start, wii_hole_size);
 
@@ -96,6 +99,7 @@ void __init wii_memory_fixups(void)
 
 	/* allow ioremapping the address space in the hole */
 	__allow_ioremap_reserved = 1;
+#endif
 }
 
 unsigned long __init wii_mmu_mapin_mem2(unsigned long top)
-- 
2.16.1

Re: [PATCH 1/6] powerpc/mm/32: Use pfn_valid to check if pointer is in RAM

[christophe leroy]

Le 20/02/2018 à 17:14, Jonathan Neuschäfer a écrit :
> The Nintendo Wii has a memory layout that places two chunks of RAM at
> non-adjacent addresses, and MMIO between them. Currently, the allocation
> of these MMIO areas is made possible by declaring the MMIO hole as
> reserved memory and allowing reserved memory to be allocated (cf.
> wii_memory_fixups).
> 
> This patch is the first step towards proper support for discontiguous
> memory on PPC32 by using pfn_valid to check if a pointer points into
> RAM, rather than open-coding the check. It should result in no
> functional difference.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>   arch/powerpc/mm/pgtable_32.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index d35d9ad3c1cd..b5c009893a44 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -147,7 +147,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags,
>   	 * Don't allow anybody to remap normal RAM that we're using.
>   	 * mem_init() sets high_memory so only do the check after that.
>   	 */
> -	if (slab_is_available() && (p < virt_to_phys(high_memory)) &&
> +	if (slab_is_available() && pfn_valid(__phys_to_pfn(p)) &&

I'm not sure this is equivalent:

high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
#define ARCH_PFN_OFFSET		((unsigned long)(MEMORY_START >> PAGE_SHIFT))
#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
set_max_mapnr(max_pfn);

So in the current implementation it checks against max_low_pfn while 
your patch checks against max_pfn

	max_low_pfn = max_pfn = memblock_end_of_DRAM() >> PAGE_SHIFT;
#ifdef CONFIG_HIGHMEM
	max_low_pfn = lowmem_end_addr >> PAGE_SHIFT;
#endif

Christophe

>   	    !(__allow_ioremap_reserved && memblock_is_region_reserved(p, size))) {
>   		printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n",
>   		       (unsigned long long)p, __builtin_return_address(0));
> 

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH 5/6] powerpc: Implement DISCONTIGMEM and allow selection on PPC32

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

Hi Jonathan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jonathan-Neusch-fer/DISCONTIGMEM-support-for-PPC32/20180221-044840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   In file included from include/linux/gfp.h:6:0,
                    from include/linux/mm.h:10,
                    from include/linux/mman.h:5,
                    from arch/powerpc/kernel/asm-offsets.c:22:
>> include/linux/mmzone.h:1239:19: error: conflicting types for 'pfn_valid'
    static inline int pfn_valid(unsigned long pfn)
                      ^~~~~~~~~
   In file included from include/linux/mmzone.h:912:0,
                    from include/linux/gfp.h:6,
                    from include/linux/mm.h:10,
                    from include/linux/mman.h:5,
                    from arch/powerpc/kernel/asm-offsets.c:22:
   arch/powerpc/include/asm/mmzone.h:40:19: note: previous definition of 'pfn_valid' was here
    static inline int pfn_valid(int pfn)
                      ^~~~~~~~~
   make[2]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +/pfn_valid +1239 include/linux/mmzone.h

c4e1be9e Dave Hansen    2017-07-06  1237  
7b7bf499 Will Deacon    2011-05-19  1238  #ifndef CONFIG_HAVE_ARCH_PFN_VALID
d41dee36 Andy Whitcroft 2005-06-23 @1239  static inline int pfn_valid(unsigned long pfn)
d41dee36 Andy Whitcroft 2005-06-23  1240  {
d41dee36 Andy Whitcroft 2005-06-23  1241  	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
d41dee36 Andy Whitcroft 2005-06-23  1242  		return 0;
29751f69 Andy Whitcroft 2005-06-23  1243  	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
d41dee36 Andy Whitcroft 2005-06-23  1244  }
7b7bf499 Will Deacon    2011-05-19  1245  #endif
d41dee36 Andy Whitcroft 2005-06-23  1246  

:::::: The code at line 1239 was first introduced by commit
:::::: d41dee369bff3b9dcb6328d4d822926c28cc2594 [PATCH] sparsemem memory model

:::::: TO: Andy Whitcroft <apw@shadowen.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24021 bytes --]

Re: [PATCH 0/6] DISCONTIGMEM support for PPC32

[Christophe LEROY]

Le 20/02/2018 à 17:14, Jonathan Neuschäfer a écrit :
> This patchset adds support for DISCONTIGMEM on 32-bit PowerPC. This is
> required to properly support the Nintendo Wii's memory layout, in which
> there are two blocks of RAM and MMIO in the middle.
> 
> Previously, this memory layout was handled by code that joins the two
> RAM blocks into one, reserves the MMIO hole, and permits allocations of
> reserved memory in ioremap. This hack didn't work with resource-based
> allocation (as used for example in the GPIO driver for Wii[1]), however.
> 
> After this patchset, users of the Wii can either select CONFIG_FLATMEM
> to get the old behaviour, or CONFIG_DISCONTIGMEM to get the new
> behaviour.

My question might me stupid, as I don't know PCC64 in deep, but when 
looking at page_is_ram() in arch/powerpc/mm/mem.c, I have the feeling 
the PPC64 implements ram by blocks. Isn't it what you are trying to 
achieve ? Wouldn't it be feasible to map to what's done in PPC64 for PPC32 ?

Christophe

> 
> Some parts of this patchset are probably not ideal (I'm thinking of my
> implementation of pfn_to_nid here), and will require some discussion/
> changes.
> 
> [1]: https://www.spinics.net/lists/devicetree/msg213956.html
> 
> Jonathan Neuschäfer (6):
>    powerpc/mm/32: Use pfn_valid to check if pointer is in RAM
>    powerpc: numa: Fix overshift on PPC32
>    powerpc: numa: Use the right #ifdef guards around functions
>    powerpc: numa: Restrict fake NUMA enulation to CONFIG_NUMA systems
>    powerpc: Implement DISCONTIGMEM and allow selection on PPC32
>    powerpc: wii: Don't rely on reserved memory hack if DISCONTIGMEM is
>      set
> 
>   arch/powerpc/Kconfig                     |  5 ++++-
>   arch/powerpc/include/asm/mmzone.h        | 21 +++++++++++++++++++++
>   arch/powerpc/mm/numa.c                   | 18 +++++++++++++++---
>   arch/powerpc/mm/pgtable_32.c             |  2 +-
>   arch/powerpc/platforms/embedded6xx/wii.c | 10 +++++++---
>   5 files changed, 48 insertions(+), 8 deletions(-)
> 

Re: [PATCH 1/6] powerpc/mm/32: Use pfn_valid to check if pointer is in RAM

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

Hello Christophe,

On Tue, Feb 20, 2018 at 06:45:09PM +0100, christophe leroy wrote:
[...]
> > -	if (slab_is_available() && (p < virt_to_phys(high_memory)) &&
> > +	if (slab_is_available() && pfn_valid(__phys_to_pfn(p)) &&
> 
> I'm not sure this is equivalent:
> 
> high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
> #define ARCH_PFN_OFFSET		((unsigned long)(MEMORY_START >> PAGE_SHIFT))
> #define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> set_max_mapnr(max_pfn);
> 
> So in the current implementation it checks against max_low_pfn while your
> patch checks against max_pfn
> 
> 	max_low_pfn = max_pfn = memblock_end_of_DRAM() >> PAGE_SHIFT;
> #ifdef CONFIG_HIGHMEM
> 	max_low_pfn = lowmem_end_addr >> PAGE_SHIFT;
> #endif

Good point, I haven't considered CONFIG_HIGHMEM before.

As far as I understand it, in the non-CONFIG_HIGHMEM case:

  - max_low_pfn is set to the same value as max_pfn, so the ioremap
    check should detect the same PFNs as RAM.

and with CONFIG_HIGHMEM:

  - max_low_pfn is set to lowmem_end_addr >> PAGE_SHIFT
  - but max_pfn isn't

So, I think you're right.


While looking through arch/powerpc/mm, I noticed that there's a
page_is_ram function, which simply uses the memblocks directly, on
PPC32. It seems like a good candidate for the RAM check in
__ioremap_caller, except that there's this code, which apparently
trashes memblock 0 completely on non-CONFIG_NEED_MULTIPLE_NODES:

  https://elixir.bootlin.com/linux/v4.16-rc2/source/arch/powerpc/mm/mem.c#L223


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] DISCONTIGMEM support for PPC32

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

Hi,

On Wed, Feb 21, 2018 at 08:06:10AM +0100, Christophe LEROY wrote:
> 
> 
> Le 20/02/2018 à 17:14, Jonathan Neuschäfer a écrit :
> > This patchset adds support for DISCONTIGMEM on 32-bit PowerPC. This is
> > required to properly support the Nintendo Wii's memory layout, in which
> > there are two blocks of RAM and MMIO in the middle.
> > 
> > Previously, this memory layout was handled by code that joins the two
> > RAM blocks into one, reserves the MMIO hole, and permits allocations of
> > reserved memory in ioremap. This hack didn't work with resource-based
> > allocation (as used for example in the GPIO driver for Wii[1]), however.
> > 
> > After this patchset, users of the Wii can either select CONFIG_FLATMEM
> > to get the old behaviour, or CONFIG_DISCONTIGMEM to get the new
> > behaviour.
> 
> My question might me stupid, as I don't know PCC64 in deep, but when looking
> at page_is_ram() in arch/powerpc/mm/mem.c, I have the feeling the PPC64
> implements ram by blocks. Isn't it what you are trying to achieve ? Wouldn't
> it be feasible to map to what's done in PPC64 for PPC32 ?

Using page_is_ram in __ioremap_caller and the same memblock-based
approach that's used on PPC64 on PPC32 *should* work, but I think due to
the following line in initmem_init, it won't:

	memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0);


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] powerpc/mm/32: Use pfn_valid to check if pointer is in RAM

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Wed, Feb 21, 2018 at 02:51:19PM +0100, Jonathan Neuschäfer wrote:
[...]
> While looking through arch/powerpc/mm, I noticed that there's a
> page_is_ram function, which simply uses the memblocks directly, on
> PPC32.

Oops, I misread the code here. memblock is used on PPC64.

> It seems like a good candidate for the RAM check in
> __ioremap_caller, except that there's this code, which apparently
> trashes memblock 0 completely on non-CONFIG_NEED_MULTIPLE_NODES:
> 
>   https://elixir.bootlin.com/linux/v4.16-rc2/source/arch/powerpc/mm/mem.c#L223
> 
> 
> Thanks,
> Jonathan Neuschäfer



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] DISCONTIGMEM support for PPC32

[Christophe LEROY]

Le 21/02/2018 à 15:42, Jonathan Neuschäfer a écrit :
> Hi,
> 
> On Wed, Feb 21, 2018 at 08:06:10AM +0100, Christophe LEROY wrote:
>>
>>
>> Le 20/02/2018 à 17:14, Jonathan Neuschäfer a écrit :
>>> This patchset adds support for DISCONTIGMEM on 32-bit PowerPC. This is
>>> required to properly support the Nintendo Wii's memory layout, in which
>>> there are two blocks of RAM and MMIO in the middle.
>>>
>>> Previously, this memory layout was handled by code that joins the two
>>> RAM blocks into one, reserves the MMIO hole, and permits allocations of
>>> reserved memory in ioremap. This hack didn't work with resource-based
>>> allocation (as used for example in the GPIO driver for Wii[1]), however.
>>>
>>> After this patchset, users of the Wii can either select CONFIG_FLATMEM
>>> to get the old behaviour, or CONFIG_DISCONTIGMEM to get the new
>>> behaviour.
>>
>> My question might me stupid, as I don't know PCC64 in deep, but when looking
>> at page_is_ram() in arch/powerpc/mm/mem.c, I have the feeling the PPC64
>> implements ram by blocks. Isn't it what you are trying to achieve ? Wouldn't
>> it be feasible to map to what's done in PPC64 for PPC32 ?
> 
> Using page_is_ram in __ioremap_caller and the same memblock-based
> approach that's used on PPC64 on PPC32 *should* work, but I think due to
> the following line in initmem_init, it won't:
> 
> 	memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0);

Can't we just fix that ?

Christophe

> 
> 
> Thanks,
> Jonathan Neuschäfer
> 

Re: [PATCH 5/6] powerpc: Implement DISCONTIGMEM and allow selection on PPC32

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Wed, Feb 21, 2018 at 07:46:28AM +0800, kbuild test robot wrote:
[...]
> >> include/linux/mmzone.h:1239:19: error: conflicting types for 'pfn_valid'
>     static inline int pfn_valid(unsigned long pfn)
>                       ^~~~~~~~~
>    In file included from include/linux/mmzone.h:912:0,
>                     from include/linux/gfp.h:6,
>                     from include/linux/mm.h:10,
>                     from include/linux/mman.h:5,
>                     from arch/powerpc/kernel/asm-offsets.c:22:
>    arch/powerpc/include/asm/mmzone.h:40:19: note: previous definition of 'pfn_valid' was here
>     static inline int pfn_valid(int pfn)
>                       ^~~~~~~~~
>    make[2]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1
>    make[2]: Target '__build' not remade because of errors.
>    make[1]: *** [prepare0] Error 2
>    make[1]: Target 'prepare' not remade because of errors.
>    make: *** [sub-make] Error 2

Oops, I'll fix this in the next version (and compile-test on ppc64...).

Weirdly enough, x86-32 and parisc define pfn_valid with an int
parameter, too (both of them since the Beginning Of Time, aka.
v2.6.12-rc2).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] DISCONTIGMEM support for PPC32

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Wed, Feb 21, 2018 at 04:02:25PM +0100, Christophe LEROY wrote:
[...]
> > > My question might me stupid, as I don't know PCC64 in deep, but when looking
> > > at page_is_ram() in arch/powerpc/mm/mem.c, I have the feeling the PPC64
> > > implements ram by blocks. Isn't it what you are trying to achieve ? Wouldn't
> > > it be feasible to map to what's done in PPC64 for PPC32 ?
> > 
> > Using page_is_ram in __ioremap_caller and the same memblock-based
> > approach that's used on PPC64 on PPC32 *should* work, but I think due to
> > the following line in initmem_init, it won't:
> > 
> > 	memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0);
> 
> Can't we just fix that ?

I'll give it a try.


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] powerpc: Implement DISCONTIGMEM and allow selection on PPC32

[christophe leroy]

Le 21/02/2018 à 17:08, Jonathan Neuschäfer a écrit :
> On Wed, Feb 21, 2018 at 07:46:28AM +0800, kbuild test robot wrote:
> [...]
>>>> include/linux/mmzone.h:1239:19: error: conflicting types for 'pfn_valid'
>>      static inline int pfn_valid(unsigned long pfn)
>>                        ^~~~~~~~~
>>     In file included from include/linux/mmzone.h:912:0,
>>                      from include/linux/gfp.h:6,
>>                      from include/linux/mm.h:10,
>>                      from include/linux/mman.h:5,
>>                      from arch/powerpc/kernel/asm-offsets.c:22:
>>     arch/powerpc/include/asm/mmzone.h:40:19: note: previous definition of 'pfn_valid' was here
>>      static inline int pfn_valid(int pfn)
>>                        ^~~~~~~~~
>>     make[2]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1
>>     make[2]: Target '__build' not remade because of errors.
>>     make[1]: *** [prepare0] Error 2
>>     make[1]: Target 'prepare' not remade because of errors.
>>     make: *** [sub-make] Error 2
> 
> Oops, I'll fix this in the next version (and compile-test on ppc64...).
> 
> Weirdly enough, x86-32 and parisc define pfn_valid with an int
> parameter, too (both of them since the Beginning Of Time, aka.
> v2.6.12-rc2).
> 

Behind the fact that the pfn type is different, my understanding is that 
you have to define CONFIG_HAVE_ARCH_PFN_VALID in the Kconfig in order to 
avoid it being included in include/linux/mmzone.h

Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH 0/6] DISCONTIGMEM support for PPC32

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Wed, Feb 21, 2018 at 04:02:25PM +0100, Christophe LEROY wrote:
[...]
> > > My question might me stupid, as I don't know PCC64 in deep, but when looking
> > > at page_is_ram() in arch/powerpc/mm/mem.c, I have the feeling the PPC64
> > > implements ram by blocks. Isn't it what you are trying to achieve ? Wouldn't
> > > it be feasible to map to what's done in PPC64 for PPC32 ?
> > 
> > Using page_is_ram in __ioremap_caller and the same memblock-based
> > approach that's used on PPC64 on PPC32 *should* work, but I think due to
> > the following line in initmem_init, it won't:
> > 
> > 	memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0);
> 
> Can't we just fix that ?

Turns out I was completely wrong about this. memblock_set_node as called
above only assigns all memory to node 0 and merges *adjacent* memblocks.
It doesn't merge the memblocks on the Wii, which are far apart.

So now I actually have a working patchset (coming soon), that's a good
deal shorter than this patchset, and hopefully won't break
CONFIG_HIGHMEM in the same way.

Thanks for your input! :)


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]