Re: [RFC PATCH 4/4] objtool/powerpc: Add --mcount specific implementation

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
> [Vous ne recevez pas souvent de courriers de la part de 
> sv@linux.vnet.ibm.com. Découvrez pourquoi cela peut être important à 
> l’adresse https://aka.ms/LearnAboutSenderIdentification.]
> 
> On 24/05/22 15:05, Christophe Leroy wrote:
>>
>> Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit :
>>> This patch enables objtool --mcount on powerpc, and
>>> adds implementation specific to powerpc.
>>>
>>> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
>>> ---
>>>    arch/powerpc/Kconfig                |  1 +
>>>    tools/objtool/arch/powerpc/decode.c | 14 ++++++++++++++
>>>    tools/objtool/check.c               | 12 +++++++-----
>>>    tools/objtool/elf.c                 | 13 +++++++++++++
>>>    tools/objtool/include/objtool/elf.h |  1 +
>>>    5 files changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 732a3f91ee5e..3373d44a1298 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -233,6 +233,7 @@ config PPC
>>>      select HAVE_NMI                         if PERF_EVENTS || (PPC64 
>>> && PPC_BOOK3S)
>>>      select HAVE_OPTPROBES
>>>      select HAVE_OBJTOOL                     if PPC64
>>> +    select HAVE_OBJTOOL_MCOUNT              if HAVE_OBJTOOL
>>>      select HAVE_PERF_EVENTS
>>>      select HAVE_PERF_EVENTS_NMI             if PPC64
>>>      select HAVE_PERF_REGS
>>> diff --git a/tools/objtool/arch/powerpc/decode.c 
>>> b/tools/objtool/arch/powerpc/decode.c
>>> index e3b77a6ce357..ad3d79fffac2 100644
>>> --- a/tools/objtool/arch/powerpc/decode.c
>>> +++ b/tools/objtool/arch/powerpc/decode.c
>>> @@ -40,12 +40,26 @@ int arch_decode_instruction(struct objtool_file 
>>> *file, const struct section *sec
>>>                          struct list_head *ops_list)
>>>    {
>>>      u32 insn;
>>> +    unsigned int opcode;
>>>
>>>      *immediate = 0;
>>>      memcpy(&insn, sec->data->d_buf+offset, 4);
>>>      *len = 4;
>>>      *type = INSN_OTHER;
>>>
>>> +    opcode = (insn >> 26);
>> You dont need the brackets here.
>>
>>> +
>>> +    switch (opcode) {
>>> +    case 18: /* bl */
>>> +            if ((insn & 3) == 1) {
>>> +                    *type = INSN_CALL;
>>> +                    *immediate = insn & 0x3fffffc;
>>> +                    if (*immediate & 0x2000000)
>>> +                            *immediate -= 0x4000000;
>>> +            }
>>> +            break;
>>> +    }
>>> +
>>>      return 0;
>>>    }
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 056302d58e23..fd8bad092f89 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -832,7 +832,7 @@ static int create_mcount_loc_sections(struct 
>>> objtool_file *file)
>>>
>>>              if (elf_add_reloc_to_insn(file->elf, sec,
>>>                                        idx * sizeof(unsigned long),
>>> -                                      R_X86_64_64,
>>> +                                      elf_reloc_type_long(file->elf),
>>>                                        insn->sec, insn->offset))
>>>                      return -1;
>>>
>>> @@ -2183,7 +2183,7 @@ static int classify_symbols(struct objtool_file 
>>> *file)
>>>                      if (arch_is_retpoline(func))
>>>                              func->retpoline_thunk = true;
>>>
>>> -                    if (!strcmp(func->name, "__fentry__"))
>>> +                    if ((!strcmp(func->name, "__fentry__")) || 
>>> (!strcmp(func->name, "_mcount")))
>>>                              func->fentry = true;
>>>
>>>                      if (is_profiling_func(func->name))
>>> @@ -2259,9 +2259,11 @@ static int decode_sections(struct objtool_file 
>>> *file)
>>>       * Must be before add_jump_destinations(), which depends on 'func'
>>>       * being set for alternatives, to enable proper sibling call 
>>> detection.
>>>       */
>>> -    ret = add_special_section_alts(file);
>>> -    if (ret)
>>> -            return ret;
>>> +    if (opts.stackval || opts.orc || opts.uaccess || opts.noinstr) {
>>> +            ret = add_special_section_alts(file);
>>> +            if (ret)
>>> +                    return ret;
>>> +    }
>> I think this change should be a patch by itself, it's not related to
>> powerpc.
> Makes sense. I'll make this a separate patch in the next revision.

Great. Can you base your next revision on the one I just sent out ?

I will now start looking at inline static calls for PPC32.

>>
>>>
>>>      ret = add_jump_destinations(file);
>>>      if (ret)
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index c25e957c1e52..95763060d551 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -793,6 +793,19 @@ elf_create_section_symbol(struct elf *elf, 
>>> struct section *sec)
>>>      return sym;
>>>    }
>>>
>>> +int elf_reloc_type_long(struct elf *elf)
>> Not sure it's a good name, because for 32 bits we have to use 'int'.
> Sure, I'll rename it to elf_reloc_type() or some such.
>>
>>> +{
>>> +    switch (elf->ehdr.e_machine) {
>>> +    case EM_X86_64:
>>> +            return R_X86_64_64;
>>> +    case EM_PPC64:
>>> +            return R_PPC64_ADDR64;
>>> +    default:
>>> +            WARN("unknown machine...");
>>> +            exit(-1);
>>> +    }
>>> +}
>> Wouldn't it be better to make that function arch specific ?
> 
> This is so that we can support cross architecture builds.
> 


I'm not sure I follow you here.

This is only based on the target, it doesn't depend on the build host so 
I can't the link with cross arch builds.

The same as you have arch_decode_instruction(), you could have 
arch_elf_reloc_type_long()
It would make sense indeed, because there is no point in supporting X86 
relocation when you don't support X86 instruction decoding.

Christophe