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

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, January 9, 2025 5:59 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Ragiadakou, Xenia <Xenia.Ragiadakou@xxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 03/11] xen/x86: introduce "cpufreq=amd-pstate" xen
> cmdline
>
> 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.

Sure, and I'll also strict it with CONFIG_AMD

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

Ack

>
> > +#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
>

Ack

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

Understood

> > @@ -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?
>

Googling the Hygon cpu, as it is based on Zen serie and also amd-pstate
feature is developed for Zen serie for the first place, the new switch-case 
code shall
apply to Hygon CPUs too

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

I've mirrored the return value, so maybe -EINVAL is better?

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

Ack

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

Okay. I'll change all amd_/-pstate defined values to amd_/-cppc

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

Ack

> Jan

Mant thanks
Penny