[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". `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
