Re: [PATCH bpf-next v3 1/3] bpf, x86: Add support for signed arena loads

[Puranjay Mohan <puranjay@kernel.org>]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Mon, 2025-09-15 at 16:28 +0000, Puranjay Mohan wrote:
>
> [...]
>
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index 008273a53e04..f2b85a10add2 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>> @@ -3064,6 +3064,11 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
>>  		if (!bpf_atomic_is_load_store(insn) &&
>>  		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
>>  			return false;
>> +		break;
>> +	case BPF_LDX | BPF_MEMSX | BPF_B:
>> +	case BPF_LDX | BPF_MEMSX | BPF_H:
>> +	case BPF_LDX | BPF_MEMSX | BPF_W:
>> +		return false;
>>  	}
>>  	return true;
>>  }
>
> Is the same hunk necessary in riscv/net/bpf_jit_comp64.c?

Yes, will add in next version.

> [...]
>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 8d34a9400a5e..a6550da34268 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1152,11 +1152,38 @@ static void emit_ldx_index(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, u32 i
>>  	*pprog = prog;
>>  }
>>  
>> +static void emit_ldsx_index(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, u32 index_reg, int off)
>> +{
>> +	u8 *prog = *pprog;
>> +
>> +	switch (size) {
>> +	case BPF_B:
>> +		/* movsx rax, byte ptr [rax + r12 + off] */
>> +		EMIT3(add_3mod(0x48, src_reg, dst_reg, index_reg), 0x0F, 0xBE);
>> +		break;
>> +	case BPF_H:
>> +		/* movsx rax, word ptr [rax + r12 + off] */
>> +		EMIT3(add_3mod(0x48, src_reg, dst_reg, index_reg), 0x0F, 0xBF);
>> +		break;
>> +	case BPF_W:
>> +		/* movsx rax, dword ptr [rax + r12 + off] */
>> +		EMIT2(add_3mod(0x48, src_reg, dst_reg, index_reg), 0x63);
>> +		break;
>> +	}
>> +	emit_insn_suffix_SIB(&prog, src_reg, dst_reg, index_reg, off);
>> +	*pprog = prog;
>> +}
>> +
>
> Encoding looks correct.
>
> [...]
>
>> @@ -2109,13 +2136,19 @@ st:			if (is_imm8(insn->off))
>>  		case BPF_LDX | BPF_PROBE_MEM32 | BPF_H:
>>  		case BPF_LDX | BPF_PROBE_MEM32 | BPF_W:
>>  		case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
>> +		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_B:
>> +		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_H:
>> +		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_W:
>>  		case BPF_STX | BPF_PROBE_MEM32 | BPF_B:
>>  		case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
>>  		case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
>>  		case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
>>  			start_of_ldx = prog;
>>  			if (BPF_CLASS(insn->code) == BPF_LDX)
>> -				emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>> +				if (BPF_MODE(insn->code) == BPF_PROBE_MEM32SX)
>> +					emit_ldsx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>> +				else
>> +					emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>>  			else
>>  				emit_stx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>
> Heh, apparently this is correct C code. Dangling else is associated
> with the closest 'if' statement. Didn't know that.

This code goes against kernel coding style, I should have used braces.
Will fix it in next version.