| Message ID | 20220829055223.24767-17-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
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 <sv@linux.ibm.com> > --- > 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 <objtool/builtin.h> > #include <objtool/endianness.h> > > +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 <christophe.leroy@csgroup.eu> > +} > + > 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 & 0x3fffffc; > + if (*immediate & 0x2000000) > + *immediate -= 0x4000000; > + } > + 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 */
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 <sv@linux.ibm.com> > --- > 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 <objtool/builtin.h> > #include <objtool/endianness.h> > > +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 & 0x3fffffc; > + if (*immediate & 0x2000000) > + *immediate -= 0x4000000; > + } > + 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 */
On Wed, Aug 31, 2022 at 12:50:07PM +0000, 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 & 0x3fffffc; > > + if (*immediate & 0x2000000) > > + *immediate -= 0x4000000; > > + } > > + 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
Segher Boessenkool wrote: > On Wed, Aug 31, 2022 at 12:50:07PM +0000, 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 & 0x3fffffc; >> > + if (*immediate & 0x2000000) >> > + *immediate -= 0x4000000; >> > + } >> > + 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
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 & 0x3fffffc; > >>> + if (*immediate & 0x2000000) > >>> + *immediate -= 0x4000000; > >>> + } > >>> + 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
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 & 0x3fffffc; >>>>> + if (*immediate & 0x2000000) >>>>> + *immediate -= 0x4000000; >>>>> + } >>>>> + 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.leroy@csgroup.eu/ Christophe
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 <objtool/builtin.h> #include <objtool/endianness.h> +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 */ + if ((insn & 3) == 1) { + *type = INSN_CALL; + *immediate = insn & 0x3fffffc; + if (*immediate & 0x2000000) + *immediate -= 0x4000000; + } + 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 */
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 | 22 +++++++++++++++++++ tools/objtool/arch/powerpc/include/arch/elf.h | 2 ++ 3 files changed, 25 insertions(+)