Re: [PATCH] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs

[Oliver Upton <oliver.upton@linux.dev>]

Hi Akihiko,

On Sun, Mar 02, 2025 at 05:12:54PM +0900, Akihiko Odaki wrote:
> Reset the current perf event when setting the vPMU counter (vPMC)
> registers (PMCCNTR_EL0 and PMEVCNTR<n>_EL0). This is a change
> corresponding to commit 9228b26194d1 ("KVM: arm64: PMU: Fix GET_ONE_REG
> for vPMC regs to return the current value") but for SET_ONE_REG.
> 
> Values of vPMC registers are saved in sysreg files on certain occasions.
> These saved values don't represent the current values of the vPMC
> registers if the perf events for the vPMCs count events after the save.
> The current values of those registers are the sum of the sysreg file
> value and the current perf event counter value.  But, when userspace
> writes those registers (using KVM_SET_ONE_REG), KVM only updates the
> sysreg file value and leaves the current perf event counter value as is.

Are you trying to change the PMCs after the VM has started?

> Fix this by calling kvm_pmu_set_counter_value(), which resests the
> current perf event as well.

I'm afraid this could introduce some oddities for save/restore of a VM.
The PMU configuration (e.g. type, event filter, nr event counters) is
subject to change before KVM_RUN.

For example, if the VM programmed an event that was filtered on the
source, KVM could erroneously allocate a perf event on the target if the
filter is restored after the vCPU sysregs.

A similar issue could happen with the PMU type not matching the final
selection as well. Attaching the perf event in KVM_REQ_RELOAD_PMU avoids
these sort of issues.

> Fixes: 051ff581ce70 ("arm64: KVM: Add access handler for event counter register")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 42791971f75887796afab905cc12f49fead39e10..1de990edc6a3e9be2a05a711621bb1bcbeac236a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1035,6 +1035,22 @@ static int get_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	return 0;
>  }
>  
> +static int set_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> +			  u64 val)
> +{
> +	u64 idx;
> +
> +	if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 0)
> +		/* PMCCNTR_EL0 */
> +		idx = ARMV8_PMU_CYCLE_IDX;
> +	else
> +		/* PMEVCNTRn_EL0 */
> +		idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);

nitpick: Let's get rid of the manual decode for both the getter and
setter. r->reg already provides the right info, we just need to
transform that into a counter index.

> +	kvm_pmu_set_counter_value(vcpu, idx, val);
> +	return 0;

WDYT about only calling kvm_pmu_set_counter_value() if the vCPU has
already run once, otherwise update the in-memory value of the register?
I think that would fix your issue while also guaranteeing that the perf
event matches the final configuration of the vPMU.

Thanks,
Oliver