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

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

[Akihiko Odaki]

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.

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

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);
+
+	kvm_pmu_set_counter_value(vcpu, idx, val);
+	return 0;
+}
+
 static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 			      struct sys_reg_params *p,
 			      const struct sys_reg_desc *r)
@@ -1328,6 +1344,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 #define PMU_PMEVCNTR_EL0(n)						\
 	{ PMU_SYS_REG(PMEVCNTRn_EL0(n)),				\
 	  .reset = reset_pmevcntr, .get_user = get_pmu_evcntr,		\
+	  .set_user = set_pmu_evcntr,					\
 	  .access = access_pmu_evcntr, .reg = (PMEVCNTR0_EL0 + n), }
 
 /* Macro to expand the PMEVTYPERn_EL0 register */
@@ -2682,7 +2699,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .access = access_pmceid, .reset = NULL },
 	{ PMU_SYS_REG(PMCCNTR_EL0),
 	  .access = access_pmu_evcntr, .reset = reset_unknown,
-	  .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr},
+	  .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr, .set_user = set_pmu_evcntr },
 	{ PMU_SYS_REG(PMXEVTYPER_EL0),
 	  .access = access_pmu_evtyper, .reset = NULL },
 	{ PMU_SYS_REG(PMXEVCNTR_EL0),

---
base-commit: 97eab90817fc214843ca8ccd4cdf68b8115ed424
change-id: 20250302-pmc-b90a86af945c

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

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

[Oliver Upton]

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