Re: [PATCH RFC v1 3/3] perf: arm_spe: Add support for SPEv1.2 inverted event filtering

[Rob Herring <robh@kernel.org>]

On Mon, Sep 5, 2022 at 9:55 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 25/08/2022 19:08, Rob Herring wrote:
> > Arm SPEv1.2 (Arm v8.7/v9.2) adds a new feature called Inverted Event
> > Filter which excludes samples matching the event filter. The feature
> > mirrors the existing event filter in PMSEVFR_EL1 adding a new register,
> > PMSNEVFR_EL1, which has the same event bit assignments.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 57904c11aece..9744da888818 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -258,6 +258,7 @@
> >  #define SYS_PMSIDR_EL1_ARCHINST_SHIFT        3
> >  #define SYS_PMSIDR_EL1_LDS_SHIFT     4
> >  #define SYS_PMSIDR_EL1_ERND_SHIFT    5
> > +#define SYS_PMSIDR_EL1_FNE_SHIFT     6
> >  #define SYS_PMSIDR_EL1_INTERVAL_SHIFT        8
> >  #define SYS_PMSIDR_EL1_INTERVAL_MASK 0xfUL
> >  #define SYS_PMSIDR_EL1_MAXSIZE_SHIFT 12
> > @@ -302,6 +303,7 @@
> >  #define SYS_PMSFCR_EL1_FE_SHIFT              0
> >  #define SYS_PMSFCR_EL1_FT_SHIFT              1
> >  #define SYS_PMSFCR_EL1_FL_SHIFT              2
> > +#define SYS_PMSFCR_EL1_FNE_SHIFT     3
> >  #define SYS_PMSFCR_EL1_B_SHIFT               16
> >  #define SYS_PMSFCR_EL1_LD_SHIFT              17
> >  #define SYS_PMSFCR_EL1_ST_SHIFT              18
> > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > index a75b03b5c8f9..724409a88423 100644
> > --- a/drivers/perf/arm_spe_pmu.c
> > +++ b/drivers/perf/arm_spe_pmu.c
> > @@ -82,6 +82,7 @@ struct arm_spe_pmu {
> >  #define SPE_PMU_FEAT_ARCH_INST                       (1UL << 3)
> >  #define SPE_PMU_FEAT_LDS                     (1UL << 4)
> >  #define SPE_PMU_FEAT_ERND                    (1UL << 5)
> > +#define SPE_PMU_FEAT_INV_FILT_EVT            (1UL << 6)
> >  #define SPE_PMU_FEAT_DEV_PROBED                      (1UL << 63)
> >       u64                                     features;
> >
> > @@ -199,6 +200,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
> >  #define ATTR_CFG_FLD_min_latency_LO          0
> >  #define ATTR_CFG_FLD_min_latency_HI          11
> >
> > +#define ATTR_CFG_FLD_inv_event_filter_CFG    config3 /* PMSNEVFR_EL1 */
> > +#define ATTR_CFG_FLD_inv_event_filter_LO     0
> > +#define ATTR_CFG_FLD_inv_event_filter_HI     63
> > +
> >  /* Why does everything I do descend into this? */
> >  #define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)                           \
> >       (lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
> > @@ -229,6 +234,7 @@ GEN_PMU_FORMAT_ATTR(branch_filter);
> >  GEN_PMU_FORMAT_ATTR(load_filter);
> >  GEN_PMU_FORMAT_ATTR(store_filter);
> >  GEN_PMU_FORMAT_ATTR(event_filter);
> > +GEN_PMU_FORMAT_ATTR(inv_event_filter);
> >  GEN_PMU_FORMAT_ATTR(min_latency);
> >
> >  static struct attribute *arm_spe_pmu_formats_attr[] = {
> > @@ -240,12 +246,27 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
> >       &format_attr_load_filter.attr,
> >       &format_attr_store_filter.attr,
> >       &format_attr_event_filter.attr,
> > +     &format_attr_inv_event_filter.attr,
> >       &format_attr_min_latency.attr,
> >       NULL,
> >  };
> >
> > +static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> > +                                               struct attribute *attr,
> > +                                               int unused)
> > +     {
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct arm_spe_pmu *spe_pmu = dev_get_drvdata(dev);
> > +
> > +     if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> > +             return 0;
> > +
> > +     return attr->mode;
> > +}
> > +
> >  static const struct attribute_group arm_spe_pmu_format_group = {
> >       .name   = "format",
> > +     .is_visible = arm_spe_pmu_format_attr_is_visible,
> >       .attrs  = arm_spe_pmu_formats_attr,
> >  };
> >
> > @@ -341,6 +362,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
> >       if (ATTR_CFG_GET_FLD(attr, event_filter))
> >               reg |= BIT(SYS_PMSFCR_EL1_FE_SHIFT);
> >
> > +     if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
> > +             reg |= BIT(SYS_PMSFCR_EL1_FNE_SHIFT);
> > +
> >       if (ATTR_CFG_GET_FLD(attr, min_latency))
> >               reg |= BIT(SYS_PMSFCR_EL1_FL_SHIFT);
> >
> > @@ -353,6 +377,12 @@ static u64 arm_spe_event_to_pmsevfr(struct perf_event *event)
> >       return ATTR_CFG_GET_FLD(attr, event_filter);
> >  }
> >
> > +static u64 arm_spe_event_to_pmsnevfr(struct perf_event *event)
> > +{
> > +     struct perf_event_attr *attr = &event->attr;
> > +     return ATTR_CFG_GET_FLD(attr, inv_event_filter);
> > +}
> > +
> >  static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
> >  {
> >       struct perf_event_attr *attr = &event->attr;
> > @@ -703,6 +733,9 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >       if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> >               return -EOPNOTSUPP;
> >
> > +     if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> > +             return -EOPNOTSUPP;
> > +
> >       if (attr->exclude_idle)
> >               return -EOPNOTSUPP;
> >
> > @@ -721,6 +754,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> >           !(spe_pmu->features & SPE_PMU_FEAT_FILT_EVT))
> >               return -EOPNOTSUPP;
> >
> > +     if ((reg & BIT(SYS_PMSFCR_EL1_FNE_SHIFT)) &&
> > +         !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> > +             return -EOPNOTSUPP;
> > +
> >       if ((reg & BIT(SYS_PMSFCR_EL1_FT_SHIFT)) &&
> >           !(spe_pmu->features & SPE_PMU_FEAT_FILT_TYP))
> >               return -EOPNOTSUPP;
> > @@ -757,6 +794,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
> >       reg = arm_spe_event_to_pmsevfr(event);
> >       write_sysreg_s(reg, SYS_PMSEVFR_EL1);
> >
> > +     reg = arm_spe_event_to_pmsnevfr(event);
> > +     write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
> > +
>
> I think this needs to check if the feature is present before writing
> otherwise you get a crash, pasted below. Otherwise it looks ok to me.

Yes, that's the 1 fix I've needed in my testing.

Rob