[PATCH v3 0/4] perf: ampere: Add support for Ampere SoC PMUs

[PATCH v3 0/4] perf: ampere: Add support for Ampere SoC PMUs

[Ilkka Koskinen]

Changes since v2:
    * Changed to use supports_64bits_atomics() and replaced the split writes
      with lo_hi_writeq()
    * Added implementation specific group validation to patch 3
    * Dropped shared interrupt patch
    * Removed unnecessary filter_enable parameter from ampere module
    * Added group validation to ampere module

Changes since v1:
    * Rather than creating a completely new driver, implemented as a submodule
      of Arm CoreSight PMU driver
    * Fixed shared filter handling


Ilkka Koskinen (4):
  perf: arm_cspmu: Split 64-bit write to 32-bit writes
  perf: arm_cspmu: Support implementation specific filters
  perf: arm_cspmu: Support implementation specific validation
  perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 243 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |  33 ++-
 drivers/perf/arm_cspmu/arm_cspmu.h            |   8 +
 6 files changed, 327 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

-- 
2.40.1

[PATCH v3 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Ilkka Koskinen]

Split the 64-bit register accesses if 64-bit access is not supported
by the PMU.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index a3f1c410b417..f8b4a149eb88 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -702,7 +702,10 @@ static void arm_cspmu_write_counter(struct perf_event *event, u64 val)
 	if (use_64b_counter_reg(cspmu)) {
 		offset = counter_offset(sizeof(u64), event->hw.idx);
 
-		writeq(val, cspmu->base1 + offset);
+		if (supports_64bit_atomics(cspmu))
+			writeq(val, cspmu->base1 + offset);
+		else
+			lo_hi_writeq(val, cspmu->base1 + offset);
 	} else {
 		offset = counter_offset(sizeof(u32), event->hw.idx);
 
-- 
2.40.1

RE: [PATCH v3 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Besar Wicaksono]

> -----Original Message-----
> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Sent: Wednesday, June 7, 2023 3:32 PM
> To: Jonathan Corbet <corbet@lwn.net>; Will Deacon <will@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; Besar Wicaksono
> <bwicaksono@nvidia.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
> Robin Murphy <robin.murphy@arm.com>
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Subject: [PATCH v3 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes
> 
> External email: Use caution opening links or attachments
> 
> 
> Split the 64-bit register accesses if 64-bit access is not supported
> by the PMU.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> index a3f1c410b417..f8b4a149eb88 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -702,7 +702,10 @@ static void arm_cspmu_write_counter(struct
> perf_event *event, u64 val)
>         if (use_64b_counter_reg(cspmu)) {
>                 offset = counter_offset(sizeof(u64), event->hw.idx);
> 
> -               writeq(val, cspmu->base1 + offset);
> +               if (supports_64bit_atomics(cspmu))

Looks good to me, but this function was recently replaced by
arm_cspmu::has_atomic_dword. Please rebase the patch.

Thanks,
Besar

> +                       writeq(val, cspmu->base1 + offset);
> +               else
> +                       lo_hi_writeq(val, cspmu->base1 + offset);
>         } else {
>                 offset = counter_offset(sizeof(u32), event->hw.idx);
> 
> --
> 2.40.1

RE: [PATCH v3 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Ilkka Koskinen]

Hi Besar,

On Tue, 20 Jun 2023, Besar Wicaksono wrote:
>> -----Original Message-----
>> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Sent: Wednesday, June 7, 2023 3:32 PM
>> To: Jonathan Corbet <corbet@lwn.net>; Will Deacon <will@kernel.org>; Mark
>> Rutland <mark.rutland@arm.com>; Besar Wicaksono
>> <bwicaksono@nvidia.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
>> Robin Murphy <robin.murphy@arm.com>
>> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Subject: [PATCH v3 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Split the 64-bit register accesses if 64-bit access is not supported
>> by the PMU.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>  drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index a3f1c410b417..f8b4a149eb88 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>> @@ -702,7 +702,10 @@ static void arm_cspmu_write_counter(struct
>> perf_event *event, u64 val)
>>         if (use_64b_counter_reg(cspmu)) {
>>                 offset = counter_offset(sizeof(u64), event->hw.idx);
>>
>> -               writeq(val, cspmu->base1 + offset);
>> +               if (supports_64bit_atomics(cspmu))
>
> Looks good to me, but this function was recently replaced by
> arm_cspmu::has_atomic_dword. Please rebase the patch.

Sure, I do that.

Cheers, Ilkka

>
> Thanks,
> Besar
>
>> +                       writeq(val, cspmu->base1 + offset);
>> +               else
>> +                       lo_hi_writeq(val, cspmu->base1 + offset);
>>         } else {
>>                 offset = counter_offset(sizeof(u32), event->hw.idx);
>>
>> --
>> 2.40.1
>
>

[PATCH v3 2/4] perf: arm_cspmu: Support implementation specific filters

[Ilkka Koskinen]

Generic filters aren't used in all the platforms. Instead, the platforms
may use different means to filter events. Add support for implementation
specific filters.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
 drivers/perf/arm_cspmu/arm_cspmu.h | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index f8b4a149eb88..72ca4f56347c 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -122,6 +122,9 @@
 
 static unsigned long arm_cspmu_cpuhp_state;
 
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				    struct hw_perf_event *hwc, u32 filter);
+
 /*
  * In CoreSight PMU architecture, all of the MMIO registers are 32-bit except
  * counter register. The counter register can be implemented as 32-bit or 64-bit
@@ -432,6 +435,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
 	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
 	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
+	CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
 
 	return 0;
 }
@@ -798,7 +802,7 @@ static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
 	writel(hwc->config, cspmu->base0 + offset);
 }
 
-static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
 					   struct hw_perf_event *hwc,
 					   u32 filter)
 {
@@ -832,7 +836,7 @@ static void arm_cspmu_start(struct perf_event *event, int pmu_flags)
 		arm_cspmu_set_cc_filter(cspmu, filter);
 	} else {
 		arm_cspmu_set_event(cspmu, hwc);
-		arm_cspmu_set_ev_filter(cspmu, hwc, filter);
+		cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
 	}
 
 	hwc->state = 0;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index 51323b175a4a..f89ae2077164 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -102,6 +102,10 @@ struct arm_cspmu_impl_ops {
 	u32 (*event_type)(const struct perf_event *event);
 	/* Decode filter value from configs */
 	u32 (*event_filter)(const struct perf_event *event);
+	/* Set event filter */
+	void (*set_ev_filter)(struct arm_cspmu *cspmu,
+			      struct hw_perf_event *hwc,
+			      u32 filter);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1

RE: [PATCH v3 2/4] perf: arm_cspmu: Support implementation specific filters

[Besar Wicaksono]

> -----Original Message-----
> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Sent: Wednesday, June 7, 2023 3:32 PM
> To: Jonathan Corbet <corbet@lwn.net>; Will Deacon <will@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; Besar Wicaksono
> <bwicaksono@nvidia.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
> Robin Murphy <robin.murphy@arm.com>
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Subject: [PATCH v3 2/4] perf: arm_cspmu: Support implementation specific
> filters
> 
> External email: Use caution opening links or attachments
> 
> 
> Generic filters aren't used in all the platforms. Instead, the platforms
> may use different means to filter events. Add support for implementation
> specific filters.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>

> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
>  drivers/perf/arm_cspmu/arm_cspmu.h | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> index f8b4a149eb88..72ca4f56347c 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -122,6 +122,9 @@
> 
>  static unsigned long arm_cspmu_cpuhp_state;
> 
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +                                   struct hw_perf_event *hwc, u32 filter);
> +
>  /*
>   * In CoreSight PMU architecture, all of the MMIO registers are 32-bit except
>   * counter register. The counter register can be implemented as 32-bit or 64-
> bit
> @@ -432,6 +435,7 @@ static int arm_cspmu_init_impl_ops(struct
> arm_cspmu *cspmu)
>         CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
>         CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
>         CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
> +       CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
> 
>         return 0;
>  }
> @@ -798,7 +802,7 @@ static inline void arm_cspmu_set_event(struct
> arm_cspmu *cspmu,
>         writel(hwc->config, cspmu->base0 + offset);
>  }
> 
> -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>                                            struct hw_perf_event *hwc,
>                                            u32 filter)
>  {
> @@ -832,7 +836,7 @@ static void arm_cspmu_start(struct perf_event
> *event, int pmu_flags)
>                 arm_cspmu_set_cc_filter(cspmu, filter);
>         } else {
>                 arm_cspmu_set_event(cspmu, hwc);
> -               arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> +               cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
>         }
> 
>         hwc->state = 0;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> index 51323b175a4a..f89ae2077164 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -102,6 +102,10 @@ struct arm_cspmu_impl_ops {
>         u32 (*event_type)(const struct perf_event *event);
>         /* Decode filter value from configs */
>         u32 (*event_filter)(const struct perf_event *event);
> +       /* Set event filter */
> +       void (*set_ev_filter)(struct arm_cspmu *cspmu,
> +                             struct hw_perf_event *hwc,
> +                             u32 filter);
>         /* Hide/show unsupported events */
>         umode_t (*event_attr_is_visible)(struct kobject *kobj,
>                                          struct attribute *attr, int unused);
> --
> 2.40.1

[PATCH v3 3/4] perf: arm_cspmu: Support implementation specific validation

[Ilkka Koskinen]

Some platforms may use e.g. different filtering mechanism and, thus,
may need different way to validate the events and group.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 13 ++++++++++++-
 drivers/perf/arm_cspmu/arm_cspmu.h |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 72ca4f56347c..9021d1878250 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -559,7 +559,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
 static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 				struct perf_event *event)
 {
-	int idx;
+	int idx, ret;
 	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
 
 	if (supports_cycle_counter(cspmu)) {
@@ -593,6 +593,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 	if (idx >= cspmu->num_logical_ctrs)
 		return -EAGAIN;
 
+	if (cspmu->impl.ops.validate_event) {
+		ret = cspmu->impl.ops.validate_event(cspmu, event);
+		if (ret)
+			return ret;
+	}
+
 	set_bit(idx, hw_events->used_ctrs);
 
 	return idx;
@@ -618,6 +624,7 @@ static bool arm_cspmu_validate_event(struct pmu *pmu,
  */
 static bool arm_cspmu_validate_group(struct perf_event *event)
 {
+	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
 	struct perf_event *sibling, *leader = event->group_leader;
 	struct arm_cspmu_hw_events fake_hw_events;
 
@@ -635,6 +642,10 @@ static bool arm_cspmu_validate_group(struct perf_event *event)
 			return false;
 	}
 
+	if (cspmu->impl.ops.validate_group &&
+	    cspmu->impl.ops.validate_group(event))
+		return false;
+
 	return arm_cspmu_validate_event(event->pmu, &fake_hw_events, event);
 }
 
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index f89ae2077164..291cedb196ea 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -106,6 +106,10 @@ struct arm_cspmu_impl_ops {
 	void (*set_ev_filter)(struct arm_cspmu *cspmu,
 			      struct hw_perf_event *hwc,
 			      u32 filter);
+	/* Implementation specific group validation */
+	int (*validate_group)(struct perf_event *event);
+	/* Implementation specific event validation */
+	int (*validate_event)(struct arm_cspmu *cspmu, struct perf_event *new);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1

Re: [PATCH v3 3/4] perf: arm_cspmu: Support implementation specific validation

[Robin Murphy]

On 07/06/2023 9:31 am, Ilkka Koskinen wrote:
> Some platforms may use e.g. different filtering mechanism and, thus,
> may need different way to validate the events and group.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>   drivers/perf/arm_cspmu/arm_cspmu.c | 13 ++++++++++++-
>   drivers/perf/arm_cspmu/arm_cspmu.h |  4 ++++
>   2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 72ca4f56347c..9021d1878250 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -559,7 +559,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
>   static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>   				struct perf_event *event)
>   {
> -	int idx;
> +	int idx, ret;
>   	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>   
>   	if (supports_cycle_counter(cspmu)) {
> @@ -593,6 +593,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>   	if (idx >= cspmu->num_logical_ctrs)
>   		return -EAGAIN;
>   
> +	if (cspmu->impl.ops.validate_event) {
> +		ret = cspmu->impl.ops.validate_event(cspmu, event);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	set_bit(idx, hw_events->used_ctrs);
>   
>   	return idx;
> @@ -618,6 +624,7 @@ static bool arm_cspmu_validate_event(struct pmu *pmu,
>    */
>   static bool arm_cspmu_validate_group(struct perf_event *event)
>   {
> +	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>   	struct perf_event *sibling, *leader = event->group_leader;
>   	struct arm_cspmu_hw_events fake_hw_events;
>   
> @@ -635,6 +642,10 @@ static bool arm_cspmu_validate_group(struct perf_event *event)
>   			return false;
>   	}
>   
> +	if (cspmu->impl.ops.validate_group &&
> +	    cspmu->impl.ops.validate_group(event))
> +		return false;

Hmm, this means that any driver wanting to use it has to duplicate all 
the group iteration logic, which isn't ideal. More than that, though, 
the way you've implemented it in patch #4 I'm not sure even does 
anything, since it only appears to be repeating the same checks that 
already happen in this path:

   arm_csmpu_validate_group()
     arm_cspmu_validate_event()
       arm_cspmu_get_event_idx()
         ops.validate_event() -> ampere_cspmu_validate_params()

so there's no need for the ops.validate_group hook to just call 
ampere_cspmu_validate_params() a second time when it's guaranteed to 
succeed (because otherwise we'd have bailed out already).

I think what we want overall is an "is this event config valid at all" 
hook from arm_cspmu_event_init() (which we don't really need to 
implement yet unless you want to start sanity-checking your actual 
rank/bank/threshold values), plus an "is this event schedulable in the 
given PMU context" hook from arm_cspmu_get_event_idx(), which should 
serve for both group validation via the fake context in event_init and 
actual scheduling in the real context in add.

Thanks,
Robin.

> +
>   	return arm_cspmu_validate_event(event->pmu, &fake_hw_events, event);
>   }
>   
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index f89ae2077164..291cedb196ea 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -106,6 +106,10 @@ struct arm_cspmu_impl_ops {
>   	void (*set_ev_filter)(struct arm_cspmu *cspmu,
>   			      struct hw_perf_event *hwc,
>   			      u32 filter);
> +	/* Implementation specific group validation */
> +	int (*validate_group)(struct perf_event *event);
> +	/* Implementation specific event validation */
> +	int (*validate_event)(struct arm_cspmu *cspmu, struct perf_event *new);
>   	/* Hide/show unsupported events */
>   	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>   					 struct attribute *attr, int unused);

Re: [PATCH v3 3/4] perf: arm_cspmu: Support implementation specific validation

[Ilkka Koskinen]

Hi Robin,

On Tue, 20 Jun 2023, Robin Murphy wrote:
> On 07/06/2023 9:31 am, Ilkka Koskinen wrote:
>> Some platforms may use e.g. different filtering mechanism and, thus,
>> may need different way to validate the events and group.
>> 
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>   drivers/perf/arm_cspmu/arm_cspmu.c | 13 ++++++++++++-
>>   drivers/perf/arm_cspmu/arm_cspmu.h |  4 ++++
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c 
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index 72ca4f56347c..9021d1878250 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>> @@ -559,7 +559,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
>>   static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>>   				struct perf_event *event)
>>   {
>> -	int idx;
>> +	int idx, ret;
>>   	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>>     	if (supports_cycle_counter(cspmu)) {
>> @@ -593,6 +593,12 @@ static int arm_cspmu_get_event_idx(struct 
>> arm_cspmu_hw_events *hw_events,
>>   	if (idx >= cspmu->num_logical_ctrs)
>>   		return -EAGAIN;
>>   +	if (cspmu->impl.ops.validate_event) {
>> +		ret = cspmu->impl.ops.validate_event(cspmu, event);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	set_bit(idx, hw_events->used_ctrs);
>>     	return idx;
>> @@ -618,6 +624,7 @@ static bool arm_cspmu_validate_event(struct pmu *pmu,
>>    */
>>   static bool arm_cspmu_validate_group(struct perf_event *event)
>>   {
>> +	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>>   	struct perf_event *sibling, *leader = event->group_leader;
>>   	struct arm_cspmu_hw_events fake_hw_events;
>>   @@ -635,6 +642,10 @@ static bool arm_cspmu_validate_group(struct 
>> perf_event *event)
>>   			return false;
>>   	}
>>   +	if (cspmu->impl.ops.validate_group &&
>> +	    cspmu->impl.ops.validate_group(event))
>> +		return false;
>
> Hmm, this means that any driver wanting to use it has to duplicate all the 
> group iteration logic, which isn't ideal. More than that, though, the way 
> you've implemented it in patch #4 I'm not sure even does anything, since it 
> only appears to be repeating the same checks that already happen in this 
> path:
>
>  arm_csmpu_validate_group()
>    arm_cspmu_validate_event()
>      arm_cspmu_get_event_idx()
>        ops.validate_event() -> ampere_cspmu_validate_params()
>
> so there's no need for the ops.validate_group hook to just call 
> ampere_cspmu_validate_params() a second time when it's guaranteed to succeed 
> (because otherwise we'd have bailed out already).

Yeah, I took another look how the framework really does it and you're 
absolutely correct, it's totally unnecessary.

>
> I think what we want overall is an "is this event config valid at all" hook 
> from arm_cspmu_event_init() (which we don't really need to implement yet 
> unless you want to start sanity-checking your actual rank/bank/threshold 
> values), plus an "is this event schedulable in the given PMU context" hook 
> from arm_cspmu_get_event_idx(), which should serve for both group validation 
> via the fake context in event_init and actual scheduling in the real context 
> in add.

Ah, that's true. I can already verify the group event has the same 
rank/bank/threshold settings as the group leader in ops.validate_event(). 
Thus, one hook seems enough.

I fix and rebase the patchset.

Cheers, Ilkka


> Thanks,
> Robin.
>
>> +
>>   	return arm_cspmu_validate_event(event->pmu, &fake_hw_events, event);
>>   }
>>   diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h 
>> b/drivers/perf/arm_cspmu/arm_cspmu.h
>> index f89ae2077164..291cedb196ea 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
>> @@ -106,6 +106,10 @@ struct arm_cspmu_impl_ops {
>>   	void (*set_ev_filter)(struct arm_cspmu *cspmu,
>>   			      struct hw_perf_event *hwc,
>>   			      u32 filter);
>> +	/* Implementation specific group validation */
>> +	int (*validate_group)(struct perf_event *event);
>> +	/* Implementation specific event validation */
>> +	int (*validate_event)(struct arm_cspmu *cspmu, struct perf_event 
>> *new);
>>   	/* Hide/show unsupported events */
>>   	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>>   					 struct attribute *attr, int unused);
>

[PATCH v3 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Ilkka Koskinen]

Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
specific registers to filter events rather than PMEVFILTnR registers.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 243 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
 5 files changed, 297 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
new file mode 100644
index 000000000000..94f93f5aee6c
--- /dev/null
+++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
@@ -0,0 +1,29 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============================================
+Ampere SoC Performance Monitoring Unit (PMU)
+============================================
+
+Ampere SoC PMU is a generic PMU IP that follows Arm CoreSight PMU architecture.
+Therefore, the driver is implemented as a submodule of arm_cspmu driver. At the
+first phase it's used for counting MCU events on AmpereOne.
+
+
+MCU PMU events
+--------------
+
+The PMU driver supports setting filters for "rank", "bank", and "threshold".
+Note, that the filters are per PMU instance rather than per event.
+
+
+Example for perf tool use::
+
+  / # perf list ampere
+
+    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
+    <...>
+    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
+    <...>
+
+  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
+        sleep 1
diff --git a/drivers/perf/arm_cspmu/Makefile b/drivers/perf/arm_cspmu/Makefile
index fedb17df982d..b80a8bd8da54 100644
--- a/drivers/perf/arm_cspmu/Makefile
+++ b/drivers/perf/arm_cspmu/Makefile
@@ -3,4 +3,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu_module.o
-arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o
+arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o ampere_cspmu.o
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.c b/drivers/perf/arm_cspmu/ampere_cspmu.c
new file mode 100644
index 000000000000..1c57feb4b6ce
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#include "ampere_cspmu.h"
+
+#define PMAUXR0		0xD80
+#define PMAUXR1		0xD84
+#define PMAUXR2		0xD88
+#define PMAUXR3		0xD8C
+
+#define to_ampere_cspmu_ctx(cspmu)	((struct ampere_cspmu_ctx *)(cspmu->impl.ctx))
+
+struct ampere_cspmu_ctx {
+	const char *name;
+	struct attribute **event_attr;
+	struct attribute **format_attr;
+};
+
+#define SOC_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
+	static inline u32 get_##_name(const struct perf_event *event)     \
+	{                                                                 \
+		return FIELD_GET(GENMASK_ULL(_end, _start),               \
+				 event->attr._config);                    \
+	}                                                                 \
+
+SOC_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 8);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(threshold, config1, 0, 7);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(rank, config1, 8, 23);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(bank, config2, 0, 31);
+
+static struct attribute *ampereone_mcu_pmu_event_attrs[] = {
+	ARM_CSPMU_EVENT_ATTR(cycle_count,		0x00),
+	ARM_CSPMU_EVENT_ATTR(act_sent,			0x01),
+	ARM_CSPMU_EVENT_ATTR(pre_sent,			0x02),
+	ARM_CSPMU_EVENT_ATTR(rd_sent,			0x03),
+	ARM_CSPMU_EVENT_ATTR(rda_sent,			0x04),
+	ARM_CSPMU_EVENT_ATTR(wr_sent,			0x05),
+	ARM_CSPMU_EVENT_ATTR(wra_sent,			0x06),
+	ARM_CSPMU_EVENT_ATTR(pd_entry_vld,		0x07),
+	ARM_CSPMU_EVENT_ATTR(sref_entry_vld,		0x08),
+	ARM_CSPMU_EVENT_ATTR(prea_sent,			0x09),
+	ARM_CSPMU_EVENT_ATTR(pre_sb_sent,		0x0a),
+	ARM_CSPMU_EVENT_ATTR(ref_sent,			0x0b),
+	ARM_CSPMU_EVENT_ATTR(rfm_sent,			0x0c),
+	ARM_CSPMU_EVENT_ATTR(ref_sb_sent,		0x0d),
+	ARM_CSPMU_EVENT_ATTR(rfm_sb_sent,		0x0e),
+	ARM_CSPMU_EVENT_ATTR(rd_rda_sent,		0x0f),
+	ARM_CSPMU_EVENT_ATTR(wr_wra_sent,		0x10),
+	ARM_CSPMU_EVENT_ATTR(raw_hazard,		0x11),
+	ARM_CSPMU_EVENT_ATTR(war_hazard,		0x12),
+	ARM_CSPMU_EVENT_ATTR(waw_hazard,		0x13),
+	ARM_CSPMU_EVENT_ATTR(rar_hazard,		0x14),
+	ARM_CSPMU_EVENT_ATTR(raw_war_waw_hazard,	0x15),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_wr_req_vld,	0x16),
+	ARM_CSPMU_EVENT_ATTR(lprd_req_vld,		0x17),
+	ARM_CSPMU_EVENT_ATTR(hprd_req_vld,		0x18),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_req_vld,		0x19),
+	ARM_CSPMU_EVENT_ATTR(prefetch_tgt,		0x1a),
+	ARM_CSPMU_EVENT_ATTR(wr_req_vld,		0x1b),
+	ARM_CSPMU_EVENT_ATTR(partial_wr_req_vld,	0x1c),
+	ARM_CSPMU_EVENT_ATTR(rd_retry,			0x1d),
+	ARM_CSPMU_EVENT_ATTR(wr_retry,			0x1e),
+	ARM_CSPMU_EVENT_ATTR(retry_gnt,			0x1f),
+	ARM_CSPMU_EVENT_ATTR(rank_change,		0x20),
+	ARM_CSPMU_EVENT_ATTR(dir_change,		0x21),
+	ARM_CSPMU_EVENT_ATTR(rank_dir_change,		0x22),
+	ARM_CSPMU_EVENT_ATTR(rank_active,		0x23),
+	ARM_CSPMU_EVENT_ATTR(rank_idle,			0x24),
+	ARM_CSPMU_EVENT_ATTR(rank_pd,			0x25),
+	ARM_CSPMU_EVENT_ATTR(rank_sref,			0x26),
+	ARM_CSPMU_EVENT_ATTR(queue_fill_gt_thresh,	0x27),
+	ARM_CSPMU_EVENT_ATTR(queue_rds_gt_thresh,	0x28),
+	ARM_CSPMU_EVENT_ATTR(queue_wrs_gt_thresh,	0x29),
+	ARM_CSPMU_EVENT_ATTR(phy_updt_complt,		0x2a),
+	ARM_CSPMU_EVENT_ATTR(tz_fail,			0x2b),
+	ARM_CSPMU_EVENT_ATTR(dram_errc,			0x2c),
+	ARM_CSPMU_EVENT_ATTR(dram_errd,			0x2d),
+	ARM_CSPMU_EVENT_ATTR(read_data_return,		0x32),
+	ARM_CSPMU_EVENT_ATTR(chi_wr_data_delta,		0x33),
+	ARM_CSPMU_EVENT_ATTR(zq_start,			0x34),
+	ARM_CSPMU_EVENT_ATTR(zq_latch,			0x35),
+	ARM_CSPMU_EVENT_ATTR(wr_fifo_full,		0x36),
+	ARM_CSPMU_EVENT_ATTR(info_fifo_full,		0x37),
+	ARM_CSPMU_EVENT_ATTR(cmd_fifo_full,		0x38),
+	ARM_CSPMU_EVENT_ATTR(dfi_nop,			0x39),
+	ARM_CSPMU_EVENT_ATTR(dfi_cmd,			0x3a),
+	ARM_CSPMU_EVENT_ATTR(rd_run_len,		0x3b),
+	ARM_CSPMU_EVENT_ATTR(wr_run_len,		0x3c),
+
+	ARM_CSPMU_EVENT_ATTR(cycles, ARM_CSPMU_EVT_CYCLES_DEFAULT),
+	NULL,
+};
+
+static struct attribute *ampereone_mcu_format_attrs[] = {
+	ARM_CSPMU_FORMAT_EVENT_ATTR,
+	ARM_CSPMU_FORMAT_ATTR(threshold, "config1:0-7"),
+	ARM_CSPMU_FORMAT_ATTR(rank, "config1:8-23"),
+	ARM_CSPMU_FORMAT_ATTR(bank, "config2:0-31"),
+	NULL,
+};
+
+static struct attribute **
+ampere_cspmu_get_event_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->event_attr;
+}
+
+static struct attribute **
+ampere_cspmu_get_format_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->format_attr;
+}
+
+static const char *
+ampere_cspmu_get_name(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->name;
+}
+
+static u32 ampere_cspmu_event_filter(const struct perf_event *event)
+{
+	return 0;
+}
+
+static void ampere_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				       struct hw_perf_event *hwc,
+				       u32 filter)
+{
+	struct perf_event *event;
+	unsigned int idx;
+	u32 threshold, rank, bank;
+
+	/*
+	 * At this point, all the events have the same filter settings.
+	 * Therefore, take the first event and use its configuration.
+	 */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	event = cspmu->hw_events.events[idx];
+
+	threshold	= get_threshold(event);
+	rank		= get_rank(event);
+	bank		= get_bank(event);
+
+	writel(threshold, cspmu->base0 + PMAUXR0);
+	writel(rank, cspmu->base0 + PMAUXR1);
+	writel(bank, cspmu->base0 + PMAUXR2);
+}
+
+static int ampere_cspmu_validate_params(struct perf_event *event, struct perf_event *event2)
+{
+	if (get_threshold(event) != get_threshold(event2) ||
+	    get_rank(event) != get_rank(event2) ||
+	    get_bank(event) != get_bank(event2))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ampere_cspmu_validate_group(struct perf_event *event)
+{
+	struct perf_event *sibling, *leader = event->group_leader;
+	int ret;
+
+	ret = ampere_cspmu_validate_params(event, leader);
+	if (ret)
+		return ret;
+
+	for_each_sibling_event(sibling, leader) {
+		ret = ampere_cspmu_validate_params(sibling, leader);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
+				       struct perf_event *new)
+{
+	struct perf_event *curr;
+	unsigned int idx;
+
+	/* We compare the global filter settings to existing events */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	/* This is the first event */
+	if (idx == cspmu->cycle_counter_logical_idx)
+		return 0;
+
+	curr = cspmu->hw_events.events[idx];
+
+	return ampere_cspmu_validate_params(curr, new);
+}
+
+static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
+				      const char *name_pattern)
+{
+	struct device *dev = cspmu->dev;
+	static atomic_t pmu_generic_idx = {0};
+
+	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
+			      atomic_fetch_inc(&pmu_generic_idx));
+}
+
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
+{
+	struct device *dev = cspmu->dev;
+	struct ampere_cspmu_ctx *ctx;
+	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+
+	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
+	ctx->format_attr	= ampereone_mcu_format_attrs;
+	ctx->name		= ampere_cspmu_format_name(cspmu,
+							   "ampere_mcu_pmu_%u");
+	cspmu->impl.ctx = ctx;
+
+	impl_ops->event_filter		= ampere_cspmu_event_filter;
+	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
+	impl_ops->validate_group	= ampere_cspmu_validate_group;
+	impl_ops->validate_event	= ampere_cspmu_validate_event;
+	impl_ops->get_name		= ampere_cspmu_get_name;
+	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
+	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
+
+	return 0;
+}
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.h b/drivers/perf/arm_cspmu/ampere_cspmu.h
new file mode 100644
index 000000000000..9b3e1628d1d6
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#ifndef __AMPERE_CSPMU_H__
+#define __AMPERE_CSPMU_H__
+
+#include "arm_cspmu.h"
+
+/* Allocate AMPERE descriptor. */
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu);
+
+#endif /* __AMPERE_CSPMU_H__ */
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 9021d1878250..885393ed993e 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -30,6 +30,7 @@
 #include <linux/platform_device.h>
 #include <acpi/processor.h>
 
+#include "ampere_cspmu.h"
 #include "arm_cspmu.h"
 #include "nvidia_cspmu.h"
 
@@ -119,6 +120,7 @@
 
 /* JEDEC-assigned JEP106 identification code */
 #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
+#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
 
 static unsigned long arm_cspmu_cpuhp_state;
 
@@ -394,6 +396,11 @@ static const struct impl_match impl_match[] = {
 	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
 	  .impl_init_ops = nv_cspmu_init_ops
 	},
+	{
+	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
+	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
+	  .impl_init_ops = ampere_cspmu_init_ops
+	},
 	{}
 };
 
-- 
2.40.1