Messages in this thread | | | Date | Tue, 12 Jun 2018 15:40:56 +0100 | From | Mark Rutland <> | Subject | Re: [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. >
| |