Re: [PATCH bpf-next v4 08/10] bpf, x86: Create two helpers for some arith operations

[Yonghong Song <yonghong.song@linux.dev>]

On 10/10/24 1:21 PM, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 10:56 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Two helpers are extracted from bpf/x86 jit:
>>    - a helper to handle 'reg1 <op>= reg2' where <op> is add/sub/and/or/xor
>>    - a helper to handle 'reg *= imm'
>>
>> Both helpers will be used in the subsequent patch.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   arch/x86/net/bpf_jit_comp.c | 51 ++++++++++++++++++++++++-------------
>>   1 file changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index a6ba85cec49a..297dd64f4b6a 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1475,6 +1475,37 @@ static void emit_alu_helper_1(u8 **pprog, u8 insn_code, u32 dst_reg, s32 imm32)
>>          *pprog = prog;
>>   }
>>
>> +/* emit ADD/SUB/AND/OR/XOR 'reg1 <op>= reg2' operations */
>> +static void emit_alu_helper_2(u8 **pprog, u8 insn_code, u32 dst_reg, u32 src_reg)
>> +{
>> +       u8 b2 = 0;
>> +       u8 *prog = *pprog;
>> +
>> +       maybe_emit_mod(&prog, dst_reg, src_reg,
>> +                      BPF_CLASS(insn_code) == BPF_ALU64);
>> +       b2 = simple_alu_opcodes[BPF_OP(insn_code)];
>> +       EMIT2(b2, add_2reg(0xC0, dst_reg, src_reg));
>> +
>> +       *pprog = prog;
>> +}
>> +
>> +/* emit 'reg *= imm' operations */
>> +static void emit_alu_helper_3(u8 **pprog, u8 insn_code, u32 dst_reg, s32 imm32)
> _1, _2, _3 ?!
>
> There must be a better way to name the helpers. Like:
>
> _1 -> emit_alu_imm
> _2 -> emit_alu_reg
> _3 -> emit_mul_imm

I struggle to get a proper name here. I originally thought about to use
emit_alu_reg_imm, emit_alu_reg_reg, but in my case, even emit_alu_reg_imm
only supports add/sub/and/or/xor and it does not support mul/div/mod, so
emit_alu_reg_imm does not really cover all alu operations so I chose
another name which is also not good.

I guess I can use the above you suggested in the above which actually
covers most alu operations.