On 17.06.2025 09:15, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, June 12, 2025 6:42 PM
>> To: Penny, Zheng <penny.zh...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
>> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger
>> Pau
>> Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>;
>> xen-
>> de...@lists.xenproject.org
>> 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.zh...@amd.com>
>>> ---
>>> 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
