Christophe Leroy's on June 11, 2019 4:46 pm:

Le 10/06/2019 à 05:08, Nicholas Piggin a écrit :

quoted

Radix can use ioremap_page_range for ioremap, after slab is available.
This makes it possible to enable huge ioremap mapping support.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
  arch/powerpc/include/asm/book3s/64/radix.h |  3 +++
  arch/powerpc/mm/book3s64/pgtable.c         | 21 +++++++++++++++++++++
  arch/powerpc/mm/book3s64/radix_pgtable.c   | 21 +++++++++++++++++++++
  arch/powerpc/mm/pgtable_64.c               |  2 +-
  4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
index 574eca33f893..e04a839cb5b9 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h

@@ -266,6 +266,9 @@ extern void radix__vmemmap_remove_mapping(unsigned long start,
  extern int radix__map_kernel_page(unsigned long ea, unsigned long pa,
  				 pgprot_t flags, unsigned int psz);
  
+extern int radix__ioremap_range(unsigned long ea, phys_addr_t pa,
+				unsigned long size, pgprot_t prot, int nid);
+

'extern' is pointless here, and checkpatch will cry.

quoted

  static inline unsigned long radix__get_tree_size(void)
  {
  	unsigned long rts_field;

diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index ff98b663c83e..953850a602f7 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c

@@ -450,3 +450,24 @@ int pmd_move_must_withdraw(struct spinlock *new_pmd_ptl,
  
  	return true;
  }
+
+int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
+{
+	unsigned long i;
+
+	if (radix_enabled())
+		return radix__ioremap_range(ea, pa, size, prot, nid);

This function looks pretty similar to the one in the previous patch.
Since radix_enabled() is available and return false for all other 
subarches, I think the above could go in the generic ioremap_range(), 
you'll only need to move the function declaration in a common file, for 
instance asm/io.h

quoted

+
+	for (i = 0; i < size; i += PAGE_SIZE) {
+		int err = map_kernel_page(ea + i, pa + i, prot);
+		if (err) {
+			if (slab_is_available())
+				unmap_kernel_range(ea, size);
+			else
+				WARN_ON_ONCE(1); /* Should clean up */
+			return err;
+		}
+	}
+
+	return 0;
+}

diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index c9bcf428dd2b..db993bc1aef3 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c

@@ -11,6 +11,7 @@
  
  #define pr_fmt(fmt) "radix-mmu: " fmt
  
+#include <linux/io.h>
  #include <linux/kernel.h>
  #include <linux/sched/mm.h>
  #include <linux/memblock.h>

@@ -1122,3 +1123,23 @@ void radix__ptep_modify_prot_commit(struct vm_area_struct *vma,
  
  	set_pte_at(mm, addr, ptep, pte);
  }
+
+int radix__ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size,
+			pgprot_t prot, int nid)
+{
+	if (likely(slab_is_available())) {
+		int err = ioremap_page_range(ea, ea + size, pa, prot);
+		if (err)
+			unmap_kernel_range(ea, size);
+		return err;
+	} else {
+		unsigned long i;
+
+		for (i = 0; i < size; i += PAGE_SIZE) {
+			int err = map_kernel_page(ea + i, pa + i, prot);
+			if (WARN_ON_ONCE(err)) /* Should clean up */
+				return err;
+		}

Same loop again.

What about not doing a radix specific function and just putting 
something like below in the core ioremap_range() function ?

	if (likely(slab_is_available()) && radix_enabled()) {
		int err = ioremap_page_range(ea, ea + size, pa, prot);

		if (err)
			unmap_kernel_range(ea, size);
		return err;
	}

Because I'm pretty sure will more and more use ioremap_page_range().

Well I agree the duplication is not so nice, but it's convenient
to see what is going on for each MMU type.

There is a significant amount of churn that needs to be done in
this layer so I prefer to make it a bit simpler despite duplication.

I would like to remove the early ioremap or make it into its own
function. Re-implement map_kernel_page with ioremap_page_range,
allow page tables that don't use slab to avoid the early check,
unbolt the hptes mapped in early boot, etc.

I just wanted to escape out the 64s and hash/radix implementations
completely until that settles.

quoted

-static int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
+int __weak ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)

Hum. Weak functions remain in unused in vmlinux unless 
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is selected.

Also, they are some how dangerous because people might change them 
without seeing that it is overridden for some particular configuration.

Well you shouldn't assume that when you see a weak function, but
what's the preferred alternative? A config option?

Thanks,
Nick