Dynamic FTRACE for PA-RISC

PATCH Superseded Sven Schnelle 2 participants 14 messages May 27, 2019 – May 29, 2019

Subsystems (3)

Generic include/asm header files PA-RISC architecture ftrace

lore

Share & tools

lore

Sven Schnelle

author

2019-05-27 19:05 UTC #0 lore raw

cover Dynamic FTRACE for PA-RISC

Hi List,

this series adds support for dynamic ftrace. See the commit message of the patch for a explanation how it works. For testing, you need a patched version of gcc because the current version has a bug which misplaces the function label. John David Anglin can provide a patch which fixes this issue.

Sven Schnelle (6):
parisc: add support for patching multiple words
parisc: add spinlock to patch function
parisc: add WARN_ON() to clear_fixmap
parisc: use pr_debug() in kernel/module.c compiler.h: add CC_USING_PATCHABLE_FUNCTION_ENTRY
parisc: add dynamic ftrace

 arch/parisc/Kconfig               |   2 +
 arch/parisc/Makefile              |  18 +++++
 arch/parisc/include/asm/ftrace.h  |  15 +++-
 arch/parisc/include/asm/patch.h   |   4 +-
 arch/parisc/kernel/Makefile       |   9 ++-
 arch/parisc/kernel/entry.S        |  64 +++++++++++++++
 arch/parisc/kernel/ftrace.c       | 129 +++++++++++++++++++++++++++---
 arch/parisc/kernel/module.c       |  64 +++++++++------
 arch/parisc/kernel/module.lds     |   7 ++
 arch/parisc/kernel/patch.c        |  88 ++++++++++++++++----
 arch/parisc/kernel/vmlinux.lds.S  |   2 +
 arch/parisc/mm/fixmap.c           |   7 +-
 include/asm-generic/vmlinux.lds.h |   7 ++
 include/linux/compiler_types.h    |   2 +
 14 files changed, 358 insertions(+), 60 deletions(-)
 create mode 100644 arch/parisc/kernel/module.lds

Signature · 1 line

2.20.1

Sven Schnelle

author

2019-05-27 19:05 UTC #1 lore raw

PATCH 1/6 parisc: add support for patching multiple words

add patch_text_multiple() which allows to patch multiple text words in memory. This can be used to copy functions.

Signed-off-by Sven Schnelle <[email protected]>

---
 arch/parisc/include/asm/patch.h |  4 +-
 arch/parisc/kernel/patch.c      | 75 ++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 17 deletions(-)

arch/parisc/include/asm/patch.h +3 -1

index 685b58a13968..1156fe11249a 100644
@@ -4,8 +4,10 @@
 
 /* stop machine and patch kernel text */
 void patch_text(void *addr, unsigned int insn);
+void patch_text_multiple(void *addr, u32 *insn, int len);
 
 /* patch kernel text with machine already stopped (e.g. in kgdb) */
-void __patch_text(void *addr, unsigned int insn);
+void __patch_text(void *addr, u32 insn);
+void __patch_text_multiple(void *addr, u32 *insn, int len);
 
 #endif

arch/parisc/kernel/patch.c +59 ~~-16~~

114 lines not on this page · raw message

Signature · 1 line

2.20.1

Rolf Eike Beer

2019-05-28 08:19 UTC #2 lore raw

Re: [PATCH 1/6] parisc: add support for patching multiple words

Quoted 14 lines · Sven Schnelle

add patch_text_multiple() which allows to patch multiple text words in memory. This can be used to copy functions.
Signed-off-by Sven Schnelle <[email protected]>
---
 arch/parisc/include/asm/patch.h |  4 +-
 arch/parisc/kernel/patch.c      | 75 ++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 17 deletions(-)
arch/parisc/include/asm/patch.h +0 -0
index 685b58a13968..1156fe11249a 100644

arch/parisc/include/asm/patch.h line 11

Quoted 9 lines

arch/parisc/include/asm/patch.h +3 -1

@@ -4,8 +4,10 @@
 
 /* stop machine and patch kernel text */
 void patch_text(void *addr, unsigned int insn);
+void patch_text_multiple(void *addr, u32 *insn, int len);
 
 /* patch kernel text with machine already stopped (e.g. in kgdb) */
-void __patch_text(void *addr, unsigned int insn);
+void __patch_text(void *addr, u32 insn);
+void __patch_text_multiple(void *addr, u32 *insn, int len);

A signed length always looks suspicious to me.

Quoted 49 lines

arch/parisc/include/asm/patch.h +0 -0

 #endif

arch/parisc/kernel/patch.c +6 -3

index cdcd981278b3..eaef5515f5b6 100644
@@ -17,15 +17,18 @@
 
 struct patch {
 	void *addr;
-	unsigned int insn;
+	u32 *insn;
+	int len;
 };
 
-static void __kprobes *patch_map(void *addr, int fixmap)
-{
+static DEFINE_RAW_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap, int 
 *need_unmap)
 	unsigned long uintaddr = (uintptr_t) addr;
 	bool module = !core_kernel_text(uintaddr);
 	struct page *page;
 
+	*need_unmap = 0;
 	if (module && IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
 		page = vmalloc_to_page(addr);

arch/parisc/kernel/patch.c +1 -0

 	else if (!module && IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))

@@ -33,6 +36,7 @@ static void __kprobes *patch_map(void *addr, int 
 fixmap)
 	else
 		return addr;
 
+	*need_unmap = 1;
 	set_fixmap(fixmap, page_to_phys(page));

arch/parisc/kernel/patch.c +7 -1

 	return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));

@@ -43,34 +47,73 @@ static void __kprobes patch_unmap(int fixmap)
 	clear_fixmap(fixmap);
 }
 
-void __kprobes __patch_text(void *addr, unsigned int insn)
+void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len)
+{
+	unsigned long start = (unsigned long)addr;
+	unsigned long end = (unsigned long)addr + len;
+	u32 *p, *fixmap;
+	int mapped;
+

arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 66

Quoted 10 lines

arch/parisc/kernel/patch.c +10 -0

+	/* Make sure we don't have any aliases in cache */
+	flush_kernel_vmap_range(addr, len);
+	flush_icache_range(start, end);
+
+	p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+
+	while (len > 0) {
+		*p++ = *insn++;
+		addr += 4;
+		len -= sizeof(u32);

I wonder if this can't just use memcpy inside the pages? If not there should be a comment describing what's going on here.

Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, but why do they use entirely different wording? What do we need "addr" for anyway, one could just look at "p" which would cross a page boundary at the same time, no?

Quoted 41 lines

arch/parisc/kernel/patch.c +22 ~~-12~~

+		if (len && !((unsigned long)addr & ~PAGE_MASK)) {
+			/*
+			 * We're crossing a page boundary, so
+			 * need to remap
+			 */
+			flush_kernel_vmap_range((void *)fixmap,
+						(p-fixmap) * sizeof(*p));
+			if (mapped)
+				patch_unmap(FIX_TEXT_POKE0);
+			p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+		}
+	}
+
+	flush_kernel_vmap_range((void *)fixmap, (p-fixmap) * sizeof(*p));
+	if (mapped)
+		patch_unmap(FIX_TEXT_POKE0);
+	flush_icache_range(start, end);
+}
+
+void __kprobes __patch_text(void *addr, u32 insn)
 {
-	void *waddr = addr;
-	int size;
-
-	waddr = patch_map(addr, FIX_TEXT_POKE0);
-	*(u32 *)waddr = insn;
-	size = sizeof(u32);
-	flush_kernel_vmap_range(waddr, size);
-	patch_unmap(FIX_TEXT_POKE0);
-	flush_icache_range((uintptr_t)(addr),
-			   (uintptr_t)(addr) + size);
+	__patch_text_multiple(addr, &insn, sizeof(insn));
 }
 
 static int __kprobes patch_text_stop_machine(void *data)
 {
 	struct patch *patch = data;
 
-	__patch_text(patch->addr, patch->insn);
-
+	__patch_text_multiple(patch->addr, patch->insn, patch->len);

arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 105

Quoted 10 lines

arch/parisc/kernel/patch.c +5 -0

 	return 0;
 }
 
 void __kprobes patch_text(void *addr, unsigned int insn)
 {
+	struct patch patch = {
+		.addr = addr,
+		.insn = &insn,
+		.len = 4
+	};

sizeof(insn)? I don't know if this makes it more readable, I personally tend to understand where the number is coming from faster if it's written this way.

arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 119

Quoted 13 lines

arch/parisc/kernel/patch.c +7 -0

+	stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL);
+}
+
+void __kprobes patch_text_multiple(void *addr, u32 *insn, int len)
+{
+
 	struct patch patch = {
 		.addr = addr,
 		.insn = insn,
+		.len = len
 	};
 
 	stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL);

Eike

Sven Schnelle

author

2019-05-29 17:49 UTC #3 lore raw

Re: [PATCH 1/6] parisc: add support for patching multiple words

Hi,

Quoted 7 lines · Rolf Eike Beer

arch/parisc/include/asm/patch.h line 11
Quoted 4 lines · Sven Schnelle
add patch_text_multiple() which allows to patch multiple text words in memory. This can be used to copy functions.
arch/parisc/include/asm/patch.h +2 -0
+void __patch_text(void *addr, u32 insn);
+void __patch_text_multiple(void *addr, u32 *insn, int len);
A signed length always looks suspicious to me.

Agreed. Will change.

Quoted 8 lines

arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 66
Quoted 6 lines
arch/parisc/kernel/patch.c +6 -0
+	p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+
+	while (len > 0) {
+		*p++ = *insn++;
+		addr += 4;
+		len -= sizeof(u32);
I wonder if this can't just use memcpy inside the pages?

I think using memcpy here makes things just more complicated and harder to read. We would need to extract the amount of bytes to copy, and call memcpy multiple times. As this code is not performance critical and usually only copies only a few bytes i doubt that it's worth the effort.

Quoted 1 line

If not there should be a comment describing what's going on here.

Is it that complicated? It's just a copy loop like in every C beginner book, the only things that makes things more complicated is the need to remap when crossing a page.

Quoted 4 lines

Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, but why do they use entirely different wording? What do we need "addr" for anyway, one could just look at "p" which would cross a page boundary at the same time, no?

You can't, because of the patch_map() call below which updates the fixed mapping. That call needs the real virtual address, while *p holds the virtual address of the fixed mapping for patching.

Quoted 5 lines

arch/parisc/kernel/patch.c +5 -0

+		if (len && !((unsigned long)addr & ~PAGE_MASK)) {
+			/*
+			 * We're crossing a page boundary, so
+			 * need to remap
+			 */

Quoted 8 lines

arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 79
Quoted 8 lines
arch/parisc/kernel/patch.c +8 -0
+			flush_kernel_vmap_range((void *)fixmap,
+						(p-fixmap) * sizeof(*p));
+			if (mapped)
+				patch_unmap(FIX_TEXT_POKE0);
+			p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+		}
+	}
+

Regards
Sven

Rolf Eike Beer

2019-05-29 17:59 UTC #4 lore raw

Re: [PATCH 1/6] parisc: add support for patching multiple words

Quoted 16 lines · Sven Schnelle

Quoted 9 lines · Rolf Eike Beer
arch/parisc/kernel/patch.c static void __kprobes patch_unmap(int fixmap) · line 66
Quoted 6 lines · Sven Schnelle
arch/parisc/kernel/patch.c +6 -0
+	p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+
+	while (len > 0) {
+		*p++ = *insn++;
+		addr += 4;
+		len -= sizeof(u32);
I wonder if this can't just use memcpy inside the pages?
I think using memcpy here makes things just more complicated and harder to read. We would need to extract the amount of bytes to copy, and call memcpy multiple times. As this code is not performance critical and usually only copies only a few bytes i doubt that it's worth the effort.

Quoted 5 lines

Quoted 1 line
If not there should be a comment describing what's going on here.
Is it that complicated? It's just a copy loop like in every C beginner book, the only things that makes things more complicated is the need to remap when crossing a page.

The copy loop not. But things like "why are you doing it backwards" come to mind. Be careful when you change the length to unsigned, your loop will not work this way anymore afterwards.

Quoted 9 lines

Quoted 5 lines
Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, but why do they use entirely different wording? What do we need "addr" for anyway, one could just look at "p" which would cross a page boundary at the
same time, no?
You can't, because of the patch_map() call below which updates the fixed mapping. That call needs the real virtual address, while *p holds the virtual address of the fixed mapping for patching.

Can that remapping really place it at a non-zero offset regarding to the underlying page? That it moves the page descriptor around is normal, but it will keep the low order bits intact, so the page boundary crossing will be still the same, no?

Eike

Sven Schnelle

author

2019-05-29 18:18 UTC #5 lore raw

Re: [PATCH 1/6] parisc: add support for patching multiple words

Quoted 8 lines · Rolf Eike Beer

Quoted 7 lines · Sven Schnelle
Quoted 5 lines · Rolf Eike Beer
Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, but why do they use entirely different wording? What do we need "addr" for anyway, one could just look at "p" which would cross a page boundary at the
same time, no?

Quoted 8 lines

Quoted 3 lines
You can't, because of the patch_map() call below which updates the fixed mapping. That call needs the real virtual address, while *p holds the virtual address of the fixed mapping for patching.
Can that remapping really place it at a non-zero offset regarding to the underlying page? That it moves the page descriptor around is normal, but it will keep the low order bits intact, so the page boundary crossing will be still the same, no?

For the page crossing check it doesn't make a difference whether you check p or addr, but for the patch_map() you can only use addr. So we have to update both variables.

Regards
Sven

Sven Schnelle

author

2019-05-27 19:05 UTC #6 lore raw

PATCH 2/6 parisc: add spinlock to patch function

In reply to Sven Schnelle

If multiple CPUs are patching code we need the spinlock to protect against parallel fixmap maps/unmap calls.

Signed-off-by Sven Schnelle <[email protected]>

---
 arch/parisc/kernel/patch.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

arch/parisc/kernel/patch.c +19 -6

index eaef5515f5b6..8ed8e7191e8d 100644
@@ -23,7 +23,9 @@ struct patch {
 
 static DEFINE_RAW_SPINLOCK(patch_lock);
 
-static void __kprobes *patch_map(void *addr, int fixmap, int *need_unmap)
+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags,
+				 int *need_unmap)
+{
 	unsigned long uintaddr = (uintptr_t) addr;
 	bool module = !core_kernel_text(uintaddr);
 	struct page *page;

@@ -38,19 +40,29 @@ static void __kprobes *patch_map(void *addr, int fixmap, int *need_unmap)
 
 	*need_unmap = 1;
 	set_fixmap(fixmap, page_to_phys(page));
+	if (flags)
+		raw_spin_lock_irqsave(&patch_lock, *flags);
+	else
+		__acquire(&patch_lock);
 
 	return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
 }
 
-static void __kprobes patch_unmap(int fixmap)
+static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
 {
 	clear_fixmap(fixmap);
+
+	if (flags)
+		raw_spin_unlock_irqrestore(&patch_lock, *flags);
+	else
+		__release(&patch_lock);
 }
 
 void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len)
 {
 	unsigned long start = (unsigned long)addr;
 	unsigned long end = (unsigned long)addr + len;
+	unsigned long flags;
 	u32 *p, *fixmap;
 	int mapped;

@@ -58,7 +70,7 @@ void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len)
 	flush_kernel_vmap_range(addr, len);
 	flush_icache_range(start, end);
 
-	p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+	p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &flags, &mapped);
 
 	while (len > 0) {
 		*p++ = *insn++;

@@ -72,14 +84,15 @@ void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len)
 			flush_kernel_vmap_range((void *)fixmap,
 						(p-fixmap) * sizeof(*p));
 			if (mapped)
-				patch_unmap(FIX_TEXT_POKE0);
-			p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
+				patch_unmap(FIX_TEXT_POKE0, &flags);
+			p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &flags,
+						&mapped);
 		}
 	}
 
 	flush_kernel_vmap_range((void *)fixmap, (p-fixmap) * sizeof(*p));
 	if (mapped)
-		patch_unmap(FIX_TEXT_POKE0);
+		patch_unmap(FIX_TEXT_POKE0, &flags);
 	flush_icache_range(start, end);
 }

Signature · 1 line

2.20.1

Sven Schnelle

author

2019-05-27 19:05 UTC #7 lore raw

PATCH 3/6 parisc: add WARN_ON() to clear_fixmap

In reply to Sven Schnelle

Calling clear_fixmap() on an already cleared fixed mapping is a bad thing to do. Add a WARN_ON() to catch such issues.

Signed-off-by Sven Schnelle <[email protected]>

---
 arch/parisc/mm/fixmap.c | 3 +++
 1 file changed, 3 insertions(+)

arch/parisc/mm/fixmap.c +3 -0

index c8d41b54fb19..36321bcd75ba 100644
@@ -35,6 +35,9 @@ void clear_fixmap(enum fixed_addresses idx)
 	pmd_t *pmd = pmd_offset(pgd, vaddr);
 	pte_t *pte = pte_offset_kernel(pmd, vaddr);
 
+	if (WARN_ON(pte_none(*pte)))
+		return;
+
 	pte_clear(&init_mm, vaddr, pte);
 
 	flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);

Signature · 1 line

2.20.1

Sven Schnelle

author

2019-05-27 19:05 UTC #8 lore raw

PATCH 4/6 parisc: use pr_debug() in kernel/module.c

In reply to Sven Schnelle

Instead of using our own version, switch to the generic pr_() calls.

Signed-off-by Sven Schnelle <[email protected]>

---
 arch/parisc/kernel/module.c | 44 ++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

arch/parisc/kernel/module.c +19 ~~-25~~

index 43778420614b..3ff3b48df6e6 100644
@@ -48,9 +48,9 @@
  *	However, SEGREL32 is used only for PARISC unwind entries, and we want
  *	those entries to have an absolute address, and not just an offset.
  *
- *	The unwind table mechanism has the ability to specify an offset for 
+ *	The unwind table mechanism has the ability to specify an offset for
  *	the unwind table; however, because we split off the init functions into
- *	a different piece of memory, it is not possible to do this using a 
+ *	a different piece of memory, it is not possible to do this using a
  *	single offset. Instead, we use the above hack for now.
  */

@@ -68,12 +68,6 @@
 #include <asm/unwind.h>
 #include <asm/sections.h>
 
-#if 0
-#define DEBUGP printk
-#else
-#define DEBUGP(fmt...)
-#endif
-
 #define RELOC_REACHABLE(val, bits) \
 	(( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
 	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) ? \

@@ -315,7 +309,7 @@ unsigned int arch_mod_section_prepend(struct module *mod,
 		* sizeof(struct stub_entry);
 }
 
-#define CONST 
+#define CONST
 int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST Elf_Shdr *sechdrs,
 			      CONST char *secstrings,

@@ -401,7 +395,7 @@ static Elf64_Word get_got(struct module *me, unsigned long value, long addend)
 
 	got[i].addr = value;
  out:
-	DEBUGP("GOT ENTRY %d[%x] val %lx\n", i, i*sizeof(struct got_entry),
+	pr_debug("GOT ENTRY %d[%lx] val %lx\n", i, i*sizeof(struct got_entry),
 	       value);
 	return i * sizeof(struct got_entry);
 }

@@ -554,7 +548,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	//unsigned long dp = (unsigned long)$global$;
 	register unsigned long dp asm ("r27");
 
-	DEBUGP("Applying relocate section %u to %u\n", relsec,
+	pr_debug("Applying relocate section %u to %u\n", relsec,
 	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */

@@ -578,7 +572,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 
 #if 0
 #define r(t) ELF32_R_TYPE(rel[i].r_info)==t ? #t :
-		DEBUGP("Symbol %s loc 0x%x val 0x%x addend 0x%x: %s\n",
+		pr_debug("Symbol %s loc 0x%x val 0x%x addend 0x%x: %s\n",
 			strtab + sym->st_name,
 			(uint32_t)loc, val, addend,
 			r(R_PARISC_PLABEL32)

@@ -619,7 +613,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* See note about special handling of SEGREL32 at
 			 * the beginning of this file.
 			 */
-			*loc = fsel(val, addend); 
+			*loc = fsel(val, addend);
 			break;
 		case R_PARISC_SECREL32:
 			/* 32-bit section relative address. */

@@ -698,7 +692,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf_Addr loc0;
 	unsigned int targetsec = sechdrs[relsec].sh_info;
 
-	DEBUGP("Applying relocate section %u to %u\n", relsec,
+	pr_debug("Applying relocate section %u to %u\n", relsec,
 	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */

@@ -740,7 +734,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 		case R_PARISC_LTOFF21L:
 			/* LT-relative; left 21 bits */
 			val = get_got(me, val, addend);
-			DEBUGP("LTOFF21L Symbol %s loc %p val %lx\n",
+			pr_debug("LTOFF21L Symbol %s loc %p val %llx\n",
 			       strtab + sym->st_name,
 			       loc, val);
 			val = lrsel(val, 0);

@@ -751,14 +745,14 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* LT-relative; right 14 bits */
 			val = get_got(me, val, addend);
 			val = rrsel(val, 0);
-			DEBUGP("LTOFF14R Symbol %s loc %p val %lx\n",
+			pr_debug("LTOFF14R Symbol %s loc %p val %llx\n",
 			       strtab + sym->st_name,
 			       loc, val);
 			*loc = mask(*loc, 14) | reassemble_14(val);
 			break;
 		case R_PARISC_PCREL22F:
 			/* PC-relative; 22 bits */
-			DEBUGP("PCREL22F Symbol %s loc %p val %lx\n",
+			pr_debug("PCREL22F Symbol %s loc %p val %llx\n",
 			       strtab + sym->st_name,
 			       loc, val);
 			val += addend;

@@ -790,7 +784,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 					val = get_stub(me, val, addend, ELF_STUB_GOT,
 						       loc0, targetsec);
 			}
-			DEBUGP("STUB FOR %s loc %lx, val %lx+%lx at %lx\n", 
+			pr_debug("STUB FOR %s loc %px, val %llx+%llx at %llx\n",
 			       strtab + sym->st_name, loc, sym->st_value,
 			       addend, val);
 			val = (val - dot - 8)/4;

@@ -810,7 +804,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* See note about special handling of SEGREL32 at
 			 * the beginning of this file.
 			 */
-			*loc = fsel(val, addend); 
+			*loc = fsel(val, addend);
 			break;
 		case R_PARISC_SECREL32:
 			/* 32-bit section relative address. */

@@ -820,14 +814,14 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* 64-bit function address */
 			if(in_local(me, (void *)(val + addend))) {
 				*loc64 = get_fdesc(me, val+addend);
-				DEBUGP("FDESC for %s at %p points to %lx\n",
+				pr_debug("FDESC for %s at %llx points to %llx\n",
 				       strtab + sym->st_name, *loc64,
 				       ((Elf_Fdesc *)*loc64)->addr);
 			} else {
 				/* if the symbol is not local to this
 				 * module then val+addend is a pointer
 				 * to the function descriptor */
-				DEBUGP("Non local FPTR64 Symbol %s loc %p val %lx\n",
+				pr_debug("Non local FPTR64 Symbol %s loc %p val %llx\n",
 				       strtab + sym->st_name,
 				       loc, val);
 				*loc64 = val + addend;

@@ -858,7 +852,7 @@ register_unwind_table(struct module *me,
 	end = table + sechdrs[me->arch.unwind_section].sh_size;
 	gp = (Elf_Addr)me->core_layout.base + me->arch.got_offset;
 
-	DEBUGP("register_unwind_table(), sect = %d at 0x%p - 0x%p (gp=0x%lx)\n",
+	pr_debug("register_unwind_table(), sect = %d at 0x%p - 0x%p (gp=0x%lx)\n",
 	       me->arch.unwind_section, table, end, gp);
 	me->arch.unwind = unwind_table_add(me->name, 0, gp, table, end);
 }

@@ -914,7 +908,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 		}
 	}
 
-	DEBUGP("module %s: strtab %p, symhdr %p\n",
+	pr_debug("module %s: strtab %p, symhdr %p\n",
 	       me->name, strtab, symhdr);
 
 	if(me->arch.got_count > MAX_GOTS) {

@@ -933,7 +927,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 	oldptr = (void *)symhdr->sh_addr;
 	newptr = oldptr + 1;	/* we start counting at 1 */
 	nsyms = symhdr->sh_size / sizeof(Elf_Sym);
-	DEBUGP("OLD num_symtab %lu\n", nsyms);
+	pr_debug("OLD num_symtab %lu\n", nsyms);
 
 	for (i = 1; i < nsyms; i++) {
 		oldptr++;	/* note, count starts at 1 so preincrement */

@@ -948,7 +942,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 
 	}
 	nsyms = newptr - (Elf_Sym *)symhdr->sh_addr;
-	DEBUGP("NEW num_symtab %lu\n", nsyms);
+	pr_debug("NEW num_symtab %lu\n", nsyms);
 	symhdr->sh_size = nsyms * sizeof(Elf_Sym);
 
 	/* find .altinstructions section */

Signature · 1 line

2.20.1

Rolf Eike Beer

2019-05-28 08:25 UTC #9 lore raw

Re: [PATCH 4/6] parisc: use pr_debug() in kernel/module.c

Quoted 37 lines · Sven Schnelle

Instead of using our own version, switch to the generic pr_() calls.
Signed-off-by Sven Schnelle <[email protected]>
---
 arch/parisc/kernel/module.c | 44 ++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 25 deletions(-)
arch/parisc/kernel/module.c +0 -0
index 43778420614b..3ff3b48df6e6 100644
@@ -48,9 +48,9 @@
  *	However, SEGREL32 is used only for PARISC unwind entries, and we 
want
* those entries to have an absolute address, and not just an offset.
*
- * The unwind table mechanism has the ability to specify an offset for
+ * The unwind table mechanism has the ability to specify an offset for
* the unwind table; however, because we split off the init functions
into
- * a different piece of memory, it is not possible to do this using a
+ * a different piece of memory, it is not possible to do this using a
* single offset. Instead, we use the above hack for now.
*/
arch/parisc/kernel/module.c +1 -1
@@ -315,7 +309,7 @@ unsigned int arch_mod_section_prepend(struct module 
 *mod,
 		* sizeof(struct stub_entry);
 }
 
-#define CONST
+#define CONST
 int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST Elf_Shdr *sechdrs,
arch/parisc/kernel/module.c +0 -0
 			      CONST char *secstrings,

arch/parisc/kernel/module.c int apply_relocate_add(Elf_Shdr *sechdrs, · line 619

Quoted 8 lines

arch/parisc/kernel/module.c +1 -1

@@ -619,7 +613,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* See note about special handling of SEGREL32 at
 			 * the beginning of this file.
 			 */
-			*loc = fsel(val, addend);
+			*loc = fsel(val, addend);
 			break;
 		case R_PARISC_SECREL32:
 			/* 32-bit section relative address. */

You are sneaking in unrelated whitespace fixes. I just want to let you know that you got caught ;)

Eike

Sven Schnelle

author

2019-05-29 17:54 UTC #10 lore raw

Re: [PATCH 4/6] parisc: use pr_debug() in kernel/module.c

Hi,

Quoted 9 lines · Rolf Eike Beer

Quoted 9 lines

arch/parisc/kernel/module.c +1 -1

-#define CONST
+#define CONST
 int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST Elf_Shdr *sechdrs,
 			      CONST char *secstrings,

@@ -619,7 +613,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			/* See note about special handling of SEGREL32 at
 			 * the beginning of this file.
 			 */

Quoted 8 lines

arch/parisc/kernel/module.c int apply_relocate_add(Elf_Shdr *sechdrs, · line 619
Quoted 5 lines
arch/parisc/kernel/module.c +1 -1
-			*loc = fsel(val, addend);
+			*loc = fsel(val, addend);
 			break;
 		case R_PARISC_SECREL32:
 			/* 32-bit section relative address. */
You are sneaking in unrelated whitespace fixes. I just want to let you know that you got caught ;)

Not contributing often to the linux kernel i'm not sure what the policy is. IMHO it's okay to fix whitespace errors when working on patches. If Helge says this is a problem i remove the whitespace changes.

Regards
Sven

Sven Schnelle

author

2019-05-27 19:05 UTC #11 lore raw

PATCH 5/6 compiler.h: add CC_USING_PATCHABLE_FUNCTION_ENTRY

In reply to Sven Schnelle

This can be used for architectures implementing dynamic ftrace via -fpatchable-function-entry.

Signed-off-by Sven Schnelle <[email protected]>

---
 include/linux/compiler_types.h | 2 ++
 1 file changed, 2 insertions(+)

include/linux/compiler_types.h +2 -0

index 19e58b9138a0..095d55c3834d 100644
@@ -112,6 +112,8 @@ struct ftrace_likely_data {
 
 #if defined(CC_USING_HOTPATCH)
 #define notrace			__attribute__((hotpatch(0, 0)))
+#elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
+#define notrace			__attribute__((patchable_function_entry(0, 0)))
 #else
 #define notrace			__attribute__((__no_instrument_function__))
 #endif

Signature · 1 line

2.20.1

Sven Schnelle

author

2019-05-27 19:05 UTC #12 lore raw

PATCH 6/6 parisc: add dynamic ftrace

In reply to Sven Schnelle

This patch implements dynamic ftrace for PA-RISC. The required mcount call sequences can get pretty long, so instead of patching the whole call sequence out of the functions, we are using -fpatchable-function-entry from gcc. This puts a configurable amount of NOPS before/at the start of the function. Taking do_sys_open() as example, which would look like this when the call is patched out:

1036b248:       08 00 02 40     nop
1036b24c:       08 00 02 40     nop
1036b250:       08 00 02 40     nop
1036b254:       08 00 02 40     nop

1036b258 <do_sys_open>:

1036b258:       08 00 02 40     nop
1036b25c:       08 03 02 41     copy r3,r1
1036b260:       6b c2 3f d9     stw rp,-14(sp)
1036b264:       08 1e 02 43     copy sp,r3
1036b268:       6f c1 01 00     stw,ma r1,80(sp)

When ftrace gets enabled for this function the kernel will patch these NOPs to:

1036b248:       10 19 57 20     <address of ftrace>
1036b24c:       6f c1 00 80     stw,ma r1,40(sp)
1036b250:       48 21 3f d1     ldw -18(r1),r1
1036b254:       e8 20 c0 02     bv,n r0(r1)

1036b258 <do_sys_open>:

1036b258:       e8 3f 1f df     b,l,n .-c,r1
1036b25c:       08 03 02 41     copy r3,r1
1036b260:       6b c2 3f d9     stw rp,-14(sp)
1036b264:       08 1e 02 43     copy sp,r3
1036b268:       6f c1 01 00     stw,ma r1,80(sp)

So the first NOP in do_sys_open() will be patched to jump backwards into some minimal trampoline code which pushes a stackframe, saves r1 which holds the return address, loads the address of the real ftrace function, and branches to that location. For 64 Bit things are getting a bit more complicated (and longer) because we must make sure that the address of ftrace location is 8 byte aligned, and the offset passed to ldd for fetching the address is 8 byte aligned as well.

Note that gcc has a bug which misplaces the function label, and needs a patch to make dynamic ftrace work.

Signed-off-by Sven Schnelle <[email protected]>

---
 arch/parisc/Kconfig               |   2 +
 arch/parisc/Makefile              |  18 +++++
 arch/parisc/include/asm/ftrace.h  |  15 +++-
 arch/parisc/kernel/Makefile       |   9 ++-
 arch/parisc/kernel/entry.S        |  64 +++++++++++++++
 arch/parisc/kernel/ftrace.c       | 129 +++++++++++++++++++++++++++---
 arch/parisc/kernel/module.c       |  20 ++++-
 arch/parisc/kernel/module.lds     |   7 ++
 arch/parisc/kernel/vmlinux.lds.S  |   2 +
 arch/parisc/mm/fixmap.c           |   4 +-
 include/asm-generic/vmlinux.lds.h |   7 ++
 11 files changed, 259 insertions(+), 18 deletions(-)
 create mode 100644 arch/parisc/kernel/module.lds

arch/parisc/Kconfig +2 -0

index 4860efa91d7b..42875ff15671 100644
@@ -59,6 +59,8 @@ config PARISC
 	select HAVE_ARCH_KGDB
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
+	select HAVE_DYNAMIC_FTRACE if $(cc-option,-fpatchable-function-entry=1,1)
+	select HAVE_FTRACE_MCOUNT_RECORD if HAVE_DYNAMIC_FTRACE
 
 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used

arch/parisc/Makefile +18 -0

index c19af26febe6..58d46665cad9 100644
@@ -47,6 +47,24 @@ ifneq ($(SUBARCH),$(UTS_MACHINE))
 	endif
 endif
 
+ifdef CONFIG_DYNAMIC_FTRACE
+ifdef CONFIG_64BIT
+NOP_COUNT := 8
+else
+NOP_COUNT := 5
+endif
+
+export CC_USING_RECORD_MCOUNT:=1
+export CC_USING_PATCHABLE_FUNCTION_ENTRY:=1
+
+KBUILD_AFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1
+KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
+		 -DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
+
+CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
+KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/parisc/kernel/module.lds
+endif
+
 OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
 
 cflags-y	:= -pipe

arch/parisc/include/asm/ftrace.h +13 -2

index 42b2c75a1645..958c0aa5dbb2 100644
@@ -5,12 +5,23 @@
 #ifndef __ASSEMBLY__
 extern void mcount(void);
 
-#define MCOUNT_INSN_SIZE 4
-
+#define MCOUNT_ADDR		((unsigned long)mcount)
+#define MCOUNT_INSN_SIZE	4
+#define CC_USING_NOP_MCOUNT
 extern unsigned long sys_call_table[];
 
 extern unsigned long return_address(unsigned int);
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+extern void ftrace_caller(void);
+
+struct dyn_arch_ftrace {
+};
+
+unsigned long ftrace_call_adjust(unsigned long addr);
+
+#endif
+
 #define ftrace_return_address(n) return_address(n)
 
 #endif /* __ASSEMBLY__ */

arch/parisc/kernel/Makefile +5 -4

index fc0df5c44468..c232266b517c 100644
@@ -14,10 +14,11 @@ obj-y	     	:= cache.o pacache.o setup.o pdt.o traps.o time.o irq.o \
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not profile debug and lowlevel utilities
-CFLAGS_REMOVE_ftrace.o = -pg
-CFLAGS_REMOVE_cache.o = -pg
-CFLAGS_REMOVE_perf.o = -pg
-CFLAGS_REMOVE_unwind.o = -pg
+CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_cache.o =  $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_perf.o = $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_unwind.o = $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_patch.o = $(CC_FLAGS_FTRACE)
 endif
 
 obj-$(CONFIG_SMP)	+= smp.o

arch/parisc/kernel/entry.S +64 -0

70 lines not on this page · raw message

arch/parisc/kernel/ftrace.c +120 -9

153 lines not on this page · raw message

arch/parisc/kernel/module.c +19 -1

index 3ff3b48df6e6..ed6640527534 100644
@@ -873,6 +873,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const char *strtab = NULL;
 	const Elf_Shdr *s;
 	char *secstrings;
+	int err, symindex = -1;
 	Elf_Sym *newptr, *oldptr;
 	Elf_Shdr *symhdr = NULL;
 #ifdef DEBUG

@@ -899,6 +900,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 		if(sechdrs[i].sh_type == SHT_SYMTAB
 		   && (sechdrs[i].sh_flags & SHF_ALLOC)) {
 			int strindex = sechdrs[i].sh_link;
+			symindex = i;
 			/* FIXME: AWFUL HACK
 			 * The cast is to drop the const from
 			 * the sechdrs pointer */

@@ -954,8 +956,24 @@ int module_finalize(const Elf_Ehdr *hdr,
 		if (!strcmp(".altinstructions", secname))
 			/* patch .altinstructions */
 			apply_alternatives(aseg, aseg + s->sh_size, me->name);
-	}
 
+		/* For 32 bit kernels we're compiling modules with
+		 * -ffunction-sections so we must relocate the addresses in the
+		 *__mcount_loc section.
+		 */
+		if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
+			if (s->sh_type == SHT_REL)
+				err = apply_relocate((Elf_Shdr *)sechdrs,
+							strtab, symindex,
+							s - sechdrs, me);
+			else if (s->sh_type == SHT_RELA)
+				err = apply_relocate_add((Elf_Shdr *)sechdrs,
+							strtab, symindex,
+							s - sechdrs, me);
+			if (err)
+				return err;
+		}
+	}
 	return 0;
 }

arch/parisc/kernel/module.lds new file +7 -0

new file mode 100644
index 000000000000..1a9a92aca5c8
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+SECTIONS {
+	__mcount_loc : {
+		*(__patchable_function_entries)
+	}
+}

arch/parisc/kernel/vmlinux.lds.S +2 -0

index cd33b4feacb1..99cd24f2ea01 100644
@@ -18,6 +18,8 @@
 				*(.data..vm0.pgd) \
 				*(.data..vm0.pte)
 
+#define CC_USING_PATCHABLE_FUNCTION_ENTRY
+
 #include <asm-generic/vmlinux.lds.h>
 
 /* needed for the processor specific cache alignment size */

arch/parisc/mm/fixmap.c +2 -2

index 36321bcd75ba..474cd241c150 100644
@@ -10,7 +10,7 @@
 #include <asm/cacheflush.h>
 #include <asm/fixmap.h>
 
-void set_fixmap(enum fixed_addresses idx, phys_addr_t phys)
+void notrace set_fixmap(enum fixed_addresses idx, phys_addr_t phys)
 {
 	unsigned long vaddr = __fix_to_virt(idx);
 	pgd_t *pgd = pgd_offset_k(vaddr);

@@ -28,7 +28,7 @@ void set_fixmap(enum fixed_addresses idx, phys_addr_t phys)
 	flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
 }
 
-void clear_fixmap(enum fixed_addresses idx)
+void notrace clear_fixmap(enum fixed_addresses idx)
 {
 	unsigned long vaddr = __fix_to_virt(idx);
 	pgd_t *pgd = pgd_offset_k(vaddr);

include/asm-generic/vmlinux.lds.h +7 -0

index 088987e9a3ea..ca42182992a5 100644
@@ -110,10 +110,17 @@
 #endif
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
+#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
+#define MCOUNT_REC()	. = ALIGN(8);				\
+			__start_mcount_loc = .;			\
+			KEEP(*(__patchable_function_entries))	\
+			__stop_mcount_loc = .;
+#else
 #define MCOUNT_REC()	. = ALIGN(8);				\
 			__start_mcount_loc = .;			\
 			KEEP(*(__mcount_loc))			\
 			__stop_mcount_loc = .;
+#endif
 #else
 #define MCOUNT_REC()
 #endif

Signature · 1 line

2.20.1

Rolf Eike Beer

2019-05-28 08:26 UTC #13 lore raw

Re: [PATCH 6/6] parisc: add dynamic ftrace

Am 2019-05-27 21:04, schrieb Sven Schnelle:

Quoted 42 lines

This patch implements dynamic ftrace for PA-RISC. The required mcount call sequences can get pretty long, so instead of patching the whole call sequence out of the functions, we are using -fpatchable-function-entry from gcc. This puts a configurable amount of NOPS before/at the start of the function. Taking do_sys_open() as example, which would look like this when the call is patched out:
1036b248:       08 00 02 40     nop
1036b24c:       08 00 02 40     nop
1036b250:       08 00 02 40     nop
1036b254:       08 00 02 40     nop
1036b258 <do_sys_open>:
1036b258:       08 00 02 40     nop
1036b25c:       08 03 02 41     copy r3,r1
1036b260:       6b c2 3f d9     stw rp,-14(sp)
1036b264:       08 1e 02 43     copy sp,r3
1036b268:       6f c1 01 00     stw,ma r1,80(sp)
When ftrace gets enabled for this function the kernel will patch these NOPs to:
1036b248:       10 19 57 20     <address of ftrace>
1036b24c:       6f c1 00 80     stw,ma r1,40(sp)
1036b250:       48 21 3f d1     ldw -18(r1),r1
1036b254:       e8 20 c0 02     bv,n r0(r1)
1036b258 <do_sys_open>:
1036b258:       e8 3f 1f df     b,l,n .-c,r1
1036b25c:       08 03 02 41     copy r3,r1
1036b260:       6b c2 3f d9     stw rp,-14(sp)
1036b264:       08 1e 02 43     copy sp,r3
1036b268:       6f c1 01 00     stw,ma r1,80(sp)
So the first NOP in do_sys_open() will be patched to jump backwards into some minimal trampoline code which pushes a stackframe, saves r1 which holds the return address, loads the address of the real ftrace function, and branches to that location. For 64 Bit things are getting a bit more complicated (and longer) because we must make sure that the address of ftrace location is 8 byte aligned, and the offset passed to ldd for fetching the address is 8 byte aligned as well.

Quoted 2 lines

Note that gcc has a bug which misplaces the function label, and needs a patch to make dynamic ftrace work.

This changelog entry _really_ needs a link to the gcc bugtracker so tracking this when doing downstream toolchain patching isn't driving other people mad trying to find out what you are talking about.

Eike