[PATCH 1/2] platform/x86: ISST: Allow additional TRL MSRs

[PATCH 1/2] platform/x86: ISST: Allow additional TRL MSRs

[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

[PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[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

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Prarit Bhargava]

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];
> 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Andy Shevchenko]

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

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Srinivas Pandruvada]

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!
> 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Srinivas Pandruvada]

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!
> > 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Andy Shevchenko]

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

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Prarit Bhargava]

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!
>>>>
>>
> 
> 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Srinivas Pandruvada]

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!
> > > > > 
> > 
> > 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Srinivas Pandruvada]

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!
> > > > 
> 
> 

Re: [PATCH 2/2] tools/power/x86/intel-speed-select: Display core count for bucket

[Andy Shevchenko]

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