On 04.08.2025 10:09, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, July 17, 2025 12:00 AM
>> 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 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
