RE: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, February 11, 2025 8:09 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> <Jason.Andryuk@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 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