[PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Kefeng Wang]

Let's arm64 use GENERIC_IOREMAP to cleanup code, and
support ioremap_prot()/HAVE_IOREMAP_PROT, which could
enable generic_access_phys().

Kefeng Wang (4):
  mm: ioremap: Setup phys_addr of struct vm_struct
  mm: ioremap: Add arch_ioremap/iounmap_check()
  arm64: mm: Convert to GENERIC_IOREMAP
  arm64: Add HAVE_IOREMAP_PROT support

 .../features/vm/ioremap_prot/arch-support.txt |  2 +-
 arch/arm64/Kconfig                            |  2 +
 arch/arm64/include/asm/io.h                   | 14 +--
 arch/arm64/include/asm/pgtable.h              | 10 +++
 arch/arm64/kernel/acpi.c                      |  2 +-
 arch/arm64/mm/hugetlbpage.c                   | 10 ---
 arch/arm64/mm/ioremap.c                       | 86 +++----------------
 include/asm-generic/io.h                      |  3 +
 mm/ioremap.c                                  | 21 ++++-
 9 files changed, 56 insertions(+), 94 deletions(-)

-- 
2.26.2

[PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Kefeng Wang]

Let's arm64 use GENERIC_IOREMAP to cleanup code, and
support ioremap_prot()/HAVE_IOREMAP_PROT, which could
enable generic_access_phys().

Kefeng Wang (4):
  mm: ioremap: Setup phys_addr of struct vm_struct
  mm: ioremap: Add arch_ioremap/iounmap_check()
  arm64: mm: Convert to GENERIC_IOREMAP
  arm64: Add HAVE_IOREMAP_PROT support

 .../features/vm/ioremap_prot/arch-support.txt |  2 +-
 arch/arm64/Kconfig                            |  2 +
 arch/arm64/include/asm/io.h                   | 14 +--
 arch/arm64/include/asm/pgtable.h              | 10 +++
 arch/arm64/kernel/acpi.c                      |  2 +-
 arch/arm64/mm/hugetlbpage.c                   | 10 ---
 arch/arm64/mm/ioremap.c                       | 86 +++----------------
 include/asm-generic/io.h                      |  3 +
 mm/ioremap.c                                  | 21 ++++-
 9 files changed, 56 insertions(+), 94 deletions(-)

-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Kefeng Wang]

Show physical address in /proc/vmallocinfo.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/ioremap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/ioremap.c b/mm/ioremap.c
index 5fe598ecd9b7..522ef899c35f 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 	if (!area)
 		return NULL;
 	vaddr = (unsigned long)area->addr;
+	area->phys_addr = addr;
 
 	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
 		free_vm_area(area);
-- 
2.26.2

[PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Kefeng Wang]

Show physical address in /proc/vmallocinfo.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/ioremap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/ioremap.c b/mm/ioremap.c
index 5fe598ecd9b7..522ef899c35f 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 	if (!area)
 		return NULL;
 	vaddr = (unsigned long)area->addr;
+	area->phys_addr = addr;
 
 	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
 		free_vm_area(area);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

Add special check hook for architecture to verify addr, size
or prot when ioremap() or iounmap(), which will make the generic
ioremap more useful.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/asm-generic/io.h |  3 +++
 mm/ioremap.c             | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ce93aaf69f8..26924fded7c3 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
 #elif defined(CONFIG_GENERIC_IOREMAP)
 #include <linux/pgtable.h>
 
+bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
+bool arch_iounmap_check(void __iomem *addr);
+
 void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
 void iounmap(volatile void __iomem *addr);
 
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 522ef899c35f..d1117005dcc7 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -11,6 +11,16 @@
 #include <linux/io.h>
 #include <linux/export.h>
 
+bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
+{
+	return true;
+}
+
+bool __weak arch_iounmap_check(void __iomem *addr)
+{
+	return true;
+}
+
 void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 {
 	unsigned long offset, vaddr;
@@ -27,6 +37,9 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 	addr -= offset;
 	size = PAGE_ALIGN(size + offset);
 
+	if (!arch_ioremap_check(addr, size, prot))
+		return NULL;
+
 	area = get_vm_area_caller(size, VM_IOREMAP,
 			__builtin_return_address(0));
 	if (!area)
@@ -45,6 +58,11 @@ EXPORT_SYMBOL(ioremap_prot);
 
 void iounmap(volatile void __iomem *addr)
 {
-	vunmap((void *)((unsigned long)addr & PAGE_MASK));
+	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
+
+	if (!arch_iounmap_check(vaddr))
+		return;
+
+	vunmap(vaddr);
 }
 EXPORT_SYMBOL(iounmap);
-- 
2.26.2

[PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

Add special check hook for architecture to verify addr, size
or prot when ioremap() or iounmap(), which will make the generic
ioremap more useful.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/asm-generic/io.h |  3 +++
 mm/ioremap.c             | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ce93aaf69f8..26924fded7c3 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
 #elif defined(CONFIG_GENERIC_IOREMAP)
 #include <linux/pgtable.h>
 
+bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
+bool arch_iounmap_check(void __iomem *addr);
+
 void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
 void iounmap(volatile void __iomem *addr);
 
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 522ef899c35f..d1117005dcc7 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -11,6 +11,16 @@
 #include <linux/io.h>
 #include <linux/export.h>
 
+bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
+{
+	return true;
+}
+
+bool __weak arch_iounmap_check(void __iomem *addr)
+{
+	return true;
+}
+
 void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 {
 	unsigned long offset, vaddr;
@@ -27,6 +37,9 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
 	addr -= offset;
 	size = PAGE_ALIGN(size + offset);
 
+	if (!arch_ioremap_check(addr, size, prot))
+		return NULL;
+
 	area = get_vm_area_caller(size, VM_IOREMAP,
 			__builtin_return_address(0));
 	if (!area)
@@ -45,6 +58,11 @@ EXPORT_SYMBOL(ioremap_prot);
 
 void iounmap(volatile void __iomem *addr)
 {
-	vunmap((void *)((unsigned long)addr & PAGE_MASK));
+	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
+
+	if (!arch_iounmap_check(vaddr))
+		return;
+
+	vunmap(vaddr);
 }
 EXPORT_SYMBOL(iounmap);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

[Kefeng Wang]

Add hook for arch's special check when ioremap() and iounmap(),
then ioremap_wc/np/cache is converted to use ioremap_prot()
from GENERIC_IOREMAP, update the Copyright and kill the unused
inclusions.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm64/Kconfig          |  1 +
 arch/arm64/include/asm/io.h | 14 +++---
 arch/arm64/kernel/acpi.c    |  2 +-
 arch/arm64/mm/ioremap.c     | 86 +++++--------------------------------
 4 files changed, 21 insertions(+), 82 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 57c4c995965f..838c45f1517b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -123,6 +123,7 @@ config ARM64
 	select GENERIC_CPU_VULNERABILITIES
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
+	select GENERIC_IOREMAP
 	select GENERIC_IRQ_IPI
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 7fd836bea7eb..773fffa62a14 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -163,13 +163,15 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
 /*
  * I/O memory mapping functions.
  */
-extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot);
-extern void iounmap(volatile void __iomem *addr);
-extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 
-#define ioremap(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
-#define ioremap_wc(addr, size)		__ioremap((addr), (size), __pgprot(PROT_NORMAL_NC))
-#define ioremap_np(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
+#define _PAGE_IOREMAP PROT_DEVICE_nGnRE
+
+#define ioremap_wc(addr, size)		ioremap_prot((addr), (size), PROT_NORMAL_NC)
+#define ioremap_np(addr, size)		ioremap_prot((addr), (size), PROT_DEVICE_nGnRnE)
+#define ioremap_cache(addr, size) ({  \
+	pfn_is_map_memory(__phys_to_pfn(addr)) ?	\
+	(void __iomem *)__phys_to_virt(addr) : ioremap_prot(addr, size, PROT_NORMAL);	\
+})
 
 /*
  * io{read,write}{16,32,64}be() macros
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index e4dea8db6924..a5a256e3f9fe 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -351,7 +351,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 				prot = __acpi_get_writethrough_mem_attribute();
 		}
 	}
-	return __ioremap(phys, size, prot);
+	return ioremap_prot(phys, size, pgprot_val(prot));
 }
 
 /*
diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
index b7c81dacabf0..90e8a248e733 100644
--- a/arch/arm64/mm/ioremap.c
+++ b/arch/arm64/mm/ioremap.c
@@ -1,96 +1,32 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/*
- * Based on arch/arm/mm/ioremap.c
- *
- * (C) Copyright 1995 1996 Linus Torvalds
- * Hacked for ARM by Phil Blundell <philb@gnu.org>
- * Hacked to allow all architectures to build, and various cleanups
- * by Russell King
- * Copyright (C) 2012 ARM Ltd.
- */
 
-#include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <linux/io.h>
 
-#include <asm/fixmap.h>
-#include <asm/tlbflush.h>
-
-static void __iomem *__ioremap_caller(phys_addr_t phys_addr, size_t size,
-				      pgprot_t prot, void *caller)
+bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
 {
-	unsigned long last_addr;
-	unsigned long offset = phys_addr & ~PAGE_MASK;
-	int err;
-	unsigned long addr;
-	struct vm_struct *area;
-
-	/*
-	 * Page align the mapping address and size, taking account of any
-	 * offset.
-	 */
-	phys_addr &= PAGE_MASK;
-	size = PAGE_ALIGN(size + offset);
-
-	/*
-	 * Don't allow wraparound, zero size or outside PHYS_MASK.
-	 */
-	last_addr = phys_addr + size - 1;
-	if (!size || last_addr < phys_addr || (last_addr & ~PHYS_MASK))
-		return NULL;
-
-	/*
-	 * Don't allow RAM to be mapped.
-	 */
-	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
-		return NULL;
+	unsigned long last_addr = addr + size - 1;
 
-	area = get_vm_area_caller(size, VM_IOREMAP, caller);
-	if (!area)
-		return NULL;
-	addr = (unsigned long)area->addr;
-	area->phys_addr = phys_addr;
+	/* Don't allow outside PHYS_MASK */
+	if (last_addr & ~PHYS_MASK)
+		return false;
 
-	err = ioremap_page_range(addr, addr + size, phys_addr, prot);
-	if (err) {
-		vunmap((void *)addr);
-		return NULL;
-	}
+	/* Don't allow RAM to be mapped. */
+	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(addr))))
+		return false;
 
-	return (void __iomem *)(offset + addr);
-}
-
-void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot)
-{
-	return __ioremap_caller(phys_addr, size, prot,
-				__builtin_return_address(0));
+	return true;
 }
-EXPORT_SYMBOL(__ioremap);
 
-void iounmap(volatile void __iomem *io_addr)
+bool arch_iounmap_check(void __iomem *addr)
 {
-	unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
-
 	/*
 	 * We could get an address outside vmalloc range in case
 	 * of ioremap_cache() reusing a RAM mapping.
 	 */
-	if (is_vmalloc_addr((void *)addr))
-		vunmap((void *)addr);
-}
-EXPORT_SYMBOL(iounmap);
-
-void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
-{
-	/* For normal memory we already have a cacheable mapping. */
-	if (pfn_is_map_memory(__phys_to_pfn(phys_addr)))
-		return (void __iomem *)__phys_to_virt(phys_addr);
-
-	return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL),
-				__builtin_return_address(0));
+	return is_vmalloc_addr(addr);
 }
-EXPORT_SYMBOL(ioremap_cache);
 
 /*
  * Must be called after early_fixmap_init
-- 
2.26.2

[PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

[Kefeng Wang]

Add hook for arch's special check when ioremap() and iounmap(),
then ioremap_wc/np/cache is converted to use ioremap_prot()
from GENERIC_IOREMAP, update the Copyright and kill the unused
inclusions.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm64/Kconfig          |  1 +
 arch/arm64/include/asm/io.h | 14 +++---
 arch/arm64/kernel/acpi.c    |  2 +-
 arch/arm64/mm/ioremap.c     | 86 +++++--------------------------------
 4 files changed, 21 insertions(+), 82 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 57c4c995965f..838c45f1517b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -123,6 +123,7 @@ config ARM64
 	select GENERIC_CPU_VULNERABILITIES
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
+	select GENERIC_IOREMAP
 	select GENERIC_IRQ_IPI
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 7fd836bea7eb..773fffa62a14 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -163,13 +163,15 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
 /*
  * I/O memory mapping functions.
  */
-extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot);
-extern void iounmap(volatile void __iomem *addr);
-extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 
-#define ioremap(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
-#define ioremap_wc(addr, size)		__ioremap((addr), (size), __pgprot(PROT_NORMAL_NC))
-#define ioremap_np(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
+#define _PAGE_IOREMAP PROT_DEVICE_nGnRE
+
+#define ioremap_wc(addr, size)		ioremap_prot((addr), (size), PROT_NORMAL_NC)
+#define ioremap_np(addr, size)		ioremap_prot((addr), (size), PROT_DEVICE_nGnRnE)
+#define ioremap_cache(addr, size) ({  \
+	pfn_is_map_memory(__phys_to_pfn(addr)) ?	\
+	(void __iomem *)__phys_to_virt(addr) : ioremap_prot(addr, size, PROT_NORMAL);	\
+})
 
 /*
  * io{read,write}{16,32,64}be() macros
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index e4dea8db6924..a5a256e3f9fe 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -351,7 +351,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 				prot = __acpi_get_writethrough_mem_attribute();
 		}
 	}
-	return __ioremap(phys, size, prot);
+	return ioremap_prot(phys, size, pgprot_val(prot));
 }
 
 /*
diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
index b7c81dacabf0..90e8a248e733 100644
--- a/arch/arm64/mm/ioremap.c
+++ b/arch/arm64/mm/ioremap.c
@@ -1,96 +1,32 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/*
- * Based on arch/arm/mm/ioremap.c
- *
- * (C) Copyright 1995 1996 Linus Torvalds
- * Hacked for ARM by Phil Blundell <philb@gnu.org>
- * Hacked to allow all architectures to build, and various cleanups
- * by Russell King
- * Copyright (C) 2012 ARM Ltd.
- */
 
-#include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <linux/io.h>
 
-#include <asm/fixmap.h>
-#include <asm/tlbflush.h>
-
-static void __iomem *__ioremap_caller(phys_addr_t phys_addr, size_t size,
-				      pgprot_t prot, void *caller)
+bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
 {
-	unsigned long last_addr;
-	unsigned long offset = phys_addr & ~PAGE_MASK;
-	int err;
-	unsigned long addr;
-	struct vm_struct *area;
-
-	/*
-	 * Page align the mapping address and size, taking account of any
-	 * offset.
-	 */
-	phys_addr &= PAGE_MASK;
-	size = PAGE_ALIGN(size + offset);
-
-	/*
-	 * Don't allow wraparound, zero size or outside PHYS_MASK.
-	 */
-	last_addr = phys_addr + size - 1;
-	if (!size || last_addr < phys_addr || (last_addr & ~PHYS_MASK))
-		return NULL;
-
-	/*
-	 * Don't allow RAM to be mapped.
-	 */
-	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
-		return NULL;
+	unsigned long last_addr = addr + size - 1;
 
-	area = get_vm_area_caller(size, VM_IOREMAP, caller);
-	if (!area)
-		return NULL;
-	addr = (unsigned long)area->addr;
-	area->phys_addr = phys_addr;
+	/* Don't allow outside PHYS_MASK */
+	if (last_addr & ~PHYS_MASK)
+		return false;
 
-	err = ioremap_page_range(addr, addr + size, phys_addr, prot);
-	if (err) {
-		vunmap((void *)addr);
-		return NULL;
-	}
+	/* Don't allow RAM to be mapped. */
+	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(addr))))
+		return false;
 
-	return (void __iomem *)(offset + addr);
-}
-
-void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot)
-{
-	return __ioremap_caller(phys_addr, size, prot,
-				__builtin_return_address(0));
+	return true;
 }
-EXPORT_SYMBOL(__ioremap);
 
-void iounmap(volatile void __iomem *io_addr)
+bool arch_iounmap_check(void __iomem *addr)
 {
-	unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
-
 	/*
 	 * We could get an address outside vmalloc range in case
 	 * of ioremap_cache() reusing a RAM mapping.
 	 */
-	if (is_vmalloc_addr((void *)addr))
-		vunmap((void *)addr);
-}
-EXPORT_SYMBOL(iounmap);
-
-void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
-{
-	/* For normal memory we already have a cacheable mapping. */
-	if (pfn_is_map_memory(__phys_to_pfn(phys_addr)))
-		return (void __iomem *)__phys_to_virt(phys_addr);
-
-	return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL),
-				__builtin_return_address(0));
+	return is_vmalloc_addr(addr);
 }
-EXPORT_SYMBOL(ioremap_cache);
 
 /*
  * Must be called after early_fixmap_init
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] arm64: Add HAVE_IOREMAP_PROT support

[Kefeng Wang]

With ioremap_prot() defination from generic ioremap, also move
pte_pgprot() from hugetlbpage.c into pgtable.h, then arm64 could
have HAVE_IOREMAP_PROT, which will enable generic_access_phys()
code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 .../features/vm/ioremap_prot/arch-support.txt          |  2 +-
 arch/arm64/Kconfig                                     |  1 +
 arch/arm64/include/asm/pgtable.h                       | 10 ++++++++++
 arch/arm64/mm/hugetlbpage.c                            | 10 ----------
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/features/vm/ioremap_prot/arch-support.txt b/Documentation/features/vm/ioremap_prot/arch-support.txt
index a6dcbe5f47b6..b39ad5d61216 100644
--- a/Documentation/features/vm/ioremap_prot/arch-support.txt
+++ b/Documentation/features/vm/ioremap_prot/arch-support.txt
@@ -9,7 +9,7 @@
     |       alpha: | TODO |
     |         arc: |  ok  |
     |         arm: | TODO |
-    |       arm64: | TODO |
+    |       arm64: |  ok  |
     |        csky: | TODO |
     |       h8300: | TODO |
     |     hexagon: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 838c45f1517b..0357708c4d32 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -188,6 +188,7 @@ config ARM64
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_GCC_PLUGINS
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS
+	select HAVE_IOREMAP_PROT
 	select HAVE_IRQ_TIME_ACCOUNTING
 	select HAVE_KVM
 	select HAVE_NMI
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 94e147e5456c..4d8ae0b7f814 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -402,6 +402,16 @@ static inline pgprot_t mk_pmd_sect_prot(pgprot_t prot)
 	return __pgprot((pgprot_val(prot) & ~PMD_TABLE_BIT) | PMD_TYPE_SECT);
 }
 
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+	unsigned long pfn = pte_pfn(pte);
+
+	return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+}
+
 #ifdef CONFIG_NUMA_BALANCING
 /*
  * See the comment in include/linux/pgtable.h
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cbace1c9e137..38d03406f6aa 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -100,16 +100,6 @@ int pud_huge(pud_t pud)
 #endif
 }
 
-/*
- * Select all bits except the pfn
- */
-static inline pgprot_t pte_pgprot(pte_t pte)
-{
-	unsigned long pfn = pte_pfn(pte);
-
-	return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
-}
-
 static int find_num_contig(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, size_t *pgsize)
 {
-- 
2.26.2

[PATCH 4/4] arm64: Add HAVE_IOREMAP_PROT support

[Kefeng Wang]

With ioremap_prot() defination from generic ioremap, also move
pte_pgprot() from hugetlbpage.c into pgtable.h, then arm64 could
have HAVE_IOREMAP_PROT, which will enable generic_access_phys()
code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 .../features/vm/ioremap_prot/arch-support.txt          |  2 +-
 arch/arm64/Kconfig                                     |  1 +
 arch/arm64/include/asm/pgtable.h                       | 10 ++++++++++
 arch/arm64/mm/hugetlbpage.c                            | 10 ----------
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/features/vm/ioremap_prot/arch-support.txt b/Documentation/features/vm/ioremap_prot/arch-support.txt
index a6dcbe5f47b6..b39ad5d61216 100644
--- a/Documentation/features/vm/ioremap_prot/arch-support.txt
+++ b/Documentation/features/vm/ioremap_prot/arch-support.txt
@@ -9,7 +9,7 @@
     |       alpha: | TODO |
     |         arc: |  ok  |
     |         arm: | TODO |
-    |       arm64: | TODO |
+    |       arm64: |  ok  |
     |        csky: | TODO |
     |       h8300: | TODO |
     |     hexagon: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 838c45f1517b..0357708c4d32 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -188,6 +188,7 @@ config ARM64
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_GCC_PLUGINS
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS
+	select HAVE_IOREMAP_PROT
 	select HAVE_IRQ_TIME_ACCOUNTING
 	select HAVE_KVM
 	select HAVE_NMI
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 94e147e5456c..4d8ae0b7f814 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -402,6 +402,16 @@ static inline pgprot_t mk_pmd_sect_prot(pgprot_t prot)
 	return __pgprot((pgprot_val(prot) & ~PMD_TABLE_BIT) | PMD_TYPE_SECT);
 }
 
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+	unsigned long pfn = pte_pfn(pte);
+
+	return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+}
+
 #ifdef CONFIG_NUMA_BALANCING
 /*
  * See the comment in include/linux/pgtable.h
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cbace1c9e137..38d03406f6aa 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -100,16 +100,6 @@ int pud_huge(pud_t pud)
 #endif
 }
 
-/*
- * Select all bits except the pfn
- */
-static inline pgprot_t pte_pgprot(pte_t pte)
-{
-	unsigned long pfn = pte_pfn(pte);
-
-	return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
-}
-
 static int find_num_contig(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, size_t *pgsize)
 {
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Add special check hook for architecture to verify addr, size
> or prot when ioremap() or iounmap(), which will make the generic
> ioremap more useful.
> 
> ...
>
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>  #elif defined(CONFIG_GENERIC_IOREMAP)
>  #include <linux/pgtable.h>
>  
> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> +bool arch_iounmap_check(void __iomem *addr);

Pet peeve.  The word "check" is a poor one.  I gives no sense of what
the function is checking and it gives no sense of how the function's
return value relates to the thing which it checks.

Maybe it returns 0 on success and -EINVAL on failure.  Don't know!

Don't you think that better names would be io_remap_ok(),
io_remap_valid(), io_remap_allowed(), etc?


Other than that, 

Acked-by: Andrew Morton <akpm@linux-foundation.org>

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Add special check hook for architecture to verify addr, size
> or prot when ioremap() or iounmap(), which will make the generic
> ioremap more useful.
> 
> ...
>
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>  #elif defined(CONFIG_GENERIC_IOREMAP)
>  #include <linux/pgtable.h>
>  
> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> +bool arch_iounmap_check(void __iomem *addr);

Pet peeve.  The word "check" is a poor one.  I gives no sense of what
the function is checking and it gives no sense of how the function's
return value relates to the thing which it checks.

Maybe it returns 0 on success and -EINVAL on failure.  Don't know!

Don't you think that better names would be io_remap_ok(),
io_remap_valid(), io_remap_allowed(), etc?


Other than that, 

Acked-by: Andrew Morton <akpm@linux-foundation.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Show physical address in /proc/vmallocinfo.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/ioremap.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 5fe598ecd9b7..522ef899c35f 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
>  	if (!area)
>  		return NULL;
>  	vaddr = (unsigned long)area->addr;
> +	area->phys_addr = addr;
>  
>  	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
>  		free_vm_area(area);

Acked-by: Andrew Morton <akpm@linux-foundation.org>

I checked a bunch of arch-specific implementations of ioremap_prot()
and they're already doing this.  As far as I can tell, only csky and
riscv actually use this file (CONFIG_GENERIC_IOREMAP=y).  But you're
ARM(?) so I'm wondering how come you're patching it?

Someone should do s/addr/phys_addr/ in this function, like the rest of
the world (sensibly) does.

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Show physical address in /proc/vmallocinfo.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/ioremap.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 5fe598ecd9b7..522ef899c35f 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
>  	if (!area)
>  		return NULL;
>  	vaddr = (unsigned long)area->addr;
> +	area->phys_addr = addr;
>  
>  	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
>  		free_vm_area(area);

Acked-by: Andrew Morton <akpm@linux-foundation.org>

I checked a bunch of arch-specific implementations of ioremap_prot()
and they're already doing this.  As far as I can tell, only csky and
riscv actually use this file (CONFIG_GENERIC_IOREMAP=y).  But you're
ARM(?) so I'm wondering how come you're patching it?

Someone should do s/addr/phys_addr/ in this function, like the rest of
the world (sensibly) does.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:12 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Subject: [PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

doh, that's why.

Re: [PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

[Andrew Morton]

On Wed, 27 Apr 2022 20:14:12 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Subject: [PATCH 3/4] arm64: mm: Convert to GENERIC_IOREMAP

doh, that's why.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>  #elif defined(CONFIG_GENERIC_IOREMAP)
>  #include <linux/pgtable.h>
>
> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> +bool arch_iounmap_check(void __iomem *addr);
> +
>  void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
>  void iounmap(volatile void __iomem *addr);
>
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 522ef899c35f..d1117005dcc7 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -11,6 +11,16 @@
>  #include <linux/io.h>
>  #include <linux/export.h>
>
> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> +{
> +       return true;
> +}
> +
> +bool __weak arch_iounmap_check(void __iomem *addr)
> +{
> +       return true;
> +}
> +

I don't really like the weak functions. The normal way to do this in
asm-generic headers
is to have something like

#ifndef arch_ioremap_check
static inline bool arch_ioremap_check(phys_addr_t addr, size_t size,
unsigned long prot)
{
       return true;
}
#endif

and then in architectures that actually do some checking, have these
bits in asm/io.h

bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
#define arch_ioremap_check arch_ioremap_check

(or alternatively an extern declaration, if the implementation is nontrivial)

It may be worth pointing out that either way requires including
asm-generic/io.h,
which most architectures don't. This is probably fine, as only csky, riscv and
now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
that any further architectures using this symbol also have to use
asm-generic/io.h.

      Arnd

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>  #elif defined(CONFIG_GENERIC_IOREMAP)
>  #include <linux/pgtable.h>
>
> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> +bool arch_iounmap_check(void __iomem *addr);
> +
>  void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
>  void iounmap(volatile void __iomem *addr);
>
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 522ef899c35f..d1117005dcc7 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -11,6 +11,16 @@
>  #include <linux/io.h>
>  #include <linux/export.h>
>
> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> +{
> +       return true;
> +}
> +
> +bool __weak arch_iounmap_check(void __iomem *addr)
> +{
> +       return true;
> +}
> +

I don't really like the weak functions. The normal way to do this in
asm-generic headers
is to have something like

#ifndef arch_ioremap_check
static inline bool arch_ioremap_check(phys_addr_t addr, size_t size,
unsigned long prot)
{
       return true;
}
#endif

and then in architectures that actually do some checking, have these
bits in asm/io.h

bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
#define arch_ioremap_check arch_ioremap_check

(or alternatively an extern declaration, if the implementation is nontrivial)

It may be worth pointing out that either way requires including
asm-generic/io.h,
which most architectures don't. This is probably fine, as only csky, riscv and
now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
that any further architectures using this symbol also have to use
asm-generic/io.h.

      Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Andrew Morton]

On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> > @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
> >  #elif defined(CONFIG_GENERIC_IOREMAP)
> >  #include <linux/pgtable.h>
> >
> > +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> > +bool arch_iounmap_check(void __iomem *addr);
> > +
> >  void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> >  void iounmap(volatile void __iomem *addr);
> >
> > diff --git a/mm/ioremap.c b/mm/ioremap.c
> > index 522ef899c35f..d1117005dcc7 100644
> > --- a/mm/ioremap.c
> > +++ b/mm/ioremap.c
> > @@ -11,6 +11,16 @@
> >  #include <linux/io.h>
> >  #include <linux/export.h>
> >
> > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> > +{
> > +       return true;
> > +}
> > +
> > +bool __weak arch_iounmap_check(void __iomem *addr)
> > +{
> > +       return true;
> > +}
> > +
> 
> I don't really like the weak functions.

How come?  They work quite nicely here?

> The normal way to do this

Is a lot more fuss.

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Andrew Morton]

On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> > @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
> >  #elif defined(CONFIG_GENERIC_IOREMAP)
> >  #include <linux/pgtable.h>
> >
> > +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> > +bool arch_iounmap_check(void __iomem *addr);
> > +
> >  void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> >  void iounmap(volatile void __iomem *addr);
> >
> > diff --git a/mm/ioremap.c b/mm/ioremap.c
> > index 522ef899c35f..d1117005dcc7 100644
> > --- a/mm/ioremap.c
> > +++ b/mm/ioremap.c
> > @@ -11,6 +11,16 @@
> >  #include <linux/io.h>
> >  #include <linux/export.h>
> >
> > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> > +{
> > +       return true;
> > +}
> > +
> > +bool __weak arch_iounmap_check(void __iomem *addr)
> > +{
> > +       return true;
> > +}
> > +
> 
> I don't really like the weak functions.

How come?  They work quite nicely here?

> The normal way to do this

Is a lot more fuss.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Wed, Apr 27, 2022 at 8:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> > >
> > > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> > > +{
> > > +       return true;
> > > +}
> > > +
> > > +bool __weak arch_iounmap_check(void __iomem *addr)
> > > +{
> > > +       return true;
> > > +}
> > > +
> >
> > I don't really like the weak functions.
>
> How come?  They work quite nicely here?

I find them rather confusing, mostly because it is less clear whether the
fallback function is used in a given configuration, or a replacement one is
present.

This is a bigger problem in some subsystems than others, and the main
place I don't like is the drivers/pci/ subsystem. A number of the uses
there should be driver specific but happen to be implemented by
architectures instead. Maybe I'm just projecting that onto other uses,
but I definitely have a bad feeling about them here.

       Arnd

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Wed, Apr 27, 2022 at 8:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> > >
> > > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
> > > +{
> > > +       return true;
> > > +}
> > > +
> > > +bool __weak arch_iounmap_check(void __iomem *addr)
> > > +{
> > > +       return true;
> > > +}
> > > +
> >
> > I don't really like the weak functions.
>
> How come?  They work quite nicely here?

I find them rather confusing, mostly because it is less clear whether the
fallback function is used in a given configuration, or a replacement one is
present.

This is a bigger problem in some subsystems than others, and the main
place I don't like is the drivers/pci/ subsystem. A number of the uses
there should be driver specific but happen to be implemented by
architectures instead. Maybe I'm just projecting that onto other uses,
but I definitely have a bad feeling about them here.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Kefeng Wang]

On 2022/4/28 1:10, Andrew Morton wrote:
> On Wed, 27 Apr 2022 20:14:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>> Show physical address in /proc/vmallocinfo.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/ioremap.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/ioremap.c b/mm/ioremap.c
>> index 5fe598ecd9b7..522ef899c35f 100644
>> --- a/mm/ioremap.c
>> +++ b/mm/ioremap.c
>> @@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
>>   	if (!area)
>>   		return NULL;
>>   	vaddr = (unsigned long)area->addr;
>> +	area->phys_addr = addr;
>>   
>>   	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
>>   		free_vm_area(area);
> Acked-by: Andrew Morton <akpm@linux-foundation.org>
>
> I checked a bunch of arch-specific implementations of ioremap_prot()
> and they're already doing this.  As far as I can tell, only csky and
> riscv actually use this file (CONFIG_GENERIC_IOREMAP=y).  But you're
> ARM(?) so I'm wondering how come you're patching it?

Hi Andrew,

I found this via reading code when debug some other issue, meanwhile,  there

are some code duplication of ioremap between arm64 and generic ioremap, so

1) bugfix: fix the above issue and test on riscv

2) cleanup: convert arm64 to use GENERIC_IOREMAP,

3) feature: after that, enable HAVE_IOREMAP_PROT on arm64

>
> Someone should do s/addr/phys_addr/ in this function, like the rest of
> the world (sensibly) does.
I will make it in next version.
>
>
> .

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Kefeng Wang]

On 2022/4/28 1:10, Andrew Morton wrote:
> On Wed, 27 Apr 2022 20:14:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>> Show physical address in /proc/vmallocinfo.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/ioremap.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/ioremap.c b/mm/ioremap.c
>> index 5fe598ecd9b7..522ef899c35f 100644
>> --- a/mm/ioremap.c
>> +++ b/mm/ioremap.c
>> @@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
>>   	if (!area)
>>   		return NULL;
>>   	vaddr = (unsigned long)area->addr;
>> +	area->phys_addr = addr;
>>   
>>   	if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
>>   		free_vm_area(area);
> Acked-by: Andrew Morton <akpm@linux-foundation.org>
>
> I checked a bunch of arch-specific implementations of ioremap_prot()
> and they're already doing this.  As far as I can tell, only csky and
> riscv actually use this file (CONFIG_GENERIC_IOREMAP=y).  But you're
> ARM(?) so I'm wondering how come you're patching it?

Hi Andrew,

I found this via reading code when debug some other issue, meanwhile,  there

are some code duplication of ioremap between arm64 and generic ioremap, so

1) bugfix: fix the above issue and test on riscv

2) cleanup: convert arm64 to use GENERIC_IOREMAP,

3) feature: after that, enable HAVE_IOREMAP_PROT on arm64

>
> Someone should do s/addr/phys_addr/ in this function, like the rest of
> the world (sensibly) does.
I will make it in next version.
>
>
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

On 2022/4/28 1:04, Andrew Morton wrote:
> On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>> Add special check hook for architecture to verify addr, size
>> or prot when ioremap() or iounmap(), which will make the generic
>> ioremap more useful.
>>
>> ...
>>
>> --- a/include/asm-generic/io.h
>> +++ b/include/asm-generic/io.h
>> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>>   #elif defined(CONFIG_GENERIC_IOREMAP)
>>   #include <linux/pgtable.h>
>>   
>> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
>> +bool arch_iounmap_check(void __iomem *addr);
> Pet peeve.  The word "check" is a poor one.  I gives no sense of what
> the function is checking and it gives no sense of how the function's
> return value relates to the thing which it checks.
>
> Maybe it returns 0 on success and -EINVAL on failure.  Don't know!
>
> Don't you think that better names would be io_remap_ok(),
> io_remap_valid(), io_remap_allowed(), etc?

Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool

for now if there is no special requirements.

>
>
> Other than that,
>
> Acked-by: Andrew Morton <akpm@linux-foundation.org>
Thanks.
> .

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

On 2022/4/28 1:04, Andrew Morton wrote:
> On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>> Add special check hook for architecture to verify addr, size
>> or prot when ioremap() or iounmap(), which will make the generic
>> ioremap more useful.
>>
>> ...
>>
>> --- a/include/asm-generic/io.h
>> +++ b/include/asm-generic/io.h
>> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>>   #elif defined(CONFIG_GENERIC_IOREMAP)
>>   #include <linux/pgtable.h>
>>   
>> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
>> +bool arch_iounmap_check(void __iomem *addr);
> Pet peeve.  The word "check" is a poor one.  I gives no sense of what
> the function is checking and it gives no sense of how the function's
> return value relates to the thing which it checks.
>
> Maybe it returns 0 on success and -EINVAL on failure.  Don't know!
>
> Don't you think that better names would be io_remap_ok(),
> io_remap_valid(), io_remap_allowed(), etc?

Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool

for now if there is no special requirements.

>
>
> Other than that,
>
> Acked-by: Andrew Morton <akpm@linux-foundation.org>
Thanks.
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

On 2022/4/28 2:20, Arnd Bergmann wrote:
> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>>   #elif defined(CONFIG_GENERIC_IOREMAP)
>>   #include <linux/pgtable.h>
>>
>> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
>> +bool arch_iounmap_check(void __iomem *addr);
>> +
>>   void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
>>   void iounmap(volatile void __iomem *addr);
>>
>> diff --git a/mm/ioremap.c b/mm/ioremap.c
>> index 522ef899c35f..d1117005dcc7 100644
>> --- a/mm/ioremap.c
>> +++ b/mm/ioremap.c
>> @@ -11,6 +11,16 @@
>>   #include <linux/io.h>
>>   #include <linux/export.h>
>>
>> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
>> +{
>> +       return true;
>> +}
>> +
>> +bool __weak arch_iounmap_check(void __iomem *addr)
>> +{
>> +       return true;
>> +}
>> +
> I don't really like the weak functions. The normal way to do this in
> asm-generic headers
> is to have something like
>
> #ifndef arch_ioremap_check
> static inline bool arch_ioremap_check(phys_addr_t addr, size_t size,
> unsigned long prot)
> {
>         return true;
> }
> #endif
>
> and then in architectures that actually do some checking, have these
> bits in asm/io.h
>
> bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> #define arch_ioremap_check arch_ioremap_check
Ok, I could use this way, and keep consistent with others definitions in 
asm/io.h
> (or alternatively an extern declaration, if the implementation is nontrivial)
>
> It may be worth pointing out that either way requires including
> asm-generic/io.h,
> which most architectures don't. This is probably fine, as only csky, riscv and
> now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
> that any further architectures using this symbol also have to use
> asm-generic/io.h.

It looks the arch is already include it,

$ git grep "asm-generic/io.h" arch/

arch/arc/include/asm/io.h:#include <asm-generic/io.h>
arch/arm/include/asm/io.h:#include <asm-generic/io.h>
arch/arm64/include/asm/io.h:#include <asm-generic/io.h>
arch/csky/include/asm/io.h:#include <asm-generic/io.h>
arch/h8300/include/asm/io.h:#include <asm-generic/io.h>
arch/ia64/include/asm/io.h:#include <asm-generic/io.h>
arch/m68k/include/asm/io.h:#include <asm-generic/io.h>
arch/m68k/include/asm/io_no.h: * that behavior here first before we 
include asm-generic/io.h.
arch/microblaze/include/asm/io.h:#include <asm-generic/io.h>
arch/nios2/include/asm/io.h:#include <asm-generic/io.h>
arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
arch/powerpc/include/asm/io.h:#include <asm-generic/io.h>
arch/riscv/include/asm/io.h:#include <asm-generic/io.h>
arch/s390/include/asm/io.h:#include <asm-generic/io.h>
arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h>
arch/um/include/asm/io.h:#include <asm-generic/io.h>
arch/x86/include/asm/io.h:#include <asm-generic/io.h>
arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>

>        Arnd
>
> .

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Kefeng Wang]

On 2022/4/28 2:20, Arnd Bergmann wrote:
> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr)
>>   #elif defined(CONFIG_GENERIC_IOREMAP)
>>   #include <linux/pgtable.h>
>>
>> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
>> +bool arch_iounmap_check(void __iomem *addr);
>> +
>>   void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
>>   void iounmap(volatile void __iomem *addr);
>>
>> diff --git a/mm/ioremap.c b/mm/ioremap.c
>> index 522ef899c35f..d1117005dcc7 100644
>> --- a/mm/ioremap.c
>> +++ b/mm/ioremap.c
>> @@ -11,6 +11,16 @@
>>   #include <linux/io.h>
>>   #include <linux/export.h>
>>
>> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot)
>> +{
>> +       return true;
>> +}
>> +
>> +bool __weak arch_iounmap_check(void __iomem *addr)
>> +{
>> +       return true;
>> +}
>> +
> I don't really like the weak functions. The normal way to do this in
> asm-generic headers
> is to have something like
>
> #ifndef arch_ioremap_check
> static inline bool arch_ioremap_check(phys_addr_t addr, size_t size,
> unsigned long prot)
> {
>         return true;
> }
> #endif
>
> and then in architectures that actually do some checking, have these
> bits in asm/io.h
>
> bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> #define arch_ioremap_check arch_ioremap_check
Ok, I could use this way, and keep consistent with others definitions in 
asm/io.h
> (or alternatively an extern declaration, if the implementation is nontrivial)
>
> It may be worth pointing out that either way requires including
> asm-generic/io.h,
> which most architectures don't. This is probably fine, as only csky, riscv and
> now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
> that any further architectures using this symbol also have to use
> asm-generic/io.h.

It looks the arch is already include it,

$ git grep "asm-generic/io.h" arch/

arch/arc/include/asm/io.h:#include <asm-generic/io.h>
arch/arm/include/asm/io.h:#include <asm-generic/io.h>
arch/arm64/include/asm/io.h:#include <asm-generic/io.h>
arch/csky/include/asm/io.h:#include <asm-generic/io.h>
arch/h8300/include/asm/io.h:#include <asm-generic/io.h>
arch/ia64/include/asm/io.h:#include <asm-generic/io.h>
arch/m68k/include/asm/io.h:#include <asm-generic/io.h>
arch/m68k/include/asm/io_no.h: * that behavior here first before we 
include asm-generic/io.h.
arch/microblaze/include/asm/io.h:#include <asm-generic/io.h>
arch/nios2/include/asm/io.h:#include <asm-generic/io.h>
arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
arch/powerpc/include/asm/io.h:#include <asm-generic/io.h>
arch/riscv/include/asm/io.h:#include <asm-generic/io.h>
arch/s390/include/asm/io.h:#include <asm-generic/io.h>
arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h>
arch/um/include/asm/io.h:#include <asm-generic/io.h>
arch/x86/include/asm/io.h:#include <asm-generic/io.h>
arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>

>        Arnd
>
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Thu, Apr 28, 2022 at 8:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> On 2022/4/28 2:20, Arnd Bergmann wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >
> > bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> > #define arch_ioremap_check arch_ioremap_check
> Ok, I could use this way, and keep consistent with others definitions in
> asm/io.h
> > (or alternatively an extern declaration, if the implementation is nontrivial)
> >
> > It may be worth pointing out that either way requires including
> > asm-generic/io.h,
> > which most architectures don't. This is probably fine, as only csky, riscv and
> > now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
> > that any further architectures using this symbol also have to use
> > asm-generic/io.h.
>
> It looks the arch is already include it,
>
> $ git grep "asm-generic/io.h" arch/
>
> arch/arc/include/asm/io.h:#include <asm-generic/io.h>
> arch/arm/include/asm/io.h:#include <asm-generic/io.h>
> arch/arm64/include/asm/io.h:#include <asm-generic/io.h>
> arch/csky/include/asm/io.h:#include <asm-generic/io.h>
> arch/h8300/include/asm/io.h:#include <asm-generic/io.h>
> arch/ia64/include/asm/io.h:#include <asm-generic/io.h>
> arch/m68k/include/asm/io.h:#include <asm-generic/io.h>
> arch/m68k/include/asm/io_no.h: * that behavior here first before we
> include asm-generic/io.h.
> arch/microblaze/include/asm/io.h:#include <asm-generic/io.h>
> arch/nios2/include/asm/io.h:#include <asm-generic/io.h>
> arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
> arch/powerpc/include/asm/io.h:#include <asm-generic/io.h>
> arch/riscv/include/asm/io.h:#include <asm-generic/io.h>
> arch/s390/include/asm/io.h:#include <asm-generic/io.h>
> arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h>
> arch/um/include/asm/io.h:#include <asm-generic/io.h>
> arch/x86/include/asm/io.h:#include <asm-generic/io.h>
> arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>

Right, it's mostly the older architectures that never started using
asm-generic/io.h:

$ git grep -L asm-generic/io.h arch/*/include/asm/io.h
arch/alpha/include/asm/io.h
arch/hexagon/include/asm/io.h
arch/mips/include/asm/io.h
arch/parisc/include/asm/io.h
arch/sh/include/asm/io.h
arch/sparc/include/asm/io.h # it is used on sparc32

That's actually less than I expected, and most of these are not
seeing a lot of upstream work any more.

        Arnd

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Arnd Bergmann]

On Thu, Apr 28, 2022 at 8:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> On 2022/4/28 2:20, Arnd Bergmann wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >
> > bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot);
> > #define arch_ioremap_check arch_ioremap_check
> Ok, I could use this way, and keep consistent with others definitions in
> asm/io.h
> > (or alternatively an extern declaration, if the implementation is nontrivial)
> >
> > It may be worth pointing out that either way requires including
> > asm-generic/io.h,
> > which most architectures don't. This is probably fine, as only csky, riscv and
> > now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require
> > that any further architectures using this symbol also have to use
> > asm-generic/io.h.
>
> It looks the arch is already include it,
>
> $ git grep "asm-generic/io.h" arch/
>
> arch/arc/include/asm/io.h:#include <asm-generic/io.h>
> arch/arm/include/asm/io.h:#include <asm-generic/io.h>
> arch/arm64/include/asm/io.h:#include <asm-generic/io.h>
> arch/csky/include/asm/io.h:#include <asm-generic/io.h>
> arch/h8300/include/asm/io.h:#include <asm-generic/io.h>
> arch/ia64/include/asm/io.h:#include <asm-generic/io.h>
> arch/m68k/include/asm/io.h:#include <asm-generic/io.h>
> arch/m68k/include/asm/io_no.h: * that behavior here first before we
> include asm-generic/io.h.
> arch/microblaze/include/asm/io.h:#include <asm-generic/io.h>
> arch/nios2/include/asm/io.h:#include <asm-generic/io.h>
> arch/openrisc/include/asm/io.h:#include <asm-generic/io.h>
> arch/powerpc/include/asm/io.h:#include <asm-generic/io.h>
> arch/riscv/include/asm/io.h:#include <asm-generic/io.h>
> arch/s390/include/asm/io.h:#include <asm-generic/io.h>
> arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h>
> arch/um/include/asm/io.h:#include <asm-generic/io.h>
> arch/x86/include/asm/io.h:#include <asm-generic/io.h>
> arch/xtensa/include/asm/io.h:#include <asm-generic/io.h>

Right, it's mostly the older architectures that never started using
asm-generic/io.h:

$ git grep -L asm-generic/io.h arch/*/include/asm/io.h
arch/alpha/include/asm/io.h
arch/hexagon/include/asm/io.h
arch/mips/include/asm/io.h
arch/parisc/include/asm/io.h
arch/sh/include/asm/io.h
arch/sparc/include/asm/io.h # it is used on sparc32

That's actually less than I expected, and most of these are not
seeing a lot of upstream work any more.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Will Deacon]

On Wed, Apr 27, 2022 at 08:14:09PM +0800, Kefeng Wang wrote:
> Let's arm64 use GENERIC_IOREMAP to cleanup code, and
> support ioremap_prot()/HAVE_IOREMAP_PROT, which could
> enable generic_access_phys().
> 
> Kefeng Wang (4):
>   mm: ioremap: Setup phys_addr of struct vm_struct
>   mm: ioremap: Add arch_ioremap/iounmap_check()
>   arm64: mm: Convert to GENERIC_IOREMAP
>   arm64: Add HAVE_IOREMAP_PROT support
> 
>  .../features/vm/ioremap_prot/arch-support.txt |  2 +-
>  arch/arm64/Kconfig                            |  2 +
>  arch/arm64/include/asm/io.h                   | 14 +--
>  arch/arm64/include/asm/pgtable.h              | 10 +++
>  arch/arm64/kernel/acpi.c                      |  2 +-
>  arch/arm64/mm/hugetlbpage.c                   | 10 ---
>  arch/arm64/mm/ioremap.c                       | 86 +++----------------
>  include/asm-generic/io.h                      |  3 +
>  mm/ioremap.c                                  | 21 ++++-
>  9 files changed, 56 insertions(+), 94 deletions(-)

That's not a massively compelling diffstat for a cleanup, in all honesty.
I looked at generic_access_phys() to try to figure out why we would want
that on arm64, but it seems like it's related to mmap() of devices in
userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
can you justify why this is something worth doing?

Will

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Will Deacon]

On Wed, Apr 27, 2022 at 08:14:09PM +0800, Kefeng Wang wrote:
> Let's arm64 use GENERIC_IOREMAP to cleanup code, and
> support ioremap_prot()/HAVE_IOREMAP_PROT, which could
> enable generic_access_phys().
> 
> Kefeng Wang (4):
>   mm: ioremap: Setup phys_addr of struct vm_struct
>   mm: ioremap: Add arch_ioremap/iounmap_check()
>   arm64: mm: Convert to GENERIC_IOREMAP
>   arm64: Add HAVE_IOREMAP_PROT support
> 
>  .../features/vm/ioremap_prot/arch-support.txt |  2 +-
>  arch/arm64/Kconfig                            |  2 +
>  arch/arm64/include/asm/io.h                   | 14 +--
>  arch/arm64/include/asm/pgtable.h              | 10 +++
>  arch/arm64/kernel/acpi.c                      |  2 +-
>  arch/arm64/mm/hugetlbpage.c                   | 10 ---
>  arch/arm64/mm/ioremap.c                       | 86 +++----------------
>  include/asm-generic/io.h                      |  3 +
>  mm/ioremap.c                                  | 21 ++++-
>  9 files changed, 56 insertions(+), 94 deletions(-)

That's not a massively compelling diffstat for a cleanup, in all honesty.
I looked at generic_access_phys() to try to figure out why we would want
that on arm64, but it seems like it's related to mmap() of devices in
userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
can you justify why this is something worth doing?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Kefeng Wang]

On 2022/4/28 18:46, Will Deacon wrote:
> On Wed, Apr 27, 2022 at 08:14:09PM +0800, Kefeng Wang wrote:
>> Let's arm64 use GENERIC_IOREMAP to cleanup code, and
>> support ioremap_prot()/HAVE_IOREMAP_PROT, which could
>> enable generic_access_phys().
>>
>> Kefeng Wang (4):
>>    mm: ioremap: Setup phys_addr of struct vm_struct
>>    mm: ioremap: Add arch_ioremap/iounmap_check()
>>    arm64: mm: Convert to GENERIC_IOREMAP
>>    arm64: Add HAVE_IOREMAP_PROT support
>>
>>   .../features/vm/ioremap_prot/arch-support.txt |  2 +-
>>   arch/arm64/Kconfig                            |  2 +
>>   arch/arm64/include/asm/io.h                   | 14 +--
>>   arch/arm64/include/asm/pgtable.h              | 10 +++
>>   arch/arm64/kernel/acpi.c                      |  2 +-
>>   arch/arm64/mm/hugetlbpage.c                   | 10 ---
>>   arch/arm64/mm/ioremap.c                       | 86 +++----------------
>>   include/asm-generic/io.h                      |  3 +
>>   mm/ioremap.c                                  | 21 ++++-
>>   9 files changed, 56 insertions(+), 94 deletions(-)
> That's not a massively compelling diffstat for a cleanup, in all honesty.
> I looked at generic_access_phys() to try to figure out why we would want
> that on arm64, but it seems like it's related to mmap() of devices in
> userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
> can you justify why this is something worth doing?

The geneirc_access_phys was introduced by

   7ae8ed5053a3 use generic_access_phys for /dev/mem mappings
   28b2ee20c7cb access_process_vm device memory infrastructure

and add into uio.c  by

   7294151d0592 "uio: provide vm access to UIO_MEM_PHYS maps"

It could let user to debug(eg, gdb or ptrace) app via access_process_vm().

also some upstream drivers and out-of-tree modules could use it too
   drivers/fpga/dfl-afu-main.c:    .access = generic_access_phys,
   drivers/pci/mmap.c:     .access = generic_access_phys,

When see the ioremap_prot from generic ioremap, for better debug, I think

we could enabled it on arm64 too.

> Will
> .

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Kefeng Wang]

On 2022/4/28 18:46, Will Deacon wrote:
> On Wed, Apr 27, 2022 at 08:14:09PM +0800, Kefeng Wang wrote:
>> Let's arm64 use GENERIC_IOREMAP to cleanup code, and
>> support ioremap_prot()/HAVE_IOREMAP_PROT, which could
>> enable generic_access_phys().
>>
>> Kefeng Wang (4):
>>    mm: ioremap: Setup phys_addr of struct vm_struct
>>    mm: ioremap: Add arch_ioremap/iounmap_check()
>>    arm64: mm: Convert to GENERIC_IOREMAP
>>    arm64: Add HAVE_IOREMAP_PROT support
>>
>>   .../features/vm/ioremap_prot/arch-support.txt |  2 +-
>>   arch/arm64/Kconfig                            |  2 +
>>   arch/arm64/include/asm/io.h                   | 14 +--
>>   arch/arm64/include/asm/pgtable.h              | 10 +++
>>   arch/arm64/kernel/acpi.c                      |  2 +-
>>   arch/arm64/mm/hugetlbpage.c                   | 10 ---
>>   arch/arm64/mm/ioremap.c                       | 86 +++----------------
>>   include/asm-generic/io.h                      |  3 +
>>   mm/ioremap.c                                  | 21 ++++-
>>   9 files changed, 56 insertions(+), 94 deletions(-)
> That's not a massively compelling diffstat for a cleanup, in all honesty.
> I looked at generic_access_phys() to try to figure out why we would want
> that on arm64, but it seems like it's related to mmap() of devices in
> userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
> can you justify why this is something worth doing?

The geneirc_access_phys was introduced by

   7ae8ed5053a3 use generic_access_phys for /dev/mem mappings
   28b2ee20c7cb access_process_vm device memory infrastructure

and add into uio.c  by

   7294151d0592 "uio: provide vm access to UIO_MEM_PHYS maps"

It could let user to debug(eg, gdb or ptrace) app via access_process_vm().

also some upstream drivers and out-of-tree modules could use it too
   drivers/fpga/dfl-afu-main.c:    .access = generic_access_phys,
   drivers/pci/mmap.c:     .access = generic_access_phys,

When see the ioremap_prot from generic ioremap, for better debug, I think

we could enabled it on arm64 too.

> Will
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Christoph Hellwig]

On Wed, Apr 27, 2022 at 08:14:10PM +0800, Kefeng Wang wrote:
> Show physical address in /proc/vmallocinfo.

Looks good:

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

Re: [PATCH 1/4] mm: ioremap: Setup phys_addr of struct vm_struct

[Christoph Hellwig]

On Wed, Apr 27, 2022 at 08:14:10PM +0800, Kefeng Wang wrote:
> Show physical address in /proc/vmallocinfo.

Looks good:

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Christoph Hellwig]

On Thu, Apr 28, 2022 at 02:16:39PM +0800, Kefeng Wang wrote:
> > Pet peeve.  The word "check" is a poor one.  I gives no sense of what
> > the function is checking and it gives no sense of how the function's
> > return value relates to the thing which it checks.
> > 
> > Maybe it returns 0 on success and -EINVAL on failure.  Don't know!
> > 
> > Don't you think that better names would be io_remap_ok(),
> > io_remap_valid(), io_remap_allowed(), etc?
> 
> Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool
> 
> for now if there is no special requirements.

Actually, there are a few architectures that can successfully ioreamp
without setting up new ptes, e.g. mips.

So I think I'd name them just arch_ioremap and arch_iounmap, and
return a "void __іomem *" from arch_ioremap, where:

 - IS_ERR means return an error
 - NULL means continue to remap
 - a non-NULL, non-IS_ERR pointer is directly returned.

Re: [PATCH 2/4] mm: ioremap: Add arch_ioremap/iounmap_check()

[Christoph Hellwig]

On Thu, Apr 28, 2022 at 02:16:39PM +0800, Kefeng Wang wrote:
> > Pet peeve.  The word "check" is a poor one.  I gives no sense of what
> > the function is checking and it gives no sense of how the function's
> > return value relates to the thing which it checks.
> > 
> > Maybe it returns 0 on success and -EINVAL on failure.  Don't know!
> > 
> > Don't you think that better names would be io_remap_ok(),
> > io_remap_valid(), io_remap_allowed(), etc?
> 
> Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool
> 
> for now if there is no special requirements.

Actually, there are a few architectures that can successfully ioreamp
without setting up new ptes, e.g. mips.

So I think I'd name them just arch_ioremap and arch_iounmap, and
return a "void __іomem *" from arch_ioremap, where:

 - IS_ERR means return an error
 - NULL means continue to remap
 - a non-NULL, non-IS_ERR pointer is directly returned.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Christoph Hellwig]

On Thu, Apr 28, 2022 at 11:46:12AM +0100, Will Deacon wrote:
> That's not a massively compelling diffstat for a cleanup, in all honesty.
> I looked at generic_access_phys() to try to figure out why we would want
> that on arm64, but it seems like it's related to mmap() of devices in
> userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
> can you justify why this is something worth doing?

While I don't care much about ioremap_prot I'd really love to eventually
convert everyone to the common ioremap code as there really is no
point in duplicating it in the architectures.

Re: [PATCH 0/4] arm64: Cleanup ioremap() and support ioremap_prot()

[Christoph Hellwig]

On Thu, Apr 28, 2022 at 11:46:12AM +0100, Will Deacon wrote:
> That's not a massively compelling diffstat for a cleanup, in all honesty.
> I looked at generic_access_phys() to try to figure out why we would want
> that on arm64, but it seems like it's related to mmap() of devices in
> userspace. Bearing in mind that CONFIG_STRICT_DEVMEM=y by default, please
> can you justify why this is something worth doing?

While I don't care much about ioremap_prot I'd really love to eventually
convert everyone to the common ioremap code as there really is no
point in duplicating it in the architectures.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel