[v1,05/10] powerpc/mm: Do early ioremaps from top to bottom on PPC64 too.
diff mbox series

Message ID 019c5d90f7027ccff00e38a3bcd633d290f6af59.1565726867.git.christophe.leroy@c-s.fr
State Superseded
Headers show
Series
  • [v1,01/10] powerpc/mm: drop ppc_md.iounmap()
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 66 lines checked
snowpatch_ozlabs/apply_patch success Successfully applied on branch next (da206bd46848568e1aaf35f00e2d78bf9bc94f95)

Commit Message

Christophe Leroy Aug. 13, 2019, 8:11 p.m. UTC
Until vmalloc system is up and running, ioremap basically
allocates addresses at the border of the IOREMAP area.

On PPC32, addresses are allocated down from the top of the area
while on PPC64, addresses are allocated up from the base of the
area.

On PPC32, the base of vmalloc area is not known yet when ioremap()
starts to be used, while the end of it is fixed. On PPC64, both the
start and the end are already fixed when ioremap() starts to being
used.

Changing PPC64 behaviour is the lighest change, so change PPC64
ioremap() to allocate addresses from the top as PPC32 does.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/mm/book3s64/hash_utils.c    |  2 +-
 arch/powerpc/mm/book3s64/radix_pgtable.c |  2 +-
 arch/powerpc/mm/pgtable_64.c             | 18 +++++++++---------
 3 files changed, 11 insertions(+), 11 deletions(-)

Comments

Christoph Hellwig Aug. 14, 2019, 5:55 a.m. UTC | #1
On Tue, Aug 13, 2019 at 08:11:38PM +0000, Christophe Leroy wrote:
> Until vmalloc system is up and running, ioremap basically
> allocates addresses at the border of the IOREMAP area.

Note that while a few other architectures have a magic hack like powerpc
to make ioremap work before vmalloc, the normal practice would be
to explicitly use early_ioremap.  I guess your change is fine for now,
but it might make sense convert powerpc to the explicit early_ioremap
scheme as well.
Christophe Leroy Aug. 14, 2019, 6:10 a.m. UTC | #2
Le 14/08/2019 à 07:55, Christoph Hellwig a écrit :
> On Tue, Aug 13, 2019 at 08:11:38PM +0000, Christophe Leroy wrote:
>> Until vmalloc system is up and running, ioremap basically
>> allocates addresses at the border of the IOREMAP area.
> 
> Note that while a few other architectures have a magic hack like powerpc
> to make ioremap work before vmalloc, the normal practice would be
> to explicitly use early_ioremap.  I guess your change is fine for now,
> but it might make sense convert powerpc to the explicit early_ioremap
> scheme as well.
> 

I've been looking into early_ioremap(), but IIUC early_ioremap() is for 
ephemeral mappings only, it expects all early mappings to be gone at the 
end of init.

PPC installs definitive early mappings (for instance for PCI). How does 
that have to be handled ?

Christophe
Christoph Hellwig Aug. 14, 2019, 6:14 a.m. UTC | #3
On Wed, Aug 14, 2019 at 08:10:59AM +0200, Christophe Leroy wrote:
> > Note that while a few other architectures have a magic hack like powerpc
> > to make ioremap work before vmalloc, the normal practice would be
> > to explicitly use early_ioremap.  I guess your change is fine for now,
> > but it might make sense convert powerpc to the explicit early_ioremap
> > scheme as well.
> > 
> 
> I've been looking into early_ioremap(), but IIUC early_ioremap() is for
> ephemeral mappings only, it expects all early mappings to be gone at the end
> of init.

Yes.

> PPC installs definitive early mappings (for instance for PCI). How does that
> have to be handled ?

Good question, and no good answer.  I've just been looking at a generic
ioremap for simple architectures, and been finding all kinds of crap
and inconsistencies, and this is one of the things I noticed.
Nicholas Piggin Aug. 19, 2019, 1:42 p.m. UTC | #4
Christophe Leroy's on August 14, 2019 6:11 am:
> Until vmalloc system is up and running, ioremap basically
> allocates addresses at the border of the IOREMAP area.
> 
> On PPC32, addresses are allocated down from the top of the area
> while on PPC64, addresses are allocated up from the base of the
> area.
 
This series looks pretty good to me, but I'm not sure about this patch.

It seems like quite a small divergence in terms of code, and it looks
like the final result still has some ifdefs in these functions. Maybe
you could just keep existing behaviour for this cleanup series so it
does not risk triggering some obscure regression? Merging behaviour
could be proposed at the end.

Thanks,
Nick
Michael Ellerman Aug. 20, 2019, 12:20 a.m. UTC | #5
Nicholas Piggin <npiggin@gmail.com> writes:
> Christophe Leroy's on August 14, 2019 6:11 am:
>> Until vmalloc system is up and running, ioremap basically
>> allocates addresses at the border of the IOREMAP area.
>> 
>> On PPC32, addresses are allocated down from the top of the area
>> while on PPC64, addresses are allocated up from the base of the
>> area.
>  
> This series looks pretty good to me, but I'm not sure about this patch.
>
> It seems like quite a small divergence in terms of code, and it looks
> like the final result still has some ifdefs in these functions. Maybe
> you could just keep existing behaviour for this cleanup series so it
> does not risk triggering some obscure regression?

Yeah that is also my feeling. Changing it *should* work, and I haven't
found anything that breaks yet, but it's one of those things that's
bound to break something for some obscure reason.

Christophe do you think you can rework it to retain the different
allocation directions at least for now?

cheers
Christophe Leroy Aug. 20, 2019, 5:10 a.m. UTC | #6
Le 20/08/2019 à 02:20, Michael Ellerman a écrit :
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Christophe Leroy's on August 14, 2019 6:11 am:
>>> Until vmalloc system is up and running, ioremap basically
>>> allocates addresses at the border of the IOREMAP area.
>>>
>>> On PPC32, addresses are allocated down from the top of the area
>>> while on PPC64, addresses are allocated up from the base of the
>>> area.
>>   
>> This series looks pretty good to me, but I'm not sure about this patch.
>>
>> It seems like quite a small divergence in terms of code, and it looks
>> like the final result still has some ifdefs in these functions. Maybe
>> you could just keep existing behaviour for this cleanup series so it
>> does not risk triggering some obscure regression?
> 
> Yeah that is also my feeling. Changing it *should* work, and I haven't
> found anything that breaks yet, but it's one of those things that's
> bound to break something for some obscure reason.
> 
> Christophe do you think you can rework it to retain the different
> allocation directions at least for now?
> 

Yes I have started addressing the comments I received, and I think for 
now I'll keep all the machinery aside from the merge. Not sure yet if 
I'll leave it in pgtables_32/64.c or if I'll add ioremap_32/64.c

Christophe

Patch
diff mbox series

diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index e6d471058597..0f954dc40346 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -1030,7 +1030,7 @@  void __init hash__early_init_mmu(void)
 	__kernel_io_start = H_KERN_IO_START;
 	__kernel_io_end = H_KERN_IO_END;
 	vmemmap = (struct page *)H_VMEMMAP_START;
-	ioremap_bot = IOREMAP_BASE;
+	ioremap_bot = IOREMAP_END;
 
 #ifdef CONFIG_PCI
 	pci_io_base = ISA_IO_BASE;
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index b4ca9e95e678..11303e2fffb1 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -611,7 +611,7 @@  void __init radix__early_init_mmu(void)
 	__kernel_io_start = RADIX_KERN_IO_START;
 	__kernel_io_end = RADIX_KERN_IO_END;
 	vmemmap = (struct page *)RADIX_VMEMMAP_START;
-	ioremap_bot = IOREMAP_BASE;
+	ioremap_bot = IOREMAP_END;
 
 #ifdef CONFIG_PCI
 	pci_io_base = ISA_IO_BASE;
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index 6fa2e969bf0e..0f0b1e1ea5ab 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -101,7 +101,7 @@  unsigned long __pte_frag_size_shift;
 EXPORT_SYMBOL(__pte_frag_size_shift);
 unsigned long ioremap_bot;
 #else /* !CONFIG_PPC_BOOK3S_64 */
-unsigned long ioremap_bot = IOREMAP_BASE;
+unsigned long ioremap_bot = IOREMAP_END;
 #endif
 
 int __weak ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
@@ -169,11 +169,11 @@  void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size,
 
 	/*
 	 * Choose an address to map it to.
-	 * Once the imalloc system is running, we use it.
+	 * Once the vmalloc system is running, we use it.
 	 * Before that, we map using addresses going
-	 * up from ioremap_bot.  imalloc will use
-	 * the addresses from ioremap_bot through
-	 * IMALLOC_END
+	 * down from ioremap_bot.  vmalloc will use
+	 * the addresses from IOREMAP_BASE through
+	 * ioremap_bot
 	 * 
 	 */
 	paligned = addr & PAGE_MASK;
@@ -186,7 +186,7 @@  void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size,
 		struct vm_struct *area;
 
 		area = __get_vm_area_caller(size, VM_IOREMAP,
-					    ioremap_bot, IOREMAP_END,
+					    IOREMAP_BASE, ioremap_bot,
 					    caller);
 		if (area == NULL)
 			return NULL;
@@ -194,9 +194,9 @@  void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size,
 		area->phys_addr = paligned;
 		ret = __ioremap_at(paligned, area->addr, size, prot);
 	} else {
-		ret = __ioremap_at(paligned, (void *)ioremap_bot, size, prot);
+		ret = __ioremap_at(paligned, (void *)ioremap_bot - size, size, prot);
 		if (ret)
-			ioremap_bot += size;
+			ioremap_bot -= size;
 	}
 
 	if (ret)
@@ -217,7 +217,7 @@  void __iounmap(volatile void __iomem *token)
 	
 	addr = (void *) ((unsigned long __force)
 			 PCI_FIX_ADDR(token) & PAGE_MASK);
-	if ((unsigned long)addr < ioremap_bot) {
+	if ((unsigned long)addr >= ioremap_bot) {
 		printk(KERN_WARNING "Attempt to iounmap early bolted mapping"
 		       " at 0x%p\n", addr);
 		return;