| Message ID | 20250206083255.1296363-4-Penny.Zheng@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | amd-cppc CPU Performance Scaling Driver | expand |
On 06.02.2025 09:32, Penny Zheng wrote: > --- /dev/null > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c > @@ -0,0 +1,64 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * amd-cppc.c - AMD Processor CPPC Frequency Driver > + * > + * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved. > + * > + * Author: Penny Zheng <penny.zheng@amd.com> > + * > + * AMD CPPC cpufreq driver introduces a new CPU performance scaling design > + * for AMD processors using the ACPI Collaborative Performance and Power > + * Control (CPPC) feature which provides finer grained frequency control range. > + */ > + > +#include <xen/init.h> > +#include <xen/param.h> > +#include <acpi/cpufreq/cpufreq.h> > + > +static bool __init amd_cppc_handle_option(const char *s, const char *end) > +{ > + int ret; > + > + ret = parse_boolean("verbose", s, end); > + if ( ret >= 0 ) > + { > + cpufreq_verbose = ret; > + return true; > + } > + > + return false; > +} > + > +int __init amd_cppc_cmdline_parse(const char *s, const char *e) > +{ > + do > + { > + const char *end = strpbrk(s, ",;"); > + > + if ( !amd_cppc_handle_option(s, end) ) You have an incoming "e" here. What if the comma / semicolon you find is past where that points? (I understand you've copied that from hwp_cmdline_parse(), and should have raised the question already there when reviewing the respective patch. It also looks as if behavior- wise all would be okay here. It's just that this very much looks like a buffer overrun on the first and second glance.) > + { > + printk(XENLOG_WARNING > + "cpufreq/amd-cppc: option '%.*s' not recognized\n", > + (int)((end ?: e) - s), s); > + > + return -EINVAL; > + } > + > + s = end ? ++end : end; The increment is odd here (again inherited from the HWP function), as "end" is about to go out of scope. > + } while ( s && s < e ); > + > + return 0; > +} > + > +static const struct cpufreq_driver __initconst_cf_clobber amd_cppc_cpufreq_driver = Once again too long a line. > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void) > case CPUFREQ_none: > ret = 0; > break; > + default: > + printk(XENLOG_WARNING "Unsupported cpufreq driver for vendor Intel\n"); Same here. The string along overruning line length is fine. But this cal then still be wrapped as printk(XENLOG_WARNING "Unsupported cpufreq driver for vendor Intel\n"); to respect the line length limit as much as possible. > @@ -131,6 +131,15 @@ static int __init cf_check setup_cpufreq_option(const char *str) > if ( arg[0] && arg[1] ) > ret = hwp_cmdline_parse(arg + 1, end); > } > + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) > + { > + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; > + cpufreq_controller = FREQCTL_xen; > + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; While apparently again a pre-existing problem, the risk of array overrun will become more manifest with this addition: People may plausibly want to pass a universal option to Xen on all their instances: "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq patch, i.e. before you further extend it. Here you will then further want to bump cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold option. I'm also missing IS_ENABLED(CONFIG_AMD) here. The counterpart thereof is present for the earlier HWP alternative. > --- a/xen/include/public/platform.h > +++ b/xen/include/public/platform.h > @@ -357,6 +357,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t); > #define XEN_PROCESSOR_PM_CX 1 > #define XEN_PROCESSOR_PM_PX 2 > #define XEN_PROCESSOR_PM_TX 4 > +#define XEN_PROCESSOR_PM_CPPC 8 Hmm, seeing this addition: Why do all of these live in a public header? They're used to set xen_processor_bits only, which is a Xen-internal variable only. With apparently one exception: PV Dom0 is passed these bits in si->flags. Does Dom0 have use for this new bit? If not it may want assigning a value such that it falls outside of SIF_PM_MASK (and then in a non-public header). Jan
On 2025-02-11 07:08, Jan Beulich wrote: > On 06.02.2025 09:32, Penny Zheng wrote: >> --- /dev/null >> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c >> @@ -0,0 +1,64 @@ >> +static bool __init amd_cppc_handle_option(const char *s, const char *end) >> +{ >> + int ret; >> + >> + ret = parse_boolean("verbose", s, end); >> + if ( ret >= 0 ) >> + { >> + cpufreq_verbose = ret; >> + return true; >> + } >> + >> + return false; >> +} >> + >> +int __init amd_cppc_cmdline_parse(const char *s, const char *e) >> +{ >> + do >> + { >> + const char *end = strpbrk(s, ",;"); >> + >> + if ( !amd_cppc_handle_option(s, end) ) > > You have an incoming "e" here. What if the comma / semicolon you find > is past where that points? (I understand you've copied that from > hwp_cmdline_parse(), and should have raised the question already there > when reviewing the respective patch. It also looks as if behavior- > wise all would be okay here. It's just that this very much looks like > a buffer overrun on the first and second glance.) It's been a while since I worked on HWP. I think my assumption was that you are inside a nul terminated string, and command line option parsing functions can handle end == NULL, so it would all work out. A too long string crossing " " or ";" would not match. >> + { >> + printk(XENLOG_WARNING >> + "cpufreq/amd-cppc: option '%.*s' not recognized\n", >> + (int)((end ?: e) - s), s); >> + >> + return -EINVAL; >> + } >> + >> + s = end ? ++end : end; > > The increment is odd here (again inherited from the HWP function), as > "end" is about to go out of scope. For HWP, I copied from setup_cpufreq_option() which does similar. You'd prefer: s = end ? end + 1 : NULL; That is more explicit which is good. I considered using NULL back when working on that. I think I when with the current form to match setup_cpufreq_option(). Regards, Jason
[AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, February 11, 2025 8:09 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline > > On 06.02.2025 09:32, Penny Zheng wrote: > > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > > @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void) > > case CPUFREQ_none: > > ret = 0; > > break; > > + default: > > + printk(XENLOG_WARNING "Unsupported cpufreq driver > > + for vendor Intel\n"); > > Same here. The string along overruning line length is fine. But this cal then still be > wrapped as > > printk(XENLOG_WARNING > "Unsupported cpufreq driver for vendor Intel\n"); > > to respect the line length limit as much as possible. > > > @@ -131,6 +131,15 @@ static int __init cf_check setup_cpufreq_option(const > char *str) > > if ( arg[0] && arg[1] ) > > ret = hwp_cmdline_parse(arg + 1, end); > > } > > + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) > > + { > > + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; > > + cpufreq_controller = FREQCTL_xen; > > + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; > > While apparently again a pre-existing problem, the risk of array overrun will become > more manifest with this addition: People may plausibly want to pass a universal > option to Xen on all their instances: > "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq patch, i.e. > before you further extend it. Here you will then further want to bump > cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold option. > Correct me if I'm wrong, We are missing dealing the scenario which looks like the following: "cpufreq=amd-cppc,hwp,verbose". `verbose` is an overrun flag when parsing amd-cppc. I've written the following fix: ``` diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index 860ae32c8a..0fe633d4bc 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -71,6 +71,22 @@ unsigned int __initdata cpufreq_xen_cnt = 1; static int __init cpufreq_cmdline_parse(const char *s, const char *e); +static int __initdata nr_cpufreq_avail_opts = 3; +static const char * __initdata cpufreq_avail_opts[nr_cpufreq_avail_opts] = { "xen", "hwp", "amd-cppc" }; + +static void __init cpufreq_cmdline_trim(const char *s) +{ + unsigned int i = 0; + + do + { + if ( strncmp(s, cpufreq_avail_opts[i], strlen(cpufreq_avail_opts[i] - 1) == 0 ) + return false; + } while ( ++i < nr_cpufreq_avail_opts ) + + return true; +} + static int __init cf_check setup_cpufreq_option(const char *str) { const char *arg = strpbrk(str, ",:;"); @@ -118,7 +134,7 @@ static int __init cf_check setup_cpufreq_option(const char *str) cpufreq_controller = FREQCTL_xen; cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen; ret = 0; - if ( arg[0] && arg[1] ) + if ( arg[0] && arg[1] && cpufreq_cmdline_trim(arg + 1) ) ret = cpufreq_cmdline_parse(arg + 1, end); } else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 && ``` > > Jan Many thanks, Penny
On 17.02.2025 11:17, Penny, Zheng wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, February 11, 2025 8:09 PM >> To: Penny, Zheng <penny.zheng@amd.com> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason >> <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; >> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal >> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- >> devel@lists.xenproject.org >> Subject: Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline >> >> On 06.02.2025 09:32, Penny Zheng wrote: >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void) >>> case CPUFREQ_none: >>> ret = 0; >>> break; >>> + default: >>> + printk(XENLOG_WARNING "Unsupported cpufreq driver >>> + for vendor Intel\n"); >> >> Same here. The string along overruning line length is fine. But this cal then still be >> wrapped as >> >> printk(XENLOG_WARNING >> "Unsupported cpufreq driver for vendor Intel\n"); >> >> to respect the line length limit as much as possible. >> >>> @@ -131,6 +131,15 @@ static int __init cf_check setup_cpufreq_option(const >> char *str) >>> if ( arg[0] && arg[1] ) >>> ret = hwp_cmdline_parse(arg + 1, end); >>> } >>> + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) >>> + { >>> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; >>> + cpufreq_controller = FREQCTL_xen; >>> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; >> >> While apparently again a pre-existing problem, the risk of array overrun will become >> more manifest with this addition: People may plausibly want to pass a universal >> option to Xen on all their instances: >> "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq patch, i.e. >> before you further extend it. Here you will then further want to bump >> cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold option. >> > > Correct me if I'm wrong, We are missing dealing the scenario which looks like the following: > "cpufreq=amd-cppc,hwp,verbose". Not so much this one (can it even overflow). It's "cpufreq=amd-cppc,hwp,xen" that I'm concerned about (or, prior to your change something redundant like "cpufreq=hwp,hwp,xen"). > `verbose` is an overrun flag when parsing amd-cppc. > I've written the following fix: > ``` > diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c > index 860ae32c8a..0fe633d4bc 100644 > --- a/xen/drivers/cpufreq/cpufreq.c > +++ b/xen/drivers/cpufreq/cpufreq.c > @@ -71,6 +71,22 @@ unsigned int __initdata cpufreq_xen_cnt = 1; > > static int __init cpufreq_cmdline_parse(const char *s, const char *e); > > +static int __initdata nr_cpufreq_avail_opts = 3; > +static const char * __initdata cpufreq_avail_opts[nr_cpufreq_avail_opts] = { "xen", "hwp", "amd-cppc" }; Does this even build? If it does, it likely won't be what you mean. You want a constant array dimension (which could actually be omitted, given the initializer), as ... > +static void __init cpufreq_cmdline_trim(const char *s) > +{ > + unsigned int i = 0; > + > + do > + { > + if ( strncmp(s, cpufreq_avail_opts[i], strlen(cpufreq_avail_opts[i] - 1) == 0 ) > + return false; > + } while ( ++i < nr_cpufreq_avail_opts ) ... you can use ARRAY_SIZE() here. (Style note: "do {" goes on a single line.) > + > + return true; > +} > + > static int __init cf_check setup_cpufreq_option(const char *str) > { > const char *arg = strpbrk(str, ",:;"); > @@ -118,7 +134,7 @@ static int __init cf_check setup_cpufreq_option(const char *str) > cpufreq_controller = FREQCTL_xen; > cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen; > ret = 0; > - if ( arg[0] && arg[1] ) > + if ( arg[0] && arg[1] && cpufreq_cmdline_trim(arg + 1) ) > ret = cpufreq_cmdline_parse(arg + 1, end); > } > 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. Jan
[AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, February 17, 2025 6:34 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline > > On 17.02.2025 11:17, Penny, Zheng wrote: > > [AMD Official Use Only - AMD Internal Distribution Only] > > > > Hi, > > > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, February 11, 2025 8:09 PM > >> To: Penny, Zheng <penny.zheng@amd.com> > >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > >> <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > >> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > >> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau > >> Monné <roger.pau@citrix.com>; Stefano Stabellini > >> <sstabellini@kernel.org>; xen- devel@lists.xenproject.org > >> Subject: Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" > >> xen cmdline > >> > >> On 06.02.2025 09:32, Penny Zheng wrote: > >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > >>> @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void) > >>> case CPUFREQ_none: > >>> ret = 0; > >>> break; > >>> + default: > >>> + printk(XENLOG_WARNING "Unsupported cpufreq > >>> + driver for vendor Intel\n"); > >> > >> Same here. The string along overruning line length is fine. But this > >> cal then still be wrapped as > >> > >> printk(XENLOG_WARNING > >> "Unsupported cpufreq driver for vendor > >> Intel\n"); > >> > >> to respect the line length limit as much as possible. > >> > >>> @@ -131,6 +131,15 @@ static int __init cf_check > >>> setup_cpufreq_option(const > >> char *str) > >>> if ( arg[0] && arg[1] ) > >>> ret = hwp_cmdline_parse(arg + 1, end); > >>> } > >>> + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) > >>> + { > >>> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; > >>> + cpufreq_controller = FREQCTL_xen; > >>> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; > >> > >> While apparently again a pre-existing problem, the risk of array > >> overrun will become more manifest with this addition: People may > >> plausibly want to pass a universal option to Xen on all their instances: > >> "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq patch, > i.e. > >> before you further extend it. Here you will then further want to bump > >> cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold > option. > >> > > > > Correct me if I'm wrong, We are missing dealing the scenario which looks like the > following: > > "cpufreq=amd-cppc,hwp,verbose". > > Not so much this one (can it even overflow). It's "cpufreq=amd-cppc,hwp,xen" > that I'm concerned about (or, prior to your change something redundant like > "cpufreq=hwp,hwp,xen"). I misunderstood before, sorry What is the appropriate behavior when user passes an option to Xen, like "cpufreq=amd-cppc,hwp,xen" ? FWIT, amd-cppc and hwp are incompatible options. Send the error info to tell them you shall choose either of them, amd-cppc, or hwp, or xen? If user wants to add fall back scheme, when amd-cppc is hardware unavailable, we fall back to xen. user shall use ";", not "," to add, like "cpufreq=amd-cppc;xen" > > > `verbose` is an overrun flag when parsing amd-cppc. > > I've written the following fix: > > ``` > > diff --git a/xen/drivers/cpufreq/cpufreq.c > > b/xen/drivers/cpufreq/cpufreq.c index 860ae32c8a..0fe633d4bc 100644 > > --- a/xen/drivers/cpufreq/cpufreq.c > > +++ b/xen/drivers/cpufreq/cpufreq.c > > @@ -71,6 +71,22 @@ unsigned int __initdata cpufreq_xen_cnt = 1; > > > > static int __init cpufreq_cmdline_parse(const char *s, const char > > *e); > > > > +static int __initdata nr_cpufreq_avail_opts = 3; static const char * > > +__initdata cpufreq_avail_opts[nr_cpufreq_avail_opts] = { "xen", > > +"hwp", "amd-cppc" }; > > Does this even build? If it does, it likely won't be what you mean. You want a > constant array dimension (which could actually be omitted, given the initializer), > as ... > > > +static void __init cpufreq_cmdline_trim(const char *s) { > > + unsigned int i = 0; > > + > > + do > > + { > > + if ( strncmp(s, cpufreq_avail_opts[i], strlen(cpufreq_avail_opts[i] - 1) == 0 ) > > + return false; > > + } while ( ++i < nr_cpufreq_avail_opts ) > > ... you can use ARRAY_SIZE() here. (Style note: "do {" goes on a single line.) > > > + > > + return true; > > +} > > + > > static int __init cf_check setup_cpufreq_option(const char *str) { > > const char *arg = strpbrk(str, ",:;"); @@ -118,7 +134,7 @@ static > > int __init cf_check setup_cpufreq_option(const char *str) > > cpufreq_controller = FREQCTL_xen; > > cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen; > > ret = 0; > > - if ( arg[0] && arg[1] ) > > + if ( arg[0] && arg[1] && cpufreq_cmdline_trim(arg + 1) ) > > ret = cpufreq_cmdline_parse(arg + 1, end); > > } > > 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. > > Jan
On 18.02.2025 05:24, Penny, Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Monday, February 17, 2025 6:34 PM >> >> On 17.02.2025 11:17, Penny, Zheng wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Tuesday, February 11, 2025 8:09 PM >>>> >>>> On 06.02.2025 09:32, Penny Zheng wrote: >>>>> @@ -131,6 +131,15 @@ static int __init cf_check >>>>> setup_cpufreq_option(const >>>> char *str) >>>>> if ( arg[0] && arg[1] ) >>>>> ret = hwp_cmdline_parse(arg + 1, end); >>>>> } >>>>> + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) >>>>> + { >>>>> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; >>>>> + cpufreq_controller = FREQCTL_xen; >>>>> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; >>>> >>>> While apparently again a pre-existing problem, the risk of array >>>> overrun will become more manifest with this addition: People may >>>> plausibly want to pass a universal option to Xen on all their instances: >>>> "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq patch, >> i.e. >>>> before you further extend it. Here you will then further want to bump >>>> cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold >> option. >>>> >>> >>> Correct me if I'm wrong, We are missing dealing the scenario which looks like the >> following: >>> "cpufreq=amd-cppc,hwp,verbose". >> >> Not so much this one (can it even overflow). It's "cpufreq=amd-cppc,hwp,xen" >> that I'm concerned about (or, prior to your change something redundant like >> "cpufreq=hwp,hwp,xen"). > > I misunderstood before, sorry > What is the appropriate behavior when user passes an option to Xen, like "cpufreq=amd-cppc,hwp,xen" ? > FWIT, amd-cppc and hwp are incompatible options. Sure, but as said people may want to use something like this uniformly on all their systems, be them AMD or Intel ones. IOW ... > Send the error info to tell them you shall choose either of them, amd-cppc, or hwp, or xen? ... no, I don't think this should be an error. > If user wants to add fall back scheme, when amd-cppc is hardware unavailable, we fall back to xen. user shall > use ";", not "," to add, like "cpufreq=amd-cppc;xen" Well, I didn't closely check whether the separator is to be semicolon or comma. Things is that people may want to use one single command line option across all their systems, old or new, Intel or AMD. Hence they may want to ask to use HWP is available, CPPC is available, or fall back to what we have had for ages. Yet they will also need to have a way to express that they want only HWP and CPPC to be tried, without falling back to the legacy driver. Hence we may not automatically fall back to that if "amp-cppc" was passed, but is unavailable. Jan
[AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, February 18, 2025 10:56 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline > > On 18.02.2025 05:24, Penny, Zheng wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Monday, February 17, 2025 6:34 PM > >> > >> On 17.02.2025 11:17, Penny, Zheng wrote: > >>>> -----Original Message----- > >>>> From: Jan Beulich <jbeulich@suse.com> > >>>> Sent: Tuesday, February 11, 2025 8:09 PM > >>>> > >>>> On 06.02.2025 09:32, Penny Zheng wrote: > >>>>> @@ -131,6 +131,15 @@ static int __init cf_check > >>>>> setup_cpufreq_option(const > >>>> char *str) > >>>>> if ( arg[0] && arg[1] ) > >>>>> ret = hwp_cmdline_parse(arg + 1, end); > >>>>> } > >>>>> + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) > >>>>> + { > >>>>> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; > >>>>> + cpufreq_controller = FREQCTL_xen; > >>>>> + cpufreq_xen_opts[cpufreq_xen_cnt++] = > >>>>> + CPUFREQ_amd_cppc; > >>>> > >>>> While apparently again a pre-existing problem, the risk of array > >>>> overrun will become more manifest with this addition: People may > >>>> plausibly want to pass a universal option to Xen on all their instances: > >>>> "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a > >>>> prereq patch, > >> i.e. > >>>> before you further extend it. Here you will then further want to > >>>> bump cpufreq_xen_opts[]'es dimension, to account for the now > >>>> sensible three-fold > >> option. > >>>> > >>> > >>> Correct me if I'm wrong, We are missing dealing the scenario which > >>> looks like the > >> following: > >>> "cpufreq=amd-cppc,hwp,verbose". > >> > >> Not so much this one (can it even overflow). It's "cpufreq=amd-cppc,hwp,xen" > >> that I'm concerned about (or, prior to your change something > >> redundant like "cpufreq=hwp,hwp,xen"). > > > > I misunderstood before, sorry > > What is the appropriate behavior when user passes an option to Xen, like > "cpufreq=amd-cppc,hwp,xen" ? > > FWIT, amd-cppc and hwp are incompatible options. > > Sure, but as said people may want to use something like this uniformly on all their > systems, be them AMD or Intel ones. IOW ... > > > Send the error info to tell them you shall choose either of them, amd-cppc, or hwp, > or xen? > > ... no, I don't think this should be an error. > > > If user wants to add fall back scheme, when amd-cppc is hardware > > unavailable, we fall back to xen. user shall use ";", not "," to add, like > "cpufreq=amd-cppc;xen" > > Well, I didn't closely check whether the separator is to be semicolon or comma. > Things is that people may want to use one single command line option across all > their systems, old or new, Intel or AMD. Hence they may want to ask to use HWP is > available, CPPC is available, or fall back to what we have had for ages. Yet they will > also need to have a way to express that they want only HWP and CPPC to be tried, > without falling back to the legacy driver. Hence we may not automatically fall back to > that if "amp-cppc" was passed, but is unavailable. > Then I suggest we use the semicolon to separate all options user would like to try, but in the priority order, like "cpufreq=hwp;amd-cppc;xen", if hwp and amd-cppc are both tried and found not supported,legacy xen will be considered. If it's only "cpufreq=hwp;amd-cppc", and when hwp and amd-cppc are both not supported, we will not automatically fall back to any. For specific driver, like "amd-cppc", sub-features like active mode will be separated by ",", like "cpufreq=hwp;amd-cppc,active;xen" > Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 9bbd00baef..78cfb8a02e 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -515,7 +515,7 @@ If set, force use of the performance counters for oprofile, rather than detectin available support. ### cpufreq -> `= none | {{ <boolean> | xen } { [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]]` +> `= none | {{ <boolean> | xen } { [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] | amd-cppc[:[verbose]]` > Default: `xen` @@ -526,7 +526,7 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels. * `<maxfreq>` and `<minfreq>` are integers which represent max and min processor frequencies respectively. * `verbose` option can be included as a string or also as `verbose=<integer>` - for `xen`. It is a boolean for `hwp`. + for `xen`. It is a boolean for `hwp` and `amd-cppc`. * `hwp` selects Hardware-Controlled Performance States (HWP) on supported Intel hardware. HWP is a Skylake+ feature which provides better CPU power management. The default is disabled. If `hwp` is selected, but hardware @@ -534,6 +534,10 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels. * `<hdc>` is a boolean to enable Hardware Duty Cycling (HDC). HDC enables the processor to autonomously force physical package components into idle state. The default is enabled, but the option only applies when `hwp` is enabled. +* `amd-cppc` selects ACPI Collaborative Performance and Power Control (CPPC) + on supported AMD hardware to provide finer grained frequency control mechanism. + The default is disabled. If `amd-cppc` is selected, but hardware support + is not available, Xen will fallback to cpufreq=xen. There is also support for `;`-separated fallback options: `cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to `xen` if diff --git a/xen/arch/x86/acpi/cpufreq/Makefile b/xen/arch/x86/acpi/cpufreq/Makefile index e7dbe434a8..a2ba34bda0 100644 --- a/xen/arch/x86/acpi/cpufreq/Makefile +++ b/xen/arch/x86/acpi/cpufreq/Makefile @@ -1,4 +1,5 @@ obj-$(CONFIG_INTEL) += acpi.o +obj-$(CONFIG_AMD) += amd-cppc.o obj-y += cpufreq.o obj-$(CONFIG_INTEL) += hwp.o obj-$(CONFIG_AMD) += powernow.o diff --git a/xen/arch/x86/acpi/cpufreq/amd-cppc.c b/xen/arch/x86/acpi/cpufreq/amd-cppc.c new file mode 100644 index 0000000000..2dca4a00f3 --- /dev/null +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * amd-cppc.c - AMD Processor CPPC Frequency Driver + * + * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved. + * + * Author: Penny Zheng <penny.zheng@amd.com> + * + * AMD CPPC cpufreq driver introduces a new CPU performance scaling design + * for AMD processors using the ACPI Collaborative Performance and Power + * Control (CPPC) feature which provides finer grained frequency control range. + */ + +#include <xen/init.h> +#include <xen/param.h> +#include <acpi/cpufreq/cpufreq.h> + +static bool __init amd_cppc_handle_option(const char *s, const char *end) +{ + int ret; + + ret = parse_boolean("verbose", s, end); + if ( ret >= 0 ) + { + cpufreq_verbose = ret; + return true; + } + + return false; +} + +int __init amd_cppc_cmdline_parse(const char *s, const char *e) +{ + do + { + const char *end = strpbrk(s, ",;"); + + if ( !amd_cppc_handle_option(s, end) ) + { + printk(XENLOG_WARNING + "cpufreq/amd-cppc: option '%.*s' not recognized\n", + (int)((end ?: e) - s), s); + + return -EINVAL; + } + + s = end ? ++end : end; + } while ( s && s < e ); + + return 0; +} + +static const struct cpufreq_driver __initconst_cf_clobber amd_cppc_cpufreq_driver = +{ + .name = XEN_AMD_CPPC_DRIVER_NAME, +}; + +int __init amd_cppc_register_driver(void) +{ + if ( !cpu_has_cppc ) + return -ENODEV; + + return cpufreq_register_driver(&amd_cppc_cpufreq_driver); +} diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c index 61e98b67bd..4bcaca1a01 100644 --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void) case CPUFREQ_none: ret = 0; break; + default: + printk(XENLOG_WARNING "Unsupported cpufreq driver for vendor Intel\n"); + break; } if ( ret != -ENODEV ) @@ -157,7 +160,34 @@ 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; + + for ( unsigned int 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\n"); + break; + } + + if ( ret != -ENODEV ) + break; + } break; } } diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c index 735c71b0e7..3d10827930 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -573,6 +573,12 @@ ret_t do_platform_op( } case XEN_PM_CPPC: + if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) ) + { + ret = -EOPNOTSUPP; + break; + } + ret = set_cppc_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.cppc_data); break; diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index f5e8bfa09e..c0c6dc4c42 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -85,7 +85,7 @@ static int __init cf_check setup_cpufreq_option(const char *str) if ( choice < 0 && !cmdline_strcmp(str, "dom0-kernel") ) { - xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX; + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_PX | XEN_PROCESSOR_PM_CPPC); cpufreq_controller = FREQCTL_dom0_kernel; opt_dom0_vcpus_pin = 1; return 0; @@ -93,7 +93,7 @@ static int __init cf_check setup_cpufreq_option(const char *str) if ( choice == 0 || !cmdline_strcmp(str, "none") ) { - xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX; + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_PX | XEN_PROCESSOR_PM_CPPC); cpufreq_controller = FREQCTL_none; return 0; } @@ -131,6 +131,15 @@ static int __init cf_check setup_cpufreq_option(const char *str) if ( arg[0] && arg[1] ) ret = hwp_cmdline_parse(arg + 1, end); } + else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") ) + { + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; + cpufreq_controller = FREQCTL_xen; + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc; + ret = 0; + if ( arg[0] && arg[1] ) + ret = amd_cppc_cmdline_parse(arg + 1, end); + } else ret = -EINVAL; diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h index 3f1b05a02e..a6fb10ea27 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -28,6 +28,7 @@ enum cpufreq_xen_opt { CPUFREQ_none, CPUFREQ_xen, CPUFREQ_hwp, + CPUFREQ_amd_cppc, }; extern enum cpufreq_xen_opt cpufreq_xen_opts[2]; extern unsigned int cpufreq_xen_cnt; @@ -267,4 +268,7 @@ int set_hwp_para(struct cpufreq_policy *policy, int acpi_cpufreq_register(void); +int amd_cppc_cmdline_parse(const char *s, const char *e); +int amd_cppc_register_driver(void); + #endif /* __XEN_CPUFREQ_PM_H__ */ diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h index b8daa8fc42..11d30c894e 100644 --- a/xen/include/public/platform.h +++ b/xen/include/public/platform.h @@ -357,6 +357,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t); #define XEN_PROCESSOR_PM_CX 1 #define XEN_PROCESSOR_PM_PX 2 #define XEN_PROCESSOR_PM_TX 4 +#define XEN_PROCESSOR_PM_CPPC 8 /* cmd type */ #define XEN_PM_CX 0 diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index b0fec271d3..42997252ef 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -423,6 +423,7 @@ struct xen_set_cppc_para { uint32_t activity_window; }; +#define XEN_AMD_CPPC_DRIVER_NAME "amd-cppc" #define XEN_HWP_DRIVER_NAME "hwp" /*
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. Actual implmentation will be introduced in the following commits. Signed-off-by: Penny Zheng <Penny.Zheng@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 --- docs/misc/xen-command-line.pandoc | 8 +++- xen/arch/x86/acpi/cpufreq/Makefile | 1 + xen/arch/x86/acpi/cpufreq/amd-cppc.c | 64 ++++++++++++++++++++++++++++ xen/arch/x86/acpi/cpufreq/cpufreq.c | 32 +++++++++++++- xen/arch/x86/platform_hypercall.c | 6 +++ xen/drivers/cpufreq/cpufreq.c | 13 +++++- xen/include/acpi/cpufreq/cpufreq.h | 4 ++ xen/include/public/platform.h | 1 + xen/include/public/sysctl.h | 1 + 9 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 xen/arch/x86/acpi/cpufreq/amd-cppc.c