Re: [PATCH v7 14/20] KVM: arm64: Implement lazy PMU context swaps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Transitioning the PMU to GUEST_OWNED fails to load the guest's virtual PMU state into the hardware registers, leading to an information leak and state corruption.
- [High] PMU read accessors directly read from hardware registers when the PMU is in the FREE state, leaking host PMU state to the guest.
- [High] Restoring PMU registers via userspace during live migration fails to transition the vCPU to GUEST_OWNED, leaving restored counters inert.
- [High] `pmu_write_evtyper` fails to update the physical hardware register, causing guest event configurations to be ignored.
- [High] Direct hardware register manipulation in `pmu_write_evcntr` and `pmu_read_evcntr` lacks preemption protection, interacting with a bug in `kvm_pmu_put` to cause state corruption.
--

commit 62dcf0081f7f3437442e2756f69cc3188de056f1
Author: Colton Lewis <coltonlewis@google.com>

KVM: arm64: Implement lazy PMU context swaps

This commit implements a simple state machine for PMU register access to 
avoid the cost of context swapping for guests that never touch the PMU. The 
PMU starts as free and transitions to guest-owned only when a guest accesses 
the PMU registers.

> diff --git a/arch/arm64/include/asm/kvm_types.h b/arch/arm64/include/asm/kvm_types.h
> index 9a126b9e2d7c9..4e39cbc80aa0b 100644
> --- a/arch/arm64/include/asm/kvm_types.h
> +++ b/arch/arm64/include/asm/kvm_types.h
> @@ -4,5 +4,9 @@
>  
>  #define KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE 40
>  
> -#endif /* _ASM_ARM64_KVM_TYPES_H */
> +enum vcpu_pmu_register_access {
> +	VCPU_PMU_ACCESS_FREE,
> +	VCPU_PMU_ACCESS_GUEST_OWNED,
> +};

How does this state machine handle PMU registers being restored via userspace
during live migration?

If userspace restores PMU registers via KVM_SET_ONE_REG, the sysreg setters
update the in-memory vCPU state but do not appear to call
kvm_pmu_set_guest_owned() or transition the state away from
VCPU_PMU_ACCESS_FREE.

When the migrated VM resumes execution, kvm_pmu_load() might ignore the PMU
context, leaving any previously running counters inert until the guest
explicitly performs a new PMU write.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 14cc419dbafad..881cea5117515 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
[ ... ]
> @@ -278,7 +297,8 @@ void kvm_pmu_load(struct kvm_vcpu *vcpu)
>  	 * If we aren't guest-owned then we know the guest isn't using
>  	 * the PMU anyway, so no need to bother with the swap.
>  	 */
> -	if (!kvm_vcpu_pmu_is_partitioned(vcpu))
> +	if (!kvm_vcpu_pmu_is_partitioned(vcpu) ||
> +	    vcpu->arch.pmu.access != VCPU_PMU_ACCESS_GUEST_OWNED)
>  		return;
>  
>  	preempt_disable();

If the PMU is in the VCPU_PMU_ACCESS_FREE state, do the PMU read accessors
directly read from hardware registers?

If a guest reads a PMU register before performing any writes, the state
remains free. Because this check skips loading the guest context, could the
read accessors execute read_sysreg() on the physical hardware and return the
host's PMU state to the guest?

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index cc3d1804ab200..e4840d93a769f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1035,6 +1035,7 @@ static bool pmu_access_event_counter_el0_disabled(struct kvm_vcpu *vcpu)
>  static void pmu_write_pmcr(struct kvm_vcpu *vcpu, u64 val)
>  {
>  	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		kvm_pmu_set_guest_owned(vcpu);
>  		write_sysreg(val, pmcr_el0);
>  		return;
>  	}

Does transitioning the PMU to VCPU_PMU_ACCESS_GUEST_OWNED here also require
loading the guest's virtual PMU state into the hardware registers?

When kvm_pmu_set_guest_owned() is called, it configures mdcr_el2 to allow
direct guest access but fails to invoke kvm_pmu_load(). Since the initial
vCPU entry skipped loading the PMU, the physical registers might still
contain the host's PMU state.

Could this allow the guest to read uninitialized hardware registers? Also,
upon preemption, kvm_pmu_put() might read these host values and overwrite the
guest's saved memory context.

[ ... ]
> @@ -1193,6 +1195,8 @@ static void pmu_write_evcntr(struct kvm_vcpu *vcpu, u64 val, u64 idx)
>  		return;
>  	}
>  
> +	kvm_pmu_set_guest_owned(vcpu);
> +
>  	if (idx == ARMV8_PMU_CYCLE_IDX) {
>  		write_sysreg(val, pmccntr_el0);
>  		return;

Is there a preemption issue when manipulating hardware registers directly
here? This problem wasn't introduced by this patch, but it significantly
impacts the PMU logic.

In pmu_write_evcntr(), the code modifies pmselr_el0 to select a counter index
before writing to pmxevcntr_el0. Since KVM sysreg handlers run with preemption
enabled, if preemption occurs mid-sequence, kvm_pmu_put() iterates over guest
counters, using and clobbering pmselr_el0.

It appears kvm_pmu_put() fails to read and save the original pmselr_el0 before
its loop, instead saving the highest guest counter index at the end. When the
task resumes, kvm_pmu_load() restores this highest index to pmselr_el0.
Execution resumes in pmu_write_evcntr(), which might then write to
pmxevcntr_el0, inadvertently mutating the highest guest counter instead of
the intended idx.

[ ... ]
> @@ -1281,6 +1285,7 @@ static void pmu_write_evtyper(struct kvm_vcpu *vcpu, u64 val, u64 idx)
>  	u64 mask;
>  
>  	if (kvm_vcpu_pmu_is_partitioned(vcpu)) {
> +		kvm_pmu_set_guest_owned(vcpu);
>  		mask = kvm_pmu_evtyper_mask(vcpu->kvm);
>  		__vcpu_assign_sys_reg(vcpu, PMEVTYPER0_EL0 + idx, val & mask);
>  		return;
>  	}

Does this sequence fail to update the physical hardware register?

The code updates the guest's event configuration in the memory array using
__vcpu_assign_sys_reg(), but omits writing the value to the physical
pmxevtyper_el0 register.

Since KVM does not invoke kvm_pmu_load() after returning from a sysreg trap,
could the active hardware counter continue counting its previous event instead
of the newly configured one?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504211813.1804997-1-coltonlewis@google.com?part=14