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

[Nicholas Piggin]

Christophe Leroy's on June 19, 2019 10:49 pm:
> 
> 
> Le 19/06/2019 à 05:59, Nicholas Piggin a écrit :
>> Christophe Leroy's on June 11, 2019 4:46 pm:
>>>
>>>
>>> Le 10/06/2019 à 05:08, Nicholas Piggin a écrit :
> 
> [snip]
> 
>>>> 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.
> 
> I can understand the benefit in some situations but here I just can't. 
> And code duplication should be avoided as much as possible as it makes 
> code maintenance more difficult.
> 
> Here you have:
> 
> +static int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned 
> long size, pgprot_t prot, int nid)
> +{
> +	unsigned long i;
> +
> +	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;
> +}
> 
> You now create a new one in another file, that is almost identical:
> 
> +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);
> +
> +	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;
> +}
> 
> Then you have to make the original one __weak.
> 
> Sorry I'm still having difficulties understanding what the benefit is.
> 
> radix_enabled() is defined for every platforms so could just add the 
> following on top of the existing ioremap_range() and voila.
> 
> +	if (radix_enabled())
> +		return radix__ioremap_range(ea, pa, size, prot, nid);
> 
> 
> And with that you wouldn't have the __weak stuff to handle.

I guess so. I don't really like radix_enabled escaping from 64s too
much though. I'll try to improve the code in follow ups if possible.

>>>> -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?
> 
> Yes you are right, nobody should assume that, but ...
> 
> But I think if the fonctions were really different, the preferred 
> alternative would be to move the original function into a file dedicated 
> to nohash64.

Possibly we could do that, but we might be able to just collapse these
back to using generic ioremap_page_range.

Thanks,
Nick