| 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 |
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
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
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
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
"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
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 --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); }
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(-)