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

[Jan Beulich <jbeulich@xxxxxxxx>]

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