[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
[Public] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@xxxxxxxx> > Sent: Monday, March 24, 2025 11:01 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 v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx" > > On 06.03.2025 09:39, Penny Zheng wrote: > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported > by all Dom0 kernels. > > processor to autonomously force physical package components into idle > > state. > > The default is enabled, but the option only applies when `hwp` is > > enabled. > > > > -There is also support for `;`-separated fallback options: > > +User could use `;`-separated options to support universal options > > +which they would like to try on any agnostic platform, *but* under > > +priority order, like > > `cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to > > `xen` if unavailable. Note: The `verbose` suboption is handled > > globally. Setting it for either the primary or fallback option > > applies to both irrespective of where > > What does "support" here mean? I fear I can't even suggest what else to use, > as I > don't follow what additional information you mean to add here. Is a change > here > really needed? > There are two changes I'd like to address: 1) ";" is not designed for fallback options anymore, like we discussed before, we would like to support something like "cpufreq=hwp;amd-cppc;xen" for users to define all universal options they would like to try. 2) Must under *priority* order. As in cpufreq_driver_init(), we are using loop to decide which driver to try firstly. If user defines "cpufreq=xen;amd-cppc", which leads legacy P-state set before amd-cppc in cpufreq_xen_opts[], then in the loop, we will try to register legacy P-state firstly, once it gets registered successfully, we will not try to register amd-cppc at all. > > --- a/xen/drivers/cpufreq/cpufreq.c > > +++ b/xen/drivers/cpufreq/cpufreq.c > > + if ( arg[0] && arg[1] ) > > + ret = cpufreq_cmdline_parse(arg + 1, end); > > + > > + return ret; > > +} > > + > > +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const > > +char *end) { > > + int ret = 0; > > + > > + xen_processor_pmbits |= XEN_PROCESSOR_PM_PX; > > + cpufreq_controller = FREQCTL_xen; > > + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp; > > + if ( arg[0] && arg[1] ) > > + ret = hwp_cmdline_parse(arg + 1, end); > > + > > + return ret; > > +} > > For both of the helpers may I suggest s/parse/process/ or some such ("handle" > might be another possible term to use), as themselves they don't do any > parsing? > Maybe I mis-understood the previous comment you said ``` > else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 && > ``` For the rest of this, I guess I'd prefer to see this in context. Also with regard to the helper function's name. ``` I thought you suggested to introduce helper function to wrap the conditional codes... Or may you were suggesting something like: ``` #ifdef CONFIG_INTEL else if ( choice < 0 && !cmdline_strcmp(str, "hwp") ) { xen_processor_pmbits |= XEN_PROCES ... } #endif ``` > In the end I'm also not entirely convinced that we need these two almost > identical > helpers (with a 3rd likely appearing in a later patch). > > Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |