[PATCH RESEND] powerpc/perf: Update perf_regs structure to include SIER

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Anju T Sudhakar <redacted>
Cc: Ravi Bangoria <redacted>
Tested-by: Ravi Bangoria <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/include/uapi/asm/perf_regs.h       | 1 +
 arch/powerpc/perf/perf_regs.c                   | 1 +
 tools/arch/powerpc/include/uapi/asm/perf_regs.h | 1 +
 tools/perf/arch/powerpc/include/perf_regs.h     | 3 ++-
 tools/perf/arch/powerpc/util/perf_regs.c        | 1 +
 5 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
index 9e52c86ccbd3..ff91192407d1 100644
--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h

@@ -46,6 +46,7 @@ enum perf_event_powerpc_regs {
 	PERF_REG_POWERPC_TRAP,
 	PERF_REG_POWERPC_DAR,
 	PERF_REG_POWERPC_DSISR,
+	PERF_REG_POWERPC_SIER,
 	PERF_REG_POWERPC_MAX,
 };
 #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */

diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index 09ceea6175ba..c262aea22ad9 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c

@@ -69,6 +69,7 @@ static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
 	PT_REGS_OFFSET(PERF_REG_POWERPC_TRAP, trap),
 	PT_REGS_OFFSET(PERF_REG_POWERPC_DAR, dar),
 	PT_REGS_OFFSET(PERF_REG_POWERPC_DSISR, dsisr),
+	PT_REGS_OFFSET(PERF_REG_POWERPC_SIER, dar),
 };
 
 u64 perf_reg_value(struct pt_regs *regs, int idx)

diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
index 9e52c86ccbd3..ff91192407d1 100644
--- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h

@@ -46,6 +46,7 @@ enum perf_event_powerpc_regs {
 	PERF_REG_POWERPC_TRAP,
 	PERF_REG_POWERPC_DAR,
 	PERF_REG_POWERPC_DSISR,
+	PERF_REG_POWERPC_SIER,
 	PERF_REG_POWERPC_MAX,
 };
 #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */

diff --git a/tools/perf/arch/powerpc/include/perf_regs.h b/tools/perf/arch/powerpc/include/perf_regs.h
index 00e37b106913..1076393e6f43 100644
--- a/tools/perf/arch/powerpc/include/perf_regs.h
+++ b/tools/perf/arch/powerpc/include/perf_regs.h

@@ -62,7 +62,8 @@ static const char *reg_names[] = {
 	[PERF_REG_POWERPC_SOFTE] = "softe",
 	[PERF_REG_POWERPC_TRAP] = "trap",
 	[PERF_REG_POWERPC_DAR] = "dar",
-	[PERF_REG_POWERPC_DSISR] = "dsisr"
+	[PERF_REG_POWERPC_DSISR] = "dsisr",
+	[PERF_REG_POWERPC_SIER] = "sier"
 };
 
 static inline const char *perf_reg_name(int id)

diff --git a/tools/perf/arch/powerpc/util/perf_regs.c b/tools/perf/arch/powerpc/util/perf_regs.c
index ec50939b0418..07fcd977d93e 100644
--- a/tools/perf/arch/powerpc/util/perf_regs.c
+++ b/tools/perf/arch/powerpc/util/perf_regs.c

@@ -52,6 +52,7 @@ const struct sample_reg sample_reg_masks[] = {
 	SMPL_REG(trap, PERF_REG_POWERPC_TRAP),
 	SMPL_REG(dar, PERF_REG_POWERPC_DAR),
 	SMPL_REG(dsisr, PERF_REG_POWERPC_DSISR),
+	SMPL_REG(sier, PERF_REG_POWERPC_SIER),
 	SMPL_REG_END
 };
 

-- 
2.7.4

Le 26/11/2018 à 19:04, Madhavan Srinivasan a écrit :

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Anju T Sudhakar <redacted>
Cc: Ravi Bangoria <redacted>
Tested-by: Ravi Bangoria <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
---

I see this patch is already in patchwork 
https://patchwork.ozlabs.org/patch/941908/ and I can't spot any 
difference between the two. Please say here (under the ---) the reason 
for resending.

Christophe

On 27/11/18 12:24 PM, Christophe LEROY wrote:

Le 26/11/2018 à 19:04, Madhavan Srinivasan a écrit :

quoted

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the 
"SIER"
printing which internally maps to the "dar" of pt_regs.

Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Anju T Sudhakar <redacted>
Cc: Ravi Bangoria <redacted>
Tested-by: Ravi Bangoria <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
---

I see this patch is already in patchwork 
https://patchwork.ozlabs.org/patch/941908/ and I can't spot any 
difference between the two. Please say here (under the ---) the reason 
for resending.

Yes there is no change, justed added the Tested-by tag
and resent it for arnaldo's review/ack.
May be should have avoided resend tag. My bad.

Maddy

Christophe

quoted

arch/powerpc/include/uapi/asm/perf_regs.h       | 1 +
  arch/powerpc/perf/perf_regs.c                   | 1 +
  tools/arch/powerpc/include/uapi/asm/perf_regs.h | 1 +
  tools/perf/arch/powerpc/include/perf_regs.h     | 3 ++-
  tools/perf/arch/powerpc/util/perf_regs.c        | 1 +
  5 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h 

b/arch/powerpc/include/uapi/asm/perf_regs.h
index 9e52c86ccbd3..ff91192407d1 100644

--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h

@@ -46,6 +46,7 @@ enum perf_event_powerpc_regs {

      PERF_REG_POWERPC_TRAP,
      PERF_REG_POWERPC_DAR,
      PERF_REG_POWERPC_DSISR,
+    PERF_REG_POWERPC_SIER,
      PERF_REG_POWERPC_MAX,
  };
  #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */

diff --git a/arch/powerpc/perf/perf_regs.c 

b/arch/powerpc/perf/perf_regs.c
index 09ceea6175ba..c262aea22ad9 100644

--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c

@@ -69,6 +69,7 @@ static unsigned int 

pt_regs_offset[PERF_REG_POWERPC_MAX] = {
      PT_REGS_OFFSET(PERF_REG_POWERPC_TRAP, trap),
      PT_REGS_OFFSET(PERF_REG_POWERPC_DAR, dar),
      PT_REGS_OFFSET(PERF_REG_POWERPC_DSISR, dsisr),
+    PT_REGS_OFFSET(PERF_REG_POWERPC_SIER, dar),
  };
    u64 perf_reg_value(struct pt_regs *regs, int idx)

diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h 

b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
index 9e52c86ccbd3..ff91192407d1 100644

--- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h

@@ -46,6 +46,7 @@ enum perf_event_powerpc_regs {

      PERF_REG_POWERPC_TRAP,
      PERF_REG_POWERPC_DAR,
      PERF_REG_POWERPC_DSISR,
+    PERF_REG_POWERPC_SIER,
      PERF_REG_POWERPC_MAX,
  };
  #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */

diff --git a/tools/perf/arch/powerpc/include/perf_regs.h 

b/tools/perf/arch/powerpc/include/perf_regs.h
index 00e37b106913..1076393e6f43 100644

--- a/tools/perf/arch/powerpc/include/perf_regs.h
+++ b/tools/perf/arch/powerpc/include/perf_regs.h

@@ -62,7 +62,8 @@ static const char *reg_names[] = {

      [PERF_REG_POWERPC_SOFTE] = "softe",
      [PERF_REG_POWERPC_TRAP] = "trap",
      [PERF_REG_POWERPC_DAR] = "dar",
-    [PERF_REG_POWERPC_DSISR] = "dsisr"
+    [PERF_REG_POWERPC_DSISR] = "dsisr",
+    [PERF_REG_POWERPC_SIER] = "sier"
  };
    static inline const char *perf_reg_name(int id)

diff --git a/tools/perf/arch/powerpc/util/perf_regs.c 

b/tools/perf/arch/powerpc/util/perf_regs.c
index ec50939b0418..07fcd977d93e 100644

--- a/tools/perf/arch/powerpc/util/perf_regs.c
+++ b/tools/perf/arch/powerpc/util/perf_regs.c

@@ -52,6 +52,7 @@ const struct sample_reg sample_reg_masks[] = {

      SMPL_REG(trap, PERF_REG_POWERPC_TRAP),
      SMPL_REG(dar, PERF_REG_POWERPC_DAR),
      SMPL_REG(dsisr, PERF_REG_POWERPC_DSISR),
+    SMPL_REG(sier, PERF_REG_POWERPC_SIER),
      SMPL_REG_END
  };

Em Mon, Nov 26, 2018 at 11:34:08PM +0530, Madhavan Srinivasan escreveu:

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

I think the patch is ok, when we talked in Vancouver I thought I saw
something like this before, i.e. adding more registers to a perf_regs.h
file, this was the cset:

  commit 0da0017f72554c005c1a04c3adc5da9eb64fa7e5
  Author: Hendrik Brueckner [off-list ref]
  Date:   Wed Nov 8 07:30:15 2017 +0100

      s390/perf: extend perf_regs support to include floating-point registers

That I came across because it broke the perf build, making me add this
cset:

  commit 10b9baa701d5023897f70a4acb3bf0235da3dc4f
  Author: Arnaldo Carvalho de Melo [off-list ref]
  Date:   Tue Nov 28 11:08:41 2017 -0300

    tools arch s390: Do not include header files from the kernel sources

:-)

Michael? What about the ppc specific details?

- Arnaldo
 

-- 

- Arnaldo

Arnaldo Carvalho de Melo [off-list ref] writes:

Em Mon, Nov 26, 2018 at 11:34:08PM +0530, Madhavan Srinivasan escreveu:

quoted

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

I think the patch is ok, when we talked in Vancouver I thought I saw
something like this before, i.e. adding more registers to a perf_regs.h
file, this was the cset:

  commit 0da0017f72554c005c1a04c3adc5da9eb64fa7e5
  Author: Hendrik Brueckner [off-list ref]
  Date:   Wed Nov 8 07:30:15 2017 +0100

      s390/perf: extend perf_regs support to include floating-point registers

That I came across because it broke the perf build, making me add this
cset:

  commit 10b9baa701d5023897f70a4acb3bf0235da3dc4f
  Author: Arnaldo Carvalho de Melo [off-list ref]
  Date:   Tue Nov 28 11:08:41 2017 -0300

    tools arch s390: Do not include header files from the kernel sources

:-)

Michael? What about the ppc specific details?

The only possible objection is that not all CPUs have an SIER register,
so on CPUs without it you'll get the content of the DAR register rather
than the SIER (because we (ab)use the DAR slot of pt_regs for the SIER).

Perhaps we should make sure that we return 0 on CPUs that don't have the
register?

cheers

On 28/11/18 9:04 AM, Michael Ellerman wrote:

Arnaldo Carvalho de Melo [off-list ref] writes:

quoted

Em Mon, Nov 26, 2018 at 11:34:08PM +0530, Madhavan Srinivasan escreveu:

quoted

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

I think the patch is ok, when we talked in Vancouver I thought I saw
something like this before, i.e. adding more registers to a perf_regs.h
file, this was the cset:

   commit 0da0017f72554c005c1a04c3adc5da9eb64fa7e5
   Author: Hendrik Brueckner [off-list ref]
   Date:   Wed Nov 8 07:30:15 2017 +0100

       s390/perf: extend perf_regs support to include floating-point registers

That I came across because it broke the perf build, making me add this
cset:

   commit 10b9baa701d5023897f70a4acb3bf0235da3dc4f
   Author: Arnaldo Carvalho de Melo [off-list ref]
   Date:   Tue Nov 28 11:08:41 2017 -0300

     tools arch s390: Do not include header files from the kernel sources

:-)

Michael? What about the ppc specific details?

The only possible objection is that not all CPUs have an SIER register,
so on CPUs without it you'll get the content of the DAR register rather
than the SIER (because we (ab)use the DAR slot of pt_regs for the SIER).

Perhaps we should make sure that we return 0 on CPUs that don't have the
register?

Yes this make sense. We should make it zero instead of having the DAR
value. I will respin the patch with that change.

Thanks for the review comments.

Maddy

cheers

Madhavan Srinivasan [off-list ref] writes:

On 28/11/18 9:04 AM, Michael Ellerman wrote:

quoted

Arnaldo Carvalho de Melo [off-list ref] writes:

quoted

Em Mon, Nov 26, 2018 at 11:34:08PM +0530, Madhavan Srinivasan escreveu:

quoted

On each sample, Sample Instruction Event Register (SIER) content
is saved in pt_regs. SIER does not have a entry as-is in the pt_regs
but instead, SIER content is saved in the "dar" register of pt_regs.

Patch adds another entry to the perf_regs structure to include the "SIER"
printing which internally maps to the "dar" of pt_regs.

I think the patch is ok, when we talked in Vancouver I thought I saw
something like this before, i.e. adding more registers to a perf_regs.h
file, this was the cset:

   commit 0da0017f72554c005c1a04c3adc5da9eb64fa7e5
   Author: Hendrik Brueckner [off-list ref]
   Date:   Wed Nov 8 07:30:15 2017 +0100

       s390/perf: extend perf_regs support to include floating-point registers

That I came across because it broke the perf build, making me add this
cset:

   commit 10b9baa701d5023897f70a4acb3bf0235da3dc4f
   Author: Arnaldo Carvalho de Melo [off-list ref]
   Date:   Tue Nov 28 11:08:41 2017 -0300

     tools arch s390: Do not include header files from the kernel sources

:-)

Michael? What about the ppc specific details?

The only possible objection is that not all CPUs have an SIER register,
so on CPUs without it you'll get the content of the DAR register rather
than the SIER (because we (ab)use the DAR slot of pt_regs for the SIER).

Perhaps we should make sure that we return 0 on CPUs that don't have the
register?

Yes this make sense. We should make it zero instead of having the DAR
value. I will respin the patch with that change.

Thanks.

There's three cases we need to cover.

The first is when we're not using core-book3s.c at all, ie. when we're
using the Freescale PMU code. You can probably handle that at build time
using CONFIG_FSL_EMB_PERF_EVENT?

Then there's the 32-bit case in core-book3s.c, currently that version of
perf_read_regs() never sets a value in dar.

And then there's the 64-bit case but PPMU_HAS_SIER is not set.

cheers