[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, August 4, 2025 4:48 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 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 <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.
>
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
> Jan
