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 <maz@kernel.org>]

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.