[PATCH v3 0/3] arm64 SMMUv3 PMU driver with IORT support

[PATCH v3 0/3] arm64 SMMUv3 PMU driver with IORT support

[Shameer Kolothum]

This adds a driver for the SMMUv3 PMU into the perf framework.
It includes an IORT update to support PM Counter Groups.

This is based on the initial work done by Neil Leeder[1]

SMMUv3 PMCG devices are named as smmuv3_pmcg_<phys_addr_page>
where <phys_addr_page> is the physical page address of the SMMU PMCG.
For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840

Usage example:
For common arch supported events:
perf stat -e smmuv3_pmcg_ff88840/transaction,filter_enable=1,
 filter_span=1,filter_stream_id=0x42/ -a pwd

For IMP DEF events:
perf stat -e smmuv3_pmcg_ff88840/event=id/ -a pwd

Sanity tested on HiSilicon platform. Further testing on supported
platforms are very much welcome.

v2 --> v3

-Addressed comments from Robin.
-Removed iort helper function to retrieve the PMCG reference smmu.
-PMCG devices are now named using the base address

v1 --> v2

- Addressed comments from Robin.
- Added an helper to retrieve the associated smmu dev and named PMUs
  to make the association visible to user.
- Added MSI support  for overflow irq

[1]https://www.spinics.net/lists/arm-kernel/msg598591.html

Neil Leeder (2):
  acpi: arm64: add iort support for PMCG
  perf: add arm64 smmuv3 pmu driver

Shameer Kolothum (1):
  perf/smmuv3: Add MSI irq support

 drivers/acpi/arm64/iort.c     |  78 ++++-
 drivers/perf/Kconfig          |   9 +
 drivers/perf/Makefile         |   1 +
 drivers/perf/arm_smmuv3_pmu.c | 794 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 870 insertions(+), 12 deletions(-)
 create mode 100644 drivers/perf/arm_smmuv3_pmu.c

-- 
2.7.4

[PATCH v3 1/3] acpi: arm64: add iort support for PMCG

[Shameer Kolothum]

From: Neil Leeder <nleeder@codeaurora.org>

Add support for the SMMU Performance Monitor Counter Group
information from ACPI. This is in preparation for its use
in the SMMUv3 PMU driver.

Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/acpi/arm64/iort.c | 78 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 08f26db..b979c86 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -356,7 +356,8 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
 		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
 		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
-		    node->type == ACPI_IORT_NODE_SMMU_V3) {
+		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
+		    node->type == ACPI_IORT_NODE_PMCG) {
 			*id_out = map->output_base;
 			return parent;
 		}
@@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct acpi_iort_node *node)
 		}
 
 		return smmu->id_mapping_index;
+	case ACPI_IORT_NODE_PMCG:
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -1309,6 +1312,50 @@ static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
 	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
 }
 
+static void __init arm_smmu_common_dma_configure(struct device *dev,
+						enum dev_dma_attr attr)
+{
+	/* We expect the dma masks to be equivalent for all SMMUs set-ups */
+	dev->dma_mask = &dev->coherent_dma_mask;
+
+	/* Configure DMA for the page table walker */
+	acpi_dma_configure(dev, attr);
+}
+
+static int __init arm_smmu_v3_pmcg_count_resources(struct acpi_iort_node *node)
+{
+	struct acpi_iort_pmcg *pmcg;
+
+	/* Retrieve PMCG specific data */
+	pmcg = (struct acpi_iort_pmcg *)node->node_data;
+
+	/*
+	 * There are always 2 memory resources.
+	 * If the overflow_gsiv is present then add that for a total of 3.
+	 */
+	return pmcg->overflow_gsiv ? 3 : 2;
+}
+
+static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
+					       struct acpi_iort_node *node)
+{
+	struct acpi_iort_pmcg *pmcg;
+
+	/* Retrieve PMCG specific data */
+	pmcg = (struct acpi_iort_pmcg *)node->node_data;
+
+	res[0].start = pmcg->page0_base_address;
+	res[0].end = pmcg->page0_base_address + SZ_4K - 1;
+	res[0].flags = IORESOURCE_MEM;
+	res[1].start = pmcg->page1_base_address;
+	res[1].end = pmcg->page1_base_address + SZ_4K - 1;
+	res[1].flags = IORESOURCE_MEM;
+
+	if (pmcg->overflow_gsiv)
+		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
+				       ACPI_EDGE_SENSITIVE, &res[2]);
+}
+
 struct iort_dev_config {
 	const char *name;
 	int (*dev_init)(struct acpi_iort_node *node);
@@ -1318,6 +1365,8 @@ struct iort_dev_config {
 				     struct acpi_iort_node *node);
 	void (*dev_set_proximity)(struct device *dev,
 				    struct acpi_iort_node *node);
+	void (*dev_dma_configure)(struct device *dev,
+					enum dev_dma_attr attr);
 };
 
 static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
@@ -1326,23 +1375,34 @@ static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
 	.dev_count_resources = arm_smmu_v3_count_resources,
 	.dev_init_resources = arm_smmu_v3_init_resources,
 	.dev_set_proximity = arm_smmu_v3_set_proximity,
+	.dev_dma_configure = arm_smmu_common_dma_configure,
 };
 
 static const struct iort_dev_config iort_arm_smmu_cfg __initconst = {
 	.name = "arm-smmu",
 	.dev_is_coherent = arm_smmu_is_coherent,
 	.dev_count_resources = arm_smmu_count_resources,
-	.dev_init_resources = arm_smmu_init_resources
+	.dev_init_resources = arm_smmu_init_resources,
+	.dev_dma_configure = arm_smmu_common_dma_configure,
+};
+
+static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = {
+	.name = "arm-smmu-v3-pmu",
+	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
+	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
 };
 
 static __init const struct iort_dev_config *iort_get_dev_cfg(
 			struct acpi_iort_node *node)
 {
+
 	switch (node->type) {
 	case ACPI_IORT_NODE_SMMU_V3:
 		return &iort_arm_smmu_v3_cfg;
 	case ACPI_IORT_NODE_SMMU:
 		return &iort_arm_smmu_cfg;
+	case ACPI_IORT_NODE_PMCG:
+		return &iort_arm_smmu_v3_pmcg_cfg;
 	default:
 		return NULL;
 	}
@@ -1398,12 +1458,6 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
 	if (ret)
 		goto dev_put;
 
-	/*
-	 * We expect the dma masks to be equivalent for
-	 * all SMMUs set-ups
-	 */
-	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
-
 	fwnode = iort_get_fwnode(node);
 
 	if (!fwnode) {
@@ -1413,11 +1467,11 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
 
 	pdev->dev.fwnode = fwnode;
 
-	attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
+	if (ops->dev_dma_configure) {
+		attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
 			DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
-
-	/* Configure DMA for the page table walker */
-	acpi_dma_configure(&pdev->dev, attr);
+		ops->dev_dma_configure(&pdev->dev, attr);
+	}
 
 	iort_set_device_domain(&pdev->dev, node);
 
-- 
2.7.4

[PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameer Kolothum]

From: Neil Leeder <nleeder@codeaurora.org>

Adds a new driver to support the SMMUv3 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 smmuv3_pmcg_<phys_addr_page> where
<phys_addr_page> is the physical page address of the SMMU PMCG.
For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840

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 smmuv3_pmcg_ff88840/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@codeaurora.org>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/perf/Kconfig          |   9 +
 drivers/perf/Makefile         |   1 +
 drivers/perf/arm_smmuv3_pmu.c | 736 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 746 insertions(+)
 create mode 100644 drivers/perf/arm_smmuv3_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 08ebaf7..34969dd 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_SMMU_V3_PMU
+	 bool "ARM SMMUv3 Performance Monitors {Extension}"
+	 depends on ARM64 && ACPI && ARM_SMMU_V3
+	   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..f10a932 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_SMMU_V3_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..2fa6c96
--- /dev/null
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -0,0 +1,736 @@
+// 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 smmuv3_pmcg_<phys_addr_page> where
+ * <phys_addr_page> is the physical page address of the SMMU PMCG.
+ * For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840
+
+ * 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 smmuv3_pmcg_ff88840/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/bitfield.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>
+
+#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_SID_SPAN_SHIFT        29
+#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_CFGR                  0xE00
+#define SMMU_PMCG_CFGR_RELOC_CTRS       BIT(20)
+#define SMMU_PMCG_CFGR_SIZE_MASK        GENMASK(13, 8)
+#define SMMU_PMCG_CFGR_NCTR_MASK        GENMASK(5, 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_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
+
+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 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 smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
+	struct device *dev = smmu_pmu->dev;
+	struct perf_event *sibling;
+	u32 event_id;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (hwc->sample_period) {
+		dev_dbg_ratelimited(dev, "Sampling not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (event->cpu < 0) {
+		dev_dbg_ratelimited(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(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(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(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(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(dev,
+				    "Can't create group on CPUs %d and %d\n",
+				    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_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_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, 0),
+	SMMU_EVENT_ATTR(transaction, 1),
+	SMMU_EVENT_ATTR(tlb_miss, 2),
+	SMMU_EVENT_ATTR(config_cache_miss, 3),
+	SMMU_EVENT_ATTR(trans_table_walk, 4),
+	SMMU_EVENT_ATTR(config_struct_access, 5),
+	SMMU_EVENT_ATTR(pcie_ats_trans_rq, 6),
+	SMMU_EVENT_ATTR(pcie_ats_trans_passed, 7),
+	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 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 irqreturn_t smmu_pmu_handle_irq(int irq_num, void *data)
+{
+	struct smmu_pmu *smmu_pmu = data;
+	u64 ovsr;
+	unsigned int idx;
+
+	ovsr = readq(smmu_pmu->reloc_base + SMMU_PMCG_OVSSET0);
+	if (!ovsr)
+		return IRQ_NONE;
+
+	writeq(ovsr, smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
+
+	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_setup_irq(struct smmu_pmu *pmu)
+{
+	unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;
+	int irq, ret = -ENXIO;
+
+	irq = pmu->irq;
+	if (irq)
+		ret = devm_request_irq(pmu->dev, irq, smmu_pmu_handle_irq,
+				       flags, "smmuv3-pmu", pmu);
+	return ret;
+}
+
+static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
+{
+	smmu_pmu_disable(&smmu_pmu->pmu);
+
+	/* Disable counter and interrupt */
+	writeq_relaxed(smmu_pmu->counter_present_mask,
+		       smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0);
+	writeq_relaxed(smmu_pmu->counter_present_mask,
+		       smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0);
+	writeq_relaxed(smmu_pmu->counter_present_mask,
+		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
+
+	writeq_relaxed(0, smmu_pmu->reg_base + SMMU_PMCG_IRQ_CFG0);
+}
+
+static int smmu_pmu_probe(struct platform_device *pdev)
+{
+	struct smmu_pmu *smmu_pmu;
+	struct resource *res_0, *res_1;
+	u32 cfgr, reg_size;
+	u64 ceid_64[2];
+	int irq, err;
+	char *name;
+	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,
+	};
+
+	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);
+	if (IS_ERR(smmu_pmu->reg_base))
+		return PTR_ERR(smmu_pmu->reg_base);
+
+	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
+
+	/* Determine if page 1 is present */
+	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
+		res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		smmu_pmu->reloc_base = devm_ioremap_resource(dev, res_1);
+		if (IS_ERR(smmu_pmu->reloc_base))
+			return PTR_ERR(smmu_pmu->reloc_base);
+	} else {
+		smmu_pmu->reloc_base = smmu_pmu->reg_base;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0)
+		smmu_pmu->irq = irq;
+
+	ceid_64[0] = readq_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CEID0);
+	ceid_64[1] = readq_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CEID1);
+	bitmap_from_arr32(smmu_pmu->supported_events, (u32 *)ceid_64,
+			  SMMU_ARCH_MAX_EVENT_ID);
+
+	smmu_pmu->num_counters = FIELD_GET(SMMU_PMCG_CFGR_NCTR_MASK, cfgr) + 1;
+	smmu_pmu->counter_present_mask = GENMASK(smmu_pmu->num_counters - 1, 0);
+
+	reg_size = FIELD_GET(SMMU_PMCG_CFGR_SIZE_MASK, cfgr);
+	smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0);
+
+	smmu_pmu_reset(smmu_pmu);
+
+	err = smmu_pmu_setup_irq(smmu_pmu);
+	if (err) {
+		dev_err(dev, "Setup irq failed, PMU @%pa\n", &res_0->start);
+		return err;
+	}
+
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "smmuv3_pmcg_%llx",
+			      (res_0->start) >> SMMU_PA_SHIFT);
+	if (!name) {
+		dev_err(dev, "Create name failed, PMU @%pa\n", &res_0->start);
+		return -EINVAL;
+	}
+
+	err = cpuhp_state_add_instance_nocalls(cpuhp_state_num,
+					       &smmu_pmu->node);
+	if (err) {
+		dev_err(dev, "Error %d registering hotplug, PMU @%pa\n",
+			err, &res_0->start);
+		return err;
+	}
+
+	/* Pick one CPU to be the preferred one to use */
+	smmu_pmu->on_cpu = get_cpu();
+	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
+
+	err = perf_pmu_register(&smmu_pmu->pmu, name, -1);
+	if (err) {
+		dev_err(dev, "Error %d registering PMU @%pa\n",
+			err, &res_0->start);
+		goto out_unregister;
+	}
+
+	put_cpu();
+	dev_info(dev, "Registered SMMU PMU @ %pa using %d counters\n",
+		 &res_0->start, smmu_pmu->num_counters);
+
+	return err;
+
+out_unregister:
+	put_cpu();
+	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/pmcg:online",
+				      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)
+{
+	platform_driver_unregister(&smmu_pmu_driver);
+	cpuhp_remove_multi_state(cpuhp_state_num);
+}
+
+module_exit(arm_smmu_pmu_exit);
+
+MODULE_DESCRIPTION("PMU driver for ARM SMMUv3 Performance Monitors Extension");
+MODULE_AUTHOR("Neil Leeder <nleeder@codeaurora.org>");
+MODULE_AUTHOR("Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v3 3/3] perf/smmuv3: Add MSI irq support

[Shameer Kolothum]

This adds support for MSI-based counter overflow interrupt.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 2fa6c96..84f7907 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -57,6 +57,7 @@
 #define SMMU_PMCG_OVSSET0               0xCC0
 #define SMMU_PMCG_CFGR                  0xE00
 #define SMMU_PMCG_CFGR_RELOC_CTRS       BIT(20)
+#define SMMU_PMCG_CFGR_MSI              BIT(21)
 #define SMMU_PMCG_CFGR_SIZE_MASK        GENMASK(13, 8)
 #define SMMU_PMCG_CFGR_NCTR_MASK        GENMASK(5, 0)
 #define SMMU_PMCG_CR                    0xE04
@@ -66,6 +67,12 @@
 #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
+
+/* MSI config fields */
+#define MSI_CFG0_ADDR_MASK              GENMASK_ULL(51, 2)
+#define MSI_CFG2_MEMATTR_DEVICE_nGnRE   0x1
 
 #define SMMU_DEFAULT_FILTER_SPAN        1
 #define SMMU_DEFAULT_FILTER_STREAM_ID   GENMASK(31, 0)
@@ -548,11 +555,62 @@ static irqreturn_t smmu_pmu_handle_irq(int irq_num, void *data)
 	return IRQ_HANDLED;
 }
 
+static void smmu_pmu_free_msis(void *data)
+{
+	struct device *dev = data;
+
+	platform_msi_domain_free_irqs(dev);
+}
+
+static void smmu_pmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	phys_addr_t doorbell;
+	struct device *dev = msi_desc_to_dev(desc);
+	struct smmu_pmu *pmu = dev_get_drvdata(dev);
+
+	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
+	doorbell &= MSI_CFG0_ADDR_MASK;
+
+	writeq_relaxed(doorbell, pmu->reg_base + SMMU_PMCG_IRQ_CFG0);
+	writel_relaxed(msg->data, pmu->reg_base + SMMU_PMCG_IRQ_CFG1);
+	writel_relaxed(MSI_CFG2_MEMATTR_DEVICE_nGnRE,
+		       pmu->reg_base + SMMU_PMCG_IRQ_CFG2);
+}
+
+static void smmu_pmu_setup_msi(struct smmu_pmu *pmu)
+{
+	struct msi_desc *desc;
+	struct device *dev = pmu->dev;
+	int ret;
+
+	/* Clear MSI address reg */
+	writeq_relaxed(0, pmu->reg_base + SMMU_PMCG_IRQ_CFG0);
+
+	/* MSI supported or not */
+	if (!(readl(pmu->reg_base + SMMU_PMCG_CFGR) & SMMU_PMCG_CFGR_MSI))
+		return;
+
+	ret = platform_msi_domain_alloc_irqs(dev, 1, smmu_pmu_write_msi_msg);
+	if (ret) {
+		dev_warn(dev, "failed to allocate MSIs\n");
+		return;
+	}
+
+	desc = first_msi_entry(dev);
+	if (desc)
+		pmu->irq = desc->irq;
+
+	/* Add callback to free MSIs on teardown */
+	devm_add_action(dev, smmu_pmu_free_msis, dev);
+}
+
 static int smmu_pmu_setup_irq(struct smmu_pmu *pmu)
 {
 	unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;
 	int irq, ret = -ENXIO;
 
+	smmu_pmu_setup_msi(pmu);
+
 	irq = pmu->irq;
 	if (irq)
 		ret = devm_request_irq(pmu->dev, irq, smmu_pmu_handle_irq,
-- 
2.7.4

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Jean-Philippe Brucker]

Hi Shameer,

I have a few comments below, mostly naive since I don't know anything
about perf drivers.

On 21/09/2018 16:08, Shameer Kolothum wrote:
> From: Neil Leeder <nleeder@codeaurora.org>
> 
> Adds a new driver to support the SMMUv3 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 smmuv3_pmcg_<phys_addr_page> where
> <phys_addr_page> is the physical page address of the SMMU PMCG.
> For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840
> 
> 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 smmuv3_pmcg_ff88840/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@codeaurora.org>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/perf/Kconfig          |   9 +
>  drivers/perf/Makefile         |   1 +
>  drivers/perf/arm_smmuv3_pmu.c | 736 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 746 insertions(+)
>  create mode 100644 drivers/perf/arm_smmuv3_pmu.c
> 
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 08ebaf7..34969dd 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_SMMU_V3_PMU
> +	 bool "ARM SMMUv3 Performance Monitors {Extension}"

Why the curly braces? I didn't find that notation in other Kconfig files

> +	 depends on ARM64 && ACPI && ARM_SMMU_V3
> +	   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..f10a932 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
[...]
> +/*
> + * 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 smmuv3_pmcg_<phys_addr_page> where
> + * <phys_addr_page> is the physical page address of the SMMU PMCG.
> + * For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840
> +
> + * 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 smmuv3_pmcg_ff88840/transaction,filter_enable=1,
> + *                       filter_span=1,filter_stream_id=0x42/ -a pwd

I'm curious, why is pwd used as example? Wouldn't something like netperf
be a more realistic workload?

> + * Applies filter pattern 0x42 to transaction events.

Adding that this pattern matches SIDs 0x42 and 0x43 might be helpful,
since span filtering is a bit awkward

[...]
> +#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);                    \

FIELD_GET could make this slightly nicer

> +	}
> +
> +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);

filter_enable is at bit 33, not 34

[...]
> +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;

Cool trick :)

> +
> +	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;

Then this function's return type probably shouldn't be unsigned

> +
> +	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 smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> +	struct device *dev = smmu_pmu->dev;
> +	struct perf_event *sibling;
> +	u32 event_id;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (hwc->sample_period) {
> +		dev_dbg_ratelimited(dev, "Sampling not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (event->cpu < 0) {
> +		dev_dbg_ratelimited(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(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(dev, "Invalid event %d for this PMU\n",
> +				    event_id);
> +		return -EINVAL;
> +	}

[...]
> +static struct attribute *smmu_pmu_events[] = {
> +	SMMU_EVENT_ATTR(cycles, 0),
> +	SMMU_EVENT_ATTR(transaction, 1),
> +	SMMU_EVENT_ATTR(tlb_miss, 2),
> +	SMMU_EVENT_ATTR(config_cache_miss, 3),
> +	SMMU_EVENT_ATTR(trans_table_walk, 4),

This name is a bit misleading - as far as I understand the event doesn't
count table walks, but memory accesses performed during a walk.

> +	SMMU_EVENT_ATTR(config_struct_access, 5),
> +	SMMU_EVENT_ATTR(pcie_ats_trans_rq, 6),
> +	SMMU_EVENT_ATTR(pcie_ats_trans_passed, 7),
> +	NULL
> +};

[...]
> +static int smmu_pmu_setup_irq(struct smmu_pmu *pmu)
> +{
> +	unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;

Why do you need SHARED?

> +	int irq, ret = -ENXIO;
> +
> +	irq = pmu->irq;
> +	if (irq)
> +		ret = devm_request_irq(pmu->dev, irq, smmu_pmu_handle_irq,
> +				       flags, "smmuv3-pmu", pmu);
> +	return ret;
> +}
> +
> +static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
> +{
> +	smmu_pmu_disable(&smmu_pmu->pmu);
> +
> +	/* Disable counter and interrupt */
> +	writeq_relaxed(smmu_pmu->counter_present_mask,
> +		       smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0);
> +	writeq_relaxed(smmu_pmu->counter_present_mask,
> +		       smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0);
> +	writeq_relaxed(smmu_pmu->counter_present_mask,
> +		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
> +
> +	writeq_relaxed(0, smmu_pmu->reg_base + SMMU_PMCG_IRQ_CFG0);

smmu_pmu_setup_msi clears CFG0 a second time, so this line can be
deleted in patch 3, or moved to smmu_pmu_setup_msi right away.

> +}
> +
> +static int smmu_pmu_probe(struct platform_device *pdev)
> +{
> +	struct smmu_pmu *smmu_pmu;
> +	struct resource *res_0, *res_1;
> +	u32 cfgr, reg_size;
> +	u64 ceid_64[2];
> +	int irq, err;
> +	char *name;
> +	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,
> +	};
> +
> +	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);
> +	if (IS_ERR(smmu_pmu->reg_base))
> +		return PTR_ERR(smmu_pmu->reg_base);
> +
> +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> +
> +	/* Determine if page 1 is present */
> +	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
> +		res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		smmu_pmu->reloc_base = devm_ioremap_resource(dev, res_1);
> +		if (IS_ERR(smmu_pmu->reloc_base))
> +			return PTR_ERR(smmu_pmu->reloc_base);
> +	} else {
> +		smmu_pmu->reloc_base = smmu_pmu->reg_base;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq > 0)
> +		smmu_pmu->irq = irq;
> +
> +	ceid_64[0] = readq_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CEID0);
> +	ceid_64[1] = readq_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CEID1);
> +	bitmap_from_arr32(smmu_pmu->supported_events, (u32 *)ceid_64,
> +			  SMMU_ARCH_MAX_EVENT_ID);
> +
> +	smmu_pmu->num_counters = FIELD_GET(SMMU_PMCG_CFGR_NCTR_MASK, cfgr) + 1;
> +	smmu_pmu->counter_present_mask = GENMASK(smmu_pmu->num_counters - 1, 0);
> +
> +	reg_size = FIELD_GET(SMMU_PMCG_CFGR_SIZE_MASK, cfgr);
> +	smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0);
> +
> +	smmu_pmu_reset(smmu_pmu);
> +
> +	err = smmu_pmu_setup_irq(smmu_pmu);
> +	if (err) {
> +		dev_err(dev, "Setup irq failed, PMU @%pa\n", &res_0->start);

You can probably remove "PMU @%pa" from error and info messages, since
the device name already uniquely identifies it:
"[    6.168200] arm-smmu-v3-pmu 2b442000.smmu-pmcg: Registered SMMU PMU
@ 0x000000002b442000 using 4 counters"

Thanks,
Jean

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Jean-Philippe Brucker]

On 02/10/2018 15:11, Jean-Philippe Brucker wrote:
>> +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);

Something I missed previously: when SMMU_PMCG_CFGR.SID_FILTER_TYPE is 1,
filtering for all counters is configured by SMMU_PMCG_SMR0 and
SMMU_PMCG_EVTYPER0 (instead of having one separate filter per counter).

In that mode with your patch, if the user applies a filter to the first
event in the list passed to perf, it will be applied to all events.
Filter applied on any subsequent event will be ignored. Could we make
this more explicit? Maybe in the probe print that the PMCG is
global-filtering, and when attempting to apply a filter to something
else than EVCNTR0, return an error?

Thanks,
Jean

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Robin Murphy]

On 02/10/18 17:19, Jean-Philippe Brucker wrote:
> On 02/10/2018 15:11, Jean-Philippe Brucker wrote:
>>> +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> 
> Something I missed previously: when SMMU_PMCG_CFGR.SID_FILTER_TYPE is 1,
> filtering for all counters is configured by SMMU_PMCG_SMR0 and
> SMMU_PMCG_EVTYPER0 (instead of having one separate filter per counter).

Oh, I hadn't even noticed it had that mode as well...

> In that mode with your patch, if the user applies a filter to the first
> event in the list passed to perf, it will be applied to all events.
> Filter applied on any subsequent event will be ignored. Could we make
> this more explicit? Maybe in the probe print that the PMCG is
> global-filtering, and when attempting to apply a filter to something
> else than EVCNTR0, return an error?

FWIW filtering is always per-counter-group on the SMMUv2 PMU, and it's 
actually pretty straightforward to cope with - pmu->add() just needs to 
reject the event if one with an incompatible configuration is already 
scheduled, so perf core handles it much like having more events than 
counters, by rotating the mutually-exclusive sets.

Robin.

RE: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameerali Kolothum Thodi]

Hi Jean,

> -----Original Message-----
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: 02 October 2018 15:11
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com; robin.murphy@arm.com
> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> neil.m.leeder@gmail.com; pabba@codeaurora.org; John Garry
> <john.garry@huawei.com>; will.deacon@arm.com; rruigrok@codeaurora.org;
> Linuxarm <linuxarm@huawei.com>; linux-kernel@vger.kernel.org; linux-
> acpi@vger.kernel.org; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver
> 
> Hi Shameer,
> 
> I have a few comments below, mostly naive since I don't know anything
> about perf drivers.

Thanks for taking a look at this.

> On 21/09/2018 16:08, Shameer Kolothum wrote:
> > From: Neil Leeder <nleeder@codeaurora.org>
> >
> > Adds a new driver to support the SMMUv3 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 smmuv3_pmcg_<phys_addr_page>
> where
> > <phys_addr_page> is the physical page address of the SMMU PMCG.
> > For example, the PMCG at 0xff88840000 is named smmuv3_pmcg_ff88840
> >
> > 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 smmuv3_pmcg_ff88840/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@codeaurora.org>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/perf/Kconfig          |   9 +
> >  drivers/perf/Makefile         |   1 +
> >  drivers/perf/arm_smmuv3_pmu.c | 736
> ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 746 insertions(+)
> >  create mode 100644 drivers/perf/arm_smmuv3_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 08ebaf7..34969dd 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_SMMU_V3_PMU
> > +	 bool "ARM SMMUv3 Performance Monitors {Extension}"
> 
> Why the curly braces? I didn't find that notation in other Kconfig files

Hmm..That's probably because I just copied a suggestion from previous
review. I will double check and correct it.

> > +	 depends on ARM64 && ACPI && ARM_SMMU_V3
> > +	   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..f10a932 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> [...]
> > +/*
> > + * 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
> smmuv3_pmcg_<phys_addr_page> where
> > + * <phys_addr_page> is the physical page address of the SMMU PMCG.
> > + * For example, the PMCG at 0xff88840000 is named
> smmuv3_pmcg_ff88840
> > +
> > + * 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
> smmuv3_pmcg_ff88840/transaction,filter_enable=1,
> > + *                       filter_span=1,filter_stream_id=0x42/ -a pwd
> 
> I'm curious, why is pwd used as example? Wouldn't something like netperf
> be a more realistic workload?

Agree. That’s a more relevant workload example.

> > + * Applies filter pattern 0x42 to transaction events.
> 
> Adding that this pattern matches SIDs 0x42 and 0x43 might be helpful,
> since span filtering is a bit awkward

Ok.

> [...]
> > +#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);                    \
> 
> FIELD_GET could make this slightly nicer

Sure.

> > +	}
> > +
> > +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);
> 
> filter_enable is at bit 33, not 34

Thanks.
 
> [...]
> > +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;
> 
> Cool trick :)
> 
> > +
> > +	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;
> 
> Then this function's return type probably shouldn't be unsigned

Oops!

> > +
> > +	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 smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> > +	struct device *dev = smmu_pmu->dev;
> > +	struct perf_event *sibling;
> > +	u32 event_id;
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (hwc->sample_period) {
> > +		dev_dbg_ratelimited(dev, "Sampling not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (event->cpu < 0) {
> > +		dev_dbg_ratelimited(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(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)) {
> 
> >= ?

I was slightly confused by the spec here as it says,

Performance events are indicated by a numeric ID, in the following ranges:
• 0x0000 to 0x007F: Architected events
• 0x0080 to 0xFFFF: IMPLEMENTATION DEFINED events

It looks to me the ids are valid including those limits.

> > +		dev_dbg_ratelimited(dev, "Invalid event %d for this PMU\n",
> > +				    event_id);
> > +		return -EINVAL;
> > +	}
> 
> [...]
> > +static struct attribute *smmu_pmu_events[] = {
> > +	SMMU_EVENT_ATTR(cycles, 0),
> > +	SMMU_EVENT_ATTR(transaction, 1),
> > +	SMMU_EVENT_ATTR(tlb_miss, 2),
> > +	SMMU_EVENT_ATTR(config_cache_miss, 3),
> > +	SMMU_EVENT_ATTR(trans_table_walk, 4),
> 
> This name is a bit misleading - as far as I understand the event doesn't
> count table walks, but memory accesses performed during a walk.

Ok. I will take a look at this.

> > +	SMMU_EVENT_ATTR(config_struct_access, 5),
> > +	SMMU_EVENT_ATTR(pcie_ats_trans_rq, 6),
> > +	SMMU_EVENT_ATTR(pcie_ats_trans_passed, 7),
> > +	NULL
> > +};
> 
> [...]
> > +static int smmu_pmu_setup_irq(struct smmu_pmu *pmu)
> > +{
> > +	unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED |
> IRQF_NO_THREAD;
> 
> Why do you need SHARED?

Though I am not aware of any implementation that is sharing this
interrupt, I think it is good to keep it that way as we are anyway
checking for the OVSSET0 status register in the interrupt handler.

> > +	int irq, ret = -ENXIO;
> > +
> > +	irq = pmu->irq;
> > +	if (irq)
> > +		ret = devm_request_irq(pmu->dev, irq,
> smmu_pmu_handle_irq,
> > +				       flags, "smmuv3-pmu", pmu);
> > +	return ret;
> > +}
> > +
> > +static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
> > +{
> > +	smmu_pmu_disable(&smmu_pmu->pmu);
> > +
> > +	/* Disable counter and interrupt */
> > +	writeq_relaxed(smmu_pmu->counter_present_mask,
> > +		       smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0);
> > +	writeq_relaxed(smmu_pmu->counter_present_mask,
> > +		       smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0);
> > +	writeq_relaxed(smmu_pmu->counter_present_mask,
> > +		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
> > +
> > +	writeq_relaxed(0, smmu_pmu->reg_base + SMMU_PMCG_IRQ_CFG0);
> 
> smmu_pmu_setup_msi clears CFG0 a second time, so this line can be
> deleted in patch 3, or moved to smmu_pmu_setup_msi right away.

Ok. I will move this to smmu_pmu_setup_msi.

> > +}
> > +
> > +static int smmu_pmu_probe(struct platform_device *pdev)
> > +{
> > +	struct smmu_pmu *smmu_pmu;
> > +	struct resource *res_0, *res_1;
> > +	u32 cfgr, reg_size;
> > +	u64 ceid_64[2];
> > +	int irq, err;
> > +	char *name;
> > +	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,
> > +	};
> > +
> > +	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);
> > +	if (IS_ERR(smmu_pmu->reg_base))
> > +		return PTR_ERR(smmu_pmu->reg_base);
> > +
> > +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> > +
> > +	/* Determine if page 1 is present */
> > +	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
> > +		res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +		smmu_pmu->reloc_base = devm_ioremap_resource(dev,
> res_1);
> > +		if (IS_ERR(smmu_pmu->reloc_base))
> > +			return PTR_ERR(smmu_pmu->reloc_base);
> > +	} else {
> > +		smmu_pmu->reloc_base = smmu_pmu->reg_base;
> > +	}
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq > 0)
> > +		smmu_pmu->irq = irq;
> > +
> > +	ceid_64[0] = readq_relaxed(smmu_pmu->reg_base +
> SMMU_PMCG_CEID0);
> > +	ceid_64[1] = readq_relaxed(smmu_pmu->reg_base +
> SMMU_PMCG_CEID1);
> > +	bitmap_from_arr32(smmu_pmu->supported_events, (u32 *)ceid_64,
> > +			  SMMU_ARCH_MAX_EVENT_ID);
> > +
> > +	smmu_pmu->num_counters =
> FIELD_GET(SMMU_PMCG_CFGR_NCTR_MASK, cfgr) + 1;
> > +	smmu_pmu->counter_present_mask = GENMASK(smmu_pmu-
> >num_counters - 1, 0);
> > +
> > +	reg_size = FIELD_GET(SMMU_PMCG_CFGR_SIZE_MASK, cfgr);
> > +	smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0);
> > +
> > +	smmu_pmu_reset(smmu_pmu);
> > +
> > +	err = smmu_pmu_setup_irq(smmu_pmu);
> > +	if (err) {
> > +		dev_err(dev, "Setup irq failed, PMU @%pa\n", &res_0->start);
> 
> You can probably remove "PMU @%pa" from error and info messages, since
> the device name already uniquely identifies it:
> "[    6.168200] arm-smmu-v3-pmu 2b442000.smmu-pmcg: Registered SMMU
> PMU
> @ 0x000000002b442000 using 4 counters"

Interesting. What I have is,

[   25.669636] arm-smmu-v3-pmu arm-smmu-v3-pmu.6.auto: Registered SMMU
PMU @ 0x0000000148001000 using 8 counters

Are you using the same patches and is booting using ACPI? IIRC, in the iort
code  it uses the name "arm-smmu-v3-pmu" and AUTO id to register/add the platform
dev. So not sure, how it is printing the address in your case. 

Please check and let me know.

Thanks,
Shameer
 
> Thanks,
> Jean

RE: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 02 October 2018 17:35
> To: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>; Shameerali
> Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com
> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> neil.m.leeder@gmail.com; pabba@codeaurora.org; John Garry
> <john.garry@huawei.com>; will.deacon@arm.com; rruigrok@codeaurora.org;
> Linuxarm <linuxarm@huawei.com>; linux-acpi@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver
> 
> On 02/10/18 17:19, Jean-Philippe Brucker wrote:
> > On 02/10/2018 15:11, Jean-Philippe Brucker wrote:
> >>> +	cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
> >
> > Something I missed previously: when SMMU_PMCG_CFGR.SID_FILTER_TYPE
> is 1,
> > filtering for all counters is configured by SMMU_PMCG_SMR0 and
> > SMMU_PMCG_EVTYPER0 (instead of having one separate filter per counter).
> 
> Oh, I hadn't even noticed it had that mode as well...

Thanks Jean. Missed that completely. 
 
> > In that mode with your patch, if the user applies a filter to the first
> > event in the list passed to perf, it will be applied to all events.
> > Filter applied on any subsequent event will be ignored. Could we make
> > this more explicit? Maybe in the probe print that the PMCG is
> > global-filtering, and when attempting to apply a filter to something
> > else than EVCNTR0, return an error?
> 
> FWIW filtering is always per-counter-group on the SMMUv2 PMU, and it's
> actually pretty straightforward to cope with - pmu->add() just needs to
> reject the event if one with an incompatible configuration is already
> scheduled, so perf core handles it much like having more events than
> counters, by rotating the mutually-exclusive sets.

I will take a look and address this in next version.

Thanks,
Shameer

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Jean-Philippe Brucker]

On 03/10/2018 09:46, Shameerali Kolothum Thodi wrote:
[...]
>>> +	/* 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)) {
>>
>>> = ?
> 
> I was slightly confused by the spec here as it says,
> 
> Performance events are indicated by a numeric ID, in the following ranges:
> • 0x0000 to 0x007F: Architected events
> • 0x0080 to 0xFFFF: IMPLEMENTATION DEFINED events
> 
> It looks to me the ids are valid including those limits.

Yes my mistake, I mixed up IMPDEF_MAX_EVENT_ID which is inclusive with
ARCH_MAX_EVENT_ID which isn't, sorry about that

[...]
>>> +		dev_err(dev, "Setup irq failed, PMU @%pa\n", &res_0->start);
>>
>> You can probably remove "PMU @%pa" from error and info messages, since
>> the device name already uniquely identifies it:
>> "[    6.168200] arm-smmu-v3-pmu 2b442000.smmu-pmcg: Registered SMMU
>> PMU
>> @ 0x000000002b442000 using 4 counters"
> 
> Interesting. What I have is,
> 
> [   25.669636] arm-smmu-v3-pmu arm-smmu-v3-pmu.6.auto: Registered SMMU
> PMU @ 0x0000000148001000 using 8 counters
> 
> Are you using the same patches and is booting using ACPI? IIRC, in the iort
> code  it uses the name "arm-smmu-v3-pmu" and AUTO id to register/add the platform
> dev. So not sure, how it is printing the address in your case. 
> 
> Please check and let me know.

Right, I've been using device tree for my tests, not ACPI. I thought it
was the core platform code that was creating the names. I guess we could
add nicer names to IORT but that's probably for a different series, so
nevermind.

Thanks,
Jean

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Robin Murphy]

On 03/10/18 10:46, Jean-Philippe Brucker wrote:
> On 03/10/2018 09:46, Shameerali Kolothum Thodi wrote:
> [...]
>>>> +	/* 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)) {
>>>
>>>> = ?
>>
>> I was slightly confused by the spec here as it says,
>>
>> Performance events are indicated by a numeric ID, in the following ranges:
>> • 0x0000 to 0x007F: Architected events
>> • 0x0080 to 0xFFFF: IMPLEMENTATION DEFINED events
>>
>> It looks to me the ids are valid including those limits.
> 
> Yes my mistake, I mixed up IMPDEF_MAX_EVENT_ID which is inclusive with
> ARCH_MAX_EVENT_ID which isn't, sorry about that

Well, really that's a clear sign that this could be improved - having 
"max" mean both "maximum" and "maximum plus one" in the same context is 
needlessly overcomplicated.

> [...]
>>>> +		dev_err(dev, "Setup irq failed, PMU @%pa\n", &res_0->start);
>>>
>>> You can probably remove "PMU @%pa" from error and info messages, since
>>> the device name already uniquely identifies it:
>>> "[    6.168200] arm-smmu-v3-pmu 2b442000.smmu-pmcg: Registered SMMU
>>> PMU
>>> @ 0x000000002b442000 using 4 counters"
>>
>> Interesting. What I have is,
>>
>> [   25.669636] arm-smmu-v3-pmu arm-smmu-v3-pmu.6.auto: Registered SMMU
>> PMU @ 0x0000000148001000 using 8 counters
>>
>> Are you using the same patches and is booting using ACPI? IIRC, in the iort
>> code  it uses the name "arm-smmu-v3-pmu" and AUTO id to register/add the platform
>> dev. So not sure, how it is printing the address in your case.
>>
>> Please check and let me know.
> 
> Right, I've been using device tree for my tests, not ACPI. I thought it
> was the core platform code that was creating the names. I guess we could
> add nicer names to IORT but that's probably for a different series, so
> nevermind.

TBH, I can't see anyone caring much about the platform device itself, 
since it's the perf pmu device that users actually want to interact 
with. As long as the latter is easily identifiable, the name of the 
former is mostly only relevant to us sanity-checking sysfs and 
/proc/{interrupts,iomem} for development.

Robin.

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Robin Murphy]

On 21/09/18 16:08, Shameer Kolothum wrote:
[...]
> +
> +	err = cpuhp_state_add_instance_nocalls(cpuhp_state_num,
> +					       &smmu_pmu->node);

In theory a hotplug event could happen as soon as the instance is 
registered...

> +	if (err) {
> +		dev_err(dev, "Error %d registering hotplug, PMU @%pa\n",
> +			err, &res_0->start);
> +		return err;
> +	}
> +
> +	/* Pick one CPU to be the preferred one to use */
> +	smmu_pmu->on_cpu = get_cpu();

...so this looks too late, i.e. a race here can result in a bogus call 
to perf_pmu_migrate_context() with an uninitialised pmu.

Robin.

> +	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
> +
> +	err = perf_pmu_register(&smmu_pmu->pmu, name, -1);
> +	if (err) {
> +		dev_err(dev, "Error %d registering PMU @%pa\n",
> +			err, &res_0->start);
> +		goto out_unregister;
> +	}
> +
> +	put_cpu();

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[John Garry]

On 21/09/2018 16:08, Shameer Kolothum wrote:
> +
> +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);
> +}

Is there some reason PMU drivers have their own edition of this 
function? I see a few similar ones here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/perf_event.c#n309

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/perf/core-book3s.c#n1986

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/kernel/perf_event.c#n239

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/qcom_l2_pmu.c#n731

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/qcom_l3_pmu.c#n655

That is apart from the event id width format.

But the leading zeroes seem to be cropped anyway in the perf tool, 
according to this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/pmu.c#n335 
(this was added in 0c24d6fb7db, in June 18)

John

> +
> +#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, 0),
> +	SMMU_EVENT_ATTR(transaction, 1),
> +	SMMU_EVENT_ATTR(tlb_miss, 2),
> +	SMMU_EVENT_ATTR(config_cache_miss, 3),
> +	SMMU_EVENT_ATTR(trans_table_walk, 4),
> +	SMMU_EVENT_ATTR(config_struct_access, 5),
> +	SMMU_EVENT_ATTR(pcie_ats_trans_rq, 6),
> +	SMMU_EVENT_ATTR(pcie_ats_trans_passed, 7),
> +	NULL
> +};

RE: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 03 October 2018 11:37
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com
> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> neil.m.leeder@gmail.com
> Subject: Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver
> 
> On 21/09/18 16:08, Shameer Kolothum wrote:
> [...]
> > +
> > +	err = cpuhp_state_add_instance_nocalls(cpuhp_state_num,
> > +					       &smmu_pmu->node);
> 
> In theory a hotplug event could happen as soon as the instance is
> registered...
> 
> > +	if (err) {
> > +		dev_err(dev, "Error %d registering hotplug, PMU @%pa\n",
> > +			err, &res_0->start);
> > +		return err;
> > +	}
> > +
> > +	/* Pick one CPU to be the preferred one to use */
> > +	smmu_pmu->on_cpu = get_cpu();
> 
> ...so this looks too late, i.e. a race here can result in a bogus call
> to perf_pmu_migrate_context() with an uninitialised pmu.

Thanks Robin. I will reorder them.

Shameer

> Robin. 
> > +	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu-
> >on_cpu)));
> > +
> > +	err = perf_pmu_register(&smmu_pmu->pmu, name, -1);
> > +	if (err) {
> > +		dev_err(dev, "Error %d registering PMU @%pa\n",
> > +			err, &res_0->start);
> > +		goto out_unregister;
> > +	}
> > +
> > +	put_cpu();

Re: [PATCH v3 1/3] acpi: arm64: add iort support for PMCG

[Lorenzo Pieralisi]

On Fri, Sep 21, 2018 at 04:08:01PM +0100, Shameer Kolothum wrote:
> From: Neil Leeder <nleeder@codeaurora.org>
> 
> Add support for the SMMU Performance Monitor Counter Group
> information from ACPI. This is in preparation for its use
> in the SMMUv3 PMU driver.
> 
> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/acpi/arm64/iort.c | 78 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 08f26db..b979c86 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -356,7 +356,8 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>  	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>  		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>  		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> -		    node->type == ACPI_IORT_NODE_SMMU_V3) {
> +		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
> +		    node->type == ACPI_IORT_NODE_PMCG) {
>  			*id_out = map->output_base;
>  			return parent;
>  		}
> @@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct acpi_iort_node *node)
>  		}
>  
>  		return smmu->id_mapping_index;
> +	case ACPI_IORT_NODE_PMCG:
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1309,6 +1312,50 @@ static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
>  	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
>  }
>  
> +static void __init arm_smmu_common_dma_configure(struct device *dev,
> +						enum dev_dma_attr attr)
> +{
> +	/* We expect the dma masks to be equivalent for all SMMUs set-ups */
> +	dev->dma_mask = &dev->coherent_dma_mask;
> +
> +	/* Configure DMA for the page table walker */
> +	acpi_dma_configure(dev, attr);
> +}

It looks like we can't get rid of this acpi_dma_configure() call
given that the platform device we create has no ACPI companion
(and I am not looking forward to fabricating one to make the
code homogeneous :)).

Still, having two methods per IORT node type (dev_is_coherent() and
dev_dma_configure()) does not make much sense, we can merge it into one
I think.

Thanks,
Lorenzo

> +static int __init arm_smmu_v3_pmcg_count_resources(struct acpi_iort_node *node)
> +{
> +	struct acpi_iort_pmcg *pmcg;
> +
> +	/* Retrieve PMCG specific data */
> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> +
> +	/*
> +	 * There are always 2 memory resources.
> +	 * If the overflow_gsiv is present then add that for a total of 3.
> +	 */
> +	return pmcg->overflow_gsiv ? 3 : 2;
> +}
> +
> +static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
> +					       struct acpi_iort_node *node)
> +{
> +	struct acpi_iort_pmcg *pmcg;
> +
> +	/* Retrieve PMCG specific data */
> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> +
> +	res[0].start = pmcg->page0_base_address;
> +	res[0].end = pmcg->page0_base_address + SZ_4K - 1;
> +	res[0].flags = IORESOURCE_MEM;
> +	res[1].start = pmcg->page1_base_address;
> +	res[1].end = pmcg->page1_base_address + SZ_4K - 1;
> +	res[1].flags = IORESOURCE_MEM;
> +
> +	if (pmcg->overflow_gsiv)
> +		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
> +				       ACPI_EDGE_SENSITIVE, &res[2]);
> +}
> +
>  struct iort_dev_config {
>  	const char *name;
>  	int (*dev_init)(struct acpi_iort_node *node);
> @@ -1318,6 +1365,8 @@ struct iort_dev_config {
>  				     struct acpi_iort_node *node);
>  	void (*dev_set_proximity)(struct device *dev,
>  				    struct acpi_iort_node *node);
> +	void (*dev_dma_configure)(struct device *dev,
> +					enum dev_dma_attr attr);
>  };
>  
>  static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
> @@ -1326,23 +1375,34 @@ static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
>  	.dev_count_resources = arm_smmu_v3_count_resources,
>  	.dev_init_resources = arm_smmu_v3_init_resources,
>  	.dev_set_proximity = arm_smmu_v3_set_proximity,
> +	.dev_dma_configure = arm_smmu_common_dma_configure,
>  };
>  
>  static const struct iort_dev_config iort_arm_smmu_cfg __initconst = {
>  	.name = "arm-smmu",
>  	.dev_is_coherent = arm_smmu_is_coherent,
>  	.dev_count_resources = arm_smmu_count_resources,
> -	.dev_init_resources = arm_smmu_init_resources
> +	.dev_init_resources = arm_smmu_init_resources,
> +	.dev_dma_configure = arm_smmu_common_dma_configure,
> +};
> +
> +static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = {
> +	.name = "arm-smmu-v3-pmu",
> +	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
> +	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
>  };
>  
>  static __init const struct iort_dev_config *iort_get_dev_cfg(
>  			struct acpi_iort_node *node)
>  {
> +
>  	switch (node->type) {
>  	case ACPI_IORT_NODE_SMMU_V3:
>  		return &iort_arm_smmu_v3_cfg;
>  	case ACPI_IORT_NODE_SMMU:
>  		return &iort_arm_smmu_cfg;
> +	case ACPI_IORT_NODE_PMCG:
> +		return &iort_arm_smmu_v3_pmcg_cfg;
>  	default:
>  		return NULL;
>  	}
> @@ -1398,12 +1458,6 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>  	if (ret)
>  		goto dev_put;
>  
> -	/*
> -	 * We expect the dma masks to be equivalent for
> -	 * all SMMUs set-ups
> -	 */
> -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> -
>  	fwnode = iort_get_fwnode(node);
>  
>  	if (!fwnode) {
> @@ -1413,11 +1467,11 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>  
>  	pdev->dev.fwnode = fwnode;
>  
> -	attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
> +	if (ops->dev_dma_configure) {
> +		attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
>  			DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
> -
> -	/* Configure DMA for the page table walker */
> -	acpi_dma_configure(&pdev->dev, attr);
> +		ops->dev_dma_configure(&pdev->dev, attr);
> +	}
>  
>  	iort_set_device_domain(&pdev->dev, node);
>  
> -- 
> 2.7.4
> 
> 

Re: [PATCH v3 1/3] acpi: arm64: add iort support for PMCG

[Robin Murphy]

On 04/10/18 17:43, Lorenzo Pieralisi wrote:
> On Fri, Sep 21, 2018 at 04:08:01PM +0100, Shameer Kolothum wrote:
>> From: Neil Leeder <nleeder@codeaurora.org>
>>
>> Add support for the SMMU Performance Monitor Counter Group
>> information from ACPI. This is in preparation for its use
>> in the SMMUv3 PMU driver.
>>
>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   drivers/acpi/arm64/iort.c | 78 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 66 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 08f26db..b979c86 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -356,7 +356,8 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>>   	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>>   		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>>   		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>> -		    node->type == ACPI_IORT_NODE_SMMU_V3) {
>> +		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
>> +		    node->type == ACPI_IORT_NODE_PMCG) {
>>   			*id_out = map->output_base;
>>   			return parent;
>>   		}
>> @@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct acpi_iort_node *node)
>>   		}
>>   
>>   		return smmu->id_mapping_index;
>> +	case ACPI_IORT_NODE_PMCG:
>> +		return 0;
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -1309,6 +1312,50 @@ static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
>>   	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
>>   }
>>   
>> +static void __init arm_smmu_common_dma_configure(struct device *dev,
>> +						enum dev_dma_attr attr)
>> +{
>> +	/* We expect the dma masks to be equivalent for all SMMUs set-ups */
>> +	dev->dma_mask = &dev->coherent_dma_mask;
>> +
>> +	/* Configure DMA for the page table walker */
>> +	acpi_dma_configure(dev, attr);
>> +}
> 
> It looks like we can't get rid of this acpi_dma_configure() call
> given that the platform device we create has no ACPI companion
> (and I am not looking forward to fabricating one to make the
> code homogeneous :)).

Yeah, given that this is essentially only for SMMUs, the alternatives 
all end up looking like too much bother to be worthwhile.

> Still, having two methods per IORT node type (dev_is_coherent() and
> dev_dma_configure()) does not make much sense, we can merge it into one
> I think.

Good point - looks the attr from dev_is_coherent is only ever passed 
through dev_dma_configure, so we may as well just have per-SMMU-type 
dev_dma_configure methods which retrieve their own relevant coherency 
directly. FWIW, on v2 I was tempted to suggest just wrapping the DMA 
setup in "if (node->type != ACPI_IORT_NODE_PMCG)..." rather than messing 
with more callbacks, but that clearly wouldn't fit well with the local 
style here.

Robin.

> 
> Thanks,
> Lorenzo
> 
>> +static int __init arm_smmu_v3_pmcg_count_resources(struct acpi_iort_node *node)
>> +{
>> +	struct acpi_iort_pmcg *pmcg;
>> +
>> +	/* Retrieve PMCG specific data */
>> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
>> +
>> +	/*
>> +	 * There are always 2 memory resources.
>> +	 * If the overflow_gsiv is present then add that for a total of 3.
>> +	 */
>> +	return pmcg->overflow_gsiv ? 3 : 2;
>> +}
>> +
>> +static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
>> +					       struct acpi_iort_node *node)
>> +{
>> +	struct acpi_iort_pmcg *pmcg;
>> +
>> +	/* Retrieve PMCG specific data */
>> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
>> +
>> +	res[0].start = pmcg->page0_base_address;
>> +	res[0].end = pmcg->page0_base_address + SZ_4K - 1;
>> +	res[0].flags = IORESOURCE_MEM;
>> +	res[1].start = pmcg->page1_base_address;
>> +	res[1].end = pmcg->page1_base_address + SZ_4K - 1;
>> +	res[1].flags = IORESOURCE_MEM;
>> +
>> +	if (pmcg->overflow_gsiv)
>> +		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
>> +				       ACPI_EDGE_SENSITIVE, &res[2]);
>> +}
>> +
>>   struct iort_dev_config {
>>   	const char *name;
>>   	int (*dev_init)(struct acpi_iort_node *node);
>> @@ -1318,6 +1365,8 @@ struct iort_dev_config {
>>   				     struct acpi_iort_node *node);
>>   	void (*dev_set_proximity)(struct device *dev,
>>   				    struct acpi_iort_node *node);
>> +	void (*dev_dma_configure)(struct device *dev,
>> +					enum dev_dma_attr attr);
>>   };
>>   
>>   static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
>> @@ -1326,23 +1375,34 @@ static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
>>   	.dev_count_resources = arm_smmu_v3_count_resources,
>>   	.dev_init_resources = arm_smmu_v3_init_resources,
>>   	.dev_set_proximity = arm_smmu_v3_set_proximity,
>> +	.dev_dma_configure = arm_smmu_common_dma_configure,
>>   };
>>   
>>   static const struct iort_dev_config iort_arm_smmu_cfg __initconst = {
>>   	.name = "arm-smmu",
>>   	.dev_is_coherent = arm_smmu_is_coherent,
>>   	.dev_count_resources = arm_smmu_count_resources,
>> -	.dev_init_resources = arm_smmu_init_resources
>> +	.dev_init_resources = arm_smmu_init_resources,
>> +	.dev_dma_configure = arm_smmu_common_dma_configure,
>> +};
>> +
>> +static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = {
>> +	.name = "arm-smmu-v3-pmu",
>> +	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
>> +	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
>>   };
>>   
>>   static __init const struct iort_dev_config *iort_get_dev_cfg(
>>   			struct acpi_iort_node *node)
>>   {
>> +
>>   	switch (node->type) {
>>   	case ACPI_IORT_NODE_SMMU_V3:
>>   		return &iort_arm_smmu_v3_cfg;
>>   	case ACPI_IORT_NODE_SMMU:
>>   		return &iort_arm_smmu_cfg;
>> +	case ACPI_IORT_NODE_PMCG:
>> +		return &iort_arm_smmu_v3_pmcg_cfg;
>>   	default:
>>   		return NULL;
>>   	}
>> @@ -1398,12 +1458,6 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>>   	if (ret)
>>   		goto dev_put;
>>   
>> -	/*
>> -	 * We expect the dma masks to be equivalent for
>> -	 * all SMMUs set-ups
>> -	 */
>> -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> -
>>   	fwnode = iort_get_fwnode(node);
>>   
>>   	if (!fwnode) {
>> @@ -1413,11 +1467,11 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>>   
>>   	pdev->dev.fwnode = fwnode;
>>   
>> -	attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
>> +	if (ops->dev_dma_configure) {
>> +		attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
>>   			DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
>> -
>> -	/* Configure DMA for the page table walker */
>> -	acpi_dma_configure(&pdev->dev, attr);
>> +		ops->dev_dma_configure(&pdev->dev, attr);
>> +	}
>>   
>>   	iort_set_device_domain(&pdev->dev, node);
>>   
>> -- 
>> 2.7.4
>>
>>

RE: [PATCH v3 1/3] acpi: arm64: add iort support for PMCG

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 04 October 2018 18:35
> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> neil.m.leeder@gmail.com
> Subject: Re: [PATCH v3 1/3] acpi: arm64: add iort support for PMCG
> 
> On 04/10/18 17:43, Lorenzo Pieralisi wrote:
> > On Fri, Sep 21, 2018 at 04:08:01PM +0100, Shameer Kolothum wrote:
> >> From: Neil Leeder <nleeder@codeaurora.org>
> >>
> >> Add support for the SMMU Performance Monitor Counter Group
> >> information from ACPI. This is in preparation for its use
> >> in the SMMUv3 PMU driver.
> >>
> >> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> >> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> >> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >>   drivers/acpi/arm64/iort.c | 78
> +++++++++++++++++++++++++++++++++++++++--------
> >>   1 file changed, 66 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> index 08f26db..b979c86 100644
> >> --- a/drivers/acpi/arm64/iort.c
> >> +++ b/drivers/acpi/arm64/iort.c
> >> @@ -356,7 +356,8 @@ static struct acpi_iort_node
> *iort_node_get_id(struct acpi_iort_node *node,
> >>   	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
> >>   		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> >>   		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> >> -		    node->type == ACPI_IORT_NODE_SMMU_V3) {
> >> +		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
> >> +		    node->type == ACPI_IORT_NODE_PMCG) {
> >>   			*id_out = map->output_base;
> >>   			return parent;
> >>   		}
> >> @@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct
> acpi_iort_node *node)
> >>   		}
> >>
> >>   		return smmu->id_mapping_index;
> >> +	case ACPI_IORT_NODE_PMCG:
> >> +		return 0;
> >>   	default:
> >>   		return -EINVAL;
> >>   	}
> >> @@ -1309,6 +1312,50 @@ static bool __init arm_smmu_is_coherent(struct
> acpi_iort_node *node)
> >>   	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
> >>   }
> >>
> >> +static void __init arm_smmu_common_dma_configure(struct device *dev,
> >> +						enum dev_dma_attr attr)
> >> +{
> >> +	/* We expect the dma masks to be equivalent for all SMMUs set-ups */
> >> +	dev->dma_mask = &dev->coherent_dma_mask;
> >> +
> >> +	/* Configure DMA for the page table walker */
> >> +	acpi_dma_configure(dev, attr);
> >> +}
> >
> > It looks like we can't get rid of this acpi_dma_configure() call
> > given that the platform device we create has no ACPI companion
> > (and I am not looking forward to fabricating one to make the
> > code homogeneous :)).
> 
> Yeah, given that this is essentially only for SMMUs, the alternatives
> all end up looking like too much bother to be worthwhile.
> 
> > Still, having two methods per IORT node type (dev_is_coherent() and
> > dev_dma_configure()) does not make much sense, we can merge it into one
> > I think.
> 
> Good point - looks the attr from dev_is_coherent is only ever passed
> through dev_dma_configure, so we may as well just have per-SMMU-type
> dev_dma_configure methods which retrieve their own relevant coherency
> directly. FWIW, on v2 I was tempted to suggest just wrapping the DMA
> setup in "if (node->type != ACPI_IORT_NODE_PMCG)..." rather than messing
> with more callbacks, but that clearly wouldn't fit well with the local
> style here.

Right,  attr is passed to dev_dma_configure only. I will merge the callbacks
as suggested in the next revision.

Thanks,
Shameer

> Robin.
> 
> >
> > Thanks,
> > Lorenzo
> >
> >> +static int __init arm_smmu_v3_pmcg_count_resources(struct
> acpi_iort_node *node)
> >> +{
> >> +	struct acpi_iort_pmcg *pmcg;
> >> +
> >> +	/* Retrieve PMCG specific data */
> >> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> >> +
> >> +	/*
> >> +	 * There are always 2 memory resources.
> >> +	 * If the overflow_gsiv is present then add that for a total of 3.
> >> +	 */
> >> +	return pmcg->overflow_gsiv ? 3 : 2;
> >> +}
> >> +
> >> +static void __init arm_smmu_v3_pmcg_init_resources(struct resource
> *res,
> >> +					       struct acpi_iort_node *node)
> >> +{
> >> +	struct acpi_iort_pmcg *pmcg;
> >> +
> >> +	/* Retrieve PMCG specific data */
> >> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> >> +
> >> +	res[0].start = pmcg->page0_base_address;
> >> +	res[0].end = pmcg->page0_base_address + SZ_4K - 1;
> >> +	res[0].flags = IORESOURCE_MEM;
> >> +	res[1].start = pmcg->page1_base_address;
> >> +	res[1].end = pmcg->page1_base_address + SZ_4K - 1;
> >> +	res[1].flags = IORESOURCE_MEM;
> >> +
> >> +	if (pmcg->overflow_gsiv)
> >> +		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
> >> +				       ACPI_EDGE_SENSITIVE, &res[2]);
> >> +}
> >> +
> >>   struct iort_dev_config {
> >>   	const char *name;
> >>   	int (*dev_init)(struct acpi_iort_node *node);
> >> @@ -1318,6 +1365,8 @@ struct iort_dev_config {
> >>   				     struct acpi_iort_node *node);
> >>   	void (*dev_set_proximity)(struct device *dev,
> >>   				    struct acpi_iort_node *node);
> >> +	void (*dev_dma_configure)(struct device *dev,
> >> +					enum dev_dma_attr attr);
> >>   };
> >>
> >>   static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
> >> @@ -1326,23 +1375,34 @@ static const struct iort_dev_config
> iort_arm_smmu_v3_cfg __initconst = {
> >>   	.dev_count_resources = arm_smmu_v3_count_resources,
> >>   	.dev_init_resources = arm_smmu_v3_init_resources,
> >>   	.dev_set_proximity = arm_smmu_v3_set_proximity,
> >> +	.dev_dma_configure = arm_smmu_common_dma_configure,
> >>   };
> >>
> >>   static const struct iort_dev_config iort_arm_smmu_cfg __initconst = {
> >>   	.name = "arm-smmu",
> >>   	.dev_is_coherent = arm_smmu_is_coherent,
> >>   	.dev_count_resources = arm_smmu_count_resources,
> >> -	.dev_init_resources = arm_smmu_init_resources
> >> +	.dev_init_resources = arm_smmu_init_resources,
> >> +	.dev_dma_configure = arm_smmu_common_dma_configure,
> >> +};
> >> +
> >> +static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg
> __initconst = {
> >> +	.name = "arm-smmu-v3-pmu",
> >> +	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
> >> +	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
> >>   };
> >>
> >>   static __init const struct iort_dev_config *iort_get_dev_cfg(
> >>   			struct acpi_iort_node *node)
> >>   {
> >> +
> >>   	switch (node->type) {
> >>   	case ACPI_IORT_NODE_SMMU_V3:
> >>   		return &iort_arm_smmu_v3_cfg;
> >>   	case ACPI_IORT_NODE_SMMU:
> >>   		return &iort_arm_smmu_cfg;
> >> +	case ACPI_IORT_NODE_PMCG:
> >> +		return &iort_arm_smmu_v3_pmcg_cfg;
> >>   	default:
> >>   		return NULL;
> >>   	}
> >> @@ -1398,12 +1458,6 @@ static int __init iort_add_platform_device(struct
> acpi_iort_node *node,
> >>   	if (ret)
> >>   		goto dev_put;
> >>
> >> -	/*
> >> -	 * We expect the dma masks to be equivalent for
> >> -	 * all SMMUs set-ups
> >> -	 */
> >> -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> >> -
> >>   	fwnode = iort_get_fwnode(node);
> >>
> >>   	if (!fwnode) {
> >> @@ -1413,11 +1467,11 @@ static int __init
> iort_add_platform_device(struct acpi_iort_node *node,
> >>
> >>   	pdev->dev.fwnode = fwnode;
> >>
> >> -	attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
> >> +	if (ops->dev_dma_configure) {
> >> +		attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ?
> >>   			DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
> >> -
> >> -	/* Configure DMA for the page table walker */
> >> -	acpi_dma_configure(&pdev->dev, attr);
> >> +		ops->dev_dma_configure(&pdev->dev, attr);
> >> +	}
> >>
> >>   	iort_set_device_domain(&pdev->dev, node);
> >>
> >> --
> >> 2.7.4
> >>
> >>

Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Robin Murphy]

Hi Shameer,

One more thing...

On 21/09/18 16:08, Shameer Kolothum wrote:
[...]
> +static int smmu_pmu_probe(struct platform_device *pdev)
> +{
> +	struct smmu_pmu *smmu_pmu;
> +	struct resource *res_0, *res_1;
> +	u32 cfgr, reg_size;
> +	u64 ceid_64[2];
> +	int irq, err;
> +	char *name;
> +	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,
> +	};
> +
> +	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);

We still need to solve the resource-claiming issue when one (or both) of 
the PMCG pages belongs to the parent device's register space. I recall 
we chucked a few nascent ideas about before; did anyone manage to come 
up with anything concrete?

Robin.

RE: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 11 October 2018 12:26
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com
> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> neil.m.leeder@gmail.com
> Subject: Re: [PATCH v3 2/3] perf: add arm64 smmuv3 pmu driver
> 
> Hi Shameer,
> 
> One more thing...
> 
> On 21/09/18 16:08, Shameer Kolothum wrote:
> [...]
> > +static int smmu_pmu_probe(struct platform_device *pdev)
> > +{
> > +	struct smmu_pmu *smmu_pmu;
> > +	struct resource *res_0, *res_1;
> > +	u32 cfgr, reg_size;
> > +	u64 ceid_64[2];
> > +	int irq, err;
> > +	char *name;
> > +	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,
> > +	};
> > +
> > +	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);
> 
> We still need to solve the resource-claiming issue when one (or both) of
> the PMCG pages belongs to the parent device's register space. I recall
> we chucked a few nascent ideas about before; did anyone manage to come
> up with anything concrete?

Right. We had an early version of an evaluation board where we had this issue, 
but this has been fixed in an updated revision and is not a priority for now. 

Agree that this is an issue as the spec doesn’t forbid using parent SMMU register
space and it looks like not an easy one to solve either. The initial idea was setting
the PMCG as a child dev, but that didn’t help.

I had an off list discussion with Lorenzo on this, but nothing concrete.

Lorenzo, 

Please update if you have any new ideas/thoughts on this.

Thanks,
Shameer

> Robin.