> On 12-Jul-2021, at 8:19 AM, Nicholas Piggin <npiggin@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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
