* [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS
@ 2020-10-15 14:45 Borislav Petkov
2020-10-15 14:46 ` [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs Borislav Petkov
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-10-15 14:45 UTC (permalink / raw)
To: X86 ML; +Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
From: Borislav Petkov <bp@suse.de>
Hi all,
here's something from my todo list: remove all in-kernel tools use of
that MSR and lastly drop it from the allowed-MSRs-list in the filtering.
Out-of-tree tools should do a similar, trivial conversion.
Constructive comments are, as always, appreciated.
Thx.
Borislav Petkov (4):
tools/power/cpupower: Read energy_perf_bias from sysfs
tools/power/turbostat: Read energy_perf_bias from sysfs
tools/power/x86_energy_perf_policy: Read energy_perf_bias from sysfs
x86/msr: Do not allow writes to MSR_IA32_ENERGY_PERF_BIAS
arch/x86/kernel/msr.c | 3 -
tools/power/cpupower/lib/cpupower.c | 23 +++-
tools/power/cpupower/lib/cpupower_intern.h | 5 +
tools/power/cpupower/utils/cpupower-info.c | 2 +-
tools/power/cpupower/utils/cpupower-set.c | 2 +-
tools/power/cpupower/utils/helpers/helpers.h | 8 +-
tools/power/cpupower/utils/helpers/misc.c | 48 ++++++++
tools/power/cpupower/utils/helpers/msr.c | 28 -----
tools/power/x86/turbostat/turbostat.c | 29 ++++-
.../x86_energy_perf_policy.c | 109 ++++++++++++++++--
10 files changed, 204 insertions(+), 53 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-15 14:45 [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
@ 2020-10-15 14:46 ` Borislav Petkov
2020-10-15 17:49 ` Shuah Khan
2020-10-15 14:46 ` [PATCH 2/4] tools/power/turbostat: " Borislav Petkov
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2020-10-15 14:46 UTC (permalink / raw)
To: X86 ML; +Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
From: Borislav Petkov <bp@suse.de>
... instead of poking at the MSR. For that, move the accessor functions
to misc.c and add a sysfs-writing function too.
There should be no functional changes resulting from this.
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Thomas Renninger <trenn@suse.com>
Cc: Shuah Khan <shuah@kernel.org>
---
tools/power/cpupower/lib/cpupower.c | 23 +++++++++-
tools/power/cpupower/lib/cpupower_intern.h | 5 ++
tools/power/cpupower/utils/cpupower-info.c | 2 +-
tools/power/cpupower/utils/cpupower-set.c | 2 +-
tools/power/cpupower/utils/helpers/helpers.h | 8 ++--
tools/power/cpupower/utils/helpers/misc.c | 48 ++++++++++++++++++++
tools/power/cpupower/utils/helpers/msr.c | 28 ------------
7 files changed, 81 insertions(+), 35 deletions(-)
diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index 3656e697537e..d2fa4bbecf74 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -16,8 +16,8 @@
unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen)
{
- int fd;
ssize_t numread;
+ int fd;
fd = open(path, O_RDONLY);
if (fd == -1)
@@ -35,6 +35,27 @@ unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen)
return (unsigned int) numread;
}
+unsigned int cpupower_write_sysfs(const char *path, char *buf, size_t buflen)
+{
+ ssize_t numwritten;
+ int fd;
+
+ fd = open(path, O_WRONLY);
+ if (fd == -1)
+ return 0;
+
+ numwritten = write(fd, buf, buflen - 1);
+ if (numwritten < 1) {
+ perror("write failed");
+ close(fd);
+ return -1;
+ }
+
+ close(fd);
+
+ return (unsigned int) numwritten;
+}
+
/*
* Detect whether a CPU is online
*
diff --git a/tools/power/cpupower/lib/cpupower_intern.h b/tools/power/cpupower/lib/cpupower_intern.h
index 4887c76d23f8..ac1112b956ec 100644
--- a/tools/power/cpupower/lib/cpupower_intern.h
+++ b/tools/power/cpupower/lib/cpupower_intern.h
@@ -1,6 +1,11 @@
/* SPDX-License-Identifier: GPL-2.0 */
#define PATH_TO_CPU "/sys/devices/system/cpu/"
+
+#ifndef MAX_LINE_LEN
#define MAX_LINE_LEN 4096
+#endif
+
#define SYSFS_PATH_MAX 255
unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen);
+unsigned int cpupower_write_sysfs(const char *path, char *buf, size_t buflen);
diff --git a/tools/power/cpupower/utils/cpupower-info.c b/tools/power/cpupower/utils/cpupower-info.c
index 0ba61a2c4d81..06345b543786 100644
--- a/tools/power/cpupower/utils/cpupower-info.c
+++ b/tools/power/cpupower/utils/cpupower-info.c
@@ -101,7 +101,7 @@ int cmd_info(int argc, char **argv)
}
if (params.perf_bias) {
- ret = msr_intel_get_perf_bias(cpu);
+ ret = cpupower_intel_get_perf_bias(cpu);
if (ret < 0) {
fprintf(stderr,
_("Could not read perf-bias value[%d]\n"), ret);
diff --git a/tools/power/cpupower/utils/cpupower-set.c b/tools/power/cpupower/utils/cpupower-set.c
index 052044d7e012..180d5ba877e6 100644
--- a/tools/power/cpupower/utils/cpupower-set.c
+++ b/tools/power/cpupower/utils/cpupower-set.c
@@ -95,7 +95,7 @@ int cmd_set(int argc, char **argv)
}
if (params.perf_bias) {
- ret = msr_intel_set_perf_bias(cpu, perf_bias);
+ ret = cpupower_intel_set_perf_bias(cpu, perf_bias);
if (ret) {
fprintf(stderr, _("Error setting perf-bias "
"value on CPU %d\n"), cpu);
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index c258eeccd05f..37dac161f3fe 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -105,8 +105,8 @@ extern struct cpupower_cpu_info cpupower_cpu_info;
extern int read_msr(int cpu, unsigned int idx, unsigned long long *val);
extern int write_msr(int cpu, unsigned int idx, unsigned long long val);
-extern int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val);
-extern int msr_intel_get_perf_bias(unsigned int cpu);
+extern int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val);
+extern int cpupower_intel_get_perf_bias(unsigned int cpu);
extern unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu);
/* Read/Write msr ****************************/
@@ -150,9 +150,9 @@ static inline int read_msr(int cpu, unsigned int idx, unsigned long long *val)
{ return -1; };
static inline int write_msr(int cpu, unsigned int idx, unsigned long long val)
{ return -1; };
-static inline int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val)
+static inline int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val)
{ return -1; };
-static inline int msr_intel_get_perf_bias(unsigned int cpu)
+static inline int cpupower_intel_get_perf_bias(unsigned int cpu)
{ return -1; };
static inline unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu)
{ return 0; };
diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
index f406adc40bad..e8f8f643a627 100644
--- a/tools/power/cpupower/utils/helpers/misc.c
+++ b/tools/power/cpupower/utils/helpers/misc.c
@@ -1,7 +1,15 @@
// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+
#if defined(__i386__) || defined(__x86_64__)
#include "helpers/helpers.h"
+#include "helpers/sysfs.h"
+
+#include "cpupower_intern.h"
#define MSR_AMD_HWCR 0xc0010015
@@ -40,4 +48,44 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
*support = *active = 1;
return 0;
}
+
+int cpupower_intel_get_perf_bias(unsigned int cpu)
+{
+ char linebuf[MAX_LINE_LEN];
+ char path[SYSFS_PATH_MAX];
+ unsigned long val;
+ char *endp;
+
+ if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
+ return -1;
+
+ snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
+
+ if (cpupower_read_sysfs(path, linebuf, MAX_LINE_LEN) == 0)
+ return -1;
+
+ val = strtol(linebuf, &endp, 0);
+ if (endp == linebuf || errno == ERANGE)
+ return -1;
+
+ return val;
+}
+
+int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val)
+{
+ char path[SYSFS_PATH_MAX];
+ char linebuf[3] = {};
+
+ if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
+ return -1;
+
+ snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
+ snprintf(linebuf, sizeof(linebuf), "%d", val);
+
+ if (cpupower_write_sysfs(path, linebuf, 3) <= 0)
+ return -1;
+
+ return 0;
+}
+
#endif /* #if defined(__i386__) || defined(__x86_64__) */
diff --git a/tools/power/cpupower/utils/helpers/msr.c b/tools/power/cpupower/utils/helpers/msr.c
index ab9950748838..8b0b6be74bb8 100644
--- a/tools/power/cpupower/utils/helpers/msr.c
+++ b/tools/power/cpupower/utils/helpers/msr.c
@@ -11,7 +11,6 @@
/* Intel specific MSRs */
#define MSR_IA32_PERF_STATUS 0x198
#define MSR_IA32_MISC_ENABLES 0x1a0
-#define MSR_IA32_ENERGY_PERF_BIAS 0x1b0
#define MSR_NEHALEM_TURBO_RATIO_LIMIT 0x1ad
/*
@@ -73,33 +72,6 @@ int write_msr(int cpu, unsigned int idx, unsigned long long val)
return -1;
}
-int msr_intel_get_perf_bias(unsigned int cpu)
-{
- unsigned long long val;
- int ret;
-
- if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
- return -1;
-
- ret = read_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, &val);
- if (ret)
- return ret;
- return val;
-}
-
-int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val)
-{
- int ret;
-
- if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
- return -1;
-
- ret = write_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, val);
- if (ret)
- return ret;
- return 0;
-}
-
unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu)
{
unsigned long long val;
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] tools/power/turbostat: Read energy_perf_bias from sysfs
2020-10-15 14:45 [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
2020-10-15 14:46 ` [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs Borislav Petkov
@ 2020-10-15 14:46 ` Borislav Petkov
2020-10-15 14:46 ` [PATCH 3/4] tools/power/x86_energy_perf_policy: " Borislav Petkov
2020-10-15 14:46 ` [PATCH 4/4] x86/msr: Do not allow writes to MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
3 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-10-15 14:46 UTC (permalink / raw)
To: X86 ML; +Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
From: Borislav Petkov <bp@suse.de>
... instead of poking at the MSR directly.
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
---
tools/power/x86/turbostat/turbostat.c | 29 ++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 33b370865d16..0baec7ea1bbd 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1721,6 +1721,25 @@ int get_mp(int cpu, struct msr_counter *mp, unsigned long long *counterp)
return 0;
}
+int get_epb(int cpu)
+{
+ char path[128 + PATH_BYTES];
+ int ret, epb = -1;
+ FILE *fp;
+
+ sprintf(path, "/sys/devices/system/cpu/cpu%d/power/energy_perf_bias", cpu);
+
+ fp = fopen_or_die(path, "r");
+
+ ret = fscanf(fp, "%d", &epb);
+ if (ret != 1)
+ err(1, "%s(%s)", __func__, path);
+
+ fclose(fp);
+
+ return epb;
+}
+
void get_apic_id(struct thread_data *t)
{
unsigned int eax, ebx, ecx, edx;
@@ -3631,9 +3650,8 @@ dump_sysfs_pstate_config(void)
*/
int print_epb(struct thread_data *t, struct core_data *c, struct pkg_data *p)
{
- unsigned long long msr;
char *epb_string;
- int cpu;
+ int cpu, epb;
if (!has_epb)
return 0;
@@ -3649,10 +3667,11 @@ int print_epb(struct thread_data *t, struct core_data *c, struct pkg_data *p)
return -1;
}
- if (get_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, &msr))
+ epb = get_epb(cpu);
+ if (epb < 0)
return 0;
- switch (msr & 0xF) {
+ switch (epb) {
case ENERGY_PERF_BIAS_PERFORMANCE:
epb_string = "performance";
break;
@@ -3666,7 +3685,7 @@ int print_epb(struct thread_data *t, struct core_data *c, struct pkg_data *p)
epb_string = "custom";
break;
}
- fprintf(outf, "cpu%d: MSR_IA32_ENERGY_PERF_BIAS: 0x%08llx (%s)\n", cpu, msr, epb_string);
+ fprintf(outf, "cpu%d: EPB: %d (%s)\n", cpu, epb, epb_string);
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] tools/power/x86_energy_perf_policy: Read energy_perf_bias from sysfs
2020-10-15 14:45 [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
2020-10-15 14:46 ` [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs Borislav Petkov
2020-10-15 14:46 ` [PATCH 2/4] tools/power/turbostat: " Borislav Petkov
@ 2020-10-15 14:46 ` Borislav Petkov
2020-10-15 14:46 ` [PATCH 4/4] x86/msr: Do not allow writes to MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
3 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-10-15 14:46 UTC (permalink / raw)
To: X86 ML; +Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
From: Borislav Petkov <bp@suse.de>
... and stop poking at the MSR directly.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
.../x86_energy_perf_policy.c | 109 ++++++++++++++++--
1 file changed, 99 insertions(+), 10 deletions(-)
diff --git a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
index 3fe1eed900d4..ad6aed1fabf8 100644
--- a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
+++ b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
@@ -91,6 +91,9 @@ unsigned int has_hwp_request_pkg; /* IA32_HWP_REQUEST_PKG */
unsigned int bdx_highest_ratio;
+#define PATH_TO_CPU "/sys/devices/system/cpu/"
+#define SYSFS_PATH_MAX 255
+
/*
* maintain compatibility with original implementation, but don't document it:
*/
@@ -668,6 +671,48 @@ int put_msr(int cpu, int offset, unsigned long long new_msr)
return 0;
}
+static unsigned int read_sysfs(const char *path, char *buf, size_t buflen)
+{
+ ssize_t numread;
+ int fd;
+
+ fd = open(path, O_RDONLY);
+ if (fd == -1)
+ return 0;
+
+ numread = read(fd, buf, buflen - 1);
+ if (numread < 1) {
+ close(fd);
+ return 0;
+ }
+
+ buf[numread] = '\0';
+ close(fd);
+
+ return (unsigned int) numread;
+}
+
+static unsigned int write_sysfs(const char *path, char *buf, size_t buflen)
+{
+ ssize_t numwritten;
+ int fd;
+
+ fd = open(path, O_WRONLY);
+ if (fd == -1)
+ return 0;
+
+ numwritten = write(fd, buf, buflen - 1);
+ if (numwritten < 1) {
+ perror("write failed\n");
+ close(fd);
+ return -1;
+ }
+
+ close(fd);
+
+ return (unsigned int) numwritten;
+}
+
void print_hwp_cap(int cpu, struct msr_hwp_cap *cap, char *str)
{
if (cpu != -1)
@@ -745,17 +790,61 @@ void write_hwp_request(int cpu, struct msr_hwp_request *hwp_req, unsigned int ms
put_msr(cpu, msr_offset, msr);
}
+static int get_epb(int cpu)
+{
+ char path[SYSFS_PATH_MAX];
+ char linebuf[3];
+ char *endp;
+ long val;
+
+ if (!has_epb)
+ return -1;
+
+ snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
+
+ if (!read_sysfs(path, linebuf, 3))
+ return -1;
+
+ val = strtol(linebuf, &endp, 0);
+ if (endp == linebuf || errno == ERANGE)
+ return -1;
+
+ return (int)val;
+}
+
+static int set_epb(int cpu, int val)
+{
+ char path[SYSFS_PATH_MAX];
+ char linebuf[3];
+ char *endp;
+ int ret;
+
+ if (!has_epb)
+ return -1;
+
+ snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
+ snprintf(linebuf, sizeof(linebuf), "%d", val);
+
+ ret = write_sysfs(path, linebuf, 3);
+ if (ret <= 0)
+ return -1;
+
+ val = strtol(linebuf, &endp, 0);
+ if (endp == linebuf || errno == ERANGE)
+ return -1;
+
+ return (int)val;
+}
+
int print_cpu_msrs(int cpu)
{
- unsigned long long msr;
struct msr_hwp_request req;
struct msr_hwp_cap cap;
+ int epb;
- if (has_epb) {
- get_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, &msr);
-
- printf("cpu%d: EPB %u\n", cpu, (unsigned int) msr);
- }
+ epb = get_epb(cpu);
+ if (epb >= 0)
+ printf("cpu%d: EPB %u\n", cpu, (unsigned int) epb);
if (!has_hwp)
return 0;
@@ -1038,15 +1127,15 @@ int enable_hwp_on_cpu(int cpu)
int update_cpu_msrs(int cpu)
{
unsigned long long msr;
-
+ int epb;
if (update_epb) {
- get_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, &msr);
- put_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, new_epb);
+ epb = get_epb(cpu);
+ set_epb(cpu, new_epb);
if (verbose)
printf("cpu%d: ENERGY_PERF_BIAS old: %d new: %d\n",
- cpu, (unsigned int) msr, (unsigned int) new_epb);
+ cpu, epb, (unsigned int) new_epb);
}
if (update_turbo) {
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] x86/msr: Do not allow writes to MSR_IA32_ENERGY_PERF_BIAS
2020-10-15 14:45 [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
` (2 preceding siblings ...)
2020-10-15 14:46 ` [PATCH 3/4] tools/power/x86_energy_perf_policy: " Borislav Petkov
@ 2020-10-15 14:46 ` Borislav Petkov
3 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-10-15 14:46 UTC (permalink / raw)
To: X86 ML; +Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
From: Borislav Petkov <bp@suse.de>
Now that all in-kernel-tree users are converted to using the sysfs file,
remove the MSR from the "allowlist".
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/msr.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index c0d409810658..b1147862730c 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -99,9 +99,6 @@ static int filter_write(u32 reg)
if (!__ratelimit(&fw_rs))
return 0;
- if (reg == MSR_IA32_ENERGY_PERF_BIAS)
- return 0;
-
pr_err("Write to unrecognized MSR 0x%x by %s (pid: %d). Please report to x86@kernel.org.\n",
reg, current->comm, current->pid);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-15 14:46 ` [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs Borislav Petkov
@ 2020-10-15 17:49 ` Shuah Khan
2020-10-16 8:37 ` Borislav Petkov
0 siblings, 1 reply; 12+ messages in thread
From: Shuah Khan @ 2020-10-15 17:49 UTC (permalink / raw)
To: Borislav Petkov, X86 ML
Cc: Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML, Shuah Khan
On 10/15/20 8:46 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> ... instead of poking at the MSR. For that, move the accessor functions
> to misc.c and add a sysfs-writing function too.
> > There should be no functional changes resulting from this.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Thomas Renninger <trenn@suse.com>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
> tools/power/cpupower/lib/cpupower.c | 23 +++++++++-
> tools/power/cpupower/lib/cpupower_intern.h | 5 ++
> tools/power/cpupower/utils/cpupower-info.c | 2 +-
> tools/power/cpupower/utils/cpupower-set.c | 2 +-
> tools/power/cpupower/utils/helpers/helpers.h | 8 ++--
> tools/power/cpupower/utils/helpers/misc.c | 48 ++++++++++++++++++++
> tools/power/cpupower/utils/helpers/msr.c | 28 ------------
> 7 files changed, 81 insertions(+), 35 deletions(-)
>
> diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
> index 3656e697537e..d2fa4bbecf74 100644
> --- a/tools/power/cpupower/lib/cpupower.c
> +++ b/tools/power/cpupower/lib/cpupower.c
> @@ -16,8 +16,8 @@
>
> unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen)
> {
> - int fd;
> ssize_t numread;
> + int fd;
>
Is there a reason to move "int fd"?
> fd = open(path, O_RDONLY);
> if (fd == -1)
> @@ -35,6 +35,27 @@ unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen)
> return (unsigned int) numread;
> }
>
> +unsigned int cpupower_write_sysfs(const char *path, char *buf, size_t buflen)
> +{
> + ssize_t numwritten;
> + int fd;
> +
> + fd = open(path, O_WRONLY);
> + if (fd == -1)
> + return 0;
> +
> + numwritten = write(fd, buf, buflen - 1);
> + if (numwritten < 1) {
> + perror("write failed");
Please add filename to the error message
> + close(fd);
> + return -1;
> + }
> +
> + close(fd);
> +
> + return (unsigned int) numwritten;
> +}
> +
> /*
> * Detect whether a CPU is online
> *
> diff --git a/tools/power/cpupower/lib/cpupower_intern.h b/tools/power/cpupower/lib/cpupower_intern.h
> index 4887c76d23f8..ac1112b956ec 100644
> --- a/tools/power/cpupower/lib/cpupower_intern.h
> +++ b/tools/power/cpupower/lib/cpupower_intern.h
> @@ -1,6 +1,11 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #define PATH_TO_CPU "/sys/devices/system/cpu/"
> +
> +#ifndef MAX_LINE_LEN
> #define MAX_LINE_LEN 4096
> +#endif
> +
> #define SYSFS_PATH_MAX 255
>
> unsigned int cpupower_read_sysfs(const char *path, char *buf, size_t buflen);
> +unsigned int cpupower_write_sysfs(const char *path, char *buf, size_t buflen);
> diff --git a/tools/power/cpupower/utils/cpupower-info.c b/tools/power/cpupower/utils/cpupower-info.c
> index 0ba61a2c4d81..06345b543786 100644
> --- a/tools/power/cpupower/utils/cpupower-info.c
> +++ b/tools/power/cpupower/utils/cpupower-info.c
> @@ -101,7 +101,7 @@ int cmd_info(int argc, char **argv)
> }
>
> if (params.perf_bias) {
> - ret = msr_intel_get_perf_bias(cpu);
> + ret = cpupower_intel_get_perf_bias(cpu);
> if (ret < 0) {
> fprintf(stderr,
> _("Could not read perf-bias value[%d]\n"), ret);
> diff --git a/tools/power/cpupower/utils/cpupower-set.c b/tools/power/cpupower/utils/cpupower-set.c
> index 052044d7e012..180d5ba877e6 100644
> --- a/tools/power/cpupower/utils/cpupower-set.c
> +++ b/tools/power/cpupower/utils/cpupower-set.c
> @@ -95,7 +95,7 @@ int cmd_set(int argc, char **argv)
> }
>
> if (params.perf_bias) {
> - ret = msr_intel_set_perf_bias(cpu, perf_bias);
> + ret = cpupower_intel_set_perf_bias(cpu, perf_bias);
> if (ret) {
> fprintf(stderr, _("Error setting perf-bias "
> "value on CPU %d\n"), cpu);
> diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
> index c258eeccd05f..37dac161f3fe 100644
> --- a/tools/power/cpupower/utils/helpers/helpers.h
> +++ b/tools/power/cpupower/utils/helpers/helpers.h
> @@ -105,8 +105,8 @@ extern struct cpupower_cpu_info cpupower_cpu_info;
> extern int read_msr(int cpu, unsigned int idx, unsigned long long *val);
> extern int write_msr(int cpu, unsigned int idx, unsigned long long val);
>
> -extern int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val);
> -extern int msr_intel_get_perf_bias(unsigned int cpu);
> +extern int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val);
> +extern int cpupower_intel_get_perf_bias(unsigned int cpu);
> extern unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu);
>
> /* Read/Write msr ****************************/
> @@ -150,9 +150,9 @@ static inline int read_msr(int cpu, unsigned int idx, unsigned long long *val)
> { return -1; };
> static inline int write_msr(int cpu, unsigned int idx, unsigned long long val)
> { return -1; };
> -static inline int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val)
> +static inline int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val)
> { return -1; };
> -static inline int msr_intel_get_perf_bias(unsigned int cpu)
> +static inline int cpupower_intel_get_perf_bias(unsigned int cpu)
> { return -1; };
> static inline unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu)
> { return 0; };
> diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
> index f406adc40bad..e8f8f643a627 100644
> --- a/tools/power/cpupower/utils/helpers/misc.c
> +++ b/tools/power/cpupower/utils/helpers/misc.c
> @@ -1,7 +1,15 @@
> // SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +
> #if defined(__i386__) || defined(__x86_64__)
>
> #include "helpers/helpers.h"
> +#include "helpers/sysfs.h"
> +
> +#include "cpupower_intern.h"
>
> #define MSR_AMD_HWCR 0xc0010015
>
> @@ -40,4 +48,44 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> *support = *active = 1;
> return 0;
> }
> +
> +int cpupower_intel_get_perf_bias(unsigned int cpu)
> +{
> + char linebuf[MAX_LINE_LEN];
> + char path[SYSFS_PATH_MAX];
> + unsigned long val;
> + char *endp;
> +
> + if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
> + return -1;
> +
> + snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
Please add return check for snprintf, please add a define for
"cpu%u/power/energy_perf_bias" since it is hardcoded in
read/write functions.
> +
> + if (cpupower_read_sysfs(path, linebuf, MAX_LINE_LEN) == 0)
> + return -1;
> +
> + val = strtol(linebuf, &endp, 0);
> + if (endp == linebuf || errno == ERANGE)
> + return -1;
> +
> + return val;
> +}
> +
> +int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val)
> +{
> + char path[SYSFS_PATH_MAX];
> + char linebuf[3] = {};
> +
> + if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
> + return -1;
> +
> + snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/power/energy_perf_bias", cpu);
Please add return check for snprintf, please add a define for
"cpu%u/power/energy_perf_bias" since it is hardcoded in
read/write functions.
> + snprintf(linebuf, sizeof(linebuf), "%d", val);
return value check?
> +
> + if (cpupower_write_sysfs(path, linebuf, 3) <= 0)
> + return -1;
> +
> + return 0;
> +}
> +
> #endif /* #if defined(__i386__) || defined(__x86_64__) */
> diff --git a/tools/power/cpupower/utils/helpers/msr.c b/tools/power/cpupower/utils/helpers/msr.c
> index ab9950748838..8b0b6be74bb8 100644
> --- a/tools/power/cpupower/utils/helpers/msr.c
> +++ b/tools/power/cpupower/utils/helpers/msr.c
> @@ -11,7 +11,6 @@
> /* Intel specific MSRs */
> #define MSR_IA32_PERF_STATUS 0x198
> #define MSR_IA32_MISC_ENABLES 0x1a0
> -#define MSR_IA32_ENERGY_PERF_BIAS 0x1b0
> #define MSR_NEHALEM_TURBO_RATIO_LIMIT 0x1ad
>
> /*
> @@ -73,33 +72,6 @@ int write_msr(int cpu, unsigned int idx, unsigned long long val)
> return -1;
> }
>
> -int msr_intel_get_perf_bias(unsigned int cpu)
> -{
> - unsigned long long val;
> - int ret;
> -
> - if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
> - return -1;
> -
> - ret = read_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, &val);
> - if (ret)
> - return ret;
> - return val;
> -}
> -
> -int msr_intel_set_perf_bias(unsigned int cpu, unsigned int val)
> -{
> - int ret;
> -
> - if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS))
> - return -1;
> -
> - ret = write_msr(cpu, MSR_IA32_ENERGY_PERF_BIAS, val);
> - if (ret)
> - return ret;
> - return 0;
> -}
> -
> unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu)
> {
> unsigned long long val;
>
thnaks,
-- Shuah
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-15 17:49 ` Shuah Khan
@ 2020-10-16 8:37 ` Borislav Petkov
2020-10-29 21:38 ` Shuah Khan
0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2020-10-16 8:37 UTC (permalink / raw)
To: Shuah Khan
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
On Thu, Oct 15, 2020 at 11:49:32AM -0600, Shuah Khan wrote:
> Is there a reason to move "int fd"?
Habit from tip - we sort function-local variables in a reverse fir tree
order. And since I'm adding cpupower_write_sysfs(), I made them look
consistent.
> > + numwritten = write(fd, buf, buflen - 1);
> > + if (numwritten < 1) {
> > + perror("write failed");
>
> Please add filename to the error message
perror(path);
or do you want me to build a string with an error message and filename?
> Please add return check for snprintf, please add a define for
> "cpu%u/power/energy_perf_bias" since it is hardcoded in
> read/write functions.
None of the other snprintf() calls in cpupower do that. Nothing checks
snprintf() retval and the last part of the sysfs path is a naked string.
Why is this different?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-16 8:37 ` Borislav Petkov
@ 2020-10-29 21:38 ` Shuah Khan
2020-10-29 21:59 ` Borislav Petkov
0 siblings, 1 reply; 12+ messages in thread
From: Shuah Khan @ 2020-10-29 21:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML,
Shuah Khan
On 10/16/20 2:37 AM, Borislav Petkov wrote:
> On Thu, Oct 15, 2020 at 11:49:32AM -0600, Shuah Khan wrote:
>> Is there a reason to move "int fd"?
>
Sorry for a late response.
Okay. Looked odd since it didn't need changing.
> Habit from tip - we sort function-local variables in a reverse fir tree
> order. And since I'm adding cpupower_write_sysfs(), I made them look
> consistent.
>
>>> + numwritten = write(fd, buf, buflen - 1);
>>> + if (numwritten < 1) {
>>> + perror("write failed");
>>
>> Please add filename to the error message
>
> perror(path);
>
> or do you want me to build a string with an error message and filename?
Right. It will be great if you can add filename to the message.
>
>> Please add return check for snprintf, please add a define for
>> "cpu%u/power/energy_perf_bias" since it is hardcoded in
>> read/write functions.
>
> None of the other snprintf() calls in cpupower do that. Nothing checks
> snprintf() retval and the last part of the sysfs path is a naked string.
>
> Why is this different?
>
All of the other ones should be changed as such. Why add more?
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-29 21:38 ` Shuah Khan
@ 2020-10-29 21:59 ` Borislav Petkov
2020-10-29 22:32 ` Shuah Khan
0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2020-10-29 21:59 UTC (permalink / raw)
To: Shuah Khan
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
On Thu, Oct 29, 2020 at 03:38:43PM -0600, Shuah Khan wrote:
> All of the other ones should be changed as such. Why add more?
Because a patch should do one thing and one thing only. So a separate
patch which converts them all in one go should come ontop. But if you
insist for the ones I'm adding to have error handling, I can do that, of
course.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-29 21:59 ` Borislav Petkov
@ 2020-10-29 22:32 ` Shuah Khan
2020-10-29 23:11 ` Borislav Petkov
2020-11-02 11:52 ` Borislav Petkov
0 siblings, 2 replies; 12+ messages in thread
From: Shuah Khan @ 2020-10-29 22:32 UTC (permalink / raw)
To: Borislav Petkov
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML,
Shuah Khan
On 10/29/20 3:59 PM, Borislav Petkov wrote:
> On Thu, Oct 29, 2020 at 03:38:43PM -0600, Shuah Khan wrote:
>> All of the other ones should be changed as such. Why add more?
>
> Because a patch should do one thing and one thing only. So a separate
> patch which converts them all in one go should come ontop. But if you
> insist for the ones I'm adding to have error handling, I can do that, of
> course.
>
A separate patch is fine.
if (numwritten < 1) {
+ perror("write failed");
Please add filename to the error message.
Thanks. Since this depends on other changes, it can go through
tip once this error message change is made.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-29 22:32 ` Shuah Khan
@ 2020-10-29 23:11 ` Borislav Petkov
2020-11-02 11:52 ` Borislav Petkov
1 sibling, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-10-29 23:11 UTC (permalink / raw)
To: Shuah Khan
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
On Thu, Oct 29, 2020 at 04:32:43PM -0600, Shuah Khan wrote:
> if (numwritten < 1) {
> + perror("write failed");
>
> Please add filename to the error message.
Yes, you said so already and will do that in the next version.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs
2020-10-29 22:32 ` Shuah Khan
2020-10-29 23:11 ` Borislav Petkov
@ 2020-11-02 11:52 ` Borislav Petkov
1 sibling, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2020-11-02 11:52 UTC (permalink / raw)
To: Shuah Khan
Cc: X86 ML, Thomas Renninger, Shuah Khan, Len Brown, linux-pm, LKML
On Thu, Oct 29, 2020 at 04:32:43PM -0600, Shuah Khan wrote:
> > Because a patch should do one thing and one thing only. So a separate
> > patch which converts them all in one go should come ontop. But if you
> > insist for the ones I'm adding to have error handling, I can do that, of
> > course.
> >
>
> A separate patch is fine.
Here's how it could look like, I've converted two users for now to get
your opinion first before I convert the rest.
Thx.
---
diff --git a/tools/power/cpupower/bench/Makefile b/tools/power/cpupower/bench/Makefile
index f68b4bc55273..d4ac380f7a56 100644
--- a/tools/power/cpupower/bench/Makefile
+++ b/tools/power/cpupower/bench/Makefile
@@ -9,13 +9,14 @@ endif
ifeq ($(strip $(STATIC)),true)
LIBS = -L../ -L$(OUTPUT) -lm
OBJS = $(OUTPUT)main.o $(OUTPUT)parse.o $(OUTPUT)system.o $(OUTPUT)benchmark.o \
- $(OUTPUT)../lib/cpufreq.o $(OUTPUT)../lib/cpupower.o
+ $(OUTPUT)../lib/cpufreq.o $(OUTPUT)../lib/cpupower.o $(OUTPUT)../utils/helpers/string.o
else
LIBS = -L../ -L$(OUTPUT) -lm -lcpupower
-OBJS = $(OUTPUT)main.o $(OUTPUT)parse.o $(OUTPUT)system.o $(OUTPUT)benchmark.o
+OBJS = $(OUTPUT)main.o $(OUTPUT)parse.o $(OUTPUT)system.o $(OUTPUT)benchmark.o \
+ $(OUTPUT)../utils/helpers/string.o
endif
-CFLAGS += -D_GNU_SOURCE -I../lib -DDEFAULT_CONFIG_FILE=\"$(confdir)/cpufreq-bench.conf\"
+CFLAGS += -D_GNU_SOURCE -I../lib -I../utils -DDEFAULT_CONFIG_FILE=\"$(confdir)/cpufreq-bench.conf\"
$(OUTPUT)%.o : %.c
$(ECHO) " CC " $@
diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
index e63dc11fa3a5..54ba48ad0983 100644
--- a/tools/power/cpupower/bench/parse.c
+++ b/tools/power/cpupower/bench/parse.c
@@ -18,6 +18,8 @@
#include "parse.h"
#include "config.h"
+#include "helpers/helpers.h"
+
/**
* converts priority string to priority
*
@@ -84,11 +86,12 @@ FILE *prepare_output(const char *dirname)
}
filename = filename_tmp;
- snprintf(filename, len - 1, "%s/benchmark_%s_%s_%li.log",
- dirname, sysdata.nodename, sysdata.release, time(NULL));
+ if (!cpupower_snprintf(filename, len - 1, "%s/benchmark_%s_%s_%li.log",
+ dirname, sysdata.nodename, sysdata.release, time(NULL)))
+ return NULL;
} else {
- snprintf(filename, len - 1, "%s/benchmark_%li.log",
- dirname, time(NULL));
+ if (!cpupower_snprintf(filename, len - 1, "%s/benchmark_%li.log", dirname, time(NULL)))
+ return NULL;
}
dprintf("logfilename: %s\n", filename);
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 37dac161f3fe..dadd919ee599 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -171,4 +171,6 @@ static inline unsigned int cpuid_ecx(unsigned int op) { return 0; };
static inline unsigned int cpuid_edx(unsigned int op) { return 0; };
#endif /* defined(__i386__) || defined(__x86_64__) */
+extern char *cpupower_snprintf(char *str, int size, const char *fmt, ...);
+
#endif /* __CPUPOWERUTILS_HELPERS__ */
diff --git a/tools/power/cpupower/utils/helpers/string.c b/tools/power/cpupower/utils/helpers/string.c
new file mode 100644
index 000000000000..bbd5e90bdc6e
--- /dev/null
+++ b/tools/power/cpupower/utils/helpers/string.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdarg.h>
+
+#include "helpers/helpers.h"
+
+/*
+ * A helper with error handling for less code clutter.
+ */
+char *cpupower_snprintf(char *str, int size, const char *fmt, ...)
+{
+ int sz = 0;
+ va_list ap;
+
+ va_start(ap, fmt);
+ sz = vsnprintf(str, size, fmt, ap);
+ va_end(ap);
+
+ if (sz < 0) {
+ perror("vsnprintf");
+ return NULL;
+ }
+
+ if (sz >= size) {
+ fprintf(stderr, "Output truncated: [%s]\n", str);
+ return NULL;
+ }
+
+ return str;
+}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-11-02 11:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 14:45 [PATCH 0/4] x86: Remove direct use of MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
2020-10-15 14:46 ` [PATCH 1/4] tools/power/cpupower: Read energy_perf_bias from sysfs Borislav Petkov
2020-10-15 17:49 ` Shuah Khan
2020-10-16 8:37 ` Borislav Petkov
2020-10-29 21:38 ` Shuah Khan
2020-10-29 21:59 ` Borislav Petkov
2020-10-29 22:32 ` Shuah Khan
2020-10-29 23:11 ` Borislav Petkov
2020-11-02 11:52 ` Borislav Petkov
2020-10-15 14:46 ` [PATCH 2/4] tools/power/turbostat: " Borislav Petkov
2020-10-15 14:46 ` [PATCH 3/4] tools/power/x86_energy_perf_policy: " Borislav Petkov
2020-10-15 14:46 ` [PATCH 4/4] x86/msr: Do not allow writes to MSR_IA32_ENERGY_PERF_BIAS Borislav Petkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.