[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, February 17, 2025 6:34 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andryuk, Jason
> <jason.andr...@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 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 <jbeul...@suse.com>
> >> Sent: Tuesday, February 11, 2025 8:09 PM
> >> To: Penny, Zheng <penny.zh...@amd.com>
> >> Cc: Huang, Ray <ray.hu...@amd.com>; Andryuk, Jason
> >> <jason.andr...@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 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
