Re: [PATCH v5 06/18] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2025 09:15, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, June 12, 2025 6:42 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 v5 06/18] xen/x86: introduce "cpufreq=amd-cppc" xen
>> cmdline
>>
>> On 27.05.2025 10:48, Penny Zheng wrote:
>>> Users need to set "cpufreq=amd-cppc" in xen cmdline to enable amd-cppc
>>> driver, which selects ACPI Collaborative Performance and Power Control
>>> (CPPC) on supported AMD hardware to provide a finer grained frequency
>>> control mechanism.
>>> `verbose` option can also be included to support verbose print.
>>>
>>> When users setting "cpufreq=amd-cppc", a new amd-cppc driver shall be
>>> registered and used. All hooks for amd-cppc driver are transiently
>>> missing and will be implemented in the ongoing commits.
>>>
>>> New xen-pm internal flag XEN_PROCESSOR_PM_CPPC is introduced, to be
>>> differentiated with legacy XEN_PROCESSOR_PM_PX. We define
>>> XEN_PROCESSOR_PM_CPPC 0x100, as it is the next value to use after
>>> 8-bits wide public xen-pm options. All xen-pm flag checking involving
>>> XEN_PROCESSOR_PM_PX shall also be updated to consider
>> XEN_PROCESSOR_PM_CPPC now.
>>>
>>> Xen is not expected to support both or mixed mode (CPPC & legacy PSS,
>>> _PCT,
>>> _PPC) operations, so only one cpufreq driver gets registerd, either
>>> amd-cppc or legacy P-states driver, which is reflected and asserted by
>>> the incompatible flags XEN_PROCESSOR_PM_PX and
>> XEN_PROCESSOR_PM_CPPC.
>>>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
>>> ---
>>> v1 -> v2:
>>> - Obey to alphabetic sorting and also strict it with CONFIG_AMD
>>> - Remove unnecessary empty comment line
>>> - Use __initconst_cf_clobber for pre-filled structure cpufreq_driver
>>> - Make new switch-case code apply to Hygon CPUs too
>>> - Change ENOSYS with EOPNOTSUPP
>>> - Blanks around binary operator
>>> - Change all amd_/-pstate defined values to amd_/-cppc
>>> ---
>>> v2 -> v3
>>> - refactor too long lines
>>> - Make sure XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC
>> incompatible
>>> flags after cpufreq register registrantion
>>> ---
>>> v3 -> v4:
>>> - introduce XEN_PROCESSOR_PM_CPPC in xen internal header
>>> - complement "Hygon" in log message
>>> - remove unnecessary if()
>>> - grow cpufreq_xen_opts[] array
>>> ---
>>> v4 -> v5:
>>> - remove XEN_PROCESSOR_PM_xxx flag sanitization from individual driver
>>> - prefer ! over "== 0" in purely boolean contexts
>>> - Blank line between non-fall-through case blocks
>>> - add build-time checking between internal and public
>>> XEN_PROCESSOR_PM_* values
>>> - define XEN_PROCESSOR_PM_CPPC with 0x100, as it is the next value to
>>> use after public interface, and public mask SIF_PM_MASK is 8 bits wide.
>>> - as Dom0 will send the CPPC/Px data whenever it could, the return
>>> value shall be 0 instead of -ENOSYS/EOPNOTSUP when platform doesn't require
>> these data.
>>> ---
>>>  docs/misc/xen-command-line.pandoc         |  7 ++-
>>>  xen/arch/x86/acpi/cpufreq/Makefile        |  1 +
>>>  xen/arch/x86/acpi/cpufreq/amd-cppc.c      | 68 +++++++++++++++++++++++
>>>  xen/arch/x86/acpi/cpufreq/cpufreq.c       | 63 ++++++++++++++++++++-
>>>  xen/arch/x86/platform_hypercall.c         | 13 ++++-
>>>  xen/drivers/acpi/pmstat.c                 |  3 +-
>>>  xen/drivers/cpufreq/cpufreq.c             | 18 +++++-
>>>  xen/include/acpi/cpufreq/cpufreq.h        |  6 +-
>>>  xen/include/acpi/cpufreq/processor_perf.h |  3 +
>>>  xen/include/public/sysctl.h               |  1 +
>>>  10 files changed, 175 insertions(+), 8 deletions(-)  create mode
>>> 100644 xen/arch/x86/acpi/cpufreq/amd-cppc.c
>>
>>> @@ -157,7 +162,63 @@ 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;
>>> +            unsigned int i;
>>> +
>>> +            if ( !IS_ENABLED(CONFIG_AMD) )
>>> +            {
>>> +                ret = -ENODEV;
>>> +                break;
>>> +            }
>>> +            ret = -ENOENT;
>>> +
>>> +            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;
>>> +                }
>>> +
>>> +                if ( ret != -ENODEV )
>>> +                    break;
>>
>> This, I think, needs some commenting. It's not quite clear why we shouldn't 
>> try the
>> next option if registration failed with other than -ENODEV.
> 
> I followed the original logic.

Which may easily itself be partly bogus.

> Now, I'm trying to understand the reason. I read the related code, there are 
> two code path erroring out other than -ENODEV
> In cpufreq_register_driver(), either the driver itself is broken, like 
> missing mandatory hooks, etc, yet in which case, IMO we shall try the 
> fallback option,
> or repeated registration, TBH, which seems unlikely to me. 
> cpufreq_driver_init() is a presmp call, so repeated registration doesn't come 
> from racing.
> Then if we successfully registered a driver, we will immediately exit the 
> loop. How come we will register twice?
> Or am I missing something for this error path:
> ```
>         if ( cpufreq_driver.init )
>                 return -EBUSY;
> ```

Imo this error path is there "just in case".

Jan