1. Patchew
  2. linux
  3. [PATCH v2 0/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs
  4. View patch

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

Akihiko Odaki posted 3 patches 11 months, 1 week ago
There is a newer version of this series
[PATCH v2 3/3] KVM: arm64: PMU: Set raw values from user to PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR}
Posted by Akihiko Odaki 11 months, 1 week ago 
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}
Posted by Marc Zyngier 11 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.