[PATCH] Fix oprofile on hyperthreaded P4's

From: Philippe Elie <phil.el@wanadoo.fr>

- oprofile is currently only profiling one sibling.  Fix that with
  appropriate register settings.

- fix an oops which could occur if the userspace driver were to request a
  non-existent resource.

- in NMI handler counter_config[i].event is accessible from user space so
  user can change the event during profiling by echo xxx >
  /dev/oprofile/event

- event mask was wrong, the bit field is 6 bits length not 5, events
  SSE_INPUT_ASSIST and X87_SIMD_MOVES_UOP was affected by masking high bit of
  event number.

diff --git a/arch/i386/oprofile/op_model_p4.c b/arch/i386/oprofile/op_model_p4.c
index ba00b67b7e2bd2d6e013ff74f32aba7f146a6714..0c3de1990aa5be92751df7b80e79e153f538541e 100644 (file)
--- a/arch/i386/oprofile/op_model_p4.c
+++ b/arch/i386/oprofile/op_model_p4.c
@@ -31,7 +31,6 @@
 #define NUM_CONTROLS_HT2 (NUM_ESCRS_HT2 + NUM_CCCRS_HT2)
 
 static unsigned int num_counters = NUM_COUNTERS_NON_HT;
-static unsigned int num_cccrs = NUM_CCCRS_NON_HT;
 
 
 /* this has to be checked dynamically since the
@@ -40,10 +39,17 @@ static unsigned int num_cccrs = NUM_CCCRS_NON_HT;
 static inline void setup_num_counters(void)
 {
 #ifdef CONFIG_SMP
-       if (smp_num_siblings == 2) {            
+       if (smp_num_siblings == 2)
                num_counters = NUM_COUNTERS_HT2;
-               num_cccrs = NUM_CCCRS_HT2;
-       }
+#endif
+}
+
+static int inline addr_increment(void)
+{
+#ifdef CONFIG_SMP
+       return smp_num_siblings == 2 ? 2 : 1;
+#else
+       return 1;
 #endif
 }
 
@@ -65,7 +71,7 @@ struct p4_event_binding {
 };
 
 /* nb: these CTR_* defines are a duplicate of defines in
-   libop/op_events.c. */
+   event/i386.p4*events. */
 
 
 #define CTR_BPU_0      (1 << 0)
@@ -88,6 +94,17 @@ static struct p4_counter_binding p4_counters [NUM_COUNTERS_NON_HT] = {
        { CTR_IQ_5,    MSR_P4_IQ_PERFCTR5,    MSR_P4_IQ_CCCR5 }
 };
 
+#define NUM_UNUSED_CCCRS       NUM_CCCRS_NON_HT - NUM_COUNTERS_NON_HT
+
+/* All cccr we don't use. */
+static int p4_unused_cccr[NUM_UNUSED_CCCRS] = {
+       MSR_P4_BPU_CCCR1,       MSR_P4_BPU_CCCR3,
+       MSR_P4_MS_CCCR1,        MSR_P4_MS_CCCR3,
+       MSR_P4_FLAME_CCCR1,     MSR_P4_FLAME_CCCR3,
+       MSR_P4_IQ_CCCR0,        MSR_P4_IQ_CCCR1,
+       MSR_P4_IQ_CCCR2,        MSR_P4_IQ_CCCR3
+};
+
 /* p4 event codes in libop/op_event.h are indices into this table. */
 
 static struct p4_event_binding p4_events[NUM_EVENTS] = {
@@ -167,13 +184,13 @@ static struct p4_event_binding p4_events[NUM_EVENTS] = {
        { /* IOQ_ALLOCATION */
                0x06, 0x03, 
                { { CTR_BPU_0, MSR_P4_FSB_ESCR0},
-                 {-1,-1} }
+                 { 0, 0 } }
        },
 
        { /* IOQ_ACTIVE_ENTRIES */
                0x06, 0x1a, 
                { { CTR_BPU_2, MSR_P4_FSB_ESCR1},
-                 {-1,-1} }
+                 { 0, 0 } }
        },
 
        { /* FSB_DATA_ACTIVITY */
@@ -185,13 +202,13 @@ static struct p4_event_binding p4_events[NUM_EVENTS] = {
        { /* BSQ_ALLOCATION */
                0x07, 0x05, 
                { { CTR_BPU_0, MSR_P4_BSU_ESCR0},
-                 {-1,-1} }
+                 { 0, 0 } }
        },
 
        { /* BSQ_ACTIVE_ENTRIES */
                0x07, 0x06,
                { { CTR_BPU_2, MSR_P4_BSU_ESCR1 /* guess */},  
-                 {-1,-1} }
+                 { 0, 0 } }
        },
 
        { /* X87_ASSIST */
@@ -336,7 +353,7 @@ static struct p4_event_binding p4_events[NUM_EVENTS] = {
 #define ESCR_SET_OS_0(escr, os) ((escr) |= (((os) & 1) << 3))
 #define ESCR_SET_USR_1(escr, usr) ((escr) |= (((usr) & 1)))
 #define ESCR_SET_OS_1(escr, os) ((escr) |= (((os) & 1) << 1))
-#define ESCR_SET_EVENT_SELECT(escr, sel) ((escr) |= (((sel) & 0x1f) << 25))
+#define ESCR_SET_EVENT_SELECT(escr, sel) ((escr) |= (((sel) & 0x3f) << 25))
 #define ESCR_SET_EVENT_MASK(escr, mask) ((escr) |= (((mask) & 0xffff) << 9))
 #define ESCR_READ(escr,high,ev,i) do {rdmsr(ev->bindings[(i)].escr_address, (escr), (high));} while (0);
 #define ESCR_WRITE(escr,high,ev,i) do {wrmsr(ev->bindings[(i)].escr_address, (escr), (high));} while (0);
@@ -358,15 +375,11 @@ static struct p4_event_binding p4_events[NUM_EVENTS] = {
 #define CTR_WRITE(l,i) do {wrmsr(p4_counters[(i)].counter_address, -(u32)(l), -1);} while (0);
 #define CTR_OVERFLOW_P(ctr) (!((ctr) & 0x80000000))
 
-/* these access the underlying cccrs 1-18, not the subset of 8 bound to "virtual counters" */
-#define RAW_CCCR_READ(low, high, i) do {rdmsr (MSR_P4_BPU_CCCR0 + (i), (low), (high));} while (0);
-#define RAW_CCCR_WRITE(low, high, i) do {wrmsr (MSR_P4_BPU_CCCR0 + (i), (low), (high));} while (0);
-
 
 /* this assigns a "stagger" to the current CPU, which is used throughout
    the code in this module as an extra array offset, to select the "even"
    or "odd" part of all the divided resources. */
-static inline unsigned int get_stagger(void)
+static unsigned int get_stagger(void)
 {
 #ifdef CONFIG_SMP
        int cpu;
@@ -395,29 +408,33 @@ static void p4_fill_in_addresses(struct op_msrs * const msrs)
        setup_num_counters();
        stag = get_stagger();
 
-       /* the 8 counter registers we pay attention to */
-       for (i = 0; i < num_counters; ++i)
+       /* the counter registers we pay attention to */
+       for (i = 0; i < num_counters; ++i) {
                msrs->counters.addrs[i] = 
                        p4_counters[VIRT_CTR(stag, i)].counter_address;
+       }
+
+       /* FIXME: bad feeling, we don't save the 10 counters we don't use. */
 
        /* 18 CCCR registers */
-       for (i=stag, addr = MSR_P4_BPU_CCCR0;
-            addr <= MSR_P4_IQ_CCCR5; ++i, addr += (1 + stag)) 
+       for (i = 0, addr = MSR_P4_BPU_CCCR0 + stag;
+            addr <= MSR_P4_IQ_CCCR5; ++i, addr += addr_increment()) {
                msrs->controls.addrs[i] = addr;
+       }
        
-       /* 43 ESCR registers */
-       for (addr = MSR_P4_BSU_ESCR0;
-            addr <= MSR_P4_SSU_ESCR0; ++i, addr += (1 + stag)){ 
+       /* 43 ESCR registers in three discontiguous group */
+       for (addr = MSR_P4_BSU_ESCR0 + stag;
+            addr <= MSR_P4_SSU_ESCR0; ++i, addr += addr_increment()) { 
                msrs->controls.addrs[i] = addr;
        }
        
-       for (addr = MSR_P4_MS_ESCR0;
-            addr <= MSR_P4_TC_ESCR1; ++i, addr += (1 + stag)){ 
+       for (addr = MSR_P4_MS_ESCR0 + stag;
+            addr <= MSR_P4_TC_ESCR1; ++i, addr += addr_increment()) { 
                msrs->controls.addrs[i] = addr;
        }
        
-       for (addr = MSR_P4_IX_ESCR0;
-            addr <= MSR_P4_CRU_ESCR3; ++i, addr += (1 + stag)){ 
+       for (addr = MSR_P4_IX_ESCR0 + stag;
+            addr <= MSR_P4_CRU_ESCR3; ++i, addr += addr_increment()) { 
                msrs->controls.addrs[i] = addr;
        }
 
@@ -435,7 +452,7 @@ static void p4_fill_in_addresses(struct op_msrs * const msrs)
 
        } else {
                /* and two copies of the second to the odd thread,
-                  for the 31st and 32nd control registers */
+                  for the 22st and 23nd control registers */
                msrs->controls.addrs[i++] = MSR_P4_CRU_ESCR5;
                msrs->controls.addrs[i++] = MSR_P4_CRU_ESCR5;
        }
@@ -456,7 +473,7 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
        stag = get_stagger();
        
        /* convert from counter *number* to counter *bit* */
-       counter_bit = 1 << ctr;
+       counter_bit = 1 << VIRT_CTR(stag, ctr);
        
        /* find our event binding structure. */
        if (counter_config[ctr].event <= 0 || counter_config[ctr].event > NUM_EVENTS) {
@@ -470,7 +487,7 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
        
        for (i = 0; i < maxbind; i++) {
                if (ev->bindings[i].virt_counter & counter_bit) {
-                       
+
                        /* modify ESCR */
                        ESCR_READ(escr, high, ev, i);
                        ESCR_CLEAR(escr);
@@ -499,6 +516,10 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
                        return;
                }
        }
+
+       printk(KERN_ERR 
+              "oprofile: P4 event code 0x%lx no binding, stag %d ctr %d\n",
+              counter_config[ctr].event, stag, ctr);
 }
 
 
@@ -517,27 +538,35 @@ static void p4_setup_ctrs(struct op_msrs const * const msrs)
                return;
        }
 
-       /* clear all cccrs (including those outside our concern) */
-       for (i = stag ; i < num_cccrs ; i += (1 + stag)) {
-               RAW_CCCR_READ(low, high, i);
+       /* clear the cccrs we will use */
+       for (i = 0 ; i < num_counters ; i++) {
+               rdmsr(p4_counters[VIRT_CTR(stag, i)].cccr_address, low, high);
+               CCCR_CLEAR(low);
+               CCCR_SET_REQUIRED_BITS(low);
+               wrmsr(p4_counters[VIRT_CTR(stag, i)].cccr_address, low, high);
+       }
+
+       /* clear cccrs outside our concern */
+       for (i = stag ; i < NUM_UNUSED_CCCRS ; i += addr_increment()) {
+               rdmsr(p4_unused_cccr[i], low, high);
                CCCR_CLEAR(low);
                CCCR_SET_REQUIRED_BITS(low);
-               RAW_CCCR_WRITE(low, high, i);
+               wrmsr(p4_unused_cccr[i], low, high);
        }
 
-       /* clear all escrs (including those outside out concern) */
+       /* clear all escrs (including those outside our concern) */
        for (addr = MSR_P4_BSU_ESCR0 + stag;
-            addr <= MSR_P4_SSU_ESCR0; addr += (1 + stag)){ 
+            addr <= MSR_P4_SSU_ESCR0; addr += addr_increment()) { 
                wrmsr(addr, 0, 0);
        }
        
        for (addr = MSR_P4_MS_ESCR0 + stag;
-            addr <= MSR_P4_TC_ESCR1; addr += (1 + stag)){ 
+            addr <= MSR_P4_TC_ESCR1; addr += addr_increment()){ 
                wrmsr(addr, 0, 0);
        }
        
        for (addr = MSR_P4_IX_ESCR0 + stag;
-            addr <= MSR_P4_CRU_ESCR3; addr += (1 + stag)){ 
+            addr <= MSR_P4_CRU_ESCR3; addr += addr_increment()){ 
                wrmsr(addr, 0, 0);
        }
 
@@ -576,7 +605,7 @@ static int p4_check_ctrs(unsigned int const cpu,
 
        for (i = 0; i < num_counters; ++i) {
                
-               if (!counter_config[i].event) 
+               if (!reset_value[i]) 
                        continue;
 
                /* 
@@ -606,8 +635,6 @@ static int p4_check_ctrs(unsigned int const cpu,
                        CCCR_CLEAR_OVF(low);
                        CCCR_WRITE(low, high, real);
                        CTR_WRITE(reset_value[i], real);
-                       /* P4 quirk: you have to re-unmask the apic vector */
-                       apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
                }
        }