[PATCH v2 0/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs

[PATCH v2 0/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs

[Akihiko Odaki]

Prepare vPMC registers for user-initiated changes after first run. This
is important specifically for debugging Windows on QEMU with GDB; QEMU
tries to write back all visible registers when resuming the VM execution
with GDB, corrupting the PMU state. Windows always uses the PMU so this
can cause adverse effects on that particular OS.

This series also contains patch "KVM: arm64: PMU: Set raw values from
user to PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}", which reverts semantic
changes made for the mentioned registers in the past. It is necessary
to migrate the PMU state properly on Firecracker, QEMU, and crosvm.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v2:
- Changed to utilize KVM_REQ_RELOAD_PMU as suggested by Oliver Upton.
- Added patch "KVM: arm64: PMU: Reload when user modifies registers"
  to cover more registers.
- Added patch "KVM: arm64: PMU: Set raw values from user to
  PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}".
- Link to v1: https://lore.kernel.org/r/20250302-pmc-v1-1-caff989093dc@daynix.com

---
Akihiko Odaki (3):
      KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs
      KVM: arm64: PMU: Reload when user modifies registers
      KVM: arm64: PMU: Set raw values from user to PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}

 arch/arm64/kvm/pmu-emul.c | 19 ++++++++++++++++---
 arch/arm64/kvm/sys_regs.c | 44 ++++++++++++++++++++++++--------------------
 include/kvm/arm_pmu.h     |  1 +
 3 files changed, 41 insertions(+), 23 deletions(-)
---
base-commit: da2f480cb24d39d480b1e235eda0dd2d01f8765b
change-id: 20250302-pmc-b90a86af945c

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

[PATCH v2 1/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs

[Akihiko Odaki]

Reload the 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 releasing the current perf event and trigger recreating one
with KVM_REQ_RELOAD_PMU.

Fixes: 051ff581ce70 ("arm64: KVM: Add access handler for event counter register")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 arch/arm64/kvm/pmu-emul.c | 16 ++++++++++++++++
 arch/arm64/kvm/sys_regs.c | 20 +++++++++++++++++++-
 include/kvm/arm_pmu.h     |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index e3e82b66e2268d37d5e2630e47ddf085a6846e1c..1402cce5625bffa706aabe5e6121d1f3817a0aaf 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -191,6 +191,22 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
 	kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, select_idx), val, false);
 }
 
+/**
+ * kvm_pmu_set_counter_value_user - set PMU counter value from user
+ * @vcpu: The vcpu pointer
+ * @select_idx: The counter index
+ * @val: The counter value
+ */
+void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
+{
+	if (!kvm_vcpu_has_pmu(vcpu))
+		return;
+
+	kvm_pmu_release_perf_event(kvm_vcpu_idx_to_pmc(vcpu, select_idx));
+	__vcpu_sys_reg(vcpu, counter_index_to_reg(select_idx)) = val;
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+}
+
 /**
  * kvm_pmu_release_perf_event - remove the perf event
  * @pmc: The PMU counter pointer
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 42791971f75887796afab905cc12f49fead39e10..27418dac791df9a89124f867879e899db175e506 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_user(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,8 @@ 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),
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 28b380ad8dfa942c4275e0c7ed3535d309b81b2f..9c062756ebfad5ea555362154459ffe9f8311c6d 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -41,6 +41,7 @@ bool kvm_supports_guest_pmuv3(void);
 #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
+void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
 u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
 u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1);
 void kvm_pmu_vcpu_init(struct kvm_vcpu *vcpu);

-- 
2.48.1

Re: [PATCH v2 1/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs

[Marc Zyngier]

On Fri, 07 Mar 2025 10:55:28 +0000,
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> Reload the 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 releasing the current perf event and trigger recreating one
> with KVM_REQ_RELOAD_PMU.
> 
> Fixes: 051ff581ce70 ("arm64: KVM: Add access handler for event counter register")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 16 ++++++++++++++++
>  arch/arm64/kvm/sys_regs.c | 20 +++++++++++++++++++-
>  include/kvm/arm_pmu.h     |  1 +
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index e3e82b66e2268d37d5e2630e47ddf085a6846e1c..1402cce5625bffa706aabe5e6121d1f3817a0aaf 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -191,6 +191,22 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
>  	kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, select_idx), val, false);
>  }
>  
> +/**
> + * kvm_pmu_set_counter_value_user - set PMU counter value from user
> + * @vcpu: The vcpu pointer
> + * @select_idx: The counter index
> + * @val: The counter value
> + */
> +void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> +{
> +	if (!kvm_vcpu_has_pmu(vcpu))
> +		return;

How can this occur? It seems to me that we only get here from a
register that has .visibility == pmu_visibility().

Or is there another way to reach this function?

> +
> +	kvm_pmu_release_perf_event(kvm_vcpu_idx_to_pmc(vcpu, select_idx));
> +	__vcpu_sys_reg(vcpu, counter_index_to_reg(select_idx)) = val;
> +	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +}
> +
>  /**
>   * kvm_pmu_release_perf_event - remove the perf event
>   * @pmc: The PMU counter pointer
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 42791971f75887796afab905cc12f49fead39e10..27418dac791df9a89124f867879e899db175e506 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_user(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,8 @@ 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),
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 28b380ad8dfa942c4275e0c7ed3535d309b81b2f..9c062756ebfad5ea555362154459ffe9f8311c6d 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -41,6 +41,7 @@ bool kvm_supports_guest_pmuv3(void);
>  #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
>  u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
>  void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
> +void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
>  u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
>  u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1);
>  void kvm_pmu_vcpu_init(struct kvm_vcpu *vcpu);

Other than the nit above, looks good to me.

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH v2 2/3] KVM: arm64: PMU: Reload when user modifies registers

[Akihiko Odaki]

Commit d0c94c49792c ("KVM: arm64: Restore PMU configuration on first
run") added the code to reload the PMU configuration on first run.
Trigger the code when a user modifies a PMU register instead so that
PMU configuration changes made by users will be applied also after the
first run.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 arch/arm64/kvm/pmu-emul.c | 3 ---
 arch/arm64/kvm/sys_regs.c | 5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 1402cce5625bffa706aabe5e6121d1f3817a0aaf..04eb3856b96576fad5afc8927c8916ff9738f9d7 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -888,9 +888,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
-	/* One-off reload of the PMU on first run */
-	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
-
 	return 0;
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 27418dac791df9a89124f867879e899db175e506..51054b7befc0b4bd822cecf717ee4a4740c4a685 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1162,6 +1162,8 @@ static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 va
 	else
 		__vcpu_sys_reg(vcpu, r->reg) &= ~val;
 
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
@@ -1322,6 +1324,8 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 		val |= ARMV8_PMU_PMCR_LC;
 
 	__vcpu_sys_reg(vcpu, r->reg) = val;
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
@@ -4276,6 +4280,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
 	}
 
 	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 }
 
 /**

-- 
2.48.1

Re: [PATCH v2 2/3] KVM: arm64: PMU: Reload when user modifies registers

[Marc Zyngier]

[dropping Shannon  from the list, as his Linaro email has been
bouncing for about 10 years, and updating Andrew's email]

On Fri, 07 Mar 2025 10:55:29 +0000,
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> Commit d0c94c49792c ("KVM: arm64: Restore PMU configuration on first
> run") added the code to reload the PMU configuration on first run.
> Trigger the code when a user modifies a PMU register instead so that
> PMU configuration changes made by users will be applied also after the
> first run.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 3 ---
>  arch/arm64/kvm/sys_regs.c | 5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 1402cce5625bffa706aabe5e6121d1f3817a0aaf..04eb3856b96576fad5afc8927c8916ff9738f9d7 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -888,9 +888,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  		   return -EINVAL;
>  	}
>  
> -	/* One-off reload of the PMU on first run */
> -	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> -
>  	return 0;
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 27418dac791df9a89124f867879e899db175e506..51054b7befc0b4bd822cecf717ee4a4740c4a685 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1162,6 +1162,8 @@ static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 va
>  	else
>  		__vcpu_sys_reg(vcpu, r->reg) &= ~val;
>  
> +	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
>  	return 0;
>  }
>  
> @@ -1322,6 +1324,8 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  		val |= ARMV8_PMU_PMCR_LC;
>  
>  	__vcpu_sys_reg(vcpu, r->reg) = val;
> +	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
>  	return 0;
>  }
>  
> @@ -4276,6 +4280,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>  	}
>  
>  	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);
> +	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);

This should be restricted to cases where a PMU is actually available.

But I also wonder what the motivation is to move this from first run
to reset? Nothing explains it in the commit message.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH v2 3/3] KVM: arm64: PMU: Set raw values from user to PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}

[Akihiko Odaki]

Commit a45f41d754e0 ("KVM: arm64: Add {get,set}_user for
PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}") changed KVM_SET_ONE_REG to update
the mentioned registers in a way matching with the behavior of guest
register writes. This is a breaking change of a UAPI though the new
semantics looks cleaner and VMMs are not prepared for this.

Firecracker, QEMU, and crosvm perform migration by listing registers
with KVM_GET_REG_LIST, getting their values with KVM_GET_ONE_REG and
setting them with KVM_SET_ONE_REG. This algorithm assumes
KVM_SET_ONE_REG restores the values retrieved with KVM_GET_ONE_REG
without any alteration. However, bit operations added by the earlier
commit do not preserve the values retried with KVM_GET_ONE_REG and
potentially break migration.

Remove the bit operations that alter the values retrieved with
KVM_GET_ONE_REG.

Fixes: a45f41d754e0 ("KVM: arm64: Add {get,set}_user for PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 arch/arm64/kvm/sys_regs.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51054b7befc0b4bd822cecf717ee4a4740c4a685..2f44d4d4f54112787683dd75ea93fd60e92dd31f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1142,26 +1142,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 val)
 {
-	bool set;
-
-	val &= kvm_pmu_valid_counter_mask(vcpu);
-
-	switch (r->reg) {
-	case PMOVSSET_EL0:
-		/* CRm[1] being set indicates a SET register, and CLR otherwise */
-		set = r->CRm & 2;
-		break;
-	default:
-		/* Op2[0] being set indicates a SET register, and CLR otherwise */
-		set = r->Op2 & 1;
-		break;
-	}
-
-	if (set)
-		__vcpu_sys_reg(vcpu, r->reg) |= val;
-	else
-		__vcpu_sys_reg(vcpu, r->reg) &= ~val;
-
+	__vcpu_sys_reg(vcpu, r->reg) = val & kvm_pmu_valid_counter_mask(vcpu);
 	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 
 	return 0;

-- 
2.48.1

Re: [PATCH v2 3/3] KVM: arm64: PMU: Set raw values from user to PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}

[Marc Zyngier]

On Fri, 07 Mar 2025 10:55:30 +0000,
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> Commit a45f41d754e0 ("KVM: arm64: Add {get,set}_user for
> PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}") changed KVM_SET_ONE_REG to update
> the mentioned registers in a way matching with the behavior of guest
> register writes. This is a breaking change of a UAPI though the new
> semantics looks cleaner and VMMs are not prepared for this.
> 
> Firecracker, QEMU, and crosvm perform migration by listing registers
> with KVM_GET_REG_LIST, getting their values with KVM_GET_ONE_REG and
> setting them with KVM_SET_ONE_REG. This algorithm assumes
> KVM_SET_ONE_REG restores the values retrieved with KVM_GET_ONE_REG
> without any alteration. However, bit operations added by the earlier
> commit do not preserve the values retried with KVM_GET_ONE_REG and
> potentially break migration.
> 
> Remove the bit operations that alter the values retrieved with
> KVM_GET_ONE_REG.
> 
> Fixes: a45f41d754e0 ("KVM: arm64: Add {get,set}_user for PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 51054b7befc0b4bd822cecf717ee4a4740c4a685..2f44d4d4f54112787683dd75ea93fd60e92dd31f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1142,26 +1142,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  
>  static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 val)
>  {
> -	bool set;
> -
> -	val &= kvm_pmu_valid_counter_mask(vcpu);
> -
> -	switch (r->reg) {
> -	case PMOVSSET_EL0:
> -		/* CRm[1] being set indicates a SET register, and CLR otherwise */
> -		set = r->CRm & 2;
> -		break;
> -	default:
> -		/* Op2[0] being set indicates a SET register, and CLR otherwise */
> -		set = r->Op2 & 1;
> -		break;
> -	}
> -
> -	if (set)
> -		__vcpu_sys_reg(vcpu, r->reg) |= val;
> -	else
> -		__vcpu_sys_reg(vcpu, r->reg) &= ~val;
> -
> +	__vcpu_sys_reg(vcpu, r->reg) = val & kvm_pmu_valid_counter_mask(vcpu);
>  	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
>  
>  	return 0;
> 

Yup, this code was definitely a brain fart. Thanks for spotting it.
One of the big mistake was to expose both CLR and SET registers to
userspace (one of the two should have been hidden).

This requires a Cc to stable@vger.kernel.org so that this can be
backported to anything from 6.12. It would also help if you put this
patch at the head of the series, before adding the PMU request (it is
then likely to be very easy to backport).

With that,

Acked-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.