Re: [PATCH] powerpc, perf: Configure BHRB filter before enabling PMU interrupts

[Michael Ellerman <michael@ellerman.id.au>]

On Thu, Oct 10, 2013 at 02:20:22PM +0530, Anshuman Khandual wrote:
> On 10/09/2013 11:33 AM, Michael Ellerman wrote:
> > On Wed, Oct 09, 2013 at 10:16:32AM +0530, Anshuman Khandual wrote:
> >> On 10/09/2013 06:51 AM, Michael Ellerman wrote:
> >>> On Tue, Oct 08, 2013 at 12:51:18PM +0530, Anshuman Khandual wrote:
> >>>> On 10/08/2013 09:51 AM, Michael Ellerman wrote:
> >>>>> On Mon, Oct 07, 2013 at 10:00:26AM +0530, Anshuman Khandual wrote:
> >>>>>> Right now the `config_bhrb` PMU specific call happens after write_mmcr0
> >>>>>> which actually enables the PMU for event counting and interrupt. So
> >>>>>> there is a small window of time where the PMU and BHRB runs without the
> >>>>>> required HW branch filter (if any) enabled in BHRB. This can cause some
> >>>>>> of the branch samples to be collected through BHRB without any filter
> >>>>>> being applied and hence affecting the correctness of the results. This
> >>>>>> patch moves the BHRB config function call before enabling the interrupts.
> >>>>>
> >>>>> Patch looks good.
> >>>>>
> >>>>> But it reminds me I have an item in my TODO list:
> >>>>>  - "Why can't config_bhrb() be done in compute_mmcr()" ?
> >>>>>
> >>>>
> >>>> compute_mmcr() function deals with generic MMCR* configs for normal PMU
> >>>> events. Even if BHRB config touches MMCRA register, it's configuration
> >>>> does not interfere with the PMU config for general events. So its best
> >>>> to keep them separate. 
> >>>
> >>> I'm unconvinced. If they'd been together to begin with this bug never
> >>> would have happened.
> >>
> >> This is an ordering of configuration problem. Putting them together in the
> >> same function does not rule out the chances of this ordering problem. Could
> >> you please kindly explain how this could have been avoided ?
> > 
> > The existing code already makes sure to write MMCRA before MMCR0.
> 
> Thats not true. One example being here at power_pmu_enable function.
>
>         write_mmcr0(cpuhw, mmcr0);
> 
>         /*
>          * Enable instruction sampling if necessary
>          */
>         if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE) {
>                 mb();
>                 mtspr(SPRN_MMCRA, cpuhw->mmcr[2]);
>         }

The only example.

The BHRB config would have been applied prior to that:

	mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
	mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
	mtspr(SPRN_MMCR0, (cpuhw->mmcr[0] & ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
				| MMCR0_FC);


So as I said, if the BHRB config was in cpuhw->mmcr[2] then the ordering would
have been correct.

> Even I think this is not right. Instruction sampling should have been
> enabled before we enable PMU interrupts. Else there is a small window
> of time where we could have the PMU enabled with events (which requires
> sampling) without the sampling itself being enabled in MMCRA.

Yes I agree. That's a separate bug, which we'll need to test on all the book3s
platforms we have perf support for.

cheers