* [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator
@ 2023-10-20 14:13 Hari Bathini
2023-10-20 14:13 ` [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions() Hari Bathini
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Naveen N. Rao
Most BPF programs are small, but they consume a page each. For systems
with busy traffic and many BPF programs, this may also add significant
pressure on instruction TLB. High iTLB pressure usually slows down the
whole system causing visible performance degradation for production
workloads.
bpf_prog_pack, a customized allocator that packs multiple bpf programs
into preallocated memory chunks, was proposed [1] to address it. This
series extends this support on powerpc.
Both bpf_arch_text_copy() & bpf_arch_text_invalidate() functions,
needed for this support depend on instruction patching in text area.
Currently, patch_instruction() supports patching only one instruction
at a time. The first patch introduces patch_instructions() function
to enable patching more than one instruction at a time. This helps in
avoiding performance degradation while JITing bpf programs.
Patches 2 & 3 implement the above mentioned arch specific functions
using patch_instructions(). Patch 4 fixes a misnomer in bpf JITing
code. The last patch enables the use of BPF prog pack allocator on
powerpc and also, ensures cleanup is handled gracefully.
[1] https://lore.kernel.org/bpf/20220204185742.271030-1-song@kernel.org/
Changes in v7:
* Fixed crash observed with !STRICT_RWX.
Changes in v6:
* No changes in patches 2-5/5 except addition of Acked-by tags from Song.
* Skipped merging code path of patch_instruction() & patch_instructions()
to avoid performance overhead observed on ppc32 with that.
Changes in v5:
* Moved introduction of patch_instructions() as 1st patch in series.
* Improved patch_instructions() to use memset & memcpy.
* Fixed the misnomer in JITing code as a separate patch.
* Removed unused bpf_flush_icache() function.
Changes in v4:
* Updated bpf_patch_instructions() definition in patch 1/5 so that
it doesn't have to be updated again in patch 2/5.
* Addressed Christophe's comment on bpf_arch_text_invalidate() return
value in patch 2/5.
Changes in v3:
* Fixed segfault issue observed on ppc32 due to inaccurate offset
calculation for branching.
* Tried to minimize the performance impact for patch_instruction()
with the introduction of patch_instructions().
* Corrected uses of u32* vs ppc_instr_t.
* Moved the change that introduces patch_instructions() to after
enabling bpf_prog_pack support.
* Added few comments to improve code readability.
Changes in v2:
* Introduced patch_instructions() to help with patching bpf programs.
Hari Bathini (5):
powerpc/code-patching: introduce patch_instructions()
powerpc/bpf: implement bpf_arch_text_copy
powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack
powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data
powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free]
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 141 +++++++++++++++++++++-
arch/powerpc/net/bpf_jit.h | 18 +--
arch/powerpc/net/bpf_jit_comp.c | 145 ++++++++++++++++++-----
arch/powerpc/net/bpf_jit_comp32.c | 13 +-
arch/powerpc/net/bpf_jit_comp64.c | 10 +-
6 files changed, 271 insertions(+), 57 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions()
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
@ 2023-10-20 14:13 ` Hari Bathini
2023-10-30 13:02 ` Aneesh Kumar K.V
2023-10-20 14:13 ` [PATCH v7 2/5] powerpc/bpf: implement bpf_arch_text_copy Hari Bathini
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
patch_instruction() entails setting up pte, patching the instruction,
clearing the pte and flushing the tlb. If multiple instructions need
to be patched, every instruction would have to go through the above
drill unnecessarily. Instead, introduce patch_instructions() function
that sets up the pte, clears the pte and flushes the tlb only once
per page range of instructions to be patched. Duplicate most of the
patch_instruction() code instead of merging with it, to avoid the
performance degradation observed on ppc32, for patch_instruction(),
with the code path merged. Also, setup poking_init() always as BPF
expects poking_init() to be setup even when STRICT_KERNEL_RWX is off.
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Acked-by: Song Liu <song@kernel.org>
---
Changes in v7:
* Fixed crash observed with !STRICT_RWX.
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 141 ++++++++++++++++++++++-
2 files changed, 139 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 3f881548fb61..0e29ccf903d0 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -74,6 +74,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);
+int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr);
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 b00112d7ad46..e1c1fd9246d8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -204,9 +204,6 @@ void __init poking_init(void)
{
int ret;
- if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
- return;
-
if (mm_patch_enabled())
ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"powerpc/text_poke_mm:online",
@@ -378,6 +375,144 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
}
NOKPROBE_SYMBOL(patch_instruction);
+static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
+{
+ unsigned long start = (unsigned long)patch_addr;
+
+ /* Repeat instruction */
+ if (repeat_instr) {
+ ppc_inst_t instr = ppc_inst_read(code);
+
+ if (ppc_inst_prefixed(instr)) {
+ u64 val = ppc_inst_as_ulong(instr);
+
+ memset64((u64 *)patch_addr, val, len / 8);
+ } else {
+ u32 val = ppc_inst_val(instr);
+
+ memset32(patch_addr, val, len / 4);
+ }
+ } else {
+ memcpy(patch_addr, code, len);
+ }
+
+ smp_wmb(); /* smp write barrier */
+ flush_icache_range(start, start + len);
+ return 0;
+}
+
+/*
+ * A page is mapped and instructions that fit the page are patched.
+ * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
+ */
+static int __do_patch_instructions_mm(u32 *addr, u32 *code, size_t len, bool repeat_instr)
+{
+ struct mm_struct *patching_mm, *orig_mm;
+ unsigned long pfn = get_patch_pfn(addr);
+ unsigned long text_poke_addr;
+ spinlock_t *ptl;
+ u32 *patch_addr;
+ pte_t *pte;
+ int err;
+
+ patching_mm = __this_cpu_read(cpu_patching_context.mm);
+ text_poke_addr = __this_cpu_read(cpu_patching_context.addr);
+ patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
+
+ pte = get_locked_pte(patching_mm, text_poke_addr, &ptl);
+ if (!pte)
+ return -ENOMEM;
+
+ __set_pte_at(patching_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
+
+ /* order PTE update before use, also serves as the hwsync */
+ asm volatile("ptesync" ::: "memory");
+
+ /* order context switch after arbitrary prior code */
+ isync();
+
+ orig_mm = start_using_temp_mm(patching_mm);
+
+ err = __patch_instructions(patch_addr, code, len, repeat_instr);
+
+ /* context synchronisation performed by __patch_instructions */
+ stop_using_temp_mm(patching_mm, orig_mm);
+
+ pte_clear(patching_mm, text_poke_addr, pte);
+ /*
+ * ptesync to order PTE update before TLB invalidation done
+ * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
+ */
+ local_flush_tlb_page_psize(patching_mm, text_poke_addr, mmu_virtual_psize);
+
+ pte_unmap_unlock(pte, ptl);
+
+ return err;
+}
+
+/*
+ * A page is mapped and instructions that fit the page are patched.
+ * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
+ */
+static int __do_patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
+{
+ unsigned long pfn = get_patch_pfn(addr);
+ unsigned long text_poke_addr;
+ u32 *patch_addr;
+ pte_t *pte;
+ int err;
+
+ text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
+ patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
+
+ pte = __this_cpu_read(cpu_patching_context.pte);
+ __set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
+ /* See ptesync comment in radix__set_pte_at() */
+ if (radix_enabled())
+ asm volatile("ptesync" ::: "memory");
+
+ err = __patch_instructions(patch_addr, code, len, repeat_instr);
+
+ pte_clear(&init_mm, text_poke_addr, pte);
+ flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+
+ return err;
+}
+
+/*
+ * Patch 'addr' with 'len' bytes of instructions from 'code'.
+ *
+ * If repeat_instr is true, the same instruction is filled for
+ * 'len' bytes.
+ */
+int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
+{
+ while (len > 0) {
+ unsigned long flags;
+ size_t plen;
+ int err;
+
+ plen = min_t(size_t, PAGE_SIZE - offset_in_page(addr), len);
+
+ local_irq_save(flags);
+ if (mm_patch_enabled())
+ err = __do_patch_instructions_mm(addr, code, plen, repeat_instr);
+ else
+ err = __do_patch_instructions(addr, code, plen, repeat_instr);
+ local_irq_restore(flags);
+ if (err)
+ return err;
+
+ len -= plen;
+ addr = (u32 *)((unsigned long)addr + plen);
+ if (!repeat_instr)
+ code = (u32 *)((unsigned long)code + plen);
+ }
+
+ return 0;
+}
+NOKPROBE_SYMBOL(patch_instructions);
+
int patch_branch(u32 *addr, unsigned long target, int flags)
{
ppc_inst_t instr;
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v7 2/5] powerpc/bpf: implement bpf_arch_text_copy
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
2023-10-20 14:13 ` [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions() Hari Bathini
@ 2023-10-20 14:13 ` Hari Bathini
2023-10-20 14:13 ` [PATCH v7 3/5] powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack Hari Bathini
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
bpf_arch_text_copy is used to dump JITed binary to RX page, allowing
multiple BPF programs to share the same page. Use the newly introduced
patch_instructions() to implement it.
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Acked-by: Song Liu <song@kernel.org>
---
* No changes in v7.
arch/powerpc/net/bpf_jit_comp.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 37043dfc1add..c740eac8d584 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -13,9 +13,13 @@
#include <linux/netdevice.h>
#include <linux/filter.h>
#include <linux/if_vlan.h>
-#include <asm/kprobes.h>
+#include <linux/kernel.h>
+#include <linux/memory.h>
#include <linux/bpf.h>
+#include <asm/kprobes.h>
+#include <asm/code-patching.h>
+
#include "bpf_jit.h"
static void bpf_jit_fill_ill_insns(void *area, unsigned int size)
@@ -274,3 +278,17 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, int pass, struct code
ctx->exentry_idx++;
return 0;
}
+
+void *bpf_arch_text_copy(void *dst, void *src, size_t len)
+{
+ int err;
+
+ if (WARN_ON_ONCE(core_kernel_text((unsigned long)dst)))
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&text_mutex);
+ err = patch_instructions(dst, src, len, false);
+ mutex_unlock(&text_mutex);
+
+ return err ? ERR_PTR(err) : dst;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v7 3/5] powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
2023-10-20 14:13 ` [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions() Hari Bathini
2023-10-20 14:13 ` [PATCH v7 2/5] powerpc/bpf: implement bpf_arch_text_copy Hari Bathini
@ 2023-10-20 14:13 ` Hari Bathini
2023-10-20 14:13 ` [PATCH v7 4/5] powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data Hari Bathini
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
Implement bpf_arch_text_invalidate and use it to fill unused part of
the bpf_prog_pack with trap instructions when a BPF program is freed.
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Acked-by: Song Liu <song@kernel.org>
---
* No changes in v7.
arch/powerpc/net/bpf_jit_comp.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index c740eac8d584..ecd7cffbbe28 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -292,3 +292,18 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len)
return err ? ERR_PTR(err) : dst;
}
+
+int bpf_arch_text_invalidate(void *dst, size_t len)
+{
+ u32 insn = BREAKPOINT_INSTRUCTION;
+ int ret;
+
+ if (WARN_ON_ONCE(core_kernel_text((unsigned long)dst)))
+ return -EINVAL;
+
+ mutex_lock(&text_mutex);
+ ret = patch_instructions(dst, &insn, len, true);
+ mutex_unlock(&text_mutex);
+
+ return ret;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v7 4/5] powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
` (2 preceding siblings ...)
2023-10-20 14:13 ` [PATCH v7 3/5] powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack Hari Bathini
@ 2023-10-20 14:13 ` Hari Bathini
2023-10-20 14:13 ` [PATCH v7 5/5] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free] Hari Bathini
2023-10-27 9:59 ` [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
powerpc64_jit_data is a misnomer as it is meant for both ppc32 and
ppc64. Rename it to powerpc_jit_data.
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Acked-by: Song Liu <song@kernel.org>
---
* No changes in v7.
arch/powerpc/net/bpf_jit_comp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index ecd7cffbbe28..e7ca270a39d5 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -43,7 +43,7 @@ int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg,
return 0;
}
-struct powerpc64_jit_data {
+struct powerpc_jit_data {
struct bpf_binary_header *header;
u32 *addrs;
u8 *image;
@@ -63,7 +63,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
u8 *image = NULL;
u32 *code_base;
u32 *addrs;
- struct powerpc64_jit_data *jit_data;
+ struct powerpc_jit_data *jit_data;
struct codegen_context cgctx;
int pass;
int flen;
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v7 5/5] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free]
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
` (3 preceding siblings ...)
2023-10-20 14:13 ` [PATCH v7 4/5] powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data Hari Bathini
@ 2023-10-20 14:13 ` Hari Bathini
2023-10-27 9:59 ` [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-20 14:13 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
Use bpf_jit_binary_pack_alloc in powerpc jit. The jit engine first
writes the program to the rw buffer. When the jit is done, the program
is copied to the final location with bpf_jit_binary_pack_finalize.
With multiple jit_subprogs, bpf_jit_free is called on some subprograms
that haven't got bpf_jit_binary_pack_finalize() yet. Implement custom
bpf_jit_free() like in commit 1d5f82d9dd47 ("bpf, x86: fix freeing of
not-finalized bpf_prog_pack") to call bpf_jit_binary_pack_finalize(),
if necessary. As bpf_flush_icache() is not needed anymore, remove it.
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Acked-by: Song Liu <song@kernel.org>
---
* No changes in v7.
arch/powerpc/net/bpf_jit.h | 18 ++---
arch/powerpc/net/bpf_jit_comp.c | 106 ++++++++++++++++++++++--------
arch/powerpc/net/bpf_jit_comp32.c | 13 ++--
arch/powerpc/net/bpf_jit_comp64.c | 10 +--
4 files changed, 96 insertions(+), 51 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 72b7bb34fade..cdea5dccaefe 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -36,9 +36,6 @@
EMIT(PPC_RAW_BRANCH(offset)); \
} while (0)
-/* bl (unconditional 'branch' with link) */
-#define PPC_BL(dest) EMIT(PPC_RAW_BL((dest) - (unsigned long)(image + ctx->idx)))
-
/* "cond" here covers BO:BI fields. */
#define PPC_BCC_SHORT(cond, dest) \
do { \
@@ -147,12 +144,6 @@ struct codegen_context {
#define BPF_FIXUP_LEN 2 /* Two instructions => 8 bytes */
#endif
-static inline void bpf_flush_icache(void *start, void *end)
-{
- smp_wmb(); /* smp write barrier */
- flush_icache_range((unsigned long)start, (unsigned long)end);
-}
-
static inline bool bpf_is_seen_register(struct codegen_context *ctx, int i)
{
return ctx->seen & (1 << (31 - i));
@@ -169,16 +160,17 @@ static inline void bpf_clear_seen_register(struct codegen_context *ctx, int i)
}
void bpf_jit_init_reg_mapping(struct codegen_context *ctx);
-int bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 func);
-int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *ctx,
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func);
+int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
u32 *addrs, int pass, bool extra_pass);
void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
void bpf_jit_realloc_regs(struct codegen_context *ctx);
int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, long exit_addr);
-int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, int pass, struct codegen_context *ctx,
- int insn_idx, int jmp_off, int dst_reg);
+int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int pass,
+ struct codegen_context *ctx, int insn_idx,
+ int jmp_off, int dst_reg);
#endif
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index e7ca270a39d5..a79d7c478074 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -44,9 +44,12 @@ int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg,
}
struct powerpc_jit_data {
- struct bpf_binary_header *header;
+ /* address of rw header */
+ struct bpf_binary_header *hdr;
+ /* address of ro final header */
+ struct bpf_binary_header *fhdr;
u32 *addrs;
- u8 *image;
+ u8 *fimage;
u32 proglen;
struct codegen_context ctx;
};
@@ -67,11 +70,14 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
struct codegen_context cgctx;
int pass;
int flen;
- struct bpf_binary_header *bpf_hdr;
+ struct bpf_binary_header *fhdr = NULL;
+ struct bpf_binary_header *hdr = NULL;
struct bpf_prog *org_fp = fp;
struct bpf_prog *tmp_fp;
bool bpf_blinded = false;
bool extra_pass = false;
+ u8 *fimage = NULL;
+ u32 *fcode_base;
u32 extable_len;
u32 fixup_len;
@@ -101,9 +107,16 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
addrs = jit_data->addrs;
if (addrs) {
cgctx = jit_data->ctx;
- image = jit_data->image;
- bpf_hdr = jit_data->header;
+ /*
+ * JIT compiled to a writable location (image/code_base) first.
+ * It is then moved to the readonly final location (fimage/fcode_base)
+ * using instruction patching.
+ */
+ fimage = jit_data->fimage;
+ fhdr = jit_data->fhdr;
proglen = jit_data->proglen;
+ hdr = jit_data->hdr;
+ image = (void *)hdr + ((void *)fimage - (void *)fhdr);
extra_pass = true;
/* During extra pass, ensure index is reset before repopulating extable entries */
cgctx.exentry_idx = 0;
@@ -123,7 +136,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
cgctx.stack_size = round_up(fp->aux->stack_depth, 16);
/* Scouting faux-generate pass 0 */
- if (bpf_jit_build_body(fp, 0, &cgctx, addrs, 0, false)) {
+ if (bpf_jit_build_body(fp, NULL, NULL, &cgctx, addrs, 0, false)) {
/* We hit something illegal or unsupported. */
fp = org_fp;
goto out_addrs;
@@ -138,7 +151,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
*/
if (cgctx.seen & SEEN_TAILCALL || !is_offset_in_branch_range((long)cgctx.idx * 4)) {
cgctx.idx = 0;
- if (bpf_jit_build_body(fp, 0, &cgctx, addrs, 0, false)) {
+ if (bpf_jit_build_body(fp, NULL, NULL, &cgctx, addrs, 0, false)) {
fp = org_fp;
goto out_addrs;
}
@@ -160,17 +173,19 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
proglen = cgctx.idx * 4;
alloclen = proglen + FUNCTION_DESCR_SIZE + fixup_len + extable_len;
- bpf_hdr = bpf_jit_binary_alloc(alloclen, &image, 4, bpf_jit_fill_ill_insns);
- if (!bpf_hdr) {
+ fhdr = bpf_jit_binary_pack_alloc(alloclen, &fimage, 4, &hdr, &image,
+ bpf_jit_fill_ill_insns);
+ if (!fhdr) {
fp = org_fp;
goto out_addrs;
}
if (extable_len)
- fp->aux->extable = (void *)image + FUNCTION_DESCR_SIZE + proglen + fixup_len;
+ fp->aux->extable = (void *)fimage + FUNCTION_DESCR_SIZE + proglen + fixup_len;
skip_init_ctx:
code_base = (u32 *)(image + FUNCTION_DESCR_SIZE);
+ fcode_base = (u32 *)(fimage + FUNCTION_DESCR_SIZE);
/* Code generation passes 1-2 */
for (pass = 1; pass < 3; pass++) {
@@ -178,8 +193,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
cgctx.idx = 0;
cgctx.alt_exit_addr = 0;
bpf_jit_build_prologue(code_base, &cgctx);
- if (bpf_jit_build_body(fp, code_base, &cgctx, addrs, pass, extra_pass)) {
- bpf_jit_binary_free(bpf_hdr);
+ if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
+ extra_pass)) {
+ bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
+ bpf_jit_binary_pack_free(fhdr, hdr);
fp = org_fp;
goto out_addrs;
}
@@ -199,17 +216,19 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
#ifdef CONFIG_PPC64_ELF_ABI_V1
/* Function descriptor nastiness: Address + TOC */
- ((u64 *)image)[0] = (u64)code_base;
+ ((u64 *)image)[0] = (u64)fcode_base;
((u64 *)image)[1] = local_paca->kernel_toc;
#endif
- fp->bpf_func = (void *)image;
+ fp->bpf_func = (void *)fimage;
fp->jited = 1;
fp->jited_len = proglen + FUNCTION_DESCR_SIZE;
- bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + bpf_hdr->size);
if (!fp->is_func || extra_pass) {
- bpf_jit_binary_lock_ro(bpf_hdr);
+ if (bpf_jit_binary_pack_finalize(fp, fhdr, hdr)) {
+ fp = org_fp;
+ goto out_addrs;
+ }
bpf_prog_fill_jited_linfo(fp, addrs);
out_addrs:
kfree(addrs);
@@ -219,8 +238,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
jit_data->addrs = addrs;
jit_data->ctx = cgctx;
jit_data->proglen = proglen;
- jit_data->image = image;
- jit_data->header = bpf_hdr;
+ jit_data->fimage = fimage;
+ jit_data->fhdr = fhdr;
+ jit_data->hdr = hdr;
}
out:
@@ -234,12 +254,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
* The caller should check for (BPF_MODE(code) == BPF_PROBE_MEM) before calling
* this function, as this only applies to BPF_PROBE_MEM, for now.
*/
-int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, int pass, struct codegen_context *ctx,
- int insn_idx, int jmp_off, int dst_reg)
+int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, u32 *fimage, int pass,
+ struct codegen_context *ctx, int insn_idx, int jmp_off,
+ int dst_reg)
{
off_t offset;
unsigned long pc;
- struct exception_table_entry *ex;
+ struct exception_table_entry *ex, *ex_entry;
u32 *fixup;
/* Populate extable entries only in the last pass */
@@ -250,9 +271,16 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, int pass, struct code
WARN_ON_ONCE(ctx->exentry_idx >= fp->aux->num_exentries))
return -EINVAL;
+ /*
+ * Program is first written to image before copying to the
+ * final location (fimage). Accordingly, update in the image first.
+ * As all offsets used are relative, copying as is to the
+ * final location should be alright.
+ */
pc = (unsigned long)&image[insn_idx];
+ ex = (void *)fp->aux->extable - (void *)fimage + (void *)image;
- fixup = (void *)fp->aux->extable -
+ fixup = (void *)ex -
(fp->aux->num_exentries * BPF_FIXUP_LEN * 4) +
(ctx->exentry_idx * BPF_FIXUP_LEN * 4);
@@ -263,17 +291,17 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32 *image, int pass, struct code
fixup[BPF_FIXUP_LEN - 1] =
PPC_RAW_BRANCH((long)(pc + jmp_off) - (long)&fixup[BPF_FIXUP_LEN - 1]);
- ex = &fp->aux->extable[ctx->exentry_idx];
+ ex_entry = &ex[ctx->exentry_idx];
- offset = pc - (long)&ex->insn;
+ offset = pc - (long)&ex_entry->insn;
if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
return -ERANGE;
- ex->insn = offset;
+ ex_entry->insn = offset;
- offset = (long)fixup - (long)&ex->fixup;
+ offset = (long)fixup - (long)&ex_entry->fixup;
if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
return -ERANGE;
- ex->fixup = offset;
+ ex_entry->fixup = offset;
ctx->exentry_idx++;
return 0;
@@ -307,3 +335,27 @@ int bpf_arch_text_invalidate(void *dst, size_t len)
return ret;
}
+
+void bpf_jit_free(struct bpf_prog *fp)
+{
+ if (fp->jited) {
+ struct powerpc_jit_data *jit_data = fp->aux->jit_data;
+ struct bpf_binary_header *hdr;
+
+ /*
+ * If we fail the final pass of JIT (from jit_subprogs),
+ * the program may not be finalized yet. Call finalize here
+ * before freeing it.
+ */
+ if (jit_data) {
+ bpf_jit_binary_pack_finalize(fp, jit_data->fhdr, jit_data->hdr);
+ kvfree(jit_data->addrs);
+ kfree(jit_data);
+ }
+ hdr = bpf_jit_binary_pack_hdr(fp);
+ bpf_jit_binary_pack_free(hdr, NULL);
+ WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(fp));
+ }
+
+ bpf_prog_unlock_free(fp);
+}
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index 7f91ea064c08..434417c755fd 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -200,12 +200,13 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
EMIT(PPC_RAW_BLR());
}
-int bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 func)
+/* Relative offset needs to be calculated based on final image location */
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
{
- s32 rel = (s32)func - (s32)(image + ctx->idx);
+ s32 rel = (s32)func - (s32)(fimage + ctx->idx);
if (image && rel < 0x2000000 && rel >= -0x2000000) {
- PPC_BL(func);
+ EMIT(PPC_RAW_BL(rel));
} else {
/* Load function address into r0 */
EMIT(PPC_RAW_LIS(_R0, IMM_H(func)));
@@ -278,7 +279,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
}
/* Assemble the body code between the prologue & epilogue */
-int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *ctx,
+int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
u32 *addrs, int pass, bool extra_pass)
{
const struct bpf_insn *insn = fp->insnsi;
@@ -997,7 +998,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
jmp_off += 4;
}
- ret = bpf_add_extable_entry(fp, image, pass, ctx, insn_idx,
+ ret = bpf_add_extable_entry(fp, image, fimage, pass, ctx, insn_idx,
jmp_off, dst_reg);
if (ret)
return ret;
@@ -1053,7 +1054,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_5), _R1, 12));
}
- ret = bpf_jit_emit_func_call_rel(image, ctx, func_addr);
+ ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
if (ret)
return ret;
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 0f8048f6dad6..79f23974a320 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -240,7 +240,7 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
return 0;
}
-int bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 func)
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
{
unsigned int i, ctx_idx = ctx->idx;
@@ -361,7 +361,7 @@ asm (
);
/* Assemble the body code between the prologue & epilogue */
-int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *ctx,
+int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
u32 *addrs, int pass, bool extra_pass)
{
enum stf_barrier_type stf_barrier = stf_barrier_type_get();
@@ -940,8 +940,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
addrs[++i] = ctx->idx * 4;
if (BPF_MODE(code) == BPF_PROBE_MEM) {
- ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1,
- 4, dst_reg);
+ ret = bpf_add_extable_entry(fp, image, fimage, pass, ctx,
+ ctx->idx - 1, 4, dst_reg);
if (ret)
return ret;
}
@@ -995,7 +995,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
if (func_addr_fixed)
ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr);
else
- ret = bpf_jit_emit_func_call_rel(image, ctx, func_addr);
+ ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
if (ret)
return ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
` (4 preceding siblings ...)
2023-10-20 14:13 ` [PATCH v7 5/5] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free] Hari Bathini
@ 2023-10-27 9:59 ` Michael Ellerman
5 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2023-10-27 9:59 UTC (permalink / raw)
To: linuxppc-dev, bpf, Hari Bathini
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Naveen N. Rao
On Fri, 20 Oct 2023 19:43:53 +0530, Hari Bathini wrote:
> Most BPF programs are small, but they consume a page each. For systems
> with busy traffic and many BPF programs, this may also add significant
> pressure on instruction TLB. High iTLB pressure usually slows down the
> whole system causing visible performance degradation for production
> workloads.
>
> bpf_prog_pack, a customized allocator that packs multiple bpf programs
> into preallocated memory chunks, was proposed [1] to address it. This
> series extends this support on powerpc.
>
> [...]
Applied to powerpc/next.
[1/5] powerpc/code-patching: introduce patch_instructions()
https://git.kernel.org/powerpc/c/465cabc97b42405eb89380ea6ba8d8b03e4ae1a2
[2/5] powerpc/bpf: implement bpf_arch_text_copy
https://git.kernel.org/powerpc/c/6efc1675acb88eef45ef0156b93f95d66a8ee759
[3/5] powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack
https://git.kernel.org/powerpc/c/033ffaf0af1f974ecf401db3f70aae6fe1a90fc5
[4/5] powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data
https://git.kernel.org/powerpc/c/de04e40600ae15fa5e484be242e74aad6de7418f
[5/5] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free]
https://git.kernel.org/powerpc/c/90d862f370b6e9de1b5d607843c5a2f9823990f3
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions()
2023-10-20 14:13 ` [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions() Hari Bathini
@ 2023-10-30 13:02 ` Aneesh Kumar K.V
2023-10-30 18:56 ` Hari Bathini
0 siblings, 1 reply; 9+ messages in thread
From: Aneesh Kumar K.V @ 2023-10-30 13:02 UTC (permalink / raw)
To: Hari Bathini, linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
Hari Bathini <hbathini@linux.ibm.com> writes:
> patch_instruction() entails setting up pte, patching the instruction,
> clearing the pte and flushing the tlb. If multiple instructions need
> to be patched, every instruction would have to go through the above
> drill unnecessarily. Instead, introduce patch_instructions() function
> that sets up the pte, clears the pte and flushes the tlb only once
> per page range of instructions to be patched. Duplicate most of the
> patch_instruction() code instead of merging with it, to avoid the
> performance degradation observed on ppc32, for patch_instruction(),
> with the code path merged. Also, setup poking_init() always as BPF
> expects poking_init() to be setup even when STRICT_KERNEL_RWX is off.
>
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> Acked-by: Song Liu <song@kernel.org>
>
A lot of this is duplicate of patch_instruction(). Can we consolidate
thing between them?
> ---
>
> Changes in v7:
> * Fixed crash observed with !STRICT_RWX.
>
>
> arch/powerpc/include/asm/code-patching.h | 1 +
> arch/powerpc/lib/code-patching.c | 141 ++++++++++++++++++++++-
> 2 files changed, 139 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 3f881548fb61..0e29ccf903d0 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -74,6 +74,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);
> +int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr);
>
> 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 b00112d7ad46..e1c1fd9246d8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -204,9 +204,6 @@ void __init poking_init(void)
> {
> int ret;
>
> - if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> - return;
> -
> if (mm_patch_enabled())
> ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "powerpc/text_poke_mm:online",
> @@ -378,6 +375,144 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
> }
> NOKPROBE_SYMBOL(patch_instruction);
>
> +static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
> +{
> + unsigned long start = (unsigned long)patch_addr;
> +
> + /* Repeat instruction */
> + if (repeat_instr) {
> + ppc_inst_t instr = ppc_inst_read(code);
> +
> + if (ppc_inst_prefixed(instr)) {
> + u64 val = ppc_inst_as_ulong(instr);
> +
> + memset64((u64 *)patch_addr, val, len / 8);
> + } else {
> + u32 val = ppc_inst_val(instr);
> +
> + memset32(patch_addr, val, len / 4);
> + }
> + } else {
> + memcpy(patch_addr, code, len);
> + }
> +
> + smp_wmb(); /* smp write barrier */
> + flush_icache_range(start, start + len);
> + return 0;
> +}
> +
> +/*
> + * A page is mapped and instructions that fit the page are patched.
> + * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
> + */
> +static int __do_patch_instructions_mm(u32 *addr, u32 *code, size_t len, bool repeat_instr)
> +{
> + struct mm_struct *patching_mm, *orig_mm;
> + unsigned long pfn = get_patch_pfn(addr);
> + unsigned long text_poke_addr;
> + spinlock_t *ptl;
> + u32 *patch_addr;
> + pte_t *pte;
> + int err;
> +
> + patching_mm = __this_cpu_read(cpu_patching_context.mm);
> + text_poke_addr = __this_cpu_read(cpu_patching_context.addr);
> + patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
> +
> + pte = get_locked_pte(patching_mm, text_poke_addr, &ptl);
> + if (!pte)
> + return -ENOMEM;
> +
> + __set_pte_at(patching_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
> +
> + /* order PTE update before use, also serves as the hwsync */
> + asm volatile("ptesync" ::: "memory");
> +
> + /* order context switch after arbitrary prior code */
> + isync();
> +
> + orig_mm = start_using_temp_mm(patching_mm);
> +
> + err = __patch_instructions(patch_addr, code, len, repeat_instr);
> +
> + /* context synchronisation performed by __patch_instructions */
> + stop_using_temp_mm(patching_mm, orig_mm);
> +
> + pte_clear(patching_mm, text_poke_addr, pte);
> + /*
> + * ptesync to order PTE update before TLB invalidation done
> + * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
> + */
> + local_flush_tlb_page_psize(patching_mm, text_poke_addr, mmu_virtual_psize);
> +
> + pte_unmap_unlock(pte, ptl);
> +
> + return err;
> +}
> +
> +/*
> + * A page is mapped and instructions that fit the page are patched.
> + * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
> + */
> +static int __do_patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
> +{
> + unsigned long pfn = get_patch_pfn(addr);
> + unsigned long text_poke_addr;
> + u32 *patch_addr;
> + pte_t *pte;
> + int err;
> +
> + text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
> + patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
> +
> + pte = __this_cpu_read(cpu_patching_context.pte);
> + __set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
> + /* See ptesync comment in radix__set_pte_at() */
> + if (radix_enabled())
> + asm volatile("ptesync" ::: "memory");
> +
> + err = __patch_instructions(patch_addr, code, len, repeat_instr);
> +
> + pte_clear(&init_mm, text_poke_addr, pte);
> + flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
> +
> + return err;
> +}
> +
> +/*
> + * Patch 'addr' with 'len' bytes of instructions from 'code'.
> + *
> + * If repeat_instr is true, the same instruction is filled for
> + * 'len' bytes.
> + */
> +int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
> +{
Will this break with prefix instructions?
> + while (len > 0) {
> + unsigned long flags;
> + size_t plen;
> + int err;
> +
> + plen = min_t(size_t, PAGE_SIZE - offset_in_page(addr), len);
> +
> + local_irq_save(flags);
> + if (mm_patch_enabled())
> + err = __do_patch_instructions_mm(addr, code, plen, repeat_instr);
> + else
> + err = __do_patch_instructions(addr, code, plen, repeat_instr);
> + local_irq_restore(flags);
> + if (err)
> + return err;
> +
> + len -= plen;
> + addr = (u32 *)((unsigned long)addr + plen);
> + if (!repeat_instr)
> + code = (u32 *)((unsigned long)code + plen);
> + }
> +
> + return 0;
> +}
> +NOKPROBE_SYMBOL(patch_instructions);
> +
> int patch_branch(u32 *addr, unsigned long target, int flags)
> {
> ppc_inst_t instr;
> --
> 2.41.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions()
2023-10-30 13:02 ` Aneesh Kumar K.V
@ 2023-10-30 18:56 ` Hari Bathini
0 siblings, 0 replies; 9+ messages in thread
From: Hari Bathini @ 2023-10-30 18:56 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Naveen N. Rao
Hi Aneesh,
On 30/10/23 6:32 pm, Aneesh Kumar K.V wrote:
> Hari Bathini <hbathini@linux.ibm.com> writes:
>
>> patch_instruction() entails setting up pte, patching the instruction,
>> clearing the pte and flushing the tlb. If multiple instructions need
>> to be patched, every instruction would have to go through the above
>> drill unnecessarily. Instead, introduce patch_instructions() function
>> that sets up the pte, clears the pte and flushes the tlb only once
>> per page range of instructions to be patched. Duplicate most of the
>> patch_instruction() code instead of merging with it, to avoid the
>> performance degradation observed on ppc32, for patch_instruction(),
>> with the code path merged. Also, setup poking_init() always as BPF
>> expects poking_init() to be setup even when STRICT_KERNEL_RWX is off.
>>
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> Acked-by: Song Liu <song@kernel.org>
>>
>
> A lot of this is duplicate of patch_instruction(). Can we consolidate
> thing between them?
True. The code was consolidated till v5 but had to duplicate most of it
to avoid performance degradation reported on ppc32:
https://lore.kernel.org/all/6cceb564-8b52-4d98-9118-92a914f4871e@csgroup.eu/
>
>> ---
>>
>> Changes in v7:
>> * Fixed crash observed with !STRICT_RWX.
>>
>>
>> arch/powerpc/include/asm/code-patching.h | 1 +
>> arch/powerpc/lib/code-patching.c | 141 ++++++++++++++++++++++-
>> 2 files changed, 139 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
>> index 3f881548fb61..0e29ccf903d0 100644
>> --- a/arch/powerpc/include/asm/code-patching.h
>> +++ b/arch/powerpc/include/asm/code-patching.h
>> @@ -74,6 +74,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);
>> +int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr);
>>
>> 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 b00112d7ad46..e1c1fd9246d8 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -204,9 +204,6 @@ void __init poking_init(void)
>> {
>> int ret;
>>
>> - if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
>> - return;
>> -
>> if (mm_patch_enabled())
>> ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>> "powerpc/text_poke_mm:online",
>> @@ -378,6 +375,144 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
>> }
>> NOKPROBE_SYMBOL(patch_instruction);
>>
>> +static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
>> +{
>> + unsigned long start = (unsigned long)patch_addr;
>> +
>> + /* Repeat instruction */
>> + if (repeat_instr) {
>> + ppc_inst_t instr = ppc_inst_read(code);
>> +
>> + if (ppc_inst_prefixed(instr)) {
>> + u64 val = ppc_inst_as_ulong(instr);
>> +
>> + memset64((u64 *)patch_addr, val, len / 8);
>> + } else {
>> + u32 val = ppc_inst_val(instr);
>> +
>> + memset32(patch_addr, val, len / 4);
>> + }
>> + } else {
>> + memcpy(patch_addr, code, len);
>> + }
>> +
>> + smp_wmb(); /* smp write barrier */
>> + flush_icache_range(start, start + len);
>> + return 0;
>> +}
>> +
>> +/*
>> + * A page is mapped and instructions that fit the page are patched.
>> + * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
>> + */
>> +static int __do_patch_instructions_mm(u32 *addr, u32 *code, size_t len, bool repeat_instr)
>> +{
>> + struct mm_struct *patching_mm, *orig_mm;
>> + unsigned long pfn = get_patch_pfn(addr);
>> + unsigned long text_poke_addr;
>> + spinlock_t *ptl;
>> + u32 *patch_addr;
>> + pte_t *pte;
>> + int err;
>> +
>> + patching_mm = __this_cpu_read(cpu_patching_context.mm);
>> + text_poke_addr = __this_cpu_read(cpu_patching_context.addr);
>> + patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
>> +
>> + pte = get_locked_pte(patching_mm, text_poke_addr, &ptl);
>> + if (!pte)
>> + return -ENOMEM;
>> +
>> + __set_pte_at(patching_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
>> +
>> + /* order PTE update before use, also serves as the hwsync */
>> + asm volatile("ptesync" ::: "memory");
>> +
>> + /* order context switch after arbitrary prior code */
>> + isync();
>> +
>> + orig_mm = start_using_temp_mm(patching_mm);
>> +
>> + err = __patch_instructions(patch_addr, code, len, repeat_instr);
>> +
>> + /* context synchronisation performed by __patch_instructions */
>> + stop_using_temp_mm(patching_mm, orig_mm);
>> +
>> + pte_clear(patching_mm, text_poke_addr, pte);
>> + /*
>> + * ptesync to order PTE update before TLB invalidation done
>> + * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
>> + */
>> + local_flush_tlb_page_psize(patching_mm, text_poke_addr, mmu_virtual_psize);
>> +
>> + pte_unmap_unlock(pte, ptl);
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * A page is mapped and instructions that fit the page are patched.
>> + * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
>> + */
>> +static int __do_patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
>> +{
>> + unsigned long pfn = get_patch_pfn(addr);
>> + unsigned long text_poke_addr;
>> + u32 *patch_addr;
>> + pte_t *pte;
>> + int err;
>> +
>> + text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
>> + patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
>> +
>> + pte = __this_cpu_read(cpu_patching_context.pte);
>> + __set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
>> + /* See ptesync comment in radix__set_pte_at() */
>> + if (radix_enabled())
>> + asm volatile("ptesync" ::: "memory");
>> +
>> + err = __patch_instructions(patch_addr, code, len, repeat_instr);
>> +
>> + pte_clear(&init_mm, text_poke_addr, pte);
>> + flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * Patch 'addr' with 'len' bytes of instructions from 'code'.
>> + *
>> + * If repeat_instr is true, the same instruction is filled for
>> + * 'len' bytes.
>> + */
>> +int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
>> +{
>
> Will this break with prefix instructions?
No, afaics.. unless, the caller fails to setup the code buffer
appropriately..
Thanks
Hari
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-30 18:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-20 14:13 [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Hari Bathini
2023-10-20 14:13 ` [PATCH v7 1/5] powerpc/code-patching: introduce patch_instructions() Hari Bathini
2023-10-30 13:02 ` Aneesh Kumar K.V
2023-10-30 18:56 ` Hari Bathini
2023-10-20 14:13 ` [PATCH v7 2/5] powerpc/bpf: implement bpf_arch_text_copy Hari Bathini
2023-10-20 14:13 ` [PATCH v7 3/5] powerpc/bpf: implement bpf_arch_text_invalidate for bpf_prog_pack Hari Bathini
2023-10-20 14:13 ` [PATCH v7 4/5] powerpc/bpf: rename powerpc64_jit_data to powerpc_jit_data Hari Bathini
2023-10-20 14:13 ` [PATCH v7 5/5] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free] Hari Bathini
2023-10-27 9:59 ` [PATCH v7 0/5] powerpc/bpf: use BPF prog pack allocator Michael Ellerman
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).