Re: [PATCH] powercap/intel_rapl: Fix handling for large time window

[Zhang, Rui]

On Thu, 2023-02-09 at 18:06 +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 2, 2023 at 2:25 AM Zhang Rui <rui.zhang@xxxxxxxxx> wrote:
> > When setting the power limit time window, software updates the 'y'
> > bits
> > and 'f' bits in the power limit register, and the value hardware
> > takes
> > follows the formula below
> > 
> >         Time window = 2 ^ y * (1 + f / 4) * Time_Unit
> > 
> > When handling large time window input from userspace, using left
> > shifting breaks in two cases,
> > 1. when ilog2(value) is bigger than 31, in expression "1 << y",
> > left
> >    shifting by more than 31 bits has undefined behavior. This
> > breaks
> >    'y'. For example, on an Alderlake platform, "1 << 32" returns 1.
> > 2. when ilog2(value) equals 31, "1 << 31" returns negative value
> >    because '1' is recognized as signed int. And this breaks 'f'.
> > 
> > Given that 'y' has 5 bits and hardware can never take a value
> > larger
> > than 31, fix the first problem by clamp the time window to the
> > maximum
> > possible value that the hardware can take.
> > 
> > Fix the second problem by using unsigned bit left shift.
> > 
> > Note that hardware has its own maximum time window limitation,
> > which
> > may be lower than the time window value retrieved from the power
> > limit
> > register. When this happens, hardware clamps the input to its
> > maximum
> > time window limitation. That is why a software clamp is preferred
> > to
> > handle the problem on hand.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> >  drivers/powercap/intel_rapl_common.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 26d00b1853b4..8b30e5259d3b 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -999,7 +999,12 @@ static u64
> > rapl_compute_time_window_core(struct rapl_package *rp, u64 value,
> > 
> >                 do_div(value, rp->time_unit);
> >                 y = ilog2(value);
> > -               f = div64_u64(4 * (value - (1 << y)), 1 << y);
> > +               if (y > 0x1f) {
> > +                       pr_warn("%s: time window too large,
> > clamped\n", rp->name);
> 
> IIUC this happens when user space provides a value that is too large.
> Why do you want to log a kernel warning in that case?
> 
Right.
I don't know any API for this purpose, so just removed the warning in
patch v2.

> > +                       return 0x7f;
> 
> Because the target hardware field has 7 bits, the function will
> return
> all ones if the exponent is too large.  It would be good to put a
> comment stating this here.
> 
sure.

thanks,
rui

> > +               }
> > +
> > +               f = div64_u64(4 * (value - (1ULL << y)), 1ULL <<
> > y);
> >                 value = (y & 0x1f) | ((f & 0x3) << 5);
> >         }
> >         return value;
> > --