lkml.org 
[lkml]   [2018]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC V2 3/3] perf: qcom: Add Falkor CPU PMU IMPLEMENTATION DEFINED event support
Hi,

On Thu, Jun 07, 2018 at 09:56:48AM -0400, Agustin Vega-Frias wrote:
> Selection of these events can be envisioned as indexing them from
> a 3D matrix:
> - the first index selects a Region Event Selection Register (PMRESRx_EL0)
> - the second index selects a group from which only one event at a time
> can be selected
> - the third index selects the event
>
> The event is encoded into perf_event_attr.config as 0xPRCCG, where:
> P [config:16 ] = prefix (flag that indicates a matrix-based event)
> R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
> G [config:0-3 ] = group (specifies the event group)
> CC [config:4-11 ] = code (specifies the event)
>
> Events with the P flag set to zero are treated as common PMUv3 events
> and are directly programmed into PMXEVTYPERx_EL0.
>
> The first two indexes are set combining the RESR and group number with
> a base number and writing it into the architected PMXEVTYPER_EL0 register.
> The third index is set by writing the code into the bits corresponding
> with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
> register.

When are the IMP DEF registers accessible at EL0? Are those goverend by
the same controls as the architected registers?

[...]

> +/*
> + * Qualcomm Technologies CPU PMU IMPLEMENTATION DEFINED extensions support
> + *
> + * Current extensions supported:
> + *
> + * - Matrix-based microarchitectural events support
> + *
> + * Selection of these events can be envisioned as indexing them from
> + * a 3D matrix:
> + * - the first index selects a Region Event Selection Register (PMRESRx_EL0)
> + * - the second index selects a group from which only one event at a time
> + * can be selected
> + * - the third index selects the event
> + *
> + * The event is encoded into perf_event_attr.config as 0xPRCCG, where:
> + * P [config:16 ] = prefix (flag that indicates a matrix-based event)
> + * R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
> + * G [config:0-3 ] = group (specifies the event group)
> + * CC [config:4-11 ] = code (specifies the event)
> + *
> + * Events with the P flag set to zero are treated as common PMUv3 events
> + * and are directly programmed into PMXEVTYPERx_EL0.

When PMUv3 is given a raw event code, the config fields should be the
PMU event number, and this conflicts with RESERVED encodings.

I'd rather we used a separate field for the QC extension events. e.g.
turn config1 into a flags field, and move the P flag there.

We *should* add code to sanity check those fields are zero in the PMUv3
driver, even though it's a potential ABI break to start now.

> + *
> + * The first two indexes are set combining the RESR and group number with
> + * a base number and writing it into the architected PMXEVTYPER_EL0 register.
> + * The third index is set by writing the code into the bits corresponding
> + * with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
> + * register.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/perf/arm_pmu.h>

You'll also need:

#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/perf_event.h>
#include <linux/printk.h>
#include <linux/types.h>

#include <asm/barrier.h>
#include <asm/sysreg.h>

> +
> +#define pmresr0_el0 sys_reg(3, 5, 11, 3, 0)
> +#define pmresr1_el0 sys_reg(3, 5, 11, 3, 2)
> +#define pmresr2_el0 sys_reg(3, 5, 11, 3, 4)
> +#define pmxevcntcr_el0 sys_reg(3, 5, 11, 0, 3)
> +
> +#define QC_EVT_PFX_SHIFT 16
> +#define QC_EVT_REG_SHIFT 12
> +#define QC_EVT_CODE_SHIFT 4
> +#define QC_EVT_GRP_SHIFT 0
> +#define QC_EVT_PFX_MASK GENMASK(QC_EVT_PFX_SHIFT, QC_EVT_PFX_SHIFT)
> +#define QC_EVT_REG_MASK GENMASK(QC_EVT_REG_SHIFT + 3, QC_EVT_REG_SHIFT)
> +#define QC_EVT_CODE_MASK GENMASK(QC_EVT_CODE_SHIFT + 7, QC_EVT_CODE_SHIFT)
> +#define QC_EVT_GRP_MASK GENMASK(QC_EVT_GRP_SHIFT + 3, QC_EVT_GRP_SHIFT)
> +#define QC_EVT_PRG_MASK (QC_EVT_PFX_MASK | QC_EVT_REG_MASK | QC_EVT_GRP_MASK)
> +#define QC_EVT_PRG(event) ((event) & QC_EVT_PRG_MASK)
> +#define QC_EVT_REG(event) (((event) & QC_EVT_REG_MASK) >> QC_EVT_REG_SHIFT)
> +#define QC_EVT_CODE(event) (((event) & QC_EVT_CODE_MASK) >> QC_EVT_CODE_SHIFT)
> +#define QC_EVT_GROUP(event) (((event) & QC_EVT_GRP_MASK) >> QC_EVT_GRP_SHIFT)
> +
> +#define QC_MAX_GROUP 7
> +#define QC_MAX_RESR 2
> +#define QC_BITS_PER_GROUP 8
> +#define QC_RESR_ENABLE BIT_ULL(63)
> +#define QC_RESR_EVT_BASE 0xd8
> +
> +static struct arm_pmu *def_ops;
> +
> +static inline void falkor_write_pmresr(u64 reg, u64 val)
> +{
> + if (reg == 0)
> + write_sysreg_s(val, pmresr0_el0);
> + else if (reg == 1)
> + write_sysreg_s(val, pmresr1_el0);
> + else
> + write_sysreg_s(val, pmresr2_el0);
> +}
> +
> +static inline u64 falkor_read_pmresr(u64 reg)
> +{
> + return (reg == 0 ? read_sysreg_s(pmresr0_el0) :
> + reg == 1 ? read_sysreg_s(pmresr1_el0) :
> + read_sysreg_s(pmresr2_el0));
> +}

Please use switch statements for both of these.

> +
> +static void falkor_set_resr(u64 reg, u64 group, u64 code)
> +{
> + u64 shift = group * QC_BITS_PER_GROUP;
> + u64 mask = GENMASK(shift + QC_BITS_PER_GROUP - 1, shift);
> + u64 val;
> +
> + val = falkor_read_pmresr(reg) & ~mask;
> + val |= (code << shift);
> + val |= QC_RESR_ENABLE;
> + falkor_write_pmresr(reg, val);
> +}
> +
> +static void falkor_clear_resr(u64 reg, u64 group)
> +{
> + u32 shift = group * QC_BITS_PER_GROUP;
> + u64 mask = GENMASK(shift + QC_BITS_PER_GROUP - 1, shift);
> + u64 val = falkor_read_pmresr(reg) & ~mask;
> +
> + falkor_write_pmresr(reg, val == QC_RESR_ENABLE ? 0 : val);
> +}
> +
> +/*
> + * Check if e1 and e2 conflict with each other
> + *
> + * e1 is a matrix-based microarchitectural event we are checking against e2.
> + * A conflict exists if the events use the same reg, group, and a different
> + * code. Events with the same code are allowed because they could be using
> + * different filters (e.g. one to count user space and the other to count
> + * kernel space events).
> + */

What problem occurs when there's a conflict?

Does the filter matter at all? When happens if I open two identical
events, both counting the same reg, group, and code, with the same
filter?

> +static inline int events_conflict(struct perf_event *e1, struct perf_event *e2)
> +{
> + if ((e1 != e2) &&
> + (e1->pmu == e2->pmu) &&
> + (QC_EVT_PRG(e1->attr.config) == QC_EVT_PRG(e2->attr.config)) &&
> + (QC_EVT_CODE(e1->attr.config) != QC_EVT_CODE(e2->attr.config))) {
> + pr_debug_ratelimited(
> + "Group exclusion: conflicting events %llx %llx\n",
> + e1->attr.config,
> + e2->attr.config);
> + return 1;
> + }
> + return 0;
> +}

This would be easier to read as a series of tests:

static inline bool events_conflict(struct perf_event *new,
struct perf_event *other)
{
/* own group leader */
if (new == other)
return false;

/* software events can't conflict */
if (is_sw_event(other))
return false;

/* No conflict if using different reg or group */
if (QC_EVT_PRG(new->attr.config) != QC_EVT_PRG(other->attr.config))
return false;

/* Same reg and group is fine so long as code matches */
if (QC_EVT_CODE(new->attr.config) == QC_EVT_PRG(other->attr.config)
return false;


pr_debug_ratelimited("Group exclusion: conflicting events %llx %llx\n",
new->attr.config, other->attr.config);

return true;

}

> +
> +/*
> + * Check if the given event is valid for the PMU and if so return the value
> + * that can be used in PMXEVTYPER_EL0 to select the event
> + */
> +static int falkor_map_event(struct perf_event *event)
> +{
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + struct perf_event *leader;
> + struct perf_event *sibling;
> +
> + if (!(event->attr.config & QC_EVT_PFX_MASK))
> + /* Common PMUv3 event, forward to the original op */
> + return def_ops->map_event(event);

The QC event codes should only be used when either:

* event->attr.type is PERF_TYPE_RAW, or:

* event->pmu.type is this PMU's dynamic type

... otherwise this will accept events meant for other PMUs, and/or
override conflicting events in other type namespaces (e.g.
PERF_EVENT_TYPE_HW, PERF_EVENT_TYPE_CACHE).

> +
> + /* Is it a valid matrix event? */
> + if ((group > QC_MAX_GROUP) || (reg > QC_MAX_RESR))
> + return -ENOENT;
> +
> + /* If part of an event group, check if the event can be put in it */
> +
> + leader = event->group_leader;
> + if (events_conflict(event, leader))
> + return -ENOENT;
> +
> + for_each_sibling_event(sibling, leader)
> + if (events_conflict(event, sibling))
> + return -ENOENT;
> +
> + return QC_RESR_EVT_BASE + reg*8 + group;

Nit: spacing around binary operators please.

> +}
> +
> +/*
> + * Find a slot for the event on the current CPU
> + */
> +static int falkor_get_event_idx(struct pmu_hw_events *cpuc, struct perf_event *event)
> +{
> + int idx;
> +
> + if (!!(event->attr.config & QC_EVT_PFX_MASK))

The '!!' isn't required.

> + /* Matrix event, check for conflicts with existing events */
> + for_each_set_bit(idx, cpuc->used_mask, ARMPMU_MAX_HWEVENTS)
> + if (cpuc->events[idx] &&
> + events_conflict(event, cpuc->events[idx]))
> + return -ENOENT;
> +
> + /* Let the original op handle the rest */
> + idx = def_ops->get_event_idx(cpuc, event);

Same comments as for falkor_map_event().

> +
> + /*
> + * This is called for actually allocating the events, but also with
> + * a dummy pmu_hw_events when validating groups, for that case we
> + * need to ensure that cpuc->events[idx] is NULL so we don't use
> + * an uninitialized pointer. Conflicts for matrix events in groups
> + * are checked during event mapping anyway (see falkor_event_map).
> + */
> + cpuc->events[idx] = NULL;
> +
> + return idx;
> +}
> +
> +/*
> + * Reset the PMU
> + */
> +static void falkor_reset(void *info)
> +{
> + struct arm_pmu *pmu = (struct arm_pmu *)info;
> + u32 i, ctrs = pmu->num_events;
> +
> + /* PMRESRx_EL0 regs are unknown at reset, except for the EN field */
> + for (i = 0; i <= QC_MAX_RESR; i++)
> + falkor_write_pmresr(i, 0);
> +
> + /* PMXEVCNTCRx_EL0 regs are unknown at reset */
> + for (i = 0; i <= ctrs; i++) {
> + write_sysreg(i, pmselr_el0);
> + isb();
> + write_sysreg_s(0, pmxevcntcr_el0);
> + }
> +
> + /* Let the original op handle the rest */
> + def_ops->reset(info);
> +}
> +
> +/*
> + * Enable the given event
> + */
> +static void falkor_enable(struct perf_event *event)
> +{
> + if (!!(event->attr.config & QC_EVT_PFX_MASK)) {

The '!!' isn't required.

> + /* Matrix event, program the appropriate PMRESRx_EL0 */
> + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 code = QC_EVT_CODE(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> + falkor_set_resr(reg, group, code);
> + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);

Why is the spinlock required?

AFACIT this should only ever be called in contexts where IRQs are
disabled already.

> + }
> +
> + /* Let the original op handle the rest */
> + def_ops->enable(event);
> +}
> +
> +/*
> + * Disable the given event
> + */
> +static void falkor_disable(struct perf_event *event)
> +{
> + /* Use the original op to disable the counter and interrupt */
> + def_ops->enable(event);
> +
> + if (!!(event->attr.config & QC_EVT_PFX_MASK)) {
> + /* Matrix event, de-program the appropriate PMRESRx_EL0 */
> + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> + falkor_clear_resr(reg, group);
> + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> + }
> +}

Same comments as with falkor_enable().

> +
> +PMU_FORMAT_ATTR(event, "config:0-15");
> +PMU_FORMAT_ATTR(prefix, "config:16");
> +PMU_FORMAT_ATTR(reg, "config:12-15");
> +PMU_FORMAT_ATTR(code, "config:4-11");
> +PMU_FORMAT_ATTR(group, "config:0-3");

What sort of events are available? Do you plan to add anything to the
userspace event database in tools/perf/pmu-events/ ?

> +
> +static struct attribute *falkor_pmu_formats[] = {
> + &format_attr_event.attr,
> + &format_attr_prefix.attr,
> + &format_attr_reg.attr,
> + &format_attr_code.attr,
> + &format_attr_group.attr,
> + NULL,
> +};
> +
> +static struct attribute_group falkor_pmu_format_attr_group = {
> + .name = "format",
> + .attrs = falkor_pmu_formats,
> +};
> +
> +static int qcom_falkor_pmu_init(struct arm_pmu *pmu, struct device *dev)
> +{
> + /* Save base arm_pmu so we can invoke its ops when appropriate */
> + def_ops = devm_kmemdup(dev, pmu, sizeof(*def_ops), GFP_KERNEL);
> + if (!def_ops) {
> + pr_warn("Failed to allocate arm_pmu for QCOM extensions");
> + return -ENODEV;
> + }
> +
> + pmu->name = "qcom_pmuv3";

All the other CPU PMUs on an ARM ACPI system will have an index suffix,
e.g. "armv8_pmuv3_0". I can see why we might want to change the name to
indicate the QC extensions, but I think we should keep the existing
pattern, with a '_0' suffix here.

> +
> + /* Override the necessary ops */
> + pmu->map_event = falkor_map_event;
> + pmu->get_event_idx = falkor_get_event_idx;
> + pmu->reset = falkor_reset;
> + pmu->enable = falkor_enable;
> + pmu->disable = falkor_disable;

I'm somewhat concerned by hooking into the existing PMU code at this
level, but I don't currently have a better suggestion.

Thanks,
Mark.

> +
> + /* Override the necessary attributes */
> + pmu->pmu.attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
> + &falkor_pmu_format_attr_group;
> +
> + return 1;
> +}
> +
> +ACPI_DECLARE_PMU_VARIANT(qcom_falkor, "QCOM8150", qcom_falkor_pmu_init);
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>

\
 
 \ /
  Last update: 2018-06-12 16:41    [W:0.056 / U:39.192 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site