turbostat: Fix Haswell Core systems
diff mbox series

Message ID 20190605135830.23941-1-prarit@redhat.com
State Superseded
Delegated to: Len Brown
Headers show
Series
  • turbostat: Fix Haswell Core systems
Related show

Commit Message

Prarit Bhargava June 5, 2019, 1:58 p.m. UTC
On Haswell (model 0x3C) turbostat fails with

turbostat: cpu0: msr offset 0x630 read failed: Input/output error

because Haswell Core does not have C8-C10.

Output C8-C10 only on Haswell ULT.

Fixes: f5a4c76ad7de ("tools/power turbostat: consolidate duplicate model
numbers")
Cc: Len Brown <len.brown@intel.com>
---
 tools/power/x86/turbostat/turbostat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kosuke Tatsukawa June 10, 2019, 3:14 a.m. UTC | #1
Hi,

> On Haswell (model 0x3C) turbostat fails with
>
> turbostat: cpu0: msr offset 0x630 read failed: Input/output error
>
> because Haswell Core does not have C8-C10.
>
> Output C8-C10 only on Haswell ULT.

has_hsw_msrs() uses the model number returned by intel_model_duplicates(),
but commit f5a4c76ad7de added code to return INTEL_FAM6_HASWELL_CORE for
INTEL_FAM6_HASWELL_ULT there.  So I think Haswell UTL also doesn't
output C8-C10 with your patch.

Something similar to the below patch is needed to differentiate between
INTEL_FAM6_HASWELL_CORE and INTEL_FAM6_HASWELL_ULT.  I don't have a
Haswell-ULT machine, so I've only tested it on a Haswell-DT CPU.

Best regards.

> Fixes: f5a4c76ad7de ("tools/power turbostat: consolidate duplicate model
> numbers")
> Cc: Len Brown <len.brown@intel.com>
> ---
>  tools/power/x86/turbostat/turbostat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index c7727be9719f..ec8797005731 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -4296,7 +4296,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
> 		return 0;
> 
> 	switch (model) {
> -	case INTEL_FAM6_HASWELL_CORE:
> +	case INTEL_FAM6_HASWELL_ULT:
> 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
> 	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
> 	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
> -- 
> 2.21.0


diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 75fc4fb..5830552 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -3209,6 +3209,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
 		break;
 	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
 	case INTEL_FAM6_HASWELL_X:	/* HSX */
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
@@ -3405,6 +3406,7 @@ int has_config_tdp(unsigned int family, unsigned int model)
 	case INTEL_FAM6_IVYBRIDGE:	/* IVB */
 	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
 	case INTEL_FAM6_HASWELL_X:	/* HSX */
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
@@ -3841,6 +3843,7 @@ void rapl_probe_intel(unsigned int family, unsigned int model)
 	case INTEL_FAM6_SANDYBRIDGE:
 	case INTEL_FAM6_IVYBRIDGE:
 	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
@@ -4032,6 +4035,7 @@ void perf_limit_reasons_probe(unsigned int family, unsigned int model)
 
 	switch (model) {
 	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
 		do_gfx_perf_limit_reasons = 1;
 	case INTEL_FAM6_HASWELL_X:	/* HSX */
@@ -4251,6 +4255,7 @@ int has_snb_msrs(unsigned int family, unsigned int model)
 	case INTEL_FAM6_IVYBRIDGE_X:	/* IVB Xeon */
 	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
 	case INTEL_FAM6_HASWELL_X:	/* HSW */
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
@@ -4284,7 +4289,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
 		return 0;
 
 	switch (model) {
-	case INTEL_FAM6_HASWELL_CORE:
+	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
 	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
@@ -4568,9 +4573,6 @@ unsigned int intel_model_duplicates(unsigned int model)
 	case INTEL_FAM6_XEON_PHI_KNM:
 		return INTEL_FAM6_XEON_PHI_KNL;
 
-	case INTEL_FAM6_HASWELL_ULT:
-		return INTEL_FAM6_HASWELL_CORE;
-
 	case INTEL_FAM6_BROADWELL_X:
 	case INTEL_FAM6_BROADWELL_XEON_D:	/* BDX-DE */
 		return INTEL_FAM6_BROADWELL_X;
Prarit Bhargava July 24, 2019, 2:29 p.m. UTC | #2
On 6/9/19 11:14 PM, Kosuke Tatsukawa wrote:
> Hi,
> 
>> On Haswell (model 0x3C) turbostat fails with
>>
>> turbostat: cpu0: msr offset 0x630 read failed: Input/output error
>>
>> because Haswell Core does not have C8-C10.
>>
>> Output C8-C10 only on Haswell ULT.
> 
> has_hsw_msrs() uses the model number returned by intel_model_duplicates(),
> but commit f5a4c76ad7de added code to return INTEL_FAM6_HASWELL_CORE for
> INTEL_FAM6_HASWELL_ULT there.  So I think Haswell UTL also doesn't
> output C8-C10 with your patch.
> 
> Something similar to the below patch is needed to differentiate between
> INTEL_FAM6_HASWELL_CORE and INTEL_FAM6_HASWELL_ULT.  I don't have a
> Haswell-ULT machine, so I've only tested it on a Haswell-DT CPU.
> 

Kosuke, my apologies this was in my junk folder and I just found it.

Yes.  I agree with your patch below.  I will resubmit.  OK to add Signed-off-by
from you?

P.

> Best regards.
> 
>> Fixes: f5a4c76ad7de ("tools/power turbostat: consolidate duplicate model
>> numbers")
>> Cc: Len Brown <len.brown@intel.com>
>> ---
>>  tools/power/x86/turbostat/turbostat.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index c7727be9719f..ec8797005731 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -4296,7 +4296,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
>> 		return 0;
>>
>> 	switch (model) {
>> -	case INTEL_FAM6_HASWELL_CORE:
>> +	case INTEL_FAM6_HASWELL_ULT:
>> 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>> 	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
>> 	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
>> -- 
>> 2.21.0
> 
> 
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 75fc4fb..5830552 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -3209,6 +3209,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
>  		break;
>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
> @@ -3405,6 +3406,7 @@ int has_config_tdp(unsigned int family, unsigned int model)
>  	case INTEL_FAM6_IVYBRIDGE:	/* IVB */
>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
> @@ -3841,6 +3843,7 @@ void rapl_probe_intel(unsigned int family, unsigned int model)
>  	case INTEL_FAM6_SANDYBRIDGE:
>  	case INTEL_FAM6_IVYBRIDGE:
>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
> @@ -4032,6 +4035,7 @@ void perf_limit_reasons_probe(unsigned int family, unsigned int model)
>  
>  	switch (model) {
>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>  		do_gfx_perf_limit_reasons = 1;
>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
> @@ -4251,6 +4255,7 @@ int has_snb_msrs(unsigned int family, unsigned int model)
>  	case INTEL_FAM6_IVYBRIDGE_X:	/* IVB Xeon */
>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>  	case INTEL_FAM6_HASWELL_X:	/* HSW */
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
> @@ -4284,7 +4289,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
>  		return 0;
>  
>  	switch (model) {
> -	case INTEL_FAM6_HASWELL_CORE:
> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>  	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
>  	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
> @@ -4568,9 +4573,6 @@ unsigned int intel_model_duplicates(unsigned int model)
>  	case INTEL_FAM6_XEON_PHI_KNM:
>  		return INTEL_FAM6_XEON_PHI_KNL;
>  
> -	case INTEL_FAM6_HASWELL_ULT:
> -		return INTEL_FAM6_HASWELL_CORE;
> -
>  	case INTEL_FAM6_BROADWELL_X:
>  	case INTEL_FAM6_BROADWELL_XEON_D:	/* BDX-DE */
>  		return INTEL_FAM6_BROADWELL_X;
>
Kosuke Tatsukawa July 24, 2019, 10:42 p.m. UTC | #3
> On 6/9/19 11:14 PM, Kosuke Tatsukawa wrote:
>> Hi,
>> 
>>> On Haswell (model 0x3C) turbostat fails with
>>>
>>> turbostat: cpu0: msr offset 0x630 read failed: Input/output error
>>>
>>> because Haswell Core does not have C8-C10.
>>>
>>> Output C8-C10 only on Haswell ULT.
>> 
>> has_hsw_msrs() uses the model number returned by intel_model_duplicates(),
>> but commit f5a4c76ad7de added code to return INTEL_FAM6_HASWELL_CORE for
>> INTEL_FAM6_HASWELL_ULT there.  So I think Haswell UTL also doesn't
>> output C8-C10 with your patch.
>> 
>> Something similar to the below patch is needed to differentiate between
>> INTEL_FAM6_HASWELL_CORE and INTEL_FAM6_HASWELL_ULT.  I don't have a
>> Haswell-ULT machine, so I've only tested it on a Haswell-DT CPU.
>> 
> 
> Kosuke, my apologies this was in my junk folder and I just found it.
> 
> Yes.  I agree with your patch below.  I will resubmit.  OK to add Signed-off-by
> from you?

Prarit, yes, that's OK with me.

Best regards.


> P.
> 
>> Best regards.
>> 
>>> Fixes: f5a4c76ad7de ("tools/power turbostat: consolidate duplicate model
>>> numbers")
>>> Cc: Len Brown <len.brown@intel.com>
>>> ---
>>>  tools/power/x86/turbostat/turbostat.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>>> index c7727be9719f..ec8797005731 100644
>>> --- a/tools/power/x86/turbostat/turbostat.c
>>> +++ b/tools/power/x86/turbostat/turbostat.c
>>> @@ -4296,7 +4296,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
>>> 		return 0;
>>>
>>> 	switch (model) {
>>> -	case INTEL_FAM6_HASWELL_CORE:
>>> +	case INTEL_FAM6_HASWELL_ULT:
>>> 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>> 	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
>>> 	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
>>> -- 
>>> 2.21.0
>> 
>> 
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index 75fc4fb..5830552 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -3209,6 +3209,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
>>  		break;
>>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
>> @@ -3405,6 +3406,7 @@ int has_config_tdp(unsigned int family, unsigned int model)
>>  	case INTEL_FAM6_IVYBRIDGE:	/* IVB */
>>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
>> @@ -3841,6 +3843,7 @@ void rapl_probe_intel(unsigned int family, unsigned int model)
>>  	case INTEL_FAM6_SANDYBRIDGE:
>>  	case INTEL_FAM6_IVYBRIDGE:
>>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
>> @@ -4032,6 +4035,7 @@ void perf_limit_reasons_probe(unsigned int family, unsigned int model)
>>  
>>  	switch (model) {
>>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>>  		do_gfx_perf_limit_reasons = 1;
>>  	case INTEL_FAM6_HASWELL_X:	/* HSX */
>> @@ -4251,6 +4255,7 @@ int has_snb_msrs(unsigned int family, unsigned int model)
>>  	case INTEL_FAM6_IVYBRIDGE_X:	/* IVB Xeon */
>>  	case INTEL_FAM6_HASWELL_CORE:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_X:	/* HSW */
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_HASWELL_GT3E:	/* HSW */
>>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>  	case INTEL_FAM6_BROADWELL_GT3E:	/* BDW */
>> @@ -4284,7 +4289,7 @@ int has_hsw_msrs(unsigned int family, unsigned int model)
>>  		return 0;
>>  
>>  	switch (model) {
>> -	case INTEL_FAM6_HASWELL_CORE:
>> +	case INTEL_FAM6_HASWELL_ULT:	/* HSW */
>>  	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
>>  	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
>>  	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */
>> @@ -4568,9 +4573,6 @@ unsigned int intel_model_duplicates(unsigned int model)
>>  	case INTEL_FAM6_XEON_PHI_KNM:
>>  		return INTEL_FAM6_XEON_PHI_KNL;
>>  
>> -	case INTEL_FAM6_HASWELL_ULT:
>> -		return INTEL_FAM6_HASWELL_CORE;
>> -
>>  	case INTEL_FAM6_BROADWELL_X:
>>  	case INTEL_FAM6_BROADWELL_XEON_D:	/* BDX-DE */
>>  		return INTEL_FAM6_BROADWELL_X;
>> 
---
Kosuke TATSUKAWA  | 1st Platform Software Division
                  | NEC Solution Innovators
                  | tatsu@ab.jp.nec.com

Patch
diff mbox series

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index c7727be9719f..ec8797005731 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -4296,7 +4296,7 @@  int has_hsw_msrs(unsigned int family, unsigned int model)
 		return 0;
 
 	switch (model) {
-	case INTEL_FAM6_HASWELL_CORE:
+	case INTEL_FAM6_HASWELL_ULT:
 	case INTEL_FAM6_BROADWELL_CORE:	/* BDW */
 	case INTEL_FAM6_SKYLAKE_MOBILE:	/* SKL */
 	case INTEL_FAM6_CANNONLAKE_MOBILE:	/* CNL */