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;
Subject: Re: [PATCH v1 4/6] objtool/LoongArch: Enable orc to be built
To:     Peter Zijlstra <peterz@infradead.org>
References: <1690272910-11869-1-git-send-email-yangtiezhu@loongson.cn>
 <1690272910-11869-5-git-send-email-yangtiezhu@loongson.cn>
 <20230725115609.GD3765278@hirez.programming.kicks-ass.net>
Cc:     Josh Poimboeuf <jpoimboe@kernel.org>,
        Huacai Chen <chenhuacai@kernel.org>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org, loongson-kernel@lists.loongnix.cn
From:   Tiezhu Yang <yangtiezhu@loongson.cn>
Message-ID: <d49af603-82cf-a88e-37db-1c031c5f8881@loongson.cn>
Date:   Fri, 28 Jul 2023 09:31:26 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20230725115609.GD3765278@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 07/25/2023 07:56 PM, Peter Zijlstra wrote:
> On Tue, Jul 25, 2023 at 04:15:08PM +0800, Tiezhu Yang wrote:
>
>> +void arch_write_orc(struct elf *elf, struct orc_entry *orc)
>> +{
>> +	orc->ra_offset = bswap_if_needed(elf, orc->ra_offset);
>> +}
>
> This hardly 'writes' anything, all it does is a bswap.
>
>> diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
>> index 1eff7e0a..6975056 100644
>> --- a/tools/objtool/orc_gen.c
>> +++ b/tools/objtool/orc_gen.c
>> @@ -14,6 +14,8 @@
>>  #include <objtool/warn.h>
>>  #include <objtool/endianness.h>
>>
>> +void __weak arch_write_orc(struct elf *elf, struct orc_entry *orc) {}
>
> And for that you need a weak function? What's wrong with an inline if
> anything?
>
>>  static int write_orc_entry(struct elf *elf, struct section *orc_sec,
>>  			   struct section *ip_sec, unsigned int idx,
>>  			   struct section *insn_sec, unsigned long insn_off,
>> @@ -26,6 +28,7 @@ static int write_orc_entry(struct elf *elf, struct section *orc_sec,
>>  	memcpy(orc, o, sizeof(*orc));
>>  	orc->sp_offset = bswap_if_needed(elf, orc->sp_offset);
>>  	orc->bp_offset = bswap_if_needed(elf, orc->bp_offset);
>> +	arch_write_orc(elf, orc);
>
> Why can't this simply be:
>
> 	orc->ra_offset = bswap_if_needed(elf, orc->ra_offset);
>
> AFAICT this won't actually do anything for x86 and I don't think Power
> got around to doing ORC yet.
>
>>
>>  	/* populate reloc for ip */
>>  	if (!elf_init_reloc_text_sym(elf, ip_sec, idx * sizeof(int), idx,
>
> Again, not much explaination for why you did things
>

The initial aim is to keep consistent with the above two lines,
but for now, I think there is no need to call bswap_if_needed()
for LoongArch, the related changes are not necessary, I will
drop them too.

LoongArch is little-endian, the __BYTE_ORDER is defined as
__LITTLE_ENDIAN, and elf->ehdr.e_ident[EI_DATA] is 1 which
is equal with ELFDATA2LSB, so need_bswap() returns false.

static inline bool need_bswap(struct elf *elf)
{
	return (__BYTE_ORDER == __LITTLE_ENDIAN) ^
	       (elf->ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
}

Thank you very much for your review, sorry for that.

Thanks,
Tiezhu