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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2025 08:31, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, August 4, 2025 4:48 PM
>> 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 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.
>>
> 
> I was trying to follow "there's another default: case missing, where you 
> simply "return 0" (to retain prior behavior ) ",
> The missing "default :" is referring the one for "switch ( 
> boot_cpu_data.x86_vendor )"? (I thought it referred " switch ( 
> cpufreq_xen_opts[i] ) " ....)
> It is a pre- existing incorrect behavior which I shall create a new commit to 
> fix it firstly
> I'll add an -ENOENTRY initializer for ret at the very beginning , and 
> complement the missing default: entry with "Unsupported vendor..." error log

Yes, I was referring to pre-existing code which I think wants adjusting in
order to then accommodate your changes there.

Jan