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 <[email protected]> > + * > + * 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
