On Mon, 11 Sep 2017, [email protected] wrote:
> - if (event->hw.idx >= UNCORE_PMC_IDX_FIXED)
> + if (event->hw.idx >= UNCORE_PMC_IDX_FREERUNNING)
> + shift = 64 - uncore_free_running_bits(box, event);
> + else if (event->hw.idx == UNCORE_PMC_IDX_FIXED)
I have no idea why the original check was
event->hw.idx >= UNCORE_PMC_IDX_FIXED
and why the new one is
event->hw.idx >= UNCORE_PMC_IDX_FREERUNNING
when you check in the next conditional:
> + if (event->hw.idx == UNCORE_PMC_IDX_FREERUNNING)
> + continue;
This stinks. Anything > UNCORE_PMC_IDX_FREERUNNING is bogus.
> @@ -454,6 +459,12 @@ static void uncore_pmu_event_start(struct perf_event
> *event, int flags)
> struct intel_uncore_box *box = uncore_event_to_box(event);
> int idx = event->hw.idx;
>
> + if (event->hw.idx == UNCORE_PMC_IDX_FREERUNNING) {
> + local64_set(&event->hw.prev_count,
> + uncore_read_counter(box, event));
A comment why this has special treatment would be helpful. Here and in
other places.
> +/*
> + * Free running MSR events have the same event code 0xff as fixed events.
> + * The Free running events umask starts from 0x10.
> + * The umask which is less than 0x10 is reserved for fixed events.
> + *
> + * The Free running events are divided into different types according to
> + * MSR location, bit width or definition. Each type is limited to only have
> + * at most 16 events.
> + * So the umask of first type starts from 0x10, the second starts from 0x20,
> + * the rest can be done in the same manner.
> + */
> +#define UNCORE_FREE_RUNNING_MSR_START 0x10
> +#define UNCORE_FREE_RUNNING_MSR_IDX(config) ((config >> 8) & 0xf)
> +#define UNCORE_FREE_RUNNING_MSR_TYPE_IDX(config) \
> + ((((config >> 8) - UNCORE_FREE_RUNNING_MSR_START) >> 4) & 0xf)
> +
Any reason why these two need to be fugly macros instead of inlines which
simply take an event or an attribute pointer?
> @@ -34,6 +52,7 @@ struct intel_uncore_ops;
> struct intel_uncore_pmu;
> struct intel_uncore_box;
> struct uncore_event_desc;
> +struct free_running_msr;
>
> struct intel_uncore_type {
> const char *name;
> @@ -41,6 +60,7 @@ struct intel_uncore_type {
> int num_boxes;
> int perf_ctr_bits;
> int fixed_ctr_bits;
> + int num_free_running_type;
s/type/types/
> @@ -128,6 +149,13 @@ struct uncore_event_desc {
> const char *config;
> };
>
> +struct free_running_msr {
> + unsigned msr_base;
unsigned int please
> + unsigned msr_off;
> + unsigned num_counters;
> + unsigned bits;
> +};
> +
> struct pci2phy_map {
> struct list_head list;
> int segment;
> @@ -214,6 +242,18 @@ static inline unsigned uncore_msr_fixed_ctr(struct
> intel_uncore_box *box)
> }
> +enum perf_uncore_iio_free_running_msr_type_id {
> + SKX_IIO_MSR_IOCLK = 0,
> + SKX_IIO_MSR_BW = 1,
> + SKX_IIO_MSR_UTIL = 2,
> +
> + SKX_IIO_FREE_RUNNING_MSR_TYPE_MAX,
> +};
Please split the patch in two parts:
1) Add infrastructure for free running counters
2) Add SKX support
Thanks,
tglx
