Re: [RFC PATCH 10/43] powerpc/64s: Always set PMU control registers to frozen/disabled when not in use

[Madhavan Srinivasan <maddy@xxxxxxxxxxxxx>]

On 7/14/21 6:09 PM, Nicholas Piggin wrote:
> Excerpts from Nicholas Piggin's message of July 12, 2021 12:41 pm:
> > Excerpts from Athira Rajeev's message of July 10, 2021 12:50 pm:
> > > > On 22-Jun-2021, at 4:27 PM, Nicholas Piggin <npiggin@xxxxxxxxx> wrote:
> > > >
> > > > KVM PMU management code looks for particular frozen/disabled bits in
> > > > the PMU registers so it knows whether it must clear them when coming
> > > > out of a guest or not. Setting this up helps KVM make these optimisations
> > > > without getting confused. Longer term the better approach might be to
> > > > move guest/host PMU switching to the perf subsystem.
> > > >
> > > > Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
> > > > ---
> > > > arch/powerpc/kernel/cpu_setup_power.c | 4 ++--
> > > > arch/powerpc/kernel/dt_cpu_ftrs.c     | 6 +++---
> > > > arch/powerpc/kvm/book3s_hv.c          | 5 +++++
> > > > arch/powerpc/perf/core-book3s.c       | 7 +++++++
> > > > 4 files changed, 17 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/kernel/cpu_setup_power.c b/arch/powerpc/kernel/cpu_setup_power.c
> > > > index a29dc8326622..3dc61e203f37 100644
> > > > --- a/arch/powerpc/kernel/cpu_setup_power.c
> > > > +++ b/arch/powerpc/kernel/cpu_setup_power.c
> > > > @@ -109,7 +109,7 @@ static void init_PMU_HV_ISA207(void)
> > > > static void init_PMU(void)
> > > > {
> > > > 	mtspr(SPRN_MMCRA, 0);
> > > > -	mtspr(SPRN_MMCR0, 0);
> > > > +	mtspr(SPRN_MMCR0, MMCR0_FC);
> > > > 	mtspr(SPRN_MMCR1, 0);
> > > > 	mtspr(SPRN_MMCR2, 0);
> > > > }
> > > > @@ -123,7 +123,7 @@ static void init_PMU_ISA31(void)
> > > > {
> > > > 	mtspr(SPRN_MMCR3, 0);
> > > > 	mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
> > > > -	mtspr(SPRN_MMCR0, MMCR0_PMCCEXT);
> > > > +	mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
> > > > }
> > > >
> > > > /*
> > > > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > > index 0a6b36b4bda8..06a089fbeaa7 100644
> > > > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > > @@ -353,7 +353,7 @@ static void init_pmu_power8(void)
> > > > 	}
> > > >
> > > > 	mtspr(SPRN_MMCRA, 0);
> > > > -	mtspr(SPRN_MMCR0, 0);
> > > > +	mtspr(SPRN_MMCR0, MMCR0_FC);
> > > > 	mtspr(SPRN_MMCR1, 0);
> > > > 	mtspr(SPRN_MMCR2, 0);
> > > > 	mtspr(SPRN_MMCRS, 0);
> > > > @@ -392,7 +392,7 @@ static void init_pmu_power9(void)
> > > > 		mtspr(SPRN_MMCRC, 0);
> > > >
> > > > 	mtspr(SPRN_MMCRA, 0);
> > > > -	mtspr(SPRN_MMCR0, 0);
> > > > +	mtspr(SPRN_MMCR0, MMCR0_FC);
> > > > 	mtspr(SPRN_MMCR1, 0);
> > > > 	mtspr(SPRN_MMCR2, 0);
> > > > }
> > > > @@ -428,7 +428,7 @@ static void init_pmu_power10(void)
> > > >
> > > > 	mtspr(SPRN_MMCR3, 0);
> > > > 	mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
> > > > -	mtspr(SPRN_MMCR0, MMCR0_PMCCEXT);
> > > > +	mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
> > > > }
> > > >
> > > > static int __init feat_enable_pmu_power10(struct dt_cpu_feature *f)
> > > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > > > index 1f30f98b09d1..f7349d150828 100644
> > > > --- a/arch/powerpc/kvm/book3s_hv.c
> > > > +++ b/arch/powerpc/kvm/book3s_hv.c
> > > > @@ -2593,6 +2593,11 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
> > > > #endif
> > > > #endif
> > > > 	vcpu->arch.mmcr[0] = MMCR0_FC;
> > > > +	if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> > > > +		vcpu->arch.mmcr[0] |= MMCR0_PMCCEXT;
> > > > +		vcpu->arch.mmcra = MMCRA_BHRB_DISABLE;
> > > > +	}
> > > > +
> > > > 	vcpu->arch.ctrl = CTRL_RUNLATCH;
> > > > 	/* default to host PVR, since we can't spoof it */
> > > > 	kvmppc_set_pvr_hv(vcpu, mfspr(SPRN_PVR));
> > > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> > > > index 51622411a7cc..e33b29ec1a65 100644
> > > > --- a/arch/powerpc/perf/core-book3s.c
> > > > +++ b/arch/powerpc/perf/core-book3s.c
> > > > @@ -1361,6 +1361,13 @@ static void power_pmu_enable(struct pmu *pmu)
> > > > 		goto out;
> > > >
> > > > 	if (cpuhw->n_events == 0) {
> > > > +		if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> > > > +			mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
> > > > +			mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
> > > > +		} else {
> > > > +			mtspr(SPRN_MMCRA, 0);
> > > > +			mtspr(SPRN_MMCR0, MMCR0_FC);
> > > > +		}
> > >
> > > Hi Nick,
> > >
> > > We are setting these bits in “power_pmu_disable” function. And disable will be called before any event gets deleted/stopped. Can you please help to understand why this is needed in power_pmu_enable path also ?
> > I'll have to go back and check what I needed it for.
> Okay, MMCRA is getting MMCRA_SDAR_MODE_DCACHE set on POWER9, by the looks.
>
> That's not necessarily a problem, but KVM sets MMCRA to 0 to disable
> SDAR updates. So KVM and perf don't agree on what the "correct" value
> for disabled is. Which could be a problem with POWER10 not setting BHRB
> disable before my series.

Nick,

BHRB disable will be only known to P10 guest kernel and in case of
P9 guest kernel it is not known. And for the P10 host and guest,
I guess we set BHRB_DISABLE in cpu_setup code and also in 
power_pmu_disable().
So you areseeing P10 guest having MMCRA as zero during context switch?

Maddy
> I'll get rid of this hunk for now, I expect things won't be exactly clean
> or consistent until the KVM host PMU code is moved into perf/ though.
>
> Thanks,
> Nick