[PATCH 2/3] powerpc/64s/radix: ioremap use ioremap_page_range

[Nicholas Piggin]

Christophe Leroy's on June 11, 2019 4:46 pm:
> 
> 
> Le 10/06/2019 à 05:08, Nicholas Piggin a écrit :
>> Radix can use ioremap_page_range for ioremap, after slab is available.
>> This makes it possible to enable huge ioremap mapping support.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>> ---
>>   arch/powerpc/include/asm/book3s/64/radix.h |  3 +++
>>   arch/powerpc/mm/book3s64/pgtable.c         | 21 +++++++++++++++++++++
>>   arch/powerpc/mm/book3s64/radix_pgtable.c   | 21 +++++++++++++++++++++
>>   arch/powerpc/mm/pgtable_64.c               |  2 +-
>>   4 files changed, 46 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
>> index 574eca33f893..e04a839cb5b9 100644
>> --- a/arch/powerpc/include/asm/book3s/64/radix.h
>> +++ b/arch/powerpc/include/asm/book3s/64/radix.h
>> @@ -266,6 +266,9 @@ extern void radix__vmemmap_remove_mapping(unsigned long start,
>>   extern int radix__map_kernel_page(unsigned long ea, unsigned long pa,
>>   				 pgprot_t flags, unsigned int psz);
>>   
>> +extern int radix__ioremap_range(unsigned long ea, phys_addr_t pa,
>> +				unsigned long size, pgprot_t prot, int nid);
>> +
> 
> 'extern' is pointless here, and checkpatch will cry.
> 
>>   static inline unsigned long radix__get_tree_size(void)
>>   {
>>   	unsigned long rts_field;
>> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
>> index ff98b663c83e..953850a602f7 100644
>> --- a/arch/powerpc/mm/book3s64/pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/pgtable.c
>> @@ -450,3 +450,24 @@ int pmd_move_must_withdraw(struct spinlock *new_pmd_ptl,
>>   
>>   	return true;
>>   }
>> +
>> +int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
>> +{
>> +	unsigned long i;
>> +
>> +	if (radix_enabled())
>> +		return radix__ioremap_range(ea, pa, size, prot, nid);
> 
> This function looks pretty similar to the one in the previous patch.
> Since radix_enabled() is available and return false for all other 
> subarches, I think the above could go in the generic ioremap_range(), 
> you'll only need to move the function declaration in a common file, for 
> instance asm/io.h
> 
>> +
>> +	for (i = 0; i < size; i += PAGE_SIZE) {
>> +		int err = map_kernel_page(ea + i, pa + i, prot);
>> +		if (err) {
>> +			if (slab_is_available())
>> +				unmap_kernel_range(ea, size);
>> +			else
>> +				WARN_ON_ONCE(1); /* Should clean up */
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> index c9bcf428dd2b..db993bc1aef3 100644
>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -11,6 +11,7 @@
>>   
>>   #define pr_fmt(fmt) "radix-mmu: " fmt
>>   
>> +#include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/sched/mm.h>
>>   #include <linux/memblock.h>
>> @@ -1122,3 +1123,23 @@ void radix__ptep_modify_prot_commit(struct vm_area_struct *vma,
>>   
>>   	set_pte_at(mm, addr, ptep, pte);
>>   }
>> +
>> +int radix__ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size,
>> +			pgprot_t prot, int nid)
>> +{
>> +	if (likely(slab_is_available())) {
>> +		int err = ioremap_page_range(ea, ea + size, pa, prot);
>> +		if (err)
>> +			unmap_kernel_range(ea, size);
>> +		return err;
>> +	} else {
>> +		unsigned long i;
>> +
>> +		for (i = 0; i < size; i += PAGE_SIZE) {
>> +			int err = map_kernel_page(ea + i, pa + i, prot);
>> +			if (WARN_ON_ONCE(err)) /* Should clean up */
>> +				return err;
>> +		}
> 
> Same loop again.
> 
> What about not doing a radix specific function and just putting 
> something like below in the core ioremap_range() function ?
> 
> 	if (likely(slab_is_available()) && radix_enabled()) {
> 		int err = ioremap_page_range(ea, ea + size, pa, prot);
> 
> 		if (err)
> 			unmap_kernel_range(ea, size);
> 		return err;
> 	}
> 
> Because I'm pretty sure will more and more use ioremap_page_range().

Well I agree the duplication is not so nice, but it's convenient
to see what is going on for each MMU type.

There is a significant amount of churn that needs to be done in
this layer so I prefer to make it a bit simpler despite duplication.

I would like to remove the early ioremap or make it into its own
function. Re-implement map_kernel_page with ioremap_page_range,
allow page tables that don't use slab to avoid the early check,
unbolt the hptes mapped in early boot, etc.

I just wanted to escape out the 64s and hash/radix implementations
completely until that settles.

>> -static int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
>> +int __weak ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
> 
> Hum. Weak functions remain in unused in vmlinux unless 
> CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is selected.
> 
> Also, they are some how dangerous because people might change them 
> without seeing that it is overridden for some particular configuration.

Well you shouldn't assume that when you see a weak function, but
what's the preferred alternative? A config option?

Thanks,
Nick