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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 09:11, Penny Zheng wrote:
> --- a/xen/arch/x86/acpi/cpufreq/Makefile
> +++ b/xen/arch/x86/acpi/cpufreq/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_INTEL) += acpi.o
>  obj-y += cpufreq.o
> +obj-y += amd-pstate.o
>  obj-$(CONFIG_INTEL) += hwp.o
>  obj-$(CONFIG_AMD) += powernow.o

Please obey to alphabetic sorting.

> --- /dev/null
> +++ b/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * amd-pstate.c - AMD Processor P-state Frequency Driver
> + *
> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * Author: Penny Zheng <penny.zheng@xxxxxxx>
> + *
> + * AMD P-State introduces a new CPU performance scaling design for AMD
> + * processors using the ACPI Collaborative Performance and Power Control 
> (CPPC)
> + * feature which provides a finer grained frequency control range.
> + *
> + */

Nit: Unnecessary empty comment line at the end.

> +#include <xen/init.h>
> +#include <xen/param.h>
> +#include <acpi/cpufreq/cpufreq.h>
> +
> +uint16_t __read_mostly dmi_max_speed_mhz;
> +
> +static bool __init amd_pstate_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_pstate_cmdline_parse(const char *s, const char *e)
> +{
> +    do
> +    {
> +        const char *end = strpbrk(s, ",;");
> +
> +        if ( !amd_pstate_handle_option(s, end) )
> +        {
> +            printk(XENLOG_WARNING "cpufreq/amd-pstate: option '%.*s' not 
> recognized\n",
> +                   (int)((end ?: e) - s), s);
> +
> +            return -EINVAL;
> +        }
> +
> +        s = end ? ++end : end;
> +    } while ( s && s < e );
> +
> +    return 0;
> +}
> +
> +static const struct cpufreq_driver __initconstrel amd_pstate_cpufreq_driver =

__initconst_cf_clobber

> --- 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");

Too long line (the format string itself shall be kept all on one line, but
the XENLOG_WARNING doesn't need to).

> @@ -156,6 +159,31 @@ static int __init cf_check cpufreq_driver_init(void)
>              break;
>  
>          case X86_VENDOR_AMD:
> +            ret = -ENOENT;
> +
> +            for ( unsigned int i = 0; i < cpufreq_xen_cnt; i++ )
> +            {
> +                switch ( cpufreq_xen_opts[i] )
> +                {
> +                case CPUFREQ_xen:
> +                    ret = powernow_register_driver();
> +                    break;
> +                case CPUFREQ_amd_pstate:
> +                    ret = amd_pstate_register_driver();
> +                    break;
> +                case CPUFREQ_none:
> +                    ret = 0;
> +                    break;
> +                default:
> +                    printk(XENLOG_WARNING "Unsupported cpufreq driver for 
> vendor AMD\n");
> +                    break;
> +                }
> +
> +                if ( ret != -ENODEV )
> +                    break;
> +            }
> +            break;
> +
>          case X86_VENDOR_HYGON:
>              ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : 
> -ENODEV;
>              break;

Is the code to handle CPPC not applicable to Hygon CPUs?

What about the IS_ENABLED(CONFIG_AMD) that the original code had? Don't
you need to mirror this in some way?

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -574,6 +574,12 @@ ret_t do_platform_op(
>  
>          case XEN_PM_CPPC:
>          {
> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) )
> +            {
> +                ret = -ENOSYS;

ENOSYS isn't appropriate for such a situation.

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -84,7 +84,7 @@ static int __init cf_check setup_cpufreq_option(const char 
> *str)
>  
>      if ( choice < 0 && !cmdline_strcmp(str, "dom0-kernel") )
>      {
> -        xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
> +        xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_PX|XEN_PROCESSOR_PM_CPPC);
>          cpufreq_controller = FREQCTL_dom0_kernel;
>          opt_dom0_vcpus_pin = 1;
>          return 0;
> @@ -92,7 +92,7 @@ static int __init cf_check setup_cpufreq_option(const char 
> *str)
>  
>      if ( choice == 0 || !cmdline_strcmp(str, "none") )
>      {
> -        xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
> +        xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_PX|XEN_PROCESSOR_PM_CPPC);

Nit (style): Blanks please around binary operators.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -28,6 +28,7 @@ enum cpufreq_xen_opt {
>      CPUFREQ_none,
>      CPUFREQ_xen,
>      CPUFREQ_hwp,
> +    CPUFREQ_amd_pstate,

Might this better be CPUFREQ_amd_cppc? "pstate" may mean various methods.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -424,6 +424,7 @@ struct xen_set_cppc_para {
>  };
>  
>  #define XEN_HWP_DRIVER_NAME "hwp"
> +#define XEN_AMD_PSTATE_DRIVER_NAME "amd-pstate"

Similarly here.

Jan