Christophe Leroy's on June 7, 2019 4:56 pm:
>
>
> Le 07/06/2019 à 08:19, Nicholas Piggin a écrit :
>> powerpc/64s does not use ioremap_page_range, so it does not get huge
>> vmap iomap mappings automatically. The radix kernel mapping function
>> already allows larger page mappings that work with huge vmap, so wire
>> that up to allow huge pages to be used for ioremap mappings.
>
> Argh ... I was on the way to merge pgtable_64.c and pgtable_32.c. This
> will complicate the task ... Anyway this looks a good improvment.
I can put the radix code into book3s64, at least then you have less
gunk in the common code.
> Any reason to limit that to Radix ?
Just that the others don't have a page size easily exposed for their
map_kernel_page.
It would be nice to make this a bit more generic so other sub archs
can enable the larger mappings just by implementing map_kernel_page.
>
>>
>> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>> ---
>> arch/powerpc/include/asm/book3s/64/pgtable.h | 8 +++
>> arch/powerpc/mm/pgtable_64.c | 58 ++++++++++++++++++--
>> include/linux/io.h | 1 +
>> lib/ioremap.c | 2 +-
>> 4 files changed, 62 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> index ccf00a8b98c6..d7a4f2d80598 100644
>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> @@ -274,6 +274,14 @@ extern unsigned long __vmalloc_end;
>> #define VMALLOC_START __vmalloc_start
>> #define VMALLOC_END __vmalloc_end
>>
>> +static inline unsigned int ioremap_max_order(void)
>> +{
>> + if (radix_enabled())
>> + return PUD_SHIFT;
>> + return 7 + PAGE_SHIFT; /* default from linux/vmalloc.h */
>> +}
>> +#define IOREMAP_MAX_ORDER ({ ioremap_max_order();})
>
> Following form doesn't work ?
>
> #define IOREMAP_MAX_ORDER ioremap_max_order()
I suppose it would. I'm not sure why I did that.
>> +
>> extern unsigned long __kernel_virt_start;
>> extern unsigned long __kernel_virt_size;
>> extern unsigned long __kernel_io_start;
>> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
>> index d2d976ff8a0e..cf02b67eee55 100644
>> --- a/arch/powerpc/mm/pgtable_64.c
>> +++ b/arch/powerpc/mm/pgtable_64.c
>> @@ -112,7 +112,7 @@ unsigned long ioremap_bot = IOREMAP_BASE;
>> * __ioremap_at - Low level function to establish the page tables
>> * for an IO mapping
>> */
>> -void __iomem *__ioremap_at(phys_addr_t pa, void *ea, unsigned long size,
>> pgprot_t prot)
>> +static void __iomem * hash__ioremap_at(phys_addr_t pa, void *ea, unsigned
>> long size, pgprot_t prot)
>
> Is this the correct name ?
>
> As far as I understand, this function will be used by nohash/64, looks
> strange to call hash__something() a function used by nohash platforms.
Yeah you're right, I'll fix that.
>> {
>> unsigned long i;
>>
>> @@ -120,6 +120,54 @@ void __iomem *__ioremap_at(phys_addr_t pa, void *ea,
>> unsigned long size, pgprot_
>> if (pgprot_val(prot) & H_PAGE_4K_PFN)
>> return NULL;
>>
>> + for (i = 0; i < size; i += PAGE_SIZE)
>> + if (map_kernel_page((unsigned long)ea + i, pa + i, prot))
>> + return NULL;
>> +
>> + return (void __iomem *)ea;
>> +}
>> +
>> +static int radix__ioremap_page_range(unsigned long addr, unsigned long end,
>> + phys_addr_t phys_addr, pgprot_t prot)
>> +{
>> + while (addr != end) {
>> + if (unlikely(ioremap_huge_disabled))
>> + goto use_small_page;
>
> I don't like too much a goto in the middle of an if/else set inside a loop.
>
> Couldn't we have two while() loops, one for the !ioremap_huge_disabled()
> and one for the ioremap_huge_disabled() case ? It would duplicate some
> code but that's only 3 small lines.
>
> Or, when ioremap_huge_disabled(), couldn't it just fallback to the
> hash__ioremap_at() function ?
Yeah okay. I'll see how the code looks after I move it into
radix_pgtable.c, it might be best to keep all radix code there together
even for the disabled case.
>> + return radix__ioremap_at(pa, ea, size, prot);
>> + return hash__ioremap_at(pa, ea, size, prot);
>
> Can't we just leave the no radix stuff here instead of making that
> hash__ioremap_at() function ?
Yeah that'll probably work better.
Thanks,
Nick
