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:2.361 / U:0.388 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site