Re: [PATCH 17/21] KVM: arm64: Detect overflows for the Partitioned PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Missing return and failure to synchronize hardware registers when transitioning to VCPU_PMU_ACCESS_GUEST_OWNED drops interrupts and corrupts guest PMU state.
- [High] kvm_pmu_part_overflow_status() evaluates overflow using stale software PMOVSSET_EL0 instead of the physical register when the PMU is guest-owned, missing hardware overflows.
- [Low] Unused prototype for `kvm_pmu_overflow_status` left in header.

Pre-existing issues:
- [High] Non-atomic read-modify-write of PMOVSSET_EL0 in hardirq context corrupts concurrent process-context writes.
- [High] Missing required `isb()` synchronization immediately following a write to the `pmcr_el0` control-plane system register.
--

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

KVM: arm64: Detect overflows for the Partitioned PMU
    
When we re-enter the VM after handling a PMU interrupt, calculate
whether it was any of the guest counters that overflowed and inject an
interrupt into the guest if so.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 64f40cfb31012..0062d1d8e1999 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[Severity: High]
This isn't a bug introduced by this patch, but looking at
kvm_pmu_direct_pmcr_write() in this file, is there a missing isb() after
writing the control register?

arch/arm64/kvm/pmu-direct.c:kvm_pmu_direct_pmcr_write() {
    write_sysreg(val, pmcr_el0);
    if (reset_p) {
...
}

Architectural rules require every write to a control-plane system register
to be followed by an isb() as the very next instruction. Since it is followed
immediately by a conditional branch, can the pipeline observe undefined state?

> @@ -426,4 +426,52 @@ void kvm_pmu_handle_guest_irq(struct arm_pmu *pmu, u64 pmovsr)
>  		return;
>  
>  	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, govf);

[Severity: High]
This is a pre-existing issue, but does this non-atomic read-modify-write
in hardirq context corrupt concurrent process-context writes?

When the physical PMU interrupt handler calls kvm_pmu_handle_guest_irq(), it
executes this macro in hardirq context. Concurrently, the VM-exit path
(access_pmovs()) can execute the same macro in process context with
interrupts enabled.

If the hardware interrupt preempts the process context between the read and
write phases, could the process context overwrite the sysreg with its stale
read and silently drop the overflow bits?

> +
> +	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)
> +{
> +	struct arm_pmu *pmu;
> +	u64 mask, pmovs, pmint, pmcr;
> +	bool overflow;
> +
> +	pmu = vcpu->kvm->arch.arm_pmu;
> +	mask = kvm_pmu_guest_counter_mask(pmu);
> +
> +	if (vcpu->arch.pmu.access == VCPU_PMU_ACCESS_FREE) {
> +		pmovs = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
> +		pmint = __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
> +		pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0);
> +
> +		if ((pmcr & ARMV8_PMU_PMCR_E) && (mask & pmovs & pmint))
> +			kvm_pmu_set_guest_owned(vcpu);
> +		else
> +			return false;
> +	}
> +

[Severity: High]
Does this fall through to read uninitialized hardware state?

When a virtual overflow is detected and kvm_pmu_set_guest_owned() is called,
it transitions the state but there is no return true; here. Execution falls
through to unconditionally read the physical hardware registers below.

Additionally, looking at kvm_pmu_set_guest_owned(), it grants direct
hardware access via MDCR_EL2 but fails to invoke kvm_pmu_load() to populate
the hardware PMU with the guest's context.

Since the hardware wasn't loaded, could these reads return host state,
causing the calculation to evaluate to false and drop the pending interrupt?

> +	pmovs = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
> +	pmint = read_pmintenset();
> +	pmcr = read_pmcr();

[Severity: High]
Does this evaluate overflow using stale software state instead of the
physical register when the PMU is guest-owned?

When the PMU is guest-owned, physical hardware overflows update the physical
register. The software context is only synchronized during kvm_pmu_put().

Since this is called during guest exit via kvm_pmu_sync_hwstate() before
kvm_pmu_put() runs, could unconditionally reading the software context via
__vcpu_sys_reg() evaluate against stale state and miss physical hardware
overflows?

> +	overflow = (pmcr & ARMV8_PMU_PMCR_E) && (mask & pmovs & pmint);
> +
> +	return overflow;
>  }

[ ... ]

> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 25163a689ae80..f72d080ee7ba2 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -93,6 +93,8 @@ bool kvm_set_pmuserenr(u64 val);
>  void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_pmu_resync_el0(void);
> +bool kvm_pmu_emul_overflow_status(struct kvm_vcpu *vcpu);
> +bool kvm_pmu_part_overflow_status(struct kvm_vcpu *vcpu);

[Severity: Low]
Since kvm_pmu_overflow_status() was renamed to kvm_pmu_emul_overflow_status()
in this patch, should the old prototype be removed from this header file?

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