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
