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: <20220829133146.15236-1-xry111@xry111.site> <20220829133146.15236-6-xry111@xry111.site>
 <CAAhV-H77jQvcJL=LF0dknd7EjiWYMZL3ZRMM1drsGkO33UM_Jg@mail.gmail.com> <4033aec9-8df5-53ae-59e1-9ec3ade5f6d7@xen0n.name>
In-Reply-To: <4033aec9-8df5-53ae-59e1-9ec3ade5f6d7@xen0n.name>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Tue, 30 Aug 2022 15:10:29 +0800
Message-ID: <CAAhV-H4PXsBop6c3or92hv01YCiY=JFLnTwjSykaeN21+YvwFQ@mail.gmail.com>
Subject: Re: [PATCH v6 5/6] LoongArch: Support PC-relative relocations in modules
To:     WANG Xuerui <kernel@xen0n.name>
Cc:     Xi Ruoyao <xry111@xry111.site>, loongarch@lists.linux.dev,
        LKML <linux-kernel@vger.kernel.org>,
        Youling Tang <tangyouling@loongson.cn>,
        Jinyang He <hejinyang@loongson.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 30, 2022 at 9:42 AM WANG Xuerui <kernel@xen0n.name> wrote:
>
> On 2022/8/29 23:08, Huacai Chen wrote:
> > [snip]
> >> +static int apply_r_larch_pcala(struct module *mod, u32 *location, Elf_Addr v,
> >> +                       s64 *rela_stack, size_t *rela_stack_top, unsigned int type)
> >> +{
> >> +       union loongarch_instruction *insn = (union loongarch_instruction *)location;
> >> +       /* Use s32 for a sign-extension deliberately. */
> >> +       s32 offset_hi20 = (void *)((v + 0x800) & ~0xfff) -
> >> +               (void *)((Elf_Addr)location & ~0xfff);
> >> +       Elf_Addr anchor = (((Elf_Addr)location) & ~0xfff) + offset_hi20;
> >> +       ptrdiff_t offset_rem = (void *)v - (void *)anchor;
> >> +
> >> +       switch (type) {
> >> +       case R_LARCH_PCALA_HI20:
> >> +               v = offset_hi20 >> 12;
> >> +               break;
> >> +       case R_LARCH_PCALA64_LO20:
> >> +               v = offset_rem >> 32;
> >> +               break;
> >> +       case R_LARCH_PCALA64_HI12:
> >> +               v = offset_rem >> 52;
> >> +               break;
> >> +       default:
> >> +               /* Do nothing. */
> >> +       }
> >> +
> >> +       switch (type) {
> >> +       case R_LARCH_PCALA_HI20:
> >> +       case R_LARCH_PCALA64_LO20:
> >> +               insn->reg1i20_format.immediate = v & 0xfffff;
> >> +               break;
> >> +       case R_LARCH_PCALA_LO12:
> >> +       case R_LARCH_PCALA64_HI12:
> >> +               insn->reg2i12_format.immediate = v & 0xfff;
> >> +               break;
> >> +       default:
> >> +               pr_err("%s: Unsupport relocation type %u\n", mod->name, type);
> >> +               return -EINVAL;
> >> +       }
> > Can we merge the two switch here?
>
> IMO leaving as-is or even splitting into two functions would be
> acceptable, as the two switches are performing two different things --
> namely "adjustFixupValue" (in LLVM-speak) and actually inserting the
> value into the insn word. But an argument for merging the two can be
> made too, because the v2.00 reloc types are purposely designed with
> unique use case for each, meaning there is actually no flexibility in
> between the fixup value's calculation and application. So I think this
> eventually comes down to coder's preference?
Merging them can just make me understand the logic better, and save
some lines. :)

Huacai
>
> --
> WANG "xen0n" Xuerui
>
> Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
>