Re: [PATCH v3 09/15] xen/x86: introduce a new amd cppc driver for cpufreq scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2025 09:40, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, March 25, 2025 5:58 PM
>>
>>> +#define amd_get_freq(name)                                                 
>>>  \
>>
>> The macro parameter is used just ...
>>
>>> +    static int amd_get_##name##_freq(const struct amd_cppc_drv_data
>>> + *data,  \
>>
>> ... here, ...
>>
>>> +                                     unsigned int *freq)                   
>>>  \
>>> +    {                                                                      
>>>  \
>>> +        const struct xen_processor_cppc *cppc_data = data->cppc_data;      
>>>  \
>>> +        uint64_t mul, div, res;                                            
>>>  \
>>> +                                                                           
>>>  \
>>> +        if ( cppc_data->name##_mhz )                                       
>>>  \
>>> +        {                                                                  
>>>  \
>>> +            /* Switch to khz */                                            
>>>  \
>>> +            *freq = cppc_data->name##_mhz * 1000;                          
>>>  \
>>
>> ... twice here forthe MHz value, and ...
>>
>>> +            return 0;                                                      
>>>  \
>>> +        }                                                                  
>>>  \
>>> +                                                                           
>>>  \
>>> +        /* Read Processor Max Speed(mhz) as anchor point */                
>>>  \
>>> +        mul = this_cpu(amd_max_freq_mhz);                                  
>>>  \
>>> +        if ( !mul )                                                        
>>>  \
>>> +            return -EINVAL;                                                
>>>  \
>>> +        div = data->caps.highest_perf;                                     
>>>  \
>>> +        res = (mul * data->caps.name##_perf * 1000) / div;                 
>>>  \
>>
>> ... here for the respective perf indicator. Why does it take ...
>>
>>> +        if ( res > UINT_MAX )                                              
>>>  \
>>> +        {                                                                  
>>>  \
>>> +            printk(XENLOG_ERR                                              
>>>  \
>>> +                   "Frequeny exceeds maximum value UINT_MAX: %lu\n", res); 
>>>  \
>>> +            return -EINVAL;                                                
>>>  \
>>> +        }                                                                  
>>>  \
>>> +        *freq = (unsigned int)res;                                         
>>>  \
>>> +                                                                           
>>>  \
>>> +        return 0;                                                          
>>>  \
>>> +    }                                                                      
>>>  \
>>> +
>>> +amd_get_freq(lowest);
>>> +amd_get_freq(nominal);
>>
>> ... two almost identical functions, when one (with two extra input 
>> parameters) would
>> suffice?
>>
> 
> I had a draft fix here, If it doesn't what you hope for, plz let me know
> ```
> static int amd_get_lowest_and_nominal_freq(const struct amd_cppc_drv_data 
> *data,
>                                            unsigned int *lowest_freq,
>                                            unsigned int *nominal_freq)

Why two outputs now when there was just one in the macro-ized form? I was
rather expecting new inputs to appear, to account for the prior uses of
the macro parameter. (As a result the function is now also quite a bit
more complex than it was before. In particular there was no ...

> {
>     const struct xen_processor_cppc *cppc_data = data->cppc_data;
>     uint64_t mul, div, res;
>     uint8_t perf;
> 
>     if ( !lowest_freq && !nominal_freq )
>         return -EINVAL;
> 
>     if ( lowest_freq && cppc_data->cpc.lowest_mhz )
>         /* Switch to khz */
>         *lowest_freq = cppc_data->cpc.lowest_mhz * 1000;
> 
>     if ( nominal_freq && cppc_data->cpc.nominal_mhz )
>         /* Switch to khz */
>         *nominal_freq = cppc_data->cpc.nominal_mhz * 1000;
> 
>     /* Still have unresolved frequency */
>     if ( (lowest_freq && !(*lowest_freq)) ||
>          (nominal_freq && !(*nominal_freq)) )
>     {
>         do {
>             /* Calculate lowest frequency firstly if need */
>             if ( lowest_freq && !(*lowest_freq) )
>                 perf = data->caps.lowest_perf;
>             else
>                 perf = data->caps.nominal_perf;
> 
>             /* Read Processor Max Speed(MHz) as anchor point */
>             mul = this_cpu(amd_max_pxfreq_mhz);
>             if ( mul == INVAL_FREQ_MHZ || !mul )
>             {
>                 printk(XENLOG_ERR
>                        "Failed to read valid processor max frequency as 
> anchor point: %lu\n",
>                        mul);
>                 return -EINVAL;
>             }
>             div = data->caps.highest_perf;
>             res = (mul * perf * 1000) / div;
> 
>             if ( res > UINT_MAX || !res )
>             {
>                 printk(XENLOG_ERR
>                        "Frequeny exceeds maximum value UINT_MAX or being zero 
> value: %lu\n",
>                        res);
>                 return -EINVAL;
>             }
> 
>             if ( lowest_freq && !(*lowest_freq) )
>                 *lowest_freq = (unsigned int)res;
>             else
>                 *nominal_freq = (unsigned int)res;
>         } while ( nominal_freq && !(*nominal_freq) );

... loop there.)

Jan

>     }
> 
>     return 0;
> }
> ```