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? 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
> 
>>> +                }
>>> +
>>> +                if ( !ret || ret == -EBUSY )
>>> +                    break;
>>> +            }
>>> +
>>>              break;
>>>          }
>>> +
>>> +        /*
>>> +         * After successful cpufreq driver registeration,
>> XEN_PROCESSOR_PM_CPPC
>>> +         * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
>>> +         */
>>> +        if ( !ret )
>>> +        {
>>> +            ASSERT(i < cpufreq_xen_cnt);
>>> +            switch ( cpufreq_xen_opts[i] )
>>
>> Hmm, this is using the the initializer of i that I commented on. I think 
>> there's
>> another default: case missing, where you simply "return 0" (to retain prior 
>> behavior).
>> But again see also yet further down.
>>
>>
>>> +            /*
>>> +             * No cpufreq driver gets registered, clear both
>>> +             * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
>>> +             */
>>> +             xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
>>> +                                       XEN_PROCESSOR_PM_PX);
>>
>> Yet more hmm - this path you want to get through for the case mentioned 
>> above.
>> But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )", 
>> which really
>> is "switch ( cpufreq_xen_opts[0] )" in that case, and that's pretty clearly 
>> wrong to
>> evaluate in then.
> 
> Correct me if I understand you wrongly:
> The above "case missing" , are we talking about is entering "case 
> CPUFREQ_none" ?
> IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will 
> have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. 
> That is, we will have px states as default driver. Even if we have failed 
> px-driver initialization, with cpufreq_xen_cnt limited to 1, we will not 
> enter CPUFREQ_none.
> CPUFREQ_none only could be set when users explicitly set 
> "cpufreq=disabled/none/0", but in which case, cpufreq_controller will be set 
> with FREQCTL_none. And the whole cpufreq_driver_init() is under " 
> cpufreq_controller == FREQCTL_xen " condition
> Or "case missing" is referring entering default case? In which case, we will 
> have -ENOENT errno. As we have ret=-ENOENT in the very beginning

Sorry, this is hard to follow. Plus I think I made the main requirement quite
clear: You want to "retain prior behavior" for all cases you don't deliberately
change to accommodate the new driver. Plus you want to watch out for pre-
existing incorrect behavior: Rather than proliferating any, such would want
adjusting.

Jan