From: Hari Bathini <hbathini@linux.ibm.com> To: Christophe Leroy <christophe.leroy@csgroup.eu>, "bpf@vger.kernel.org" <bpf@vger.kernel.org>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org> Cc: Song Liu <songliubraving@fb.com>, Daniel Borkmann <daniel@iogearbox.net>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, John Fastabend <john.fastabend@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, Paul Mackerras <paulus@samba.org>, Jordan Niethe <jniethe5@gmail.com>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>, Yonghong Song <yhs@fb.com>, KP Singh <kpsingh@kernel.org>, Martin KaFai Lau <kafai@fb.com> Subject: Re: [PATCH v2 5/5] bpf ppc32: Add instructions for atomic_[cmp]xchg Date: Tue, 14 Jun 2022 00:41:47 +0530 [thread overview] Message-ID: <3d5f05d1-448f-58a6-20b0-3e9f0b13df03@linux.ibm.com> (raw) In-Reply-To: <f09b59ee-c965-a140-4d03-723830cba66d@csgroup.eu> On 11/06/22 11:04 pm, Christophe Leroy wrote: > > > Le 10/06/2022 à 17:55, Hari Bathini a écrit : >> This adds two atomic opcodes BPF_XCHG and BPF_CMPXCHG on ppc32, both >> of which include the BPF_FETCH flag. The kernel's atomic_cmpxchg >> operation fundamentally has 3 operands, but we only have two register >> fields. Therefore the operand we compare against (the kernel's API >> calls it 'old') is hard-coded to be BPF_REG_R0. Also, kernel's >> atomic_cmpxchg returns the previous value at dst_reg + off. JIT the >> same for BPF too with return value put in BPF_REG_0. >> >> BPF_REG_R0 = atomic_cmpxchg(dst_reg + off, BPF_REG_R0, src_reg); >> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> --- >> >> Changes in v2: >> * Moved variable declaration to avoid late declaration error on >> some compilers. >> * Tried to make code readable and compact. >> >> >> arch/powerpc/net/bpf_jit_comp32.c | 25 ++++++++++++++++++++++--- >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c >> index 28dc6a1a8f2f..43f1c76d48ce 100644 >> --- a/arch/powerpc/net/bpf_jit_comp32.c >> +++ b/arch/powerpc/net/bpf_jit_comp32.c >> @@ -297,6 +297,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> u32 ax_reg = bpf_to_ppc(BPF_REG_AX); >> u32 tmp_reg = bpf_to_ppc(TMP_REG); >> u32 size = BPF_SIZE(code); >> + u32 save_reg, ret_reg; >> s16 off = insn[i].off; >> s32 imm = insn[i].imm; >> bool func_addr_fixed; >> @@ -799,6 +800,9 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> * BPF_STX ATOMIC (atomic ops) >> */ >> case BPF_STX | BPF_ATOMIC | BPF_W: >> + save_reg = _R0; >> + ret_reg = src_reg; >> + >> bpf_set_seen_register(ctx, tmp_reg); >> bpf_set_seen_register(ctx, ax_reg); >> >> @@ -829,6 +833,21 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> case BPF_XOR | BPF_FETCH: >> EMIT(PPC_RAW_XOR(_R0, _R0, src_reg)); >> break; >> + case BPF_CMPXCHG: >> + /* >> + * Return old value in BPF_REG_0 for BPF_CMPXCHG & >> + * in src_reg for other cases. >> + */ >> + ret_reg = bpf_to_ppc(BPF_REG_0); >> + >> + /* Compare with old value in BPF_REG_0 */ >> + EMIT(PPC_RAW_CMPW(bpf_to_ppc(BPF_REG_0), _R0)); >> + /* Don't set if different from old value */ >> + PPC_BCC_SHORT(COND_NE, (ctx->idx + 3) * 4); >> + fallthrough; >> + case BPF_XCHG: >> + save_reg = src_reg; > > I'm a bit lost, when save_reg is src_reg, don't we expect the upper part > (ie src_reg - 1) to be explicitely zeroised ? > For BPF_FETCH variants, old value is returned in src_reg (ret_reg). In all such cases, higher 32-bit is zero'ed. But in case of BPF_CMPXCHG, src_reg is untouched as BPF_REG_0 is used instead. So, higher 32-bit remains untouched for that case alone.. >> + break; >> default: >> pr_err_ratelimited("eBPF filter atomic op code %02x (@%d) unsupported\n", >> code, i); >> @@ -836,15 +855,15 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> } >> >> /* store new value */ >> - EMIT(PPC_RAW_STWCX(_R0, tmp_reg, dst_reg)); >> + EMIT(PPC_RAW_STWCX(save_reg, tmp_reg, dst_reg)); >> /* we're done if this succeeded */ >> PPC_BCC_SHORT(COND_NE, tmp_idx); >> >> /* For the BPF_FETCH variant, get old data into src_reg */ With this commit, this comment is not true for BPF_CMPXCHG. So, this comment should not be removed.. Thanks Hari
WARNING: multiple messages have this Message-ID (diff)
From: Hari Bathini <hbathini@linux.ibm.com> To: Christophe Leroy <christophe.leroy@csgroup.eu>, "bpf@vger.kernel.org" <bpf@vger.kernel.org>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org> Cc: Song Liu <songliubraving@fb.com>, Daniel Borkmann <daniel@iogearbox.net>, John Fastabend <john.fastabend@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, Paul Mackerras <paulus@samba.org>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>, Yonghong Song <yhs@fb.com>, KP Singh <kpsingh@kernel.org>, Jordan Niethe <jniethe5@gmail.com>, Martin KaFai Lau <kafai@fb.com> Subject: Re: [PATCH v2 5/5] bpf ppc32: Add instructions for atomic_[cmp]xchg Date: Tue, 14 Jun 2022 00:41:47 +0530 [thread overview] Message-ID: <3d5f05d1-448f-58a6-20b0-3e9f0b13df03@linux.ibm.com> (raw) In-Reply-To: <f09b59ee-c965-a140-4d03-723830cba66d@csgroup.eu> On 11/06/22 11:04 pm, Christophe Leroy wrote: > > > Le 10/06/2022 à 17:55, Hari Bathini a écrit : >> This adds two atomic opcodes BPF_XCHG and BPF_CMPXCHG on ppc32, both >> of which include the BPF_FETCH flag. The kernel's atomic_cmpxchg >> operation fundamentally has 3 operands, but we only have two register >> fields. Therefore the operand we compare against (the kernel's API >> calls it 'old') is hard-coded to be BPF_REG_R0. Also, kernel's >> atomic_cmpxchg returns the previous value at dst_reg + off. JIT the >> same for BPF too with return value put in BPF_REG_0. >> >> BPF_REG_R0 = atomic_cmpxchg(dst_reg + off, BPF_REG_R0, src_reg); >> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> --- >> >> Changes in v2: >> * Moved variable declaration to avoid late declaration error on >> some compilers. >> * Tried to make code readable and compact. >> >> >> arch/powerpc/net/bpf_jit_comp32.c | 25 ++++++++++++++++++++++--- >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c >> index 28dc6a1a8f2f..43f1c76d48ce 100644 >> --- a/arch/powerpc/net/bpf_jit_comp32.c >> +++ b/arch/powerpc/net/bpf_jit_comp32.c >> @@ -297,6 +297,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> u32 ax_reg = bpf_to_ppc(BPF_REG_AX); >> u32 tmp_reg = bpf_to_ppc(TMP_REG); >> u32 size = BPF_SIZE(code); >> + u32 save_reg, ret_reg; >> s16 off = insn[i].off; >> s32 imm = insn[i].imm; >> bool func_addr_fixed; >> @@ -799,6 +800,9 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> * BPF_STX ATOMIC (atomic ops) >> */ >> case BPF_STX | BPF_ATOMIC | BPF_W: >> + save_reg = _R0; >> + ret_reg = src_reg; >> + >> bpf_set_seen_register(ctx, tmp_reg); >> bpf_set_seen_register(ctx, ax_reg); >> >> @@ -829,6 +833,21 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> case BPF_XOR | BPF_FETCH: >> EMIT(PPC_RAW_XOR(_R0, _R0, src_reg)); >> break; >> + case BPF_CMPXCHG: >> + /* >> + * Return old value in BPF_REG_0 for BPF_CMPXCHG & >> + * in src_reg for other cases. >> + */ >> + ret_reg = bpf_to_ppc(BPF_REG_0); >> + >> + /* Compare with old value in BPF_REG_0 */ >> + EMIT(PPC_RAW_CMPW(bpf_to_ppc(BPF_REG_0), _R0)); >> + /* Don't set if different from old value */ >> + PPC_BCC_SHORT(COND_NE, (ctx->idx + 3) * 4); >> + fallthrough; >> + case BPF_XCHG: >> + save_reg = src_reg; > > I'm a bit lost, when save_reg is src_reg, don't we expect the upper part > (ie src_reg - 1) to be explicitely zeroised ? > For BPF_FETCH variants, old value is returned in src_reg (ret_reg). In all such cases, higher 32-bit is zero'ed. But in case of BPF_CMPXCHG, src_reg is untouched as BPF_REG_0 is used instead. So, higher 32-bit remains untouched for that case alone.. >> + break; >> default: >> pr_err_ratelimited("eBPF filter atomic op code %02x (@%d) unsupported\n", >> code, i); >> @@ -836,15 +855,15 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> } >> >> /* store new value */ >> - EMIT(PPC_RAW_STWCX(_R0, tmp_reg, dst_reg)); >> + EMIT(PPC_RAW_STWCX(save_reg, tmp_reg, dst_reg)); >> /* we're done if this succeeded */ >> PPC_BCC_SHORT(COND_NE, tmp_idx); >> >> /* For the BPF_FETCH variant, get old data into src_reg */ With this commit, this comment is not true for BPF_CMPXCHG. So, this comment should not be removed.. Thanks Hari
next prev parent reply other threads:[~2022-06-13 19:13 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-10 15:55 [PATCH v2 0/5] Atomics support for eBPF on powerpc Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-10 15:55 ` [PATCH v2 1/5] bpf ppc64: add support for BPF_ATOMIC bitwise operations Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-10 15:55 ` [PATCH v2 2/5] bpf ppc64: add support for atomic fetch operations Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-10 15:55 ` [PATCH v2 3/5] bpf ppc64: Add instructions for atomic_[cmp]xchg Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-10 15:55 ` [PATCH v2 4/5] bpf ppc32: add support for BPF_ATOMIC bitwise operations Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-11 17:14 ` Christophe Leroy 2022-06-11 17:14 ` Christophe Leroy 2022-06-13 19:00 ` Hari Bathini 2022-06-13 19:00 ` Hari Bathini 2022-06-10 15:55 ` [PATCH v2 5/5] bpf ppc32: Add instructions for atomic_[cmp]xchg Hari Bathini 2022-06-10 15:55 ` Hari Bathini 2022-06-11 17:34 ` Christophe Leroy 2022-06-11 17:34 ` Christophe Leroy 2022-06-13 19:11 ` Hari Bathini [this message] 2022-06-13 19:11 ` Hari Bathini 2022-06-13 19:14 ` Hari Bathini 2022-06-13 19:14 ` Hari Bathini 2022-06-24 10:41 ` Naveen N. Rao 2022-06-24 10:41 ` Naveen N. Rao 2022-06-24 10:37 ` [PATCH v2 0/5] Atomics support for eBPF on powerpc Naveen N. Rao 2022-06-24 10:37 ` Naveen N. Rao 2022-07-04 11:33 ` Michael Ellerman 2022-07-04 11:33 ` Michael Ellerman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3d5f05d1-448f-58a6-20b0-3e9f0b13df03@linux.ibm.com \ --to=hbathini@linux.ibm.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=christophe.leroy@csgroup.eu \ --cc=daniel@iogearbox.net \ --cc=jniethe5@gmail.com \ --cc=john.fastabend@gmail.com \ --cc=kafai@fb.com \ --cc=kpsingh@kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=naveen.n.rao@linux.ibm.com \ --cc=netdev@vger.kernel.org \ --cc=paulus@samba.org \ --cc=songliubraving@fb.com \ --cc=yhs@fb.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.