Re: [PATCH v9 19/22] KVM: arm64: Detect overflows for the Partitioned PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] kvm: arm64: PMU partition clears hardware PMOVSSET_EL0 without restore
- [High] kvm: arm64: Data race in PMOVSSET_EL0 update against NMI handler
- [Medium] kvm: arm64: kvm_pmu_should_notify_user() indirectly alters hardware PMU state
--

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

KVM: arm64: Detect overflows for the Partitioned PMU

When re-entering the VM or synchronizing PMU state after a guest counter
overflow interrupt, evaluate whether any enabled and unmasked guest
counter has overflowed and update the virtual PMU interrupt level in
the vGIC accordingly.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> @@ -534,6 +534,61 @@ void kvm_pmu_handle_guest_irq(u64 pmovsr)
>  
>  	for_each_set_bit(i, (unsigned long *)&govf, 64)
>  		set_bit(i, (unsigned long *)__ctxt_sys_reg(&vcpu->arch.ctxt, PMOVSSET_EL0));
> +
> +	if (kvm_pmu_part_overflow_status(vcpu)) {
> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> +
> +		if (!in_nmi())
> +			kvm_vcpu_kick(vcpu);
> +		else
> +			irq_work_queue(&vcpu->arch.pmu.overflow_work);
> +	}
> +}
> +
> +/**
> + * kvm_pmu_part_overflow_status() - Determine if any guest counters have overflowed
[ ... ]
> +bool kvm_pmu_part_overflow_status(struct kvm_vcpu *vcpu)
> +{
> +	u64 mask = kvm_vcpu_pmu_guest_counter_mask(vcpu);
> +	u64 pmovs, pmint, pmcr;
> +	unsigned long flags;
> +	bool overflow;
> +
> +	local_irq_save(flags);
> +	if (vcpu->arch.pmu.loaded_on_cpu &&
> +	    vcpu == kvm_get_running_vcpu()) {
> +		u64 hw_ovf = read_sysreg(pmovsset_el0) & mask;
> +
> +		if (hw_ovf) {
> +			__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, hw_ovf);

[Severity: High]
Is this non-atomic update vulnerable to a race with pseudo-NMIs?

Since local_irq_save() does not mask pseudo-NMIs on ARM64, the PMU interrupt
can fire as an NMI and invoke kvm_pmu_handle_guest_irq(), which updates this
exact software state using an atomic set_bit().

When the NMI returns, this interrupted non-atomic read-modify-write
operation will complete and could silently overwrite the NMI handler's
updates, resulting in lost interrupts and corrupted hardware state.

> +			write_sysreg(hw_ovf, pmovsclr_el0);

[Severity: Critical]
Does this break the guest's ability to read the overflow bit?

The hardware overflow register is cleared here and the state is saved to the
software context, but the canonical software state is never written back to
the hardware before the guest accesses it directly.

Because the partitioned PMU grants the guest direct, untrapped EL0/EL1
access to PMOVSSET_EL0 for its assigned counters, failing to restore this
will cause the guest to read a cleared hardware register (a 0 value) upon
resuming execution, permanently breaking its ability to identify which
counter overflowed.

> +		}
> +		pmint = read_pmintenset();
> +		pmcr = read_pmcr();
> +	} else {
> +		pmint = __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
> +		pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0);
> +	}
> +
> +	pmovs = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
> +	local_irq_restore(flags);
> +
> +	overflow = (pmcr & ARMV8_PMU_PMCR_E) && (mask & pmovs & pmint);
> +
> +	if (overflow && kvm_pmu_get_access(vcpu) == VCPU_PMU_ACCESS_FREE)
> +		kvm_pmu_set_guest_owned(vcpu);
> +
> +	return overflow;
>  }

> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c

[ ... ]

> @@ -296,8 +296,14 @@ bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_sync_regs *sregs = &vcpu->run->s.regs;
>  	bool run_level = sregs->device_irq_level & KVM_ARM_DEV_PMU;
> +	bool overflow;
>  
> -	return kvm_pmu_overflow_status(vcpu) != run_level;
> +	if (kvm_pmu_is_partitioned(vcpu->kvm))
> +		overflow = kvm_pmu_part_overflow_status(vcpu);

[Severity: Medium]
Does this call introduce hardware-altering side effects into a runnable
status predicate?

kvm_arch_vcpu_runnable() evaluates kvm_pmu_should_notify_user() in sleep
polling loops (like kvm_vcpu_block). However, kvm_pmu_part_overflow_status()
conditionally transitions PMU access to VCPU_PMU_ACCESS_GUEST_OWNED and
invokes kvm_pmu_load(), forcing hardware register writes from a pure getter.

Triggering hardware reprogramming inside waitqueue checks violates
architectural layering and could introduce latency or synchronization issues.

> +	else
> +		overflow = kvm_pmu_emul_overflow_status(vcpu);
> +
> +	return overflow != run_level;
>  }

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