Re: [PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform

2020-07-09 Thread Athira Rajeev 


> On 08-Jul-2020, at 5:34 PM, Michael Ellerman  wrote:
> 
> Athira Rajeev  > writes:
>> Include capability flag `PERF_PMU_CAP_EXTENDED_REGS` for power10
>> and expose MMCR3, SIER2, SIER3 registers as part of extended regs.
>> Also introduce `PERF_REG_PMU_MASK_31` to define extended mask
>> value at runtime for power10
>> 
>> Signed-off-by: Athira Rajeev 
>> ---
>> arch/powerpc/include/uapi/asm/perf_regs.h   |  6 ++
>> arch/powerpc/perf/perf_regs.c   | 10 +-
>> arch/powerpc/perf/power10-pmu.c |  6 ++
>> tools/arch/powerpc/include/uapi/asm/perf_regs.h |  6 ++
>> tools/perf/arch/powerpc/include/perf_regs.h |  3 +++
>> tools/perf/arch/powerpc/util/perf_regs.c|  6 ++
> 
> Please split into a kernel patch and a tools patch. And cc the tools people.

Ok sure
> 
>> 6 files changed, 36 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h 
>> b/arch/powerpc/include/uapi/asm/perf_regs.h
>> index 485b1d5..020b51c 100644
>> --- a/arch/powerpc/include/uapi/asm/perf_regs.h
>> +++ b/arch/powerpc/include/uapi/asm/perf_regs.h
>> @@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
>>  PERF_REG_POWERPC_MMCR0,
>>  PERF_REG_POWERPC_MMCR1,
>>  PERF_REG_POWERPC_MMCR2,
>> +PERF_REG_POWERPC_MMCR3,
>> +PERF_REG_POWERPC_SIER2,
>> +PERF_REG_POWERPC_SIER3,
>>  /* Max regs without the extended regs */
>>  PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
>> };
>> @@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
>> #define PERF_REG_PMU_MASK_300   (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 
>> 1) \
>>  - PERF_REG_PMU_MASK)
>> 
>> +/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
>> +#define PERF_REG_PMU_MASK_31(((1ULL << (PERF_REG_POWERPC_SIER3 + 
>> 1)) - 1) \
>> +- PERF_REG_PMU_MASK)
> 
> Wrapping that provides no benefit, just let it be long.
> 

Ok,

>> #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
>> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
>> index c8a7e8c..c969935 100644
>> --- a/arch/powerpc/perf/perf_regs.c
>> +++ b/arch/powerpc/perf/perf_regs.c
>> @@ -81,6 +81,12 @@ static u64 get_ext_regs_value(int idx)
>>  return mfspr(SPRN_MMCR1);
>>  case PERF_REG_POWERPC_MMCR2:
>>  return mfspr(SPRN_MMCR2);
>> +case PERF_REG_POWERPC_MMCR3:
>> +return mfspr(SPRN_MMCR3);
>> +case PERF_REG_POWERPC_SIER2:
>> +return mfspr(SPRN_SIER2);
>> +case PERF_REG_POWERPC_SIER3:
>> +return mfspr(SPRN_SIER3);
> 
> Indentation is wrong.
> 
>>  default: return 0;
>>  }
>> }
>> @@ -89,7 +95,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>> {
>>  u64 PERF_REG_EXTENDED_MAX;
>> 
>> -if (cpu_has_feature(CPU_FTR_ARCH_300))
>> +if (cpu_has_feature(CPU_FTR_ARCH_31))
>> +PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_SIER3 + 1;
> 
> There's no way to know if that's correct other than going back to the
> header to look at the list of values.
> 
> So instead you should define it in the header, next to the other values,
> with a meaningful name, like PERF_REG_MAX_ISA_31 or something.
> 
>> +else if (cpu_has_feature(CPU_FTR_ARCH_300))
>>  PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_MMCR2 + 1;
> 
> Same.
> 

Ok, will make this change

>>  if (idx == PERF_REG_POWERPC_SIER &&
>> diff --git a/arch/powerpc/perf/power10-pmu.c 
>> b/arch/powerpc/perf/power10-pmu.c
>> index 07fb919..51082d6 100644
>> --- a/arch/powerpc/perf/power10-pmu.c
>> +++ b/arch/powerpc/perf/power10-pmu.c
>> @@ -86,6 +86,8 @@
>> #define POWER10_MMCRA_IFM3   0xC000UL
>> #define POWER10_MMCRA_BHRB_MASK  0xC000UL
>> 
>> +extern u64 mask_var;
> 
> Why is it extern? Also not a good name for a global.
> 
> Hang on, it's not even used? Is there some macro magic somewhere?

This is defined in patch 8 "powerpc/perf: Add support for outputting extended 
regs in perf intr_regs”, 
which adds the base support for extended regs in powerpc. Current patch covers 
changes to support
It for power10. 

`mask_var` is used to define `PERF_REG_EXTENDED_MASK` at run time. 
`PERF_REG_EXTENDED_MASK` basically contains mask value of supported extended 
registers.
And since supported registers may differ between processor versions, we are 
defining this mask at runtime.

The #define is done in arch/powerpc/include/asm/perf_event_server.h ( in patch 
8 ).
In the PMU driver init, we will set the respective mask value ( in the below 
code ). Hence it is extern

Sorry for the confusion here. 

Thanks
Athira

> 
>> /* Table of alternatives, sorted by column 0 */
>> static const unsigned int power10_event_alternatives[][MAX_ALT] = {
>>  { PM_RUN_CYC_ALT,   PM_RUN_CYC },
>> @@ -397,6 +399,7 @@ static void power10_config_bhrb(u64 pmu_bhrb_filter)
>>

Re: [PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform

2020-07-08 Thread Michael Ellerman 
Athira Rajeev  writes:
> Include capability flag `PERF_PMU_CAP_EXTENDED_REGS` for power10
> and expose MMCR3, SIER2, SIER3 registers as part of extended regs.
> Also introduce `PERF_REG_PMU_MASK_31` to define extended mask
> value at runtime for power10
>
> Signed-off-by: Athira Rajeev 
> ---
>  arch/powerpc/include/uapi/asm/perf_regs.h   |  6 ++
>  arch/powerpc/perf/perf_regs.c   | 10 +-
>  arch/powerpc/perf/power10-pmu.c |  6 ++
>  tools/arch/powerpc/include/uapi/asm/perf_regs.h |  6 ++
>  tools/perf/arch/powerpc/include/perf_regs.h |  3 +++
>  tools/perf/arch/powerpc/util/perf_regs.c|  6 ++

Please split into a kernel patch and a tools patch. And cc the tools people.

>  6 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h 
> b/arch/powerpc/include/uapi/asm/perf_regs.h
> index 485b1d5..020b51c 100644
> --- a/arch/powerpc/include/uapi/asm/perf_regs.h
> +++ b/arch/powerpc/include/uapi/asm/perf_regs.h
> @@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
>   PERF_REG_POWERPC_MMCR0,
>   PERF_REG_POWERPC_MMCR1,
>   PERF_REG_POWERPC_MMCR2,
> + PERF_REG_POWERPC_MMCR3,
> + PERF_REG_POWERPC_SIER2,
> + PERF_REG_POWERPC_SIER3,
>   /* Max regs without the extended regs */
>   PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
>  };
> @@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
>  #define PERF_REG_PMU_MASK_300   (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 
> 1) \
>   - PERF_REG_PMU_MASK)
>  
> +/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
> +#define PERF_REG_PMU_MASK_31 (((1ULL << (PERF_REG_POWERPC_SIER3 + 1)) - 1) \
> + - PERF_REG_PMU_MASK)

Wrapping that provides no benefit, just let it be long.

>  #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
> index c8a7e8c..c969935 100644
> --- a/arch/powerpc/perf/perf_regs.c
> +++ b/arch/powerpc/perf/perf_regs.c
> @@ -81,6 +81,12 @@ static u64 get_ext_regs_value(int idx)
>   return mfspr(SPRN_MMCR1);
>   case PERF_REG_POWERPC_MMCR2:
>   return mfspr(SPRN_MMCR2);
> + case PERF_REG_POWERPC_MMCR3:
> + return mfspr(SPRN_MMCR3);
> + case PERF_REG_POWERPC_SIER2:
> + return mfspr(SPRN_SIER2);
> + case PERF_REG_POWERPC_SIER3:
> + return mfspr(SPRN_SIER3);

Indentation is wrong.

>   default: return 0;
>   }
>  }
> @@ -89,7 +95,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  {
>   u64 PERF_REG_EXTENDED_MAX;
>  
> - if (cpu_has_feature(CPU_FTR_ARCH_300))
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_SIER3 + 1;

There's no way to know if that's correct other than going back to the
header to look at the list of values.

So instead you should define it in the header, next to the other values,
with a meaningful name, like PERF_REG_MAX_ISA_31 or something.

> + else if (cpu_has_feature(CPU_FTR_ARCH_300))
>   PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_MMCR2 + 1;

Same.

>   if (idx == PERF_REG_POWERPC_SIER &&
> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
> index 07fb919..51082d6 100644
> --- a/arch/powerpc/perf/power10-pmu.c
> +++ b/arch/powerpc/perf/power10-pmu.c
> @@ -86,6 +86,8 @@
>  #define POWER10_MMCRA_IFM3   0xC000UL
>  #define POWER10_MMCRA_BHRB_MASK  0xC000UL
>  
> +extern u64 mask_var;

Why is it extern? Also not a good name for a global.

Hang on, it's not even used? Is there some macro magic somewhere?

>  /* Table of alternatives, sorted by column 0 */
>  static const unsigned int power10_event_alternatives[][MAX_ALT] = {
>   { PM_RUN_CYC_ALT,   PM_RUN_CYC },
> @@ -397,6 +399,7 @@ static void power10_config_bhrb(u64 pmu_bhrb_filter)
>   .cache_events   = _cache_events,
>   .attr_groups= power10_pmu_attr_groups,
>   .bhrb_nr= 32,
> + .capabilities   = PERF_PMU_CAP_EXTENDED_REGS,
>  };
>  
>  int init_power10_pmu(void)
> @@ -408,6 +411,9 @@ int init_power10_pmu(void)
>   strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
>   return -ENODEV;
>  
> + /* Set the PERF_REG_EXTENDED_MASK here */
> + mask_var = PERF_REG_PMU_MASK_31;
> +
>   rc = register_power_pmu(_pmu);
>   if (rc)
>   return rc;


cheers

Re: [PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform

2020-07-07 Thread Athira Rajeev 



> On 02-Jul-2020, at 3:10 PM, kernel test robot  wrote:
> 
> Hi Athira,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on tip/perf/core v5.8-rc3 next-20200702]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use  as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:
> https://github.com/0day-ci/linux/commits/Athira-Rajeev/powerpc-perf-Add-support-for-power10-PMU-Hardware/20200701-181147
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-pmac32_defconfig (attached as .config)
> compiler: powerpc-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
>chmod +x ~/bin/make.cross
># save the attached .config to linux build tree
>COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross 
> ARCH=powerpc 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
> 
> All errors (new ones prefixed by >>):
> 
>   {standard input}: Assembler messages:
>>> {standard input}:84: Error: unsupported relocation against SPRN_SIER2
>>> {standard input}:91: Error: unsupported relocation against SPRN_SIER3
>>> {standard input}:119: Error: unsupported relocation against SPRN_MMCR3

These regs are not valid for ppc32 platform. Will fix this by including usage 
of these regs under conditional check for
“CONFIG_PPC64” in next version

Thanks
Athira Rajeev
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
> <.config.gz>

Re: [PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform

2020-07-02 Thread kernel test robot 
Hi Athira,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on tip/perf/core v5.8-rc3 next-20200702]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Athira-Rajeev/powerpc-perf-Add-support-for-power10-PMU-Hardware/20200701-181147
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-pmac32_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross 
ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

   {standard input}: Assembler messages:
>> {standard input}:84: Error: unsupported relocation against SPRN_SIER2
>> {standard input}:91: Error: unsupported relocation against SPRN_SIER3
>> {standard input}:119: Error: unsupported relocation against SPRN_MMCR3

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip

[PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform

2020-07-01 Thread Athira Rajeev 
Include capability flag `PERF_PMU_CAP_EXTENDED_REGS` for power10
and expose MMCR3, SIER2, SIER3 registers as part of extended regs.
Also introduce `PERF_REG_PMU_MASK_31` to define extended mask
value at runtime for power10

Signed-off-by: Athira Rajeev 
---
 arch/powerpc/include/uapi/asm/perf_regs.h   |  6 ++
 arch/powerpc/perf/perf_regs.c   | 10 +-
 arch/powerpc/perf/power10-pmu.c |  6 ++
 tools/arch/powerpc/include/uapi/asm/perf_regs.h |  6 ++
 tools/perf/arch/powerpc/include/perf_regs.h |  3 +++
 tools/perf/arch/powerpc/util/perf_regs.c|  6 ++
 6 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h 
b/arch/powerpc/include/uapi/asm/perf_regs.h
index 485b1d5..020b51c 100644
--- a/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
PERF_REG_POWERPC_MMCR0,
PERF_REG_POWERPC_MMCR1,
PERF_REG_POWERPC_MMCR2,
+   PERF_REG_POWERPC_MMCR3,
+   PERF_REG_POWERPC_SIER2,
+   PERF_REG_POWERPC_SIER3,
/* Max regs without the extended regs */
PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
 };
@@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
 #define PERF_REG_PMU_MASK_300   (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
- PERF_REG_PMU_MASK)
 
+/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
+#define PERF_REG_PMU_MASK_31   (((1ULL << (PERF_REG_POWERPC_SIER3 + 1)) - 1) \
+   - PERF_REG_PMU_MASK)
 #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
index c8a7e8c..c969935 100644
--- a/arch/powerpc/perf/perf_regs.c
+++ b/arch/powerpc/perf/perf_regs.c
@@ -81,6 +81,12 @@ static u64 get_ext_regs_value(int idx)
return mfspr(SPRN_MMCR1);
case PERF_REG_POWERPC_MMCR2:
return mfspr(SPRN_MMCR2);
+   case PERF_REG_POWERPC_MMCR3:
+   return mfspr(SPRN_MMCR3);
+   case PERF_REG_POWERPC_SIER2:
+   return mfspr(SPRN_SIER2);
+   case PERF_REG_POWERPC_SIER3:
+   return mfspr(SPRN_SIER3);
default: return 0;
}
 }
@@ -89,7 +95,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 {
u64 PERF_REG_EXTENDED_MAX;
 
-   if (cpu_has_feature(CPU_FTR_ARCH_300))
+   if (cpu_has_feature(CPU_FTR_ARCH_31))
+   PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_SIER3 + 1;
+   else if (cpu_has_feature(CPU_FTR_ARCH_300))
PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_MMCR2 + 1;
 
if (idx == PERF_REG_POWERPC_SIER &&
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index 07fb919..51082d6 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -86,6 +86,8 @@
 #define POWER10_MMCRA_IFM3 0xC000UL
 #define POWER10_MMCRA_BHRB_MASK0xC000UL
 
+extern u64 mask_var;
+
 /* Table of alternatives, sorted by column 0 */
 static const unsigned int power10_event_alternatives[][MAX_ALT] = {
{ PM_RUN_CYC_ALT,   PM_RUN_CYC },
@@ -397,6 +399,7 @@ static void power10_config_bhrb(u64 pmu_bhrb_filter)
.cache_events   = _cache_events,
.attr_groups= power10_pmu_attr_groups,
.bhrb_nr= 32,
+   .capabilities   = PERF_PMU_CAP_EXTENDED_REGS,
 };
 
 int init_power10_pmu(void)
@@ -408,6 +411,9 @@ int init_power10_pmu(void)
strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
return -ENODEV;
 
+   /* Set the PERF_REG_EXTENDED_MASK here */
+   mask_var = PERF_REG_PMU_MASK_31;
+
rc = register_power_pmu(_pmu);
if (rc)
return rc;
diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h 
b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
index 485b1d5..020b51c 100644
--- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
+++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
@@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
PERF_REG_POWERPC_MMCR0,
PERF_REG_POWERPC_MMCR1,
PERF_REG_POWERPC_MMCR2,
+   PERF_REG_POWERPC_MMCR3,
+   PERF_REG_POWERPC_SIER2,
+   PERF_REG_POWERPC_SIER3,
/* Max regs without the extended regs */
PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
 };
@@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
 #define PERF_REG_PMU_MASK_300   (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
- PERF_REG_PMU_MASK)
 
+/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
+#define PERF_REG_PMU_MASK_31   (((1ULL << (PERF_REG_POWERPC_SIER3 + 1)) - 1) \
+   - PERF_REG_PMU_MASK)
 #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
diff --git