Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
Le 05/09/2022 à 22:43, Segher Boessenkool a écrit : > Hi! > > On Mon, Sep 05, 2022 at 04:15:07PM +0530, Naveen N. Rao wrote: >> Segher Boessenkool wrote: > + if ((insn & 3) == 1) { > + *type = INSN_CALL; > + *immediate = insn & 0x3fc; > + if (*immediate & 0x200) > + *immediate -= 0x400; > + } > + break; > + } >>> >>> Does this handle AA=1 correctly at all? That is valid both with and >>> without relocations, just like AA=0. Same for AA=1 LK=0 btw. >>> >>> If you only handle AA=0, the code should explicitly test for that. >> >> The code does test for AA=0 LK=1 with the if statement there? > > Yes, but that is not what I said :-) > > It may be fine to not *handle* AA=1 at all, but the code should at least > scream bloody murder when it encounters it anyway :-) > By the way, I proposed a cleanup patch that handles it, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/ebe11b73d1015a17034a2c4bedf093fa57f5d29f.1662032631.git.christophe.le...@csgroup.eu/ Christophe
Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
Hi! On Mon, Sep 05, 2022 at 04:15:07PM +0530, Naveen N. Rao wrote: > Segher Boessenkool wrote: > >>> + if ((insn & 3) == 1) { > >>> + *type = INSN_CALL; > >>> + *immediate = insn & 0x3fc; > >>> + if (*immediate & 0x200) > >>> + *immediate -= 0x400; > >>> + } > >>> + break; > >>> + } > > > >Does this handle AA=1 correctly at all? That is valid both with and > >without relocations, just like AA=0. Same for AA=1 LK=0 btw. > > > >If you only handle AA=0, the code should explicitly test for that. > > The code does test for AA=0 LK=1 with the if statement there? Yes, but that is not what I said :-) It may be fine to not *handle* AA=1 at all, but the code should at least scream bloody murder when it encounters it anyway :-) Segher
Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
Segher Boessenkool wrote: On Wed, Aug 31, 2022 at 12:50:07PM +, Christophe Leroy wrote: Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : > + opcode = insn >> 26; > + > + switch (opcode) { > + case 18: /* bl */ case 18 is more than 'bl', it includes also 'b'. In both cases, the calculation of *immediate is the same. It also is "ba" and "bla", sometimes written as "b[l][a]". It would therefore be more correct to perform the calculation and setup of *immediate outside the 'if ((insn & 3) == 1)', that would avoid unnecessary churn the day we add support for branches without link. Yeah, and probably move the comments around: + case 18: /* b[l][a] */ + if ((insn & 3) == 1) { /* bl */ > + if ((insn & 3) == 1) { > + *type = INSN_CALL; > + *immediate = insn & 0x3fc; > + if (*immediate & 0x200) > + *immediate -= 0x400; > + } > + break; > + } Does this handle AA=1 correctly at all? That is valid both with and without relocations, just like AA=0. Same for AA=1 LK=0 btw. If you only handle AA=0, the code should explicitly test for that. The code does test for AA=0 LK=1 with the if statement there? - Naveen
Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
On Wed, Aug 31, 2022 at 12:50:07PM +, Christophe Leroy wrote: > Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : > > + opcode = insn >> 26; > > + > > + switch (opcode) { > > + case 18: /* bl */ > > case 18 is more than 'bl', it includes also 'b'. > In both cases, the calculation of *immediate is the same. It also is "ba" and "bla", sometimes written as "b[l][a]". > It would therefore be more correct to perform the calculation and setup > of *immediate outside the 'if ((insn & 3) == 1)', that would avoid > unnecessary churn the day we add support for branches without link. > > > + if ((insn & 3) == 1) { > > + *type = INSN_CALL; > > + *immediate = insn & 0x3fc; > > + if (*immediate & 0x200) > > + *immediate -= 0x400; > > + } > > + break; > > + } Does this handle AA=1 correctly at all? That is valid both with and without relocations, just like AA=0. Same for AA=1 LK=0 btw. If you only handle AA=0, the code should explicitly test for that. Segher
Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : > This patch enables objtool --mcount on powerpc, and > adds implementation specific to powerpc. > > Signed-off-by: Sathvika Vasireddy > --- > arch/powerpc/Kconfig | 1 + > tools/objtool/arch/powerpc/decode.c | 22 +++ > tools/objtool/arch/powerpc/include/arch/elf.h | 2 ++ > 3 files changed, 25 insertions(+) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index dc05cd23c233..6be2e68fa9eb 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -238,6 +238,7 @@ config PPC > select HAVE_NMI if PERF_EVENTS || (PPC64 && > PPC_BOOK3S) > select HAVE_OPTPROBES > select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL > + 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 8b6a14680da7..b71c265ed503 100644 > --- a/tools/objtool/arch/powerpc/decode.c > +++ b/tools/objtool/arch/powerpc/decode.c > @@ -9,6 +9,14 @@ > #include > #include > > +bool arch_ftrace_match(char *name) > +{ > + if (!strcmp(name, "_mcount")) > + return true; > + > + return false; > +} > + > unsigned long arch_dest_reloc_offset(int addend) > { > return addend; > @@ -41,12 +49,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; > insn = bswap_if_needed(file->elf, *(u32 *)(sec->data->d_buf + offset)); > *len = 4; > *type = INSN_OTHER; > > + opcode = insn >> 26; > + > + switch (opcode) { > + case 18: /* bl */ case 18 is more than 'bl', it includes also 'b'. In both cases, the calculation of *immediate is the same. It would therefore be more correct to perform the calculation and setup of *immediate outside the 'if ((insn & 3) == 1)', that would avoid unnecessary churn the day we add support for branches without link. > + if ((insn & 3) == 1) { > + *type = INSN_CALL; > + *immediate = insn & 0x3fc; > + if (*immediate & 0x200) > + *immediate -= 0x400; > + } > + break; > + } > + > return 0; > } > > diff --git a/tools/objtool/arch/powerpc/include/arch/elf.h > b/tools/objtool/arch/powerpc/include/arch/elf.h > index 3c8ebb7d2a6b..73f9ae172fe5 100644 > --- a/tools/objtool/arch/powerpc/include/arch/elf.h > +++ b/tools/objtool/arch/powerpc/include/arch/elf.h > @@ -4,5 +4,7 @@ > #define _OBJTOOL_ARCH_ELF > > #define R_NONE R_PPC_NONE > +#define R_ABS64 R_PPC64_ADDR64 > +#define R_ABS32 R_PPC_ADDR32 > > #endif /* _OBJTOOL_ARCH_ELF */
Re: [PATCH v2 16/16] objtool/powerpc: Add --mcount specific implementation
Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : > This patch enables objtool --mcount on powerpc, and > adds implementation specific to powerpc. > > Signed-off-by: Sathvika Vasireddy > --- > arch/powerpc/Kconfig | 1 + > tools/objtool/arch/powerpc/decode.c | 22 +++ > tools/objtool/arch/powerpc/include/arch/elf.h | 2 ++ > 3 files changed, 25 insertions(+) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index dc05cd23c233..6be2e68fa9eb 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -238,6 +238,7 @@ config PPC > select HAVE_NMI if PERF_EVENTS || (PPC64 && > PPC_BOOK3S) > select HAVE_OPTPROBES > select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL > + 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 8b6a14680da7..b71c265ed503 100644 > --- a/tools/objtool/arch/powerpc/decode.c > +++ b/tools/objtool/arch/powerpc/decode.c > @@ -9,6 +9,14 @@ > #include > #include > > +bool arch_ftrace_match(char *name) > +{ > + if (!strcmp(name, "_mcount")) > + return true; > + > + return false; Same as for x86, could be: return !strcmp(name, "_mcount"); Reviewed-by: Christophe Leroy > +} > + > unsigned long arch_dest_reloc_offset(int addend) > { > return addend; > @@ -41,12 +49,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; > insn = bswap_if_needed(file->elf, *(u32 *)(sec->data->d_buf + offset)); > *len = 4; > *type = INSN_OTHER; > > + opcode = insn >> 26; > + > + switch (opcode) { > + case 18: /* bl */ > + if ((insn & 3) == 1) { > + *type = INSN_CALL; > + *immediate = insn & 0x3fc; > + if (*immediate & 0x200) > + *immediate -= 0x400; > + } > + break; > + } > + > return 0; > } > > diff --git a/tools/objtool/arch/powerpc/include/arch/elf.h > b/tools/objtool/arch/powerpc/include/arch/elf.h > index 3c8ebb7d2a6b..73f9ae172fe5 100644 > --- a/tools/objtool/arch/powerpc/include/arch/elf.h > +++ b/tools/objtool/arch/powerpc/include/arch/elf.h > @@ -4,5 +4,7 @@ > #define _OBJTOOL_ARCH_ELF > > #define R_NONE R_PPC_NONE > +#define R_ABS64 R_PPC64_ADDR64 > +#define R_ABS32 R_PPC_ADDR32 > > #endif /* _OBJTOOL_ARCH_ELF */