[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, January 9, 2025 5:59 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Stabellini, Stefano <stefano.stabell...@amd.com>; Huang, Ray
> <ray.hu...@amd.com>; Ragiadakou, Xenia <xenia.ragiada...@amd.com>;
> Andryuk, Jason <jason.andr...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
> <sstabell...@kernel.org>; Roger Pau Monné <roger....@citrix.com>; xen-
> de...@lists.xenproject.org
> 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.zh...@amd.com>
> > + *
> > + * 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
