RE: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

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

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, May 12, 2025 11:58 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> On 07.05.2025 05:18, Penny, Zheng wrote:
> > [Public]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Tuesday, April 29, 2025 7:47 PM
> >> To: Penny, Zheng <penny.zheng@xxxxxxx>
> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
> >>
> >> On 29.04.2025 12:36, Jan Beulich wrote:
> >>> On 14.04.2025 09:40, Penny Zheng wrote:
> >>>> --- a/xen/drivers/cpufreq/cpufreq.c
> >>>> +++ b/xen/drivers/cpufreq/cpufreq.c
> >>>> @@ -71,6 +71,49 @@ unsigned int __initdata cpufreq_xen_cnt = 1;
> >>>>
> >>>>  static int __init cpufreq_cmdline_parse(const char *s, const char
> >>>> *e);
> >>>>
> >>>> +static bool __init cpufreq_opts_contain(enum cpufreq_xen_opt
> >>>> +option) {
> >>>> +    unsigned int count = cpufreq_xen_cnt;
> >>>> +
> >>>> +    while ( count )
> >>>> +    {
> >>>> +        if ( cpufreq_xen_opts[--count] == option )
> >>>> +            return true;
> >>>> +    }
> >>>> +
> >>>> +    return false;
> >>>> +}
> >>>> +
> >>>> +static int __init handle_cpufreq_cmdline(enum cpufreq_xen_opt
> >>>> +option) {
> >>>> +    int ret = 0;
> >>>> +
> >>>> +    if ( cpufreq_opts_contain(option) )
> >>>> +    {
> >>>> +        const char *cpufreq_opts_str[] = { "CPUFREQ_xen",
> >>>> + "CPUFREQ_hwp" };
> >>>
> >>>         const char *const __initconstrel cpufreq_opts_str[] = {
> >>> "CPUFREQ_xen", "CPUFREQ_hwp" };
> >>>
> >>> (line wrapped suitably, of course) Or maybe even better
> >>>
> >>>         const char __initconst cpufreq_opts_str[][12] = {
> >>> "CPUFREQ_xen", "CPUFREQ_hwp" };
> >>>
> >>> for the string literals to also end up in .init.rodata.
> >>
> >> Actually, it didn't even occur to me that there might be a "static" 
> >> missing here,
> too.
> >
> > Sorry, I may need more explanation, what is the "static" missing you are 
> > referring?
>
> In your code cpufreq_opts_str[] is an automatic variable, which the compiler 
> needs
> to emit code for in order to instantiate it on the stack. This can be avoided 
> if you
> make the array a static variable, as then all construction occurs at build 
> time.
>
> >> Plus I'm missing any arrangement for the array slots to remain in
> >> sync with the corresponding enumerators. With that ...
> >>
> >
> > Thanks for the detailed instructions, learned and I'll change it to
> >         const char __initconst cpufreq_opts_str[][4] = { "xen", "hwp"
> > }; And for in sync with the corresponding enumerators, maybe we shall add
> comment here,
> >         /* The order of cpufreq string literal must be in sync with
> > the order in "enum cpufreq_xen_opt" */
>
> Instead of a comment I has rather hoping for some use of dedicated array slot
> initializers.

Understood. I'll use "CPUFREQ_xxx" as array slot index.
        static const char __initconst cpufreq_opts_str[][5] = {
                [CPUFREQ_none] = "none",
                [CPUFREQ_xen] = "xen",
                [CPUFREQ_hwp] = "hwp",
        };

>
> Jan