Re: [PATCH bpf-next 08/10] riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table handling

Re: [PATCH bpf-next 08/10] riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table handling

[Pu Lehui]

On 2025/9/13 23:51, Chris Mason wrote:
> On Sat, 19 Jul 2025 09:17:28 +0000 Pu Lehui <pulehui@huaweicloud.com> wrote:
> 
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> Add ex_insn_off and ex_jmp_off fields to struct rv_jit_context so that
>> add_exception_handler() does not need to be immediately followed by the
>> instruction to add the exception table. ex_insn_off indicates the offset
>> of the instruction to add the exception table, and ex_jmp_off indicates
>> the offset to jump over the faulting instruction. This is to prepare for
>> adding the exception table to atomic instructions later, because some
>> atomic instructions need to perform zext or other operations.
>>
> 
> Hi everyone,
> 
> I've been working on some patch review automation, and I recently ran it on
> the bpf-next branch.  I don't know the verifier well enough to decide if this
> is a false positive, but Alexei asked me to kick off discussion, so:
> 
>> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
>> index 8e813809d3054..56b592af53a64 100644
>> --- a/arch/riscv/net/bpf_jit_comp64.c
>> +++ b/arch/riscv/net/bpf_jit_comp64.c
> 
> [ ... ]
> 
>> -static int emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
>> +static void emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
>>   {
>> -	int insns_start;
>> -
>>   	if (is_12b_int(off)) {
>> -		insns_start = ctx->ninsns;
>> +		ctx->ex_insn_off = ctx->ninsns;
>>   		emit_stx_insn(rd, off, rs, size, ctx);
>> -		return ctx->ninsns - insns_start;
>> +		ctx->ex_jmp_off = ctx->ex_jmp_off;

Hi Chris,

I'm not sure if I have misunderstood your intention, maybe just for 
talking about reviewing automation? But the code I checked in the 
bpf-next branch is inconsistent with yours. The code here in bpf-next is:

`ctx->ex_jmp_off = ctx->ninsns;`

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/riscv/net/bpf_jit_comp64.c?h=next-20250912#n553

> 
> Does this assignment leave ex_jmp_off uninitialized? Looking at emit_ldx()
> and emit_st(), this should probably be "ctx->ex_jmp_off = ctx->ninsns;" to
> match the pattern. Without proper initialization, add_exception_handler()
> might use stale offset values when creating exception table entries for
> the is_12b_int() code path in emit_stx().
> 
>> +		return;
>>   	}
>>   
>>   	emit_imm(RV_REG_T1, off, ctx);
>>   	emit_add(RV_REG_T1, RV_REG_T1, rd, ctx);
>> -	insns_start = ctx->ninsns;
>> +	ctx->ex_insn_off = ctx->ninsns;
>>   	emit_stx_insn(RV_REG_T1, 0, rs, size, ctx);
>> -	return ctx->ninsns - insns_start;
>> +	ctx->ex_jmp_off = ctx->ninsns;
>>   }
> 
> -chris


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next 08/10] riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table handling

[Chris Mason]

On 9/15/25 5:39 AM, Pu Lehui wrote:
> 
> 
> On 2025/9/13 23:51, Chris Mason wrote:
>> On Sat, 19 Jul 2025 09:17:28 +0000 Pu Lehui <pulehui@huaweicloud.com> wrote:
>>
>>> From: Pu Lehui <pulehui@huawei.com>
>>>
>>> Add ex_insn_off and ex_jmp_off fields to struct rv_jit_context so that
>>> add_exception_handler() does not need to be immediately followed by the
>>> instruction to add the exception table. ex_insn_off indicates the offset
>>> of the instruction to add the exception table, and ex_jmp_off indicates
>>> the offset to jump over the faulting instruction. This is to prepare for
>>> adding the exception table to atomic instructions later, because some
>>> atomic instructions need to perform zext or other operations.
>>>
>>
>> Hi everyone,
>>
>> I've been working on some patch review automation, and I recently ran it on
>> the bpf-next branch.  I don't know the verifier well enough to decide if this
>> is a false positive, but Alexei asked me to kick off discussion, so:
>>
>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
>>> index 8e813809d3054..56b592af53a64 100644
>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>
>> [ ... ]
>>
>>> -static int emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
>>> +static void emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
>>>   {
>>> -	int insns_start;
>>> -
>>>   	if (is_12b_int(off)) {
>>> -		insns_start = ctx->ninsns;
>>> +		ctx->ex_insn_off = ctx->ninsns;
>>>   		emit_stx_insn(rd, off, rs, size, ctx);
>>> -		return ctx->ninsns - insns_start;
>>> +		ctx->ex_jmp_off = ctx->ex_jmp_off;
> 
> Hi Chris,
> 
> I'm not sure if I have misunderstood your intention, maybe just for 
> talking about reviewing automation? But the code I checked in the 
> bpf-next branch is inconsistent with yours. The code here in bpf-next is:
> 
> `ctx->ex_jmp_off = ctx->ninsns;`
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/riscv/net/bpf_jit_comp64.c?h=next-20250912#n553 

You're right, the review tooling got confused because it only indexed
the x86 version.  Sorry about that, thanks for double checking.

-chris

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH bpf-next 00/10] Add support arena atomics for RV64

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

patch 1-3 refactor redundant load and store operations.
patch 4-7 add Zacas instructions for cmpxchg.
patch 8 optimizes exception table handling.
patch 9-10 add support arena atomics for RV64.

Tests `test_progs -t atomic,arena` have passed as shown bellow,
as well as `test_verifier` and `test_bpf.ko` have passed.

$ ./test_progs -t arena,atomic
#3/1     arena_atomics/add:OK
#3/2     arena_atomics/sub:OK
#3/3     arena_atomics/and:OK
#3/4     arena_atomics/or:OK
#3/5     arena_atomics/xor:OK
#3/6     arena_atomics/cmpxchg:OK
#3/7     arena_atomics/xchg:OK
#3/8     arena_atomics/uaf:OK
#3/9     arena_atomics/load_acquire:OK
#3/10    arena_atomics/store_release:OK
#3       arena_atomics:OK
#4/1     arena_htab/arena_htab_llvm:OK
#4/2     arena_htab/arena_htab_asm:OK
#4       arena_htab:OK
#5/1     arena_list/arena_list_1:OK
#5/2     arena_list/arena_list_1000:OK
#5       arena_list:OK
#6/1     arena_spin_lock/arena_spin_lock_1:OK
#6/2     arena_spin_lock/arena_spin_lock_1000:OK
#6/3     arena_spin_lock/arena_spin_lock_50000:OK
#6       arena_spin_lock:OK
#10      atomic_bounds:OK
#11/1    atomics/add:OK
#11/2    atomics/sub:OK
#11/3    atomics/and:OK
#11/4    atomics/or:OK
#11/5    atomics/xor:OK
#11/6    atomics/cmpxchg:OK
#11/7    atomics/xchg:OK
#11      atomics:OK
#513/1   verifier_arena/basic_alloc1:OK
#513/2   verifier_arena/basic_alloc2:OK
#513/3   verifier_arena/basic_alloc3:OK
#513/4   verifier_arena/basic_reserve1:OK
#513/5   verifier_arena/basic_reserve2:OK
#513/6   verifier_arena/reserve_twice:OK
#513/7   verifier_arena/reserve_invalid_region:OK
#513/8   verifier_arena/iter_maps1:OK
#513/9   verifier_arena/iter_maps2:OK
#513/10  verifier_arena/iter_maps3:OK
#513     verifier_arena:OK
#514/1   verifier_arena_large/big_alloc1:OK
#514/2   verifier_arena_large/access_reserved:OK
#514/3   verifier_arena_large/request_partially_reserved:OK
#514/4   verifier_arena_large/free_reserved:OK
#514/5   verifier_arena_large/big_alloc2:OK
#514     verifier_arena_large:OK
Summary: 8/39 PASSED, 0 SKIPPED, 0 FAILED

Pu Lehui (10):
  riscv, bpf: Extract emit_stx() helper
  riscv, bpf: Extract emit_st() helper
  riscv, bpf: Extract emit_ldx() helper
  riscv: Separate toolchain support dependency from RISCV_ISA_ZACAS
  riscv, bpf: Add rv_ext_enabled macro for runtime detection extentsion
  riscv, bpf: Add Zacas instructions
  riscv, bpf: Optimize cmpxchg insn with Zacas support
  riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table
    handling
  riscv, bpf: Add support arena atomics for RV64
  selftests/bpf: Enable arena atomics tests for RV64

 arch/riscv/Kconfig                            |   1 -
 arch/riscv/include/asm/cmpxchg.h              |   6 +-
 arch/riscv/kernel/setup.c                     |   1 +
 arch/riscv/net/bpf_jit.h                      |  70 ++-
 arch/riscv/net/bpf_jit_comp64.c               | 516 +++++-------------
 .../selftests/bpf/progs/arena_atomics.c       |   9 +-
 6 files changed, 214 insertions(+), 389 deletions(-)

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH bpf-next 08/10] riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table handling

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

Add ex_insn_off and ex_jmp_off fields to struct rv_jit_context so that
add_exception_handler() does not need to be immediately followed by the
instruction to add the exception table. ex_insn_off indicates the offset
of the instruction to add the exception table, and ex_jmp_off indicates
the offset to jump over the faulting instruction. This is to prepare for
adding the exception table to atomic instructions later, because some
atomic instructions need to perform zext or other operations.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 arch/riscv/net/bpf_jit.h        |  2 +
 arch/riscv/net/bpf_jit_comp64.c | 84 +++++++++++++++------------------
 2 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 0790f40b7e9d..be2915444ce5 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -78,6 +78,8 @@ struct rv_jit_context {
 	int epilogue_offset;
 	int *offset;		/* BPF to RV */
 	int nexentries;
+	int ex_insn_off;
+	int ex_jmp_off;
 	unsigned long flags;
 	int stack_size;
 	u64 arena_vm_start;
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 8e813809d305..56b592af53a6 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -511,57 +511,54 @@ static void emit_stx_insn(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context
 	}
 }
 
-static int emit_ldx(u8 rd, s16 off, u8 rs, u8 size, bool sign_ext,
+static void emit_ldx(u8 rd, s16 off, u8 rs, u8 size, bool sign_ext,
 		    struct rv_jit_context *ctx)
 {
-	int insns_start;
-
 	if (is_12b_int(off)) {
-		insns_start = ctx->ninsns;
+		ctx->ex_insn_off = ctx->ninsns;
 		emit_ldx_insn(rd, off, rs, size, sign_ext, ctx);
-		return ctx->ninsns - insns_start;
+		ctx->ex_jmp_off = ctx->ninsns;
+		return;
 	}
 
 	emit_imm(RV_REG_T1, off, ctx);
 	emit_add(RV_REG_T1, RV_REG_T1, rs, ctx);
-	insns_start = ctx->ninsns;
+	ctx->ex_insn_off = ctx->ninsns;
 	emit_ldx_insn(rd, 0, RV_REG_T1, size, sign_ext, ctx);
-	return ctx->ninsns - insns_start;
+	ctx->ex_jmp_off = ctx->ninsns;
 }
 
-static int emit_st(u8 rd, s16 off, s32 imm, u8 size, struct rv_jit_context *ctx)
+static void emit_st(u8 rd, s16 off, s32 imm, u8 size, struct rv_jit_context *ctx)
 {
-	int insns_start;
-
 	emit_imm(RV_REG_T1, imm, ctx);
 	if (is_12b_int(off)) {
-		insns_start = ctx->ninsns;
+		ctx->ex_insn_off = ctx->ninsns;
 		emit_stx_insn(rd, off, RV_REG_T1, size, ctx);
-		return ctx->ninsns - insns_start;
+		ctx->ex_jmp_off = ctx->ninsns;
+		return;
 	}
 
 	emit_imm(RV_REG_T2, off, ctx);
 	emit_add(RV_REG_T2, RV_REG_T2, rd, ctx);
-	insns_start = ctx->ninsns;
+	ctx->ex_insn_off = ctx->ninsns;
 	emit_stx_insn(RV_REG_T2, 0, RV_REG_T1, size, ctx);
-	return ctx->ninsns - insns_start;
+	ctx->ex_jmp_off = ctx->ninsns;
 }
 
-static int emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
+static void emit_stx(u8 rd, s16 off, u8 rs, u8 size, struct rv_jit_context *ctx)
 {
-	int insns_start;
-
 	if (is_12b_int(off)) {
-		insns_start = ctx->ninsns;
+		ctx->ex_insn_off = ctx->ninsns;
 		emit_stx_insn(rd, off, rs, size, ctx);
-		return ctx->ninsns - insns_start;
+		ctx->ex_jmp_off = ctx->ninsns;
+		return;
 	}
 
 	emit_imm(RV_REG_T1, off, ctx);
 	emit_add(RV_REG_T1, RV_REG_T1, rd, ctx);
-	insns_start = ctx->ninsns;
+	ctx->ex_insn_off = ctx->ninsns;
 	emit_stx_insn(RV_REG_T1, 0, rs, size, ctx);
-	return ctx->ninsns - insns_start;
+	ctx->ex_jmp_off = ctx->ninsns;
 }
 
 static int emit_atomic_ld_st(u8 rd, u8 rs, const struct bpf_insn *insn,
@@ -700,9 +697,8 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
 }
 
 /* For accesses to BTF pointers, add an entry to the exception table */
-static int add_exception_handler(const struct bpf_insn *insn,
-				 struct rv_jit_context *ctx,
-				 int dst_reg, int insn_len)
+static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
+				 struct rv_jit_context *ctx)
 {
 	struct exception_table_entry *ex;
 	unsigned long pc;
@@ -710,21 +706,22 @@ static int add_exception_handler(const struct bpf_insn *insn,
 	off_t fixup_offset;
 
 	if (!ctx->insns || !ctx->ro_insns || !ctx->prog->aux->extable ||
-	    (BPF_MODE(insn->code) != BPF_PROBE_MEM && BPF_MODE(insn->code) != BPF_PROBE_MEMSX &&
-	     BPF_MODE(insn->code) != BPF_PROBE_MEM32))
+	    ctx->ex_insn_off <= 0 || ctx->ex_jmp_off <= 0)
 		return 0;
 
-	if (WARN_ON_ONCE(ctx->nexentries >= ctx->prog->aux->num_exentries))
-		return -EINVAL;
+	if (BPF_MODE(insn->code) != BPF_PROBE_MEM &&
+	    BPF_MODE(insn->code) != BPF_PROBE_MEMSX &&
+	    BPF_MODE(insn->code) != BPF_PROBE_MEM32)
+		return 0;
 
-	if (WARN_ON_ONCE(insn_len > ctx->ninsns))
+	if (WARN_ON_ONCE(ctx->nexentries >= ctx->prog->aux->num_exentries))
 		return -EINVAL;
 
-	if (WARN_ON_ONCE(!rvc_enabled() && insn_len == 1))
+	if (WARN_ON_ONCE(ctx->ex_insn_off > ctx->ninsns || ctx->ex_jmp_off > ctx->ninsns))
 		return -EINVAL;
 
 	ex = &ctx->prog->aux->extable[ctx->nexentries];
-	pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
+	pc = (unsigned long)&ctx->ro_insns[ctx->ex_insn_off];
 
 	/*
 	 * This is the relative offset of the instruction that may fault from
@@ -748,7 +745,7 @@ static int add_exception_handler(const struct bpf_insn *insn,
 	 * that may fault. The execution will jump to this after handling the
 	 * fault.
 	 */
-	fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
+	fixup_offset = (long)&ex->fixup - (long)&ctx->ro_insns[ctx->ex_jmp_off];
 	if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
 		return -ERANGE;
 
@@ -765,6 +762,8 @@ static int add_exception_handler(const struct bpf_insn *insn,
 		FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
 	ex->type = EX_TYPE_BPF;
 
+	ctx->ex_insn_off = 0;
+	ctx->ex_jmp_off = 0;
 	ctx->nexentries++;
 	return 0;
 }
@@ -1774,7 +1773,6 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 	case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
 	{
 		bool sign_ext;
-		int insn_len;
 
 		sign_ext = BPF_MODE(insn->code) == BPF_MEMSX ||
 			   BPF_MODE(insn->code) == BPF_PROBE_MEMSX;
@@ -1784,9 +1782,9 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 			rs = RV_REG_T2;
 		}
 
-		insn_len = emit_ldx(rd, off, rs, BPF_SIZE(code), sign_ext, ctx);
+		emit_ldx(rd, off, rs, BPF_SIZE(code), sign_ext, ctx);
 
-		ret = add_exception_handler(insn, ctx, rd, insn_len);
+		ret = add_exception_handler(insn, rd, ctx);
 		if (ret)
 			return ret;
 
@@ -1809,21 +1807,17 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 	case BPF_ST | BPF_PROBE_MEM32 | BPF_H:
 	case BPF_ST | BPF_PROBE_MEM32 | BPF_W:
 	case BPF_ST | BPF_PROBE_MEM32 | BPF_DW:
-	{
-		int insn_len;
-
 		if (BPF_MODE(insn->code) == BPF_PROBE_MEM32) {
 			emit_add(RV_REG_T3, rd, RV_REG_ARENA, ctx);
 			rd = RV_REG_T3;
 		}
 
-		insn_len = emit_st(rd, off, imm, BPF_SIZE(code), ctx);
+		emit_st(rd, off, imm, BPF_SIZE(code), ctx);
 
-		ret = add_exception_handler(insn, ctx, REG_DONT_CLEAR_MARKER, insn_len);
+		ret = add_exception_handler(insn, REG_DONT_CLEAR_MARKER, ctx);
 		if (ret)
 			return ret;
 		break;
-	}
 
 	/* STX: *(size *)(dst + off) = src */
 	case BPF_STX | BPF_MEM | BPF_B:
@@ -1835,21 +1829,17 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 	case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
 	case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
 	case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
-	{
-		int insn_len;
-
 		if (BPF_MODE(insn->code) == BPF_PROBE_MEM32) {
 			emit_add(RV_REG_T2, rd, RV_REG_ARENA, ctx);
 			rd = RV_REG_T2;
 		}
 
-		insn_len = emit_stx(rd, off, rs, BPF_SIZE(code), ctx);
+		emit_stx(rd, off, rs, BPF_SIZE(code), ctx);
 
-		ret = add_exception_handler(insn, ctx, REG_DONT_CLEAR_MARKER, insn_len);
+		ret = add_exception_handler(insn, REG_DONT_CLEAR_MARKER, ctx);
 		if (ret)
 			return ret;
 		break;
-	}
 
 	case BPF_STX | BPF_ATOMIC | BPF_B:
 	case BPF_STX | BPF_ATOMIC | BPF_H:
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next 08/10] riscv, bpf: Add ex_insn_off and ex_jmp_off for exception table handling

[Björn Töpel]

Pu Lehui <pulehui@huaweicloud.com> writes:

> From: Pu Lehui <pulehui@huawei.com>
>
> Add ex_insn_off and ex_jmp_off fields to struct rv_jit_context so that
> add_exception_handler() does not need to be immediately followed by the
> instruction to add the exception table. ex_insn_off indicates the offset
> of the instruction to add the exception table, and ex_jmp_off indicates
> the offset to jump over the faulting instruction. This is to prepare for
> adding the exception table to atomic instructions later, because some
> atomic instructions need to perform zext or other operations.
>
> Signed-off-by: Pu Lehui <pulehui@huawei.com>

[...]

> @@ -748,7 +745,7 @@ static int add_exception_handler(const struct bpf_insn *insn,
>  	 * that may fault. The execution will jump to this after handling the
>  	 * fault.
>  	 */

Nit: After the upcoming change, the comment ^^^ is not entierly true.
Don't respin for this, but a follow up would be nice!

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv