Re: [PATCH bpf-next] bpf, mips: Add support for BPF_SDIV and BPF_SMOD in the mips JIT

[Philippe Mathieu-Daudé]

Hi Nicholas,

On 16/7/26 23:10, Nicholas Dudar wrote:
> build_insn() picks the div/mod opcode by ALU width alone and never
> reads insn->off, so the mips32 and mips64 JITs do not implement signed
> BPF_SDIV and BPF_SMOD (off == 1). Signed div/mod get an unsigned
> quotient and remainder rather than the signed result the verifier and
> interpreter produce for negative operands. Neither JIT has a signed
> divide opcode or C helper.
>
> Add the signed path alongside the unsigned one:
>
> - uasm gains div/ddiv/mod/dmod emitters (and their R6 forms) next to
>    the existing divu/ddivu/modu/dmodu. This is shared MIPS assembler
>    infrastructure, so it lives with the unsigned emitters it mirrors,
>    not in the JIT.
> - The 32-bit and 64-bit JITs thread an is_signed flag (from
>    insn->off == 1) through emit_alu_r()/emit_alu_r64() and pick the
>    signed opcode. On 32-bit MIPS, ALU64 runs through a C helper, so
>    add div64_s64() and a signed remainder helper beside the existing
>    div64_u64()/jit_mod64().
> - valid_alu_i() drops the divide-by-power-of-two strength reduction
>    (/ (1<<n) -> >> n, % (1<<n) -> & mask) for the signed case. An
>    arithmetic shift rounds toward negative infinity and a mask drops
>    the sign, so the signed K-form falls through to the register path.
>
> The K-form rejects imm == 0 at verify time, and MIPS div/ddiv, like
> divu/ddivu, does not trap on S32_MIN/-1 or S64_MIN/-1. The ALU32
> result is zero-extended after each div/mod, for the signed and
> unsigned cases.
>
> Signed-off-by: Nicholas Dudar <main.kalliope@xxxxxxxxx>
> Assisted-by: Claude:claude-opus-4-8
> ---
>   arch/mips/include/asm/uasm.h   |  6 ++++
>   arch/mips/mm/uasm-mips.c       | 10 ++++++
>   arch/mips/mm/uasm.c            | 15 ++++++---
>   arch/mips/net/bpf_jit_comp.c   | 47 ++++++++++++++++++++++------
>   arch/mips/net/bpf_jit_comp.h   |  4 +--
>   arch/mips/net/bpf_jit_comp32.c | 28 +++++++++++------
>   arch/mips/net/bpf_jit_comp64.c | 57 ++++++++++++++++++++++------------
>   7 files changed, 122 insertions(+), 45 deletions(-)


> diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c
> index 6ee4abe6a1f7..504daea4aac2 100644
> --- a/arch/mips/net/bpf_jit_comp.c
> +++ b/arch/mips/net/bpf_jit_comp.c
> @@ -208,7 +208,7 @@ void emit_mov_r(struct jit_context *ctx, u8 dst, u8 src)
>   }
>   
>   /* Validate ALU immediate range */
> -bool valid_alu_i(u8 op, s32 imm)
> +bool valid_alu_i(u8 op, s32 imm, bool is_signed)
>   {
>   	switch (BPF_OP(op)) {
>   	case BPF_NEG:
> @@ -237,6 +237,15 @@ bool valid_alu_i(u8 op, s32 imm)
>   		return imm == 0 || (imm > 0 && is_power_of_2(imm));
>   	case BPF_DIV:
>   	case BPF_MOD:
> +		/*
> +		 * The rewrite to a shift/mask below is an unsigned-only
> +		 * optimization: arithmetic right shift rounds toward
> +		 * negative infinity rather than zero, and masking does not
> +		 * reproduce a negative dividend's sign. Force the signed
> +		 * case through the general register path instead.
> +		 */
> +		if (is_signed)
> +			return false;
>   		/* imm must be an 17-bit power of two */
>   		return (u32)imm <= 0x10000 && is_power_of_2((u32)imm);
>   	}
> @@ -339,7 +348,7 @@ void emit_alu_i(struct jit_context *ctx, u8 dst, s32 imm, u8 op)
>   }
>   
>   /* ALU register operation (32-bit) */
> -void emit_alu_r(struct jit_context *ctx, u8 dst, u8 src, u8 op)
> +void emit_alu_r(struct jit_context *ctx, u8 dst, u8 src, u8 op, bool is_signed)
>   {
>   	switch (BPF_OP(op)) {
>   	/* dst = dst & src */
> @@ -385,20 +394,38 @@ void emit_alu_r(struct jit_context *ctx, u8 dst, u8 src, u8 op)
>   		break;
>   	/* dst = dst / src */
>   	case BPF_DIV:
> -		if (cpu_has_mips32r6) {
> -			emit(ctx, divu_r6, dst, dst, src);
> +		if (is_signed) {
> +			if (cpu_has_mips32r6) {
> +				emit(ctx, div_r6, dst, dst, src);
> +			} else {
> +				emit(ctx, div, dst, src);
> +				emit(ctx, mflo, dst);
> +			}
>   		} else {
> -			emit(ctx, divu, dst, src);
> -			emit(ctx, mflo, dst);
> +			if (cpu_has_mips32r6) {
> +				emit(ctx, divu_r6, dst, dst, src);
> +			} else {
> +				emit(ctx, divu, dst, src);
> +				emit(ctx, mflo, dst);
> +			}
>   		}
>   		break;
>   	/* dst = dst % src */
>   	case BPF_MOD:
> -		if (cpu_has_mips32r6) {
> -			emit(ctx, modu, dst, dst, src);
> +		if (is_signed) {
> +			if (cpu_has_mips32r6) {
> +				emit(ctx, mod, dst, dst, src);
> +			} else {
> +				emit(ctx, div, dst, src);
> +				emit(ctx, mfhi, dst);
> +			}
>   		} else {
> -			emit(ctx, divu, dst, src);
> -			emit(ctx, mfhi, dst);
> +			if (cpu_has_mips32r6) {
> +				emit(ctx, modu, dst, dst, src);
> +			} else {
> +				emit(ctx, divu, dst, src);
> +				emit(ctx, mfhi, dst);
> +			}
>   		}
>   		break;
>   	}
Your patch would be clearer if you extract a pair of emit_div()
and emit_mod() helpers in a preliminary patch, then include the
32r6 support (this patch).

My 2 cents anyway.

Regards,

Phil.