On = 12-Jul-2021, at 8:11 AM, Nicholas Piggin <npiggin@gmail.com> = wrote:

Excerpts from Athira Rajeev's message of July 10, 2021 = 12:50 pm:

On 22-Jun-2021, at 4:27 PM, Nicholas Piggin = <npiggin@gmail.com> 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@gmail.com>
---
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] =3D = MMCR0_FC;
+ = if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+ = vcpu->arch.mmcr[0] |=3D MMCR0_PMCCEXT;
+ = vcpu->arch.mmcra =3D MMCRA_BHRB_DISABLE;
+ = }
+
= vcpu->arch.ctrl =3D 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 =3D=3D 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 =E2=80=9Cpower_pmu_disable=E2=80=9D 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.

Basically what I'm trying to achieve is that when the guest = and host
are not running perf, then the registers should match. This = way the host
can test them with mfspr but does not need to fix them = with mtspr.

It's not too important for performance after TM = facility demand faulting
and the nested guest PMU fix means you can = usually just skip that
entirely, but I think it's cleaner. I'll = double check my perf/ changes
it's possible that part should be split = out or is unnecessary.

Sure = Nick

power_pmu_disable already sets = MMCRA_BHRB_DISABLE/MMCR0_FC/MMCR0_PMCCEXT bits.
Hence trying to = understand in which scenario we found this was needed for = power_pmu_enable.

Thanks
Athira

Thanks,
Nick