[RFC PATCH 11/43] KVM: PPC: Book3S HV P9: Implement PMU save/restore in C

[Athira Rajeev]

> On 12-Jul-2021, at 8:19 AM, Nicholas Piggin <npiggin at gmail.com> wrote:
> 
> Excerpts from Athira Rajeev's message of July 10, 2021 12:47 pm:
>> 
>> 
>>> On 22-Jun-2021, at 4:27 PM, Nicholas Piggin <npiggin at gmail.com> wrote:
>>> 
>>> Implement the P9 path PMU save/restore code in C, and remove the
>>> POWER9/10 code from the P7/8 path assembly.
>>> 
>>> -449 cycles (8533) POWER9 virt-mode NULL hcall
>>> 
>>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>>> ---
>>> arch/powerpc/include/asm/asm-prototypes.h |   5 -
>>> arch/powerpc/kvm/book3s_hv.c              | 205 ++++++++++++++++++++--
>>> arch/powerpc/kvm/book3s_hv_interrupts.S   |  13 +-
>>> arch/powerpc/kvm/book3s_hv_rmhandlers.S   |  43 +----
>>> 4 files changed, 200 insertions(+), 66 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
>>> index 02ee6f5ac9fe..928db8ef9a5a 100644
>>> --- a/arch/powerpc/include/asm/asm-prototypes.h
>>> +++ b/arch/powerpc/include/asm/asm-prototypes.h
>>> @@ -136,11 +136,6 @@ static inline void kvmppc_restore_tm_hv(struct kvm_vcpu *vcpu, u64 msr,
>>> 					bool preserve_nv) { }
>>> #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
>>> 
>>> -void kvmhv_save_host_pmu(void);
>>> -void kvmhv_load_host_pmu(void);
>>> -void kvmhv_save_guest_pmu(struct kvm_vcpu *vcpu, bool pmu_in_use);
>>> -void kvmhv_load_guest_pmu(struct kvm_vcpu *vcpu);
>>> -
>>> void kvmppc_p9_enter_guest(struct kvm_vcpu *vcpu);
>>> 
>>> long kvmppc_h_set_dabr(struct kvm_vcpu *vcpu, unsigned long dabr);
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index f7349d150828..b1b94b3563b7 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3635,6 +3635,188 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>>> 	trace_kvmppc_run_core(vc, 1);
>>> }
>>> 
>>> +/*
>>> + * Privileged (non-hypervisor) host registers to save.
>>> + */
>>> +struct p9_host_os_sprs {
>>> +	unsigned long dscr;
>>> +	unsigned long tidr;
>>> +	unsigned long iamr;
>>> +	unsigned long amr;
>>> +	unsigned long fscr;
>>> +
>>> +	unsigned int pmc1;
>>> +	unsigned int pmc2;
>>> +	unsigned int pmc3;
>>> +	unsigned int pmc4;
>>> +	unsigned int pmc5;
>>> +	unsigned int pmc6;
>>> +	unsigned long mmcr0;
>>> +	unsigned long mmcr1;
>>> +	unsigned long mmcr2;
>>> +	unsigned long mmcr3;
>>> +	unsigned long mmcra;
>>> +	unsigned long siar;
>>> +	unsigned long sier1;
>>> +	unsigned long sier2;
>>> +	unsigned long sier3;
>>> +	unsigned long sdar;
>>> +};
>>> +
>>> +static void freeze_pmu(unsigned long mmcr0, unsigned long mmcra)
>>> +{
>>> +	if (!(mmcr0 & MMCR0_FC))
>>> +		goto do_freeze;
>>> +	if (mmcra & MMCRA_SAMPLE_ENABLE)
>>> +		goto do_freeze;
>>> +	if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>>> +		if (!(mmcr0 & MMCR0_PMCCEXT))
>>> +			goto do_freeze;
>>> +		if (!(mmcra & MMCRA_BHRB_DISABLE))
>>> +			goto do_freeze;
>>> +	}
>>> +	return;
>> 
>> 
>> Hi Nick
>> 
>> When freezing the PMU, do we need to also set pmcregs_in_use to zero ?
> 
> Not immediately, we still need to save out the values of the PMU 
> registers. If we clear pmcregs_in_use, then our hypervisor can discard 
> the contents of those registers at any time.
> 
>> Also, why we need these above conditions like MMCRA_SAMPLE_ENABLE,  MMCR0_PMCCEXT checks also before freezing ?
> 
> Basically just because that's the condition we wnat to set the PMU to 
> before entering the guest if the guest does not have its own PMU 
> registers in use.
> 
> I'm not entirely sure this is correct / optimal for perf though, so we
> should double check that. I think some of this stuff should be wrapped 
> up and put into perf/ subdirectory as I've said before. KVM shouldn't 
> need to know about exactly how PMU is to be set up and managed by
> perf, we should just be able to ask perf to save/restore/switch state.

Hi Nick,

Agree to your point. It makes sense that some of these perf code should be moved under perf/ directory.

Athira
> 
> Thanks,
> Nick