[RFC PATCH 0/4] assorted virtual / physical address fixes

[RFC PATCH 0/4] assorted virtual / physical address fixes

[Nicholas Piggin]

Implementing VIRTUAL_BUG_ON to catch incorrect usage of __va and __pa
showed up a few possible issues. Actually patch 1 was found by
inspection (I will check whether I may attribute the reporter).

Thanks,
Nick

Nicholas Piggin (4):
  powerpc/64s/radix: Fix memory hotplug section page table creation
  powerpc/64s/radix: Fix memory hot-unplug page table split
  powerpc/perf: fix imc allocation failure
  powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses

 arch/powerpc/include/asm/page.h          | 14 ++++++++++--
 arch/powerpc/mm/book3s64/radix_pgtable.c |  6 ++---
 arch/powerpc/perf/imc-pmu.c              | 29 +++++++++++++++---------
 3 files changed, 33 insertions(+), 16 deletions(-)

-- 
2.20.1

[RFC PATCH 1/4] powerpc/64s/radix: Fix memory hotplug section page table creation

[Nicholas Piggin]

create_physical_mapping expects physical addresses, but creating and
splitting these mappings after boot is supplying virtual (effective)
addresses. This can be hit by booting with limited memory then probing
new physical memory sections.

Cc: Reza Arbab <arbab@linux.vnet.ibm.com>
Fixes: 6cc27341b21a8 ("powerpc/mm: add radix__create_section_mapping()")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index b4ca9e95e678..c5cc16ab1954 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -902,7 +902,7 @@ int __meminit radix__create_section_mapping(unsigned long start, unsigned long e
 		return -1;
 	}
 
-	return create_physical_mapping(start, end, nid);
+	return create_physical_mapping(__pa(start), __pa(end), nid);
 }
 
 int __meminit radix__remove_section_mapping(unsigned long start, unsigned long end)
-- 
2.20.1

Re: [RFC PATCH 1/4] powerpc/64s/radix: Fix memory hotplug section page table creation

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> create_physical_mapping expects physical addresses, but creating and
> splitting these mappings after boot is supplying virtual (effective)
> addresses. This can be hit by booting with limited memory then probing
> new physical memory sections.
>
> Cc: Reza Arbab <arbab@linux.vnet.ibm.com>
> Fixes: 6cc27341b21a8 ("powerpc/mm: add radix__create_section_mapping()")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

This is not catastrophic because create_physical_mapping() just uses
start/end to construct virtual addresses anyway, and __va(__va(x)) == __va(x) ?

Although we do pass those through as region_start/end which then go to
memblock_alloc_try_nid(). But I guess that doesn't happen after boot,
which is the case you're talking about.

So I think looks good, change log could use a bit more detail though :)

cheers

> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index b4ca9e95e678..c5cc16ab1954 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -902,7 +902,7 @@ int __meminit radix__create_section_mapping(unsigned long start, unsigned long e
>  		return -1;
>  	}
>  
> -	return create_physical_mapping(start, end, nid);
> +	return create_physical_mapping(__pa(start), __pa(end), nid);
>  }
>  
>  int __meminit radix__remove_section_mapping(unsigned long start, unsigned long end)
> -- 
> 2.20.1

Re: [RFC PATCH 1/4] powerpc/64s/radix: Fix memory hotplug section page table creation

[Nicholas Piggin]

Michael Ellerman's on July 23, 2019 8:52 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> create_physical_mapping expects physical addresses, but creating and
>> splitting these mappings after boot is supplying virtual (effective)
>> addresses. This can be hit by booting with limited memory then probing
>> new physical memory sections.
>>
>> Cc: Reza Arbab <arbab@linux.vnet.ibm.com>
>> Fixes: 6cc27341b21a8 ("powerpc/mm: add radix__create_section_mapping()")
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> This is not catastrophic because create_physical_mapping() just uses
> start/end to construct virtual addresses anyway, and __va(__va(x)) == __va(x) ?

A bit more subtle, it calls __map_kernel_page with the pa as well.
pfn_pte ends up masking the top 0xc bits out with PTE_RPN_MASK,
which is what saves us.

Hmm, so we really should also have a VM_BUG_ON in pfn_pte if it's
given a pfn with the top PAGE_SHIFT bit or PTE_RPN_MASK bits set.
I'll add that as a patch 5.
 
> Although we do pass those through as region_start/end which then go to
> memblock_alloc_try_nid(). But I guess that doesn't happen after boot,
> which is the case you're talking about.
> 
> So I think looks good, change log could use a bit more detail though :)

Thanks for taking a look. I'll resend after a bit more testing and
some changelog improvement.

Thanks,
Nick

[RFC PATCH 2/4] powerpc/64s/radix: Fix memory hot-unplug page table split

[Nicholas Piggin]

create_physical_mapping expects physical addresses, but splitting
these mapping on hot unplug is supplying virtual (effective)
addresses.

[I'm not sure how to test this one]

Cc: Balbir Singh <bsingharora@gmail.com>
Fixes: 4dd5f8a99e791 ("powerpc/mm/radix: Split linear mapping on hot-unplug")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index c5cc16ab1954..2204d8eeb784 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -737,8 +737,8 @@ static int __meminit stop_machine_change_mapping(void *data)
 
 	spin_unlock(&init_mm.page_table_lock);
 	pte_clear(&init_mm, params->aligned_start, params->pte);
-	create_physical_mapping(params->aligned_start, params->start, -1);
-	create_physical_mapping(params->end, params->aligned_end, -1);
+	create_physical_mapping(__pa(params->aligned_start), __pa(params->start), -1);
+	create_physical_mapping(__pa(params->end), __pa(params->aligned_end), -1);
 	spin_lock(&init_mm.page_table_lock);
 	return 0;
 }
-- 
2.20.1

[RFC PATCH 3/4] powerpc/perf: fix imc allocation failure

[Nicholas Piggin]

alloc_pages_node return value should be tested before applying
page_address.

Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/perf/imc-pmu.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index dea243185ea4..cb50a9e1fd2d 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -577,6 +577,7 @@ static int core_imc_mem_init(int cpu, int size)
 {
 	int nid, rc = 0, core_id = (cpu / threads_per_core);
 	struct imc_mem_info *mem_info;
+	struct page *page;
 
 	/*
 	 * alloc_pages_node() will allocate memory for core in the
@@ -587,11 +588,12 @@ static int core_imc_mem_init(int cpu, int size)
 	mem_info->id = core_id;
 
 	/* We need only vbase for core counters */
-	mem_info->vbase = page_address(alloc_pages_node(nid,
-					  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
-					  __GFP_NOWARN, get_order(size)));
-	if (!mem_info->vbase)
+	page = alloc_pages_node(nid,
+				GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
+				__GFP_NOWARN, get_order(size));
+	if (!page)
 		return -ENOMEM;
+	mem_info->vbase = page_address(page);
 
 	/* Init the mutex */
 	core_imc_refc[core_id].id = core_id;
@@ -849,15 +851,17 @@ static int thread_imc_mem_alloc(int cpu_id, int size)
 	int nid = cpu_to_node(cpu_id);
 
 	if (!local_mem) {
+		struct page *page;
 		/*
 		 * This case could happen only once at start, since we dont
 		 * free the memory in cpu offline path.
 		 */
-		local_mem = page_address(alloc_pages_node(nid,
+		page = alloc_pages_node(nid,
 				  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
-				  __GFP_NOWARN, get_order(size)));
-		if (!local_mem)
+				  __GFP_NOWARN, get_order(size));
+		if (!page)
 			return -ENOMEM;
+		local_mem = page_address(page);
 
 		per_cpu(thread_imc_mem, cpu_id) = local_mem;
 	}
@@ -1095,11 +1099,14 @@ static int trace_imc_mem_alloc(int cpu_id, int size)
 	int core_id = (cpu_id / threads_per_core);
 
 	if (!local_mem) {
-		local_mem = page_address(alloc_pages_node(phys_id,
-					GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
-					__GFP_NOWARN, get_order(size)));
-		if (!local_mem)
+		struct page *page;
+
+		page = alloc_pages_node(phys_id,
+				GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
+				__GFP_NOWARN, get_order(size));
+		if (!page)
 			return -ENOMEM;
+		local_mem = page_address(page);
 		per_cpu(trace_imc_mem, cpu_id) = local_mem;
 
 		/* Initialise the counters for trace mode */
-- 
2.20.1

Re: [RFC PATCH 3/4] powerpc/perf: fix imc allocation failure

[Anju T Sudhakar]

On 7/22/19 11:16 PM, Nicholas Piggin wrote:
> alloc_pages_node return value should be tested before applying
> page_address.
>
> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Tested-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>

[RFC PATCH 4/4] powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses

[Nicholas Piggin]

Ensure __va is given an address below PAGE_OFFSET, and __pa is given
one above PAGE_OFFSET.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/page.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 0d52f57fca04..c8bb14ff4713 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn)
 /*
  * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET
  * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit.
+ * This also results in better code generation.
  */
-#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET))
-#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL)
+#define __va(x)								\
+({									\
+	VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET);		\
+	(void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET);	\
+})
+
+#define __pa(x)								\
+({									\
+	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
+	(unsigned long)(x) & 0x0fffffffffffffffUL;			\
+})
 
 #else /* 32-bit, non book E */
 #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START))
-- 
2.20.1