Re: [PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs

[Athira Rajeev]

> On 13-Jul-2020, at 6:20 PM, Michael Ellerman  wrote:
> 
> Athira Rajeev  > writes:
>>> On 08-Jul-2020, at 4:32 PM, Michael Ellerman  wrote:
>>> 
>>> Athira Rajeev  writes:
>>> ...
 diff --git a/arch/powerpc/perf/core-book3s.c 
 b/arch/powerpc/perf/core-book3s.c
 index cd6a742..5c64bd3 100644
 --- a/arch/powerpc/perf/core-book3s.c
 +++ b/arch/powerpc/perf/core-book3s.c
 @@ -39,10 +39,10 @@ struct cpu_hw_events {
unsigned int flags[MAX_HWEVENTS];
/*
 * The order of the MMCR array is:
 -   *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
 +   *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
 *  - 32-bit, MMCR0, MMCR1, MMCR2
 */
 -  unsigned long mmcr[4];
 +  unsigned long mmcr[5];
struct perf_event *limited_counter[MAX_LIMITED_HWCOUNTERS];
u8  limited_hwidx[MAX_LIMITED_HWCOUNTERS];
u64 alternatives[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES];
>>> ...
 @@ -1310,6 +1326,10 @@ static void power_pmu_enable(struct pmu *pmu)
if (!cpuhw->n_added) {
mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
 +#ifdef CONFIG_PPC64
 +  if (ppmu->flags & PPMU_ARCH_310S)
 +  mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
 +#endif /* CONFIG_PPC64 */
goto out_enable;
}
 
 @@ -1353,6 +1373,11 @@ static void power_pmu_enable(struct pmu *pmu)
if (ppmu->flags & PPMU_ARCH_207S)
mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
 
 +#ifdef CONFIG_PPC64
 +  if (ppmu->flags & PPMU_ARCH_310S)
 +  mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
 +#endif /* CONFIG_PPC64 */
>>> 
>>> I don't think you need the #ifdef CONFIG_PPC64?
>> 
>> Hi Michael
>> 
>> Thanks for reviewing this series.
>> 
>> SPRN_MMCR3 is not defined for PPC32 and we hit build failure for 
>> pmac32_defconfig.
>> The #ifdef CONFIG_PPC64 is to address this.
> 
> We like to avoid #ifdefs in the body of the code like that.
> 
> There's a bunch of existing #defines near the top of the file to make
> 32-bit work, I think you should just add another for this, so eg:
> 
> #ifdef CONFIG_PPC32
> ...
> #define SPRN_MMCR30

Ok Ok. Found that currently we do same way as you mentioned for MMCRA which is 
not supported for 32-bit
I will work on this change

Thanks
Athira 
> 
> cheers

Re: [PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs

[Michael Ellerman]

Athira Rajeev  writes:
>> On 08-Jul-2020, at 4:32 PM, Michael Ellerman  wrote:
>> 
>> Athira Rajeev  writes:
>> ...
>>> diff --git a/arch/powerpc/perf/core-book3s.c 
>>> b/arch/powerpc/perf/core-book3s.c
>>> index cd6a742..5c64bd3 100644
>>> --- a/arch/powerpc/perf/core-book3s.c
>>> +++ b/arch/powerpc/perf/core-book3s.c
>>> @@ -39,10 +39,10 @@ struct cpu_hw_events {
>>> unsigned int flags[MAX_HWEVENTS];
>>> /*
>>>  * The order of the MMCR array is:
>>> -*  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
>>> +*  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
>>>  *  - 32-bit, MMCR0, MMCR1, MMCR2
>>>  */
>>> -   unsigned long mmcr[4];
>>> +   unsigned long mmcr[5];
>>> struct perf_event *limited_counter[MAX_LIMITED_HWCOUNTERS];
>>> u8  limited_hwidx[MAX_LIMITED_HWCOUNTERS];
>>> u64 alternatives[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES];
>> ...
>>> @@ -1310,6 +1326,10 @@ static void power_pmu_enable(struct pmu *pmu)
>>> if (!cpuhw->n_added) {
>>> mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
>>> mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
>>> +#ifdef CONFIG_PPC64
>>> +   if (ppmu->flags & PPMU_ARCH_310S)
>>> +   mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
>>> +#endif /* CONFIG_PPC64 */
>>> goto out_enable;
>>> }
>>> 
>>> @@ -1353,6 +1373,11 @@ static void power_pmu_enable(struct pmu *pmu)
>>> if (ppmu->flags & PPMU_ARCH_207S)
>>> mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
>>> 
>>> +#ifdef CONFIG_PPC64
>>> +   if (ppmu->flags & PPMU_ARCH_310S)
>>> +   mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
>>> +#endif /* CONFIG_PPC64 */
>> 
>> I don't think you need the #ifdef CONFIG_PPC64?
>
> Hi Michael
>
> Thanks for reviewing this series.
>
> SPRN_MMCR3 is not defined for PPC32 and we hit build failure for 
> pmac32_defconfig.
> The #ifdef CONFIG_PPC64 is to address this.

We like to avoid #ifdefs in the body of the code like that.

There's a bunch of existing #defines near the top of the file to make
32-bit work, I think you should just add another for this, so eg:

#ifdef CONFIG_PPC32
...
#define SPRN_MMCR3  0

cheers

Re: [PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs

[Athira Rajeev]

> On 08-Jul-2020, at 4:32 PM, Michael Ellerman  wrote:
> 
> Athira Rajeev  writes:
> ...
>> diff --git a/arch/powerpc/perf/core-book3s.c 
>> b/arch/powerpc/perf/core-book3s.c
>> index cd6a742..5c64bd3 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -39,10 +39,10 @@ struct cpu_hw_events {
>>  unsigned int flags[MAX_HWEVENTS];
>>  /*
>>   * The order of the MMCR array is:
>> - *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
>> + *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
>>   *  - 32-bit, MMCR0, MMCR1, MMCR2
>>   */
>> -unsigned long mmcr[4];
>> +unsigned long mmcr[5];
>>  struct perf_event *limited_counter[MAX_LIMITED_HWCOUNTERS];
>>  u8  limited_hwidx[MAX_LIMITED_HWCOUNTERS];
>>  u64 alternatives[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES];
> ...
>> @@ -1310,6 +1326,10 @@ static void power_pmu_enable(struct pmu *pmu)
>>  if (!cpuhw->n_added) {
>>  mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
>>  mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
>> +#ifdef CONFIG_PPC64
>> +if (ppmu->flags & PPMU_ARCH_310S)
>> +mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
>> +#endif /* CONFIG_PPC64 */
>>  goto out_enable;
>>  }
>> 
>> @@ -1353,6 +1373,11 @@ static void power_pmu_enable(struct pmu *pmu)
>>  if (ppmu->flags & PPMU_ARCH_207S)
>>  mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
>> 
>> +#ifdef CONFIG_PPC64
>> +if (ppmu->flags & PPMU_ARCH_310S)
>> +mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
>> +#endif /* CONFIG_PPC64 */
> 
> I don't think you need the #ifdef CONFIG_PPC64?

Hi Michael

Thanks for reviewing this series.

SPRN_MMCR3 is not defined for PPC32 and we hit build failure for 
pmac32_defconfig.
The #ifdef CONFIG_PPC64 is to address this.

Thanks
Athira


> 
> cheers

Re: [PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs

[Michael Ellerman]

Athira Rajeev  writes:
...
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index cd6a742..5c64bd3 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -39,10 +39,10 @@ struct cpu_hw_events {
>   unsigned int flags[MAX_HWEVENTS];
>   /*
>* The order of the MMCR array is:
> -  *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
> +  *  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
>*  - 32-bit, MMCR0, MMCR1, MMCR2
>*/
> - unsigned long mmcr[4];
> + unsigned long mmcr[5];
>   struct perf_event *limited_counter[MAX_LIMITED_HWCOUNTERS];
>   u8  limited_hwidx[MAX_LIMITED_HWCOUNTERS];
>   u64 alternatives[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES];
...
> @@ -1310,6 +1326,10 @@ static void power_pmu_enable(struct pmu *pmu)
>   if (!cpuhw->n_added) {
>   mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
>   mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
> +#ifdef CONFIG_PPC64
> + if (ppmu->flags & PPMU_ARCH_310S)
> + mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
> +#endif /* CONFIG_PPC64 */
>   goto out_enable;
>   }
>  
> @@ -1353,6 +1373,11 @@ static void power_pmu_enable(struct pmu *pmu)
>   if (ppmu->flags & PPMU_ARCH_207S)
>   mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
>  
> +#ifdef CONFIG_PPC64
> + if (ppmu->flags & PPMU_ARCH_310S)
> + mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
> +#endif /* CONFIG_PPC64 */

I don't think you need the #ifdef CONFIG_PPC64?

cheers

[PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs

[Athira Rajeev]

From: Madhavan Srinivasan 

PowerISA v3.1 includes new performance monitoring unit(PMU)
special purpose registers (SPRs). They are

Monitor Mode Control Register 3 (MMCR3)
Sampled Instruction Event Register 2 (SIER2)
Sampled Instruction Event Register 3 (SIER3)

MMCR3 is added for further sampling related configuration
control. SIER2/SIER3 are added to provide additional
information about the sampled instruction.

Patch adds new PPMU flag called "PPMU_ARCH_310S" to support
handling of these new SPRs, updates the struct thread_struct
to include these new SPRs, increase the size of mmcr[] array
by one to include MMCR3 in struct cpu_hw_event. This is needed
to support programming of MMCR3 SPR during event_[enable/disable].
Patch also adds the sysfs support for the MMCR3 SPR along with
SPRN_ macros for these new pmu sprs.

Signed-off-by: Madhavan Srinivasan 
---
 arch/powerpc/include/asm/perf_event_server.h |  1 +
 arch/powerpc/include/asm/processor.h |  4 
 arch/powerpc/include/asm/reg.h   |  6 ++
 arch/powerpc/kernel/sysfs.c  |  8 
 arch/powerpc/perf/core-book3s.c  | 29 ++--
 5 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/perf_event_server.h 
b/arch/powerpc/include/asm/perf_event_server.h
index 3e9703f..895aeaa 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -69,6 +69,7 @@ struct power_pmu {
 #define PPMU_HAS_SIER  0x0040 /* Has SIER */
 #define PPMU_ARCH_207S 0x0080 /* PMC is architecture v2.07S */
 #define PPMU_NO_SIAR   0x0100 /* Do not use SIAR */
+#define PPMU_ARCH_310S 0x0200 /* Has MMCR3, SIER2 and SIER3 */
 
 /*
  * Values for flags to get_alternatives()
diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index 52a6783..a466e94 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -272,6 +272,10 @@ struct thread_struct {
unsignedmmcr0;
 
unsignedused_ebb;
+   unsigned long   mmcr3;
+   unsigned long   sier2;
+   unsigned long   sier3;
+
 #endif
 };
 
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 88e6c78..21a1b2d 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -876,7 +876,9 @@
 #define   MMCR0_FCHV   0x0001UL /* freeze conditions in hypervisor mode */
 #define SPRN_MMCR1 798
 #define SPRN_MMCR2 785
+#define SPRN_MMCR3 754
 #define SPRN_UMMCR2769
+#define SPRN_UMMCR3738
 #define SPRN_MMCRA 0x312
 #define   MMCRA_SDSYNC 0x8000UL /* SDAR synced with SIAR */
 #define   MMCRA_SDAR_DCACHE_MISS 0x4000UL
@@ -918,6 +920,10 @@
 #define   SIER_SIHV0x100   /* Sampled MSR_HV */
 #define   SIER_SIAR_VALID  0x040   /* SIAR contents valid */
 #define   SIER_SDAR_VALID  0x020   /* SDAR contents valid */
+#define SPRN_SIER2 752
+#define SPRN_SIER3 753
+#define SPRN_USIER2736
+#define SPRN_USIER3737
 #define SPRN_SIAR  796
 #define SPRN_SDAR  797
 #define SPRN_TACR  888
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 571b325..46b4ebc 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -622,8 +622,10 @@ void ppc_enable_pmcs(void)
 SYSFS_PMCSETUP(pmc8, SPRN_PMC8);
 
 SYSFS_PMCSETUP(mmcra, SPRN_MMCRA);
+SYSFS_PMCSETUP(mmcr3, SPRN_MMCR3);
 
 static DEVICE_ATTR(mmcra, 0600, show_mmcra, store_mmcra);
+static DEVICE_ATTR(mmcr3, 0600, show_mmcr3, store_mmcr3);
 #endif /* HAS_PPC_PMC56 */
 
 
@@ -886,6 +888,9 @@ static int register_cpu_online(unsigned int cpu)
 #ifdef CONFIG_PMU_SYSFS
if (cpu_has_feature(CPU_FTR_MMCRA))
device_create_file(s, _attr_mmcra);
+
+   if (cpu_has_feature(CPU_FTR_ARCH_31))
+   device_create_file(s, _attr_mmcr3);
 #endif /* CONFIG_PMU_SYSFS */
 
if (cpu_has_feature(CPU_FTR_PURR)) {
@@ -980,6 +985,9 @@ static int unregister_cpu_online(unsigned int cpu)
 #ifdef CONFIG_PMU_SYSFS
if (cpu_has_feature(CPU_FTR_MMCRA))
device_remove_file(s, _attr_mmcra);
+
+   if (cpu_has_feature(CPU_FTR_ARCH_31))
+   device_remove_file(s, _attr_mmcr3);
 #endif /* CONFIG_PMU_SYSFS */
 
if (cpu_has_feature(CPU_FTR_PURR)) {
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index cd6a742..5c64bd3 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -39,10 +39,10 @@ struct cpu_hw_events {
unsigned int flags[MAX_HWEVENTS];
/*
 * The order of the MMCR array is:
-*  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
+*  - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
 *  - 32-bit, MMCR0, MMCR1, MMCR2
 */
-   unsigned long mmcr[4];
+   unsigned