perf: introduce model specific events and AMD IBS

This implements the perf nmi handler for ibs interrupts. The code was copied from oprofile and should be merged somewhen.

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.c     |   10 ++++
 arch/x86/kernel/cpu/perf_event_amd.c |   87 ++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/perf_event.c +34 -0

index a42d033..8f9674f 100644
 
+static int init_ibs_nmi(void)
+{
+#define IBSCTL_LVTOFFSETVAL		(1 << 8)
+#define IBSCTL				0x1cc
+	struct pci_dev *cpu_cfg;
+	int nodes;
+	u32 value = 0;
+
+	/* per CPU setup */
+	on_each_cpu(apic_init_ibs_nmi_per_cpu, NULL, 1);
+
+	nodes = 0;
+	cpu_cfg = NULL;
+	do {
+		cpu_cfg = pci_get_device(PCI_VENDOR_ID_AMD,
+					 PCI_DEVICE_ID_AMD_10H_NB_MISC,
+					 cpu_cfg);
+		if (!cpu_cfg)
+			break;
+		++nodes;
+		pci_write_config_dword(cpu_cfg, IBSCTL, ibs_eilvt_off
+				       | IBSCTL_LVTOFFSETVAL);
+		pci_read_config_dword(cpu_cfg, IBSCTL, &value);
+		if (value != (ibs_eilvt_off | IBSCTL_LVTOFFSETVAL)) {
+			pci_dev_put(cpu_cfg);
+			printk(KERN_DEBUG "Failed to setup IBS LVT offset, "
+				"IBSCTL = 0x%08x", value);
+			return 1;
+		}
+	} while (1);
+
+	if (!nodes) {
+		printk(KERN_DEBUG "No CPU node configured for IBS");
+		return 1;

arch/x86/kernel/cpu/perf_event_amd.c static const u64 amd_perfmon_event_map[] = · line 170

Quoted 5 lines

arch/x86/kernel/cpu/perf_event_amd.c +5 -0

+/* uninitialize the APIC for the IBS interrupts if needed */
+static void clear_ibs_nmi(void)
+{
+	on_each_cpu(apic_clear_ibs_nmi_per_cpu, NULL, 1);
+}

That on_each_cpu() looks wonky, why isn't this in the hotplug hooks?

Quoted 8 lines · Peter Zijlstra

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* uninitialize the APIC for the IBS interrupts if needed */
+static void clear_ibs_nmi(void)
+{
+	on_each_cpu(apic_clear_ibs_nmi_per_cpu, NULL, 1);
+}

That on_each_cpu() looks wonky, why isn't this in the hotplug hooks?

Right, it should be there. Will update this patch.

From bcb2c4aec6bf09fc7c05f31dde9dacd57b9c679c Mon Sep 17 00:00:00 2001

This patch implements IBS for perf_event. It extends the AMD pmu to handle model specific IBS events.

With the attr.model_spec bit set we can setup hardware events others than generic performance counters. A special PMU 64 bit config value can be passed through the perf_event interface. The concept of PMU model-specific arguments was practiced already in Perfmon2. The type of event (8 bits) is determinded from the config value too, bit 32-39 are reserved for this.

There are two types of IBS events for instruction fetch (IBS_FETCH) and instruction execution (IBS_OP). Both events are implemented as special x86 events. The event allocation is implemented by using special event constraints for ibs. This way, the generic event scheduler can be used to allocate ibs events.

IBS can only be set up with raw (model specific) config values and raw data samples. The event attributes for the syscall should be programmed like this (IBS_FETCH):

       memset(&attr, 0, sizeof(attr));
       attr.type        = PERF_TYPE_RAW;
       attr.sample_type = PERF_SAMPLE_CPU | PERF_SAMPLE_RAW; attr.config      = IBS_FETCH_CONFIG_DEFAULT attr.config     |=
               ((unsigned long long)MODEL_SPEC_TYPE_IBS_FETCH << 32) & MODEL_SPEC_TYPE_MASK;
       attr.model_spec  = 1;

The whole ibs example will be part of libpfm4.

The next patch implements the ibs interrupt handler.

Cc Stephane Eranian <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/include/asm/perf_event.h    |    4 +- arch/x86/kernel/cpu/perf_event.c     |   20 ++++ arch/x86/kernel/cpu/perf_event_amd.c |  161 ++++++++++++++++++++++++++++++++-

 3 files changed, 179 insertions(+), 6 deletions(-)

arch/x86/include/asm/perf_event.h +3 -1

index 9f10215..fd5c326 100644
@@ -102,13 +102,15 @@ union cpuid10_edx {
  #define X86_PMC_IDX_FIXED_BUS_CYCLES                   (X86_PMC_IDX_FIXED + 2)
 
  /*
- * We model BTS tracing as another fixed-mode PMC.
+ * Masks for special PMU features
  *
  * We choose a value in the middle of the fixed event range, since lower
  * values are used by actual fixed events and higher values are used
  * to indicate other overflow conditions in the PERF_GLOBAL_STATUS msr.
  */
  #define X86_PMC_IDX_SPECIAL_BTS                                (X86_PMC_IDX_SPECIAL + 0)
+#define X86_PMC_IDX_SPECIAL_IBS_FETCH                  (X86_PMC_IDX_SPECIAL + 1)
+#define X86_PMC_IDX_SPECIAL_IBS_OP                     (X86_PMC_IDX_SPECIAL + 2)
 
  /* IbsFetchCtl bits/masks */
  #define IBS_FETCH_RAND_EN              (1ULL<<57)

arch/x86/kernel/cpu/perf_event.c +20 -0

index 8f9674f..e2328f4 100644
@@ -184,6 +184,26 @@ union perf_capabilities {
  };
 
  /*
+ * Model specific hardware events
+ *
+ * With the attr.model_spec bit set we can setup hardware events
+ * others than generic performance counters. A special PMU 64 bit
+ * config value can be passed through the perf_event interface. The
+ * concept of PMU model-specific arguments was practiced already in
+ * Perfmon2. The type of event (8 bits) is determinded from the config
+ * value too, bit 32-39 are reserved for this.
+ */
+#define MODEL_SPEC_TYPE_IBS_FETCH      0
+#define MODEL_SPEC_TYPE_IBS_OP         1
+
+#define MODEL_SPEC_TYPE_MASK           (0xFFULL << 32)
+
+static inline int get_model_spec_type(u64 config)
+{
+       return (config & MODEL_SPEC_TYPE_MASK) >> 32;
+}
+
+/*
  * struct x86_pmu - generic x86 pmu
  */
  struct x86_pmu {

arch/x86/kernel/cpu/perf_event_amd.c +4 -0

index 27daead..3dc327c 100644
@@ -2,6 +2,10 @@
 
  #include <linux/pci.h>
 
+#define IBS_FETCH_CONFIG_MASK  (IBS_FETCH_RAND_EN | IBS_FETCH_MAX_CNT)
+#define IBS_OP_CONFIG_MASK     (IBS_OP_CNT_CTL | IBS_OP_MAX_CNT)
+#define AMD64_NUM_COUNTERS     4
+
  static DEFINE_RAW_SPINLOCK(amd_nb_lock);
 

 static __initconst const u64 amd_hw_cache_event_ids

arch/x86/kernel/cpu/perf_event_amd.c +23 -0

@@ -193,6 +197,118 @@ static void release_ibs_hardware(void)
        clear_ibs_nmi();
  }
 
+static inline void amd_pmu_disable_ibs(void)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       u64 val;
+
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_FETCH, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSFETCHCTL, val);
+               val &= ~IBS_FETCH_ENABLE;
+               wrmsrl(MSR_AMD64_IBSFETCHCTL, val);
+       }
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_OP, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSOPCTL, val);
+               val &= ~IBS_OP_ENABLE;
+               wrmsrl(MSR_AMD64_IBSOPCTL, val);
+       }
+}
+
+static inline void amd_pmu_enable_ibs(void)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       u64 val;
+
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_FETCH, cpuc->active_mask)) {

arch/x86/kernel/cpu/perf_event_amd.c +7 -0

+
+static int amd_pmu_ibs_config(struct perf_event *event)
+{
+       int type;
+
+       if (!x86_pmu.ibs)
+               return -ENODEV;

arch/x86/kernel/cpu/perf_event_amd.c +11 -0

+       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

arch/x86/kernel/cpu/perf_event_amd.c +53 -1

+       default:
+               return -ENODEV;
+       }
+
+       return 0;
+}
+
+static inline void __amd_pmu_enable_ibs_event(struct hw_perf_event *hwc)
+{
+       if (hwc->idx == X86_PMC_IDX_SPECIAL_IBS_FETCH)
+               __x86_pmu_enable_event(hwc, IBS_FETCH_ENABLE);
+       else if (hwc->idx == X86_PMC_IDX_SPECIAL_IBS_OP)
+               __x86_pmu_enable_event(hwc, IBS_OP_ENABLE);
+}
+
+static void amd_pmu_disable_all(void)
+{
+       x86_pmu_disable_all();
+       amd_pmu_disable_ibs();
+}
+
+static void amd_pmu_enable_all(int added)
+{
+       x86_pmu_enable_all(added);
+       amd_pmu_enable_ibs();
+}
+
+static void amd_pmu_enable_event(struct perf_event *event)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       struct hw_perf_event *hwc = &event->hw;
+
+       if (!cpuc->enabled)
+               return;
+
+       if (hwc->idx < X86_PMC_IDX_SPECIAL)
+               __x86_pmu_enable_event(hwc, ARCH_PERFMON_EVENTSEL_ENABLE);
+       else
+               __amd_pmu_enable_ibs_event(hwc);
+}
+
  static u64 amd_pmu_event_map(int hw_event)
  {
        return amd_perfmon_event_map[hw_event];

@@ -200,7 +316,12 @@ static u64 amd_pmu_event_map(int hw_event)
 
  static int amd_pmu_hw_config(struct perf_event *event)
  {
-       int ret = x86_pmu_hw_config(event);
+       int ret;
+
+       if (event->attr.model_spec)
+               return amd_pmu_ibs_config(event);
+
+       ret = x86_pmu_hw_config(event);
 
        if (ret)
                return ret;

@@ -214,6 +335,23 @@ static int amd_pmu_hw_config(struct perf_event *event)
  }
 
  /*
+ * AMD64 events - list of special events (IBS)
+ */
+static struct event_constraint amd_event_constraints[] =
+{
+       /*
+        * The value for the weight of these constraints is higher

Quoted 27 lines

arch/x86/kernel/cpu/perf_event_amd.c +27 -0

+static inline void amd_pmu_disable_ibs(void)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       u64 val;
+
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_FETCH, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSFETCHCTL, val);
+               val &= ~IBS_FETCH_ENABLE;
+               wrmsrl(MSR_AMD64_IBSFETCHCTL, val);
+       }
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_OP, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSOPCTL, val);
+               val &= ~IBS_OP_ENABLE;
+               wrmsrl(MSR_AMD64_IBSOPCTL, val);
+       }
+}
+
+static inline void amd_pmu_enable_ibs(void)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       u64 val;
+
+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_FETCH, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSFETCHCTL, val);
+               val |= IBS_FETCH_ENABLE;
+               wrmsrl(MSR_AMD64_IBSFETCHCTL, val);
+       }

Quoted 6 lines

+       if (test_bit(X86_PMC_IDX_SPECIAL_IBS_OP, cpuc->active_mask)) {
+               rdmsrl(MSR_AMD64_IBSOPCTL, val);
+               val |= IBS_OP_ENABLE;
+               wrmsrl(MSR_AMD64_IBSOPCTL, val);
+       }
+}

Aren't you picking up stale state by using read-modify-write here?

Quoted 9 lines

+       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?

Quoted 13 lines

+       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

Quoted 8 lines

+                */
+               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.

Quoted 9 lines

arch/x86/kernel/cpu/perf_event_amd.c +9 -0

+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),

arch/x86/kernel/cpu/perf_event_amd.c static int amd_pmu_hw_config(struct perf_event *event) · line 353

Quoted 5 lines

+       __EVENT_CONSTRAINT(0, 1ULL << X86_PMC_IDX_SPECIAL_IBS_OP, 0,
+                          AMD64_NUM_COUNTERS + 1),
+       EVENT_CONSTRAINT_END

arch/x86/kernel/cpu/perf_event_amd.c +2 -0

+};
+

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.

Quoted 2 lines

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.

Quoted 4 lines · Robert Richter

Quoted 2 lines

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.

Stephane, do you think having the lower 4 bits set is worth an EINVAL? I would rather ignore them since the accuracy is not really necessary compared to a range lets say from 100000 cycles? Otherwise this will make the setup of ibs much more complicated. The check could be moved to userland and generate a waring or so.

Quoted 11 lines · Robert Richter

Quoted 4 lines · Robert Richter

Quoted 2 lines

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.

Stephane, do you think having the lower 4 bits set is worth an EINVAL? I would rather ignore them since the accuracy is not really necessary compared to a range lets say from 100000 cycles? Otherwise this will make the setup of ibs much more complicated. The check could be moved to userland and generate a waring or so.

Explain why you think it would be more complicated? If I recall there is already a function to validate the attrs:
amd_pmu_hw_config().
But may be you are talking about userland setup.

Here is one argument why this might be important. Some people like to know exactly
the sampling period because they use a particular value, like a prime number. You
chopping off the bottom 4 bits could break this logic silently.

Quoted 6 lines · Stephane Eranian

Quoted 5 lines · Robert Richter

Quoted 4 lines · Robert Richter

Quoted 2 lines

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.

From 79738167383838eb93f0388f8b7983fe822b36c4 Mon Sep 17 00:00:00 2001

Except for the sample period IBS can only be set up with raw (model specific) config values and raw data samples. The event attributes for the syscall should be programmed like this (IBS_FETCH):

       memset(&attr, 0, sizeof(attr));
       attr.type        = PERF_TYPE_RAW;
       attr.sample_type = PERF_SAMPLE_CPU | PERF_SAMPLE_RAW; attr.config      = IBS_FETCH_CONFIG_DEFAULT attr.config     |=
               ((unsigned long long)MODEL_SPEC_TYPE_IBS_FETCH << 32) & MODEL_SPEC_TYPE_MASK;
       attr.model_spec  = 1;

 /*
+ * Model specific hardware events
+ *
+ * With the attr.model_spec bit set we can setup hardware events
+ * others than generic performance counters. A special PMU 64 bit
+ * config value can be passed through the perf_event interface. The
+ * concept of PMU model-specific arguments was practiced already in
+ * Perfmon2. The type of event (8 bits) is determinded from the config
+ * value too, bit 32-39 are reserved for this.
+ */

Why do you need model_spec, in addition to your special encoding?

Quoted 10 lines

 /*
+ * Model specific hardware events
+ *
+ * With the attr.model_spec bit set we can setup hardware events
+ * others than generic performance counters. A special PMU 64 bit
+ * config value can be passed through the perf_event interface. The
+ * concept of PMU model-specific arguments was practiced already in
+ * Perfmon2. The type of event (8 bits) is determinded from the config
+ * value too, bit 32-39 are reserved for this.
+ */

Isn't the config field big enough to encode all the information you need? In the kernel, you could check bit 32-39 and based on host CPU determine whether it refers to IBS or is a bogus value. I am trying to figure out what model_spec buys you. I believe RAW does not mean the final value as accepted by HW but a value that must be interpreted by the model-specific code to eventually come up with a raw HW value. In the current code, the RAW value is never passed as is, it is assembled from various bits and pieces incl. attr.config of course.

Quoted 12 lines · Stephane Eranian

Why do you need model_spec, in addition to your special encoding?

Quoted 10 lines

 /*
+ * Model specific hardware events
+ *
+ * With the attr.model_spec bit set we can setup hardware events
+ * others than generic performance counters. A special PMU 64 bit
+ * config value can be passed through the perf_event interface. The
+ * concept of PMU model-specific arguments was practiced already in
+ * Perfmon2. The type of event (8 bits) is determinded from the config
+ * value too, bit 32-39 are reserved for this.
+ */

Quoted 8 lines

Isn't the config field big enough to encode all the information you need? In the kernel, you could check bit 32-39 and based on host CPU determine whether it refers to IBS or is a bogus value. I am trying to figure out what model_spec buys you. I believe RAW does not mean the final value as accepted by HW but a value that must be interpreted by the model-specific code to eventually come up with a raw HW value. In the current code, the RAW value is never passed as is, it is assembled from various bits and pieces incl. attr.config of course.

The raw config value without the model_spec bit set would asume a config value for a generic x86 counter. We could reuse also an unused bit in this config value, but what if this bit will be somewhen used?

Or, it is available on one hw bot not another? So I found it much cleaner to introduce this attribute flag that can be reused on other architectures. Also, you will then have the freedom to implement model specific generic events without using raw_config. As most pmu features are setup with a 64 bit config value, I would prefer to have also the encoding of the model specific event type outside of the config value. A want to be close to the hw register layout without shifting bits back and forth. This may also introduce trouble in the future if we need all 64 bits.

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]

Robert,

Some more comments about model_spec.

Quoted 4 lines

Except for the sample period IBS can only be set up with raw (model specific) config values and raw data samples. The event attributes for the syscall should be programmed like this (IBS_FETCH):

Quoted 9 lines

       memset(&attr, 0, sizeof(attr));
       attr.type        = PERF_TYPE_RAW;
       attr.sample_type = PERF_SAMPLE_CPU | PERF_SAMPLE_RAW; attr.config      = IBS_FETCH_CONFIG_DEFAULT attr.config     |=
               ((unsigned long long)MODEL_SPEC_TYPE_IBS_FETCH << 32) & MODEL_SPEC_TYPE_MASK;
       attr.model_spec  = 1;

Why do you need model_spec, in addition to your special encoding?

Quoted 10 lines

 /*
+ * Model specific hardware events
+ *
+ * With the attr.model_spec bit set we can setup hardware events
+ * others than generic performance counters. A special PMU 64 bit
+ * config value can be passed through the perf_event interface. The
+ * concept of PMU model-specific arguments was practiced already in
+ * Perfmon2. The type of event (8 bits) is determinded from the config
+ * value too, bit 32-39 are reserved for this.
+ */

Isn't the config field big enough to encode all the information you need? In the kernel, you could check bit 32-39 and based on host CPU determine whether it refers to IBS or is a bogus value. I am trying to figure out what model_spec buys you. I believe RAW does not mean the final value as accepted by HW but a value that must be interpreted by the model-specific code to eventually come up with a raw HW value. In the current code, the RAW value is never passed as is, it is assembled from various bits and pieces incl. attr.config of course.

This patch implements code to handle ibs interrupts. If ibs data is available a raw perf_event data sample is created and sent back to the userland. Currently only the data is stored only in the raw data, but this could be extended in a later patch by generating generic event data such as the rip from the ibs sampling data.

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/include/asm/msr-index.h     |    3 ++ arch/x86/kernel/cpu/perf_event_amd.c |   65 +++++++++++++++++++++++++++++++++-

 2 files changed, 67 insertions(+), 1 deletions(-)

arch/x86/include/asm/msr-index.h +0 -0

index bc473ac..a7e4aa5 100644

arch/x86/include/asm/msr-index.h +0 -0

  /* Fam 10h MSRs */
  #define MSR_FAM10H_MMIO_CONF_BASE      0xc0010058

arch/x86/kernel/cpu/perf_event_amd.c +18 -0

index 3dc327c..78b0b34 100644
@@ -283,6 +283,69 @@ static inline void __amd_pmu_enable_ibs_event(struct hw_perf_event *hwc)
                __x86_pmu_enable_event(hwc, IBS_OP_ENABLE);
  }
 
+static int amd_pmu_check_ibs(int idx, unsigned int msr, u64 valid,
+                            u64 reenable, int size, struct pt_regs *iregs)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       struct perf_event *event = cpuc->events[idx];
+       struct perf_sample_data data;
+       struct perf_raw_record raw;
+       struct pt_regs regs;
+       u64 buffer[MSR_AMD64_IBS_SIZE_MAX];
+       u64 *buf = buffer;
+       int i;
+
+       if (!test_bit(idx, cpuc->active_mask))
+               return 0;
+
+       rdmsrl(msr++, *buf);
+       if (!(*buf++ & valid))
+               return 0;

arch/x86/kernel/cpu/perf_event_amd.c +22 -0

+       regs = *iregs; /* later: update ip from ibs sample */
+
+       if (perf_event_overflow(event, 1, &data, &regs))
+               x86_pmu_stop(event);
+       else
+               __x86_pmu_enable_event(&event->hw, reenable);
+
+       return 1;
+}
+
+static int amd_pmu_handle_irq(struct pt_regs *regs)
+{
+       int handled, handled2;
+
+       handled = x86_pmu_handle_irq(regs);
+
+       if (!x86_pmu.ibs)
+               return handled;
+
+       handled2 = 0;
+       handled2 += amd_pmu_check_ibs(X86_PMC_IDX_SPECIAL_IBS_FETCH,
+                                     MSR_AMD64_IBSFETCHCTL, IBS_FETCH_VAL,

arch/x86/include/asm/msr-index.h line 116

Quoted 9 lines

arch/x86/include/asm/msr-index.h +1 -0

index bc473ac..a7e4aa5 100644
@@ -113,6 +113,7 @@
  #define MSR_AMD64_IBSFETCHCTL          0xc0011030
  #define MSR_AMD64_IBSFETCHLINAD                0xc0011031
  #define MSR_AMD64_IBSFETCHPHYSAD       0xc0011032
+#define MSR_AMD64_IBSFETCH_SIZE                3

I would use COUNT instead of size given the unit is registers not bytes.

Quoted 22 lines

arch/x86/include/asm/msr-index.h +22 -0

+static int amd_pmu_check_ibs(int idx, unsigned int msr, u64 valid,
+                            u64 reenable, int size, struct pt_regs *iregs)
+{
+       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+       struct perf_event *event = cpuc->events[idx];
+       struct perf_sample_data data;
+       struct perf_raw_record raw;
+       struct pt_regs regs;
+       u64 buffer[MSR_AMD64_IBS_SIZE_MAX];
+       u64 *buf = buffer;
+       int i;
+
+       if (!test_bit(idx, cpuc->active_mask))
+               return 0;
+
+       rdmsrl(msr++, *buf);
+       if (!(*buf++ & valid))
+               return 0;
+
+       perf_sample_data_init(&data, 0);
+       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
+               for (i = 1; i < size; i++)

Quoted 6 lines

+                       rdmsrl(msr++, *buf++);
+               raw.size = sizeof(u64) * size;
+               raw.data = buffer;
+               data.raw = &raw;
+       }
+

Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

Quoted 18 lines

arch/x86/include/asm/msr-index.h +18 -0

+static int amd_pmu_handle_irq(struct pt_regs *regs)
+{
+       int handled, handled2;
+
+       handled = x86_pmu_handle_irq(regs);
+
+       if (!x86_pmu.ibs)
+               return handled;
+
+       handled2 = 0;
+       handled2 += amd_pmu_check_ibs(X86_PMC_IDX_SPECIAL_IBS_FETCH,
+                                     MSR_AMD64_IBSFETCHCTL, IBS_FETCH_VAL,
+                                     IBS_FETCH_ENABLE, MSR_AMD64_IBSFETCH_SIZE,
+                                     regs);
+       handled2 += amd_pmu_check_ibs(X86_PMC_IDX_SPECIAL_IBS_OP,
+                                     MSR_AMD64_IBSOPCTL, IBS_OP_VAL,
+                                     IBS_OP_ENABLE, MSR_AMD64_IBSOP_SIZE,
+                                     regs);

arch/x86/include/asm/msr-index.h

Quoted 4 lines

arch/x86/include/asm/msr-index.h +4 -0

+
+       if (handled2)
+               inc_irq_stat(apic_perf_irqs);
+

If you have both regular counter intr + IBS you will double-count apic_perf_irqs.
I would do: if (handled2 && !handled) inc_irq_stat().

Quoted 9 lines

+       perf_sample_data_init(&data, 0);
+       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
+               for (i = 1; i < size; i++)
+                       rdmsrl(msr++, *buf++);
+               raw.size = sizeof(u64) * size;
+               raw.data = buffer;
+               data.raw = &raw;
+       }
+

Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

Quoted 9 lines

+       perf_sample_data_init(&data, 0);
+       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
+               for (i = 1; i < size; i++)
+                       rdmsrl(msr++, *buf++);
+               raw.size = sizeof(u64) * size;
+               raw.data = buffer;
+               data.raw = &raw;
+       }
+

Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

Quoted 3 lines

+       regs = *iregs; /* later: update ip from ibs sample */ +
+       if (perf_event_overflow(event, 1, &data, &regs))

Review

Quoted 6 lines

+               x86_pmu_stop(event);
+       else
+               __x86_pmu_enable_event(&event->hw, reenable);

(quoted diff) +3 -0

+
+       return 1;
+}

From 6373951f1c660400650066b73c3bb2f6d232be67 Mon Sep 17 00:00:00 2001

From 6373951f1c660400650066b73c3bb2f6d232be67 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Fri, 7 May 2010 15:49:56 +0200
Subject: [PATCH] perf: fix raw sample size if no sampling data is attached

The header size of a raw sample is not included in the total size of a raw data sample. Thus, if no data is attached the size must be null. In this case a buffer overflow may occur when copying the sampling data.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/perf_event.c +1 -1

index 9dbe8cd..f6ddae9 100644
@@ -3229,7 +3229,7 @@ void perf_output_sample(struct
 perf_output_handle *handle,
                                u32     size;
                                u32     data;
                        } raw = {
-                               .size = sizeof(u32),
+                               .size = 0,
                                .data = 0,
                        };

                        perf_output_put(handle, raw); 

kernel/perf_event.c +0 -0

 			};
 			perf_output_put(handle, raw);

Quoted 9 lines

+       perf_sample_data_init(&data, 0);
+       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
+               for (i = 1; i < size; i++)
+                       rdmsrl(msr++, *buf++);
+               raw.size = sizeof(u64) * size;
+               raw.data = buffer;
+               data.raw = &raw;
+       }
+

Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

From 2427dda67b072f27ecff678f8829b9e2fc537988 Mon Sep 17 00:00:00 2001

Quoted 11 lines · Stephane Eranian

Quoted 9 lines

+       perf_sample_data_init(&data, 0);
+       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
+               for (i = 1; i < size; i++)
+                       rdmsrl(msr++, *buf++);
+               raw.size = sizeof(u64) * size;
+               raw.data = buffer;
+               data.raw = &raw;
+       }
+

Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

When thinking about this I was wondering if it wouldn't make sense to better fix the alignment and move the data buffer to a 64 bit boundary. So take a look at the enclosed RFC patch. Though it breaks the ABI it would solve some problems. I think more than it introduces. Hopefully I found all effected code locations using it.

-Robert

--

From 2427dda67b072f27ecff678f8829b9e2fc537988 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Fri, 7 May 2010 15:32:45 +0200
Subject: [PATCH] perf: align raw sample data on 64-bit boundaries

In the current implementation 64 bit raw sample data values are not aligned due to the 32 bit size header. The size header is located before the data buffer on a 64 bit boundary. This leads to unaligned memory access to the data buffer for arrays or structures containing 64 bit values. To avoid this, the patch adds a 32 bit reserved value to the raw sample data header. The data buffer starts then at a 64 bit boundary.

This changes the ABI and requires changes in the userland tools. For tools/perf this is at a single location in event.c only. This could also introduce some overhead on smaller architectures, but currently this is only used on x86 or for transferring raw tracepoint data.

Though an ABI change should be avoided, it is worth to align raw sample data on 64-bit boundaries as the change fixes unaligned memory access, eases the implementation for raw sample data and reduces the risk of data corruption due to different pad structures inserted by the compiler.

Signed-off-by Robert Richter <[email protected]>

---

Quoted 5 lines

This changes the ABI and requires changes in the userland tools. For tools/perf this is at a single location in event.c only. This could also introduce some overhead on smaller architectures, but currently this is only used on x86 or for transferring raw tracepoint data.

No this is used on any architectures that event have a minimal support for perf events.

I use tracepoint raw samples in sparc 64 for example (which has much more than the minimal support).

I don't think we should do this. Ok it's true we've screwed up something there but breaking the ABI is going to make the things even worst I think.

I would feel better with a new PERF_SAMPLE_RAW_ALIGNED sample_type and schedule the deprecation of PERF_SAMPLE_RAW for later but keep it for some releases.

Quoted 10 lines · Frederic Weisbecker

Quoted 5 lines

This changes the ABI and requires changes in the userland tools. For tools/perf this is at a single location in event.c only. This could also introduce some overhead on smaller architectures, but currently this is only used on x86 or for transferring raw tracepoint data.

No this is used on any architectures that event have a minimal support for perf events.

Quoted 2 lines

I use tracepoint raw samples in sparc 64 for example (which has much more than the minimal support).

Isn't here the same alignment problem on archs there unsigned long is 64 bit? Also, most samples I found have a size of multiples of 8 bytes, so even on 32 bit archs there would be a padding of 4 bytes somethere in the sample.

Quoted 3 lines

I don't think we should do this. Ok it's true we've screwed up something there but breaking the ABI is going to make the things even worst I think.

I was not sure how hard an ABI breakage would be. I think the small number of users of raw samples is manageable, but I understand if you feel uncomfortable with it.

Quoted 4 lines

I would feel better with a new PERF_SAMPLE_RAW_ALIGNED sample_type and schedule the deprecation of PERF_SAMPLE_RAW for later but keep it for some releases.

This could be an alternative. Though, it duplicates code paths and introduces ugly sample type checks. Another alternative would be to check the size value, if it is (n * sizeof(u64)) we could asume 64 bit alignment. But maybe this makes things much worse.

-Robert

It doesn't duplicate much code paths, we only have a few corner cases to plug in. And more importantly, that would be temporary if we schedule the older PERF_SAMPLE_RAW in, say, three releases from now.

This ensures an easy forward compatibility (older perf tools -> newer kernel). But the backward compatibility is less easy (newer perf tools -> older kernel) as it means we need to test dynamically if we have PERF_SAMPLE_RAW_ALIGNED, otherwise we need to fall back to using the older one.

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

The patch set triggers this warning in perf_prepare_sample():

WARN_ON_ONCE(size & (sizeof(u64)-1))

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

The patch set triggers this warning in perf_prepare_sample():

WARN_ON_ONCE(size & (sizeof(u64)-1))

Should a raw data sample padded to 64 bit?

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

The general approach is to introduce a flag to mark an event as model specific. With that flag set a model specific ibs (raw) config value can be passed to the pmu for setup. When there are ibs samples available, it is sent back as a raw data sample to the userland. So we have a raw config value and raw sampling data. This requires the userland to setup ibs and further extract and process sampling data.

Patches 1-8 rework and refactor the code to prepare the ibs implementation. This is done in patches 9-12.

I will add ibs example code to libpfm4.

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

I would much rather it uses the ->precise thing PEBS also uses, otherwise we keep growing funny arch extensions and end up with a totally fragmented trainwreck of an ABI.

Quoted 7 lines

The general approach is to introduce a flag to mark an event as model specific. With that flag set a model specific ibs (raw) config value can be passed to the pmu for setup. When there are ibs samples available, it is sent back as a raw data sample to the userland. So we have a raw config value and raw sampling data. This requires the userland to setup ibs and further extract and process sampling data.

Quoted 4 lines

Patches 1-8 rework and refactor the code to prepare the ibs implementation. This is done in patches 9-12.

I will add ibs example code to libpfm4.

Please add a valid usecase to tools/perf/ instead.

Quoted 4 lines · Peter Zijlstra

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

Quoted 3 lines

I would much rather it uses the ->precise thing PEBS also uses, otherwise we keep growing funny arch extensions and end up with a totally fragmented trainwreck of an ABI.

I agree that an exiting flag could be reused. But the naming 'precise' could be misleading. Maybe we rename it to 'model_spec' or something else that underlines the idea of having model specific setups.

Quoted 9 lines · Robert Richter

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

I would much rather it uses the ->precise thing PEBS also uses, otherwise we keep growing funny arch extensions and end up with a totally fragmented trainwreck of an ABI.

Quoted 3 lines

I agree that an exiting flag could be reused. But the naming 'precise' could be misleading. Maybe we rename it to 'model_spec' or something else that underlines the idea of having model specific setups.

Right, so I really hate PERF_SAMPLE_RAW, and I'm considering simply removing that for PEBS as well, its just too ugly. If we want the register set we need to work on getting PERF_SAMPLE_REGS in a sensible shape.

As to the meaning for ->precise, its meant to convey the counters are not affected by skid and the like, I thought IBS provided exact IPs as well (/me should re-read the IBS docs).

The thing with something like ->model_spec and PERF_SAMPLE_RAW is that it doesn't provide a clear model, the user doesn't know what to expect of it, it could be anything.

We want the ABI to express clear concepts, and things like lets bypass everything and just dump stuff out to userspace really are to be avoided at all costs.

Sadly IBS seems to be an utter trainwreck in the concept department (I'm still struggling how to make a sensible interpretation of the data it gathers).

Also not using sample_period for the sample period is of course utterly unacceptable.

Okay, so you're suggesting everything is exposed via PERF_SAMPLE_REGS. PEBS does capture machine state which is easily mapped onto PERF_SAMPLE_REGS. Well, that's until you look at PEB-LL on Nehalem where is captures latencies and data
addresses.

Those could be funneled through PERF_SAMPLE_REGS
as well, I believe. But that means, PERF_SAMPLE_REGS would need some configuration
bitmask to name the registers of interest, e.g. EAX, EDX, IBSOP_DATA, IBSOP_PHYSAD,
and so on.

Quoted 6 lines · Stephane Eranian

Okay, so you're suggesting everything is exposed via PERF_SAMPLE_REGS. PEBS does capture machine state which is easily mapped onto PERF_SAMPLE_REGS. Well, that's until you look at PEB-LL on Nehalem where is captures latencies and data
addresses.

Quoted 6 lines

Those could be funneled through PERF_SAMPLE_REGS
as well, I believe. But that means, PERF_SAMPLE_REGS would need some configuration
bitmask to name the registers of interest, e.g. EAX, EDX, IBSOP_DATA, IBSOP_PHYSAD,
and so on.

What is the idea of PERF_SAMPLE_REGS? A git grep PERF_SAMPLE_REGS only returns a single line in -tip. I know nothing about it.

Quoted 9 lines · Robert Richter

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Robert Richter

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

I would much rather it uses the ->precise thing PEBS also uses, otherwise we keep growing funny arch extensions and end up with a totally fragmented trainwreck of an ABI.

Quoted 3 lines

I agree that an exiting flag could be reused. But the naming 'precise' could be misleading. Maybe we rename it to 'model_spec' or something else that underlines the idea of having model specific setups.

Right, so I really hate PERF_SAMPLE_RAW, and I'm considering simply removing that for PEBS as well, its just too ugly. If we want the register set we need to work on getting PERF_SAMPLE_REGS in a sensible shape.

As to the meaning for ->precise, its meant to convey the counters are not affected by skid and the like, I thought IBS provided exact IPs as well (/me should re-read the IBS docs).

The thing with something like ->model_spec and PERF_SAMPLE_RAW is that it doesn't provide a clear model, the user doesn't know what to expect of it, it could be anything.

We want the ABI to express clear concepts, and things like lets bypass everything and just dump stuff out to userspace really are to be avoided at all costs.

Sadly IBS seems to be an utter trainwreck in the concept department (I'm still struggling how to make a sensible interpretation of the data it gathers).

The thing I absolutely want to avoid is the ABI becoming a fragmented trainwreck like oprofile is.

Also not using sample_period for the sample period is of course utterly unacceptable.

This patch series introduces model specific events and impments AMD IBS (Instruction Based Sampling) for perf_events.

I suggest to apply patches 1, 2, 3, 5, 6 to tip/perf/core if you don't have objections. I will then resubmit a new version of all remaining patches.