Re: [PATCH v5 06/18] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2025 10:48, Penny Zheng wrote:
> Users need to set "cpufreq=amd-cppc" in xen cmdline to enable
> amd-cppc driver, which selects ACPI Collaborative Performance
> and Power Control (CPPC) on supported AMD hardware to provide a
> finer grained frequency control mechanism.
> `verbose` option can also be included to support verbose print.
> 
> When users setting "cpufreq=amd-cppc", a new amd-cppc driver
> shall be registered and used. All hooks for amd-cppc driver are transiently
> missing and will be implemented in the ongoing commits.
> 
> New xen-pm internal flag XEN_PROCESSOR_PM_CPPC is introduced, to be
> differentiated with legacy XEN_PROCESSOR_PM_PX. We define
> XEN_PROCESSOR_PM_CPPC 0x100, as it is the next value to use after 8-bits wide
> public xen-pm options. All xen-pm flag checking involving XEN_PROCESSOR_PM_PX
> shall also be updated to consider XEN_PROCESSOR_PM_CPPC now.
> 
> Xen is not expected to support both or mixed mode (CPPC & legacy PSS, _PCT,
> _PPC) operations, so only one cpufreq driver gets registerd, either amd-cppc
> or legacy P-states driver, which is reflected and asserted by the incompatible
> flags XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v1 -> v2:
> - Obey to alphabetic sorting and also strict it with CONFIG_AMD
> - Remove unnecessary empty comment line
> - Use __initconst_cf_clobber for pre-filled structure cpufreq_driver
> - Make new switch-case code apply to Hygon CPUs too
> - Change ENOSYS with EOPNOTSUPP
> - Blanks around binary operator
> - Change all amd_/-pstate defined values to amd_/-cppc
> ---
> v2 -> v3
> - refactor too long lines
> - Make sure XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC incompatible flags
> after cpufreq register registrantion
> ---
> v3 -> v4:
> - introduce XEN_PROCESSOR_PM_CPPC in xen internal header
> - complement "Hygon" in log message
> - remove unnecessary if()
> - grow cpufreq_xen_opts[] array
> ---
> v4 -> v5:
> - remove XEN_PROCESSOR_PM_xxx flag sanitization from individual driver
> - prefer ! over "== 0" in purely boolean contexts
> - Blank line between non-fall-through case blocks
> - add build-time checking between internal and public XEN_PROCESSOR_PM_*
> values
> - define XEN_PROCESSOR_PM_CPPC with 0x100, as it is the next value to use
> after public interface, and public mask SIF_PM_MASK is 8 bits wide.
> - as Dom0 will send the CPPC/Px data whenever it could, the return value shall
> be 0 instead of -ENOSYS/EOPNOTSUP when platform doesn't require these data.
> ---
>  docs/misc/xen-command-line.pandoc         |  7 ++-
>  xen/arch/x86/acpi/cpufreq/Makefile        |  1 +
>  xen/arch/x86/acpi/cpufreq/amd-cppc.c      | 68 +++++++++++++++++++++++
>  xen/arch/x86/acpi/cpufreq/cpufreq.c       | 63 ++++++++++++++++++++-
>  xen/arch/x86/platform_hypercall.c         | 13 ++++-
>  xen/drivers/acpi/pmstat.c                 |  3 +-
>  xen/drivers/cpufreq/cpufreq.c             | 18 +++++-
>  xen/include/acpi/cpufreq/cpufreq.h        |  6 +-
>  xen/include/acpi/cpufreq/processor_perf.h |  3 +
>  xen/include/public/sysctl.h               |  1 +
>  10 files changed, 175 insertions(+), 8 deletions(-)
>  create mode 100644 xen/arch/x86/acpi/cpufreq/amd-cppc.c

This patch does quite a bit more than what the subject suggests it means to
do; please consider adjusting.

> +int __init amd_cppc_register_driver(void)
> +{
> +    if ( !cpu_has_cppc )
> +    {
> +        xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> +        return -ENODEV;
> +    }
> +
> +    return cpufreq_register_driver(&amd_cppc_cpufreq_driver);

Isn't this premature? I fear in particular that some of the hooks, which
are still all NULL, might have a way of getting invoked.

> @@ -157,7 +162,63 @@ static int __init cf_check cpufreq_driver_init(void)
>  
>          case X86_VENDOR_AMD:
>          case X86_VENDOR_HYGON:
> -            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : 
> -ENODEV;
> +            unsigned int i;
> +
> +            if ( !IS_ENABLED(CONFIG_AMD) )
> +            {
> +                ret = -ENODEV;
> +                break;
> +            }
> +            ret = -ENOENT;
> +
> +            for ( i = 0; i < cpufreq_xen_cnt; i++ )
> +            {
> +                switch ( cpufreq_xen_opts[i] )
> +                {
> +                case CPUFREQ_xen:
> +                    ret = powernow_register_driver();
> +                    break;
> +
> +                case CPUFREQ_amd_cppc:
> +                    ret = amd_cppc_register_driver();
> +                    break;
> +
> +                case CPUFREQ_none:
> +                    ret = 0;
> +                    break;
> +
> +                default:
> +                    printk(XENLOG_WARNING
> +                           "Unsupported cpufreq driver for vendor AMD or 
> Hygon\n");
> +                    break;
> +                }
> +
> +                if ( ret != -ENODEV )
> +                    break;

This, I think, needs some commenting. It's not quite clear why we shouldn't
try the next option if registration failed with other than -ENODEV.

> +            }
> +
> +            /*
> +             * After cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC
> +             * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> +             */
> +            if ( !ret && i < cpufreq_xen_cnt )
> +            {
> +                switch ( cpufreq_xen_opts[i] )
> +                {
> +                case CPUFREQ_amd_cppc:
> +                    xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
> +                    break;
> +
> +                case CPUFREQ_xen:
> +                    xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> +                    break;
> +
> +                case CPUFREQ_none:
> +                default:

What's the point of the separate "case" label here?

> +                    break;
> +                }
> +            }

Why does this pruning of xen_processor_pmbits sit in the AMD-only code path?
Iirc earlier on you had it in the *_register_driver() themselves, and my
request was to put it in a central, generic place. It's central now, but not
generic (and this way it could as well have remained in *_register_driver()).

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -539,9 +539,12 @@ ret_t do_platform_op(
>          case XEN_PM_PX:
>              if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
>              {
> -                ret = -ENOSYS;
> +                ret = 0;
>                  break;
>              }
> +            /* Xen doesn't support mixed mode */
> +            ASSERT(!(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC));
> +
>              ret = set_px_pminfo(op->u.set_pminfo.id, 
> &op->u.set_pminfo.u.perf);
>              break;

I don't see how this change relates to the purpose of the patch. From the
description (and in the absence of a code comment) it also doesn't become
clear at all why that change of return value would be needed (and would be
correct to do). From comments I gave on earlier versions of this series, I
think I can guess what this is about, but such shouldn't be slipped in
silently.

> @@ -573,6 +576,14 @@ ret_t do_platform_op(
>          }
>  
>          case XEN_PM_CPPC:
> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) )
> +            {
> +                ret = 0;
> +                break;
> +            }

As per above, this yielding success needs justifying.

> @@ -94,6 +95,8 @@ static int __init handle_cpufreq_cmdline(enum 
> cpufreq_xen_opt option)
>  {
>      int ret = 0;
>  
> +    /* Do not occupy bits reserved for public xen-pm */
> +    BUILD_BUG_ON(MASK_INSR(XEN_PROCESSOR_PM_CPPC, SIF_PM_MASK));

This looks like an abuse of MASK_INSR(). Why not simply

    BUILD_BUG_ON(XEN_PROCESSOR_PM_CPPC & SIF_PM_MASK);

?

Also please separate this by a blank line from what follows, or perhaps
even better ...

> @@ -105,6 +108,10 @@ static int __init handle_cpufreq_cmdline(enum 
> cpufreq_xen_opt option)
>      cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
>      switch ( option )
>      {
> +    case CPUFREQ_amd_cppc:
> +        xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC;
> +        break;

... move it here.

> @@ -172,6 +179,13 @@ 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 ( IS_ENABLED(CONFIG_AMD) && choice < 0 &&
> +                  !cmdline_strcmp(str, "amd-cppc") )
> +        {
> +            ret = handle_cpufreq_cmdline(CPUFREQ_amd_cppc);
> +            if ( arg[0] && arg[1] )
> +                ret = amd_cppc_cmdline_parse(arg + 1, end);
> +        }

See Jason's comment on the earlier patch.

> --- a/xen/include/acpi/cpufreq/processor_perf.h
> +++ b/xen/include/acpi/cpufreq/processor_perf.h
> @@ -5,6 +5,9 @@
>  #include <public/sysctl.h>
>  #include <xen/acpi.h>
>  
> +/* Internal ability bits */
> +#define XEN_PROCESSOR_PM_CPPC   0x100

The comment wants extending, to have a reference to the other XEN_PROCESSOR_PM_*
bits and perhaps also to SIF_PM_MASK. In fact the BUILD_BUG_ON() that I 
commented
on above could be replaced by

#if XEN_PROCESSOR_PM_CPPC & SIF_PM_MASK
# error "..."
#endif

right here.

Jan