RE: [PATCH v2 3/4] perf: add arm64 smmuv3 pmu driver
From: Shameerali Kolothum Thodi
Date: Mon Sep 10 2018 - 12:37:47 EST
Hi Robin,
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: 10 September 2018 12:02
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
> lorenzo.pieralisi@xxxxxxx
> Cc: will.deacon@xxxxxxx; mark.rutland@xxxxxxx; Guohanjun (Hanjun Guo)
> <guohanjun@xxxxxxxxxx>; John Garry <john.garry@xxxxxxxxxx>;
> pabba@xxxxxxxxxxxxxx; vkilari@xxxxxxxxxxxxxx; rruigrok@xxxxxxxxxxxxxx;
> linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>;
> neil.m.leeder@xxxxxxxxx
> Subject: Re: [PATCH v2 3/4] perf: add arm64 smmuv3 pmu driver
>
> [ note: for some reason I decided to review this from the bottom up,
> so it probably makes no sense unless you read it backwards ]
>
> On 2018-07-24 12:45 PM, Shameer Kolothum wrote:
> > From: Neil Leeder <nleeder@xxxxxxxxxxxxxx>
> >
> > Adds a new driver to support the SMMU v3 PMU and add it into the perf
> > events framework.
> >
> > Each SMMU node may have multiple PMUs associated with it, each of
> > which may support different events.
> >
> > SMMUv3 PMCG devices are named as arm_smmu_v3_x_pmcg_y where x
> denotes
> > the associated smmuv3 dev id(if any) and y denotes the pmu dev id.
> >
> > Filtering by stream id is done by specifying filtering parameters with
> > the event. options are:
> > filter_enable - 0 = no filtering, 1 = filtering enabled
> > filter_span - 0 = exact match, 1 = pattern match
> > filter_stream_id - pattern to filter against Further filtering
> > information is available in the SMMU documentation.
> >
> > Example: perf stat -e arm_smmu_v3_0_pmcg_6/transaction,filter_enable=1,
> > filter_span=1,filter_stream_id=0x42/ -a pwd
> > Applies filter pattern 0x42 to transaction events.
> >
> > SMMU events are not attributable to a CPU, so task mode and sampling
> > are not supported.
> >
> > Signed-off-by: Neil Leeder <nleeder@xxxxxxxxxxxxxx>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> > ---
> > drivers/perf/Kconfig | 9 +
> > drivers/perf/Makefile | 1 +
> > drivers/perf/arm_smmuv3_pmu.c | 838
> ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 848 insertions(+)
> > create mode 100644 drivers/perf/arm_smmuv3_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig index
> > 08ebaf7..0b9cc1a 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -52,6 +52,15 @@ config ARM_PMU_ACPI
> > depends on ARM_PMU && ACPI
> > def_bool y
> >
> > +config ARM_SMMUV3_PMU
> > + bool "ARM SMMUv3 PMU"
>
> Nit: I'd be inlined to use "Performance Monitors {Extension}" or "PMCG"
> in user-facing text, since "PMU" is not the architectural terminology in this
> particular case.
Ok.
> > + depends on ARM64 && ACPI
> > + help
> > + Provides support for the SMMU version 3 performance monitor unit
> (PMU)
> > + on ARM-based systems.
> > + Adds the SMMU PMU into the perf events subsystem for
> > + monitoring SMMU performance events.
> > +
> > config ARM_DSU_PMU
> > tristate "ARM DynamIQ Shared Unit (DSU) PMU"
> > depends on ARM64
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile index
> > b3902bd..b3ae48d 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -4,6 +4,7 @@ obj-$(CONFIG_ARM_CCN) += arm-ccn.o
> > obj-$(CONFIG_ARM_DSU_PMU) += arm_dsu_pmu.o
> > obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
> > obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
> > +obj-$(CONFIG_ARM_SMMUV3_PMU) += arm_smmuv3_pmu.o
> > obj-$(CONFIG_HISI_PMU) += hisilicon/
> > obj-$(CONFIG_QCOM_L2_PMU) += qcom_l2_pmu.o
> > obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o diff --git
> > a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c new
> > file mode 100644 index 0000000..b3dc394
> > --- /dev/null
> > +++ b/drivers/perf/arm_smmuv3_pmu.c
> > @@ -0,0 +1,838 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (c) 2017 The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> You don't really need to add the license text as well as SPDX. Except for the fact
> that in this case they don't match - which is it?
Right. I will stick to SPDX-License-Identifier: GPL-2.0+
>
> > + */
> > +
> > +/*
> > + * This driver adds support for perf events to use the Performance
> > + * Monitor Counter Groups (PMCG) associated with an SMMUv3 node
> > + * to monitor that node.
> > + *
> > + * SMMUv3 PMCG devices are named as arm_smmu_v3.x_pmcg.y where x
> > + * denotes the associated smmuv3 dev id and y denotes the pmu dev id.
> > + *
> > + * Filtering by stream id is done by specifying filtering parameters
> > + * with the event. options are:
> > + * filter_enable - 0 = no filtering, 1 = filtering enabled
> > + * filter_span - 0 = exact match, 1 = pattern match
> > + * filter_stream_id - pattern to filter against
> > + * Further filtering information is available in the SMMU documentation.
> > + *
> > + * Example: perf stat -e
> arm_smmu_v3.0_pmcg.6/transaction,filter_enable=1,
> > + * filter_span=1,filter_stream_id=0x42/ -a pwd
> > + * Applies filter pattern 0x42 to transaction events.
> > + *
> > + * SMMU events are not attributable to a CPU, so task mode and
> > +sampling
> > + * are not supported.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/acpi_iort.h>
> > +#include <linux/bitops.h>
> > +#include <linux/cpuhotplug.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/msi.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/smp.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/local64.h>
>
> asm/ headers in drivers always look a bit suspicious; since the dependency on
> local64_t is from the perf API anyway, I reckon it's safe to pick this up implicitly
> from perf_event.h (like most others do).
Ok.
> > +
> > +#define SMMU_PMCG_EVCNTR0 0x0
> > +#define SMMU_PMCG_EVCNTR(n, stride) (SMMU_PMCG_EVCNTR0 + (n)
> * (stride))
> > +#define SMMU_PMCG_EVTYPER0 0x400
> > +#define SMMU_PMCG_EVTYPER(n) (SMMU_PMCG_EVTYPER0 + (n) *
> 4)
> > +#define SMMU_PMCG_EVTYPER_SEC_SID_SHIFT 30
> > +#define SMMU_PMCG_EVTYPER_SID_SPAN_SHIFT 29
> > +#define SMMU_PMCG_EVTYPER_EVENT_MASK GENMASK(15, 0)
> > +#define SMMU_PMCG_SVR0 0x600
> > +#define SMMU_PMCG_SVR(n, stride) (SMMU_PMCG_SVR0 + (n) *
> (stride))
> > +#define SMMU_PMCG_SMR0 0xA00
> > +#define SMMU_PMCG_SMR(n) (SMMU_PMCG_SMR0 + (n) * 4)
> > +#define SMMU_PMCG_CNTENSET0 0xC00
> > +#define SMMU_PMCG_CNTENCLR0 0xC20
> > +#define SMMU_PMCG_INTENSET0 0xC40
> > +#define SMMU_PMCG_INTENCLR0 0xC60
> > +#define SMMU_PMCG_OVSCLR0 0xC80
> > +#define SMMU_PMCG_OVSSET0 0xCC0
> > +#define SMMU_PMCG_CAPR 0xD88
> > +#define SMMU_PMCG_SCR 0xDF8
> > +#define SMMU_PMCG_CFGR 0xE00
> > +#define SMMU_PMCG_CFGR_SID_FILTER_TYPE BIT(23)
> > +#define SMMU_PMCG_CFGR_CAPTURE BIT(22)
> > +#define SMMU_PMCG_CFGR_MSI BIT(21)
> > +#define SMMU_PMCG_CFGR_RELOC_CTRS BIT(20)
> > +#define SMMU_PMCG_CFGR_SIZE_MASK GENMASK(13, 8)
> > +#define SMMU_PMCG_CFGR_SIZE_SHIFT 8
> > +#define SMMU_PMCG_CFGR_COUNTER_SIZE_32 31
> > +#define SMMU_PMCG_CFGR_NCTR_MASK GENMASK(5, 0)
> > +#define SMMU_PMCG_CFGR_NCTR_SHIFT 0
> > +#define SMMU_PMCG_CR 0xE04
> > +#define SMMU_PMCG_CR_ENABLE BIT(0)
> > +#define SMMU_PMCG_CEID0 0xE20
> > +#define SMMU_PMCG_CEID1 0xE28
> > +#define SMMU_PMCG_IRQ_CTRL 0xE50
> > +#define SMMU_PMCG_IRQ_CTRL_IRQEN BIT(0)
> > +#define SMMU_PMCG_IRQ_CFG0 0xE58
> > +#define SMMU_PMCG_IRQ_CFG1 0xE60
> > +#define SMMU_PMCG_IRQ_CFG2 0xE64
> > +#define SMMU_PMCG_IRQ_STATUS 0xE68
> > +
> > +#define SMMU_COUNTER_RELOAD BIT(31)
> > +#define SMMU_DEFAULT_FILTER_SEC 0
> > +#define SMMU_DEFAULT_FILTER_SPAN 1
> > +#define SMMU_DEFAULT_FILTER_STREAM_ID GENMASK(31, 0)
> > +
> > +#define SMMU_MAX_COUNTERS 64
> > +#define SMMU_ARCH_MAX_EVENT_ID 128
> > +
> > +#define SMMU_IMPDEF_MAX_EVENT_ID 0xFFFF
> > +
> > +#define SMMU_PA_SHIFT 12
> > +
> > +/* Events */
> > +#define SMMU_PMU_CYCLES 0
> > +#define SMMU_PMU_TRANSACTION 1
> > +#define SMMU_PMU_TLB_MISS 2
> > +#define SMMU_PMU_CONFIG_CACHE_MISS 3
> > +#define SMMU_PMU_TRANS_TABLE_WALK 4
> > +#define SMMU_PMU_CONFIG_STRUCT_ACCESS 5
> > +#define SMMU_PMU_PCIE_ATS_TRANS_RQ 6
> > +#define SMMU_PMU_PCIE_ATS_TRANS_PASSED 7
>
> It might just be me, but I actually find it *less* helpful to have this extra level of
> indirection between the event names and numbers.
Agree.
> > +
> > +static int cpuhp_state_num;
> > +
> > +struct smmu_pmu {
> > + struct hlist_node node;
> > + struct perf_event *events[SMMU_MAX_COUNTERS];
> > + DECLARE_BITMAP(used_counters, SMMU_MAX_COUNTERS);
> > + DECLARE_BITMAP(supported_events, SMMU_ARCH_MAX_EVENT_ID);
> > + unsigned int irq;
> > + unsigned int on_cpu;
> > + struct pmu pmu;
> > + unsigned int num_counters;
> > + struct device *dev;
> > + void __iomem *reg_base;
> > + void __iomem *reloc_base;
> > + u64 counter_present_mask;
> > + u64 counter_mask;
> > +};
> > +
> > +#define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> > +
> > +#define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _size,
> _shift) \
> > + static inline u32 get_##_name(struct perf_event *event) \
> > + { \
> > + return (event->attr._config >> (_shift)) & \
> > + GENMASK_ULL((_size) - 1, 0); \
> > + }
> > +
> > +SMMU_PMU_EVENT_ATTR_EXTRACTOR(event, config, 16, 0);
> > +SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_stream_id, config1, 32, 0);
> > +SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_span, config1, 1, 32);
> > +SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_enable, config1, 1, 34);
> > +
> > +static inline void smmu_pmu_enable(struct pmu *pmu) {
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(pmu);
> > +
> > + writel(SMMU_PMCG_CR_ENABLE, smmu_pmu->reg_base +
> SMMU_PMCG_CR);
> > + writel(SMMU_PMCG_IRQ_CTRL_IRQEN,
> > + smmu_pmu->reg_base + SMMU_PMCG_IRQ_CTRL); }
> > +
> > +static inline void smmu_pmu_disable(struct pmu *pmu) {
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(pmu);
> > +
> > + writel(0, smmu_pmu->reg_base + SMMU_PMCG_CR);
> > + writel(0, smmu_pmu->reg_base + SMMU_PMCG_IRQ_CTRL); }
> > +
> > +static inline void smmu_pmu_counter_set_value(struct smmu_pmu
> *smmu_pmu,
> > + u32 idx, u64 value)
> > +{
> > + if (smmu_pmu->counter_mask & BIT(32))
> > + writeq(value, smmu_pmu->reloc_base +
> SMMU_PMCG_EVCNTR(idx, 8));
> > + else
> > + writel(value, smmu_pmu->reloc_base +
> SMMU_PMCG_EVCNTR(idx, 4)); }
> > +
> > +static inline u64 smmu_pmu_counter_get_value(struct smmu_pmu
> > +*smmu_pmu, u32 idx) {
> > + u64 value;
> > +
> > + if (smmu_pmu->counter_mask & BIT(32))
> > + value = readq(smmu_pmu->reloc_base +
> SMMU_PMCG_EVCNTR(idx, 8));
> > + else
> > + value = readl(smmu_pmu->reloc_base +
> SMMU_PMCG_EVCNTR(idx, 4));
> > +
> > + return value;
> > +}
> > +
> > +static inline void smmu_pmu_counter_enable(struct smmu_pmu
> *smmu_pmu,
> > +u32 idx) {
> > + writeq(BIT(idx), smmu_pmu->reg_base + SMMU_PMCG_CNTENSET0);
> }
> > +
> > +static inline void smmu_pmu_counter_disable(struct smmu_pmu
> > +*smmu_pmu, u32 idx) {
> > + writeq(BIT(idx), smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0);
> }
> > +
> > +static inline void smmu_pmu_interrupt_enable(struct smmu_pmu
> > +*smmu_pmu, u32 idx) {
> > + writeq(BIT(idx), smmu_pmu->reg_base + SMMU_PMCG_INTENSET0); }
> > +
> > +static inline void smmu_pmu_interrupt_disable(struct smmu_pmu
> *smmu_pmu,
> > + u32 idx)
> > +{
> > + writeq(BIT(idx), smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0); }
> > +
> > +static inline void smmu_pmu_set_evtyper(struct smmu_pmu *smmu_pmu,
> u32 idx,
> > + u32 val)
> > +{
> > + writel(val, smmu_pmu->reg_base + SMMU_PMCG_EVTYPER(idx)); }
> > +
> > +static inline void smmu_pmu_set_smr(struct smmu_pmu *smmu_pmu, u32
> > +idx, u32 val) {
> > + writel(val, smmu_pmu->reg_base + SMMU_PMCG_SMR(idx)); }
> > +
> > +static inline u64 smmu_pmu_getreset_ovsr(struct smmu_pmu *smmu_pmu)
> {
> > + u64 result = readq_relaxed(smmu_pmu->reloc_base +
> > +SMMU_PMCG_OVSSET0);
>
> Hmm, this is the only relaxed access in the driver, but IIRC the IRQ status is
> about the one place where you usually *do* want a non-relaxed access :/
Right. This is the only relaxed access and not sure why it was selected for this
(from v1).
> > +
> > + writeq(result, smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
> > + return result;
>
> Again, I very much think this would be clearer *not* broken out into a single-
> use helper (the name isn't entirely self-explanatory either).
> Plus, as-is you can't avoid pointlessly writing back 0 to OVSCLR in the shared-
> interrupt case.
Ok.
> > +}
> > +
> > +static void smmu_pmu_event_update(struct perf_event *event) {
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > + u64 delta, prev, now;
> > + u32 idx = hwc->idx;
> > +
> > + do {
> > + prev = local64_read(&hwc->prev_count);
> > + now = smmu_pmu_counter_get_value(smmu_pmu, idx);
> > + } while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
> > +
> > + /* handle overflow. */
> > + delta = now - prev;
> > + delta &= smmu_pmu->counter_mask;
> > +
> > + local64_add(delta, &event->count);
> > +}
> > +
> > +static void smmu_pmu_set_period(struct smmu_pmu *smmu_pmu,
> > + struct hw_perf_event *hwc)
> > +{
> > + u32 idx = hwc->idx;
> > + u64 new;
> > +
> > + /*
> > + * We limit the max period to half the max counter value of the
> counter
> > + * size, so that even in the case of extreme interrupt latency the
> > + * counter will (hopefully) not wrap past its initial value.
> > + */
> > + new = smmu_pmu->counter_mask >> 1;
> > +
> > + local64_set(&hwc->prev_count, new);
> > + smmu_pmu_counter_set_value(smmu_pmu, idx, new); }
> > +
> > +static unsigned int smmu_pmu_get_event_idx(struct smmu_pmu
> *smmu_pmu)
> > +{
> > + unsigned int idx;
> > + unsigned int num_ctrs = smmu_pmu->num_counters;
> > +
> > + idx = find_first_zero_bit(smmu_pmu->used_counters, num_ctrs);
> > + if (idx == num_ctrs)
> > + /* The counters are all in use. */
> > + return -EAGAIN;
> > +
> > + set_bit(idx, smmu_pmu->used_counters);
> > +
> > + return idx;
> > +}
> > +
> > +/*
> > + * Implementation of abstract pmu functionality required by
> > + * the core perf events code.
> > + */
> > +
> > +static int smmu_pmu_event_init(struct perf_event *event) {
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct perf_event *sibling;
> > + struct smmu_pmu *smmu_pmu;
> > + u32 event_id;
> > +
> > + if (event->attr.type != event->pmu->type)
> > + return -ENOENT;
> > +
> > + smmu_pmu = to_smmu_pmu(event->pmu);
> > +
> > + if (hwc->sample_period) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Sampling not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (event->cpu < 0) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Per-task mode not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + /* We cannot filter accurately so we just don't allow it. */
> > + if (event->attr.exclude_user || event->attr.exclude_kernel ||
> > + event->attr.exclude_hv || event->attr.exclude_idle) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Can't exclude execution levels\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + /* Verify specified event is supported on this PMU */
> > + event_id = get_event(event);
> > + if (((event_id < SMMU_ARCH_MAX_EVENT_ID) &&
> > + (!test_bit(event_id, smmu_pmu->supported_events))) ||
> > + (event_id > SMMU_IMPDEF_MAX_EVENT_ID)) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Invalid event %d for this PMU\n",
> > + event_id);
> > + return -EINVAL;
> > + }
> > +
> > + /* Don't allow groups with mixed PMUs, except for s/w events */
> > + if (event->group_leader->pmu != event->pmu &&
> > + !is_software_event(event->group_leader)) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Can't create mixed PMU group\n");
> > + return -EINVAL;
> > + }
> > +
> > + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> > + sibling_list)
> > + if (sibling->pmu != event->pmu &&
> > + !is_software_event(sibling)) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Can't create mixed PMU group\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Ensure all events in a group are on the same cpu */
> > + if ((event->group_leader != event) &&
> > + (event->cpu != event->group_leader->cpu)) {
> > + dev_dbg_ratelimited(smmu_pmu->dev,
> > + "Can't create group on CPUs %d and %d",
> > + event->cpu, event->group_leader->cpu);
> > + return -EINVAL;
> > + }
> > +
> > + hwc->idx = -1;
> > +
> > + /*
> > + * Ensure all events are on the same cpu so all events are in the
> > + * same cpu context, to avoid races on pmu_enable etc.
> > + */
> > + event->cpu = smmu_pmu->on_cpu;
> > +
> > + return 0;
> > +}
> > +
> > +static void smmu_pmu_event_start(struct perf_event *event, int flags)
> > +{
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + int idx = hwc->idx;
> > + u32 evtyper;
> > + u32 filter_span;
> > + u32 filter_stream_id;
> > +
> > + hwc->state = 0;
> > +
> > + smmu_pmu_set_period(smmu_pmu, hwc);
> > +
> > + if (get_filter_enable(event)) {
> > + filter_span = get_filter_span(event);
> > + filter_stream_id = get_filter_stream_id(event);
> > + } else {
> > + filter_span = SMMU_DEFAULT_FILTER_SPAN;
> > + filter_stream_id = SMMU_DEFAULT_FILTER_STREAM_ID;
> > + }
> > +
> > + evtyper = get_event(event) |
> > + filter_span << SMMU_PMCG_EVTYPER_SID_SPAN_SHIFT;
> > +
> > + smmu_pmu_set_evtyper(smmu_pmu, idx, evtyper);
> > + smmu_pmu_set_smr(smmu_pmu, idx, filter_stream_id);
> > + smmu_pmu_interrupt_enable(smmu_pmu, idx);
> > + smmu_pmu_counter_enable(smmu_pmu, idx); }
> > +
> > +static void smmu_pmu_event_stop(struct perf_event *event, int flags)
> > +{
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + int idx = hwc->idx;
> > +
> > + if (hwc->state & PERF_HES_STOPPED)
> > + return;
> > +
> > + smmu_pmu_interrupt_disable(smmu_pmu, idx);
>
> Is there any need to toggle the interrupt like this? As long as the counter's
> stopped, it's not going to overflow and generate an IRQ. Yes, there's a race
> where you might take an interrupt for a stopped counter if it overflows *while*
> CNTENCLR is being written, but explicitly writing INTENCLR like this doesn't
> solve that anyway - an IRQ may be latched at the GIC (or be an in-flight MSI
> write) as you write INTENCLR, so you could still end up taking it 'spuriously'
> after hitting CNTENCLR.
Hmm... Few ones in drivers/perf seems to follow the interrupt enable/disable
sequence. I will check again.
> > + smmu_pmu_counter_disable(smmu_pmu, idx);
> > +
> > + if (flags & PERF_EF_UPDATE)
> > + smmu_pmu_event_update(event);
> > + hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE; }
> > +
> > +static int smmu_pmu_event_add(struct perf_event *event, int flags) {
> > + struct hw_perf_event *hwc = &event->hw;
> > + int idx;
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > +
> > + idx = smmu_pmu_get_event_idx(smmu_pmu);
> > + if (idx < 0)
> > + return idx;
> > +
> > + hwc->idx = idx;
> > + hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > + smmu_pmu->events[idx] = event;
> > + local64_set(&hwc->prev_count, 0);
> > +
> > + if (flags & PERF_EF_START)
> > + smmu_pmu_event_start(event, flags);
> > +
> > + /* Propagate changes to the userspace mapping. */
> > + perf_event_update_userpage(event);
> > +
> > + return 0;
> > +}
> > +
> > +static void smmu_pmu_event_del(struct perf_event *event, int flags) {
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > + int idx = hwc->idx;
> > +
> > + smmu_pmu_event_stop(event, flags | PERF_EF_UPDATE);
> > + smmu_pmu->events[idx] = NULL;
> > + clear_bit(idx, smmu_pmu->used_counters);
> > +
> > + perf_event_update_userpage(event);
> > +}
> > +
> > +static void smmu_pmu_event_read(struct perf_event *event) {
> > + smmu_pmu_event_update(event);
> > +}
> > +
> > +/* cpumask */
> > +
> > +static ssize_t smmu_pmu_cpumask_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct smmu_pmu *smmu_pmu =
> to_smmu_pmu(dev_get_drvdata(dev));
> > +
> > + return cpumap_print_to_pagebuf(true, buf,
> > +cpumask_of(smmu_pmu->on_cpu)); }
> > +
> > +static struct device_attribute smmu_pmu_cpumask_attr =
> > + __ATTR(cpumask, 0444, smmu_pmu_cpumask_show, NULL);
> > +
> > +static struct attribute *smmu_pmu_cpumask_attrs[] = {
> > + &smmu_pmu_cpumask_attr.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group smmu_pmu_cpumask_group = {
> > + .attrs = smmu_pmu_cpumask_attrs,
> > +};
> > +
> > +/* Events */
> > +
> > +ssize_t smmu_pmu_event_show(struct device *dev,
> > + struct device_attribute *attr, char *page) {
> > + struct perf_pmu_events_attr *pmu_attr;
> > +
> > + pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
> > +
> > + return sprintf(page, "event=0x%02llx\n", pmu_attr->id); }
> > +
> > +#define SMMU_EVENT_ATTR(_name, _id)
> \
> > + (&((struct perf_pmu_events_attr[]) { \
> > + { .attr = __ATTR(_name, 0444, smmu_pmu_event_show,
> NULL), \
> > + .id = _id, } \
> > + })[0].attr.attr)
> > +
> > +static struct attribute *smmu_pmu_events[] = {
> > + SMMU_EVENT_ATTR(cycles, SMMU_PMU_CYCLES),
> > + SMMU_EVENT_ATTR(transaction, SMMU_PMU_TRANSACTION),
> > + SMMU_EVENT_ATTR(tlb_miss, SMMU_PMU_TLB_MISS),
> > + SMMU_EVENT_ATTR(config_cache_miss,
> SMMU_PMU_CONFIG_CACHE_MISS),
> > + SMMU_EVENT_ATTR(trans_table_walk,
> SMMU_PMU_TRANS_TABLE_WALK),
> > + SMMU_EVENT_ATTR(config_struct_access,
> SMMU_PMU_CONFIG_STRUCT_ACCESS),
> > + SMMU_EVENT_ATTR(pcie_ats_trans_rq,
> SMMU_PMU_PCIE_ATS_TRANS_RQ),
> > + SMMU_EVENT_ATTR(pcie_ats_trans_passed,
> SMMU_PMU_PCIE_ATS_TRANS_PASSED),
> > + NULL
> > +};
> > +
> > +static umode_t smmu_pmu_event_is_visible(struct kobject *kobj,
> > + struct attribute *attr, int unused) {
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct smmu_pmu *smmu_pmu =
> to_smmu_pmu(dev_get_drvdata(dev));
> > + struct perf_pmu_events_attr *pmu_attr;
> > +
> > + pmu_attr = container_of(attr, struct perf_pmu_events_attr,
> > +attr.attr);
> > +
> > + if (test_bit(pmu_attr->id, smmu_pmu->supported_events))
> > + return attr->mode;
> > +
> > + return 0;
> > +}
> > +static struct attribute_group smmu_pmu_events_group = {
> > + .name = "events",
> > + .attrs = smmu_pmu_events,
> > + .is_visible = smmu_pmu_event_is_visible, };
> > +
> > +/* Formats */
> > +PMU_FORMAT_ATTR(event, "config:0-15");
> > +PMU_FORMAT_ATTR(filter_stream_id, "config1:0-31");
> > +PMU_FORMAT_ATTR(filter_span, "config1:32");
> > +PMU_FORMAT_ATTR(filter_enable, "config1:33");
> > +
> > +static struct attribute *smmu_pmu_formats[] = {
> > + &format_attr_event.attr,
> > + &format_attr_filter_stream_id.attr,
> > + &format_attr_filter_span.attr,
> > + &format_attr_filter_enable.attr,
> > + NULL
> > +};
> > +
> > +static struct attribute_group smmu_pmu_format_group = {
> > + .name = "format",
> > + .attrs = smmu_pmu_formats,
> > +};
> > +
> > +static const struct attribute_group *smmu_pmu_attr_grps[] = {
> > + &smmu_pmu_cpumask_group,
> > + &smmu_pmu_events_group,
> > + &smmu_pmu_format_group,
> > + NULL,
> > +};
> > +
> > +/*
> > + * Generic device handlers
> > + */
> > +
> > +static unsigned int get_num_counters(struct smmu_pmu *smmu_pmu) {
> > + u32 cfgr = readl(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> > +
> > + return ((cfgr & SMMU_PMCG_CFGR_NCTR_MASK) >>
> SMMU_PMCG_CFGR_NCTR_SHIFT)
> > + + 1;
>
> The code would be considerably simpler if this were inline at the one single
> place it's ever used. Re-reading CFGR for every single field is just silly.
Ok.
> > +}
> > +
> > +static int smmu_pmu_offline_cpu(unsigned int cpu, struct hlist_node
> > +*node) {
> > + struct smmu_pmu *smmu_pmu;
> > + unsigned int target;
> > +
> > + smmu_pmu = hlist_entry_safe(node, struct smmu_pmu, node);
> > + if (cpu != smmu_pmu->on_cpu)
> > + return 0;
> > +
> > + target = cpumask_any_but(cpu_online_mask, cpu);
> > + if (target >= nr_cpu_ids)
> > + return 0;
> > +
> > + perf_pmu_migrate_context(&smmu_pmu->pmu, cpu, target);
> > + smmu_pmu->on_cpu = target;
> > + WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(target)));
> > +
> > + return 0;
> > +}
> > +
> > +static int smmu_pmu_get_dev_id(const char *name, unsigned long *id) {
> > + char *temp, *start, *end;
> > + int ret = -EINVAL;
> > +
> > + temp = kstrdup(name, GFP_KERNEL);
> > + if (!temp)
> > + return ret;
> > +
> > + end = strrchr(temp, '.');
> > + if (!end)
> > + goto out;
> > +
> > + temp[end - temp] = '\0';
> > + start = strchr(temp, '.');
> > + if (!start)
> > + goto out;
> > +
> > + ret = kstrtoul(&temp[start - temp + 1], 10, id);
>
> That's a pretty roundabout way to not simply dereference
> to_platform_device(dev)->id ;)
True. My bad :)
> Also, how relevant is it going to be for future DT support? We don't really want
> too many artificial dependencies on the way ACPI support happens to currently
> be implemented.
Sorry, it's not clear to me what is proposed here as far as naming the PMU is
concerned. Please see below as well.
> > +out:
> > + kfree(temp);
> > + return ret;
> > +}
> > +
> > +
> > +static char *smmu_pmu_assign_name(struct smmu_pmu *pmu) {
> > + unsigned long id;
> > + struct device *smmu, *dev = pmu->dev;
> > + char *s_name = NULL, *p_name = NULL;
> > +
> > + smmu = iort_find_pmcg_ref_smmu(dev);
> > + if (smmu) {
> > + if (!smmu_pmu_get_dev_id(dev_name(smmu), &id))
> > + s_name = kasprintf(GFP_KERNEL,
> "arm_smmu_v3_%lu", id);
> > + }
> > +
> > + if (!s_name)
> > + s_name = kasprintf(GFP_KERNEL, "arm_smmu_v3");
>
> As I touched on before, I think it's worth generalising this from the start, and
> trying to resolve the component reference to a struct device rather than
> IORT/SMMU specific internals. However it also occurs to me that maybe this
> isn't as important as it first seemed - since the auto-numbered ID doesn't
> actually say which PMCG is which, the only way for the user to actually identify
> which PMU is the correct one to count events for a particular endpoint is still to
> grovel up the base address, so as long as the PMU name uniquely correlates to
> the PMCG device, I'm not sure anything really matters beyond that.
So If I understand this correctly,
iort_find_pmcg_ref_smmu() should be something like iort_find_pmcg_ref()
which returns the associated struct device for the ref node and then, pmu is
named as,
arm_smmu_v3_x_pmcg_y
nc_dev_name_x_pmcg_y
pciXXXX_pmcg_y (Itâs a bit tricky for RC as we will end up with struct pci_bus)
(where x and y are auto ids)
Please let me know if this is what is proposed here.
It is possible to include the pmcg base address instead of the auto-numbered id
as proposed in v1 series.
> > +
> > + if (smmu_pmu_get_dev_id(dev_name(dev), &id))
> > + goto out;
> > +
> > + p_name = devm_kasprintf(dev, GFP_KERNEL, "%s_pmcg_%lu", s_name,
> id);
> > +out:
> > + kfree(s_name);
> > + return p_name;
> > +}
> > +
> > +static irqreturn_t smmu_pmu_handle_irq(int irq_num, void *data) {
> > + struct smmu_pmu *smmu_pmu = data;
> > + u64 ovsr;
> > + unsigned int idx;
> > +
> > + ovsr = smmu_pmu_getreset_ovsr(smmu_pmu);
> > + if (!ovsr)
> > + return IRQ_NONE;
> > +
> > + for_each_set_bit(idx, (unsigned long *)&ovsr, smmu_pmu-
> >num_counters) {
> > + struct perf_event *event = smmu_pmu->events[idx];
> > + struct hw_perf_event *hwc;
> > +
> > + if (WARN_ON_ONCE(!event))
> > + continue;
> > +
> > + smmu_pmu_event_update(event);
> > + hwc = &event->hw;
> > +
> > + smmu_pmu_set_period(smmu_pmu, hwc);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int smmu_pmu_reset(struct smmu_pmu *smmu_pmu) {
> > + /* Disable counter and interrupt */
> > + writeq(smmu_pmu->counter_present_mask,
> > + smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0);
> > + writeq(smmu_pmu->counter_present_mask,
> > + smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0);
>
> There are a number of other registers like OVS and IRQ_CFG* which have
> unknown reset values so probably want sanitising.
>
> > +
> > + smmu_pmu_disable(&smmu_pmu->pmu);
>
> It might make more sense to hit the global disable first, then clean up the rest
> of the state.
Ok.
> > + return 0;
> > +}
> > +
> > +static int smmu_pmu_probe(struct platform_device *pdev) {
> > + struct smmu_pmu *smmu_pmu;
> > + struct resource *mem_resource_0, *mem_resource_1;
> > + void __iomem *mem_map_0, *mem_map_1;
> > + unsigned int reg_size;
> > + u64 ceid_64[2];
> > + int irq, err;
> > + struct device *dev = &pdev->dev;
> > +
> > + smmu_pmu = devm_kzalloc(dev, sizeof(*smmu_pmu), GFP_KERNEL);
> > + if (!smmu_pmu)
> > + return -ENOMEM;
> > +
> > + smmu_pmu->dev = dev;
> > +
> > + platform_set_drvdata(pdev, smmu_pmu);
> > + smmu_pmu->pmu = (struct pmu) {
> > + .task_ctx_nr = perf_invalid_context,
> > + .pmu_enable = smmu_pmu_enable,
> > + .pmu_disable = smmu_pmu_disable,
> > + .event_init = smmu_pmu_event_init,
> > + .add = smmu_pmu_event_add,
> > + .del = smmu_pmu_event_del,
> > + .start = smmu_pmu_event_start,
> > + .stop = smmu_pmu_event_stop,
> > + .read = smmu_pmu_event_read,
> > + .attr_groups = smmu_pmu_attr_grps,
> > + };
> > +
> > + mem_resource_0 = platform_get_resource(pdev, IORESOURCE_MEM,
> 0);
> > + mem_map_0 = devm_ioremap_resource(dev, mem_resource_0);
> > +
> > + if (IS_ERR(mem_map_0)) {
> > + dev_err(dev, "Can't map SMMU PMU @%pa\n",
> > + &mem_resource_0->start);
> > + return PTR_ERR(mem_map_0);
> > + }
> > +
> > + smmu_pmu->reg_base = mem_map_0;
> > +
> > + smmu_pmu->pmu.name = smmu_pmu_assign_name(smmu_pmu);
>
> It seems a bit odd to assign to pmu.name directly, given that that's really
> perf_pmu_register()'s job. (Bonus nit: it also feels a bit odd to be worrying
> about the PMU name right in the middle of discovering the hardware;
> personally I'd leave it until the end it's actually needed.
> TBH the whole function seems a bit mixed up in terms of logical order)
Agree and also I will try to make the order more logical.
> > + if (!smmu_pmu->pmu.name) {
> > + dev_err(dev, "Failed to create PMU name");
> > + return -EINVAL;
> > + }
> > +
> > + ceid_64[0] = readq(smmu_pmu->reg_base + SMMU_PMCG_CEID0);
> > + ceid_64[1] = readq(smmu_pmu->reg_base + SMMU_PMCG_CEID1);
> > + bitmap_from_arr32(smmu_pmu->supported_events, (u32 *)ceid_64,
> > + SMMU_ARCH_MAX_EVENT_ID);
>
> I probably said this before and have forgotten the outcome, but is this endian-
> safe, or does it risk shuffling alternate words in the bitmap?
Right. This was a u32[4] array before and I thought the conclusion was to use
u64[2] and cast. I will double check.
>
> > +
> > + /* Determine if page 1 is present */
> > + if (readl(smmu_pmu->reg_base + SMMU_PMCG_CFGR) &
> > + SMMU_PMCG_CFGR_RELOC_CTRS) {
> > + mem_resource_1 = platform_get_resource(pdev,
> IORESOURCE_MEM, 1);
> > + mem_map_1 = devm_ioremap_resource(dev,
> mem_resource_1);
> > +
> > + if (IS_ERR(mem_map_1)) {
> > + dev_err(dev, "Can't map SMMU PMU @%pa\n",
> > + &mem_resource_1->start);
> > + return PTR_ERR(mem_map_1);
> > + }
> > + smmu_pmu->reloc_base = mem_map_1;
> > + } else {
> > + smmu_pmu->reloc_base = smmu_pmu->reg_base;
> > + }
>
> Actually, you may as well pull the number-of-counters stuff up here, because
> it's silly to go and read CFGR twice in the same function.
Ok.
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(dev, "Failed to get valid irq for smmu @%pa\n",
> > + &mem_resource_0->start);
> > + return irq;
> > + }
> > +
> > + err = devm_request_irq(dev, irq, smmu_pmu_handle_irq,
> > + IRQF_NOBALANCING | IRQF_SHARED |
> IRQF_NO_THREAD,
> > + "smmu-pmu", smmu_pmu);
> > + if (err) {
> > + dev_err(dev,
> > + "Unable to request IRQ%d for SMMU PMU
> counters\n", irq);
> > + return err;
> > + }
> > +
> > + smmu_pmu->irq = irq;
> > +
> > + /* Pick one CPU to be the preferred one to use */
> > + smmu_pmu->on_cpu = smp_processor_id();
>
> Do we want this to be a get_cpu() to avoid complications from hotplug events
> between here and actually having registered the handler? ISTR copying that
> pattern from somewhere (probably arm-cci) for one of my drivers.
Ok. I will refer arm-cci.
>
> > + WARN_ON(irq_set_affinity(smmu_pmu->irq,
> > +cpumask_of(smmu_pmu->on_cpu)));
> > +
> > + smmu_pmu->num_counters = get_num_counters(smmu_pmu);
> > + smmu_pmu->counter_present_mask = GENMASK(smmu_pmu-
> >num_counters - 1, 0);
> > + reg_size = (readl(smmu_pmu->reg_base + SMMU_PMCG_CFGR) &
> > + SMMU_PMCG_CFGR_SIZE_MASK) >>
> SMMU_PMCG_CFGR_SIZE_SHIFT;
>
> Nit: it might be worth using some bitfield.h magic to clean up accessors like this
> (yes, it's still my new favourite thing). Also, we may as well use relaxed reads
> for the ID registers at least, since they definitely have no subtle ordering to
> worry about.
Ok.
> > + smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0);
> > +
> > + smmu_pmu_reset(smmu_pmu);
> > +
> > + err = cpuhp_state_add_instance_nocalls(cpuhp_state_num,
> > + &smmu_pmu->node);
> > + if (err) {
> > + dev_err(dev, "Error %d registering hotplug", err);
> > + return err;
> > + }
> > +
> > + err = perf_pmu_register(&smmu_pmu->pmu, smmu_pmu->pmu.name,
> -1);
> > + if (err) {
> > + dev_err(dev, "Error %d registering SMMU PMU\n", err);
> > + goto out_unregister;
> > + }
> > +
> > + dev_info(dev, "Registered SMMU PMU @ %pa using %d counters\n",
> > + &mem_resource_0->start, smmu_pmu->num_counters);
> > +
> > + return err;
> > +
> > +out_unregister:
> > + cpuhp_state_remove_instance_nocalls(cpuhp_state_num,
> &smmu_pmu->node);
> > + return err;
> > +}
> > +
> > +static int smmu_pmu_remove(struct platform_device *pdev) {
> > + struct smmu_pmu *smmu_pmu = platform_get_drvdata(pdev);
> > +
> > + perf_pmu_unregister(&smmu_pmu->pmu);
> > + cpuhp_state_remove_instance_nocalls(cpuhp_state_num,
> > +&smmu_pmu->node);
> > +
> > + return 0;
> > +}
> > +
> > +static void smmu_pmu_shutdown(struct platform_device *pdev) {
> > + struct smmu_pmu *smmu_pmu = platform_get_drvdata(pdev);
> > +
> > + smmu_pmu_disable(&smmu_pmu->pmu);
> > +}
> > +
> > +static struct platform_driver smmu_pmu_driver = {
> > + .driver = {
> > + .name = "arm-smmu-v3-pmu",
> > + },
> > + .probe = smmu_pmu_probe,
> > + .remove = smmu_pmu_remove,
> > + .shutdown = smmu_pmu_shutdown,
> > +};
> > +
> > +static int __init arm_smmu_pmu_init(void) {
> > + cpuhp_state_num =
> cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> > + "perf/arm/smmupmu:online",
>
> Nit: "smmupmu" looks a bit wacky as a name - TBH I think just "smmuv3"
> or "pmcg" would be enough.
Ok.
>
> > + NULL,
> > + smmu_pmu_offline_cpu);
> > + if (cpuhp_state_num < 0)
> > + return cpuhp_state_num;
> > +
> > + return platform_driver_register(&smmu_pmu_driver);
> > +}
> > +module_init(arm_smmu_pmu_init);
> > +
> > +static void __exit arm_smmu_pmu_exit(void) {
>
> Should we not be removing the hotplug state?
Yes. That is missed.
Thanks,
Shameer
> Robin.
>
> > + platform_driver_unregister(&smmu_pmu_driver);
> > +}
> > +
> > +module_exit(arm_smmu_pmu_exit);
> > +MODULE_LICENSE("GPL v2");
> >