Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline and amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2025 10:09, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, July 17, 2025 12:00 AM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
>> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger 
>> Pau
>> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
>> xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen 
>> cmdline
>> and amd-cppc driver
>>
>> On 11.07.2025 05:50, Penny Zheng wrote:
>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> @@ -128,12 +128,14 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>>      if ( cpufreq_controller == FREQCTL_xen )
>>>      {
>>> +        unsigned int i = 0;
>>
>> Pointless initializer; both for() loops set i to 0. But also see further 
>> down.
>>
>>> @@ -157,9 +164,70 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>>          case X86_VENDOR_AMD:
>>>          case X86_VENDOR_HYGON:
>>> -            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
>> ENODEV;
>>> +            if ( !IS_ENABLED(CONFIG_AMD) )
>>> +            {
>>> +                ret = -ENODEV;
>>> +                break;
>>> +            }
>>> +            ret = -ENOENT;
>>
>> The code structure is sufficiently different from the Intel counterpart for 
>> this to
>> perhaps better move ...
>>
>>> +            for ( i = 0; i < cpufreq_xen_cnt; i++ )
>>> +            {
>>> +                switch ( cpufreq_xen_opts[i] )
>>> +                {
>>> +                case CPUFREQ_xen:
>>> +                    ret = powernow_register_driver();
>>> +                    break;
>>> +
>>> +                case CPUFREQ_amd_cppc:
>>> +                    ret = amd_cppc_register_driver();
>>> +                    break;
>>> +
>>> +                case CPUFREQ_none:
>>> +                    ret = 0;
>>> +                    break;
>>> +
>>> +                default:
>>> +                    printk(XENLOG_WARNING
>>> +                           "Unsupported cpufreq driver for vendor AMD or 
>>> Hygon\n");
>>> +                    break;
>>
>> ... here.
>>
> 
> Are we suggesting moving
> "
>         if ( !IS_ENABLED(CONFIG_AMD) )
>         {
>                 ret = -ENODEV;
>                 break;
>         }
> " here?

That's what I said, didn't I?

> In which case, When CONFIG_AMD=n and users doesn't provide "cpufreq=xxx", we 
> will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = 
> CPUFREQ_xen. powernow_register_driver() hence gets invoked. The thing is that 
> we don't have stub for it and it is compiled under CONFIG_AMD
> I suggest to change to use #ifdef CONFIG_AMD code wrapping

Perhaps necessary, yes. As you know, we generally prefer IS_ENABLED() where 
possible,
but when not possible, #ifdef is certainly okay to use.

Jan