Re: [RFC PATCH 2/2] objtool/powerpc: Enhance objtool to fixup alternate feature relative addresses

[Sathvika Vasireddy <sv@linux.vnet.ibm.com>]

Hi Nathan,

On 4/23/24 5:58 AM, Nathan Chancellor wrote:
> Hi Sathvika,
>
> On Mon, Apr 22, 2024 at 02:52:06PM +0530, Sathvika Vasireddy wrote:
>> Implement build-time fixup of alternate feature relative addresses for
>> the out-of-line (else) patch code. Initial posting to achieve the same
>> using another tool can be found at [1]. Idea is to implement this using
>> objtool instead of introducing another tool since it already has elf
>> parsing and processing covered.
>>
>> Introduce --ftr-fixup as an option to objtool to do feature fixup at
>> build-time.
>>
>> Couple of issues and warnings encountered while implementing feature
>> fixup using objtool are as follows:
>>
>> 1. libelf is creating corrupted vmlinux file after writing necessary
>> changes to the file. Due to this, kexec is not able to load new
>> kernel.
>>
>> It gives the following error:
>>          ELF Note corrupted !
>>          Cannot determine the file type of vmlinux
>>
>> To fix this issue, after opening vmlinux file, make a call to
>> elf_flagelf (e, ELF_C_SET, ELF_F_LAYOUT). This instructs libelf not
>> to touch the segment and section layout. It informs the library
>> that the application will take responsibility for the layout of the
>> file and that the library should not insert any padding between
>> sections.
>>
>> 2. Fix can't find starting instruction warnings when run on vmlinux
>>
>> Objtool throws a lot of can't find starting instruction warnings
>> when run on vmlinux with --ftr-fixup option.
>>
>> These warnings are seen because find_insn() function looks for
>> instructions at offsets that are relative to the start of the section.
>> In case of individual object files (.o), there are no can't find
>> starting instruction warnings seen because the actual offset
>> associated with an instruction is itself a relative offset since the
>> sections start at offset 0x0.
>>
>> However, in case of vmlinux, find_insn() function fails to find
>> instructions at the actual offset associated with an instruction
>> since the sections in vmlinux do not start at offset 0x0. Due to
>> this, find_insn() will look for absolute offset and not the relative
>> offset. This is resulting in a lot of can't find starting instruction
>> warnings when objtool is run on vmlinux.
>>
>> To fix this, pass offset that is relative to the start of the section
>> to find_insn().
>>
>> find_insn() is also looking for symbols of size 0. But, objtool does
>> not store empty STT_NOTYPE symbols in the rbtree. Due to this,
>> for empty symbols, objtool is throwing can't find starting
>> instruction warnings. Fix this by ignoring symbols that are of
>> size 0 since objtool does not add them to the rbtree.
>>
>> 3. Objtool is throwing unannotated intra-function call warnings
>> when run on vmlinux with --ftr-fixup option.
>>
>> One such example:
>>
>> vmlinux: warning: objtool: .text+0x3d94:
>>                          unannotated intra-function call
>>
>> .text + 0x3d94 = c000000000008000 + 3d94 = c0000000000081d4
>>
>> c0000000000081d4: 45 24 02 48  bl c00000000002a618
>> <system_reset_exception+0x8>
>>
>> c00000000002a610 <system_reset_exception>:
>> c00000000002a610:       0e 01 4c 3c     addis   r2,r12,270
>>                          c00000000002a610: R_PPC64_REL16_HA    .TOC.
>> c00000000002a614:       f0 6c 42 38     addi    r2,r2,27888
>>                          c00000000002a614: R_PPC64_REL16_LO    .TOC.+0x4
>> c00000000002a618:       a6 02 08 7c     mflr    r0
>>
>> This is happening because we should be looking for destination
>> symbols that are at absolute offsets instead of relative offsets.
>> After fixing dest_off to point to absolute offset, there are still
>> a lot of these warnings shown.
>>
>> In the above example, objtool is computing the destination
>> offset to be c00000000002a618, which points to a completely
>> different instruction. find_call_destination() is looking for this
>> offset and failing. Instead, we should be looking for destination
>> offset c00000000002a610 which points to system_reset_exception
>> function.
>>
>> Even after fixing the way destination offset is computed, and
>> after looking for dest_off - 0x8 in cases where the original offset
>> is not found, there are still a lot of unannotated intra-function
>> call warnings generated. This is due to symbols that are not
>> properly annotated.
>>
>> So, for now, as a hack to curb these warnings, do not emit
>> unannotated intra-function call warnings when objtool is run
>> with --ftr-fixup option.
>>
>> TODO:
>> This patch enables build time feature fixup only for powerpc little
>> endian configs. There are boot failures with big endian configs.
>> Posting this as an initial RFC to get some review comments while I work
>> on big endian issues.
>>
>> [1]
>> https://lore.kernel.org/linuxppc-dev/20170521010130.13552-1-npiggin@gmail.com/
>>
>> Co-developed-by: Nicholas Piggin <npiggin@gmail.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
> When I build this series with LLVM 14 [1] (due to an issue I report
> below), I am getting a crash when CONFIG_FTR_FIXUP_SELFTEST is disabled.
>
> diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig
> index 544a65fda77b..95d2906ec814 100644
> --- a/arch/powerpc/configs/ppc64_defconfig
> +++ b/arch/powerpc/configs/ppc64_defconfig
> @@ -427,7 +427,6 @@ CONFIG_BLK_DEV_IO_TRACE=y
>   CONFIG_IO_STRICT_DEVMEM=y
>   CONFIG_PPC_EMULATED_STATS=y
>   CONFIG_CODE_PATCHING_SELFTEST=y
> -CONFIG_FTR_FIXUP_SELFTEST=y
>   CONFIG_MSI_BITMAP_SELFTEST=y
>   CONFIG_XMON=y
>   CONFIG_BOOTX_TEXT=y
>
>    $ make -kj"$(nproc)" ARCH=powerpc LLVM=$PWD/llvm-14.0.6-x86_64/bin/ ppc64le_defconfig vmlinux
>    ...
>      LD      vmlinux
>      NM      System.map
>      SORTTAB vmlinux
>      CHKHEAD vmlinux
>      CHKREL  vmlinux
>    make[4]: *** [scripts/Makefile.vmlinux:38: vmlinux] Error 139
>    make[4]: *** Deleting file 'vmlinux
>    ...
>
> I do not see the objtool command in V=1 output but I do see the end of
> scripts/link-vmlinux.sh, so it does seem like it is crashing in objtool.
>
> [1]: from https://mirrors.edge.kernel.org/pub/tools/llvm/

Thanks for reporting this, and apologies for the delay in response.

This issue is happening while processing __fw_ftr_fixup section, which 
has no data and dereferencing this null pointer is causing segmentation 
fault. I was able to recreate the issue, and the below diff fixes it for me.

diff --git a/tools/objtool/arch/powerpc/special.c 
b/tools/objtool/arch/powerpc/special.c
index 5ec3eed34eb0..67329d44db24 100644
--- a/tools/objtool/arch/powerpc/special.c
+++ b/tools/objtool/arch/powerpc/special.c
@@ -53,38 +53,39 @@ int process_fixup_entries(struct objtool_file *file)
                 if (strstr(sec->name, "_ftr_fixup") != NULL) {
                         Elf_Data *data = sec->data;

-                       if (data && data->d_size > 0)
+                       if (data && data->d_size > 0) {
                                 nr = data->d_size / sizeof(struct 
fixup_entry);

-                       for (i = 0; i < nr; i++) {
-                               struct fixup_entry *dst;
-                               unsigned long idx;
-                               unsigned long long off;
-                               struct fixup_entry *src;
+                               for (i = 0; i < nr; i++) {
+                                       struct fixup_entry *dst;
+                                       unsigned long idx;
+                                       unsigned long long off;
+                                       struct fixup_entry *src;

-                               idx = i * sizeof(struct fixup_entry);
-                               off = sec->sh.sh_addr + data->d_off + idx;
-                               src = data->d_buf + idx;
+                                       idx = i * sizeof(struct 
fixup_entry);
+                                       off = sec->sh.sh_addr + 
data->d_off + idx;
+                                       src = data->d_buf + idx;

-                               if (src->alt_start_off == src->alt_end_off)
-                                       continue;
+                                       if (src->alt_start_off == 
src->alt_end_off)
+                                               continue;

-                               fes = realloc(fes, (nr_fes + 1) * 
sizeof(struct fixup_entry));
-                               dst = &fes[nr_fes];
-                               nr_fes++;
+                                       fes = realloc(fes, (nr_fes + 1) 
* sizeof(struct fixup_entry));
+                                       dst = &fes[nr_fes];
+                                       nr_fes++;

-                               dst->mask = __le64_to_cpu(src->mask);
-                               dst->value = __le64_to_cpu(src->value);
-                               dst->start_off = 
__le64_to_cpu(src->start_off) + off;
-                               dst->end_off = 
__le64_to_cpu(src->end_off) + off;
-                               dst->alt_start_off = 
__le64_to_cpu(src->alt_start_off) + off;
-                               dst->alt_end_off = 
__le64_to_cpu(src->alt_end_off) + off;
+                                       dst->mask = 
__le64_to_cpu(src->mask);
+                                       dst->value = 
__le64_to_cpu(src->value);
+                                       dst->start_off = 
__le64_to_cpu(src->start_off) + off;
+                                       dst->end_off = 
__le64_to_cpu(src->end_off) + off;
+                                       dst->alt_start_off = 
__le64_to_cpu(src->alt_start_off) + off;
+                                       dst->alt_end_off = 
__le64_to_cpu(src->alt_end_off) + off;

-                               if (dst->alt_start_off < fe_alt_start)
-                                       fe_alt_start = dst->alt_start_off;
+                                       if (dst->alt_start_off < 
fe_alt_start)
+                                               fe_alt_start = 
dst->alt_start_off;

-                               if (dst->alt_end_off > fe_alt_end)
-                                       fe_alt_end = dst->alt_end_off;
+                                       if (dst->alt_end_off > fe_alt_end)
+                                               fe_alt_end = 
dst->alt_end_off;
+                               }
                         }
                 }
         }
@@ -246,7 +247,6 @@ static struct symbol *find_symbol_at_address(struct 
objtool_file *file,
  int process_alt_relocations(struct objtool_file *file)
  {
         struct section *section;
-       size_t n = 0;
         uint32_t insn;
         uint32_t *i;
         unsigned int opcode;
@@ -276,8 +276,6 @@ int process_alt_relocations(struct objtool_file *file)
                                 target = target + 0x8;
                 }

-               n++;
-
                 fe = find_fe_altaddr(addr);
                 if (fe) {

I'll include these changes in the next version.

Thanks,
Sathvika