All of lore.kernel.org
 help / color / mirror / Atom feed
From: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
To: kexec@lists.infradead.org
Subject: [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
Date: Thu, 19 May 2022 14:43:38 +0530	[thread overview]
Message-ID: <1652950421.sq10xtycz9.naveen@linux.ibm.com> (raw)
In-Reply-To: <20220518134705.7ae186d5419b24d689a38ccc@linux-foundation.org>

Andrew Morton wrote:
> On Wed, 18 May 2022 23:48:28 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Since commit d1bcae833b32f1 ("ELF: Don't generate unused section
>> symbols") [1], binutils (v2.36+) started dropping section symbols that
>> it thought were unused.  This isn't an issue in general, but with
>> kexec_file.c, gcc is placing kexec_arch_apply_relocations[_add] into a
>> separate .text.unlikely section and the section symbol ".text.unlikely"
>> is being dropped. Due to this, recordmcount is unable to find a non-weak
>> symbol in .text.unlikely to generate a relocation record against.
>> 
>> Address this by dropping the weak attribute from these functions:
>> - arch_kexec_apply_relocations() is not overridden by any architecture
>>   today, so just drop the weak attribute.
>> - arch_kexec_apply_relocations_add() is only overridden by x86 and s390.
>>   Retain the function prototype for those and move the weak
>>   implementation into the header as a static inline for other
>>   architectures.
>> 
>> ...
>>
> 
> Sigh.  This patch demonstrates why I like __weak :<
> 
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -229,6 +225,30 @@ extern int crash_exclude_mem_range(struct crash_mem *mem,
>>  				   unsigned long long mend);
>>  extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
>>  				       void **addr, unsigned long *sz);
>> +
>> +#if defined(CONFIG_X86_64) || defined(CONFIG_S390)
> 
> Let's avoid listing the architectures here?  Better to add
> 
> 	select ARCH_HAVE_ARCH_KEXEC_APPLY_RELOCATIONS_ADD
> 
> to arch/<arch>/Kconfig?

I followed the approach used in commit 6e7b64b9dd6d96 ("elfcore: fix 
building with clang") since here again, it was overridden on only two 
architectures. I also wanted to avoid touching the architecture headers 
so as to make it simpler to backport.

But, as Michael points out, using a #ifdef isn't too much of a change 
either. I also confirmed that those changes still apply cleanly all the 
way back to v5.10. I've posted a v2 which takes this approach.

> 
> Please cc me on any additional work on this.

I've copied you on the v2 patch. Thanks!


- Naveen



WARNING: multiple messages have this Message-ID (diff)
From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org,
	Eric Biederman <ebiederm@xmission.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
Date: Thu, 19 May 2022 14:43:38 +0530	[thread overview]
Message-ID: <1652950421.sq10xtycz9.naveen@linux.ibm.com> (raw)
In-Reply-To: <20220518134705.7ae186d5419b24d689a38ccc@linux-foundation.org>

Andrew Morton wrote:
> On Wed, 18 May 2022 23:48:28 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Since commit d1bcae833b32f1 ("ELF: Don't generate unused section
>> symbols") [1], binutils (v2.36+) started dropping section symbols that
>> it thought were unused.  This isn't an issue in general, but with
>> kexec_file.c, gcc is placing kexec_arch_apply_relocations[_add] into a
>> separate .text.unlikely section and the section symbol ".text.unlikely"
>> is being dropped. Due to this, recordmcount is unable to find a non-weak
>> symbol in .text.unlikely to generate a relocation record against.
>> 
>> Address this by dropping the weak attribute from these functions:
>> - arch_kexec_apply_relocations() is not overridden by any architecture
>>   today, so just drop the weak attribute.
>> - arch_kexec_apply_relocations_add() is only overridden by x86 and s390.
>>   Retain the function prototype for those and move the weak
>>   implementation into the header as a static inline for other
>>   architectures.
>> 
>> ...
>>
> 
> Sigh.  This patch demonstrates why I like __weak :<
> 
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -229,6 +225,30 @@ extern int crash_exclude_mem_range(struct crash_mem *mem,
>>  				   unsigned long long mend);
>>  extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
>>  				       void **addr, unsigned long *sz);
>> +
>> +#if defined(CONFIG_X86_64) || defined(CONFIG_S390)
> 
> Let's avoid listing the architectures here?  Better to add
> 
> 	select ARCH_HAVE_ARCH_KEXEC_APPLY_RELOCATIONS_ADD
> 
> to arch/<arch>/Kconfig?

I followed the approach used in commit 6e7b64b9dd6d96 ("elfcore: fix 
building with clang") since here again, it was overridden on only two 
architectures. I also wanted to avoid touching the architecture headers 
so as to make it simpler to backport.

But, as Michael points out, using a #ifdef isn't too much of a change 
either. I also confirmed that those changes still apply cleanly all the 
way back to v5.10. I've posted a v2 which takes this approach.

> 
> Please cc me on any additional work on this.

I've copied you on the v2 patch. Thanks!


- Naveen


WARNING: multiple messages have this Message-ID (diff)
From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Biederman <ebiederm@xmission.com>,
	kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org,
	Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
Date: Thu, 19 May 2022 14:43:38 +0530	[thread overview]
Message-ID: <1652950421.sq10xtycz9.naveen@linux.ibm.com> (raw)
In-Reply-To: <20220518134705.7ae186d5419b24d689a38ccc@linux-foundation.org>

Andrew Morton wrote:
> On Wed, 18 May 2022 23:48:28 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> Since commit d1bcae833b32f1 ("ELF: Don't generate unused section
>> symbols") [1], binutils (v2.36+) started dropping section symbols that
>> it thought were unused.  This isn't an issue in general, but with
>> kexec_file.c, gcc is placing kexec_arch_apply_relocations[_add] into a
>> separate .text.unlikely section and the section symbol ".text.unlikely"
>> is being dropped. Due to this, recordmcount is unable to find a non-weak
>> symbol in .text.unlikely to generate a relocation record against.
>> 
>> Address this by dropping the weak attribute from these functions:
>> - arch_kexec_apply_relocations() is not overridden by any architecture
>>   today, so just drop the weak attribute.
>> - arch_kexec_apply_relocations_add() is only overridden by x86 and s390.
>>   Retain the function prototype for those and move the weak
>>   implementation into the header as a static inline for other
>>   architectures.
>> 
>> ...
>>
> 
> Sigh.  This patch demonstrates why I like __weak :<
> 
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -229,6 +225,30 @@ extern int crash_exclude_mem_range(struct crash_mem *mem,
>>  				   unsigned long long mend);
>>  extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
>>  				       void **addr, unsigned long *sz);
>> +
>> +#if defined(CONFIG_X86_64) || defined(CONFIG_S390)
> 
> Let's avoid listing the architectures here?  Better to add
> 
> 	select ARCH_HAVE_ARCH_KEXEC_APPLY_RELOCATIONS_ADD
> 
> to arch/<arch>/Kconfig?

I followed the approach used in commit 6e7b64b9dd6d96 ("elfcore: fix 
building with clang") since here again, it was overridden on only two 
architectures. I also wanted to avoid touching the architecture headers 
so as to make it simpler to backport.

But, as Michael points out, using a #ifdef isn't too much of a change 
either. I also confirmed that those changes still apply cleanly all the 
way back to v5.10. I've posted a v2 which takes this approach.

> 
> Please cc me on any additional work on this.

I've copied you on the v2 patch. Thanks!


- Naveen


  reply	other threads:[~2022-05-19  9:13 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-18 18:18 [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add] Naveen N. Rao
2022-05-18 18:18 ` Naveen N. Rao
2022-05-18 18:18 ` Naveen N. Rao
2022-05-18 20:47 ` Andrew Morton
2022-05-18 20:47   ` Andrew Morton
2022-05-18 20:47   ` Andrew Morton
2022-05-19  9:13   ` Naveen N. Rao [this message]
2022-05-19  9:13     ` Naveen N. Rao
2022-05-19  9:13     ` Naveen N. Rao
2022-05-18 21:59 ` Eric W. Biederman
2022-05-18 21:59   ` Eric W. Biederman
2022-05-18 21:59   ` Eric W. Biederman
2022-05-19  2:58   ` Baoquan He
2022-05-19  2:58     ` Baoquan He
2022-05-19  2:58     ` Baoquan He
2022-05-19  9:28     ` Naveen N. Rao
2022-05-19  9:28       ` Naveen N. Rao
2022-05-19  9:28       ` Naveen N. Rao
2022-05-19 17:59     ` Eric W. Biederman
2022-05-19 17:59       ` Eric W. Biederman
2022-05-19 17:59       ` Eric W. Biederman
2022-05-20 10:46       ` Baoquan He
2022-05-20 10:46         ` Baoquan He
2022-05-20 10:46         ` Baoquan He
2022-05-20 19:25         ` Eric W. Biederman
2022-05-20 19:25           ` Eric W. Biederman
2022-05-20 19:25           ` Eric W. Biederman
2022-05-25 19:56           ` Andrew Morton
2022-05-25 19:56             ` Andrew Morton
2022-05-25 19:56             ` Andrew Morton
2022-05-26 11:00             ` Naveen N. Rao
2022-05-26 11:00               ` Naveen N. Rao
2022-05-26 11:00               ` Naveen N. Rao
2022-05-19  5:41 ` Michael Ellerman
2022-05-19  5:41   ` Michael Ellerman
2022-05-19  5:41   ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1652950421.sq10xtycz9.naveen@linux.ibm.com \
    --to=naveen.n.rao@linux.vnet.ibm.com \
    --cc=kexec@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.