[PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[Christophe Leroy]

__kernel_map_pages() is almost identical for PPC32 and RADIX.

Refactor it.

On PPC32 it is not needed for KFENCE, but to keep it simple
just make it similar to PPC64.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/book3s/64/pgtable.h | 10 ----------
 arch/powerpc/include/asm/book3s/64/radix.h   |  2 --
 arch/powerpc/mm/book3s64/radix_pgtable.c     | 14 --------------
 arch/powerpc/mm/pageattr.c                   | 19 +++++++++++++++++++
 arch/powerpc/mm/pgtable_32.c                 | 15 ---------------
 5 files changed, 19 insertions(+), 41 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 927d585652bc..62c43d3d80ec 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -1027,16 +1027,6 @@ static inline void vmemmap_remove_mapping(unsigned long start,
 }
 #endif
 
-#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
-static inline void __kernel_map_pages(struct page *page, int numpages, int enable)
-{
-	if (radix_enabled())
-		radix__kernel_map_pages(page, numpages, enable);
-	else
-		hash__kernel_map_pages(page, numpages, enable);
-}
-#endif
-
 static inline pte_t pmd_pte(pmd_t pmd)
 {
 	return __pte_raw(pmd_raw(pmd));
diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
index 357e23a403d3..8f55ff74bb68 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h
@@ -362,8 +362,6 @@ int radix__create_section_mapping(unsigned long start, unsigned long end,
 int radix__remove_section_mapping(unsigned long start, unsigned long end);
 #endif /* CONFIG_MEMORY_HOTPLUG */
 
-void radix__kernel_map_pages(struct page *page, int numpages, int enable);
-
 #ifdef CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP
 #define vmemmap_can_optimize vmemmap_can_optimize
 bool vmemmap_can_optimize(struct vmem_altmap *altmap, struct dev_pagemap *pgmap);
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index c6a4ac766b2b..e16e2fd104c5 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -1339,20 +1339,6 @@ void __ref radix__vmemmap_free(unsigned long start, unsigned long end,
 #endif
 #endif
 
-#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
-void radix__kernel_map_pages(struct page *page, int numpages, int enable)
-{
-	unsigned long addr;
-
-	addr = (unsigned long)page_address(page);
-
-	if (enable)
-		set_memory_p(addr, numpages);
-	else
-		set_memory_np(addr, numpages);
-}
-#endif
-
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 
 unsigned long radix__pmd_hugepage_update(struct mm_struct *mm, unsigned long addr,
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 421db7c4f2a4..16b8d20d6ca8 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
 	return apply_to_existing_page_range(&init_mm, start, size,
 					    change_page_attr, (void *)action);
 }
+
+#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
+#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
+void __kernel_map_pages(struct page *page, int numpages, int enable)
+{
+	unsigned long addr = (unsigned long)page_address(page);
+
+	if (PageHighMem(page))
+		return;
+
+	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
+		hash__kernel_map_pages(page, numpages, enable);
+	else if (enable)
+		set_memory_p(addr, numpages);
+	else
+		set_memory_np(addr, numpages);
+}
+#endif
+#endif
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index e94919853ca3..4be97b4a44f9 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -188,18 +188,3 @@ void mark_rodata_ro(void)
 	ptdump_check_wx();
 }
 #endif
-
-#if defined(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) && defined(CONFIG_DEBUG_PAGEALLOC)
-void __kernel_map_pages(struct page *page, int numpages, int enable)
-{
-	unsigned long addr = (unsigned long)page_address(page);
-
-	if (PageHighMem(page))
-		return;
-
-	if (enable)
-		set_memory_p(addr, numpages);
-	else
-		set_memory_np(addr, numpages);
-}
-#endif /* CONFIG_DEBUG_PAGEALLOC */
-- 
2.43.0

[PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map_pages()

[Christophe Leroy]

set_memory_p() and set_memory_np() can fail.

As mentioned in linux/mm.h:

/*
 * To support DEBUG_PAGEALLOC architecture must ensure that
 * __kernel_map_pages() never fails
 */

So panic in case set_memory_p() or set_memory_np() fail
in __kernel_map_pages().

Link: https://github.com/KSPP/linux/issues/7
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
 arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
 arch/powerpc/mm/pageattr.c                | 10 +++++++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
index 6e70ae511631..8f47ae79f2a6 100644
--- a/arch/powerpc/include/asm/book3s/64/hash.h
+++ b/arch/powerpc/include/asm/book3s/64/hash.h
@@ -269,7 +269,7 @@ int hash__create_section_mapping(unsigned long start, unsigned long end,
 				 int nid, pgprot_t prot);
 int hash__remove_section_mapping(unsigned long start, unsigned long end);
 
-void hash__kernel_map_pages(struct page *page, int numpages, int enable);
+int hash__kernel_map_pages(struct page *page, int numpages, int enable);
 
 #endif /* !__ASSEMBLY__ */
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 0626a25b0d72..01c3b4b65241 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -2172,7 +2172,7 @@ static void kernel_unmap_linear_page(unsigned long vaddr, unsigned long lmi)
 				     mmu_kernel_ssize, 0);
 }
 
-void hash__kernel_map_pages(struct page *page, int numpages, int enable)
+int hash__kernel_map_pages(struct page *page, int numpages, int enable)
 {
 	unsigned long flags, vaddr, lmi;
 	int i;
@@ -2189,6 +2189,7 @@ void hash__kernel_map_pages(struct page *page, int numpages, int enable)
 			kernel_unmap_linear_page(vaddr, lmi);
 	}
 	local_irq_restore(flags);
+	return 0;
 }
 #endif /* CONFIG_DEBUG_PAGEALLOC || CONFIG_KFENCE */
 
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 16b8d20d6ca8..62b678585878 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
 #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
+	int err;
 	unsigned long addr = (unsigned long)page_address(page);
 
 	if (PageHighMem(page))
 		return;
 
 	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
-		hash__kernel_map_pages(page, numpages, enable);
+		err = hash__kernel_map_pages(page, numpages, enable);
 	else if (enable)
-		set_memory_p(addr, numpages);
+		err = set_memory_p(addr, numpages);
 	else
-		set_memory_np(addr, numpages);
+		err = set_memory_np(addr, numpages);
+
+	if (err)
+		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
 }
 #endif
 #endif
-- 
2.43.0

Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map_pages()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> set_memory_p() and set_memory_np() can fail.
>
> As mentioned in linux/mm.h:
>
> /*
>  * To support DEBUG_PAGEALLOC architecture must ensure that
>  * __kernel_map_pages() never fails
>  */
>
> So panic in case set_memory_p() or set_memory_np() fail
> in __kernel_map_pages().
>
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
>  arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
>  arch/powerpc/mm/pageattr.c                | 10 +++++++---
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
...
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 16b8d20d6ca8..62b678585878 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>  #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>  void __kernel_map_pages(struct page *page, int numpages, int enable)
>  {
> +	int err;
>  	unsigned long addr = (unsigned long)page_address(page);
>  
>  	if (PageHighMem(page))
>  		return;
>  
>  	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
> -		hash__kernel_map_pages(page, numpages, enable);
> +		err = hash__kernel_map_pages(page, numpages, enable);
>  	else if (enable)
> -		set_memory_p(addr, numpages);
> +		err = set_memory_p(addr, numpages);
>  	else
> -		set_memory_np(addr, numpages);
> +		err = set_memory_np(addr, numpages);
> +
> +	if (err)
> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");

This doesn't compile, it's missing __func__ I guess.

Seems like we could keep it simpler though, it should hopefully never
happen anyway, eg:

  panic("%s: changing memory protections failed\n", __func__);


cheers

Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map_pages()

[Christophe Leroy]

Le 21/02/2024 à 13:09, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> set_memory_p() and set_memory_np() can fail.
>>
>> As mentioned in linux/mm.h:
>>
>> /*
>>   * To support DEBUG_PAGEALLOC architecture must ensure that
>>   * __kernel_map_pages() never fails
>>   */
>>
>> So panic in case set_memory_p() or set_memory_np() fail
>> in __kernel_map_pages().
>>
>> Link: https://github.com/KSPP/linux/issues/7
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
>>   arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
>>   arch/powerpc/mm/pageattr.c                | 10 +++++++---
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
> ...
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index 16b8d20d6ca8..62b678585878 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>   #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>   void __kernel_map_pages(struct page *page, int numpages, int enable)
>>   {
>> +	int err;
>>   	unsigned long addr = (unsigned long)page_address(page);
>>   
>>   	if (PageHighMem(page))
>>   		return;
>>   
>>   	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>> -		hash__kernel_map_pages(page, numpages, enable);
>> +		err = hash__kernel_map_pages(page, numpages, enable);
>>   	else if (enable)
>> -		set_memory_p(addr, numpages);
>> +		err = set_memory_p(addr, numpages);
>>   	else
>> -		set_memory_np(addr, numpages);
>> +		err = set_memory_np(addr, numpages);
>> +
>> +	if (err)
>> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
> 
> This doesn't compile, it's missing __func__ I guess.

Damn, I was too quick when I took into account checkpatch's feedback, 
sorry for that.

> 
> Seems like we could keep it simpler though, it should hopefully never
> happen anyway, eg:
> 
>    panic("%s: changing memory protections failed\n", __func__);

Sure, let's do that. Do you want a v2 or you do the change directly ?

Thanks
Christophe

Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map_pages()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 21/02/2024 à 13:09, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> set_memory_p() and set_memory_np() can fail.
>>>
>>> As mentioned in linux/mm.h:
>>>
>>> /*
>>>   * To support DEBUG_PAGEALLOC architecture must ensure that
>>>   * __kernel_map_pages() never fails
>>>   */
>>>
>>> So panic in case set_memory_p() or set_memory_np() fail
>>> in __kernel_map_pages().
>>>
>>> Link: https://github.com/KSPP/linux/issues/7
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>   arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
>>>   arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
>>>   arch/powerpc/mm/pageattr.c                | 10 +++++++---
>>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>>
>> ...
>>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>>> index 16b8d20d6ca8..62b678585878 100644
>>> --- a/arch/powerpc/mm/pageattr.c
>>> +++ b/arch/powerpc/mm/pageattr.c
>>> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>>   #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>>   void __kernel_map_pages(struct page *page, int numpages, int enable)
>>>   {
>>> +	int err;
>>>   	unsigned long addr = (unsigned long)page_address(page);
>>>   
>>>   	if (PageHighMem(page))
>>>   		return;
>>>   
>>>   	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>>> -		hash__kernel_map_pages(page, numpages, enable);
>>> +		err = hash__kernel_map_pages(page, numpages, enable);
>>>   	else if (enable)
>>> -		set_memory_p(addr, numpages);
>>> +		err = set_memory_p(addr, numpages);
>>>   	else
>>> -		set_memory_np(addr, numpages);
>>> +		err = set_memory_np(addr, numpages);
>>> +
>>> +	if (err)
>>> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
>> 
>> This doesn't compile, it's missing __func__ I guess.
>
> Damn, I was too quick when I took into account checkpatch's feedback, 
> sorry for that.
>
>> 
>> Seems like we could keep it simpler though, it should hopefully never
>> happen anyway, eg:
>> 
>>    panic("%s: changing memory protections failed\n", __func__);
>
> Sure, let's do that. Do you want a v2 or you do the change directly ?

No need for a v2, I'll just fix it up when applying.

cheers

Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> __kernel_map_pages() is almost identical for PPC32 and RADIX.
>
> Refactor it.
>
> On PPC32 it is not needed for KFENCE, but to keep it simple
> just make it similar to PPC64.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h | 10 ----------
>  arch/powerpc/include/asm/book3s/64/radix.h   |  2 --
>  arch/powerpc/mm/book3s64/radix_pgtable.c     | 14 --------------
>  arch/powerpc/mm/pageattr.c                   | 19 +++++++++++++++++++
>  arch/powerpc/mm/pgtable_32.c                 | 15 ---------------
>  5 files changed, 19 insertions(+), 41 deletions(-)
>
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 421db7c4f2a4..16b8d20d6ca8 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>  	return apply_to_existing_page_range(&init_mm, start, size,
>  					    change_page_attr, (void *)action);
>  }
> +
> +#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
> +#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
> +void __kernel_map_pages(struct page *page, int numpages, int enable)
> +{
> +	unsigned long addr = (unsigned long)page_address(page);
> +
> +	if (PageHighMem(page))
> +		return;
> +
> +	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
> +		hash__kernel_map_pages(page, numpages, enable);
> +	else if (enable)
> +		set_memory_p(addr, numpages);
> +	else
> +		set_memory_np(addr, numpages);
> +}

This doesn't build on 32-bit, eg. ppc32_allmodconfig:

../arch/powerpc/mm/pageattr.c: In function '__kernel_map_pages':
../arch/powerpc/mm/pageattr.c:116:23: error: implicit declaration of function 'hash__kernel_map_pages' [-Werror=implicit-function-declaration]
  116 |                 err = hash__kernel_map_pages(page, numpages, enable);
      |                       ^~~~~~~~~~~~~~~~~~~~~~

I couldn't see a nice way to get around it, so ended up with:

void __kernel_map_pages(struct page *page, int numpages, int enable)
{
	int err;
	unsigned long addr = (unsigned long)page_address(page);

	if (PageHighMem(page))
		return;

#ifdef CONFIG_PPC_BOOK3S_64
	if (!radix_enabled())
		err = hash__kernel_map_pages(page, numpages, enable);
	else
#endif
	if (enable)
		err = set_memory_p(addr, numpages);
	else
		err = set_memory_np(addr, numpages);



cheers

Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[Christophe Leroy]

Le 22/02/2024 à 06:32, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> __kernel_map_pages() is almost identical for PPC32 and RADIX.
>>
>> Refactor it.
>>
>> On PPC32 it is not needed for KFENCE, but to keep it simple
>> just make it similar to PPC64.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/book3s/64/pgtable.h | 10 ----------
>>   arch/powerpc/include/asm/book3s/64/radix.h   |  2 --
>>   arch/powerpc/mm/book3s64/radix_pgtable.c     | 14 --------------
>>   arch/powerpc/mm/pageattr.c                   | 19 +++++++++++++++++++
>>   arch/powerpc/mm/pgtable_32.c                 | 15 ---------------
>>   5 files changed, 19 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index 421db7c4f2a4..16b8d20d6ca8 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>   	return apply_to_existing_page_range(&init_mm, start, size,
>>   					    change_page_attr, (void *)action);
>>   }
>> +
>> +#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
>> +#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>> +void __kernel_map_pages(struct page *page, int numpages, int enable)
>> +{
>> +	unsigned long addr = (unsigned long)page_address(page);
>> +
>> +	if (PageHighMem(page))
>> +		return;
>> +
>> +	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>> +		hash__kernel_map_pages(page, numpages, enable);
>> +	else if (enable)
>> +		set_memory_p(addr, numpages);
>> +	else
>> +		set_memory_np(addr, numpages);
>> +}
> 
> This doesn't build on 32-bit, eg. ppc32_allmodconfig:
> 
> ../arch/powerpc/mm/pageattr.c: In function '__kernel_map_pages':
> ../arch/powerpc/mm/pageattr.c:116:23: error: implicit declaration of function 'hash__kernel_map_pages' [-Werror=implicit-function-declaration]
>    116 |                 err = hash__kernel_map_pages(page, numpages, enable);
>        |                       ^~~~~~~~~~~~~~~~~~~~~~
> 
> I couldn't see a nice way to get around it, so ended up with:
> 
> void __kernel_map_pages(struct page *page, int numpages, int enable)
> {
> 	int err;
> 	unsigned long addr = (unsigned long)page_address(page);
> 
> 	if (PageHighMem(page))
> 		return;
> 
> #ifdef CONFIG_PPC_BOOK3S_64
> 	if (!radix_enabled())
> 		err = hash__kernel_map_pages(page, numpages, enable);
> 	else
> #endif
> 	if (enable)
> 		err = set_memory_p(addr, numpages);
> 	else
> 		err = set_memory_np(addr, numpages);
> 


I missed something it seems. Not good to leave something unterminated 
when you leave for vacation and think it was finished when you come back.

The best solution I see is to move hash__kernel_map_pages() prototype 
somewhere else.

$ git grep -e hash__ -e radix__ -- arch/powerpc/include/asm/*.h
arch/powerpc/include/asm/bug.h:void hash__do_page_fault(struct pt_regs *);
arch/powerpc/include/asm/mmu.h:extern void radix__mmu_cleanup_all(void);
arch/powerpc/include/asm/mmu_context.h:extern void 
radix__switch_mmu_context(struct mm_struct *prev,
arch/powerpc/include/asm/mmu_context.h:         return 
radix__switch_mmu_context(prev, next);
arch/powerpc/include/asm/mmu_context.h:extern int 
hash__alloc_context_id(void);
arch/powerpc/include/asm/mmu_context.h:void __init 
hash__reserve_context_id(int id);
arch/powerpc/include/asm/mmu_context.h: context_id = 
hash__alloc_context_id();
arch/powerpc/include/asm/mmu_context.h:  * radix__flush_all_mm() to 
determine the scope (local/global)
arch/powerpc/include/asm/mmu_context.h:         radix__flush_all_mm(mm);


Maybe asm/mmu.h ?

Or mm/mmu_decl.h ?

Christophe

Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 22/02/2024 à 06:32, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> __kernel_map_pages() is almost identical for PPC32 and RADIX.
>>>
>>> Refactor it.
>>>
>>> On PPC32 it is not needed for KFENCE, but to keep it simple
>>> just make it similar to PPC64.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>   arch/powerpc/include/asm/book3s/64/pgtable.h | 10 ----------
>>>   arch/powerpc/include/asm/book3s/64/radix.h   |  2 --
>>>   arch/powerpc/mm/book3s64/radix_pgtable.c     | 14 --------------
>>>   arch/powerpc/mm/pageattr.c                   | 19 +++++++++++++++++++
>>>   arch/powerpc/mm/pgtable_32.c                 | 15 ---------------
>>>   5 files changed, 19 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>>> index 421db7c4f2a4..16b8d20d6ca8 100644
>>> --- a/arch/powerpc/mm/pageattr.c
>>> +++ b/arch/powerpc/mm/pageattr.c
>>> @@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>>   	return apply_to_existing_page_range(&init_mm, start, size,
>>>   					    change_page_attr, (void *)action);
>>>   }
>>> +
>>> +#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
>>> +#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>> +void __kernel_map_pages(struct page *page, int numpages, int enable)
>>> +{
>>> +	unsigned long addr = (unsigned long)page_address(page);
>>> +
>>> +	if (PageHighMem(page))
>>> +		return;
>>> +
>>> +	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>>> +		hash__kernel_map_pages(page, numpages, enable);
>>> +	else if (enable)
>>> +		set_memory_p(addr, numpages);
>>> +	else
>>> +		set_memory_np(addr, numpages);
>>> +}
>> 
>> This doesn't build on 32-bit, eg. ppc32_allmodconfig:
>> 
>> ../arch/powerpc/mm/pageattr.c: In function '__kernel_map_pages':
>> ../arch/powerpc/mm/pageattr.c:116:23: error: implicit declaration of function 'hash__kernel_map_pages' [-Werror=implicit-function-declaration]
>>    116 |                 err = hash__kernel_map_pages(page, numpages, enable);
>>        |                       ^~~~~~~~~~~~~~~~~~~~~~
>> 
>> I couldn't see a nice way to get around it, so ended up with:
>> 
>> void __kernel_map_pages(struct page *page, int numpages, int enable)
>> {
>> 	int err;
>> 	unsigned long addr = (unsigned long)page_address(page);
>> 
>> 	if (PageHighMem(page))
>> 		return;
>> 
>> #ifdef CONFIG_PPC_BOOK3S_64
>> 	if (!radix_enabled())
>> 		err = hash__kernel_map_pages(page, numpages, enable);
>> 	else
>> #endif
>> 	if (enable)
>> 		err = set_memory_p(addr, numpages);
>> 	else
>> 		err = set_memory_np(addr, numpages);
>> 
>
>
> I missed something it seems. Not good to leave something unterminated 
> when you leave for vacation and think it was finished when you come back.
>
> The best solution I see is to move hash__kernel_map_pages() prototype 
> somewhere else.

> $ git grep -e hash__ -e radix__ -- arch/powerpc/include/asm/*.h
> arch/powerpc/include/asm/bug.h:void hash__do_page_fault(struct pt_regs *);
> arch/powerpc/include/asm/mmu.h:extern void radix__mmu_cleanup_all(void);
> arch/powerpc/include/asm/mmu_context.h:extern void radix__switch_mmu_context(struct mm_struct *prev,
> arch/powerpc/include/asm/mmu_context.h:         return radix__switch_mmu_context(prev, next);
> arch/powerpc/include/asm/mmu_context.h:extern int hash__alloc_context_id(void);
> arch/powerpc/include/asm/mmu_context.h:void __init hash__reserve_context_id(int id);
> arch/powerpc/include/asm/mmu_context.h: context_id = hash__alloc_context_id();
> arch/powerpc/include/asm/mmu_context.h:  * radix__flush_all_mm() to determine the scope (local/global)
> arch/powerpc/include/asm/mmu_context.h:         radix__flush_all_mm(mm);

If anything I'd prefer to move those out of there into the book3s/64/
headers :)

> Maybe asm/mmu.h ?
>
> Or mm/mmu_decl.h ?

Yeah I'll do that. It's a bit of a dumping ground, but at least it's
internal to arch code, not exported to the rest of the kernel.

cheers

Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()

[Michael Ellerman]

On Fri, 16 Feb 2024 11:17:33 +0100, Christophe Leroy wrote:
> __kernel_map_pages() is almost identical for PPC32 and RADIX.
> 
> Refactor it.
> 
> On PPC32 it is not needed for KFENCE, but to keep it simple
> just make it similar to PPC64.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc: Refactor __kernel_map_pages()
      https://git.kernel.org/powerpc/c/3c8016e681c5e0f5f3ad15edb4569727cd32eaff
[2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map_pages()
      https://git.kernel.org/powerpc/c/9cbacb834b4afcb55eb8ac5115fa82fc7ede5c83

cheers