Re: [PATCH 19/21] KVM: selftests: Add find_bit to KVM library

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The non-atomic `__vcpu_rmw_sys_reg` macro is used to update `PMOVSSET_EL0` from both the VCPU thread and the PMU NMI handler, leading to lost guest overflow events.
--

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

KVM: selftests: Add find_bit to KVM library

This commit adds the find_bit functionality to the KVM selftests library.
It resolves separate compilation issues for selftests that have a dependency
on find_bit by including it similarly to rbtree.c.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> new file mode 100644
> index 0000000000000..2d2294b78ebe0
> --- /dev/null
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> +void kvm_pmu_put(struct kvm_vcpu *vcpu)
> +{

[ ... ]

> +	/* Save pending guest hardware overflows. */
> +	local_irq_save(flags);
> +	val = read_sysreg(pmovsset_el0);
> +	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, val & mask);
> +	write_sysreg(val & mask, pmovsclr_el0);
> +	local_irq_restore(flags);
> +
> +	/* Stop guest counters and disable interrupts in hardware. */
> +	write_sysreg(mask, pmcntenclr_el0);
> +	write_sysreg(mask, pmintenclr_el1);
> +	kvm_pmu_set_guest_counters(pmu, 0);
> +	preempt_enable();
> +}

[Severity: High]
Does this non-atomic register update risk losing guest overflow events?

Since __vcpu_rmw_sys_reg() is non-atomic, and local_irq_save() does not mask
NMIs on ARM64 when pseudo-NMIs are enabled, could an NMI interrupt the VCPU
thread exactly between the load and store phases here? 

If the PMU NMI handler injects a guest overflow flag during this window,
wouldn't it be overwritten and permanently lost by the stale writeback?

[ ... ]

> +void kvm_pmu_handle_guest_irq(struct arm_pmu *pmu, u64 pmovsr)
> +{
> +	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> +	u64 mask = kvm_pmu_guest_counter_mask(pmu);
> +	u64 govf = pmovsr & mask;
> +
> +	write_pmovsclr(govf);
> +
> +	if (!vcpu)
> +		return;
> +
> +	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, govf);

[Severity: High]
Is this concurrent modification safe?

Since this NMI handler asynchronously modifies PMOVSSET_EL0 using the same
non-atomic __vcpu_rmw_sys_reg() macro used by the VCPU thread in kvm_pmu_put(),
could they race? 

If this handler runs while the VCPU thread is performing its own
read-modify-write on PMOVSSET_EL0, wouldn't the guest overflow flag be
overwritten and lost, leading to incorrect counter states in the guest?

> +
> +	if (kvm_pmu_part_overflow_status(vcpu)) {
> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);

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