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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature