diff mbox series

[v5,RESEND,14/17] mm/ioremap: Consider IOREMAP space in generic ioremap

Message ID 20230515090848.833045-15-bhe@redhat.com (mailing list archive)
State New
Headers show
Series mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand

Commit Message

Baoquan He May 15, 2023, 9:08 a.m. UTC 
From: Christophe Leroy <christophe.leroy@csgroup.eu>

Architectures like powerpc have a dedicated space for IOREMAP mappings.

If so, use it in generic_ioremap_pro().

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

Comments

Mike Rapoport May 16, 2023, 6:53 a.m. UTC | #1 
On Mon, May 15, 2023 at 05:08:45PM +0800, Baoquan He wrote:
> From: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> Architectures like powerpc have a dedicated space for IOREMAP mappings.
> 
> If so, use it in generic_ioremap_pro().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Baoquan He <bhe@redhat.com>

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> ---
>  mm/ioremap.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 2fbe6b9bc50e..4a7749d85044 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -35,8 +35,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;
> @@ -66,7 +71,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);
>  }
>  
> -- 
> 2.34.1
> 
>
Christoph Hellwig May 17, 2023, 6:41 a.m. UTC | #2 
On Mon, May 15, 2023 at 05:08:45PM +0800, Baoquan He wrote:
> @@ -35,8 +35,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

I think this would be cleaner if we'd just always use
__get_vm_area_caller and at the top of the file add a:

#ifndef IOREMAP_START
#define IOREMAP_START	VMALLOC_START
#define IOREMAP_END	VMALLOC_END
#endif

Together with a little comment that ioremap often, but not always
uses the generic vmalloc area.
Christoph Hellwig May 17, 2023, 6:44 a.m. UTC | #3 
On Tue, May 16, 2023 at 11:41:26PM -0700, Christoph Hellwig wrote:
> I think this would be cleaner if we'd just always use
> __get_vm_area_caller and at the top of the file add a:
> 
> #ifndef IOREMAP_START
> #define IOREMAP_START	VMALLOC_START
> #define IOREMAP_END	VMALLOC_END
> #endif
> 
> Together with a little comment that ioremap often, but not always
> uses the generic vmalloc area.

.. and with that we can also simply is_ioremap_addr by moving it
to ioremap.c and making it always operate on the IOREMAP constants.
Baoquan He May 20, 2023, 3:28 a.m. UTC | #4 
On 05/16/23 at 11:41pm, Christoph Hellwig wrote:
> On Mon, May 15, 2023 at 05:08:45PM +0800, Baoquan He wrote:
> > @@ -35,8 +35,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
> 
> I think this would be cleaner if we'd just always use
> __get_vm_area_caller and at the top of the file add a:
> 
> #ifndef IOREMAP_START
> #define IOREMAP_START	VMALLOC_START
> #define IOREMAP_END	VMALLOC_END
> #endif
> 
> Together with a little comment that ioremap often, but not always
> uses the generic vmalloc area.

Great idea, will do as suggested.
Baoquan He May 20, 2023, 3:31 a.m. UTC | #5 
On 05/16/23 at 11:44pm, Christoph Hellwig wrote:
> On Tue, May 16, 2023 at 11:41:26PM -0700, Christoph Hellwig wrote:
> > I think this would be cleaner if we'd just always use
> > __get_vm_area_caller and at the top of the file add a:
> > 
> > #ifndef IOREMAP_START
> > #define IOREMAP_START	VMALLOC_START
> > #define IOREMAP_END	VMALLOC_END
> > #endif
> > 
> > Together with a little comment that ioremap often, but not always
> > uses the generic vmalloc area.
> 
> .. and with that we can also simply is_ioremap_addr by moving it
> to ioremap.c and making it always operate on the IOREMAP constants.

Great idea too, will do. Put this into a separate patch?
Christoph Hellwig May 20, 2023, 5:04 a.m. UTC | #6 
On Sat, May 20, 2023 at 11:31:04AM +0800, Baoquan He wrote:
> > > Together with a little comment that ioremap often, but not always
> > > uses the generic vmalloc area.
> > 
> > .. and with that we can also simply is_ioremap_addr by moving it
> > to ioremap.c and making it always operate on the IOREMAP constants.
> 
> Great idea too, will do. Put this into a separate patch?

Yes.
Baoquan He May 30, 2023, 9:37 a.m. UTC | #7 
On 05/16/23 at 11:44pm, Christoph Hellwig wrote:
> On Tue, May 16, 2023 at 11:41:26PM -0700, Christoph Hellwig wrote:
> > I think this would be cleaner if we'd just always use
> > __get_vm_area_caller and at the top of the file add a:
> > 
> > #ifndef IOREMAP_START
> > #define IOREMAP_START	VMALLOC_START
> > #define IOREMAP_END	VMALLOC_END
> > #endif
> > 
> > Together with a little comment that ioremap often, but not always
> > uses the generic vmalloc area.
> 
> .. and with that we can also simply is_ioremap_addr by moving it
> to ioremap.c and making it always operate on the IOREMAP constants.

In the current code, is_ioremap_addr() is being used in kernel/iomem.c.
However, mm/ioremap.c is only built in when CONFIG_GENERIC_IOREMAP is
enabled. This will impact those architectures which haven't taken
GENERIC_IOREMAP way.

[~]$ git grep is_ioremap_addr
arch/powerpc/include/asm/pgtable.h:#define is_ioremap_addr is_ioremap_addr
arch/powerpc/include/asm/pgtable.h:static inline bool is_ioremap_addr(const void *x)
include/linux/mm.h:static inline bool is_ioremap_addr(const void *x)
include/linux/mm.h:static inline bool is_ioremap_addr(const void *x)
kernel/iomem.c: if (is_ioremap_addr(addr))
mm/ioremap.c:   if (is_ioremap_addr(vaddr))

[bhe@MiWiFi-R3L-srv linux-arm64]$ git grep ioremap mm/Makefile
mm/Makefile:obj-$(CONFIG_GENERIC_EARLY_IOREMAP) += early_ioremap.o
mm/Makefile:obj-$(CONFIG_GENERIC_IOREMAP) += ioremap.o

If we want to consolidate code, we can move is_ioremap_addr() to
include/linux/mm.h libe below. Not sure if it's fine. With it,
both kernel/iomem.c and mm/ioremap.c can use is_ioremap_addr().

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ce77080c79..0fbb94f0f025 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1041,9 +1041,25 @@ unsigned long vmalloc_to_pfn(const void *addr);
  * On nommu, vmalloc/vfree wrap through kmalloc/kfree directly, so there
  * is no special casing required.
  */
-
-#ifndef is_ioremap_addr
-#define is_ioremap_addr(x) is_vmalloc_addr(x)
+#if defined(CONFIG_HAS_IOMEM) || defined(CONFIG_GENERIC_IOREMAP)
+/*
+ * Ioremap often, but not always uses the generic vmalloc area. E.g on
+ * Power ARCH, it could have different ioremap space. 
+ */
+#ifndef IOREMAP_START
+#define IOREMAP_START   VMALLOC_START
+#define IOREMAP_END     VMALLOC_END
+#endif
+static inline bool is_ioremap_addr(const void *x)
+{
+	unsigned long addr = (unsigned long)kasan_reset_tag(x);
+	return addr >= IOREMAP_START && addr < IOREMAP_END;
+}
+#else
+static inline bool is_ioremap_addr(const void *x)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_MMU
diff mbox series

Patch

diff --git a/mm/ioremap.c b/mm/ioremap.c
index 2fbe6b9bc50e..4a7749d85044 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -35,8 +35,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;
@@ -66,7 +71,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);
 }