Re: [PATCH 08/21] KVM: arm64: Set up FGT for Partitioned PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `pmu_is_partitioned()` unsafely accesses a per-CPU variable via `host_data_ptr()` without preemption disabled, and evaluates incorrectly on heterogeneous systems.
- [High] `has_host_pmu_partition_support()` enables PMU partitioning without checking for `FEAT_FGT`, breaking vPMU interrupts on systems without it.
- [Low] Dummy inline implementation of `pmu_is_partitioned()` unnecessarily drops type safety by using `void *`.
--

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

KVM: arm64: Set up FGT for Partitioned PMU

This commit utilizes fine-grained traps (FEAT_FGT) to avoid trapping 
common PMU register accesses by the guest, which improves performance 
for partitioned PMUs. It selectively traps specific registers like 
PMOVS_EL0 to track overflow IRQs while leaving common counters untrapped.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 74e40e4915416..0135989667564 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
> @@ -20,3 +22,37 @@ bool has_host_pmu_partition_support(void)
>  	return has_vhe() &&
>  		system_supports_pmuv3();
>  }

[Severity: High]
Should this also check for cpus_have_final_cap(ARM64_HAS_FGT)?

Without FEAT_FGT, PMOVS_EL0 accesses won't be trapped. Since the host PMU
IRQ handler clears the hardware overflow bit and records it in a software
virtual overflow state, an untrapped guest read will incorrectly see 0,
and guest writes will bypass KVM completely.

Could this lead to KVM's virtual overflow state never being cleared,
resulting in a persistent vIRQ and a guest interrupt storm on pre-ARMv8.6
systems?

> +
> +/**
> + * pmu_is_partitioned() - Determine if given PMU is partitioned
> + * @pmu: Pointer to arm_pmu struct
> + *
> + * Determine if given PMU is partitioned by looking at hpmn field. The
> + * PMU is partitioned if this field is less than the number of
> + * counters in the system.
> + *
> + * Return: True if the PMU is partitioned, false otherwise
> + */
> +bool pmu_is_partitioned(struct arm_pmu *pmu)
> +{
> +	if (!pmu)
> +		return false;
> +
> +	return pmu->max_guest_counters >= 0 &&
> +		pmu->max_guest_counters <= *host_data_ptr(nr_event_counters);
> +}

[Severity: High]
Does evaluating host_data_ptr() here require preemption to be disabled?

host_data_ptr() expands to this_cpu_ptr(&kvm_host_data), and this function
is heavily used by KVM's PMU sysreg accessors which are reachable from
guest exit handlers with preemption enabled. Accessing this_cpu_ptr() here
can trigger a "BUG: using smp_processor_id() in preemptible" warning.

Additionally, on heterogeneous platforms (big.LITTLE) where different CPUs
might have a different number of event counters, could the PMU partition
status sporadically flip between true and false depending on which CPU the
thread is scheduled on, silently corrupting the vPMU state?

[ ... ]
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index f9a0823666949..36960b9e52da2 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
[ ... ]
> @@ -231,6 +238,11 @@ static inline bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int id
>  
>  static inline void kvm_pmu_nested_transition(struct kvm_vcpu *vcpu) {}
>  
> +static inline bool pmu_is_partitioned(void *pmu)
> +{
> +	return false;
> +}
> +

[Severity: Low]
Is there a specific reason to use void *pmu for this fallback stub?

Since struct arm_pmu is forward declared and widely used in this header,
using struct arm_pmu *pmu instead would preserve static type checking when
the feature is disabled.

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