* [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2
@ 2022-09-01 5:58 Benjamin Gray
2022-09-01 5:58 ` [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text Benjamin Gray
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Benjamin Gray @ 2022-09-01 5:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: christophe.leroy, Benjamin Gray, ajd, npiggin
WIP implementation of out-of-line static calls for PowerPC 64-bit ELF V2 ABI.
Static calls patch an indirect branch into a direct branch at runtime.
Out-of-line specifically has a caller directly call a trampoline, and
the trampoline gets patched to directly call the target. This current
implementation has a known issue described in detail below, and is
presented here for any comments or suggestions.
64-bit ELF V2 specifies a table of contents (TOC) pointer stored in r2.
Functions that use a TOC can use it to perform various operations
relative to its value. When the caller and target use different TOCs,
the static call implementation must ensure the TOC is kept consistent
so that neither tries to use the other's TOC.
However, while the trampoline can change the caller's TOC to the target's
TOC, it cannot restore the caller's TOC when the target returns. For the
trampoline to do this would require the target to return to the trampoline,
and so the return address back to the caller would need to be saved to
the stack. But the trampoline cannot move the stack because the target
may be expecting parameters relative to the stack pointer (when registers
are insufficient or varargs are used). And as static calls are usable in
generic code, there can be no arch-specific restrictions on parameters
that would sidestep this issue.
Normally the TOC change issue is resolved by the caller, which will save
and restore its TOC if necessary. For static calls though the caller
sees the trampoline as a local function, so assumes it does not change
the TOC and treats r2 as nonvolatile (no save & restore added).
This is a simialar problem to that faced by livepatch. Static calls may have
a few more options though, because the call is performed through a
`static_call` macro, allowing annotation and insertion of inline assembly
at every callsite.
I can think of several possible solutions, but they are relatively complex:
1. Patching the callsites at runtime, as is done for inline static calls.
This also requires some inline assembly to save `r2` to the TOC pointer
Doubleword slot on the stack before each static call, as the caller may
not have done so in its prologue. It should be easy to add though, because
static calls are invoked through the `static_call` macro that can be
modified appropriately. The runtime patching then modifies the trailing
function call `nop` to restore this r2 value.
The patching itself can probably be done at compile time at kernel callsites.
2. Use the livepatch stack method of growing the base of the stack backwards.
I haven't looked too closely at the implementation though, especially
regarding how much room is available.
The benefit of this method is that there can be zero overhead when the
trampoline and target share a TOC. So the trampoline in kernel-only
calls can still just be a single direct branch.
3. Remove the local entry point from the trampoline. This makes the trampoline
less efficient, as it cannot assume r2 to be correct, but should at least
cause the caller to automatically save and restore r2 without manual patching.
From the ABI manual:
> 2.2.1. Function Call Linkage Protocols
> A function that uses a TOC pointer always has a separate local entry point
> [...], and preserves r2 when called via its local entry point.
>
> 2.2.2.1. Register Roles
> (a) Register r2 is nonvolatile with respect to calls between functions
> in the same compilation unit, except under the conditions in footnote (b)
> (b) Register r2 is volatile and available for use in a function that does not
> use a TOC pointer and that does not guarantee that it preserves r2.
So not having a local entry point implies not using a TOC pointer, which
implies r2 is volatile if the trampoline does not guarantee that it preserves
r2. However experimenting with such a trampoline showed the caller still did
not preserve its TOC when necessary, even when the trampoline used instructions
that wrote to r2. Possibly there's an attribute that can be used to mark the
necessary info, but I could not find one.
Benjamin Gray (3):
static_call: Move static call selftest to static_call.c
powerpc/64: Add support for out-of-line static calls
powerpc/64: Add tests for out-of-line static calls
Russell Currey (1):
powerpc/code-patching: add patch_memory() for writing RO text
arch/powerpc/Kconfig | 23 +-
arch/powerpc/include/asm/code-patching.h | 2 +
arch/powerpc/include/asm/static_call.h | 45 +++-
arch/powerpc/kernel/Makefile | 4 +-
arch/powerpc/kernel/static_call.c | 184 +++++++++++++++-
arch/powerpc/kernel/static_call_test.c | 257 +++++++++++++++++++++++
arch/powerpc/lib/code-patching.c | 65 ++++++
kernel/static_call.c | 43 ++++
kernel/static_call_inline.c | 43 ----
9 files changed, 613 insertions(+), 53 deletions(-)
create mode 100644 arch/powerpc/kernel/static_call_test.c
base-commit: c5e4d5e99162ba8025d58a3af7ad103f155d2df7
--
2.37.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
@ 2022-09-01 5:58 ` Benjamin Gray
2022-09-01 7:01 ` Christophe Leroy
2022-09-01 5:58 ` [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c Benjamin Gray
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Benjamin Gray @ 2022-09-01 5:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: christophe.leroy, Benjamin Gray, ajd, npiggin
From: Russell Currey <ruscur@russell.cc>
powerpc allocates a text poke area of one page that is used by
patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
is enabled.
patch_instruction() is only designed for instructions,
so writing data using the text poke area can only happen 4 bytes
at a time - each with a page map/unmap, pte flush and syncs.
This patch introduces patch_memory(), implementing the same
interface as memcpy(), similar to x86's text_poke() and s390's
s390_kernel_write(). patch_memory() only needs to map the text
poke area once, unless the write would cross a page boundary.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 65 ++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 1c6316ec4b74..3de90748bce7 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -76,6 +76,7 @@ int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
int patch_branch(u32 *addr, unsigned long target, int flags);
int patch_instruction(u32 *addr, ppc_inst_t instr);
int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
+void *patch_memory(void *dest, const void *src, size_t size);
static inline unsigned long patch_site_addr(s32 *site)
{
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 6edf0697a526..0cca39af44cb 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -14,6 +14,7 @@
#include <asm/page.h>
#include <asm/code-patching.h>
#include <asm/inst.h>
+#include <asm/cacheflush.h>
static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr)
{
@@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
return err;
}
+
+static int do_patch_memory(void *dest, const void *src, size_t size)
+{
+ int err;
+ unsigned long text_poke_addr, patch_addr;
+
+ text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
+
+ err = map_patch_area(dest, text_poke_addr);
+ if (err)
+ return err;
+
+ patch_addr = text_poke_addr + offset_in_page(dest);
+ copy_to_kernel_nofault((u8 *)patch_addr, src, size);
+
+ flush_icache_range(patch_addr, size);
+ unmap_patch_area(text_poke_addr);
+
+ return 0;
+}
+
+/**
+ * patch_memory - write data using the text poke area
+ *
+ * @dest: destination address
+ * @src: source address
+ * @size: size in bytes
+ *
+ * like memcpy(), but using the text poke area. No atomicity guarantees.
+ * Do not use for instructions, use patch_instruction() instead.
+ * Handles crossing page boundaries, though you shouldn't need to.
+ *
+ * Return value:
+ * @dest
+ **/
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ int err;
+ unsigned long flags;
+ size_t written, write_size;
+
+ // If the poke area isn't set up, it's early boot and we can just memcpy.
+ if (!this_cpu_read(text_poke_area))
+ return memcpy(dest, src, size);
+
+ for (written = 0; written < size; written += write_size) {
+ // Write as much as possible without crossing a page boundary.
+ write_size = min_t(size_t, size - written,
+ PAGE_SIZE - offset_in_page(dest + written));
+
+ local_irq_save(flags);
+ err = do_patch_memory(dest + written, src + written, write_size);
+ local_irq_restore(flags);
+ if (err)
+ return ERR_PTR(err);
+ }
+
+ return dest;
+}
#else /* !CONFIG_STRICT_KERNEL_RWX */
static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
@@ -190,6 +250,11 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
return raw_patch_instruction(addr, instr);
}
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ return memcpy(dest, src, size);
+}
+
#endif /* CONFIG_STRICT_KERNEL_RWX */
__ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);
--
2.37.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
2022-09-01 5:58 ` [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text Benjamin Gray
@ 2022-09-01 5:58 ` Benjamin Gray
2022-09-01 6:50 ` Christophe Leroy
2022-09-01 5:58 ` [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls Benjamin Gray
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Benjamin Gray @ 2022-09-01 5:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: christophe.leroy, Benjamin Gray, ajd, npiggin
These tests are out-of-line only, so moving them to the
out-of-line file allows them to be run when an arch does
not implement inline static calls.
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
kernel/static_call.c | 43 +++++++++++++++++++++++++++++++++++++
kernel/static_call_inline.c | 43 -------------------------------------
2 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/kernel/static_call.c b/kernel/static_call.c
index e9c3e69f3837..953ec46e5691 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -6,3 +6,46 @@ long __static_call_return0(void)
return 0;
}
EXPORT_SYMBOL_GPL(__static_call_return0);
+
+#ifdef CONFIG_STATIC_CALL_SELFTEST
+
+static int func_a(int x)
+{
+ return x+1;
+}
+
+static int func_b(int x)
+{
+ return x+2;
+}
+
+DEFINE_STATIC_CALL(sc_selftest, func_a);
+
+static struct static_call_data {
+ int (*func)(int);
+ int val;
+ int expect;
+} static_call_data [] __initdata = {
+ { NULL, 2, 3 },
+ { func_b, 2, 4 },
+ { func_a, 2, 3 }
+};
+
+static int __init test_static_call_init(void)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(static_call_data); i++ ) {
+ struct static_call_data *scd = &static_call_data[i];
+
+ if (scd->func)
+ static_call_update(sc_selftest, scd->func);
+
+ WARN_ON(static_call(sc_selftest)(scd->val) != scd->expect);
+ }
+
+ return 0;
+}
+early_initcall(test_static_call_init);
+
+#endif /* CONFIG_STATIC_CALL_SELFTEST */
diff --git a/kernel/static_call_inline.c b/kernel/static_call_inline.c
index dc5665b62814..64d04d054698 100644
--- a/kernel/static_call_inline.c
+++ b/kernel/static_call_inline.c
@@ -498,46 +498,3 @@ int __init static_call_init(void)
return 0;
}
early_initcall(static_call_init);
-
-#ifdef CONFIG_STATIC_CALL_SELFTEST
-
-static int func_a(int x)
-{
- return x+1;
-}
-
-static int func_b(int x)
-{
- return x+2;
-}
-
-DEFINE_STATIC_CALL(sc_selftest, func_a);
-
-static struct static_call_data {
- int (*func)(int);
- int val;
- int expect;
-} static_call_data [] __initdata = {
- { NULL, 2, 3 },
- { func_b, 2, 4 },
- { func_a, 2, 3 }
-};
-
-static int __init test_static_call_init(void)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(static_call_data); i++ ) {
- struct static_call_data *scd = &static_call_data[i];
-
- if (scd->func)
- static_call_update(sc_selftest, scd->func);
-
- WARN_ON(static_call(sc_selftest)(scd->val) != scd->expect);
- }
-
- return 0;
-}
-early_initcall(test_static_call_init);
-
-#endif /* CONFIG_STATIC_CALL_SELFTEST */
--
2.37.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
2022-09-01 5:58 ` [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text Benjamin Gray
2022-09-01 5:58 ` [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c Benjamin Gray
@ 2022-09-01 5:58 ` Benjamin Gray
2022-09-01 7:33 ` Christophe Leroy
2022-09-01 5:58 ` [RFC PATCH 4/4] powerpc/64: Add tests " Benjamin Gray
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Benjamin Gray @ 2022-09-01 5:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: christophe.leroy, Benjamin Gray, ajd, npiggin
Implement static call support for 64 bit V2 ABI. This requires
making sure the TOC is kept correct across kernel-module
boundaries. As a secondary concern, it tries to use the local
entry point of a target wherever possible. It does so by
checking if both tramp & target are kernel code, and falls
back to detecting the common global entry point patterns
if modules are involved. Detecting the global entry point is
also required for setting the local entry point as the trampoline
target: if we cannot detect the local entry point, then we need to
convservatively initialise r12 and use the global entry point.
The implementation is incorrect as-is, because a trampoline cannot
use the stack. Doing so has the same issue described in 85baa095,
where parameters passed relative to the stack pointer (large arg count
or varargs) are broken. However the trampoline must guarantee the
TOC be restored before the caller uses it again.
Static call sites are themselves static, so it is feasible to handle
this by patching the callsites. However the linker currently
does not seem to realise that the trampoline treats r2 as volatile
(even with an alternative trampoline that does not have a separate
local entry point), and so does not insert the appropriate restoration
at the call site.
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
arch/powerpc/Kconfig | 13 +++-
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/include/asm/static_call.h | 45 ++++++++++++-
arch/powerpc/kernel/Makefile | 3 +-
arch/powerpc/kernel/static_call.c | 80 ++++++++++++++++++++++--
5 files changed, 132 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 4c466acdc70d..1d5abbeb2c40 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -248,7 +248,7 @@ config PPC
select HAVE_SOFTIRQ_ON_OWN_STACK
select HAVE_STACKPROTECTOR if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2)
select HAVE_STACKPROTECTOR if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13)
- select HAVE_STATIC_CALL if PPC32
+ select HAVE_STATIC_CALL if PPC_ENABLE_STATIC_CALL
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_VIRT_CPU_ACCOUNTING
select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE
@@ -1023,6 +1023,17 @@ config PPC_RTAS_FILTER
Say Y unless you know what you are doing and the filter is causing
problems for you.
+config PPC_ENABLE_STATIC_CALL
+ bool "Enable static calls"
+ default y
+ depends on PPC32 || PPC64_ELF_ABI_V2
+ help
+ PowerPC static calls with the ELF V2 ABI are not as straightforward
+ as checking if the target is in range of a branch or not. They must
+ also ensure the TOC remains consistent. This leads to complex
+ performance results, so it's useful to make them configurable to
+ allow testing with the generic implementation too.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 3de90748bce7..319cb1eef71c 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -126,6 +126,7 @@ int translate_branch(ppc_inst_t *instr, const u32 *dest, const u32 *src);
bool is_conditional_branch(ppc_inst_t instr);
#define OP_RT_RA_MASK 0xffff0000UL
+#define OP_SI_MASK 0x0000ffffUL
#define LIS_R2 (PPC_RAW_LIS(_R2, 0))
#define ADDIS_R2_R12 (PPC_RAW_ADDIS(_R2, _R12, 0))
#define ADDI_R2_R2 (PPC_RAW_ADDI(_R2, _R2, 0))
diff --git a/arch/powerpc/include/asm/static_call.h b/arch/powerpc/include/asm/static_call.h
index de1018cc522b..d85ff3f88c8e 100644
--- a/arch/powerpc/include/asm/static_call.h
+++ b/arch/powerpc/include/asm/static_call.h
@@ -2,12 +2,49 @@
#ifndef _ASM_POWERPC_STATIC_CALL_H
#define _ASM_POWERPC_STATIC_CALL_H
+#if defined(CONFIG_PPC64_ELF_ABI_V2)
+
+#define __PPC_SCT(name, inst) \
+ asm(".pushsection .text, \"ax\" \n" \
+ ".align 6 \n" \
+ ".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
+ STATIC_CALL_TRAMP_STR(name) ": \n" \
+ " addis 2, 12, (.TOC.-" STATIC_CALL_TRAMP_STR(name) ")@ha \n" \
+ " addi 2, 2, (.TOC.-" STATIC_CALL_TRAMP_STR(name) ")@l \n" \
+ ".localentry " STATIC_CALL_TRAMP_STR(name) ", .-" STATIC_CALL_TRAMP_STR(name) "\n" \
+ " " inst " \n" \
+ " mflr 0 \n" \
+ " std 0, 16(1) \n" \
+ " stdu 1, -32(1) \n" \
+ " std 2, 24(1) \n" \
+ " addis 12, 2, 2f@toc@ha \n" \
+ " ld 12, 2f@toc@l(12) \n" \
+ " mtctr 12 \n" \
+ " bctrl \n" \
+ " ld 2, 24(1) \n" \
+ " addi 1, 1, 32 \n" \
+ " ld 0, 16(1) \n" \
+ " mtlr 0 \n" \
+ " blr \n" \
+ "1: li 3, 0 \n" \
+ " blr \n" \
+ ".balign 8 \n" \
+ "2: .8byte 0 \n" \
+ ".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
+ ".size " STATIC_CALL_TRAMP_STR(name) ", . - " STATIC_CALL_TRAMP_STR(name) " \n" \
+ ".popsection \n")
+
+#define PPC_SCT_RET0 64 /* Offset of label 1 */
+#define PPC_SCT_DATA 72 /* Offset of label 2 (aligned) */
+
+#elif defined(PPC32)
+
#define __PPC_SCT(name, inst) \
asm(".pushsection .text, \"ax\" \n" \
".align 5 \n" \
".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
STATIC_CALL_TRAMP_STR(name) ": \n" \
- inst " \n" \
+ " " inst " \n" \
" lis 12,2f@ha \n" \
" lwz 12,2f@l(12) \n" \
" mtctr 12 \n" \
@@ -22,8 +59,12 @@
#define PPC_SCT_RET0 20 /* Offset of label 1 */
#define PPC_SCT_DATA 28 /* Offset of label 2 */
+#else /* !CONFIG_PPC64_ELF_ABI_V2 && !PPC32 */
+#error "Unsupported ABI"
+#endif /* CONFIG_PPC64_ELF_ABI_V2 */
+
#define ARCH_DEFINE_STATIC_CALL_TRAMP(name, func) __PPC_SCT(name, "b " #func)
#define ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name) __PPC_SCT(name, "blr")
-#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name) __PPC_SCT(name, "b .+20")
+#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name) __PPC_SCT(name, "b 1f")
#endif /* _ASM_POWERPC_STATIC_CALL_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 06d2d1f78f71..a30d0d0f5499 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -128,8 +128,9 @@ extra-y += vmlinux.lds
obj-$(CONFIG_RELOCATABLE) += reloc_$(BITS).o
-obj-$(CONFIG_PPC32) += entry_32.o setup_32.o early_32.o static_call.o
+obj-$(CONFIG_PPC32) += entry_32.o setup_32.o early_32.o
obj-$(CONFIG_PPC64) += dma-iommu.o iommu.o
+obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o
obj-$(CONFIG_KGDB) += kgdb.o
obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
index 863a7aa24650..ed3bc361fdb0 100644
--- a/arch/powerpc/kernel/static_call.c
+++ b/arch/powerpc/kernel/static_call.c
@@ -1,33 +1,101 @@
// SPDX-License-Identifier: GPL-2.0
+#include <asm/code-patching.h>
+#include <linux/export.h>
+#include <linux/kconfig.h>
+#include <linux/kernel.h>
#include <linux/memory.h>
#include <linux/static_call.h>
+#include <linux/syscalls.h>
-#include <asm/code-patching.h>
+#ifdef CONFIG_PPC64_ELF_ABI_V2
+
+static void* ppc_function_toc(u32 *func) {
+ u32 insn1 = *func;
+ u32 insn2 = *(func+1);
+ u64 si1 = sign_extend64((insn1 & OP_SI_MASK) << 16, 31);
+ u64 si2 = sign_extend64(insn2 & OP_SI_MASK, 15);
+ u64 addr = ((u64) func + si1) + si2;
+
+ if ((((insn1 & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
+ ((insn1 & OP_RT_RA_MASK) == LIS_R2)) &&
+ ((insn2 & OP_RT_RA_MASK) == ADDI_R2_R2))
+ return (void*) addr;
+ else
+ return NULL;
+}
+
+static bool shares_toc(void *func1, void *func2) {
+ void* func1_toc;
+ void* func2_toc;
+
+ if (func1 == NULL || func2 == NULL)
+ return false;
+
+ /* Assume the kernel only uses a single TOC */
+ if (core_kernel_text((unsigned long)func1) &&
+ core_kernel_text((unsigned long)func2))
+ return true;
+
+ /* Fall back to calculating the TOC from common patterns
+ * if modules are involved
+ */
+ func1_toc = ppc_function_toc(func1);
+ func2_toc = ppc_function_toc(func2);
+ return func1_toc != NULL && func2_toc != NULL && (func1_toc == func2_toc);
+}
+
+#endif /* CONFIG_PPC64_ELF_ABI_V2 */
void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
{
int err;
bool is_ret0 = (func == __static_call_return0);
unsigned long target = (unsigned long)(is_ret0 ? tramp + PPC_SCT_RET0 : func);
- bool is_short = is_offset_in_branch_range((long)target - (long)tramp);
+ bool is_short;
+ void* tramp_entry;
if (!tramp)
return;
+ tramp_entry = (void*)ppc_function_entry(tramp);
+
+#ifdef CONFIG_PPC64_ELF_ABI_V2
+ if (shares_toc(tramp, (void*)target)) {
+ /* Confirm that the local entry point is in range */
+ is_short = is_offset_in_branch_range(
+ (long)ppc_function_entry((void*)target) - (long)tramp_entry);
+ } else {
+ /* Combine out-of-range with not sharing a TOC. Though it's possible an
+ * out-of-range target shares a TOC, handling this separately complicates
+ * the trampoline. It's simpler to always use the global entry point
+ * in this case.
+ */
+ is_short = false;
+ }
+#else /* !CONFIG_PPC64_ELF_ABI_V2 */
+ is_short = is_offset_in_branch_range((long)target - (long)tramp);
+#endif /* CONFIG_PPC64_ELF_ABI_V2 */
+
mutex_lock(&text_mutex);
if (func && !is_short) {
- err = patch_instruction(tramp + PPC_SCT_DATA, ppc_inst(target));
+ /* This assumes that the update is atomic. The current implementation uses
+ * stw/std and the store location is aligned. A sync is issued by one of the
+ * patch_instruction/patch_branch functions below.
+ */
+ err = PTR_ERR_OR_ZERO(patch_memory(
+ tramp + PPC_SCT_DATA, &target, sizeof(target)));
if (err)
goto out;
}
if (!func)
- err = patch_instruction(tramp, ppc_inst(PPC_RAW_BLR()));
+ err = patch_instruction(tramp_entry, ppc_inst(PPC_RAW_BLR()));
else if (is_short)
- err = patch_branch(tramp, target, 0);
+ err = patch_branch(tramp_entry, ppc_function_entry((void*) target), 0);
else
- err = patch_instruction(tramp, ppc_inst(PPC_RAW_NOP()));
+ err = patch_instruction(tramp_entry, ppc_inst(PPC_RAW_NOP()));
+
out:
mutex_unlock(&text_mutex);
--
2.37.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 4/4] powerpc/64: Add tests for out-of-line static calls
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
` (2 preceding siblings ...)
2022-09-01 5:58 ` [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls Benjamin Gray
@ 2022-09-01 5:58 ` Benjamin Gray
2022-09-01 8:07 ` [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Christophe Leroy
2022-09-13 3:31 ` Benjamin Gray
5 siblings, 0 replies; 11+ messages in thread
From: Benjamin Gray @ 2022-09-01 5:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: christophe.leroy, Benjamin Gray, ajd, npiggin
KUnit tests for the various combinations of caller/trampoline/target and
kernel/module. They must be run from a module loaded at runtime to
guarantee they have a different TOC to the kernel.
The tests try to mitigate the chance of panicing by restoring the
TOC after every static call. Not all possible errors can be caught
by this, but it makes certain kinds of errors easier to debug.
Currently the large arg count test is failing due to the trampoline
implementation using the stack.
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
arch/powerpc/Kconfig | 10 +
arch/powerpc/kernel/Makefile | 1 +
arch/powerpc/kernel/static_call.c | 104 ++++++++++
arch/powerpc/kernel/static_call_test.c | 257 +++++++++++++++++++++++++
4 files changed, 372 insertions(+)
create mode 100644 arch/powerpc/kernel/static_call_test.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1d5abbeb2c40..39e17c35e885 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -1034,6 +1034,16 @@ config PPC_ENABLE_STATIC_CALL
performance results, so it's useful to make them configurable to
allow testing with the generic implementation too.
+config PPC_STATIC_CALL_KUNIT_TEST
+ tristate "KUnit tests for PPC64 ELF ABI V2 static calls"
+ default KUNIT_ALL_TESTS
+ depends on HAVE_STATIC_CALL && PPC64_ELF_ABI_V2 && KUNIT && m
+ help
+ Tests that check the TOC is kept consistent across all combinations
+ of caller/trampoline/target being kernel/module. Must be built as a
+ module and loaded at runtime to ensure the module has a different
+ TOC to the kernel.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index a30d0d0f5499..22c07e3d34df 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_RELOCATABLE) += reloc_$(BITS).o
obj-$(CONFIG_PPC32) += entry_32.o setup_32.o early_32.o
obj-$(CONFIG_PPC64) += dma-iommu.o iommu.o
obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o
+obj-$(CONFIG_PPC_STATIC_CALL_KUNIT_TEST) += static_call_test.o
obj-$(CONFIG_KGDB) += kgdb.o
obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
index ed3bc361fdb0..93a4ab80b451 100644
--- a/arch/powerpc/kernel/static_call.c
+++ b/arch/powerpc/kernel/static_call.c
@@ -7,6 +7,10 @@
#include <linux/static_call.h>
#include <linux/syscalls.h>
+#if IS_MODULE(CONFIG_PPC_STATIC_CALL_KUNIT_TEST)
+#include <kunit/test.h>
+#endif
+
#ifdef CONFIG_PPC64_ELF_ABI_V2
static void* ppc_function_toc(u32 *func) {
@@ -103,3 +107,103 @@ void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
panic("%s: patching failed %pS at %pS\n", __func__, func, tramp);
}
EXPORT_SYMBOL_GPL(arch_static_call_transform);
+
+
+#if IS_MODULE(CONFIG_PPC_STATIC_CALL_KUNIT_TEST)
+
+/* The following are some kernel hooks for testing the static call
+ * implementation from the static_call_test module. The bulk of the
+ * assertions are run in that module, except for the TOC checks that
+ * must be done in the core kernel context.
+ */
+
+/* Reserve these registers for testing (same registers as in static_call_test.c) */
+register void* current_toc asm ("r2");
+register void* module_toc asm ("r14");
+register void* actual_module_toc asm ("r15");
+register void* kernel_toc asm ("r16");
+register void* actual_toc asm ("r17");
+
+static void* static_kernel_toc;
+static void* static_actual_toc;
+
+#define fixup_toc(test) \
+ actual_toc = current_toc; \
+ current_toc = kernel_toc; \
+ KUNIT_EXPECT_PTR_EQ(test, kernel_toc, actual_toc)
+
+void ppc_sc_kernel_toc_init(void)
+{
+ static_kernel_toc = kernel_toc;
+ static_actual_toc = actual_toc;
+
+ kernel_toc = current_toc;
+}
+
+void ppc_sc_kernel_toc_exit(void)
+{
+ kernel_toc = static_kernel_toc;
+ actual_toc = static_actual_toc;
+}
+
+int ppc_sc_kernel_target_1(struct kunit* test)
+{
+ fixup_toc(test);
+ return 1;
+}
+
+int ppc_sc_kernel_target_2(struct kunit* test)
+{
+ fixup_toc(test);
+ return 2;
+}
+
+DEFINE_STATIC_CALL(ppc_sc_kernel, ppc_sc_kernel_target_1);
+
+int ppc_sc_kernel_call(struct kunit* test)
+{
+ int ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ return ret;
+}
+
+int ppc_sc_kernel_call_indirect(struct kunit* test, int (*fn)(struct kunit*))
+{
+ int ret = fn(test);
+ fixup_toc(test);
+ return ret;
+}
+
+long ppc_sc_kernel_target_big(struct kunit* test,
+ long a,
+ long b,
+ long c,
+ long d,
+ long e,
+ long f,
+ long g,
+ long h,
+ long i)
+{
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, a, b);
+ KUNIT_EXPECT_EQ(test, a, c);
+ KUNIT_EXPECT_EQ(test, a, d);
+ KUNIT_EXPECT_EQ(test, a, e);
+ KUNIT_EXPECT_EQ(test, a, f);
+ KUNIT_EXPECT_EQ(test, a, g);
+ KUNIT_EXPECT_EQ(test, a, h);
+ KUNIT_EXPECT_EQ(test, a, i);
+ return ~a;
+}
+
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_toc_init);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_toc_exit);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_1);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_2);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_big);
+EXPORT_STATIC_CALL_GPL(ppc_sc_kernel);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_call);
+EXPORT_SYMBOL_GPL(ppc_sc_kernel_call_indirect);
+
+#endif /* IS_MODULE(CONFIG_PPC_STATIC_CALL_KUNIT_TEST) */
diff --git a/arch/powerpc/kernel/static_call_test.c b/arch/powerpc/kernel/static_call_test.c
new file mode 100644
index 000000000000..136227a623b4
--- /dev/null
+++ b/arch/powerpc/kernel/static_call_test.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+#include <linux/kconfig.h>
+#include <linux/module.h>
+#include <linux/static_call.h>
+
+/*
+ * Tests to ensure correctness in a variety of cases for static calls.
+ *
+ * The tests focus on ensuring the TOC is kept consistent across the
+ * module-kernel boundary, as compilers can't see that a trampoline
+ * defined locally in the kernel might be jumping to a function in a
+ * module. This makes it important that these tests are compiled as a
+ * module, so the TOC will be different to the kernel's.
+ *
+ * Register variables are used to allow easy position independent
+ * correction of a TOC before it is used for anything. This means
+ * a failing test doesn't always crash the whole kernel. The registers
+ * are initialised on entry and restored on exit of each test using
+ * KUnit's init and exit hooks. The tests only call internal and
+ * specially defined kernel functions, so the use of these registers
+ * will not clobber anything else.
+ */
+
+extern void ppc_sc_kernel_toc_init(void);
+extern void ppc_sc_kernel_toc_exit(void);
+DECLARE_STATIC_CALL(ppc_sc_kernel, int(struct kunit*));
+extern int ppc_sc_kernel_target_1(struct kunit* test);
+extern int ppc_sc_kernel_target_2(struct kunit* test);
+extern long ppc_sc_kernel_target_big(struct kunit* test,
+ long a,
+ long b,
+ long c,
+ long d,
+ long e,
+ long f,
+ long g,
+ long h,
+ long i);
+extern int ppc_sc_kernel_call(struct kunit* test);
+extern int ppc_sc_kernel_call_indirect(struct kunit* test, int(*fn)(struct kunit*));
+
+/* Registers we reserve for use while testing */
+register void* current_toc asm ("r2");
+register void* module_toc asm ("r14");
+register void* actual_toc asm ("r15");
+register void* kernel_toc asm ("r16");
+register void* actual_kernel_toc asm ("r17");
+
+/* To hold a copy of the old register values while we test */
+static void* static_module_toc;
+static void* static_actual_toc;
+
+/* Restore the TOC to a known correct value and check if the prior value was bad */
+#define fixup_toc(test) \
+ actual_toc = current_toc; \
+ current_toc = module_toc; \
+ KUNIT_EXPECT_PTR_EQ(test, module_toc, actual_toc)
+
+static int module_target_11(struct kunit *test)
+{
+ fixup_toc(test);
+ return 11;
+}
+
+static int module_target_12(struct kunit *test)
+{
+ fixup_toc(test);
+ return 12;
+}
+
+DEFINE_STATIC_CALL(module_sc, module_target_11);
+
+DEFINE_STATIC_CALL_RET0(module_sc_ret0, long(void));
+DEFINE_STATIC_CALL_NULL(module_sc_null, long(long));
+
+static long add_one(long *val)
+{
+ return (*val)++;
+}
+
+static void null_function_test(struct kunit *test)
+{
+ long val = 0;
+
+ /* Check argument unconditionally evaluated */
+ static_call_cond(module_sc_null)(add_one(&val));
+ fixup_toc(test);
+ KUNIT_ASSERT_EQ(test, 1, val);
+}
+
+static void return_zero_test(struct kunit *test)
+{
+ long ret;
+ KUNIT_ASSERT_EQ(test, 0, static_call(module_sc_ret0)());
+
+ static_call_update(ppc_sc_kernel, (void*) __static_call_return0);
+ ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ KUNIT_ASSERT_EQ(test, 0, ret);
+
+ static_call_update(module_sc, (void*) __static_call_return0);
+ ret = static_call(module_sc)(test);
+ fixup_toc(test);
+ KUNIT_ASSERT_EQ(test, 0, ret);
+}
+
+static void kernel_kernel_kernel_test(struct kunit *test)
+{
+ static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_1);
+ KUNIT_EXPECT_EQ(test, 1, ppc_sc_kernel_call(test));
+
+ static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_2);
+ KUNIT_EXPECT_EQ(test, 2, ppc_sc_kernel_call(test));
+}
+
+static void kernel_kernel_module_test(struct kunit *test)
+{
+ static_call_update(ppc_sc_kernel, module_target_11);
+ KUNIT_EXPECT_EQ(test, 11, ppc_sc_kernel_call(test));
+
+ static_call_update(ppc_sc_kernel, module_target_12);
+ KUNIT_EXPECT_EQ(test, 12, ppc_sc_kernel_call(test));
+}
+
+static void kernel_module_kernel_test(struct kunit *test)
+{
+ static_call_update(module_sc, ppc_sc_kernel_target_1);
+ KUNIT_EXPECT_EQ(test, 1, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
+
+ static_call_update(module_sc, ppc_sc_kernel_target_2);
+ KUNIT_EXPECT_EQ(test, 2, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
+}
+
+static void kernel_module_module_test(struct kunit *test)
+{
+ static_call_update(module_sc, module_target_11);
+ KUNIT_EXPECT_EQ(test, 11, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
+
+ static_call_update(module_sc, module_target_12);
+ KUNIT_EXPECT_EQ(test, 12, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
+}
+
+static void module_kernel_kernel_test(struct kunit *test)
+{
+ long ret;
+
+ static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_1);
+ ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 1, ret);
+
+ static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_2);
+ ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 2, ret);
+}
+
+static void module_kernel_module_test(struct kunit *test)
+{
+ long ret;
+
+ static_call_update(ppc_sc_kernel, module_target_11);
+ ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 11, ret);
+
+ static_call_update(ppc_sc_kernel, module_target_12);
+ ret = static_call(ppc_sc_kernel)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 12, ret);
+}
+
+static void module_module_kernel_test(struct kunit *test)
+{
+ long ret;
+
+ static_call_update(module_sc, ppc_sc_kernel_target_1);
+ ret = static_call(module_sc)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 1, ret);
+
+ static_call_update(module_sc, ppc_sc_kernel_target_2);
+ ret = static_call(module_sc)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 2,ret);
+}
+
+static void module_module_module_test(struct kunit *test)
+{
+ long ret;
+
+ static_call_update(module_sc, module_target_11);
+ ret = static_call(module_sc)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 11, ret);
+
+ static_call_update(module_sc, module_target_12);
+ ret = static_call(module_sc)(test);
+ fixup_toc(test);
+ KUNIT_EXPECT_EQ(test, 12, ret);
+}
+
+DEFINE_STATIC_CALL(module_sc_stack_params, ppc_sc_kernel_target_big);
+
+static void stack_parameters_test(struct kunit *test)
+{
+ long m = 0x1234567887654321;
+ long ret = static_call(module_sc_stack_params)(test, m, m, m, m, m, m, m, m, m);
+ fixup_toc(test);
+ KUNIT_ASSERT_EQ(test, ~m, ret);
+}
+
+static struct kunit_case static_call_test_cases[] = {
+ KUNIT_CASE(null_function_test),
+ KUNIT_CASE(return_zero_test),
+ KUNIT_CASE(stack_parameters_test),
+ KUNIT_CASE(kernel_kernel_kernel_test),
+ KUNIT_CASE(kernel_kernel_module_test),
+ KUNIT_CASE(kernel_module_kernel_test),
+ KUNIT_CASE(kernel_module_module_test),
+ KUNIT_CASE(module_kernel_kernel_test),
+ KUNIT_CASE(module_kernel_module_test),
+ KUNIT_CASE(module_module_kernel_test),
+ KUNIT_CASE(module_module_module_test),
+ {}
+};
+
+static int ppc_static_call_test_init(struct kunit *test)
+{
+ static_module_toc = module_toc;
+ static_actual_toc = actual_toc;
+ module_toc = current_toc;
+
+ ppc_sc_kernel_toc_init();
+
+ return 0;
+}
+
+static void ppc_static_call_test_exit(struct kunit *test)
+{
+ module_toc = static_module_toc;
+ actual_toc = static_actual_toc;
+
+ ppc_sc_kernel_toc_exit();
+}
+
+static struct kunit_suite ppc_static_call_test_suite = {
+ .name = "ppc-static-call",
+ .test_cases = static_call_test_cases,
+ .init = ppc_static_call_test_init,
+ .exit = ppc_static_call_test_exit,
+};
+kunit_test_suite(ppc_static_call_test_suite);
+
+MODULE_AUTHOR("Benjamin Gray <bgray@linux.ibm.com>");
+MODULE_LICENSE("GPL");
--
2.37.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c
2022-09-01 5:58 ` [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c Benjamin Gray
@ 2022-09-01 6:50 ` Christophe Leroy
0 siblings, 0 replies; 11+ messages in thread
From: Christophe Leroy @ 2022-09-01 6:50 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, Peter Zijlstra, npiggin@gmail.com,
Ard Biesheuvel, Jason Baron, Steven Rostedt, Josh Poimboeuf
Le 01/09/2022 à 07:58, Benjamin Gray a écrit :
> These tests are out-of-line only, so moving them to the
> out-of-line file allows them to be run when an arch does
> not implement inline static calls.
These tests look like standalone code, would be better to have a new
static_call_selftest.c that is built when CONFIG_STATIC_CALL_SELFTEST is
set.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> kernel/static_call.c | 43 +++++++++++++++++++++++++++++++++++++
> kernel/static_call_inline.c | 43 -------------------------------------
> 2 files changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index e9c3e69f3837..953ec46e5691 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -6,3 +6,46 @@ long __static_call_return0(void)
> return 0;
> }
> EXPORT_SYMBOL_GPL(__static_call_return0);
> +
> +#ifdef CONFIG_STATIC_CALL_SELFTEST
> +
> +static int func_a(int x)
> +{
> + return x+1;
> +}
> +
> +static int func_b(int x)
> +{
> + return x+2;
> +}
> +
> +DEFINE_STATIC_CALL(sc_selftest, func_a);
> +
> +static struct static_call_data {
> + int (*func)(int);
> + int val;
> + int expect;
> +} static_call_data [] __initdata = {
> + { NULL, 2, 3 },
> + { func_b, 2, 4 },
> + { func_a, 2, 3 }
> +};
> +
> +static int __init test_static_call_init(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(static_call_data); i++ ) {
> + struct static_call_data *scd = &static_call_data[i];
> +
> + if (scd->func)
> + static_call_update(sc_selftest, scd->func);
> +
> + WARN_ON(static_call(sc_selftest)(scd->val) != scd->expect);
> + }
> +
> + return 0;
> +}
> +early_initcall(test_static_call_init);
> +
> +#endif /* CONFIG_STATIC_CALL_SELFTEST */
> diff --git a/kernel/static_call_inline.c b/kernel/static_call_inline.c
> index dc5665b62814..64d04d054698 100644
> --- a/kernel/static_call_inline.c
> +++ b/kernel/static_call_inline.c
> @@ -498,46 +498,3 @@ int __init static_call_init(void)
> return 0;
> }
> early_initcall(static_call_init);
> -
> -#ifdef CONFIG_STATIC_CALL_SELFTEST
> -
> -static int func_a(int x)
> -{
> - return x+1;
> -}
> -
> -static int func_b(int x)
> -{
> - return x+2;
> -}
> -
> -DEFINE_STATIC_CALL(sc_selftest, func_a);
> -
> -static struct static_call_data {
> - int (*func)(int);
> - int val;
> - int expect;
> -} static_call_data [] __initdata = {
> - { NULL, 2, 3 },
> - { func_b, 2, 4 },
> - { func_a, 2, 3 }
> -};
> -
> -static int __init test_static_call_init(void)
> -{
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(static_call_data); i++ ) {
> - struct static_call_data *scd = &static_call_data[i];
> -
> - if (scd->func)
> - static_call_update(sc_selftest, scd->func);
> -
> - WARN_ON(static_call(sc_selftest)(scd->val) != scd->expect);
> - }
> -
> - return 0;
> -}
> -early_initcall(test_static_call_init);
> -
> -#endif /* CONFIG_STATIC_CALL_SELFTEST */
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text
2022-09-01 5:58 ` [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text Benjamin Gray
@ 2022-09-01 7:01 ` Christophe Leroy
2022-09-06 1:53 ` Russell Currey
0 siblings, 1 reply; 11+ messages in thread
From: Christophe Leroy @ 2022-09-01 7:01 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, Peter Zijlstra, npiggin@gmail.com,
Ard Biesheuvel, Jason Baron, Steven Rostedt, Josh Poimboeuf
Le 01/09/2022 à 07:58, Benjamin Gray a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> powerpc allocates a text poke area of one page that is used by
> patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
> is enabled.
>
> patch_instruction() is only designed for instructions,
> so writing data using the text poke area can only happen 4 bytes
> at a time - each with a page map/unmap, pte flush and syncs.
>
> This patch introduces patch_memory(), implementing the same
> interface as memcpy(), similar to x86's text_poke() and s390's
> s390_kernel_write(). patch_memory() only needs to map the text
> poke area once, unless the write would cross a page boundary.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> arch/powerpc/include/asm/code-patching.h | 1 +
> arch/powerpc/lib/code-patching.c | 65 ++++++++++++++++++++++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 1c6316ec4b74..3de90748bce7 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -76,6 +76,7 @@ int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
> int patch_branch(u32 *addr, unsigned long target, int flags);
> int patch_instruction(u32 *addr, ppc_inst_t instr);
> int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> +void *patch_memory(void *dest, const void *src, size_t size);
>
> static inline unsigned long patch_site_addr(s32 *site)
> {
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 6edf0697a526..0cca39af44cb 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -14,6 +14,7 @@
> #include <asm/page.h>
> #include <asm/code-patching.h>
> #include <asm/inst.h>
> +#include <asm/cacheflush.h>
>
> static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr)
> {
> @@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
>
> return err;
> }
> +
> +static int do_patch_memory(void *dest, const void *src, size_t size)
> +{
> + int err;
> + unsigned long text_poke_addr, patch_addr;
> +
> + text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> +
> + err = map_patch_area(dest, text_poke_addr);
This is not in line with the optimisation done by
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220815114840.1468656-1-mpe@ellerman.id.au/
> + if (err)
> + return err;
> +
> + patch_addr = text_poke_addr + offset_in_page(dest);
> + copy_to_kernel_nofault((u8 *)patch_addr, src, size);
copy_to_kernel_nofault() has a performance cost.
> +
> + flush_icache_range(patch_addr, size);
Is that needed ? We are patching data, not text.
> + unmap_patch_area(text_poke_addr);
> +
> + return 0;
> +}
> +
> +/**
> + * patch_memory - write data using the text poke area
> + *
> + * @dest: destination address
> + * @src: source address
> + * @size: size in bytes
> + *
> + * like memcpy(), but using the text poke area. No atomicity guarantees.
> + * Do not use for instructions, use patch_instruction() instead.
> + * Handles crossing page boundaries, though you shouldn't need to.
> + *
> + * Return value:
> + * @dest
> + **/
> +void *patch_memory(void *dest, const void *src, size_t size)
> +{
> + int err;
> + unsigned long flags;
> + size_t written, write_size;
> +
> + // If the poke area isn't set up, it's early boot and we can just memcpy.
> + if (!this_cpu_read(text_poke_area))
> + return memcpy(dest, src, size);
> +
> + for (written = 0; written < size; written += write_size) {
> + // Write as much as possible without crossing a page boundary.
> + write_size = min_t(size_t, size - written,
> + PAGE_SIZE - offset_in_page(dest + written));
> +
> + local_irq_save(flags);
> + err = do_patch_memory(dest + written, src + written, write_size);
> + local_irq_restore(flags);
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + return dest;
> +}
> #else /* !CONFIG_STRICT_KERNEL_RWX */
>
> static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
> @@ -190,6 +250,11 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
> return raw_patch_instruction(addr, instr);
> }
>
> +void *patch_memory(void *dest, const void *src, size_t size)
> +{
> + return memcpy(dest, src, size);
In do_patch_memory() you have flush_icache_range(patch_addr, size);
If that's really needed there, why don't we need it here as well ?
> +}
> +
> #endif /* CONFIG_STRICT_KERNEL_RWX */
>
> __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls
2022-09-01 5:58 ` [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls Benjamin Gray
@ 2022-09-01 7:33 ` Christophe Leroy
0 siblings, 0 replies; 11+ messages in thread
From: Christophe Leroy @ 2022-09-01 7:33 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, Peter Zijlstra, npiggin@gmail.com,
Ard Biesheuvel, Jason Baron, Steven Rostedt, Josh Poimboeuf
Le 01/09/2022 à 07:58, Benjamin Gray a écrit :
> [Vous ne recevez pas souvent de courriers de bgray@linux.ibm.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Implement static call support for 64 bit V2 ABI. This requires
> making sure the TOC is kept correct across kernel-module
> boundaries. As a secondary concern, it tries to use the local
> entry point of a target wherever possible. It does so by
> checking if both tramp & target are kernel code, and falls
> back to detecting the common global entry point patterns
> if modules are involved. Detecting the global entry point is
> also required for setting the local entry point as the trampoline
> target: if we cannot detect the local entry point, then we need to
> convservatively initialise r12 and use the global entry point.
>
> The implementation is incorrect as-is, because a trampoline cannot
> use the stack. Doing so has the same issue described in 85baa095,
> where parameters passed relative to the stack pointer (large arg count
> or varargs) are broken. However the trampoline must guarantee the
> TOC be restored before the caller uses it again.
And the implementation as is wouldn't have much added value
performancewise because of the additional save/restore on the stack.
>
> Static call sites are themselves static, so it is feasible to handle
> this by patching the callsites. However the linker currently
> does not seem to realise that the trampoline treats r2 as volatile
> (even with an alternative trampoline that does not have a separate
> local entry point), and so does not insert the appropriate restoration
> at the call site.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> arch/powerpc/Kconfig | 13 +++-
> arch/powerpc/include/asm/code-patching.h | 1 +
> arch/powerpc/include/asm/static_call.h | 45 ++++++++++++-
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/static_call.c | 80 ++++++++++++++++++++++--
> 5 files changed, 132 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 4c466acdc70d..1d5abbeb2c40 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -248,7 +248,7 @@ config PPC
> select HAVE_SOFTIRQ_ON_OWN_STACK
> select HAVE_STACKPROTECTOR if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2)
> select HAVE_STACKPROTECTOR if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13)
> - select HAVE_STATIC_CALL if PPC32
> + select HAVE_STATIC_CALL if PPC_ENABLE_STATIC_CALL
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_VIRT_CPU_ACCOUNTING
> select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE
> @@ -1023,6 +1023,17 @@ config PPC_RTAS_FILTER
> Say Y unless you know what you are doing and the filter is causing
> problems for you.
>
> +config PPC_ENABLE_STATIC_CALL
> + bool "Enable static calls"
> + default y
> + depends on PPC32 || PPC64_ELF_ABI_V2
> + help
> + PowerPC static calls with the ELF V2 ABI are not as straightforward
> + as checking if the target is in range of a branch or not. They must
> + also ensure the TOC remains consistent. This leads to complex
> + performance results, so it's useful to make them configurable to
> + allow testing with the generic implementation too.
> +
Only as part of your RFC I guess, we shouldn't make it configurable at
the end.
> endmenu
>
> config ISA_DMA_API
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 3de90748bce7..319cb1eef71c 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -126,6 +126,7 @@ int translate_branch(ppc_inst_t *instr, const u32 *dest, const u32 *src);
> bool is_conditional_branch(ppc_inst_t instr);
>
> #define OP_RT_RA_MASK 0xffff0000UL
> +#define OP_SI_MASK 0x0000ffffUL
> #define LIS_R2 (PPC_RAW_LIS(_R2, 0))
> #define ADDIS_R2_R12 (PPC_RAW_ADDIS(_R2, _R12, 0))
> #define ADDI_R2_R2 (PPC_RAW_ADDI(_R2, _R2, 0))
> diff --git a/arch/powerpc/include/asm/static_call.h b/arch/powerpc/include/asm/static_call.h
> index de1018cc522b..d85ff3f88c8e 100644
> --- a/arch/powerpc/include/asm/static_call.h
> +++ b/arch/powerpc/include/asm/static_call.h
> @@ -2,12 +2,49 @@
> #ifndef _ASM_POWERPC_STATIC_CALL_H
> #define _ASM_POWERPC_STATIC_CALL_H
>
> +#if defined(CONFIG_PPC64_ELF_ABI_V2)
> +
> +#define __PPC_SCT(name, inst) \
> + asm(".pushsection .text, \"ax\" \n" \
> + ".align 6 \n" \
> + ".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
> + STATIC_CALL_TRAMP_STR(name) ": \n" \
> + " addis 2, 12, (.TOC.-" STATIC_CALL_TRAMP_STR(name) ")@ha \n" \
> + " addi 2, 2, (.TOC.-" STATIC_CALL_TRAMP_STR(name) ")@l \n" \
> + ".localentry " STATIC_CALL_TRAMP_STR(name) ", .-" STATIC_CALL_TRAMP_STR(name) "\n" \
> + " " inst " \n" \
> + " mflr 0 \n" \
> + " std 0, 16(1) \n" \
Use PPC_LR_STKOFF
> + " stdu 1, -32(1) \n" \
Is that correct ? For PPC64 we have PPC_MIN_STKFRM 112
> + " std 2, 24(1) \n" \
Use R2_STACK_OFFSET
> + " addis 12, 2, 2f@toc@ha \n" \
> + " ld 12, 2f@toc@l(12) \n" \
> + " mtctr 12 \n" \
> + " bctrl \n" \
> + " ld 2, 24(1) \n" \
> + " addi 1, 1, 32 \n" \
> + " ld 0, 16(1) \n" \
> + " mtlr 0 \n" \
> + " blr \n" \
> + "1: li 3, 0 \n" \
> + " blr \n" \
> + ".balign 8 \n" \
> + "2: .8byte 0 \n" \
> + ".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
> + ".size " STATIC_CALL_TRAMP_STR(name) ", . - " STATIC_CALL_TRAMP_STR(name) " \n" \
> + ".popsection \n")
> +
> +#define PPC_SCT_RET0 64 /* Offset of label 1 */
> +#define PPC_SCT_DATA 72 /* Offset of label 2 (aligned) */
> +
> +#elif defined(PPC32)
> +
> #define __PPC_SCT(name, inst) \
> asm(".pushsection .text, \"ax\" \n" \
> ".align 5 \n" \
> ".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
> STATIC_CALL_TRAMP_STR(name) ": \n" \
> - inst " \n" \
> + " " inst " \n" \
> " lis 12,2f@ha \n" \
> " lwz 12,2f@l(12) \n" \
> " mtctr 12 \n" \
> @@ -22,8 +59,12 @@
> #define PPC_SCT_RET0 20 /* Offset of label 1 */
> #define PPC_SCT_DATA 28 /* Offset of label 2 */
>
> +#else /* !CONFIG_PPC64_ELF_ABI_V2 && !PPC32 */
> +#error "Unsupported ABI"
> +#endif /* CONFIG_PPC64_ELF_ABI_V2 */
> +
> #define ARCH_DEFINE_STATIC_CALL_TRAMP(name, func) __PPC_SCT(name, "b " #func)
> #define ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name) __PPC_SCT(name, "blr")
> -#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name) __PPC_SCT(name, "b .+20")
> +#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name) __PPC_SCT(name, "b 1f")
>
> #endif /* _ASM_POWERPC_STATIC_CALL_H */
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 06d2d1f78f71..a30d0d0f5499 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -128,8 +128,9 @@ extra-y += vmlinux.lds
>
> obj-$(CONFIG_RELOCATABLE) += reloc_$(BITS).o
>
> -obj-$(CONFIG_PPC32) += entry_32.o setup_32.o early_32.o static_call.o
> +obj-$(CONFIG_PPC32) += entry_32.o setup_32.o early_32.o
> obj-$(CONFIG_PPC64) += dma-iommu.o iommu.o
> +obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o
> obj-$(CONFIG_KGDB) += kgdb.o
> obj-$(CONFIG_BOOTX_TEXT) += btext.o
> obj-$(CONFIG_SMP) += smp.o
> diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
> index 863a7aa24650..ed3bc361fdb0 100644
> --- a/arch/powerpc/kernel/static_call.c
> +++ b/arch/powerpc/kernel/static_call.c
> @@ -1,33 +1,101 @@
> // SPDX-License-Identifier: GPL-2.0
> +#include <asm/code-patching.h>
> +#include <linux/export.h>
> +#include <linux/kconfig.h>
> +#include <linux/kernel.h>
> #include <linux/memory.h>
> #include <linux/static_call.h>
> +#include <linux/syscalls.h>
>
> -#include <asm/code-patching.h>
> +#ifdef CONFIG_PPC64_ELF_ABI_V2
> +
> +static void* ppc_function_toc(u32 *func) {
> + u32 insn1 = *func;
> + u32 insn2 = *(func+1);
> + u64 si1 = sign_extend64((insn1 & OP_SI_MASK) << 16, 31);
> + u64 si2 = sign_extend64(insn2 & OP_SI_MASK, 15);
> + u64 addr = ((u64) func + si1) + si2;
> +
> + if ((((insn1 & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
> + ((insn1 & OP_RT_RA_MASK) == LIS_R2)) &&
> + ((insn2 & OP_RT_RA_MASK) == ADDI_R2_R2))
> + return (void*) addr;
> + else
> + return NULL;
> +}
> +
> +static bool shares_toc(void *func1, void *func2) {
> + void* func1_toc;
> + void* func2_toc;
> +
> + if (func1 == NULL || func2 == NULL)
> + return false;
> +
> + /* Assume the kernel only uses a single TOC */
> + if (core_kernel_text((unsigned long)func1) &&
> + core_kernel_text((unsigned long)func2))
> + return true;
> +
> + /* Fall back to calculating the TOC from common patterns
> + * if modules are involved
> + */
> + func1_toc = ppc_function_toc(func1);
> + func2_toc = ppc_function_toc(func2);
> + return func1_toc != NULL && func2_toc != NULL && (func1_toc == func2_toc);
> +}
> +
> +#endif /* CONFIG_PPC64_ELF_ABI_V2 */
>
> void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
> {
> int err;
> bool is_ret0 = (func == __static_call_return0);
> unsigned long target = (unsigned long)(is_ret0 ? tramp + PPC_SCT_RET0 : func);
> - bool is_short = is_offset_in_branch_range((long)target - (long)tramp);
> + bool is_short;
> + void* tramp_entry;
>
> if (!tramp)
> return;
>
> + tramp_entry = (void*)ppc_function_entry(tramp);
> +
> +#ifdef CONFIG_PPC64_ELF_ABI_V2
> + if (shares_toc(tramp, (void*)target)) {
> + /* Confirm that the local entry point is in range */
> + is_short = is_offset_in_branch_range(
> + (long)ppc_function_entry((void*)target) - (long)tramp_entry);
> + } else {
> + /* Combine out-of-range with not sharing a TOC. Though it's possible an
> + * out-of-range target shares a TOC, handling this separately complicates
> + * the trampoline. It's simpler to always use the global entry point
> + * in this case.
> + */
> + is_short = false;
> + }
> +#else /* !CONFIG_PPC64_ELF_ABI_V2 */
> + is_short = is_offset_in_branch_range((long)target - (long)tramp);
> +#endif /* CONFIG_PPC64_ELF_ABI_V2 */
> +
> mutex_lock(&text_mutex);
>
> if (func && !is_short) {
> - err = patch_instruction(tramp + PPC_SCT_DATA, ppc_inst(target));
> + /* This assumes that the update is atomic. The current implementation uses
> + * stw/std and the store location is aligned. A sync is issued by one of the
> + * patch_instruction/patch_branch functions below.
> + */
> + err = PTR_ERR_OR_ZERO(patch_memory(
> + tramp + PPC_SCT_DATA, &target, sizeof(target)));
> if (err)
> goto out;
> }
>
> if (!func)
> - err = patch_instruction(tramp, ppc_inst(PPC_RAW_BLR()));
> + err = patch_instruction(tramp_entry, ppc_inst(PPC_RAW_BLR()));
> else if (is_short)
> - err = patch_branch(tramp, target, 0);
> + err = patch_branch(tramp_entry, ppc_function_entry((void*) target), 0);
> else
> - err = patch_instruction(tramp, ppc_inst(PPC_RAW_NOP()));
> + err = patch_instruction(tramp_entry, ppc_inst(PPC_RAW_NOP()));
> +
> out:
> mutex_unlock(&text_mutex);
>
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
` (3 preceding siblings ...)
2022-09-01 5:58 ` [RFC PATCH 4/4] powerpc/64: Add tests " Benjamin Gray
@ 2022-09-01 8:07 ` Christophe Leroy
2022-09-13 3:31 ` Benjamin Gray
5 siblings, 0 replies; 11+ messages in thread
From: Christophe Leroy @ 2022-09-01 8:07 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, Peter Zijlstra, npiggin@gmail.com,
Ard Biesheuvel, Jason Baron, Steven Rostedt, Josh Poimboeuf
CCing static call maintainers/reviewers.
And note that my email address has changed to
christophe.leroy@csgroup.eu monthes ago.
Le 01/09/2022 à 07:58, Benjamin Gray a écrit :
> [Vous ne recevez pas souvent de courriers de bgray@linux.ibm.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> WIP implementation of out-of-line static calls for PowerPC 64-bit ELF V2 ABI.
> Static calls patch an indirect branch into a direct branch at runtime.
> Out-of-line specifically has a caller directly call a trampoline, and
> the trampoline gets patched to directly call the target. This current
> implementation has a known issue described in detail below, and is
> presented here for any comments or suggestions.
For a wider audience I recommend you to copy people from the core STATIC
BRANCH/CALL (see MAINTAINERS file)
>
> 64-bit ELF V2 specifies a table of contents (TOC) pointer stored in r2.
> Functions that use a TOC can use it to perform various operations
> relative to its value. When the caller and target use different TOCs,
> the static call implementation must ensure the TOC is kept consistent
> so that neither tries to use the other's TOC.
>
> However, while the trampoline can change the caller's TOC to the target's
> TOC, it cannot restore the caller's TOC when the target returns. For the
> trampoline to do this would require the target to return to the trampoline,
> and so the return address back to the caller would need to be saved to
> the stack. But the trampoline cannot move the stack because the target
> may be expecting parameters relative to the stack pointer (when registers
> are insufficient or varargs are used). And as static calls are usable in
> generic code, there can be no arch-specific restrictions on parameters
> that would sidestep this issue.
>
> Normally the TOC change issue is resolved by the caller, which will save
> and restore its TOC if necessary. For static calls though the caller
> sees the trampoline as a local function, so assumes it does not change
> the TOC and treats r2 as nonvolatile (no save & restore added).
>
> This is a simialar problem to that faced by livepatch. Static calls may have
> a few more options though, because the call is performed through a
> `static_call` macro, allowing annotation and insertion of inline assembly
> at every callsite.
>
> I can think of several possible solutions, but they are relatively complex:
>
> 1. Patching the callsites at runtime, as is done for inline static calls.
> This also requires some inline assembly to save `r2` to the TOC pointer
> Doubleword slot on the stack before each static call, as the caller may
> not have done so in its prologue. It should be easy to add though, because
> static calls are invoked through the `static_call` macro that can be
> modified appropriately. The runtime patching then modifies the trailing
> function call `nop` to restore this r2 value.
I'm working at implementing inline static calls for ppc32. Will copy you
next spin (If I don't forget).
>
> The patching itself can probably be done at compile time at kernel callsites.
>
> 2. Use the livepatch stack method of growing the base of the stack backwards.
> I haven't looked too closely at the implementation though, especially
> regarding how much room is available.
>
> The benefit of this method is that there can be zero overhead when the
> trampoline and target share a TOC. So the trampoline in kernel-only
> calls can still just be a single direct branch.
>
> 3. Remove the local entry point from the trampoline. This makes the trampoline
> less efficient, as it cannot assume r2 to be correct, but should at least
> cause the caller to automatically save and restore r2 without manual patching.
> From the ABI manual:
>
> > 2.2.1. Function Call Linkage Protocols
> > A function that uses a TOC pointer always has a separate local entry point
> > [...], and preserves r2 when called via its local entry point.
> >
> > 2.2.2.1. Register Roles
> > (a) Register r2 is nonvolatile with respect to calls between functions
> > in the same compilation unit, except under the conditions in footnote (b)
> > (b) Register r2 is volatile and available for use in a function that does not
> > use a TOC pointer and that does not guarantee that it preserves r2.
>
> So not having a local entry point implies not using a TOC pointer, which
> implies r2 is volatile if the trampoline does not guarantee that it preserves
> r2. However experimenting with such a trampoline showed the caller still did
> not preserve its TOC when necessary, even when the trampoline used instructions
> that wrote to r2. Possibly there's an attribute that can be used to mark the
> necessary info, but I could not find one.
>
Another possible solution (at least for kernel) is to restore r2 from
PACA instead of restoring it from the stack. So no worry whether the
caller stored it or not. Something similar is done by module code, see
comment before create_ftrace_stub()
>
> Benjamin Gray (3):
> static_call: Move static call selftest to static_call.c
> powerpc/64: Add support for out-of-line static calls
> powerpc/64: Add tests for out-of-line static calls
>
> Russell Currey (1):
> powerpc/code-patching: add patch_memory() for writing RO text
>
> arch/powerpc/Kconfig | 23 +-
> arch/powerpc/include/asm/code-patching.h | 2 +
> arch/powerpc/include/asm/static_call.h | 45 +++-
> arch/powerpc/kernel/Makefile | 4 +-
> arch/powerpc/kernel/static_call.c | 184 +++++++++++++++-
> arch/powerpc/kernel/static_call_test.c | 257 +++++++++++++++++++++++
> arch/powerpc/lib/code-patching.c | 65 ++++++
> kernel/static_call.c | 43 ++++
> kernel/static_call_inline.c | 43 ----
> 9 files changed, 613 insertions(+), 53 deletions(-)
> create mode 100644 arch/powerpc/kernel/static_call_test.c
>
>
> base-commit: c5e4d5e99162ba8025d58a3af7ad103f155d2df7
> --
> 2.37.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text
2022-09-01 7:01 ` Christophe Leroy
@ 2022-09-06 1:53 ` Russell Currey
0 siblings, 0 replies; 11+ messages in thread
From: Russell Currey @ 2022-09-06 1:53 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Gray, linuxppc-dev@lists.ozlabs.org
Cc: ajd@linux.ibm.com, Peter Zijlstra, Steven Rostedt, Ard Biesheuvel,
Jason Baron, npiggin@gmail.com, Josh Poimboeuf
On Thu, 2022-09-01 at 07:01 +0000, Christophe Leroy wrote:
>
>
> Le 01/09/2022 à 07:58, Benjamin Gray a écrit :
> > From: Russell Currey <ruscur@russell.cc>
> >
> > powerpc allocates a text poke area of one page that is used by
> > patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
> > is enabled.
> >
> > patch_instruction() is only designed for instructions,
> > so writing data using the text poke area can only happen 4 bytes
> > at a time - each with a page map/unmap, pte flush and syncs.
> >
> > This patch introduces patch_memory(), implementing the same
> > interface as memcpy(), similar to x86's text_poke() and s390's
> > s390_kernel_write(). patch_memory() only needs to map the text
> > poke area once, unless the write would cross a page boundary.
> >
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > ---
> > arch/powerpc/include/asm/code-patching.h | 1 +
> > arch/powerpc/lib/code-patching.c | 65
> > ++++++++++++++++++++++++
> > 2 files changed, 66 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/code-patching.h
> > b/arch/powerpc/include/asm/code-patching.h
> > index 1c6316ec4b74..3de90748bce7 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -76,6 +76,7 @@ int create_cond_branch(ppc_inst_t *instr, const
> > u32 *addr,
> > int patch_branch(u32 *addr, unsigned long target, int flags);
> > int patch_instruction(u32 *addr, ppc_inst_t instr);
> > int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> > +void *patch_memory(void *dest, const void *src, size_t size);
> >
> > static inline unsigned long patch_site_addr(s32 *site)
> > {
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index 6edf0697a526..0cca39af44cb 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -14,6 +14,7 @@
> > #include <asm/page.h>
> > #include <asm/code-patching.h>
> > #include <asm/inst.h>
> > +#include <asm/cacheflush.h>
> >
> > static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr,
> > u32 *patch_addr)
> > {
> > @@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr,
> > ppc_inst_t instr)
> >
> > return err;
> > }
> > +
> > +static int do_patch_memory(void *dest, const void *src, size_t
> > size)
> > +{
> > + int err;
> > + unsigned long text_poke_addr, patch_addr;
> > +
> > + text_poke_addr = (unsigned
> > long)__this_cpu_read(text_poke_area)->addr;
> > +
> > + err = map_patch_area(dest, text_poke_addr);
>
> This is not in line with the optimisation done by
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220815114840.1468656-1-mpe@ellerman.id.au/
This patch hasn't changed since last year, thanks for the pointer.
>
> > + if (err)
> > + return err;
> > +
> > + patch_addr = text_poke_addr + offset_in_page(dest);
> > + copy_to_kernel_nofault((u8 *)patch_addr, src, size);
>
> copy_to_kernel_nofault() has a performance cost.
>
> > +
> > + flush_icache_range(patch_addr, size);
>
> Is that needed ? We are patching data, not text.
It's necessary if it gets used to patch text, which it might. Maybe we
should add a variable and only flush if the caller thinks it's needed.
The comment below should be updated for that too.
> > + unmap_patch_area(text_poke_addr);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * patch_memory - write data using the text poke area
> > + *
> > + * @dest: destination address
> > + * @src: source address
> > + * @size: size in bytes
> > + *
> > + * like memcpy(), but using the text poke area. No atomicity
> > guarantees.
> > + * Do not use for instructions, use patch_instruction() instead.
> > + * Handles crossing page boundaries, though you shouldn't need to.
> > + *
> > + * Return value:
> > + * @dest
> > + **/
> > +void *patch_memory(void *dest, const void *src, size_t size)
> > +{
> > + int err;
> > + unsigned long flags;
> > + size_t written, write_size;
> > +
> > + // If the poke area isn't set up, it's early boot and we
> > can just memcpy.
> > + if (!this_cpu_read(text_poke_area))
> > + return memcpy(dest, src, size);
> > +
> > + for (written = 0; written < size; written += write_size) {
> > + // Write as much as possible without crossing a
> > page boundary.
> > + write_size = min_t(size_t, size - written,
> > + PAGE_SIZE - offset_in_page(dest
> > + written));
> > +
> > + local_irq_save(flags);
> > + err = do_patch_memory(dest + written, src +
> > written, write_size);
> > + local_irq_restore(flags);
> > + if (err)
> > + return ERR_PTR(err);
> > + }
> > +
> > + return dest;
> > +}
> > #else /* !CONFIG_STRICT_KERNEL_RWX */
> >
> > static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
> > @@ -190,6 +250,11 @@ static int do_patch_instruction(u32 *addr,
> > ppc_inst_t instr)
> > return raw_patch_instruction(addr, instr);
> > }
> >
> > +void *patch_memory(void *dest, const void *src, size_t size)
> > +{
> > + return memcpy(dest, src, size);
>
> In do_patch_memory() you have flush_icache_range(patch_addr, size);
>
> If that's really needed there, why don't we need it here as well ?
Good point. I might make the arguments
(void *dest, const void *src, size_t size, bool text)
and only do the icache flush if text is true.
>
> > +}
> > +
> > #endif /* CONFIG_STRICT_KERNEL_RWX */
> >
> > __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);
> > --
> > 2.37.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
` (4 preceding siblings ...)
2022-09-01 8:07 ` [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Christophe Leroy
@ 2022-09-13 3:31 ` Benjamin Gray
5 siblings, 0 replies; 11+ messages in thread
From: Benjamin Gray @ 2022-09-13 3:31 UTC (permalink / raw)
To: linuxppc-dev
On Thu, 2022-09-01 at 15:58 +1000, Benjamin Gray wrote:
> So not having a local entry point implies not using a TOC
> pointer, which
> implies r2 is volatile if the trampoline does not guarantee that
> it preserves
> r2. However experimenting with such a trampoline showed the
> caller still did
> not preserve its TOC when necessary, even when the trampoline
> used instructions
> that wrote to r2. Possibly there's an attribute that can be used
> to mark the
> necessary info, but I could not find one.
The `.localentry` directive is more general than just specifying where
the local entry is: it can be used to set the relevant ELF bits
directly. So the solution here is setting `.localentry NAME, 1` on the
SC trampoline.
It's not an optimal solution, as it inserts another trampoline to save
r2 before calling the SC trampoline, but it would allow a correct
implementation without the work needed in the other choices.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-09-13 3:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-01 5:58 [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
2022-09-01 5:58 ` [RFC PATCH 1/4] powerpc/code-patching: add patch_memory() for writing RO text Benjamin Gray
2022-09-01 7:01 ` Christophe Leroy
2022-09-06 1:53 ` Russell Currey
2022-09-01 5:58 ` [RFC PATCH 2/4] static_call: Move static call selftest to static_call.c Benjamin Gray
2022-09-01 6:50 ` Christophe Leroy
2022-09-01 5:58 ` [RFC PATCH 3/4] powerpc/64: Add support for out-of-line static calls Benjamin Gray
2022-09-01 7:33 ` Christophe Leroy
2022-09-01 5:58 ` [RFC PATCH 4/4] powerpc/64: Add tests " Benjamin Gray
2022-09-01 8:07 ` [RFC PATCH 0/4] Out-of-line static calls for powerpc64 ELF V2 Christophe Leroy
2022-09-13 3:31 ` Benjamin Gray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).