| Message ID | 20230202013140.30416-1-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | powercap/intel_rapl: Fix handling for large time window | expand |
On Thu, Feb 2, 2023 at 2:25 AM Zhang Rui <rui.zhang@intel.com> 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@intel.com> > --- > 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? > + 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. > + } > + > + f = div64_u64(4 * (value - (1ULL << y)), 1ULL << y); > value = (y & 0x1f) | ((f & 0x3) << 5); > } > return value; > --
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@intel.com> 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@intel.com> > > --- > > 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; > > --
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); + return 0x7f; + } + + f = div64_u64(4 * (value - (1ULL << y)), 1ULL << y); value = (y & 0x1f) | ((f & 0x3) << 5); } return value;
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@intel.com> --- drivers/powercap/intel_rapl_common.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)