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

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

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.

> 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.
> 

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().
> 

Agreed