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

[Jan Beulich <jbeulich@xxxxxxxx>]

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