All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
@ 2021-06-20 14:45 Athira Rajeev
  2021-06-20 14:45 ` [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of " Athira Rajeev
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Athira Rajeev @ 2021-06-20 14:45 UTC (permalink / raw)
  To: mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry

Patch set adds PMU registers namely Sampled Instruction Address Register
(SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
in PowerPC. These registers provides the instruction/data address and
adding these to extended regs helps in debug purposes.

Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
Patch 2/2 includes perf tools side changes to add the SPRs to
sample_reg_mask to use with -I? option.

Athira Rajeev (2):
  powerpc/perf: Expose instruction and data address registers as part of
    extended regs
  tools/perf: Add perf tools support to expose instruction and data
    address registers as part of extended regs

 arch/powerpc/include/uapi/asm/perf_regs.h       | 12 +++++++-----
 arch/powerpc/perf/perf_regs.c                   |  4 ++++
 tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
 tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
 tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
 5 files changed, 22 insertions(+), 10 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs
  2021-06-20 14:45 [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs Athira Rajeev
@ 2021-06-20 14:45 ` Athira Rajeev
  2021-09-08  5:17   ` Michael Ellerman
  2021-06-20 14:46 ` [PATCH 2/2] tools/perf: Add perf tools support to expose " Athira Rajeev
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Athira Rajeev @ 2021-06-20 14:45 UTC (permalink / raw)
  To: mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry

Patch adds support to include Sampled Instruction Address Register
(SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
PERF_REG_EXTENDED_MAX to include these SPR's.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
 arch/powerpc/perf/perf_regs.c             |  4 ++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
index 578b3ee..cf5eee5 100644
--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -61,6 +61,8 @@ enum perf_event_powerpc_regs {
 	PERF_REG_POWERPC_PMC4,
 	PERF_REG_POWERPC_PMC5,
 	PERF_REG_POWERPC_PMC6,
+	PERF_REG_POWERPC_SDAR,
+	PERF_REG_POWERPC_SIAR,
 	/* Max regs without the extended regs */
 	PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
 };
@@ -72,16 +74,16 @@ enum perf_event_powerpc_regs {
 
 /*
  * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300
- * includes 9 SPRS from MMCR0 to PMC6 excluding the
+ * includes 11 SPRS from MMCR0 to SIAR excluding the
  * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300.
  */
-#define PERF_REG_PMU_MASK_300   ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
+#define PERF_REG_PMU_MASK_300   ((0x3fffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
 
 /*
  * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31
- * includes 12 SPRs from MMCR0 to PMC6.
+ * includes 14 SPRs from MMCR0 to SIAR.
  */
-#define PERF_REG_PMU_MASK_31   (0xfffULL << PERF_REG_POWERPC_MMCR0)
+#define PERF_REG_PMU_MASK_31   (0x3fffULL << PERF_REG_POWERPC_MMCR0)
 
-#define PERF_REG_EXTENDED_MAX  (PERF_REG_POWERPC_PMC6 + 1)
+#define PERF_REG_EXTENDED_MAX  (PERF_REG_POWERPC_SIAR + 1)
 #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index b931eed..51d31b6 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c
@@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
 		return mfspr(SPRN_SIER2);
 	case PERF_REG_POWERPC_SIER3:
 		return mfspr(SPRN_SIER3);
+	case PERF_REG_POWERPC_SDAR:
+		return mfspr(SPRN_SDAR);
 #endif
+	case PERF_REG_POWERPC_SIAR:
+		return mfspr(SPRN_SIAR);
 	default: return 0;
 	}
 }
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] tools/perf: Add perf tools support to expose instruction and data address registers as part of extended regs
  2021-06-20 14:45 [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs Athira Rajeev
  2021-06-20 14:45 ` [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of " Athira Rajeev
@ 2021-06-20 14:46 ` Athira Rajeev
  2021-06-21  4:09 ` [PATCH 0/2] powerpc/perf: Add instruction and data address registers to " Nageswara Sastry
  2021-09-02  7:34 ` kajoljain
  3 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2021-06-20 14:46 UTC (permalink / raw)
  To: mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry

Patch enables presenting of Sampled Instruction Address Register (SIAR)
and Sampled Data Address Register (SDAR) SPRs as part of extended regsiters
for perf tool. Add these SPR's to sample_reg_mask in the tool side (to use
with -I? option).

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
 tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
 tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
index 578b3ee..cf5eee5 100644
--- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -61,6 +61,8 @@ enum perf_event_powerpc_regs {
 	PERF_REG_POWERPC_PMC4,
 	PERF_REG_POWERPC_PMC5,
 	PERF_REG_POWERPC_PMC6,
+	PERF_REG_POWERPC_SDAR,
+	PERF_REG_POWERPC_SIAR,
 	/* Max regs without the extended regs */
 	PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
 };
@@ -72,16 +74,16 @@ enum perf_event_powerpc_regs {
 
 /*
  * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300
- * includes 9 SPRS from MMCR0 to PMC6 excluding the
+ * includes 11 SPRS from MMCR0 to SIAR excluding the
  * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300.
  */
-#define PERF_REG_PMU_MASK_300   ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
+#define PERF_REG_PMU_MASK_300   ((0x3fffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
 
 /*
  * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31
- * includes 12 SPRs from MMCR0 to PMC6.
+ * includes 14 SPRs from MMCR0 to SIAR.
  */
-#define PERF_REG_PMU_MASK_31   (0xfffULL << PERF_REG_POWERPC_MMCR0)
+#define PERF_REG_PMU_MASK_31   (0x3fffULL << PERF_REG_POWERPC_MMCR0)
 
-#define PERF_REG_EXTENDED_MAX  (PERF_REG_POWERPC_PMC6 + 1)
+#define PERF_REG_EXTENDED_MAX  (PERF_REG_POWERPC_SIAR + 1)
 #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 04e5dc0..93339d1 100644
--- a/tools/perf/arch/powerpc/include/perf_regs.h
+++ b/tools/perf/arch/powerpc/include/perf_regs.h
@@ -77,6 +77,8 @@
 	[PERF_REG_POWERPC_PMC4] = "pmc4",
 	[PERF_REG_POWERPC_PMC5] = "pmc5",
 	[PERF_REG_POWERPC_PMC6] = "pmc6",
+	[PERF_REG_POWERPC_SDAR] = "sdar",
+	[PERF_REG_POWERPC_SIAR] = "siar",
 };
 
 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 8116a25..8d07a78 100644
--- a/tools/perf/arch/powerpc/util/perf_regs.c
+++ b/tools/perf/arch/powerpc/util/perf_regs.c
@@ -74,6 +74,8 @@
 	SMPL_REG(pmc4, PERF_REG_POWERPC_PMC4),
 	SMPL_REG(pmc5, PERF_REG_POWERPC_PMC5),
 	SMPL_REG(pmc6, PERF_REG_POWERPC_PMC6),
+	SMPL_REG(sdar, PERF_REG_POWERPC_SDAR),
+	SMPL_REG(siar, PERF_REG_POWERPC_SIAR),
 	SMPL_REG_END
 };
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
  2021-06-20 14:45 [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs Athira Rajeev
  2021-06-20 14:45 ` [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of " Athira Rajeev
  2021-06-20 14:46 ` [PATCH 2/2] tools/perf: Add perf tools support to expose " Athira Rajeev
@ 2021-06-21  4:09 ` Nageswara Sastry
  2021-09-02  7:34 ` kajoljain
  3 siblings, 0 replies; 12+ messages in thread
From: Nageswara Sastry @ 2021-06-21  4:09 UTC (permalink / raw)
  To: Athira Rajeev, mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev



On 20/06/21 8:15 pm, Athira Rajeev wrote:
> Patch set adds PMU registers namely Sampled Instruction Address Register
> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
> in PowerPC. These registers provides the instruction/data address and
> adding these to extended regs helps in debug purposes.
> 
> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
> Patch 2/2 includes perf tools side changes to add the SPRs to
> sample_reg_mask to use with -I? option.
> 
> Athira Rajeev (2):
>    powerpc/perf: Expose instruction and data address registers as part of
>      extended regs
>    tools/perf: Add perf tools support to expose instruction and data
>      address registers as part of extended regs


Tested with the following scenarios on P9, P10 - PowerVM environment
1. perf record -I? - shows added - sdar, siar
2. perf record -I <workload> and perf report -D - shows added - sdar, 
siar with and with out counts.

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>


>   arch/powerpc/include/uapi/asm/perf_regs.h       | 12 +++++++-----
>   arch/powerpc/perf/perf_regs.c                   |  4 ++++
>   tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>   tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
>   tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
>   5 files changed, 22 insertions(+), 10 deletions(-)
> 

-- 
Thanks and Regards
R.Nageswara Sastry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
  2021-06-20 14:45 [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs Athira Rajeev
                   ` (2 preceding siblings ...)
  2021-06-21  4:09 ` [PATCH 0/2] powerpc/perf: Add instruction and data address registers to " Nageswara Sastry
@ 2021-09-02  7:34 ` kajoljain
  2021-09-06  2:43   ` Athira Rajeev
  3 siblings, 1 reply; 12+ messages in thread
From: kajoljain @ 2021-09-02  7:34 UTC (permalink / raw)
  To: Athira Rajeev, mpe, acme, jolsa; +Cc: maddy, linuxppc-dev, rnsastry



On 6/20/21 8:15 PM, Athira Rajeev wrote:
> Patch set adds PMU registers namely Sampled Instruction Address Register
> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
> in PowerPC. These registers provides the instruction/data address and
> adding these to extended regs helps in debug purposes.
> 
> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
> Patch 2/2 includes perf tools side changes to add the SPRs to
> sample_reg_mask to use with -I? option.
> 
> Athira Rajeev (2):
>   powerpc/perf: Expose instruction and data address registers as part of
>     extended regs
>   tools/perf: Add perf tools support to expose instruction and data
>     address registers as part of extended regs
> 

Patchset looks good to me.

Reviewed-By: kajol Jain<kjain@linux.ibm.com>

Thanks,
Kajol Jain

>  arch/powerpc/include/uapi/asm/perf_regs.h       | 12 +++++++-----
>  arch/powerpc/perf/perf_regs.c                   |  4 ++++
>  tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>  tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
>  tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
>  5 files changed, 22 insertions(+), 10 deletions(-)
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
  2021-09-02  7:34 ` kajoljain
@ 2021-09-06  2:43   ` Athira Rajeev
  2021-09-11 19:09     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 12+ messages in thread
From: Athira Rajeev @ 2021-09-06  2:43 UTC (permalink / raw)
  To: kajoljain, Arnaldo Carvalho de Melo
  Cc: Madhavan Srinivasan, linuxppc-dev, Jiri Olsa, rnsastry



> On 02-Sep-2021, at 1:04 PM, kajoljain <kjain@linux.ibm.com> wrote:
> 
> 
> 
> On 6/20/21 8:15 PM, Athira Rajeev wrote:
>> Patch set adds PMU registers namely Sampled Instruction Address Register
>> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
>> in PowerPC. These registers provides the instruction/data address and
>> adding these to extended regs helps in debug purposes.
>> 
>> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
>> Patch 2/2 includes perf tools side changes to add the SPRs to
>> sample_reg_mask to use with -I? option.
>> 
>> Athira Rajeev (2):
>>  powerpc/perf: Expose instruction and data address registers as part of
>>    extended regs
>>  tools/perf: Add perf tools support to expose instruction and data
>>    address registers as part of extended regs
>> 
> 
> Patchset looks good to me.
> 
> Reviewed-By: kajol Jain<kjain@linux.ibm.com>

Hi Arnaldo,

Requesting for your review on this patchset.

Thanks
Athira
> 
> Thanks,
> Kajol Jain
> 
>> arch/powerpc/include/uapi/asm/perf_regs.h       | 12 +++++++-----
>> arch/powerpc/perf/perf_regs.c                   |  4 ++++
>> tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>> tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
>> tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
>> 5 files changed, 22 insertions(+), 10 deletions(-)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs
  2021-06-20 14:45 ` [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of " Athira Rajeev
@ 2021-09-08  5:17   ` Michael Ellerman
  2021-09-09  2:48     ` Athira Rajeev
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2021-09-08  5:17 UTC (permalink / raw)
  To: Athira Rajeev, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry

Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> Patch adds support to include Sampled Instruction Address Register
> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
> PERF_REG_EXTENDED_MAX to include these SPR's.
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>  arch/powerpc/perf/perf_regs.c             |  4 ++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
...
> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
> index b931eed..51d31b6 100644
> --- a/arch/powerpc/perf/perf_regs.c
> +++ b/arch/powerpc/perf/perf_regs.c
> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
>  		return mfspr(SPRN_SIER2);
>  	case PERF_REG_POWERPC_SIER3:
>  		return mfspr(SPRN_SIER3);
> +	case PERF_REG_POWERPC_SDAR:
> +		return mfspr(SPRN_SDAR);
>  #endif
> +	case PERF_REG_POWERPC_SIAR:
> +		return mfspr(SPRN_SIAR);
>  	default: return 0;
>  	}

This file is built for all powerpc configs that have PERF_EVENTS. Which
includes CPUs that don't have SDAR or SIAR.

Don't we need checks in perf_reg_value() like we do for SIER?

I guess we already got this wrong when we added the Power10 registers,
SIER2/3 etc.

cheers

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs
  2021-09-08  5:17   ` Michael Ellerman
@ 2021-09-09  2:48     ` Athira Rajeev
  2021-09-20  7:13       ` Michael Ellerman
  0 siblings, 1 reply; 12+ messages in thread
From: Athira Rajeev @ 2021-09-09  2:48 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Madhavan Srinivasan, rnsastry, kajoljain,
	Arnaldo Carvalho de Melo, Jiri Olsa, linuxppc-dev



> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>> Patch adds support to include Sampled Instruction Address Register
>> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
>> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
>> PERF_REG_EXTENDED_MAX to include these SPR's.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>> arch/powerpc/perf/perf_regs.c             |  4 ++++
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>> 
> ...
>> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
>> index b931eed..51d31b6 100644
>> --- a/arch/powerpc/perf/perf_regs.c
>> +++ b/arch/powerpc/perf/perf_regs.c
>> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
>> 		return mfspr(SPRN_SIER2);
>> 	case PERF_REG_POWERPC_SIER3:
>> 		return mfspr(SPRN_SIER3);
>> +	case PERF_REG_POWERPC_SDAR:
>> +		return mfspr(SPRN_SDAR);
>> #endif
>> +	case PERF_REG_POWERPC_SIAR:
>> +		return mfspr(SPRN_SIAR);
>> 	default: return 0;
>> 	}
> 
> This file is built for all powerpc configs that have PERF_EVENTS. Which
> includes CPUs that don't have SDAR or SIAR.
> 
> Don't we need checks in perf_reg_value() like we do for SIER?

Hi Michael,

Thanks for the review.

SIER is part of PERF_REG_PMU_MASK and hence check is needed to see if platform supports SIER.
Incase of extended regs, they are part of PERF_REG_EXTENDED_MASK and this mask is
filled with supported registers while registering the PMU ( ie during init_power9/10_pmu ). So these registers will be added
only for supported platforms. The validity of extended mask is also done in PMU common code 
( In kernel/events/core.c with PERF_REG_EXTENDED_MASK check ). So an unsupported platform requesting for extended
registers won’t get it.

Thanks
Athira
> 
> I guess we already got this wrong when we added the Power10 registers,
> SIER2/3 etc.
> 
> cheers


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
  2021-09-06  2:43   ` Athira Rajeev
@ 2021-09-11 19:09     ` Arnaldo Carvalho de Melo
  2021-09-20  6:54       ` Michael Ellerman
  0 siblings, 1 reply; 12+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-09-11 19:09 UTC (permalink / raw)
  To: Athira Rajeev
  Cc: Madhavan Srinivasan, rnsastry, kajoljain, Jiri Olsa, linuxppc-dev

Em Mon, Sep 06, 2021 at 08:13:13AM +0530, Athira Rajeev escreveu:
> 
> 
> > On 02-Sep-2021, at 1:04 PM, kajoljain <kjain@linux.ibm.com> wrote:
> > 
> > 
> > 
> > On 6/20/21 8:15 PM, Athira Rajeev wrote:
> >> Patch set adds PMU registers namely Sampled Instruction Address Register
> >> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
> >> in PowerPC. These registers provides the instruction/data address and
> >> adding these to extended regs helps in debug purposes.
> >> 
> >> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
> >> Patch 2/2 includes perf tools side changes to add the SPRs to
> >> sample_reg_mask to use with -I? option.
> >> 
> >> Athira Rajeev (2):
> >>  powerpc/perf: Expose instruction and data address registers as part of
> >>    extended regs
> >>  tools/perf: Add perf tools support to expose instruction and data
> >>    address registers as part of extended regs
> >> 
> > 
> > Patchset looks good to me.
> > 
> > Reviewed-By: kajol Jain<kjain@linux.ibm.com>
> 
> Hi Arnaldo,
> 
> Requesting for your review on this patchset.

So, this touches the kernel, usually I get a patchkit when the kernel
bits landed, is that the case by now?

- Arnaldo

> 
> Thanks
> Athira
> > 
> > Thanks,
> > Kajol Jain
> > 
> >> arch/powerpc/include/uapi/asm/perf_regs.h       | 12 +++++++-----
> >> arch/powerpc/perf/perf_regs.c                   |  4 ++++
> >> tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
> >> tools/perf/arch/powerpc/include/perf_regs.h     |  2 ++
> >> tools/perf/arch/powerpc/util/perf_regs.c        |  2 ++
> >> 5 files changed, 22 insertions(+), 10 deletions(-)

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
  2021-09-11 19:09     ` Arnaldo Carvalho de Melo
@ 2021-09-20  6:54       ` Michael Ellerman
  0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2021-09-20  6:54 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Athira Rajeev
  Cc: kajoljain, Madhavan Srinivasan, linuxppc-dev, Jiri Olsa, rnsastry

Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> Em Mon, Sep 06, 2021 at 08:13:13AM +0530, Athira Rajeev escreveu:
>> > On 02-Sep-2021, at 1:04 PM, kajoljain <kjain@linux.ibm.com> wrote:
>> > On 6/20/21 8:15 PM, Athira Rajeev wrote:
>> >> Patch set adds PMU registers namely Sampled Instruction Address Register
>> >> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs
>> >> in PowerPC. These registers provides the instruction/data address and
>> >> adding these to extended regs helps in debug purposes.
>> >> 
>> >> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask.
>> >> Patch 2/2 includes perf tools side changes to add the SPRs to
>> >> sample_reg_mask to use with -I? option.
>> >> 
>> >> Athira Rajeev (2):
>> >>  powerpc/perf: Expose instruction and data address registers as part of
>> >>    extended regs
>> >>  tools/perf: Add perf tools support to expose instruction and data
>> >>    address registers as part of extended regs
>> >> 
>> > 
>> > Patchset looks good to me.
>> > 
>> > Reviewed-By: kajol Jain<kjain@linux.ibm.com>
>> 
>> Hi Arnaldo,
>> 
>> Requesting for your review on this patchset.
>
> So, this touches the kernel, usually I get a patchkit when the kernel
> bits landed, is that the case by now?

Not yet, I'd like some changes to the kernel patch.

cheers

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs
  2021-09-09  2:48     ` Athira Rajeev
@ 2021-09-20  7:13       ` Michael Ellerman
  2021-09-21  3:01         ` Athira Rajeev
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2021-09-20  7:13 UTC (permalink / raw)
  To: Athira Rajeev
  Cc: Madhavan Srinivasan, rnsastry, kajoljain,
	Arnaldo Carvalho de Melo, Jiri Olsa, linuxppc-dev

Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> 
>> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>>> Patch adds support to include Sampled Instruction Address Register
>>> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
>>> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
>>> PERF_REG_EXTENDED_MAX to include these SPR's.
>>> 
>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>>> arch/powerpc/perf/perf_regs.c             |  4 ++++
>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>> 
>> ...
>>> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
>>> index b931eed..51d31b6 100644
>>> --- a/arch/powerpc/perf/perf_regs.c
>>> +++ b/arch/powerpc/perf/perf_regs.c
>>> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
>>> 		return mfspr(SPRN_SIER2);
>>> 	case PERF_REG_POWERPC_SIER3:
>>> 		return mfspr(SPRN_SIER3);
>>> +	case PERF_REG_POWERPC_SDAR:
>>> +		return mfspr(SPRN_SDAR);
>>> #endif
>>> +	case PERF_REG_POWERPC_SIAR:
>>> +		return mfspr(SPRN_SIAR);
>>> 	default: return 0;
>>> 	}
>> 
>> This file is built for all powerpc configs that have PERF_EVENTS. Which
>> includes CPUs that don't have SDAR or SIAR.
>> 
>> Don't we need checks in perf_reg_value() like we do for SIER?
>
> Hi Michael,
>
> Thanks for the review.
>
> SIER is part of PERF_REG_PMU_MASK and hence check is needed to see if platform supports SIER.
> Incase of extended regs, they are part of PERF_REG_EXTENDED_MASK and this mask is
> filled with supported registers while registering the PMU ( ie during init_power9/10_pmu ). So these registers will be added
> only for supported platforms. The validity of extended mask is also done in PMU common code 
> ( In kernel/events/core.c with PERF_REG_EXTENDED_MASK check ). So an unsupported platform requesting for extended
> registers won’t get it.

Right, I'd forgotten how that works.

But I think part of the reason I didn't remember is that
PERF_REG_PMU_MASK_31 doesn't mention those regs by name, it's just a hex
constant, ie:

-#define PERF_REG_PMU_MASK_31   (0xfffULL << PERF_REG_POWERPC_MMCR0)
+#define PERF_REG_PMU_MASK_31   (0x3fffULL << PERF_REG_POWERPC_MMCR0)

Presumably you tested that the added 0x3 there sets the right bits for
SDAR and SIAR, but it's 1) not obvious and 2) fragile.

So I'd like it better if we constructed the PERF_REG_PMU_MASK_31, and
other similar masks, by or'ing together the actual register value
constants.

eg. something like:

#define PERF_REG_PMU_MASK_31	\
	((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \
	(1ul << PERF_REG_POWERPC_MMCR2) | (1ul << PERF_REG_POWERPC_MMCR3) | \
	(1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3) | \
	(1ul << PERF_REG_POWERPC_PMC1) | (1ul << PERF_REG_POWERPC_PMC2) | \
	(1ul << PERF_REG_POWERPC_PMC3) | (1ul << PERF_REG_POWERPC_PMC4) | \
	(1ul << PERF_REG_POWERPC_PMC5) | (1ul << PERF_REG_POWERPC_PMC6))


Also PERF_REG_EXTENDED_MAX should be part of the enum, just like
PERF_REG_POWERPC_MAX.

cheers

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of extended regs
  2021-09-20  7:13       ` Michael Ellerman
@ 2021-09-21  3:01         ` Athira Rajeev
  0 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2021-09-21  3:01 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Madhavan Srinivasan, rnsastry, kajoljain,
	Arnaldo Carvalho de Melo, Jiri Olsa, linuxppc-dev



> On 20-Sep-2021, at 12:43 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>>> On 08-Sep-2021, at 10:47 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> 
>>> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>>>> Patch adds support to include Sampled Instruction Address Register
>>>> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
>>>> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
>>>> PERF_REG_EXTENDED_MAX to include these SPR's.
>>>> 
>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>>> ---
>>>> arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++++++-----
>>>> arch/powerpc/perf/perf_regs.c             |  4 ++++
>>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>> 
>>> ...
>>>> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
>>>> index b931eed..51d31b6 100644
>>>> --- a/arch/powerpc/perf/perf_regs.c
>>>> +++ b/arch/powerpc/perf/perf_regs.c
>>>> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
>>>> 		return mfspr(SPRN_SIER2);
>>>> 	case PERF_REG_POWERPC_SIER3:
>>>> 		return mfspr(SPRN_SIER3);
>>>> +	case PERF_REG_POWERPC_SDAR:
>>>> +		return mfspr(SPRN_SDAR);
>>>> #endif
>>>> +	case PERF_REG_POWERPC_SIAR:
>>>> +		return mfspr(SPRN_SIAR);
>>>> 	default: return 0;
>>>> 	}
>>> 
>>> This file is built for all powerpc configs that have PERF_EVENTS. Which
>>> includes CPUs that don't have SDAR or SIAR.
>>> 
>>> Don't we need checks in perf_reg_value() like we do for SIER?
>> 
>> Hi Michael,
>> 
>> Thanks for the review.
>> 
>> SIER is part of PERF_REG_PMU_MASK and hence check is needed to see if platform supports SIER.
>> Incase of extended regs, they are part of PERF_REG_EXTENDED_MASK and this mask is
>> filled with supported registers while registering the PMU ( ie during init_power9/10_pmu ). So these registers will be added
>> only for supported platforms. The validity of extended mask is also done in PMU common code 
>> ( In kernel/events/core.c with PERF_REG_EXTENDED_MASK check ). So an unsupported platform requesting for extended
>> registers won’t get it.
> 
> Right, I'd forgotten how that works.
> 
> But I think part of the reason I didn't remember is that
> PERF_REG_PMU_MASK_31 doesn't mention those regs by name, it's just a hex
> constant, ie:
> 
> -#define PERF_REG_PMU_MASK_31   (0xfffULL << PERF_REG_POWERPC_MMCR0)
> +#define PERF_REG_PMU_MASK_31   (0x3fffULL << PERF_REG_POWERPC_MMCR0)
> 
> Presumably you tested that the added 0x3 there sets the right bits for
> SDAR and SIAR, but it's 1) not obvious and 2) fragile.
> 
> So I'd like it better if we constructed the PERF_REG_PMU_MASK_31, and
> other similar masks, by or'ing together the actual register value
> constants.
> 
> eg. something like:
> 
> #define PERF_REG_PMU_MASK_31	\
> 	((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \
> 	(1ul << PERF_REG_POWERPC_MMCR2) | (1ul << PERF_REG_POWERPC_MMCR3) | \
> 	(1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3) | \
> 	(1ul << PERF_REG_POWERPC_PMC1) | (1ul << PERF_REG_POWERPC_PMC2) | \
> 	(1ul << PERF_REG_POWERPC_PMC3) | (1ul << PERF_REG_POWERPC_PMC4) | \
> 	(1ul << PERF_REG_POWERPC_PMC5) | (1ul << PERF_REG_POWERPC_PMC6))
> 
> 
> Also PERF_REG_EXTENDED_MAX should be part of the enum, just like
> PERF_REG_POWERPC_MAX.

Sure Michael,

I will address these changes in next version

Thanks
Athira
> 
> cheers


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-09-21  3:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-20 14:45 [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs Athira Rajeev
2021-06-20 14:45 ` [PATCH 1/2] powerpc/perf: Expose instruction and data address registers as part of " Athira Rajeev
2021-09-08  5:17   ` Michael Ellerman
2021-09-09  2:48     ` Athira Rajeev
2021-09-20  7:13       ` Michael Ellerman
2021-09-21  3:01         ` Athira Rajeev
2021-06-20 14:46 ` [PATCH 2/2] tools/perf: Add perf tools support to expose " Athira Rajeev
2021-06-21  4:09 ` [PATCH 0/2] powerpc/perf: Add instruction and data address registers to " Nageswara Sastry
2021-09-02  7:34 ` kajoljain
2021-09-06  2:43   ` Athira Rajeev
2021-09-11 19:09     ` Arnaldo Carvalho de Melo
2021-09-20  6:54       ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.