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

[Jan Beulich <jbeulich@suse.com>]

On 04.08.2025 10:09, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, July 17, 2025 12:00 AM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
>> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel@lists.xenproject.org
>> 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