Re: [PATCH v3 3/3] KVM: PPC: Book3s HV: Ensure reliable usage of latency counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: PPC: Book3S HV: u8 overflow in
  kvmhv_set_l2_counters_status()
- [Medium] KVM: PPC: Book3S HV: l2_counters_enable underflow across CPU
  hotplug
--

commit c5caaeddc35ba638841a3b81b1767bed8f97a3e3
Author: Gautam Menghani <gautam@linux.ibm.com>

KVM: PPC: Book3s HV: Ensure reliable usage of latency counters

This commit replaces the boolean assignment of l2_counters_enable in
kvmhv_set_l2_counters_status() with spinlock-protected increments and
decrements so the kvmppc_vcpu_stats tracepoint and vpa-pmu driver can
enable VPA latency counters concurrently.

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 5fa6f93548add..e1bde3c6b0c4e 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4142,14 +4142,19 @@ int kvmhv_get_l2_counters_status(void)
>  		get_lppaca()->l2_counters_enable;
>  }
>  
> +static DEFINE_RAW_SPINLOCK(l2_counters_lock);
>  void kvmhv_set_l2_counters_status(int cpu, bool status)
>  {
> +	unsigned long flags;
> +
>  	if (!firmware_has_feature(FW_FEATURE_LPAR))
>  		return;
> +	raw_spin_lock_irqsave(&l2_counters_lock, flags);
>  	if (status)
> -		lppaca_of(cpu).l2_counters_enable = 1;
> +		lppaca_of(cpu).l2_counters_enable++;

[Severity: Medium]
Can lppaca_of(cpu).l2_counters_enable overflow here?

In struct lppaca, l2_counters_enable is a u8, while vpa_pmu_event_init()
places no upper bound on how many perf_sw_context events can be opened and
scheduled on the same CPU via vpa_pmu_add().  When 256 consumers enable
the counters on one CPU, l2_counters_enable wraps to 0, causing the
hypervisor to stop accumulating VPA latency counters and
kvmhv_get_l2_counters_status() to return 0 while active consumers are
running.

>  	else
> -		lppaca_of(cpu).l2_counters_enable = 0;
> +		lppaca_of(cpu).l2_counters_enable--;
> +	raw_spin_unlock_irqrestore(&l2_counters_lock, flags);
>  }
>  EXPORT_SYMBOL(kvmhv_set_l2_counters_status);

[Severity: Medium]
Could lppaca_of(cpu).l2_counters_enable underflow across CPU hotplug?

kvmhv_counters_tracepoint_regfunc() and
kvmhv_counters_tracepoint_unregfunc() both iterate over
for_each_present_cpu(cpu) without CPU hotplug callbacks:

arch/powerpc/kvm/book3s_hv.c:kvmhv_counters_tracepoint_regfunc() {
	for_each_present_cpu(cpu) {
		kvmhv_set_l2_counters_status(cpu, true);
	}
	return 0;
}

If a CPU is hot-added via pseries_add_processor() while the
kvmppc_vcpu_stats tracepoint is enabled, disabling the tracepoint
decrements l2_counters_enable on that CPU from 0 to 255 (or drops an
active vpa_pmu event's reference), leaving the VPA counters enabled after
all users exit.  Conversely, hot-removing a CPU while the tracepoint is
active skips the decrement on that CPU when the tracepoint is later
disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106153136.330165-1-gautam@linux.ibm.com?part=3