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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2025 09:32, Penny Zheng wrote:
> --- /dev/null
> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * amd-cppc.c - AMD Processor CPPC Frequency Driver
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * Author: Penny Zheng <penny.zheng@xxxxxxx>
> + *
> + * AMD CPPC cpufreq driver introduces a new CPU performance scaling design
> + * for AMD processors using the ACPI Collaborative Performance and Power
> + * Control (CPPC) feature which provides finer grained frequency control 
> range.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/param.h>
> +#include <acpi/cpufreq/cpufreq.h>
> +
> +static bool __init amd_cppc_handle_option(const char *s, const char *end)
> +{
> +    int ret;
> +
> +    ret = parse_boolean("verbose", s, end);
> +    if ( ret >= 0 )
> +    {
> +        cpufreq_verbose = ret;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +int __init amd_cppc_cmdline_parse(const char *s, const char *e)
> +{
> +    do
> +    {
> +        const char *end = strpbrk(s, ",;");
> +
> +        if ( !amd_cppc_handle_option(s, end) )

You have an incoming "e" here. What if the comma / semicolon you find
is past where that points? (I understand you've copied that from
hwp_cmdline_parse(), and should have raised the question already there
when reviewing the respective patch. It also looks as if behavior-
wise all would be okay here. It's just that this very much looks like
a buffer overrun on the first and second glance.)

> +        {
> +            printk(XENLOG_WARNING
> +                   "cpufreq/amd-cppc: option '%.*s' not recognized\n",
> +                   (int)((end ?: e) - s), s);
> +
> +            return -EINVAL;
> +        }
> +
> +        s = end ? ++end : end;

The increment is odd here (again inherited from the HWP function), as
"end" is about to go out of scope.

> +    } while ( s && s < e );
> +
> +    return 0;
> +}
> +
> +static const struct cpufreq_driver __initconst_cf_clobber 
> amd_cppc_cpufreq_driver =

Once again too long a line.

> --- 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.

I'm also missing IS_ENABLED(CONFIG_AMD) here. The counterpart thereof is
present for the earlier HWP alternative.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -357,6 +357,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
>  #define XEN_PROCESSOR_PM_CX  1
>  #define XEN_PROCESSOR_PM_PX  2
>  #define XEN_PROCESSOR_PM_TX  4
> +#define XEN_PROCESSOR_PM_CPPC        8

Hmm, seeing this addition: Why do all of these live in a public header?
They're used to set xen_processor_bits only, which is a Xen-internal
variable only. With apparently one exception: PV Dom0 is passed these
bits in si->flags. Does Dom0 have use for this new bit? If not it may
want assigning a value such that it falls outside of SIF_PM_MASK (and
then in a non-public header).

Jan