Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221013154000.3462836-1-chenhuacai@loongson.cn>
 <8a8fa581-94a9-649d-8c01-f1afd4bc9514@xen0n.name> <CAAhV-H5U1c_wfWLuxMaHD6c9-k+g-iSqgtcJVwceoL13J7hEiA@mail.gmail.com>
 <ae6260e5-0eb6-a615-7032-6481cd186f3f@loongson.cn>
In-Reply-To: <ae6260e5-0eb6-a615-7032-6481cd186f3f@loongson.cn>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Fri, 14 Oct 2022 16:58:07 +0800
Message-ID: <CAAhV-H4H5x=XmShR7s_=bwTE8mN37QTt7t8QRaYy1i3oYQKWgw@mail.gmail.com>
Subject: Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case
To:     Tiezhu Yang <yangtiezhu@loongson.cn>
Cc:     WANG Xuerui <kernel@xen0n.name>,
        Huacai Chen <chenhuacai@loongson.cn>,
        loongarch@lists.linux.dev, Xuefeng Li <lixuefeng@loongson.cn>,
        Guo Ren <guoren@kernel.org>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 14, 2022 at 10:18 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>
>
> On 10/14/2022 09:13 AM, Huacai Chen wrote:
> > Hi, Xuerui,
> >
> > On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <kernel@xen0n.name> wrote:
> >>
> >> On 10/13/22 23:40, Huacai Chen wrote:
> >>> Not all compilers support declare variables in switch-case, so move
> >>> declarations to the beginning of a function. Otherwise we may get such
> >>> build errors:
>
> ...
>
> >>>
> >>>   static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
> >>>   {
> >>> -     const bool is32 = BPF_CLASS(insn->code) == BPF_ALU ||
> >>> -                       BPF_CLASS(insn->code) == BPF_JMP32;
> >>> +     u8 t0 = -1;
> >> Here "t0" seems to be a versatile temp value, while the "t1" below is
> >> the actual GPR $t1. What about renaming "t0" to something like "tmp" to
> >> reduce confusion? I believe due to things like "t0 = LOONGARCH_GPR_ZERO"
> >> the "t0" is 100% not an actual mapping to $t0.
> > I rename t7 to t0 because there is no t3-t6, t7 looks very strange.
> > But from emit_cond_jmp() the 3rd and 4th parameters have no difference
> > so I suppose t0 is just OK, then whether rename it to tmp depends on
> > Tiezhu's opinion.
> >
>
> Use "tmp" seems better due to it is a temp value.
OK, then I will use tmp or just tm for alignment.

>
> >>> +     u64 func_addr;
> >>> +     bool func_addr_fixed;
> >>> +     int i = insn - ctx->prog->insnsi;
> >>> +     int ret, jmp_offset;
> >>>       const u8 code = insn->code;
> >>>       const u8 cond = BPF_OP(code);
> >>>       const u8 t1 = LOONGARCH_GPR_T1;
> >>> @@ -400,8 +402,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> >>>       const u8 dst = regmap[insn->dst_reg];
> >>>       const s16 off = insn->off;
> >>>       const s32 imm = insn->imm;
> >>> -     int jmp_offset;
> >>> -     int i = insn - ctx->prog->insnsi;
> >>> +     const u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> >>> +     const bool is32 = BPF_CLASS(insn->code) == BPF_ALU || BPF_CLASS(insn->code) == BPF_JMP32;
> >> Please consider reducing diff damage and not touching parts not directly
> >> affected by this change. For example this "is32" declaration and
> >> initialization was moved although not related to this change.
>
> It looks reasonable, one change per patch is better.
>
> > I think defining variables from simple to complex and grouping them
> > can make life easier. :)
> >
>
> No strong opinion on this, I am OK either way.
>
> Thanks,
> Tiezhu
>
>