diff mbox series

[2/4] x86/idle: Get PC{8..10} counters for Tiger and Alder Lake

Message ID 20230718132334.2087-1-simon@invisiblethingslab.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Simon Gaiser July 18, 2023, 1:23 p.m. UTC 
---
 xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Simon Gaiser July 18, 2023, 1:29 p.m. UTC | #1 
Please excuse the wrong subject prefix. It should have been [XEN PATCH].
This is a single patch.

Simon
Jan Beulich July 18, 2023, 1:35 p.m. UTC | #2 
On 18.07.2023 15:23, Simon Gaiser wrote:
> ---
>  xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

This lacks both S-o-b and a proper description. The latter in
particular because you ...

> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -155,6 +155,12 @@ static void cf_check do_get_hw_residencies(void *arg)
>  
>      switch ( c->x86_model )
>      {
> +    /* Tiger Lake */
> +    case 0x8C:
> +    case 0x8D:
> +    /* Alder Lake */
> +    case 0x97:
> +    case 0x9A:
>      /* 4th generation Intel Core (Haswell) */
>      case 0x45:
>          GET_PC8_RES(hw_res->pc8);
> @@ -185,9 +191,6 @@ static void cf_check do_get_hw_residencies(void *arg)
>      case 0x6C:
>      case 0x7D:
>      case 0x7E:
> -    /* Tiger Lake */
> -    case 0x8C:
> -    case 0x8D:
>      /* Kaby Lake */
>      case 0x8E:
>      case 0x9E:

... don't just add new case labels, but you actually move two. It
wants explaining whether this was outright wrong, or what else
causes the movement.

Jan
Simon Gaiser July 18, 2023, 1:46 p.m. UTC | #3 
Jan Beulich:
> On 18.07.2023 15:23, Simon Gaiser wrote:
>> ---
>>  xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> This lacks both S-o-b and a proper description. The latter in
> particular because you ...

Yeah, I also noticed in the meantime, sorry. Will be fixed in v2.

>> --- a/xen/arch/x86/acpi/cpu_idle.c
>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>> @@ -155,6 +155,12 @@ static void cf_check do_get_hw_residencies(void *arg)
>>  
>>      switch ( c->x86_model )
>>      {
>> +    /* Tiger Lake */
>> +    case 0x8C:
>> +    case 0x8D:
>> +    /* Alder Lake */
>> +    case 0x97:
>> +    case 0x9A:
>>      /* 4th generation Intel Core (Haswell) */
>>      case 0x45:
>>          GET_PC8_RES(hw_res->pc8);
>> @@ -185,9 +191,6 @@ static void cf_check do_get_hw_residencies(void *arg)
>>      case 0x6C:
>>      case 0x7D:
>>      case 0x7E:
>> -    /* Tiger Lake */
>> -    case 0x8C:
>> -    case 0x8D:
>>      /* Kaby Lake */
>>      case 0x8E:
>>      case 0x9E:
> 
> ... don't just add new case labels, but you actually move two. It
> wants explaining whether this was outright wrong, or what else
> causes the movement.

Yes, as the commit message says it get those PC{8..10} counters for
Tiger and Alder Lake. The former already had a label, therefore the
move. I assume that when Tiger Lake was added it was an oversight to not
also read those package C-state counters. I can add that to the commit
messages.
Jan Beulich July 18, 2023, 2:04 p.m. UTC | #4 
On 18.07.2023 15:46, Simon Gaiser wrote:
> Jan Beulich:
>> On 18.07.2023 15:23, Simon Gaiser wrote:
>>> ---
>>>  xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> This lacks both S-o-b and a proper description. The latter in
>> particular because you ...
> 
> Yeah, I also noticed in the meantime, sorry. Will be fixed in v2.
> 
>>> --- a/xen/arch/x86/acpi/cpu_idle.c
>>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>>> @@ -155,6 +155,12 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>  
>>>      switch ( c->x86_model )
>>>      {
>>> +    /* Tiger Lake */
>>> +    case 0x8C:
>>> +    case 0x8D:
>>> +    /* Alder Lake */
>>> +    case 0x97:
>>> +    case 0x9A:
>>>      /* 4th generation Intel Core (Haswell) */
>>>      case 0x45:
>>>          GET_PC8_RES(hw_res->pc8);
>>> @@ -185,9 +191,6 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>      case 0x6C:
>>>      case 0x7D:
>>>      case 0x7E:
>>> -    /* Tiger Lake */
>>> -    case 0x8C:
>>> -    case 0x8D:
>>>      /* Kaby Lake */
>>>      case 0x8E:
>>>      case 0x9E:
>>
>> ... don't just add new case labels, but you actually move two. It
>> wants explaining whether this was outright wrong, or what else
>> causes the movement.
> 
> Yes, as the commit message says it get those PC{8..10} counters for
> Tiger and Alder Lake.

But that's the problem - there was no commit message.

> The former already had a label, therefore the
> move. I assume that when Tiger Lake was added it was an oversight to not
> also read those package C-state counters.

Or the SDM wasn't clear, and we needed to err on the safe side.

Jan
Simon Gaiser July 18, 2023, 8:17 p.m. UTC | #5 
Jan Beulich:
> On 18.07.2023 15:46, Simon Gaiser wrote:
>> Jan Beulich:
>>> On 18.07.2023 15:23, Simon Gaiser wrote:
>>>> ---
>>>>  xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> This lacks both S-o-b and a proper description. The latter in
>>> particular because you ...
>>
>> Yeah, I also noticed in the meantime, sorry. Will be fixed in v2.
>>
>>>> --- a/xen/arch/x86/acpi/cpu_idle.c
>>>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>>>> @@ -155,6 +155,12 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>>  
>>>>      switch ( c->x86_model )
>>>>      {
>>>> +    /* Tiger Lake */
>>>> +    case 0x8C:
>>>> +    case 0x8D:
>>>> +    /* Alder Lake */
>>>> +    case 0x97:
>>>> +    case 0x9A:
>>>>      /* 4th generation Intel Core (Haswell) */
>>>>      case 0x45:
>>>>          GET_PC8_RES(hw_res->pc8);
>>>> @@ -185,9 +191,6 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>>      case 0x6C:
>>>>      case 0x7D:
>>>>      case 0x7E:
>>>> -    /* Tiger Lake */
>>>> -    case 0x8C:
>>>> -    case 0x8D:
>>>>      /* Kaby Lake */
>>>>      case 0x8E:
>>>>      case 0x9E:
>>>
>>> ... don't just add new case labels, but you actually move two. It
>>> wants explaining whether this was outright wrong, or what else
>>> causes the movement.
>>
>> Yes, as the commit message says it get those PC{8..10} counters for
>> Tiger and Alder Lake.
> 
> But that's the problem - there was no commit message.

I'm used to that in git "commit message" refers to the whole thing,
including the "title" (everything till the first blank line. Usually
only a single line. Put into the Subject header by format-patch). And
there it says exactly this, which I considered enough when drafting it.
Will send a v2 with a more verbose description.

>> The former already had a label, therefore the
>> move. I assume that when Tiger Lake was added it was an oversight to not
>> also read those package C-state counters.
> 
> Or the SDM wasn't clear, and we needed to err on the safe side.

The SDM [1] seems to be indeed a mess regarding
MSR_PKG_C{8..10}_RESIDENCY. If I didn't missed something in that huge
document it lists PC8 and PC9 only for Intel Core 4th gen with CPUID
06_45H (table 2-31). For PC10 it additionally list Atoms starting with
Goldmont (table 2-12 and references to it).

But it already contradicts itself by listing on page 5002/5003 06_4Fh
(some Xeons) as another model that supports those MSRs. It refers to
table 2-38 there, but that table doesn't contain those MSRs.

Linux' pmc_core [2] and turbostat [3] both use those MSRs on Tiger and
Alder Lake. And on my Tiger Lake test system I get useful data from
there.

Is the code in Linux a good enough reference?

Simon

[1]:
I looked at what's currently linked on Intel's website. "325462-080US"
from "June 2023"
https://cdrdv2.intel.com/v1/dl/getContent/671200

[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/core.c?h=v6.4#n44
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/tgl.c?h=v6.4#n188
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/adl.c?h=v6.4#n291

[3]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5763
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5074
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5409
Jan Beulich July 19, 2023, 7:53 a.m. UTC | #6 
On 18.07.2023 22:17, Simon Gaiser wrote:
> Jan Beulich:
>> On 18.07.2023 15:46, Simon Gaiser wrote:
>>> Jan Beulich:
>>>> On 18.07.2023 15:23, Simon Gaiser wrote:
>>>>> ---
>>>>>  xen/arch/x86/acpi/cpu_idle.c | 9 ++++++---
>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> This lacks both S-o-b and a proper description. The latter in
>>>> particular because you ...
>>>
>>> Yeah, I also noticed in the meantime, sorry. Will be fixed in v2.
>>>
>>>>> --- a/xen/arch/x86/acpi/cpu_idle.c
>>>>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>>>>> @@ -155,6 +155,12 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>>>  
>>>>>      switch ( c->x86_model )
>>>>>      {
>>>>> +    /* Tiger Lake */
>>>>> +    case 0x8C:
>>>>> +    case 0x8D:
>>>>> +    /* Alder Lake */
>>>>> +    case 0x97:
>>>>> +    case 0x9A:
>>>>>      /* 4th generation Intel Core (Haswell) */
>>>>>      case 0x45:
>>>>>          GET_PC8_RES(hw_res->pc8);

One note here: Please follow what we do later in this switch and
maintain roughly time-wise ordering between the case blocks. IOW
you want to insert below the Haswell entry, not above.

>>>>> @@ -185,9 +191,6 @@ static void cf_check do_get_hw_residencies(void *arg)
>>>>>      case 0x6C:
>>>>>      case 0x7D:
>>>>>      case 0x7E:
>>>>> -    /* Tiger Lake */
>>>>> -    case 0x8C:
>>>>> -    case 0x8D:
>>>>>      /* Kaby Lake */
>>>>>      case 0x8E:
>>>>>      case 0x9E:
>>>>
>>>> ... don't just add new case labels, but you actually move two. It
>>>> wants explaining whether this was outright wrong, or what else
>>>> causes the movement.
>>>
>>> Yes, as the commit message says it get those PC{8..10} counters for
>>> Tiger and Alder Lake.
>>
>> But that's the problem - there was no commit message.
> 
> I'm used to that in git "commit message" refers to the whole thing,
> including the "title" (everything till the first blank line. Usually
> only a single line. Put into the Subject header by format-patch). And
> there it says exactly this, which I considered enough when drafting it.
> Will send a v2 with a more verbose description.
> 
>>> The former already had a label, therefore the
>>> move. I assume that when Tiger Lake was added it was an oversight to not
>>> also read those package C-state counters.
>>
>> Or the SDM wasn't clear, and we needed to err on the safe side.
> 
> The SDM [1] seems to be indeed a mess regarding
> MSR_PKG_C{8..10}_RESIDENCY. If I didn't missed something in that huge
> document it lists PC8 and PC9 only for Intel Core 4th gen with CPUID
> 06_45H (table 2-31). For PC10 it additionally list Atoms starting with
> Goldmont (table 2-12 and references to it).
> 
> But it already contradicts itself by listing on page 5002/5003 06_4Fh
> (some Xeons) as another model that supports those MSRs. It refers to
> table 2-38 there, but that table doesn't contain those MSRs.
> 
> Linux' pmc_core [2] and turbostat [3] both use those MSRs on Tiger and
> Alder Lake. And on my Tiger Lake test system I get useful data from
> there.
> 
> Is the code in Linux a good enough reference?

Well, the SDM has to be the primary reference. Linux can be used, sure,
if the respective commits look trustworthy. Note though that if we were
to follow Linux, yet more changes than what you propose would want
doing, afaics.

Note also that personally I wouldn't accept references to a user space
tool (i.e. turbostat) as justification.

Jan

> [1]:
> I looked at what's currently linked on Intel's website. "325462-080US"
> from "June 2023"
> https://cdrdv2.intel.com/v1/dl/getContent/671200
> 
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/core.c?h=v6.4#n44
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/tgl.c?h=v6.4#n188
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/pmc/adl.c?h=v6.4#n291
> 
> [3]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5763
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5074
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/power/x86/turbostat/turbostat.c?h=v6.4#n5409
diff mbox series

Patch

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 557bc6ef86..a6d3175156 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -155,6 +155,12 @@  static void cf_check do_get_hw_residencies(void *arg)
 
     switch ( c->x86_model )
     {
+    /* Tiger Lake */
+    case 0x8C:
+    case 0x8D:
+    /* Alder Lake */
+    case 0x97:
+    case 0x9A:
     /* 4th generation Intel Core (Haswell) */
     case 0x45:
         GET_PC8_RES(hw_res->pc8);
@@ -185,9 +191,6 @@  static void cf_check do_get_hw_residencies(void *arg)
     case 0x6C:
     case 0x7D:
     case 0x7E:
-    /* Tiger Lake */
-    case 0x8C:
-    case 0x8D:
     /* Kaby Lake */
     case 0x8E:
     case 0x9E: