Re: [PATCH v4 07/15] xen/cpufreq: fix core frequency calculation for AMD Family 1Ah CPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2025 08:12, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, April 17, 2025 11:23 PM
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -570,12 +573,35 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
>>>                                                            :
>>> c->cpu_core_id);  }
>>>
>>> +static uint64_t amd_parse_freq(unsigned char c, uint64_t value)
>>
>> Considering how it's used, does "value" need to be any wider than unsigned 
>> int?
>> What about the return type?
> 
> Value is the value of 64bit PstateDef MSR, although we are only using the 
> lower 32bit to calculate frequency
> Maybe its better to leave it as uint64_t ?

I won't insist, but changing to unsigned int will, I think, produce better
code (hence why I suggested the change).

> I'll change the return type to unsigned int, and do the following check anyhow
>         #define INVAL_FREQ_MHZ  (~(unsigned int)0)
>         if ( freq >= UINT_MAX )
>                 return INVAL_FREQ_MHZ;
>         else
>                 return (unsigned int) freq;

But please avoid such unnecessary "else" (which I think I did ask for before).

Jan