* [PATCH 1/2] platform/x86: ISST: Allow additional TRL MSRs @ 2019-09-05 23:37 Srinivas Pandruvada 2019-09-05 23:37 ` [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket Srinivas Pandruvada 0 siblings, 1 reply; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-05 23:37 UTC (permalink / raw) To: andriy.shevchenko Cc: prarit, darcari, linux-kernel, platform-driver-x86, Srinivas Pandruvada Additional Turbo Ratio Limit (TRL) MSRs are required to get bucket vs core count relationship. So add them to the list of allowed MSRs. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/platform/x86/intel_speed_select_if/isst_if_common.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c index 68d75391db57..3de5a3c66529 100644 --- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c +++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c @@ -29,6 +29,8 @@ static struct isst_if_cmd_cb punit_callbacks[ISST_IF_DEV_MAX]; static int punit_msr_white_list[] = { MSR_TURBO_RATIO_LIMIT, MSR_CONFIG_TDP_CONTROL, + MSR_TURBO_RATIO_LIMIT1, + MSR_TURBO_RATIO_LIMIT2, }; struct isst_valid_cmd_ranges { -- 2.17.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-05 23:37 [PATCH 1/2] platform/x86: ISST: Allow additional TRL MSRs Srinivas Pandruvada @ 2019-09-05 23:37 ` Srinivas Pandruvada 2019-09-06 9:39 ` Prarit Bhargava 0 siblings, 1 reply; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-05 23:37 UTC (permalink / raw) To: andriy.shevchenko Cc: prarit, darcari, linux-kernel, platform-driver-x86, Srinivas Pandruvada Read the bucket and core count relationship via MSR and display when displaying turbo ratio limits. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../power/x86/intel-speed-select/isst-core.c | 22 +++++++++++++++++++ .../x86/intel-speed-select/isst-display.c | 6 ++--- tools/power/x86/intel-speed-select/isst.h | 1 + 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c index 8de4ac39a008..2f864c4b994d 100644 --- a/tools/power/x86/intel-speed-select/isst-core.c +++ b/tools/power/x86/intel-speed-select/isst-core.c @@ -188,6 +188,24 @@ int isst_get_get_trl(int cpu, int level, int avx_level, int *trl) return 0; } +int isst_get_trl_bucket_info(int cpu, unsigned long long *buckets_info) +{ + int ret; + + debug_printf("cpu:%d bucket info via MSR\n", cpu); + + *buckets_info = 0; + + ret = isst_send_msr_command(cpu, 0x1ae, 0, buckets_info); + if (ret) + return ret; + + debug_printf("cpu:%d bucket info via MSR successful 0x%llx\n", cpu, + *buckets_info); + + return 0; +} + int isst_set_tdp_level_msr(int cpu, int tdp_level) { int ret; @@ -563,6 +581,10 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev) if (ret) return ret; + ret = isst_get_trl_bucket_info(cpu, &ctdp_level->buckets_info); + if (ret) + return ret; + ret = isst_get_get_trl(cpu, i, 0, ctdp_level->trl_sse_active_cores); if (ret) diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c index 8500cf2997a6..df4aa99c4e92 100644 --- a/tools/power/x86/intel-speed-select/isst-display.c +++ b/tools/power/x86/intel-speed-select/isst-display.c @@ -372,7 +372,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, format_and_print(outf, base_level + 5, header, NULL); snprintf(header, sizeof(header), "core-count"); - snprintf(value, sizeof(value), "%d", j); + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); format_and_print(outf, base_level + 6, header, value); snprintf(header, sizeof(header), @@ -389,7 +389,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, format_and_print(outf, base_level + 5, header, NULL); snprintf(header, sizeof(header), "core-count"); - snprintf(value, sizeof(value), "%d", j); + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); format_and_print(outf, base_level + 6, header, value); snprintf(header, sizeof(header), @@ -407,7 +407,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, format_and_print(outf, base_level + 5, header, NULL); snprintf(header, sizeof(header), "core-count"); - snprintf(value, sizeof(value), "%d", j); + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); format_and_print(outf, base_level + 6, header, value); snprintf(header, sizeof(header), diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h index 221881761609..2f7f62765eb6 100644 --- a/tools/power/x86/intel-speed-select/isst.h +++ b/tools/power/x86/intel-speed-select/isst.h @@ -134,6 +134,7 @@ struct isst_pkg_ctdp_level_info { size_t core_cpumask_size; cpu_set_t *core_cpumask; int cpu_count; + unsigned long long buckets_info; int trl_sse_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; int trl_avx_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; int trl_avx_512_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; -- 2.17.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-05 23:37 ` [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket Srinivas Pandruvada @ 2019-09-06 9:39 ` Prarit Bhargava 2019-09-06 13:46 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Prarit Bhargava @ 2019-09-06 9:39 UTC (permalink / raw) To: Srinivas Pandruvada, andriy.shevchenko Cc: darcari, linux-kernel, platform-driver-x86 On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > Read the bucket and core count relationship via MSR and display > when displaying turbo ratio limits. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > .../power/x86/intel-speed-select/isst-core.c | 22 +++++++++++++++++++ > .../x86/intel-speed-select/isst-display.c | 6 ++--- > tools/power/x86/intel-speed-select/isst.h | 1 + > 3 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c > index 8de4ac39a008..2f864c4b994d 100644 > --- a/tools/power/x86/intel-speed-select/isst-core.c > +++ b/tools/power/x86/intel-speed-select/isst-core.c > @@ -188,6 +188,24 @@ int isst_get_get_trl(int cpu, int level, int avx_level, int *trl) > return 0; > } > > +int isst_get_trl_bucket_info(int cpu, unsigned long long *buckets_info) > +{ > + int ret; > + > + debug_printf("cpu:%d bucket info via MSR\n", cpu); > + > + *buckets_info = 0; > + > + ret = isst_send_msr_command(cpu, 0x1ae, 0, buckets_info); ^^^ you can get rid of the magic number 0x1ae by doing (sorry for the cut-and-paste) diff --git a/tools/power/x86/intel-speed-select/Makefile b/tools/power/x86/intel index 12c6939dca2a..087d802ad844 100644 --- a/tools/power/x86/intel-speed-select/Makefile +++ b/tools/power/x86/intel-speed-select/Makefile @@ -15,6 +15,8 @@ endif MAKEFLAGS += -r override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include +override CFLAGS += -I../../../include +override CFLAGS += -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' ALL_TARGETS := intel-speed-select ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-s index 2f7f62765eb6..00d159dc12a6 100644 --- a/tools/power/x86/intel-speed-select/isst.h +++ b/tools/power/x86/intel-speed-select/isst.h @@ -7,6 +7,7 @@ #ifndef _ISST_H_ #define _ISST_H_ +#include MSRHEADER #include <stdio.h> #include <unistd.h> #include <sys/types.h> and replacing the MSR addresses with the names of the MSRs. > + if (ret) > + return ret; > + As I've been looking at this code I have been wondering why didn't you just use the standard /dev/cpu/X/msr interface that other x86 power utilities (turbostat, x86_energy_perf_policy) use? Implementing msr_read() is trivial (warning untested and uncompiled code) static void read_msr(int cpu, int offset, unsigned long long *msr) { ssize_t retval; char pathname[32]; int fd; sprintf(pathname, "/dev/cpu/%d/msr", cpu); fd = open(pathname, O_RDONLY); if (fd < 0) err(-1, "%s open failed", pathname); retval = pread(fd, msr, sizeof(*msr), offset); if (retval != (sizeof *msr)) err(-1, "%s failed: cpu %d msr offset 0x%llx\n", __func__, cpu, (unsigned long long)offset); close(fd); } and would result in a significant reduction in code in the driver and the tool IMO. write_msr() is equally trivial. P. > + debug_printf("cpu:%d bucket info via MSR successful 0x%llx\n", cpu, > + *buckets_info); > + > + return 0; > +} > + > int isst_set_tdp_level_msr(int cpu, int tdp_level) > { > int ret; > @@ -563,6 +581,10 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev) > if (ret) > return ret; > > + ret = isst_get_trl_bucket_info(cpu, &ctdp_level->buckets_info); > + if (ret) > + return ret; > + > ret = isst_get_get_trl(cpu, i, 0, > ctdp_level->trl_sse_active_cores); > if (ret) > diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c > index 8500cf2997a6..df4aa99c4e92 100644 > --- a/tools/power/x86/intel-speed-select/isst-display.c > +++ b/tools/power/x86/intel-speed-select/isst-display.c > @@ -372,7 +372,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, > format_and_print(outf, base_level + 5, header, NULL); > > snprintf(header, sizeof(header), "core-count"); > - snprintf(value, sizeof(value), "%d", j); > + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); > format_and_print(outf, base_level + 6, header, value); > > snprintf(header, sizeof(header), > @@ -389,7 +389,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, > format_and_print(outf, base_level + 5, header, NULL); > > snprintf(header, sizeof(header), "core-count"); > - snprintf(value, sizeof(value), "%d", j); > + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); > format_and_print(outf, base_level + 6, header, value); > > snprintf(header, sizeof(header), > @@ -407,7 +407,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level, > format_and_print(outf, base_level + 5, header, NULL); > > snprintf(header, sizeof(header), "core-count"); > - snprintf(value, sizeof(value), "%d", j); > + snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff); > format_and_print(outf, base_level + 6, header, value); > > snprintf(header, sizeof(header), > diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h > index 221881761609..2f7f62765eb6 100644 > --- a/tools/power/x86/intel-speed-select/isst.h > +++ b/tools/power/x86/intel-speed-select/isst.h > @@ -134,6 +134,7 @@ struct isst_pkg_ctdp_level_info { > size_t core_cpumask_size; > cpu_set_t *core_cpumask; > int cpu_count; > + unsigned long long buckets_info; > int trl_sse_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; > int trl_avx_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; > int trl_avx_512_active_cores[ISST_TRL_MAX_ACTIVE_CORES]; > ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-06 9:39 ` Prarit Bhargava @ 2019-09-06 13:46 ` Andy Shevchenko 2019-09-06 14:50 ` Srinivas Pandruvada 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2019-09-06 13:46 UTC (permalink / raw) To: Prarit Bhargava Cc: Srinivas Pandruvada, darcari, linux-kernel, platform-driver-x86 On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava wrote: > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > Read the bucket and core count relationship via MSR and display > > when displaying turbo ratio limits. > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, buckets_info); > > ^^^ you can get rid of the magic number 0x1ae by doing (sorry for the cut-and-paste) > > diff --git a/tools/power/x86/intel-speed-select/Makefile b/tools/power/x86/intel > index 12c6939dca2a..087d802ad844 100644 > --- a/tools/power/x86/intel-speed-select/Makefile > +++ b/tools/power/x86/intel-speed-select/Makefile > @@ -15,6 +15,8 @@ endif > MAKEFLAGS += -r > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include > +override CFLAGS += -I../../../include > +override CFLAGS += -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' I guess it can be done in more neat way. > As I've been looking at this code I have been wondering why didn't you just use > the standard /dev/cpu/X/msr interface that other x86 power utilities (turbostat, > x86_energy_perf_policy) use? Implementing msr_read() is trivial (warning > untested and uncompiled code) Actually good point! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-06 13:46 ` Andy Shevchenko @ 2019-09-06 14:50 ` Srinivas Pandruvada 2019-09-06 15:07 ` Srinivas Pandruvada 0 siblings, 1 reply; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-06 14:50 UTC (permalink / raw) To: Andy Shevchenko, Prarit Bhargava Cc: darcari, linux-kernel, platform-driver-x86 On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: > On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava wrote: > > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > > Read the bucket and core count relationship via MSR and display > > > when displaying turbo ratio limits. > > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, buckets_info); > > > > ^^^ you can get rid of the magic number 0x1ae by doing (sorry for > > the cut-and-paste) > > > > diff --git a/tools/power/x86/intel-speed-select/Makefile > > b/tools/power/x86/intel > > index 12c6939dca2a..087d802ad844 100644 > > --- a/tools/power/x86/intel-speed-select/Makefile > > +++ b/tools/power/x86/intel-speed-select/Makefile > > @@ -15,6 +15,8 @@ endif > > MAKEFLAGS += -r > > > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include > > +override CFLAGS += -I../../../include > > +override CFLAGS += > > -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' No, we can't use msr_index. > > I guess it can be done in more neat way. > > > As I've been looking at this code I have been wondering why didn't > > you just use > > the standard /dev/cpu/X/msr interface that other x86 power > > utilities (turbostat, > > x86_energy_perf_policy) use? Implementing msr_read() is trivial > > (warning > > untested and uncompiled code) No. We can't. The MSR interface is disabled on several distribution and platforms with secured boot. So some special MSRs are only allowed via this IOCTL interface. Thanks, Srinivas > > Actually good point! > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-06 14:50 ` Srinivas Pandruvada @ 2019-09-06 15:07 ` Srinivas Pandruvada 2019-09-07 18:18 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-06 15:07 UTC (permalink / raw) To: Andy Shevchenko, Prarit Bhargava Cc: darcari, linux-kernel, platform-driver-x86 On Fri, 2019-09-06 at 07:50 -0700, Srinivas Pandruvada wrote: > On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: > > On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava wrote: > > > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > > > Read the bucket and core count relationship via MSR and display > > > > when displaying turbo ratio limits. > > > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, > > > > buckets_info); > > > > > > ^^^ you can get rid of the magic number 0x1ae by doing (sorry for > > > the cut-and-paste) > > > > > > diff --git a/tools/power/x86/intel-speed-select/Makefile > > > b/tools/power/x86/intel > > > index 12c6939dca2a..087d802ad844 100644 > > > --- a/tools/power/x86/intel-speed-select/Makefile > > > +++ b/tools/power/x86/intel-speed-select/Makefile > > > @@ -15,6 +15,8 @@ endif > > > MAKEFLAGS += -r > > > > > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include > > > +override CFLAGS += -I../../../include > > > +override CFLAGS += > > > -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' > > No, we can't use msr_index. This comment was meant for use of /dev/cpu/X/msr not msr_index. I didn't want to bring in dependency on msr-index.h for couple of 2 MSRs and the names in msr-index.h doesn't really reflect the actual processing, they are doing. For example MSR_TURBO_RATIO_LIMIT1 for 0x1ae. The definition of 0x1AE is different on cpu model 0x55 and beyond. > > > > > I guess it can be done in more neat way. > > > > > As I've been looking at this code I have been wondering why > > > didn't > > > you just use > > > the standard /dev/cpu/X/msr interface that other x86 power > > > utilities (turbostat, > > > x86_energy_perf_policy) use? Implementing msr_read() is trivial > > > (warning > > > untested and uncompiled code) > > No. We can't. The MSR interface is disabled on several distribution > and > platforms with secured boot. So some special MSRs are only allowed > via > this IOCTL interface. > > Thanks, > Srinivas > > > > > > Actually good point! > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-06 15:07 ` Srinivas Pandruvada @ 2019-09-07 18:18 ` Andy Shevchenko 2019-09-08 9:46 ` Prarit Bhargava 2019-09-08 14:43 ` Srinivas Pandruvada 0 siblings, 2 replies; 11+ messages in thread From: Andy Shevchenko @ 2019-09-07 18:18 UTC (permalink / raw) To: Srinivas Pandruvada Cc: Andy Shevchenko, Prarit Bhargava, David Arcari, Linux Kernel Mailing List, Platform Driver On Fri, Sep 6, 2019 at 10:47 PM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Fri, 2019-09-06 at 07:50 -0700, Srinivas Pandruvada wrote: > > On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: > > > On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava wrote: > > > > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > > > > Read the bucket and core count relationship via MSR and display > > > > > when displaying turbo ratio limits. > > > > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, > > > > > buckets_info); > > > > > > > > ^^^ you can get rid of the magic number 0x1ae by doing (sorry for > > > > the cut-and-paste) > > > > > > > > diff --git a/tools/power/x86/intel-speed-select/Makefile > > > > b/tools/power/x86/intel > > > > index 12c6939dca2a..087d802ad844 100644 > > > > --- a/tools/power/x86/intel-speed-select/Makefile > > > > +++ b/tools/power/x86/intel-speed-select/Makefile > > > > @@ -15,6 +15,8 @@ endif > > > > MAKEFLAGS += -r > > > > > > > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include > > > > +override CFLAGS += -I../../../include > > > > +override CFLAGS += > > > > -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' > > > > No, we can't use msr_index. > This comment was meant for use of /dev/cpu/X/msr not msr_index. > I didn't want to bring in dependency on msr-index.h for couple of 2 > MSRs and the names in msr-index.h doesn't really reflect the actual > processing, they are doing. For example MSR_TURBO_RATIO_LIMIT1 for > 0x1ae. The definition of 0x1AE is different on cpu model 0x55 and > beyond. > > > It seems not applicable on top of tools patch series I had applied before. > > > > > > I guess it can be done in more neat way. > > > > > > > As I've been looking at this code I have been wondering why > > > > didn't > > > > you just use > > > > the standard /dev/cpu/X/msr interface that other x86 power > > > > utilities (turbostat, > > > > x86_energy_perf_policy) use? Implementing msr_read() is trivial > > > > (warning > > > > untested and uncompiled code) > > > > No. We can't. The MSR interface is disabled on several distribution > > and > > platforms with secured boot. So some special MSRs are only allowed > > via > > this IOCTL interface. > > > > Thanks, > > Srinivas > > > > > > > > > > Actually good point! > > > > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-07 18:18 ` Andy Shevchenko @ 2019-09-08 9:46 ` Prarit Bhargava 2019-09-08 14:13 ` Srinivas Pandruvada 2019-09-08 14:43 ` Srinivas Pandruvada 1 sibling, 1 reply; 11+ messages in thread From: Prarit Bhargava @ 2019-09-08 9:46 UTC (permalink / raw) To: Andy Shevchenko, Srinivas Pandruvada Cc: Andy Shevchenko, David Arcari, Linux Kernel Mailing List, Platform Driver On 9/7/19 2:18 PM, Andy Shevchenko wrote: > On Fri, Sep 6, 2019 at 10:47 PM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: >> >> On Fri, 2019-09-06 at 07:50 -0700, Srinivas Pandruvada wrote: >>> On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: >>>> On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava wrote: >>>>> On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: >>>>>> Read the bucket and core count relationship via MSR and display >>>>>> when displaying turbo ratio limits. >>>>>> + ret = isst_send_msr_command(cpu, 0x1ae, 0, >>>>>> buckets_info); >>>>> >>>>> ^^^ you can get rid of the magic number 0x1ae by doing (sorry for >>>>> the cut-and-paste) >>>>> >>>>> diff --git a/tools/power/x86/intel-speed-select/Makefile >>>>> b/tools/power/x86/intel >>>>> index 12c6939dca2a..087d802ad844 100644 >>>>> --- a/tools/power/x86/intel-speed-select/Makefile >>>>> +++ b/tools/power/x86/intel-speed-select/Makefile >>>>> @@ -15,6 +15,8 @@ endif >>>>> MAKEFLAGS += -r >>>>> >>>>> override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include >>>>> +override CFLAGS += -I../../../include >>>>> +override CFLAGS += >>>>> -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' >>> >>> No, we can't use msr_index. >> This comment was meant for use of /dev/cpu/X/msr not msr_index. >> I didn't want to bring in dependency on msr-index.h for couple of 2 >> MSRs and the names in msr-index.h doesn't really reflect the actual >> processing, they are doing. For example MSR_TURBO_RATIO_LIMIT1 for >> 0x1ae. The definition of 0x1AE is different on cpu model 0x55 and >> beyond. >> >>> > > It seems not applicable on top of tools patch series I had applied before. > >>>> >>>> I guess it can be done in more neat way. >>>> >>>>> As I've been looking at this code I have been wondering why >>>>> didn't >>>>> you just use >>>>> the standard /dev/cpu/X/msr interface that other x86 power >>>>> utilities (turbostat, >>>>> x86_energy_perf_policy) use? Implementing msr_read() is trivial >>>>> (warning >>>>> untested and uncompiled code) >>> >>> No. We can't. The MSR interface is disabled on several distribution >>> and >>> platforms with secured boot. So some special MSRs are only allowed >>> via >>> this IOCTL interface. >>> Which distros don't have /dev/cpu/X/msr ? None of other Intel tools have this restriction (or requirement depending on your point of view). Why is intel-speed-select special that we have to jump through hoops? P. >>> Thanks, >>> Srinivas >>> >>> >>>> >>>> Actually good point! >>>> >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-08 9:46 ` Prarit Bhargava @ 2019-09-08 14:13 ` Srinivas Pandruvada 0 siblings, 0 replies; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-08 14:13 UTC (permalink / raw) To: Prarit Bhargava, Andy Shevchenko Cc: Andy Shevchenko, David Arcari, Linux Kernel Mailing List, Platform Driver On Sun, 2019-09-08 at 05:46 -0400, Prarit Bhargava wrote: > > On 9/7/19 2:18 PM, Andy Shevchenko wrote: > > On Fri, Sep 6, 2019 at 10:47 PM Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > > > On Fri, 2019-09-06 at 07:50 -0700, Srinivas Pandruvada wrote: > > > > On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava > > > > > wrote: > > > > > > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > > > > > > Read the bucket and core count relationship via MSR and > > > > > > > display > > > > > > > when displaying turbo ratio limits. > > > > > > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, > > > > > > > buckets_info); > > > > > > > > > > > > ^^^ you can get rid of the magic number 0x1ae by doing > > > > > > (sorry for > > > > > > the cut-and-paste) > > > > > > > > > > > > diff --git a/tools/power/x86/intel-speed-select/Makefile > > > > > > b/tools/power/x86/intel > > > > > > index 12c6939dca2a..087d802ad844 100644 > > > > > > --- a/tools/power/x86/intel-speed-select/Makefile > > > > > > +++ b/tools/power/x86/intel-speed-select/Makefile > > > > > > @@ -15,6 +15,8 @@ endif > > > > > > MAKEFLAGS += -r > > > > > > > > > > > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE > > > > > > -I$(OUTPUT)include > > > > > > +override CFLAGS += -I../../../include > > > > > > +override CFLAGS += > > > > > > -DMSRHEADER='"../../../../arch/x86/include/asm/msr- > > > > > > index.h"' > > > > > > > > No, we can't use msr_index. > > > > > > This comment was meant for use of /dev/cpu/X/msr not msr_index. > > > I didn't want to bring in dependency on msr-index.h for couple of > > > 2 > > > MSRs and the names in msr-index.h doesn't really reflect the > > > actual > > > processing, they are doing. For example MSR_TURBO_RATIO_LIMIT1 > > > for > > > 0x1ae. The definition of 0x1AE is different on cpu model 0x55 and > > > beyond. > > > > > > > > > > > It seems not applicable on top of tools patch series I had applied > > before. > > > > > > > > > > > > I guess it can be done in more neat way. > > > > > > > > > > > As I've been looking at this code I have been wondering why > > > > > > didn't > > > > > > you just use > > > > > > the standard /dev/cpu/X/msr interface that other x86 power > > > > > > utilities (turbostat, > > > > > > x86_energy_perf_policy) use? Implementing msr_read() is > > > > > > trivial > > > > > > (warning > > > > > > untested and uncompiled code) > > > > > > > > No. We can't. The MSR interface is disabled on several > > > > distribution > > > > and > > > > platforms with secured boot. So some special MSRs are only > > > > allowed > > > > via > > > > this IOCTL interface. > > > > > > Which distros don't have /dev/cpu/X/msr ? Google "UEFI Secure Boot" The patch for kernel lock down was from RedHat, which restricted MSRs! > > None of other Intel tools have this restriction (or requirement > depending on > your point of view). Why is intel-speed-select special that we have > to > jump through hoops? Not sure why you call jumping hoops here. You are comparing debug tools with run time service from kernel for Intel speed select. This is not about tool. This tool is giving example how user spaces can control SST features through a kernel interface which should work with any distribution including home grown distros. And this interface is not for testing like turbostat or similar. So if any orchestration software can send same message as this tool and able to control SST features whether there is MSR access is there or not. Also you are missing feature for save/restore state after suspend/resume. > > P. > > > > > Thanks, > > > > Srinivas > > > > > > > > > > > > > > > > > > Actually good point! > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-07 18:18 ` Andy Shevchenko 2019-09-08 9:46 ` Prarit Bhargava @ 2019-09-08 14:43 ` Srinivas Pandruvada 2019-09-09 10:24 ` Andy Shevchenko 1 sibling, 1 reply; 11+ messages in thread From: Srinivas Pandruvada @ 2019-09-08 14:43 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Prarit Bhargava, David Arcari, Linux Kernel Mailing List, Platform Driver On Sat, 2019-09-07 at 21:18 +0300, Andy Shevchenko wrote: > On Fri, Sep 6, 2019 at 10:47 PM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > > > On Fri, 2019-09-06 at 07:50 -0700, Srinivas Pandruvada wrote: > > > On Fri, 2019-09-06 at 16:46 +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 06, 2019 at 05:39:54AM -0400, Prarit Bhargava > > > > wrote: > > > > > On 9/5/19 7:37 PM, Srinivas Pandruvada wrote: > > > > > > Read the bucket and core count relationship via MSR and > > > > > > display > > > > > > when displaying turbo ratio limits. > > > > > > + ret = isst_send_msr_command(cpu, 0x1ae, 0, > > > > > > buckets_info); > > > > > > > > > > ^^^ you can get rid of the magic number 0x1ae by doing (sorry > > > > > for > > > > > the cut-and-paste) > > > > > > > > > > diff --git a/tools/power/x86/intel-speed-select/Makefile > > > > > b/tools/power/x86/intel > > > > > index 12c6939dca2a..087d802ad844 100644 > > > > > --- a/tools/power/x86/intel-speed-select/Makefile > > > > > +++ b/tools/power/x86/intel-speed-select/Makefile > > > > > @@ -15,6 +15,8 @@ endif > > > > > MAKEFLAGS += -r > > > > > > > > > > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE > > > > > -I$(OUTPUT)include > > > > > +override CFLAGS += -I../../../include > > > > > +override CFLAGS += > > > > > -DMSRHEADER='"../../../../arch/x86/include/asm/msr-index.h"' > > > > > > No, we can't use msr_index. > > > > This comment was meant for use of /dev/cpu/X/msr not msr_index. > > I didn't want to bring in dependency on msr-index.h for couple of 2 > > MSRs and the names in msr-index.h doesn't really reflect the actual > > processing, they are doing. For example MSR_TURBO_RATIO_LIMIT1 for > > 0x1ae. The definition of 0x1AE is different on cpu model 0x55 and > > beyond. > > > > > > > It seems not applicable on top of tools patch series I had applied > before. I have rebased on the top of your review branch and resent. Thanks, Srinivas > > > > > > > > > I guess it can be done in more neat way. > > > > > > > > > As I've been looking at this code I have been wondering why > > > > > didn't > > > > > you just use > > > > > the standard /dev/cpu/X/msr interface that other x86 power > > > > > utilities (turbostat, > > > > > x86_energy_perf_policy) use? Implementing msr_read() is > > > > > trivial > > > > > (warning > > > > > untested and uncompiled code) > > > > > > No. We can't. The MSR interface is disabled on several > > > distribution > > > and > > > platforms with secured boot. So some special MSRs are only > > > allowed > > > via > > > this IOCTL interface. > > > > > > Thanks, > > > Srinivas > > > > > > > > > > > > > > Actually good point! > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket 2019-09-08 14:43 ` Srinivas Pandruvada @ 2019-09-09 10:24 ` Andy Shevchenko 0 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2019-09-09 10:24 UTC (permalink / raw) To: Srinivas Pandruvada Cc: Andy Shevchenko, Prarit Bhargava, David Arcari, Linux Kernel Mailing List, Platform Driver On Sun, Sep 8, 2019 at 5:43 PM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Sat, 2019-09-07 at 21:18 +0300, Andy Shevchenko wrote: > > On Fri, Sep 6, 2019 at 10:47 PM Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > It seems not applicable on top of tools patch series I had applied > > before. > I have rebased on the top of your review branch and resent. Pushed to my review and testing queue, thanks! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-09 10:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-05 23:37 [PATCH 1/2] platform/x86: ISST: Allow additional TRL MSRs Srinivas Pandruvada 2019-09-05 23:37 ` [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket Srinivas Pandruvada 2019-09-06 9:39 ` Prarit Bhargava 2019-09-06 13:46 ` Andy Shevchenko 2019-09-06 14:50 ` Srinivas Pandruvada 2019-09-06 15:07 ` Srinivas Pandruvada 2019-09-07 18:18 ` Andy Shevchenko 2019-09-08 9:46 ` Prarit Bhargava 2019-09-08 14:13 ` Srinivas Pandruvada 2019-09-08 14:43 ` Srinivas Pandruvada 2019-09-09 10:24 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).