Re: [PATCH 2/3] MIPS: eBPF: Provide eBPF support for MIPS64R6

[Paul Burton]

Hi Hassan,

Thanks - overall this is looking good :)

On Fri, Mar 08, 2019 at 03:32:09PM -0800, Hassan Naveed wrote:
> Currently eBPF support is available on MIPS64R2 only. Use MIPS64R6
> variants of instructions like multiply, divide, movn, movz so eBPF
> can run on the newer ISA. Also, we only need to check ISA revision
> before JIT'ing code, because by this point, we already know CPU
> supports 64 bits since it's running a 64 bit kernel. It would have
> crashed otherwise.

It might be worth stating that we know it's running a 64 bit kernel
because the eBPF JIT is only included in kernels with CONFIG_64BIT=y due
to Kconfig dependencies.

> @@ -1006,8 +1048,25 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>  			emit_instr(ctx, dsubu, MIPS_R_T8, dst, src);
>  			emit_instr(ctx, sltu, MIPS_R_AT, dst, src);
>  			/* SP known to be non-zero, movz becomes boolean not */
> -			emit_instr(ctx, movz, MIPS_R_T9, MIPS_R_SP, MIPS_R_T8);
> -			emit_instr(ctx, movn, MIPS_R_T9, MIPS_R_ZERO, MIPS_R_T8);
> +			if (MIPS_ISA_REV >= 6) {
> +				emit_instr(ctx, seleqz, MIPS_R_T7,
> +						MIPS_R_SP, MIPS_R_T8);
> +				emit_instr(ctx, selnez, MIPS_R_T9,
> +						MIPS_R_T9, MIPS_R_T8);
> +				emit_instr(ctx, or, MIPS_R_T9,
> +						MIPS_R_T9, MIPS_R_T7);
> +				emit_instr(ctx, selnez, MIPS_R_T7,
> +						MIPS_R_ZERO, MIPS_R_T8);
> +				emit_instr(ctx, seleqz, MIPS_R_T9,
> +						MIPS_R_T9, MIPS_R_T8);
> +				emit_instr(ctx, or, MIPS_R_T9,
> +						MIPS_R_T9, MIPS_R_T7);
> +			} else {
> +				emit_instr(ctx, movz, MIPS_R_T9,
> +						MIPS_R_SP, MIPS_R_T8);
> +				emit_instr(ctx, movn, MIPS_R_T9,
> +						MIPS_R_ZERO, MIPS_R_T8);
> +			}
>  			emit_instr(ctx, or, MIPS_R_AT, MIPS_R_T9, MIPS_R_AT);
>  			cmp_eq = bpf_op == BPF_JGT;
>  			dst = MIPS_R_AT;

Looking at this, I think we can simplify it a lot. What the original
movn/movz code is doing is this:

	/* movz */
	if (!t8)
		t9 = sp;

	/* movn */
	if (t8)
		t9 = 0;

Or, simplified:

	t9 = t8 ? sp : 0;

This actually matches up very well with the r6 selnez instruction, so I
think the MIPSr6 version can be just one instruction:

	selnez	t9, sp, t8

Could you give that a try?

> @@ -1245,7 +1304,6 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>  		if (emit_bpf_tail_call(ctx, this_idx))
>  			return -EINVAL;
>  		break;
> -
>  	case BPF_ALU | BPF_END | BPF_FROM_BE:
>  	case BPF_ALU | BPF_END | BPF_FROM_LE:
>  		dst = ebpf_to_mips_reg(ctx, insn, dst_reg);

Please leave that empty line there; it helps readability.

> @@ -1366,6 +1424,12 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>  		if (src < 0)
>  			return src;
>  		if (BPF_MODE(insn->code) == BPF_XADD) {
> +			if (MIPS_ISA_REV >= 6) {
> +				emit_instr(ctx, daddiu, MIPS_R_T6,
> +						dst, mem_off);
> +				mem_off = 0;
> +				dst = MIPS_R_T6;
> +			}
>  			switch (BPF_SIZE(insn->code)) {
>  			case BPF_W:
>  				if (get_reg_val_type(ctx, this_idx, insn->src_reg) == REG_32BIT) {

It might be worth adding a comment before your if statement explaining
that in MIPSr6 the encoded immediate for ll/sc instructions shrunk from
16 bits to 9 bits, hence the need for the temporary register.

Actually you could conditionalise this upon mem_off actually being too
big for the 9 bit field. For example something like:

	if ((mem_off >= 0) && (mem_off < BIT(9))) {
		/*
		 * mem_off is positive & fits within the 9 bit ll/sc
		 * instruction immediate field.
		 */
	} else if ((mem_off < 0) && (-mem_off <= BIT(9))) {
		/*
		 * mem_off is negative & fits within the 9 bit ll/sc
		 * instruction immediate field.
		 */
	} else if (MIPS_ISA_REV >= 6) {
		use the temp register
	}

That way we'd avoid the temp register in cases where the immediate is
small enough to fit in the instruction anyway.

And further this all makes me wonder what happens if we have an
immediate that doesn't fit in 16 bits... I suspect we have a bug there
on both pre-r6 (ll/sc immediate field overflow) & r6 (daddiu immediate
field overflow). Though in practice I don't know if it's possible to hit
it, and it's not a new issue so let's look at that separately & not gate
this work on it.

One last comment - it'd be good to copy the BPF maintainers too for v2.
You can get the full list of people to email from get_maintainer.pl:

$ ./scripts/get_maintainer.pl -f arch/mips/net/ebpf_jit.c
Paul Burton <paul.burton@xxxxxxxx> (maintainer:BPF JIT for MIPS (32-BIT AND 64-BIT))
Ralf Baechle <ralf@xxxxxxxxxxxxxx> (supporter:MIPS)
James Hogan <jhogan@xxxxxxxxxx> (supporter:MIPS)
Alexei Starovoitov <ast@xxxxxxxxxx> (supporter:BPF (Safe dynamic programs and tools))
Daniel Borkmann <daniel@xxxxxxxxxxxxx> (supporter:BPF (Safe dynamic programs and tools))
Martin KaFai Lau <kafai@xxxxxx> (reviewer:BPF (Safe dynamic programs and tools))
Song Liu <songliubraving@xxxxxx> (reviewer:BPF (Safe dynamic programs and tools))
Yonghong Song <yhs@xxxxxx> (reviewer:BPF (Safe dynamic programs and tools))
netdev@xxxxxxxxxxxxxxx (open list:BPF JIT for MIPS (32-BIT AND 64-BIT))
bpf@xxxxxxxxxxxxxxx (open list:BPF JIT for MIPS (32-BIT AND 64-BIT))
linux-mips@xxxxxxxxxxxxxxx (open list:MIPS)
linux-kernel@xxxxxxxxxxxxxxx (open list)

Thanks,
    Paul