[PATCH 2/8] powerpc/perf: Rework disable logic in pmu_disable()

[Sukadev Bhattiprolu]

Anshuman Khandual [khandual at linux.vnet.ibm.com] wrote:
| On 06/24/2013 04:58 PM, Michael Ellerman wrote:
| > In pmu_disable() we disable the PMU by setting the FC (Freeze Counters)
| > bit in MMCR0. In order to do this we have to read/modify/write MMCR0.
| > 
| > It's possible that we read a value from MMCR0 which has PMAO (PMU Alert
| > Occurred) set. When we write that value back it will cause an interrupt
| > to occur. We will then end up in the PMU interrupt handler even though
| > we are supposed to have just disabled the PMU.
| > 
| 
| Is that possible ? First of all MMCR0[PMAO] could not be written by SW.
| Even if you try writing it, how its going to generate PMU interrupt ?
| HW sets this bit MMCR0[PMAO] after a PMU interrupt has already occurred
| not that if we set this, a PMU interrupt would be generated.

Looks like writing 1 MMCR0[PMAO] is allowed (to save interrupts across
partition swaps) and it does generate the interrupt.

| 
| > We can avoid this by making sure we never write PMAO back. We should not
| 
| Making sure that we dont write PMAO back is a good idea though.
| 
| > lose interrupts because when the PMU is re-enabled the overflowed values
| > will cause another interrupt.


Is it enough to set the FC and clear the PMAO - or should we also clear the
PMAE in pmu_disable() (and set it back in pmu_enable()) ?

The PMU spec says "...Alert will occur when enabled condition or event exists
and Performance Monitor Alerts are enabled through MMCR0[PMAE] field"

The condition of overflowing counter will still exist and the PMAE is still
set. So, won't the PMU simply turn PMAO back on after we clear it ?

Or is it that PMAO is only set when counting is enabled but the interrupt is
generated even when counting is disabled ?
| > 
| 
| I doubt this theory.
| 
| > We also reorder the clearing of SAMPLE_ENABLE so that is done after the
| > PMU is frozen. Otherwise there is a small window between the clearing of
| > SAMPLE_ENABLE and the setting of FC where we could take an interrupt and
| > incorrectly see SAMPLE_ENABLE not set. This would for example change the
| > logic in perf_read_regs().
| > 
Good point.