Re: [PATCH v7 13/20] KVM: arm64: Apply dynamic guest counter reservations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hard lockup / Deadlock due to acquiring `perf_ctx_lock` with hardware interrupts enabled.
- [High] Data race and cross-CPU isolation breach due to dynamic modification of the global PMU counter mask (`cntr_mask`).
- [High] NULL pointer dereference risk due to lockless, unprotected access to the per-CPU perf events array.
- [Medium] Pending host perf events remain permanently inactive due to skipped PMU reschedule updates.
--

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

KVM: arm64: Apply dynamic guest counter reservations

Apply dynamic guest counter reservations by checking if the requested
guest mask collides with any events the host has scheduled and calling
pmu_perf_resched_update() with a hook that updates the mask of
available counters in between schedule out and schedule in.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 2252d3b905db9..14cc419dbafad 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> @@ -100,6 +100,73 @@ u8 kvm_pmu_hpmn(struct kvm_vcpu *vcpu)
> +static void kvm_pmu_set_guest_counters(struct arm_pmu *cpu_pmu, u64 guest_mask)
> +{
> +	struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> +	DECLARE_BITMAP(guest_bitmap, ARMPMU_MAX_HWEVENTS);
> +	DECLARE_BITMAP(new_mask, ARMPMU_MAX_HWEVENTS);
> +	bool need_resched = false;
> +
> +	bitmap_from_arr64(guest_bitmap, &guest_mask, ARMPMU_MAX_HWEVENTS);
> +	bitmap_copy(new_mask, cpu_pmu->hw_cntr_mask, ARMPMU_MAX_HWEVENTS);
> +
> +	if (guest_mask) {
> +		/* Subtract guest counters from available host mask */
> +		bitmap_andnot(new_mask, new_mask, guest_bitmap, ARMPMU_MAX_HWEVENTS);
> +
> +		/* Did we collide with an active host event? */
> +		if (bitmap_intersects(cpuc->used_mask, guest_bitmap, ARMPMU_MAX_HWEVENTS)) {
> +			int idx;
> +
> +			need_resched = true;
> +			cpuc->host_squeezed = true;
> +
> +			/* Look for pinned events that are about to be preempted */
> +			for_each_set_bit(idx, guest_bitmap, ARMPMU_MAX_HWEVENTS) {
> +				if (test_bit(idx, cpuc->used_mask) && cpuc->events[idx] &&
> +				    cpuc->events[idx]->attr.pinned) {

Is there a risk of a NULL pointer dereference here?

The per-CPU events array is accessed without locks and with hardware interrupts
enabled. If an interrupt occurs (like an IPI triggering an event removal)
between the evaluation of cpuc->events[idx] and reading ->attr.pinned, and
the compiler issues separate memory reads due to a lack of READ_ONCE(), could
the second read observe a NULL pointer?

> +					pr_warn_ratelimited("perf: Pinned host event squeezed out by KVM guest PMU partition\n");
> +					break;
> +				}
> +			}
> +		}
> +	} else {
> +		/*
> +		 * Restoring to hw_cntr_mask.
> +		 * Only resched if we previously squeezed an event.
> +		 */
> +		if (cpuc->host_squeezed) {
> +			need_resched = true;
> +			cpuc->host_squeezed = false;
> +		}
> +	}

What happens to host perf events created while the guest is actively running?

It looks like cpuc->host_squeezed is only set if a host event collided when
the guest was loaded. If a new host event is created while the guest is
running, it will fail to allocate a counter due to the restricted mask.

When the guest unloads, cpuc->host_squeezed is false, so no reschedule is
requested. Will the perf subsystem be notified that counters have been freed,
or will the host event remain permanently inactive?

> +
> +	if (need_resched) {
> +		/* Collision: run full perf reschedule */
> +		perf_pmu_resched_update(&cpu_pmu->pmu, kvm_pmu_update_mask, new_mask);

Could this cause a deadlock if a Performance Monitor Interrupt (PMI) or
perf IPI fires?

kvm_pmu_load() calls this with preemption disabled but leaves hardware
interrupts enabled. perf_pmu_resched_update() unconditionally calls
perf_ctx_lock(), which acquires the context lock without disabling local IRQs.

If an interrupt fires on the same CPU while this lock is held, and the
handler attempts to acquire ctx->lock via raw_spin_lock_irqsave(), will
it spin indefinitely?

> +	} else {
> +		/* Host was never using guest counters anyway */
> +		bitmap_copy(cpu_pmu->cntr_mask, new_mask, ARMPMU_MAX_HWEVENTS);

Does modifying cpu_pmu->cntr_mask here introduce a data race?

struct arm_pmu represents PMU hardware capabilities shared across multiple CPUs
in a domain. If this per-CPU load/put path dynamically mutates cntr_mask
without global synchronization, could reserving guest counters on one CPU
incorrectly prevent host perf events from running on another CPU?

Also, when a vCPU unloads, it appears to unconditionally restore cntr_mask to
hw_cntr_mask, which might wipe out active reservations from other vCPUs.

> +	}
> +}

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