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 <npig...@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
