diff mbox series

[1/2] LoongArch: BPF: Treat function address as 64-bit value

Message ID 20230212035236.1436532-2-hengqi.chen@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Allow mixing bpf2bpf calls with tailcalls on LoongArch | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 fail Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 fail Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 fail Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix

Commit Message

Hengqi Chen Feb. 12, 2023, 3:52 a.m. UTC 
Let's always use 4 instructions for function address in JIT.
So that the instruction sequences don't change between the first
pass and the extra pass for function calls.

Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 arch/loongarch/net/bpf_jit.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Tiezhu Yang Feb. 13, 2023, 3:01 a.m. UTC | #1 
Hi Hengqi,

On 02/12/2023 11:52 AM, Hengqi Chen wrote:
> Let's always use 4 instructions for function address in JIT.
> So that the instruction sequences don't change between the first
> pass and the extra pass for function calls.
>
> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  arch/loongarch/net/bpf_jit.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index c4b1947ebf76..2d952110be72 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -446,6 +446,27 @@ static int add_exception_handler(const struct bpf_insn *insn,
>  	return 0;
>  }
>
> +static inline void emit_addr_move(struct jit_ctx *ctx, enum loongarch_gpr rd, u64 addr)
> +{
> +	u64 imm_11_0, imm_31_12, imm_51_32, imm_63_52;
> +
> +	/* lu12iw rd, imm_31_12 */
> +	imm_31_12 = (addr >> 12) & 0xfffff;
> +	emit_insn(ctx, lu12iw, rd, imm_31_12);
> +
> +	/* ori rd, rd, imm_11_0 */
> +	imm_11_0 = addr & 0xfff;
> +	emit_insn(ctx, ori, rd, rd, imm_11_0);
> +
> +	/* lu32id rd, imm_51_32 */
> +	imm_51_32 = (addr >> 32) & 0xfffff;
> +	emit_insn(ctx, lu32id, rd, imm_51_32);
> +
> +	/* lu52id rd, rd, imm_63_52 */
> +	imm_63_52 = (addr >> 52) & 0xfff;
> +	emit_insn(ctx, lu52id, rd, rd, imm_63_52);
> +}
> +
>  static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
>  {
>  	u8 tm = -1;
> @@ -841,7 +862,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>  		if (ret < 0)
>  			return ret;
>
> -		move_imm(ctx, t1, func_addr, is32);
> +		emit_addr_move(ctx, t1, func_addr);
>  		emit_insn(ctx, jirl, t1, LOONGARCH_GPR_RA, 0);
>  		move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
>  		break;
>

The code itself looks good to me.

Could you please give more detailed info in the commit message?
For example, description of problem, steps to reproduce, ...
I think the descriptions in the cover letter are useful, it is
better to record them in the commit message.

Additionally, emit_addr_move() is similar with move_imm(), it is
better to define emit_addr_move() before move_imm() in bpf_jit.h.

Thanks,
Tiezhu
Tiezhu Yang Feb. 13, 2023, 3:18 a.m. UTC | #2 
On 02/13/2023 11:01 AM, Tiezhu Yang wrote:
> Hi Hengqi,
>
> On 02/12/2023 11:52 AM, Hengqi Chen wrote:
>> Let's always use 4 instructions for function address in JIT.
>> So that the instruction sequences don't change between the first
>> pass and the extra pass for function calls.
>>
>> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>>  arch/loongarch/net/bpf_jit.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
>> index c4b1947ebf76..2d952110be72 100644
>> --- a/arch/loongarch/net/bpf_jit.c
>> +++ b/arch/loongarch/net/bpf_jit.c
>> @@ -446,6 +446,27 @@ static int add_exception_handler(const struct
>> bpf_insn *insn,
>>      return 0;
>>  }
>>
>> +static inline void emit_addr_move(struct jit_ctx *ctx, enum
>> loongarch_gpr rd, u64 addr)

Small nit:

Maybe use move_addr() ( like move_imm() ) is better than
emit_addr_move()?

Thanks,
Tiezhu
Hengqi Chen Feb. 14, 2023, 3:36 p.m. UTC | #3 
Hi Tiezhu,

On Mon, Feb 13, 2023 at 11:02 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> Hi Hengqi,
>
> On 02/12/2023 11:52 AM, Hengqi Chen wrote:
> > Let's always use 4 instructions for function address in JIT.
> > So that the instruction sequences don't change between the first
> > pass and the extra pass for function calls.
> >
> > Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> > ---
> >  arch/loongarch/net/bpf_jit.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> > index c4b1947ebf76..2d952110be72 100644
> > --- a/arch/loongarch/net/bpf_jit.c
> > +++ b/arch/loongarch/net/bpf_jit.c
> > @@ -446,6 +446,27 @@ static int add_exception_handler(const struct bpf_insn *insn,
> >       return 0;
> >  }
> >
> > +static inline void emit_addr_move(struct jit_ctx *ctx, enum loongarch_gpr rd, u64 addr)
> > +{
> > +     u64 imm_11_0, imm_31_12, imm_51_32, imm_63_52;
> > +
> > +     /* lu12iw rd, imm_31_12 */
> > +     imm_31_12 = (addr >> 12) & 0xfffff;
> > +     emit_insn(ctx, lu12iw, rd, imm_31_12);
> > +
> > +     /* ori rd, rd, imm_11_0 */
> > +     imm_11_0 = addr & 0xfff;
> > +     emit_insn(ctx, ori, rd, rd, imm_11_0);
> > +
> > +     /* lu32id rd, imm_51_32 */
> > +     imm_51_32 = (addr >> 32) & 0xfffff;
> > +     emit_insn(ctx, lu32id, rd, imm_51_32);
> > +
> > +     /* lu52id rd, rd, imm_63_52 */
> > +     imm_63_52 = (addr >> 52) & 0xfff;
> > +     emit_insn(ctx, lu52id, rd, rd, imm_63_52);
> > +}
> > +
> >  static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
> >  {
> >       u8 tm = -1;
> > @@ -841,7 +862,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> >               if (ret < 0)
> >                       return ret;
> >
> > -             move_imm(ctx, t1, func_addr, is32);
> > +             emit_addr_move(ctx, t1, func_addr);
> >               emit_insn(ctx, jirl, t1, LOONGARCH_GPR_RA, 0);
> >               move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
> >               break;
> >
>
> The code itself looks good to me.
>
> Could you please give more detailed info in the commit message?
> For example, description of problem, steps to reproduce, ...
> I think the descriptions in the cover letter are useful, it is
> better to record them in the commit message.
>
> Additionally, emit_addr_move() is similar with move_imm(), it is
> better to define emit_addr_move() before move_imm() in bpf_jit.h.
>
> Thanks,
> Tiezhu
>

I've addressed your comments and send a v2 for review.
The second patch is dropped, as it is incomplete.

Cheers,
---
Hengqi
diff mbox series

Patch

diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index c4b1947ebf76..2d952110be72 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -446,6 +446,27 @@  static int add_exception_handler(const struct bpf_insn *insn,
 	return 0;
 }
 
+static inline void emit_addr_move(struct jit_ctx *ctx, enum loongarch_gpr rd, u64 addr)
+{
+	u64 imm_11_0, imm_31_12, imm_51_32, imm_63_52;
+
+	/* lu12iw rd, imm_31_12 */
+	imm_31_12 = (addr >> 12) & 0xfffff;
+	emit_insn(ctx, lu12iw, rd, imm_31_12);
+
+	/* ori rd, rd, imm_11_0 */
+	imm_11_0 = addr & 0xfff;
+	emit_insn(ctx, ori, rd, rd, imm_11_0);
+
+	/* lu32id rd, imm_51_32 */
+	imm_51_32 = (addr >> 32) & 0xfffff;
+	emit_insn(ctx, lu32id, rd, imm_51_32);
+
+	/* lu52id rd, rd, imm_63_52 */
+	imm_63_52 = (addr >> 52) & 0xfff;
+	emit_insn(ctx, lu52id, rd, rd, imm_63_52);
+}
+
 static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
 {
 	u8 tm = -1;
@@ -841,7 +862,7 @@  static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
 		if (ret < 0)
 			return ret;
 
-		move_imm(ctx, t1, func_addr, is32);
+		emit_addr_move(ctx, t1, func_addr);
 		emit_insn(ctx, jirl, t1, LOONGARCH_GPR_RA, 0);
 		move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
 		break;