Re: [PATCH] powercap: intel_rapl: Sign-extend the PMU delta on counter wraparound

On Fri, 2026-08-14 at 11:10 +0800, Li Yifan wrote:
> From: "Li, Yifan" <[email protected]>
> 
> The RAPL PMU misreports energy when the hardware energy counter
> overflows and wraps back to zero.  perf event counts are defined to
> increase monotonically, but a single wraparound makes the PMU event
> count jump backwards by nearly the full counter range, and consumers
> that take the difference of two reads in unsigned arithmetic then
> underflow and report an absurd value.
> 
> On a Panther Lake system (energy unit 61.035 uJ, counter range
> 262144 J) the package counter wraps every ~2.9 hours at 25 W, and
> turbostat prints one bogus sample per wraparound, per domain:
> 
>   PkgTmp  PkgWatt         CorWatt GFXWatt RAMWatt SysWatt
>   44      24.97           16.30   3.90    1.87    2145386370.35
>   43      2145240612.10   16.13   4.02    1.91    40.46
> 
> The RAPL energy counters are 32-bit wide on every register interface:
> MSR, MMIO and TPMI all describe ENERGY_COUNTER with a GENMASK(31, 0)
> mask.  rapl_read_data_raw() applies that mask, so
> event_read_counter()
> returns the counter zero-extended in a u64.
> 
> rapl_event_update() then computes
> 
> 	delta = new_raw_count - prev_raw_count;
> 
> without reducing the result modulo 2^32.  While the counter does not
> wrap this is correct, but once the hardware counter wraps,
> new_raw_count < prev_raw_count and delta becomes (true_delta - 2^32),
> a large negative value.  Declaring delta as s64 only makes that value
> representable; it does not correct it.  That bogus delta is scaled
> and
> added to event->count, which is where the backwards jump comes from.
> 
> Fix it the way arch/x86/events/rapl.c has done since the RAPL PMU was
> first introduced: shift both values up so that the 64-bit subtraction
> reduces modulo 2^32, then shift the difference back down with an
> arithmetic shift to sign-extend it.
> 
> This is correct as long as at most one wraparound happens between two
> updates, which the existing overflow hrtimer already guarantees: its
> period is half of the counter range at the 200 W reference used in
> rapl_package_add_pmu_locked().
> 
> The problem has been present since the powercap RAPL PMU was added,
> but
> only affected TPMI RAPL until commit 748d6ba43afd ("powercap:
> intel_rapl: Enable MSR-based RAPL PMU support") routed MSR RAPL
> through
> the same PMU, which exposed it on client platforms such as Panther
> Lake.
> 
> Fixes: 575024a8aa7c ("powercap: intel_rapl: Introduce APIs for PMU
> support")
> Reported-by: Jyoti, Anand B <[email protected]>
> Signed-off-by: Li, Yifan <[email protected]>
> Signed-off-by: Gao Jianfeng <[email protected]>
> Tested-by: Jyoti, Anand B <[email protected]>

Acked-by: Srinivas Pandruvada <[email protected]>

> ---
>  drivers/powercap/intel_rapl_common.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/powercap/intel_rapl_common.c
> b/drivers/powercap/intel_rapl_common.c
> index 1006d183d508..6b7d11a0abc5 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -32,6 +32,9 @@
>  
>  #define ENERGY_STATUS_MASK		GENMASK(31, 0)
>  
> +/* Width of the RAPL energy counters, see the *_ENERGY_STATUS_MASK
> defines */
> +#define RAPL_CNTR_WIDTH			32
> +
>  #define POWER_UNIT_OFFSET		0x00
>  #define POWER_UNIT_MASK			GENMASK(3, 0)
>  
> @@ -1227,6 +1230,7 @@ static u64 rapl_event_update(struct perf_event
> *event)
>  	struct rapl_package_pmu_data *data =
> event_to_pmu_data(event);
>  	u64 prev_raw_count, new_raw_count;
>  	s64 delta, sdelta;
> +	int shift = 64 - RAPL_CNTR_WIDTH;
>  
>  	/*
>  	 * Follow the generic code to drain hwc->prev_count.
> @@ -1243,8 +1247,13 @@ static u64 rapl_event_update(struct perf_event
> *event)
>  	 * Now we have the new raw value and have updated the prev
>  	 * timestamp already. We can now calculate the elapsed delta
>  	 * (event-)time and add that to the generic event.
> +	 *
> +	 * Careful, the counter is narrower than u64 and is not
> +	 * sign-extended above its physical width.  Shift both
> values up
> +	 * so that the subtraction wraps, then shift the result back
> down.
>  	 */
> -	delta = new_raw_count - prev_raw_count;
> +	delta = (new_raw_count << shift) - (prev_raw_count <<
> shift);
> +	delta >>= shift;
>  
>  	/*
>  	 * Scale delta to smallest unit (2^-32)
> 
> base-commit: f5bbbfec59b4e2fb7520a91de3df8a6174325d6a