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

[Jonathan Neuschäfer]

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




signature.asc
Description: PGP signature

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

[Jonathan Neuschäfer]

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


signature.asc
Description: PGP signature

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

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