Re: [PATCH v2 3/8] bpf powerpc: refactor JIT compiler code

[Hari Bathini <hbathini@linux.ibm.com>]

Hi Christophe,

Thanks for reviewing the series.

On 17/09/21 9:40 pm, Christophe Leroy wrote:
> 
> 
> Le 17/09/2021 à 17:30, Hari Bathini a écrit :
>> Refactor powerpc JITing. This simplifies adding BPF_PROBE_MEM support.
> 
> Could you describe a bit more what you are refactoring exactly ?

I am trying to do more than BPF_PROBE_MEM needs. Will keep the changes 
minimal (BPF_PROBE_MEM specific) and update the changelog..

> 
> 
>>
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>
>> Changes in v2:
>> * New patch to refactor a bit of JITing code.
>>
>>
>>   arch/powerpc/net/bpf_jit_comp32.c | 50 +++++++++++---------
>>   arch/powerpc/net/bpf_jit_comp64.c | 76 ++++++++++++++++---------------
>>   2 files changed, 68 insertions(+), 58 deletions(-)
>>
>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
>> b/arch/powerpc/net/bpf_jit_comp32.c
>> index b60b59426a24..c8ae14c316e3 100644
>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>> @@ -276,17 +276,17 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 
>> *image, struct codegen_context *
>>       u32 exit_addr = addrs[flen];
>>       for (i = 0; i < flen; i++) {
>> -        u32 code = insn[i].code;
>>           u32 dst_reg = bpf_to_ppc(ctx, insn[i].dst_reg);
>> -        u32 dst_reg_h = dst_reg - 1;
>>           u32 src_reg = bpf_to_ppc(ctx, insn[i].src_reg);
>> -        u32 src_reg_h = src_reg - 1;
>>           u32 tmp_reg = bpf_to_ppc(ctx, TMP_REG);
>> +        u32 true_cond, code = insn[i].code;
>> +        u32 dst_reg_h = dst_reg - 1;
>> +        u32 src_reg_h = src_reg - 1;
> 
> All changes above seems unneeded and not linked to the current patch. 
> Please leave cosmetic changes outside and focus on necessary changes.
> 
>> +        u32 size = BPF_SIZE(code);
>>           s16 off = insn[i].off;
>>           s32 imm = insn[i].imm;
>>           bool func_addr_fixed;
>>           u64 func_addr;
>> -        u32 true_cond;
>>           /*
>>            * addrs[] maps a BPF bytecode address into a real offset from
>> @@ -809,25 +809,33 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 
>> *image, struct codegen_context *
>>           /*
>>            * BPF_LDX
>>            */
>> -        case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + 
>> off) */
>> -            EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
>> -            if (!fp->aux->verifier_zext)
>> -                EMIT(PPC_RAW_LI(dst_reg_h, 0));
>> -            break;
>> -        case BPF_LDX | BPF_MEM | BPF_H: /* dst = *(u16 *)(ul) (src + 
>> off) */
>> -            EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
>> -            if (!fp->aux->verifier_zext)
>> -                EMIT(PPC_RAW_LI(dst_reg_h, 0));
>> -            break;
>> -        case BPF_LDX | BPF_MEM | BPF_W: /* dst = *(u32 *)(ul) (src + 
>> off) */
>> -            EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
>> -            if (!fp->aux->verifier_zext)
>> +        /* dst = *(u8 *)(ul) (src + off) */
>> +        case BPF_LDX | BPF_MEM | BPF_B:
>> +        /* dst = *(u16 *)(ul) (src + off) */
>> +        case BPF_LDX | BPF_MEM | BPF_H:
>> +        /* dst = *(u32 *)(ul) (src + off) */
>> +        case BPF_LDX | BPF_MEM | BPF_W:
>> +        /* dst = *(u64 *)(ul) (src + off) */
>> +        case BPF_LDX | BPF_MEM | BPF_DW:
> Why changing the location of the comments ? I found it more readable 
> before.

Sure. I will revert that change.

>> +            switch (size) {
>> +            case BPF_B:
>> +                EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
>> +                break;
>> +            case BPF_H:
>> +                EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
>> +                break;
>> +            case BPF_W:
>> +                EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
>> +                break;
>> +            case BPF_DW:
>> +                EMIT(PPC_RAW_LWZ(dst_reg_h, src_reg, off));
>> +                EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off + 4));
>> +                break;
>> +            }
> 
> BPF_B, BPF_H, ... are not part of an enum. Are you sure GCC is happy to 
> have no default ?

I used gcc 10.3 for ppc32 & gcc 8.3 for ppc64. No warnings.
Though, no harm adding the below, I guess..

	default:
		break;

Thanks
Hari