diff mbox series

[RFC,7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

Message ID 8c7ac4667c6a3cc48f98110117536f60d51ece4a.1665568707.git.christophe.leroy@csgroup.eu (mailing list archive)
State New
Headers show
Series mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative) | expand

Commit Message

Christophe Leroy Oct. 12, 2022, 10:09 a.m. UTC
Some architecture have a dedicated space for IOREMAP mappings.

If so, use it in generic_ioremap_pro().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 mm/ioremap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Arnd Bergmann Oct. 12, 2022, 10:39 a.m. UTC | #1
On Wed, Oct 12, 2022, at 12:09 PM, Christophe Leroy wrote:
> Some architecture have a dedicated space for IOREMAP mappings.
>
> If so, use it in generic_ioremap_pro().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

"Some" means exactly powerpc64, right? It looks like microblaze
and powerpc32 still share some of this code, but effectively
just use the vmalloc area once the slab allocator is up.

Is the special case still useful for powerpc64 or could this be
changed to do it the same as everything else?

    Arnd
Alexander Gordeev Oct. 16, 2022, 7:54 a.m. UTC | #2
On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
> "Some" means exactly powerpc64, right? It looks like microblaze
> and powerpc32 still share some of this code, but effectively
> just use the vmalloc area once the slab allocator is up.
> 
> Is the special case still useful for powerpc64 or could this be
> changed to do it the same as everything else?

Or make it the other way around and set IOREMAP_START/IOREMAP_END
to VMALLOC_START/VMALLOC_END by default?

> 
>     Arnd
Arnd Bergmann Oct. 16, 2022, 11:51 a.m. UTC | #3
On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>> "Some" means exactly powerpc64, right? It looks like microblaze
>> and powerpc32 still share some of this code, but effectively
>> just use the vmalloc area once the slab allocator is up.
>> 
>> Is the special case still useful for powerpc64 or could this be
>> changed to do it the same as everything else?
>
> Or make it the other way around and set IOREMAP_START/IOREMAP_END
> to VMALLOC_START/VMALLOC_END by default?

Sure, if there is a reason for actually making them different.
From the git history, it appears that before commit 3d5134ee8341
("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
ioremap() and vmalloc() handling was largely duplicated. Ben
cleaned it up by making most of the implementation shared but left
the separate address spaces.

My guess is that there was no technical reason for this, other
than having no reason to change the behavior at the time.

       Arnd
Christophe Leroy Oct. 16, 2022, 4:56 p.m. UTC | #4
I'm more focussed on powerpc32

+ Adding linuxppc-dev, someone else might help.

Le 16/10/2022 à 13:51, Arnd Bergmann a écrit :
> On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
>> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>>> "Some" means exactly powerpc64, right? It looks like microblaze
>>> and powerpc32 still share some of this code, but effectively
>>> just use the vmalloc area once the slab allocator is up.
>>>
>>> Is the special case still useful for powerpc64 or could this be
>>> changed to do it the same as everything else?
>>
>> Or make it the other way around and set IOREMAP_START/IOREMAP_END
>> to VMALLOC_START/VMALLOC_END by default?
> 
> Sure, if there is a reason for actually making them different.
>  From the git history, it appears that before commit 3d5134ee8341
> ("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
> ioremap() and vmalloc() handling was largely duplicated. Ben
> cleaned it up by making most of the implementation shared but left
> the separate address spaces.
> 
> My guess is that there was no technical reason for this, other
> than having no reason to change the behavior at the time.
> 
>         Arnd
Michael Ellerman Oct. 17, 2022, 12:50 p.m. UTC | #5
"Arnd Bergmann" <arnd@arndb.de> writes:
> On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
>> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>>> "Some" means exactly powerpc64, right? It looks like microblaze
>>> and powerpc32 still share some of this code, but effectively
>>> just use the vmalloc area once the slab allocator is up.
>>> 
>>> Is the special case still useful for powerpc64 or could this be
>>> changed to do it the same as everything else?
>>
>> Or make it the other way around and set IOREMAP_START/IOREMAP_END
>> to VMALLOC_START/VMALLOC_END by default?
>
> Sure, if there is a reason for actually making them different.
> From the git history, it appears that before commit 3d5134ee8341
> ("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
> ioremap() and vmalloc() handling was largely duplicated. Ben
> cleaned it up by making most of the implementation shared but left
> the separate address spaces.
>
> My guess is that there was no technical reason for this, other
> than having no reason to change the behavior at the time.

I think the immediate reason for it is that on some CPUs we have to use
4K pages in the HPT for IO mappings, but PAGE_SIZE == 64K, and we can
only have a single page size per segment (256M or 1T).

cheers
Arnd Bergmann Oct. 17, 2022, 8:52 p.m. UTC | #6
On Mon, Oct 17, 2022, at 2:50 PM, Michael Ellerman wrote:
> "Arnd Bergmann" <arnd@arndb.de> writes:
>>
>> My guess is that there was no technical reason for this, other
>> than having no reason to change the behavior at the time.
>
> I think the immediate reason for it is that on some CPUs we have to use
> 4K pages in the HPT for IO mappings, but PAGE_SIZE == 64K, and we can
> only have a single page size per segment (256M or 1T).

Right, makes sense. Both the original patch, or the variant with
defining IOREMAP_START everywhere seem fine to me then.

     Arnd
diff mbox series

Patch

diff --git a/mm/ioremap.c b/mm/ioremap.c
index 9f34a8f90b58..a2be9cda975b 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -31,8 +31,13 @@  void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
 	if (!ioremap_allowed(phys_addr, size, pgprot_val(prot)))
 		return NULL;
 
+#ifdef IOREMAP_START
+	area = __get_vm_area_caller(size, VM_IOREMAP, IOREMAP_START,
+				    IOREMAP_END, __builtin_return_address(0));
+#else
 	area = get_vm_area_caller(size, VM_IOREMAP,
 			__builtin_return_address(0));
+#endif
 	if (!area)
 		return NULL;
 	vaddr = (unsigned long)area->addr;
@@ -62,7 +67,7 @@  void generic_iounmap(volatile void __iomem *addr)
 	if (!iounmap_allowed(vaddr))
 		return;
 
-	if (is_vmalloc_addr(vaddr))
+	if (is_ioremap_addr(vaddr))
 		vunmap(vaddr);
 }