Le 19/06/2019 à 05:59, Nicholas Piggin a écrit :
Christophe Leroy's on June 11, 2019 4:46 pm:
Le 10/06/2019 à 05:08, Nicholas Piggin a écrit :
[snip]
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.
I can understand the benefit in some situations but here I just can't.
And code duplication should be avoided as much as possible as it makes
code maintenance more difficult.
Here you have:
+static int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned
long size, pgprot_t prot, int nid)
+{
+ unsigned long i;
+
+ 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;
+}
You now create a new one in another file, that is almost identical:
+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);
+
+ 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;
+}
Then you have to make the original one __weak.
Sorry I'm still having difficulties understanding what the benefit is.
radix_enabled() is defined for every platforms so could just add the
following on top of the existing ioremap_range() and voila.
+ if (radix_enabled())
+ return radix__ioremap_range(ea, pa, size, prot, nid);
And with that you wouldn't have the __weak stuff to handle.
-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?
Yes you are right, nobody should assume that, but ...
But I think if the fonctions were really different, the preferred
alternative would be to move the original function into a file dedicated
to nohash64.
Christophe
