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;
Message-ID: <4033aec9-8df5-53ae-59e1-9ec3ade5f6d7@xen0n.name>
Date:   Tue, 30 Aug 2022 09:42:27 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:106.0)
 Gecko/20100101 Thunderbird/106.0a1
Subject: Re: [PATCH v6 5/6] LoongArch: Support PC-relative relocations in
 modules
Content-Language: en-US
To:     Huacai Chen <chenhuacai@kernel.org>, Xi Ruoyao <xry111@xry111.site>
Cc:     loongarch@lists.linux.dev, LKML <linux-kernel@vger.kernel.org>,
        Youling Tang <tangyouling@loongson.cn>,
        Jinyang He <hejinyang@loongson.cn>
References: <20220829133146.15236-1-xry111@xry111.site>
 <20220829133146.15236-6-xry111@xry111.site>
 <CAAhV-H77jQvcJL=LF0dknd7EjiWYMZL3ZRMM1drsGkO33UM_Jg@mail.gmail.com>
From:   WANG Xuerui <kernel@xen0n.name>
In-Reply-To: <CAAhV-H77jQvcJL=LF0dknd7EjiWYMZL3ZRMM1drsGkO33UM_Jg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/