| Message ID | 1393840403-26639-2-git-send-email-jean.pihet@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
A new version is coming asap after a review of the code with Steve. Thx, Jean On 3 March 2014 10:53, Jean Pihet <jean.pihet@linaro.org> wrote: > Introducing perf_regs_load function, which is going > to be used for dwarf unwind test in following patches. > > It takes single argument as a pointer to the regs dump > buffer and populates it with current registers values. > > Signed-off-by: Jean Pihet <jean.pihet@linaro.org> > Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com> > Cc: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net> > Cc: David Ahern <dsahern@gmail.com> > Cc: Jiri Olsa <jolsa@redhat.com> > --- > tools/perf/arch/arm/Makefile | 1 + > tools/perf/arch/arm/include/perf_regs.h | 2 ++ > tools/perf/arch/arm/tests/regs_load.S | 51 +++++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+) > create mode 100644 tools/perf/arch/arm/tests/regs_load.S > > diff --git a/tools/perf/arch/arm/Makefile b/tools/perf/arch/arm/Makefile > index 67e9b3d..9b8f87e 100644 > --- a/tools/perf/arch/arm/Makefile > +++ b/tools/perf/arch/arm/Makefile > @@ -4,4 +4,5 @@ LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o > endif > ifndef NO_LIBUNWIND > LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/unwind-libunwind.o > +LIB_OBJS += $(OUTPUT)arch/$(ARCH)/tests/regs_load.o > endif > diff --git a/tools/perf/arch/arm/include/perf_regs.h b/tools/perf/arch/arm/include/perf_regs.h > index 2a1cfde..1476ae7 100644 > --- a/tools/perf/arch/arm/include/perf_regs.h > +++ b/tools/perf/arch/arm/include/perf_regs.h > @@ -5,6 +5,8 @@ > #include "../../util/types.h" > #include <asm/perf_regs.h> > > +void perf_regs_load(u64 *regs); > + > #define PERF_REGS_MASK ((1ULL << PERF_REG_ARM_MAX) - 1) > #define PERF_REG_IP PERF_REG_ARM_PC > #define PERF_REG_SP PERF_REG_ARM_SP > diff --git a/tools/perf/arch/arm/tests/regs_load.S b/tools/perf/arch/arm/tests/regs_load.S > new file mode 100644 > index 0000000..241c6df > --- /dev/null > +++ b/tools/perf/arch/arm/tests/regs_load.S > @@ -0,0 +1,51 @@ > +#include <linux/linkage.h> > + > +#define R0 0x00 > +#define R1 0x08 > +#define R2 0x10 > +#define R3 0x18 > +#define R4 0x20 > +#define R5 0x28 > +#define R6 0x30 > +#define R7 0x38 > +#define R8 0x40 > +#define R9 0x48 > +#define SL 0x50 > +#define FP 0x58 > +#define IP 0x60 > +#define SP 0x68 > +#define LR 0x70 > +#define PC 0x78 > + > +@ Implementation of void perf_regs_load(u64 *regs); > +@ > +@ This functions fills in the 'regs' buffer from the actual registers values. > +@ Note that the return values (i.e. caller values) of sp and lr > +@ are retrieved and stored, in order to skip the call to this function. > + > +.text > +.type perf_regs_load,%function > +ENTRY(perf_regs_load) > + push {r1} > + > + str r0, [r0, #R0] > + str r1, [r0, #R1] > + str r2, [r0, #R2] > + str r3, [r0, #R3] > + str r4, [r0, #R4] > + str r5, [r0, #R5] > + str r6, [r0, #R6] > + str r7, [r0, #R7] > + str r8, [r0, #R8] > + str r9, [r0, #R9] > + str sl, [r0, #SL] > + str fp, [r0, #FP] > + str ip, [r0, #IP] > + add r1, sp, #4 @ Retrieve and save sp at entry time > + str r1, [r0, #SP] > + str lr, [r0, #LR] > + str lr, [r0, #PC] @ Save caller PC > + > + pop {r1} > + bx lr > +ENDPROC(perf_regs_load) > -- > 1.7.11.7 >
Hi Jean, On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: > Introducing perf_regs_load function, which is going > to be used for dwarf unwind test in following patches. > > It takes single argument as a pointer to the regs dump > buffer and populates it with current registers values. [...] > diff --git a/tools/perf/arch/arm/tests/regs_load.S b/tools/perf/arch/arm/tests/regs_load.S > new file mode 100644 > index 0000000..241c6df > --- /dev/null > +++ b/tools/perf/arch/arm/tests/regs_load.S > @@ -0,0 +1,51 @@ > +#include <linux/linkage.h> > + > +#define R0 0x00 > +#define R1 0x08 Why are you using a 64-bit stride for 32-bit registers? (which prevents you from using stm later on). > +.text > +.type perf_regs_load,%function > +ENTRY(perf_regs_load) > + push {r1} Do you only push r1 here so that you can do the stack arithmetic later? That doesn't make sense to me -- can't you str sp directly? > + str r0, [r0, #R0] > + str r1, [r0, #R1] > + str r2, [r0, #R2] > + str r3, [r0, #R3] > + str r4, [r0, #R4] > + str r5, [r0, #R5] > + str r6, [r0, #R6] > + str r7, [r0, #R7] > + str r8, [r0, #R8] > + str r9, [r0, #R9] > + str sl, [r0, #SL] > + str fp, [r0, #FP] > + str ip, [r0, #IP] > + add r1, sp, #4 @ Retrieve and save sp at entry time > + str r1, [r0, #SP] > + str lr, [r0, #LR] > + str lr, [r0, #PC] @ Save caller PC This isn't necessarily the `caller PC' (depending on how you define it). It's the return address, which is probably (but not always) the instruction following the branch to this function. Will
Hi Will, On 4 March 2014 12:00, Will Deacon <will.deacon@arm.com> wrote: > Hi Jean, > > On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: >> Introducing perf_regs_load function, which is going >> to be used for dwarf unwind test in following patches. >> >> It takes single argument as a pointer to the regs dump >> buffer and populates it with current registers values. > > [...] > >> diff --git a/tools/perf/arch/arm/tests/regs_load.S b/tools/perf/arch/arm/tests/regs_load.S >> new file mode 100644 >> index 0000000..241c6df >> --- /dev/null >> +++ b/tools/perf/arch/arm/tests/regs_load.S >> @@ -0,0 +1,51 @@ >> +#include <linux/linkage.h> >> + >> +#define R0 0x00 >> +#define R1 0x08 > > Why are you using a 64-bit stride for 32-bit registers? (which prevents you > from using stm later on). This is because perf internally uses a 64-bit array for the registers, cf. the function prototype in the comment above. > >> +.text >> +.type perf_regs_load,%function >> +ENTRY(perf_regs_load) >> + push {r1} > > Do you only push r1 here so that you can do the stack arithmetic later? That > doesn't make sense to me -- can't you str sp directly? Yes indeed! I submitted a new version yesterday with that change and a fix in the comments. > >> + str r0, [r0, #R0] >> + str r1, [r0, #R1] >> + str r2, [r0, #R2] >> + str r3, [r0, #R3] >> + str r4, [r0, #R4] >> + str r5, [r0, #R5] >> + str r6, [r0, #R6] >> + str r7, [r0, #R7] >> + str r8, [r0, #R8] >> + str r9, [r0, #R9] >> + str sl, [r0, #SL] >> + str fp, [r0, #FP] >> + str ip, [r0, #IP] >> + add r1, sp, #4 @ Retrieve and save sp at entry time >> + str r1, [r0, #SP] >> + str lr, [r0, #LR] >> + str lr, [r0, #PC] @ Save caller PC > > This isn't necessarily the `caller PC' (depending on how you define it). > It's the return address, which is probably (but not always) the instruction > following the branch to this function. Agreed. However the perf test code expects a registers buffer filled in with the caller's values. I can change the comment here, is that needed? Thanks for reviewing! Jean > > Will > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Wed, Mar 05, 2014 at 02:17:00AM +0000, Jean Pihet wrote: > On 4 March 2014 12:00, Will Deacon <will.deacon@arm.com> wrote: > > On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: > >> + str lr, [r0, #PC] @ Save caller PC > > > > This isn't necessarily the `caller PC' (depending on how you define it). > > It's the return address, which is probably (but not always) the instruction > > following the branch to this function. > Agreed. However the perf test code expects a registers buffer filled > in with the caller's values. > I can change the comment here, is that needed? It depends what the perf test code really expects. At the moment, you're not providing it with anything consistent which doesn't sound correct. What does it use the caller PC for? Will
On Thu, Mar 06, 2014 at 11:33:15AM +0000, Will Deacon wrote: > On Wed, Mar 05, 2014 at 02:17:00AM +0000, Jean Pihet wrote: > > On 4 March 2014 12:00, Will Deacon <will.deacon@arm.com> wrote: > > > On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: > > >> + str lr, [r0, #PC] @ Save caller PC > > > > > > This isn't necessarily the `caller PC' (depending on how you define it). > > > It's the return address, which is probably (but not always) the instruction > > > following the branch to this function. > > Agreed. However the perf test code expects a registers buffer filled > > in with the caller's values. > > I can change the comment here, is that needed? > > It depends what the perf test code really expects. At the moment, you're not > providing it with anything consistent which doesn't sound correct. the code expects caller's PC. That is what the x86 test code expects from perf_regs_load. We take the return IP saved by call instruction: ENTRY(perf_regs_load) ... movq 0(%rsp), %rax movq %rax, IP(%rdi) ... jirka
HI Will, Steve, On 6 March 2014 18:22, Jiri Olsa <jolsa@redhat.com> wrote: > On Thu, Mar 06, 2014 at 11:33:15AM +0000, Will Deacon wrote: >> On Wed, Mar 05, 2014 at 02:17:00AM +0000, Jean Pihet wrote: >> > On 4 March 2014 12:00, Will Deacon <will.deacon@arm.com> wrote: >> > > On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: >> > >> + str lr, [r0, #PC] @ Save caller PC >> > > >> > > This isn't necessarily the `caller PC' (depending on how you define it). >> > > It's the return address, which is probably (but not always) the instruction >> > > following the branch to this function. >> > Agreed. However the perf test code expects a registers buffer filled >> > in with the caller's values. >> > I can change the comment here, is that needed? >> >> It depends what the perf test code really expects. At the moment, you're not >> providing it with anything consistent which doesn't sound correct. > > the code expects caller's PC. That is what the x86 test code > expects from perf_regs_load. We take the return IP saved by > call instruction: > > ENTRY(perf_regs_load) > ... > movq 0(%rsp), %rax > movq %rax, IP(%rdi) > ... > > jirka The perf built-in test code expects the caller PC to do the unwinding from. Does that sound correct to you? Do you want me to add a note like this one (as done in the upcoming aarch64 patches, to be submitted asap after testing): " /* * Implementation of void perf_regs_load(u64 *regs); * * This functions fills in the 'regs' buffer from the actual registers values. * * Notes: * 1. the return value of the pc is retrieved from lr and stored, in order * to skip the call to this function, * 2. the current value of lr is merely retrieved and stored because the * value before the call to this function is unknown at this time; it will * be unwound from the dwarf information in unwind__get_entries. */ " Please let me know how to make this better. Regards, Jean
On Tue, Mar 11, 2014 at 06:25:11PM +0000, Jean Pihet wrote: > HI Will, Steve, Hi Jean, > On 6 March 2014 18:22, Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Mar 06, 2014 at 11:33:15AM +0000, Will Deacon wrote: > >> On Wed, Mar 05, 2014 at 02:17:00AM +0000, Jean Pihet wrote: > >> > On 4 March 2014 12:00, Will Deacon <will.deacon@arm.com> wrote: > >> > > On Mon, Mar 03, 2014 at 09:53:21AM +0000, Jean Pihet wrote: > >> > >> + str lr, [r0, #PC] @ Save caller PC > >> > > > >> > > This isn't necessarily the `caller PC' (depending on how you define it). > >> > > It's the return address, which is probably (but not always) the instruction > >> > > following the branch to this function. > >> > Agreed. However the perf test code expects a registers buffer filled > >> > in with the caller's values. > >> > I can change the comment here, is that needed? > >> > >> It depends what the perf test code really expects. At the moment, you're not > >> providing it with anything consistent which doesn't sound correct. > > > > the code expects caller's PC. That is what the x86 test code > > expects from perf_regs_load. We take the return IP saved by > > call instruction: > > > > ENTRY(perf_regs_load) > > ... > > movq 0(%rsp), %rax > > movq %rax, IP(%rdi) > > ... > > > > jirka > > The perf built-in test code expects the caller PC to do the unwinding > from. Does that sound correct to you? Yes, but we're not necessarily providing that in your code. We're providing the return address, which could be different (e.g., if this was a tail call). > Do you want me to add a note like this one (as done in the upcoming > aarch64 patches, to be submitted asap after testing): > " > /* > * Implementation of void perf_regs_load(u64 *regs); > * > * This functions fills in the 'regs' buffer from the actual registers values. > * > * Notes: > * 1. the return value of the pc is retrieved from lr and stored, in order > * to skip the call to this function, > * 2. the current value of lr is merely retrieved and stored because the > * value before the call to this function is unknown at this time; it will > * be unwound from the dwarf information in unwind__get_entries. > */ > " I think the main thing is that your code may well work for this specific test case (and perhaps that's good enough) but we should be clear about the assumptions you're making. Will
diff --git a/tools/perf/arch/arm/Makefile b/tools/perf/arch/arm/Makefile index 67e9b3d..9b8f87e 100644 --- a/tools/perf/arch/arm/Makefile +++ b/tools/perf/arch/arm/Makefile @@ -4,4 +4,5 @@ LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o endif ifndef NO_LIBUNWIND LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/unwind-libunwind.o +LIB_OBJS += $(OUTPUT)arch/$(ARCH)/tests/regs_load.o endif diff --git a/tools/perf/arch/arm/include/perf_regs.h b/tools/perf/arch/arm/include/perf_regs.h index 2a1cfde..1476ae7 100644 --- a/tools/perf/arch/arm/include/perf_regs.h +++ b/tools/perf/arch/arm/include/perf_regs.h @@ -5,6 +5,8 @@ #include "../../util/types.h" #include <asm/perf_regs.h> +void perf_regs_load(u64 *regs); + #define PERF_REGS_MASK ((1ULL << PERF_REG_ARM_MAX) - 1) #define PERF_REG_IP PERF_REG_ARM_PC #define PERF_REG_SP PERF_REG_ARM_SP diff --git a/tools/perf/arch/arm/tests/regs_load.S b/tools/perf/arch/arm/tests/regs_load.S new file mode 100644 index 0000000..241c6df --- /dev/null +++ b/tools/perf/arch/arm/tests/regs_load.S @@ -0,0 +1,51 @@ +#include <linux/linkage.h> + +#define R0 0x00 +#define R1 0x08 +#define R2 0x10 +#define R3 0x18 +#define R4 0x20 +#define R5 0x28 +#define R6 0x30 +#define R7 0x38 +#define R8 0x40 +#define R9 0x48 +#define SL 0x50 +#define FP 0x58 +#define IP 0x60 +#define SP 0x68 +#define LR 0x70 +#define PC 0x78 + +@ Implementation of void perf_regs_load(u64 *regs); +@ +@ This functions fills in the 'regs' buffer from the actual registers values. +@ Note that the return values (i.e. caller values) of sp and lr +@ are retrieved and stored, in order to skip the call to this function. + +.text +.type perf_regs_load,%function +ENTRY(perf_regs_load) + push {r1} + + str r0, [r0, #R0] + str r1, [r0, #R1] + str r2, [r0, #R2] + str r3, [r0, #R3] + str r4, [r0, #R4] + str r5, [r0, #R5] + str r6, [r0, #R6] + str r7, [r0, #R7] + str r8, [r0, #R8] + str r9, [r0, #R9] + str sl, [r0, #SL] + str fp, [r0, #FP] + str ip, [r0, #IP] + add r1, sp, #4 @ Retrieve and save sp at entry time + str r1, [r0, #SP] + str lr, [r0, #LR] + str lr, [r0, #PC] @ Save caller PC + + pop {r1} + bx lr +ENDPROC(perf_regs_load)
Introducing perf_regs_load function, which is going to be used for dwarf unwind test in following patches. It takes single argument as a pointer to the regs dump buffer and populates it with current registers values. Signed-off-by: Jean Pihet <jean.pihet@linaro.org> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Ingo Molnar <mingo@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net> Cc: David Ahern <dsahern@gmail.com> Cc: Jiri Olsa <jolsa@redhat.com> --- tools/perf/arch/arm/Makefile | 1 + tools/perf/arch/arm/include/perf_regs.h | 2 ++ tools/perf/arch/arm/tests/regs_load.S | 51 +++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 tools/perf/arch/arm/tests/regs_load.S