RE: [PATCH v2 3/4] perf: add arm64 smmuv3 pmu driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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");
> >





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux