Re: [PATCH 11/12] perf, x86: implement AMD IBS event configuration

[Robert Richter <robert.richter@amd.com>]

On 19.04.10 15:46:29, Stephane Eranian wrote:
> > +       if (event->hw.sample_period)
> > +               /*
> > +                * The usage of the sample period attribute to
> > +                * calculate the IBS max count value is not yet
> > +                * supported, the max count must be in the raw config
> > +                * value.
> > +                */
> > +               return -ENOSYS;
> > +
> What is the problem with directly using the period here, rejecting
> any value that is off range or with bottom 4 bits set?

Yes, I will create an updated version of this patch.

> > +       if (event->attr.type != PERF_TYPE_RAW)
> > +               /* only raw sample types are supported */
> > +               return -EINVAL;
> > +
> > +       type = get_model_spec_type(event->attr.config);
> > +       switch (type) {
> > +       case MODEL_SPEC_TYPE_IBS_FETCH:
> > +               event->hw.config = IBS_FETCH_CONFIG_MASK & event->attr.config;
> > +               event->hw.idx = X86_PMC_IDX_SPECIAL_IBS_FETCH;
> > +               /*
> > +                * dirty hack, needed for __x86_pmu_enable_event(), we
> > +                * should better change event->hw.config_base into
> > +                * event->hw.config_msr that already includes the index
> > +                */
> > +               event->hw.config_base = MSR_AMD64_IBSFETCHCTL - event->hw.idx;
> > +               break;
> > +       case MODEL_SPEC_TYPE_IBS_OP:
> > +               event->hw.config = IBS_OP_CONFIG_MASK & event->attr.config;
> > +               event->hw.idx = X86_PMC_IDX_SPECIAL_IBS_OP;
> > +               event->hw.config_base = MSR_AMD64_IBSOPCTL - event->hw.idx;
> > +               break;
> 
> IBSOP.cnt_ctl only available from RevC, need to check and reject if older.

Right, for this patch I will modify IBS_OP_CONFIG_MASK to RevB only
bits, later I will add cpuid detection and pmu revision checks in a
separate patch.

> > +static struct event_constraint amd_event_constraints[] =
> > +{
> > +       /*
> > +        * The value for the weight of these constraints is higher
> > +        * than in the unconstrainted case to process ibs after the
> > +        * generic counters in x86_schedule_events().
> > +        */
> > +       __EVENT_CONSTRAINT(0, 1ULL << X86_PMC_IDX_SPECIAL_IBS_FETCH, 0,
> > +                          AMD64_NUM_COUNTERS + 1),
> > +       __EVENT_CONSTRAINT(0, 1ULL << X86_PMC_IDX_SPECIAL_IBS_OP, 0,
> > +                          AMD64_NUM_COUNTERS + 1),
> > +       EVENT_CONSTRAINT_END
> > +};
> > +
> I think you could define EVENT_IBS_CONSTRAINT() and shorten
> the definitions here. You could pass FETCH or OP and the macro
> would do the bit shifting. This is how it's done for fixed counters on Intel.

Ok, I will update this.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com