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

[Baoquan He]

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