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

  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(-)
  

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
> 
>
  

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.
  

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.
  

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.
  

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?
  

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.
  

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
  

On Tue, May 30, 2023 at 05:37:23PM +0800, Baoquan He wrote:
> 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().

Can we just add a ne header for this given that no one else really
needs it?
  

On 06/01/23 at 04:13am, Christoph Hellwig wrote:
> On Tue, May 30, 2023 at 05:37:23PM +0800, Baoquan He wrote:
> > 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().
> 
> Can we just add a ne header for this given that no one else really
> needs it?

Is it OK like below?

From fe5d4d25afa1e989fa82877c8466a97fc8bd8c93 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Fri, 2 Jun 2023 18:36:48 +0800
Subject: [PATCH] mm: move is_ioremap_addr() into new header file
Content-type: text/plain

Now is_ioremap_addr() is only used in kernel/iomem.c and gonna be used
in mm/ioremap.c. Move it into its own new header file linux/ioremap.h.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 include/linux/ioremap.h | 29 +++++++++++++++++++++++++++++
 include/linux/mm.h      |  5 -----
 kernel/iomem.c          |  1 +
 mm/ioremap.c            |  1 +
 4 files changed, 31 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/ioremap.h

diff --git a/include/linux/ioremap.h b/include/linux/ioremap.h
new file mode 100644
index 000000000000..2fd51a77ebdc
--- /dev/null
+++ b/include/linux/ioremap.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_IOREMAP_H
+#define _LINUX_IOREMAP_H
+
+#include <linux/kasan.h>
+#include <asm/pgtable.h>
+
+#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
+
+#endif /* _LINUX_IOREMAP_H */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ce77080c79..7379f19768b4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1041,11 +1041,6 @@ 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)
-#endif
-
 #ifdef CONFIG_MMU
 extern bool is_vmalloc_addr(const void *x);
 extern int is_vmalloc_or_module_addr(const void *x);
diff --git a/kernel/iomem.c b/kernel/iomem.c
index 62c92e43aa0d..9682471e6471 100644
--- a/kernel/iomem.c
+++ b/kernel/iomem.c
@@ -3,6 +3,7 @@
 #include <linux/types.h>
 #include <linux/io.h>
 #include <linux/mm.h>
+#include <linux/ioremap.h>
 
 #ifndef ioremap_cache
 /* temporary while we convert existing ioremap_cache users to memremap */
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 0248e630561b..3dede3302eba 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -10,6 +10,7 @@
 #include <linux/mm.h>
 #include <linux/io.h>
 #include <linux/export.h>
+#include <linux/ioremap.h>
 
 /*
  * Ioremap often, but not always uses the generic vmalloc area. E.g on
  

On Fri, Jun 02, 2023 at 06:42:59PM +0800, Baoquan He wrote:
> On 06/01/23 at 04:13am, Christoph Hellwig wrote:
> > On Tue, May 30, 2023 at 05:37:23PM +0800, Baoquan He wrote:
> > > 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().
> > 
> > Can we just add a ne header for this given that no one else really
> > needs it?
> 
> Is it OK like below?

Looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>