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