2018-02-20 16:18:36

by J. Neuschäfer

[permalink] [raw]
Subject: [PATCH 0/6] DISCONTIGMEM support for PPC32

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



2018-02-20 16:18:18

by J. Neuschäfer

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

Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
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


2018-02-20 16:18:41

by J. Neuschäfer

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

Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
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


2018-02-20 16:19:34

by J. Neuschäfer

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

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 <[email protected]>
---
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


2018-02-20 16:20:01

by J. Neuschäfer

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

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 <[email protected]>
---
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


2018-02-20 16:20:22

by J. Neuschäfer

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

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 <[email protected]>
---
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


2018-02-20 16:22:55

by J. Neuschäfer

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

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 <[email protected]>
---

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


2018-02-20 17:46:48

by Christophe Leroy

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



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 <[email protected]>
> ---
> 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


2018-02-20 23:48:20

by kernel test robot

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

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 <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>

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


Attachments:
(No filename) (2.96 kB)
.config.gz (23.46 kB)
Download all attachments

2018-02-21 07:07:18

by Christophe Leroy

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



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

2018-02-21 15:07:02

by Christophe Leroy

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



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
>

2018-02-21 18:48:03

by J. Neuschäfer

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

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


Attachments:
(No filename) (1.56 kB)
signature.asc (836.00 B)
Download all attachments

2018-02-21 18:54:07

by J. Neuschäfer

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

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



Attachments:
(No filename) (613.00 B)
signature.asc (836.00 B)
Download all attachments

2018-02-21 18:54:15

by J. Neuschäfer

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

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


Attachments:
(No filename) (1.39 kB)
signature.asc (836.00 B)
Download all attachments

2018-02-21 18:59:26

by J. Neuschäfer

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

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


Attachments:
(No filename) (1.15 kB)
signature.asc (836.00 B)
Download all attachments

2018-02-21 19:06:04

by J. Neuschäfer

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

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


Attachments:
(No filename) (762.00 B)
signature.asc (836.00 B)
Download all attachments

2018-02-21 19:12:36

by Christophe Leroy

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



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


2018-02-21 23:33:15

by J. Neuschäfer

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

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


Attachments:
(No filename) (1.11 kB)
signature.asc (836.00 B)
Download all attachments