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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 11:17, Penny, Zheng wrote:
> [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".

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