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

[Rafael J. Wysocki (Intel)]

On Fri, Aug 14, 2026 at 7:46 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 8/13/2026 8:10 PM, Li Yifan wrote:
> > From: "Li, Yifan" <yifan2.li@xxxxxxxxx>
> >
> > 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 <anand.b.jyoti@xxxxxxxxx>
> > Signed-off-by: Li, Yifan <yifan2.li@xxxxxxxxx>
> > Signed-off-by: Gao Jianfeng <jianfeng.gao@xxxxxxxxx>
> > Tested-by: Jyoti, Anand B <anand.b.jyoti@xxxxxxxxx>
> > ---
>
> Looks good to me.
>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

Applied as 7.3-rc material, thanks!

> >  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
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>